Merge branch '274-make-gitlab-client-timeout-jtw-token-expiry-configurable' into 'master'

Make GitLab client timeout / JTW token expiry configurable

Closes #274

See merge request gitlab-org/gitlab-pages!219

6 files changed, 185 insertions, 83 deletions

diff --git a/app_config.go b/app_config.go
index 639ece85..245a9e0d 100644
--- a/app_config.go
+++ b/app_config.go
@@ -1,5 +1,7 @@
 package main
 
+import "time"
+
 type appConfig struct {
 	Domain                 string
 	ArtifactsServer        string
@@ -25,15 +27,17 @@ type appConfig struct {
 	LogFormat  string
 	LogVerbose bool
 
-	StoreSecret        string
-	GitLabServer       string
-	GitLabAPISecretKey []byte
-	ClientID           string
-	ClientSecret       string
-	RedirectURI        string
-	SentryDSN          string
-	SentryEnvironment  string
-	CustomHeaders      []string
+	StoreSecret              string
+	GitLabServer             string
+	GitLabAPISecretKey       []byte
+	GitlabClientHTTPTimeout  time.Duration
+	GitlabJWTTokenExpiration time.Duration
+	ClientID                 string
+	ClientSecret             string
+	RedirectURI              string
+	SentryDSN                string
+	SentryEnvironment        string
+	CustomHeaders            []string
 }
 
 // GitlabServerURL returns URL to a GitLab instance.
@@ -45,3 +49,11 @@ func (config appConfig) GitlabServerURL() string {
 func (config appConfig) GitlabAPISecret() []byte {
 	return config.GitLabAPISecretKey
 }
+
+func (config appConfig) GitlabClientConnectionTimeout() time.Duration {
+	return config.GitlabClientHTTPTimeout
+}
+
+func (config appConfig) GitlabJWTTokenExpiry() time.Duration {
+	return config.GitlabJWTTokenExpiration
+}
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index 5fea6ae3..bbdb8c21 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -2,6 +2,7 @@ package source
 
 import (
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/require"
 
@@ -21,6 +22,13 @@ func (c sourceConfig) GitlabServerURL() string {
 func (c sourceConfig) GitlabAPISecret() []byte {
 	return []byte(c.secret)
 }
+func (c sourceConfig) GitlabClientConnectionTimeout() time.Duration {
+	return 10 * time.Second
+}
+
+func (c sourceConfig) GitlabJWTTokenExpiry() time.Duration {
+	return 30 * time.Second
+}
 
 func TestDomainSources(t *testing.T) {
 	t.Run("when GitLab API URL has been provided", func(t *testing.T) {
diff --git a/internal/source/gitlab/client/client.go b/internal/source/gitlab/client/client.go
index f5ecf355..59776a8f 100644
--- a/internal/source/gitlab/client/client.go
+++ b/internal/source/gitlab/client/client.go
@@ -19,18 +19,15 @@ import (
 
 // Client is a HTTP client to access Pages internal API
 type Client struct {
-	secretKey  []byte
-	baseURL    *url.URL
-	httpClient *http.Client
+	secretKey      []byte
+	baseURL        *url.URL
+	httpClient     *http.Client
+	jwtTokenExpiry time.Duration
 }
 
-// TODO make these values configurable https://gitlab.com/gitlab-org/gitlab-pages/issues/274
-var tokenTimeout = 30 * time.Second
-var connectionTimeout = 10 * time.Second
-
 // NewClient initializes and returns new Client baseUrl is
 // appConfig.GitLabServer secretKey is appConfig.GitLabAPISecretKey
-func NewClient(baseURL string, secretKey []byte) (*Client, error) {
+func NewClient(baseURL string, secretKey []byte, connectionTimeout, jwtTokenExpiry time.Duration) (*Client, error) {
 	if len(baseURL) == 0 || len(secretKey) == 0 {
 		return nil, errors.New("GitLab API URL or API secret has not been provided")
 	}
@@ -40,6 +37,14 @@ func NewClient(baseURL string, secretKey []byte) (*Client, error) {
 		return nil, err
 	}
 
+	if connectionTimeout == 0 {
+		return nil, errors.New("GitLab HTTP client connection timeout has not been provided")
+	}
+
+	if jwtTokenExpiry == 0 {
+		return nil, errors.New("GitLab JWT token expiry has not been provided")
+	}
+
 	return &Client{
 		secretKey: secretKey,
 		baseURL:   url,
@@ -47,12 +52,13 @@ func NewClient(baseURL string, secretKey []byte) (*Client, error) {
 			Timeout:   connectionTimeout,
 			Transport: httptransport.Transport,
 		},
+		jwtTokenExpiry: jwtTokenExpiry,
 	}, nil
 }
 
 // NewFromConfig creates a new client from Config struct
 func NewFromConfig(config Config) (*Client, error) {
-	return NewClient(config.GitlabServerURL(), config.GitlabAPISecret())
+	return NewClient(config.GitlabServerURL(), config.GitlabAPISecret(), config.GitlabClientConnectionTimeout(), config.GitlabJWTTokenExpiry())
 }
 
 // Resolve returns a VirtualDomain configuration wrapped into a Lookup for a
@@ -151,7 +157,7 @@ func (gc *Client) request(ctx context.Context, method string, endpoint *url.URL)
 func (gc *Client) token() (string, error) {
 	claims := jwt.StandardClaims{
 		Issuer:    "gitlab-pages",
-		ExpiresAt: time.Now().Add(tokenTimeout).Unix(),
+		ExpiresAt: time.Now().UTC().Add(gc.jwtTokenExpiry).Unix(),
 	}
 
 	token, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(gc.secretKey)
diff --git a/internal/source/gitlab/client/client_test.go b/internal/source/gitlab/client/client_test.go
index 247de5c7..bdfbe6b0 100644
--- a/internal/source/gitlab/client/client_test.go
+++ b/internal/source/gitlab/client/client_test.go
@@ -7,6 +7,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"testing"
+	"time"
 
 	jwt "github.com/dgrijalva/jwt-go"
 	"github.com/stretchr/testify/require"
@@ -14,34 +15,90 @@ import (
 	"gitlab.com/gitlab-org/gitlab-pages/internal/fixture"
 )
 
+const (
+	defaultClientConnTimeout = 10 * time.Second
+	defaultJWTTokenExpiry    = 30 * time.Second
+)
+
 func TestNewValidBaseURL(t *testing.T) {
-	_, err := NewClient("https://gitlab.com", secretKey())
+	_, err := NewClient("https://gitlab.com", secretKey(t), defaultClientConnTimeout, defaultJWTTokenExpiry)
 	require.NoError(t, err)
 }
 
-func TestNewInvalidBaseURL(t *testing.T) {
-	t.Run("when API URL is not valid", func(t *testing.T) {
-		client, err := NewClient("%", secretKey())
-
-		require.Error(t, err)
-		require.Nil(t, client)
-	})
-
-	t.Run("when API URL is empty", func(t *testing.T) {
-		client, err := NewClient("", secretKey())
-
-		require.Nil(t, client)
-		require.EqualError(t, err, "GitLab API URL or API secret has not been provided")
-	})
+func TestNewInvalidConfiguration(t *testing.T) {
+	type args struct {
+		baseURL           string
+		secretKey         []byte
+		connectionTimeout time.Duration
+		jwtTokenExpiry    time.Duration
+	}
 
-	t.Run("when API secret is empty", func(t *testing.T) {
-		client, err := NewClient("https://gitlab.com", []byte{})
+	tests := []struct {
+		name       string
+		args       args
+		wantErrMsg string
+	}{
+
+		{
+			name: "invalid_api_url",
+			args: args{
+				baseURL:           "%",
+				secretKey:         secretKey(t),
+				connectionTimeout: defaultClientConnTimeout,
+				jwtTokenExpiry:    defaultJWTTokenExpiry,
+			},
+			wantErrMsg: "parse %: invalid URL escape \"%\"",
+		},
+		{
+			name: "invalid_api_url_empty",
+			args: args{
+				baseURL:           "",
+				secretKey:         secretKey(t),
+				connectionTimeout: defaultClientConnTimeout,
+				jwtTokenExpiry:    defaultJWTTokenExpiry,
+			},
+			wantErrMsg: "GitLab API URL or API secret has not been provided",
+		},
+		{
+			name: "invalid_api_secret_empty",
+			args: args{
+				baseURL:           "https://gitlab.com",
+				secretKey:         []byte{},
+				connectionTimeout: defaultClientConnTimeout,
+				jwtTokenExpiry:    defaultJWTTokenExpiry,
+			},
+			wantErrMsg: "GitLab API URL or API secret has not been provided",
+		},
+		{
+			name: "invalid_http_client_timeout",
+			args: args{
+				baseURL:           "https://gitlab.com",
+				secretKey:         secretKey(t),
+				connectionTimeout: 0,
+				jwtTokenExpiry:    defaultJWTTokenExpiry,
+			},
+			wantErrMsg: "GitLab HTTP client connection timeout has not been provided",
+		},
+		{
+			name: "invalid_jwt_token_expiry",
+			args: args{
+				baseURL:           "https://gitlab.com",
+				secretKey:         secretKey(t),
+				connectionTimeout: defaultClientConnTimeout,
+				jwtTokenExpiry:    0,
+			},
+			wantErrMsg: "GitLab JWT token expiry has not been provided",
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
 
-		require.Nil(t, client)
-		require.EqualError(t, err, "GitLab API URL or API secret has not been provided")
-	})
+			got, err := NewClient(tt.args.baseURL, tt.args.secretKey, tt.args.connectionTimeout, tt.args.jwtTokenExpiry)
+			require.Nil(t, got)
+			require.EqualError(t, err, tt.wantErrMsg)
+		})
+	}
 }
-
 func TestLookupForErrorResponses(t *testing.T) {
 	tests := map[int]string{
 		http.StatusUnauthorized: "HTTP status: 401",
@@ -60,8 +117,7 @@ func TestLookupForErrorResponses(t *testing.T) {
 			server := httptest.NewServer(mux)
 			defer server.Close()
 
-			client, err := NewClient(server.URL, secretKey())
-			require.NoError(t, err)
+			client := defaultClient(t, server.URL)
 
 			lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
@@ -81,8 +137,7 @@ func TestMissingDomain(t *testing.T) {
 	server := httptest.NewServer(mux)
 	defer server.Close()
 
-	client, err := NewClient(server.URL, secretKey())
-	require.NoError(t, err)
+	client := defaultClient(t, server.URL)
 
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 
@@ -123,8 +178,7 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) {
 	server := httptest.NewServer(mux)
 	defer server.Close()
 
-	client, err := NewClient(server.URL, secretKey())
-	require.NoError(t, err)
+	client := defaultClient(t, server.URL)
 
 	lookup := client.GetLookup(context.Background(), "group.gitlab.io")
 	require.NoError(t, lookup.Error)
@@ -143,12 +197,13 @@ func TestGetVirtualDomainAuthenticatedRequest(t *testing.T) {
 }
 
 func validateToken(t *testing.T, tokenString string) {
+	t.Helper()
 	token, err := jwt.Parse(tokenString, func(token *jwt.Token) (interface{}, error) {
 		if _, ok := token.Method.(*jwt.SigningMethodHMAC); !ok {
 			return nil, fmt.Errorf("Unexpected signing method: %v", token.Header["alg"])
 		}
 
-		return secretKey(), nil
+		return secretKey(t), nil
 	})
 	require.NoError(t, err)
 
@@ -159,7 +214,20 @@ func validateToken(t *testing.T, tokenString string) {
 	require.Equal(t, "gitlab-pages", claims["iss"])
 }
 
-func secretKey() []byte {
-	secretKey, _ := base64.StdEncoding.DecodeString(fixture.GitLabAPISecretKey)
+func secretKey(t *testing.T) []byte {
+	t.Helper()
+
+	secretKey, err := base64.StdEncoding.DecodeString(fixture.GitLabAPISecretKey)
+	require.NoError(t, err)
+
 	return secretKey
 }
+
+func defaultClient(t *testing.T, url string) *Client {
+	t.Helper()
+
+	client, err := NewClient(url, secretKey(t), defaultClientConnTimeout, defaultJWTTokenExpiry)
+	require.NoError(t, err)
+
+	return client
+}
diff --git a/internal/source/gitlab/client/config.go b/internal/source/gitlab/client/config.go
index 49c13a60..4ed14267 100644
--- a/internal/source/gitlab/client/config.go
+++ b/internal/source/gitlab/client/config.go
@@ -1,8 +1,12 @@
 package client
 
+import "time"
+
 // Config represents an interface that is configuration provider for client
 // capable of comunicating with GitLab
 type Config interface {
 	GitlabServerURL() string
 	GitlabAPISecret() []byte
+	GitlabClientConnectionTimeout() time.Duration
+	GitlabJWTTokenExpiry() time.Duration
 }
diff --git a/main.go b/main.go
index c4a6a4e2..8459314d 100644
--- a/main.go
+++ b/main.go
@@ -8,6 +8,7 @@ import (
 	"net/url"
 	"os"
 	"strings"
+	"time"
 
 	"github.com/namsral/flag"
 	log "github.com/sirupsen/logrus"
@@ -33,39 +34,41 @@ func init() {
 }
 
 var (
-	pagesRootCert          = flag.String("root-cert", "", "The default path to file certificate to serve static pages")
-	pagesRootKey           = flag.String("root-key", "", "The default path to file certificate to serve static pages")
-	redirectHTTP           = flag.Bool("redirect-http", false, "Redirect pages from HTTP to HTTPS")
-	useHTTP2               = flag.Bool("use-http2", true, "Enable HTTP2 support")
-	pagesRoot              = flag.String("pages-root", "shared/pages", "The directory where pages are stored")
-	pagesDomain            = flag.String("pages-domain", "gitlab-example.com", "The domain to serve static pages")
-	artifactsServer        = flag.String("artifacts-server", "", "API URL to proxy artifact requests to, e.g.: 'https://gitlab.com/api/v4'")
-	artifactsServerTimeout = flag.Int("artifacts-server-timeout", 10, "Timeout (in seconds) for a proxied request to the artifacts server")
-	pagesStatus            = flag.String("pages-status", "", "The url path for a status page, e.g., /@status")
-	metricsAddress         = flag.String("metrics-address", "", "The address to listen on for metrics requests")
-	sentryDSN              = flag.String("sentry-dsn", "", "The address for sending sentry crash reporting to")
-	sentryEnvironment      = flag.String("sentry-environment", "", "The environment for sentry crash reporting")
-	daemonUID              = flag.Uint("daemon-uid", 0, "Drop privileges to this user")
-	daemonGID              = flag.Uint("daemon-gid", 0, "Drop privileges to this group")
-	daemonInplaceChroot    = flag.Bool("daemon-inplace-chroot", false, "Fall back to a non-bind-mount chroot of -pages-root when daemonizing")
-	logFormat              = flag.String("log-format", "text", "The log output format: 'text' or 'json'")
-	logVerbose             = flag.Bool("log-verbose", false, "Verbose logging")
-	_                      = flag.String("admin-secret-path", "", "DEPRECATED")
-	_                      = flag.String("admin-unix-listener", "", "DEPRECATED")
-	_                      = 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.")
-	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")
-	gitLabAPISecretKey     = flag.String("api-secret-key", "", "File with secret key used to authenticate with the GitLab API")
-	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")
-	tlsMinVersion          = flag.String("tls-min-version", "tls1.2", tlsconfig.FlagUsage("min"))
-	tlsMaxVersion          = flag.String("tls-max-version", "", tlsconfig.FlagUsage("max"))
+	pagesRootCert           = flag.String("root-cert", "", "The default path to file certificate to serve static pages")
+	pagesRootKey            = flag.String("root-key", "", "The default path to file certificate to serve static pages")
+	redirectHTTP            = flag.Bool("redirect-http", false, "Redirect pages from HTTP to HTTPS")
+	useHTTP2                = flag.Bool("use-http2", true, "Enable HTTP2 support")
+	pagesRoot               = flag.String("pages-root", "shared/pages", "The directory where pages are stored")
+	pagesDomain             = flag.String("pages-domain", "gitlab-example.com", "The domain to serve static pages")
+	artifactsServer         = flag.String("artifacts-server", "", "API URL to proxy artifact requests to, e.g.: 'https://gitlab.com/api/v4'")
+	artifactsServerTimeout  = flag.Int("artifacts-server-timeout", 10, "Timeout (in seconds) for a proxied request to the artifacts server")
+	pagesStatus             = flag.String("pages-status", "", "The url path for a status page, e.g., /@status")
+	metricsAddress          = flag.String("metrics-address", "", "The address to listen on for metrics requests")
+	sentryDSN               = flag.String("sentry-dsn", "", "The address for sending sentry crash reporting to")
+	sentryEnvironment       = flag.String("sentry-environment", "", "The environment for sentry crash reporting")
+	daemonUID               = flag.Uint("daemon-uid", 0, "Drop privileges to this user")
+	daemonGID               = flag.Uint("daemon-gid", 0, "Drop privileges to this group")
+	daemonInplaceChroot     = flag.Bool("daemon-inplace-chroot", false, "Fall back to a non-bind-mount chroot of -pages-root when daemonizing")
+	logFormat               = flag.String("log-format", "text", "The log output format: 'text' or 'json'")
+	logVerbose              = flag.Bool("log-verbose", false, "Verbose logging")
+	_                       = flag.String("admin-secret-path", "", "DEPRECATED")
+	_                       = flag.String("admin-unix-listener", "", "DEPRECATED")
+	_                       = 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.")
+	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")
+	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")
+	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")
+	tlsMinVersion           = flag.String("tls-min-version", "tls1.2", tlsconfig.FlagUsage("min"))
+	tlsMaxVersion           = flag.String("tls-max-version", "", tlsconfig.FlagUsage("max"))
 
 	disableCrossOriginRequests = flag.Bool("disable-cross-origin-requests", false, "Disable cross-origin requests")
 
@@ -176,7 +179,8 @@ func configFromFlags() appConfig {
 	}
 
 	config.GitLabServer = gitlabServerFromFlags()
-
+	config.GitlabClientHTTPTimeout = *gitlabClientHTTPTimeout
+	config.GitlabJWTTokenExpiration = *gitlabClientJWTExpiry
 	config.StoreSecret = *secret
 	config.ClientID = *clientID
 	config.ClientSecret = *clientSecret