From 2d9fda6b31bc405ddace566aba650ff79ebe061e Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Apr 2020 15:00:05 +1000 Subject: [PATCH 1/6] Fix deprecated args validation Fixes check for deprecated arguments to cater for key=value arugments. It also logs the warning if a deprecated flag is used. --- internal/deprecatedargs/deprecatedargs.go | 7 ++++++- internal/deprecatedargs/deprecatedargs_test.go | 2 ++ main.go | 8 ++++---- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/internal/deprecatedargs/deprecatedargs.go b/internal/deprecatedargs/deprecatedargs.go index 56219e475..9bc5a0496 100644 --- a/internal/deprecatedargs/deprecatedargs.go +++ b/internal/deprecatedargs/deprecatedargs.go @@ -13,7 +13,12 @@ func Validate(args []string) error { argMap := make(map[string]bool) for _, arg := range args { - argMap[arg] = true + keyValue := strings.Split(arg, "=") + if len(keyValue) >= 1 { + argMap[keyValue[0]] = true + } else { + argMap[arg] = true + } } for _, deprecatedArg := range deprecatedArgs { diff --git a/internal/deprecatedargs/deprecatedargs_test.go b/internal/deprecatedargs/deprecatedargs_test.go index b1ebeb41f..d301ec3b9 100644 --- a/internal/deprecatedargs/deprecatedargs_test.go +++ b/internal/deprecatedargs/deprecatedargs_test.go @@ -22,6 +22,8 @@ func TestInvalidParms(t *testing.T) { "Auth secret passed": []string{"gitlab-pages", "-auth-secret", "abc123"}, "Sentry DSN passed": []string{"gitlab-pages", "-sentry-dsn", "abc123"}, "Multiple keys passed": []string{"gitlab-pages", "-auth-client-id", "abc123", "-auth-client-secret", "abc123"}, + "key=value": []string{"gitlab-pages", "-auth-client-id=abc123"}, + "multiple key=value": []string{"gitlab-pages", "-auth-client-id=abc123", "-auth-client-secret=abc123"}, } for name, args := range tests { diff --git a/main.go b/main.go index 40aef8134..d550b387d 100644 --- a/main.go +++ b/main.go @@ -58,15 +58,15 @@ var ( _ = flag.String("admin-https-listener", "", "DEPRECATED") _ = flag.String("admin-https-cert", "", "DEPRECATED") _ = flag.String("admin-https-key", "", "DEPRECATED") - secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long.") + secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long; will be deprecated soon") gitLabAuthServer = flag.String("auth-server", "", "DEPRECATED, use gitlab-server instead. GitLab server, for example https://www.gitlab.com") gitLabServer = flag.String("gitlab-server", "", "GitLab server, for example https://www.gitlab.com") internalGitLabServer = flag.String("internal-gitlab-server", "", "Internal GitLab server used for API requests, useful if you want to send that traffic over an internal load balancer, example value https://www.gitlab.com (defaults to value of gitlab-server)") gitLabAPISecretKey = flag.String("api-secret-key", "", "File with secret key used to authenticate with the GitLab API") gitlabClientHTTPTimeout = flag.Duration("gitlab-client-http-timeout", 10*time.Second, "GitLab API HTTP client connection timeout in seconds (default: 10s)") gitlabClientJWTExpiry = flag.Duration("gitlab-client-jwt-expiry", 30*time.Second, "JWT Token expiry time in seconds (default: 30s)") - clientID = flag.String("auth-client-id", "", "GitLab application Client ID") - clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret") + clientID = flag.String("auth-client-id", "", "GitLab application Client ID; will be deprecated soon") + clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret; will be deprecated soon") redirectURI = flag.String("auth-redirect-uri", "", "GitLab application redirect URI") maxConns = flag.Uint("max-conns", 5000, "Limit on the number of concurrent connections to the HTTP, HTTPS or proxy listeners") insecureCiphers = flag.Bool("insecure-ciphers", false, "Use default list of cipher suites, may contain insecure ones like 3DES and RC4") @@ -241,7 +241,7 @@ func initErrorReporting(sentryDSN, sentryEnvironment string) { func loadConfig() appConfig { if err := deprecatedargs.Validate(os.Args[1:]); err != nil { - log.WithError(err) + log.WithError(err).Warn("Using deprecated arguments") } config := configFromFlags() -- GitLab From cf03e89ed1b63f763dab88b60d6e9148e2f70b19 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Tue, 21 Apr 2020 17:00:21 +1000 Subject: [PATCH 2/6] Enforce loading secrets from file Passing secrets via command line is not allowed anymore. A config file should be used instead. The default filename is `gitlab-pages-config`. The following command line options will throw an error and prevent pages from running if set explicitly: - `-auth-client-id` - `-auth-client-secret` - `-auth-secret` --- .gitignore | 1 + acceptance_test.go | 18 +++++-- helpers_test.go | 49 +++++++++++++------ internal/deprecatedargs/deprecatedargs.go | 34 ------------- internal/validateargs/validateargs.go | 44 +++++++++++++++++ .../validateargs_test.go} | 22 +++++++-- main.go | 15 ++++-- 7 files changed, 120 insertions(+), 63 deletions(-) delete mode 100644 internal/deprecatedargs/deprecatedargs.go create mode 100644 internal/validateargs/validateargs.go rename internal/{deprecatedargs/deprecatedargs_test.go => validateargs/validateargs_test.go} (70%) diff --git a/.gitignore b/.gitignore index db328895f..7c50688bf 100644 --- a/.gitignore +++ b/.gitignore @@ -2,6 +2,7 @@ shared/pages/.update /gitlab-pages /vendor +/gitlab-pages-config # Used by the makefile /.GOPATH diff --git a/acceptance_test.go b/acceptance_test.go index 79a9b2759..da27473c3 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -4,6 +4,7 @@ import ( "crypto/tls" "fmt" "io/ioutil" + "log" "mime" "net" "net/http" @@ -20,6 +21,7 @@ import ( ) var pagesBinary = flag.String("gitlab-pages-binary", "./gitlab-pages", "Path to the gitlab-pages binary") +var accessControlConfigFile string // TODO: Use TCP port 0 everywhere to avoid conflicts. The binary could output // the actual port (and type of listener) for us to read in place of the @@ -66,6 +68,16 @@ func skipUnlessEnabled(t *testing.T, conditions ...string) { } } +func TestMain(m *testing.M) { + var err error + accessControlConfigFile, err = accessControlConfig("clientID", "clientSecret", "authSecret") + if err != nil { + log.Fatal(err) + } + defer os.Remove(accessControlConfigFile) + + os.Exit(m.Run()) +} func TestUnknownHostReturnsNotFound(t *testing.T) { skipUnlessEnabled(t) teardown := RunPagesProcess(t, *pagesBinary, listeners, "") @@ -682,12 +694,10 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { listeners, "", certFile, + "-config="+accessControlConfigFile, "-artifacts-server="+artifactServerURL, - "-auth-client-id=1", - "-auth-client-secret=1", "-auth-server="+testServer.URL, "-auth-redirect-uri=https://projects.gitlab-example.com/auth", - "-auth-secret=something-very-secret", tt.binaryOption, ) defer teardown() @@ -856,7 +866,7 @@ func TestWhenAuthIsEnabledPrivateWillRedirectToAuthorize(t *testing.T) { require.Equal(t, "https", url.Scheme) require.Equal(t, "gitlab-auth.com", url.Host) require.Equal(t, "/oauth/authorize", url.Path) - require.Equal(t, "1", url.Query().Get("client_id")) + require.Equal(t, "clientID", url.Query().Get("client_id")) require.Equal(t, "https://projects.gitlab-example.com/auth", url.Query().Get("redirect_uri")) require.NotEqual(t, "", url.Query().Get("state")) } diff --git a/helpers_test.go b/helpers_test.go index 29ed87de8..e6a8d03e4 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -183,28 +183,25 @@ func RunPagesProcessWithEnvs(t *testing.T, wait bool, pagesPath string, listener return runPagesProcess(t, wait, pagesPath, listeners, promPort, envs, extraArgs...) } -func RunPagesProcessWithAuth(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string) (teardown func()) { - return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, "-auth-client-id=1", - "-auth-client-secret=1", +func RunPagesProcessWithAuth(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string) func() { + return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, + "-config="+accessControlConfigFile, "-auth-server=https://gitlab-auth.com", - "-auth-redirect-uri=https://projects.gitlab-example.com/auth", - "-auth-secret=something-very-secret") + "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } -func RunPagesProcessWithAuthServer(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, authServer string) (teardown func()) { - return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, "-auth-client-id=1", - "-auth-client-secret=1", +func RunPagesProcessWithAuthServer(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, authServer string) func() { + return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, + "-config="+accessControlConfigFile, "-auth-server="+authServer, - "-auth-redirect-uri=https://projects.gitlab-example.com/auth", - "-auth-secret=something-very-secret") + "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } -func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, sslCertFile string, authServer string) (teardown func()) { - return runPagesProcess(t, true, pagesPath, listeners, promPort, []string{"SSL_CERT_FILE=" + sslCertFile}, "-auth-client-id=1", - "-auth-client-secret=1", +func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, sslCertFile string, authServer string) func() { + return runPagesProcess(t, true, pagesPath, listeners, promPort, []string{"SSL_CERT_FILE=" + sslCertFile}, + "-config="+accessControlConfigFile, "-auth-server="+authServer, - "-auth-redirect-uri=https://projects.gitlab-example.com/auth", - "-auth-secret=something-very-secret") + "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } func runPagesProcess(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, extraEnv []string, extraArgs ...string) (teardown func()) { @@ -440,3 +437,25 @@ func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server { return httptest.NewServer(handler) } + +func accessControlConfig(clientID, clientSecret, authSecret string) (string, error) { + f, err := ioutil.TempFile(os.TempDir(), "gitlab-pages-config") + if err != nil { + return "", err + } + defer f.Close() + + config := fmt.Sprintf(` +auth-client-id=%s +auth-client-secret=%s +auth-secret=%s +`, clientID, clientSecret, authSecret) + + _, err = f.Write([]byte(config)) + if err != nil { + return "", err + } + + return f.Name(), nil + +} diff --git a/internal/deprecatedargs/deprecatedargs.go b/internal/deprecatedargs/deprecatedargs.go deleted file mode 100644 index 9bc5a0496..000000000 --- a/internal/deprecatedargs/deprecatedargs.go +++ /dev/null @@ -1,34 +0,0 @@ -package deprecatedargs - -import ( - "fmt" - "strings" -) - -var deprecatedArgs = []string{"-auth-client-id", "-auth-client-secret", "-auth-secret", "-sentry-dsn"} - -// Validate checks if deprecated params have been used -func Validate(args []string) error { - foundDeprecatedArgs := []string{} - argMap := make(map[string]bool) - - for _, arg := range args { - keyValue := strings.Split(arg, "=") - if len(keyValue) >= 1 { - argMap[keyValue[0]] = true - } else { - argMap[arg] = true - } - } - - for _, deprecatedArg := range deprecatedArgs { - if argMap[deprecatedArg] { - foundDeprecatedArgs = append(foundDeprecatedArgs, deprecatedArg) - } - } - - if len(foundDeprecatedArgs) > 0 { - return fmt.Errorf("Deprecation message: %s should not be passed as a command line arguments", strings.Join(foundDeprecatedArgs, ", ")) - } - return nil -} diff --git a/internal/validateargs/validateargs.go b/internal/validateargs/validateargs.go new file mode 100644 index 000000000..263d3c529 --- /dev/null +++ b/internal/validateargs/validateargs.go @@ -0,0 +1,44 @@ +package validateargs + +import ( + "fmt" + "strings" +) + +var deprecatedArgs = []string{"-sentry-dsn"} +var notAllowedArgs = []string{"-auth-client-id", "-auth-client-secret", "-auth-secret"} + +// Deprecated checks if deprecated params have been used +func Deprecated(args []string) error { + var foundDeprecatedArgs []string + + argsStr := strings.Join(args, " ") + for _, deprecatedArg := range deprecatedArgs { + if strings.Contains(argsStr, deprecatedArg) { + foundDeprecatedArgs = append(foundDeprecatedArgs, deprecatedArg) + } + } + + if len(foundDeprecatedArgs) > 0 { + return fmt.Errorf("deprecation message: %s should not be passed as a command line arguments", strings.Join(foundDeprecatedArgs, ", ")) + } + return nil +} + +// NotAllowed checks if explicitly not allowed params have been used +func NotAllowed(args []string) error { + var foundNotAllowedArgs []string + + argsStr := strings.Join(args, " ") + for _, notAllowedArg := range notAllowedArgs { + if strings.Contains(argsStr, notAllowedArg) { + foundNotAllowedArgs = append(foundNotAllowedArgs, notAllowedArg) + } + } + + if len(foundNotAllowedArgs) > 0 { + return fmt.Errorf("%s should not be passed as a command line arguments", strings.Join(foundNotAllowedArgs, ", ")) + } + + return nil +} diff --git a/internal/deprecatedargs/deprecatedargs_test.go b/internal/validateargs/validateargs_test.go similarity index 70% rename from internal/deprecatedargs/deprecatedargs_test.go rename to internal/validateargs/validateargs_test.go index d301ec3b9..02f2f2efb 100644 --- a/internal/deprecatedargs/deprecatedargs_test.go +++ b/internal/validateargs/validateargs_test.go @@ -1,4 +1,4 @@ -package deprecatedargs +package validateargs import ( "testing" @@ -11,16 +11,28 @@ func TestValidParams(t *testing.T) { "-listen-http", ":3010", "-artifacts-server", "http://192.168.1.123:3000/api/v4", "-pages-domain", "127.0.0.1.xip.io"} - res := Validate(args) + res := Deprecated(args) require.Nil(t, res) } -func TestInvalidParms(t *testing.T) { +func TestInvalidDeprecatedParms(t *testing.T) { + tests := map[string][]string{ + "Sentry DSN passed": []string{"gitlab-pages", "-sentry-dsn", "abc123"}, + } + + for name, args := range tests { + t.Run(name, func(t *testing.T) { + err := Deprecated(args) + require.Error(t, err) + }) + } +} + +func TestInvalidNotAllowedParams(t *testing.T) { tests := map[string][]string{ "Client ID passed": []string{"gitlab-pages", "-auth-client-id", "abc123"}, "Client secret passed": []string{"gitlab-pages", "-auth-client-secret", "abc123"}, "Auth secret passed": []string{"gitlab-pages", "-auth-secret", "abc123"}, - "Sentry DSN passed": []string{"gitlab-pages", "-sentry-dsn", "abc123"}, "Multiple keys passed": []string{"gitlab-pages", "-auth-client-id", "abc123", "-auth-client-secret", "abc123"}, "key=value": []string{"gitlab-pages", "-auth-client-id=abc123"}, "multiple key=value": []string{"gitlab-pages", "-auth-client-id=abc123", "-auth-client-secret=abc123"}, @@ -28,7 +40,7 @@ func TestInvalidParms(t *testing.T) { for name, args := range tests { t.Run(name, func(t *testing.T) { - err := Validate(args) + err := NotAllowed(args) require.Error(t, err) }) } diff --git a/main.go b/main.go index d550b387d..2614fa0bc 100644 --- a/main.go +++ b/main.go @@ -15,10 +15,10 @@ import ( log "github.com/sirupsen/logrus" "gitlab.com/gitlab-org/labkit/errortracking" - "gitlab.com/gitlab-org/gitlab-pages/internal/deprecatedargs" "gitlab.com/gitlab-org/gitlab-pages/internal/host" "gitlab.com/gitlab-org/gitlab-pages/internal/logging" "gitlab.com/gitlab-org/gitlab-pages/internal/tlsconfig" + "gitlab.com/gitlab-org/gitlab-pages/internal/validateargs" "gitlab.com/gitlab-org/gitlab-pages/metrics" ) @@ -58,15 +58,15 @@ var ( _ = flag.String("admin-https-listener", "", "DEPRECATED") _ = flag.String("admin-https-cert", "", "DEPRECATED") _ = flag.String("admin-https-key", "", "DEPRECATED") - secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long; will be deprecated soon") + secret = flag.String("auth-secret", "", "Cookie store hash key, should be at least 32 bytes long") gitLabAuthServer = flag.String("auth-server", "", "DEPRECATED, use gitlab-server instead. GitLab server, for example https://www.gitlab.com") gitLabServer = flag.String("gitlab-server", "", "GitLab server, for example https://www.gitlab.com") internalGitLabServer = flag.String("internal-gitlab-server", "", "Internal GitLab server used for API requests, useful if you want to send that traffic over an internal load balancer, example value https://www.gitlab.com (defaults to value of gitlab-server)") gitLabAPISecretKey = flag.String("api-secret-key", "", "File with secret key used to authenticate with the GitLab API") gitlabClientHTTPTimeout = flag.Duration("gitlab-client-http-timeout", 10*time.Second, "GitLab API HTTP client connection timeout in seconds (default: 10s)") gitlabClientJWTExpiry = flag.Duration("gitlab-client-jwt-expiry", 30*time.Second, "JWT Token expiry time in seconds (default: 30s)") - clientID = flag.String("auth-client-id", "", "GitLab application Client ID; will be deprecated soon") - clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret; will be deprecated soon") + clientID = flag.String("auth-client-id", "", "GitLab application Client ID") + clientSecret = flag.String("auth-client-secret", "", "GitLab application Client Secret") redirectURI = flag.String("auth-redirect-uri", "", "GitLab application redirect URI") maxConns = flag.Uint("max-conns", 5000, "Limit on the number of concurrent connections to the HTTP, HTTPS or proxy listeners") insecureCiphers = flag.Bool("insecure-ciphers", false, "Use default list of cipher suites, may contain insecure ones like 3DES and RC4") @@ -240,7 +240,11 @@ func initErrorReporting(sentryDSN, sentryEnvironment string) { } func loadConfig() appConfig { - if err := deprecatedargs.Validate(os.Args[1:]); err != nil { + if err := validateargs.NotAllowed(os.Args[1:]); err != nil { + log.WithError(err).Fatal("Using invalid arguments, use -config=gitlab-pages-config file instead") + } + + if err := validateargs.Deprecated(os.Args[1:]); err != nil { log.WithError(err).Warn("Using deprecated arguments") } @@ -286,6 +290,7 @@ func loadConfig() appConfig { func appMain() { var showVersion = flag.Bool("version", false, "Show version") + // read from -config=/path/to/gitlab-pages-config flag.String(flag.DefaultConfigFlagname, "", "path to config file") flag.Parse() if err := tlsconfig.ValidateTLSVersions(*tlsMinVersion, *tlsMaxVersion); err != nil { -- GitLab From b9c69dc5390d15e114d0771d2c217a4be80693b2 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Apr 2020 11:03:13 +1000 Subject: [PATCH 3/6] Consolidate args validation --- acceptance_test.go | 1 + internal/validateargs/validateargs.go | 35 ++++++++++------------ internal/validateargs/validateargs_test.go | 5 +++- 3 files changed, 21 insertions(+), 20 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index da27473c3..fe41a8941 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -78,6 +78,7 @@ func TestMain(m *testing.M) { os.Exit(m.Run()) } + func TestUnknownHostReturnsNotFound(t *testing.T) { skipUnlessEnabled(t) teardown := RunPagesProcess(t, *pagesBinary, listeners, "") diff --git a/internal/validateargs/validateargs.go b/internal/validateargs/validateargs.go index 263d3c529..3b75b69bf 100644 --- a/internal/validateargs/validateargs.go +++ b/internal/validateargs/validateargs.go @@ -5,39 +5,36 @@ import ( "strings" ) +const ( + deprecatedMessage = "command line options have been deprecated:" + notAllowedMsg = "invalid command line arguments:" +) + var deprecatedArgs = []string{"-sentry-dsn"} var notAllowedArgs = []string{"-auth-client-id", "-auth-client-secret", "-auth-secret"} // Deprecated checks if deprecated params have been used func Deprecated(args []string) error { - var foundDeprecatedArgs []string - - argsStr := strings.Join(args, " ") - for _, deprecatedArg := range deprecatedArgs { - if strings.Contains(argsStr, deprecatedArg) { - foundDeprecatedArgs = append(foundDeprecatedArgs, deprecatedArg) - } - } - - if len(foundDeprecatedArgs) > 0 { - return fmt.Errorf("deprecation message: %s should not be passed as a command line arguments", strings.Join(foundDeprecatedArgs, ", ")) - } - return nil + return validate(args, deprecatedArgs, deprecatedMessage) } // NotAllowed checks if explicitly not allowed params have been used func NotAllowed(args []string) error { - var foundNotAllowedArgs []string + return validate(args, notAllowedArgs, notAllowedMsg) +} + +func validate(args, invalidArgs []string, errMsg string) error { + var foundInvalidArgs []string argsStr := strings.Join(args, " ") - for _, notAllowedArg := range notAllowedArgs { - if strings.Contains(argsStr, notAllowedArg) { - foundNotAllowedArgs = append(foundNotAllowedArgs, notAllowedArg) + for _, invalidArg := range invalidArgs { + if strings.Contains(argsStr, invalidArg) { + foundInvalidArgs = append(foundInvalidArgs, invalidArg) } } - if len(foundNotAllowedArgs) > 0 { - return fmt.Errorf("%s should not be passed as a command line arguments", strings.Join(foundNotAllowedArgs, ", ")) + if len(foundInvalidArgs) > 0 { + return fmt.Errorf("%s %s", errMsg, strings.Join(foundInvalidArgs, ", ")) } return nil diff --git a/internal/validateargs/validateargs_test.go b/internal/validateargs/validateargs_test.go index 02f2f2efb..4ec5cd898 100644 --- a/internal/validateargs/validateargs_test.go +++ b/internal/validateargs/validateargs_test.go @@ -17,13 +17,15 @@ func TestValidParams(t *testing.T) { func TestInvalidDeprecatedParms(t *testing.T) { tests := map[string][]string{ - "Sentry DSN passed": []string{"gitlab-pages", "-sentry-dsn", "abc123"}, + "Sentry DSN passed": []string{"gitlab-pages", "-sentry-dsn", "abc123"}, + "Sentry DSN using key=value": []string{"gitlab-pages", "-sentry-dsn=abc123"}, } for name, args := range tests { t.Run(name, func(t *testing.T) { err := Deprecated(args) require.Error(t, err) + require.Contains(t, err.Error(), deprecatedMessage) }) } } @@ -42,6 +44,7 @@ func TestInvalidNotAllowedParams(t *testing.T) { t.Run(name, func(t *testing.T) { err := NotAllowed(args) require.Error(t, err) + require.Contains(t, err.Error(), notAllowedMsg) }) } } -- GitLab From c50b59d216af4d5929587e8ef5b460acbcd3cdd4 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Wed, 22 Apr 2020 14:38:28 +1000 Subject: [PATCH 4/6] Rename test config file --- acceptance_test.go | 8 ++++---- helpers_test.go | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index fe41a8941..29a6928db 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -21,7 +21,7 @@ import ( ) var pagesBinary = flag.String("gitlab-pages-binary", "./gitlab-pages", "Path to the gitlab-pages binary") -var accessControlConfigFile string +var configFile string // TODO: Use TCP port 0 everywhere to avoid conflicts. The binary could output // the actual port (and type of listener) for us to read in place of the @@ -70,11 +70,11 @@ func skipUnlessEnabled(t *testing.T, conditions ...string) { func TestMain(m *testing.M) { var err error - accessControlConfigFile, err = accessControlConfig("clientID", "clientSecret", "authSecret") + configFile, err = accessControlConfig("clientID", "clientSecret", "authSecret") if err != nil { log.Fatal(err) } - defer os.Remove(accessControlConfigFile) + defer os.Remove(configFile) os.Exit(m.Run()) } @@ -695,7 +695,7 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { listeners, "", certFile, - "-config="+accessControlConfigFile, + "-config="+configFile, "-artifacts-server="+artifactServerURL, "-auth-server="+testServer.URL, "-auth-redirect-uri=https://projects.gitlab-example.com/auth", diff --git a/helpers_test.go b/helpers_test.go index e6a8d03e4..94d699aee 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -185,21 +185,21 @@ func RunPagesProcessWithEnvs(t *testing.T, wait bool, pagesPath string, listener func RunPagesProcessWithAuth(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string) func() { return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, - "-config="+accessControlConfigFile, + "-config="+configFile, "-auth-server=https://gitlab-auth.com", "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } func RunPagesProcessWithAuthServer(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, authServer string) func() { return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, - "-config="+accessControlConfigFile, + "-config="+configFile, "-auth-server="+authServer, "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, sslCertFile string, authServer string) func() { return runPagesProcess(t, true, pagesPath, listeners, promPort, []string{"SSL_CERT_FILE=" + sslCertFile}, - "-config="+accessControlConfigFile, + "-config="+configFile, "-auth-server="+authServer, "-auth-redirect-uri=https://projects.gitlab-example.com/auth") } -- GitLab From 3d60bf10dafec57eb89f8194d18402a69f29c701 Mon Sep 17 00:00:00 2001 From: Jaime Martinez Date: Mon, 27 Apr 2020 16:25:37 +1000 Subject: [PATCH 5/6] Write config file for some acceptance tests Use filename from closure --- acceptance_test.go | 26 +++++++-------------- helpers_test.go | 57 +++++++++++++++++++++++++++++++--------------- 2 files changed, 47 insertions(+), 36 deletions(-) diff --git a/acceptance_test.go b/acceptance_test.go index 29a6928db..4a1b30286 100644 --- a/acceptance_test.go +++ b/acceptance_test.go @@ -4,7 +4,6 @@ import ( "crypto/tls" "fmt" "io/ioutil" - "log" "mime" "net" "net/http" @@ -21,7 +20,6 @@ import ( ) var pagesBinary = flag.String("gitlab-pages-binary", "./gitlab-pages", "Path to the gitlab-pages binary") -var configFile string // TODO: Use TCP port 0 everywhere to avoid conflicts. The binary could output // the actual port (and type of listener) for us to read in place of the @@ -68,17 +66,6 @@ func skipUnlessEnabled(t *testing.T, conditions ...string) { } } -func TestMain(m *testing.M) { - var err error - configFile, err = accessControlConfig("clientID", "clientSecret", "authSecret") - if err != nil { - log.Fatal(err) - } - defer os.Remove(configFile) - - os.Exit(m.Run()) -} - func TestUnknownHostReturnsNotFound(t *testing.T) { skipUnlessEnabled(t) teardown := RunPagesProcess(t, *pagesBinary, listeners, "") @@ -663,7 +650,7 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { host: "group.gitlab-example.com", path: "/-/private/-/jobs/1/artifacts/delayed_200.html", status: http.StatusBadGateway, - binaryOption: "-artifacts-server-timeout=1", + binaryOption: "artifacts-server-timeout=1", }, { name: "Proxying 404 from server", @@ -689,6 +676,13 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + configFile, cleanup := defaultConfigFileWith(t, + "artifacts-server="+artifactServerURL, + "auth-server="+testServer.URL, + "auth-redirect-uri=https://projects.gitlab-example.com/auth", + tt.binaryOption) + defer cleanup() + teardown := RunPagesProcessWithSSLCertFile( t, *pagesBinary, @@ -696,10 +690,6 @@ func TestPrivateArtifactProxyRequest(t *testing.T) { "", certFile, "-config="+configFile, - "-artifacts-server="+artifactServerURL, - "-auth-server="+testServer.URL, - "-auth-redirect-uri=https://projects.gitlab-example.com/auth", - tt.binaryOption, ) defer teardown() diff --git a/helpers_test.go b/helpers_test.go index 94d699aee..195c3cea8 100644 --- a/helpers_test.go +++ b/helpers_test.go @@ -184,24 +184,34 @@ func RunPagesProcessWithEnvs(t *testing.T, wait bool, pagesPath string, listener } func RunPagesProcessWithAuth(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string) func() { + configFile, cleanup := defaultConfigFileWith(t, + "auth-server=https://gitlab-auth.com", + "auth-redirect-uri=https://projects.gitlab-example.com/auth") + defer cleanup() + return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, "-config="+configFile, - "-auth-server=https://gitlab-auth.com", - "-auth-redirect-uri=https://projects.gitlab-example.com/auth") + ) } func RunPagesProcessWithAuthServer(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, authServer string) func() { + configFile, cleanup := defaultConfigFileWith(t, + "auth-server="+authServer, + "auth-redirect-uri=https://projects.gitlab-example.com/auth") + defer cleanup() + return runPagesProcess(t, true, pagesPath, listeners, promPort, nil, - "-config="+configFile, - "-auth-server="+authServer, - "-auth-redirect-uri=https://projects.gitlab-example.com/auth") + "-config="+configFile) } func RunPagesProcessWithAuthServerWithSSL(t *testing.T, pagesPath string, listeners []ListenSpec, promPort string, sslCertFile string, authServer string) func() { + configFile, cleanup := defaultConfigFileWith(t, + "auth-server="+authServer, + "auth-redirect-uri=https://projects.gitlab-example.com/auth") + defer cleanup() + return runPagesProcess(t, true, pagesPath, listeners, promPort, []string{"SSL_CERT_FILE=" + sslCertFile}, - "-config="+configFile, - "-auth-server="+authServer, - "-auth-redirect-uri=https://projects.gitlab-example.com/auth") + "-config="+configFile) } func runPagesProcess(t *testing.T, wait bool, pagesPath string, listeners []ListenSpec, promPort string, extraEnv []string, extraArgs ...string) (teardown func()) { @@ -438,24 +448,35 @@ func NewGitlabDomainsSourceStub(t *testing.T) *httptest.Server { return httptest.NewServer(handler) } -func accessControlConfig(clientID, clientSecret, authSecret string) (string, error) { +func newConfigFile(configs ...string) (string, error) { f, err := ioutil.TempFile(os.TempDir(), "gitlab-pages-config") if err != nil { return "", err } defer f.Close() - config := fmt.Sprintf(` -auth-client-id=%s -auth-client-secret=%s -auth-secret=%s -`, clientID, clientSecret, authSecret) - - _, err = f.Write([]byte(config)) - if err != nil { - return "", err + for _, config := range configs { + _, err := fmt.Fprintf(f, "%s\n", config) + if err != nil { + return "", err + } } return f.Name(), nil +} + +func defaultConfigFileWith(t *testing.T, configs ...string) (string, func()) { + configs = append(configs, "auth-client-id=clientID", + "auth-client-secret=clientSecret", + "auth-secret=authSecret") + + name, err := newConfigFile(configs...) + require.NoError(t, err) + + cleanup := func() { + err := os.Remove(name) + require.NoError(t, err) + } + return name, cleanup } -- GitLab From 26a4f0247516ef896b680773bea5bf71bfe0e68e Mon Sep 17 00:00:00 2001 From: Vladimir Shushlin Date: Fri, 8 May 2020 15:13:19 +0300 Subject: [PATCH 6/6] Test that standart arguments are allowed --- internal/validateargs/validateargs_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/validateargs/validateargs_test.go b/internal/validateargs/validateargs_test.go index 4ec5cd898..7bf2ec105 100644 --- a/internal/validateargs/validateargs_test.go +++ b/internal/validateargs/validateargs_test.go @@ -11,8 +11,8 @@ func TestValidParams(t *testing.T) { "-listen-http", ":3010", "-artifacts-server", "http://192.168.1.123:3000/api/v4", "-pages-domain", "127.0.0.1.xip.io"} - res := Deprecated(args) - require.Nil(t, res) + require.NoError(t, Deprecated(args)) + require.NoError(t, NotAllowed(args)) } func TestInvalidDeprecatedParms(t *testing.T) { -- GitLab