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`

7 files changed, 120 insertions, 63 deletions

diff --git a/.gitignore b/.gitignore
index db328895..7c50688b 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 79a9b275..da27473c 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 29ed87de..e6a8d03e 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 9bc5a049..00000000
--- 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 00000000..263d3c52
--- /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
index d301ec3b..02f2f2ef 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 d550b387..2614fa0b 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 {