Merge branch 'tls-metrics' into 'master'

Add TLS for metrics

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

6 files changed, 176 insertions, 7 deletions

diff --git a/README.md b/README.md
index c4e2313c..4033f0c3 100644
--- a/README.md
+++ b/README.md
@@ -166,6 +166,8 @@ $ make
 $ ./gitlab-pages -listen-http ":8090" -metrics-address ":9235" -pages-root path/to/gitlab/shared/pages -pages-domain example.com
 ```
 
+Passing the `-metrics-certificate` and `-metrics-key` flags along with `-metrics-address` flag would add TLS to the metrics.
+
 ### Structured logging
 
 You can use the `-log-format json` option to make GitLab Pages output
diff --git a/app.go b/app.go
index c7797297..6ce414a4 100644
--- a/app.go
+++ b/app.go
@@ -277,8 +277,8 @@ func (a *theApp) Run() error {
 	}
 
 	// Serve metrics for Prometheus
-	if a.config.General.MetricsAddress != "" {
-		s := a.listenMetrics(eg, a.config.General.MetricsAddress)
+	if a.config.Metrics.Address != "" {
+		s := a.listenMetrics(eg, a.config.Metrics)
 		servers = append(servers, s)
 	}
 
@@ -322,13 +322,17 @@ func (a *theApp) listen(eg *errgroup.Group, addr string, h http.Handler, errTrac
 	return server
 }
 
-func (a *theApp) listenMetrics(eg *errgroup.Group, addr string) *http.Server {
+func (a *theApp) listenMetrics(eg *errgroup.Group, config cfg.Metrics) *http.Server {
 	server := &http.Server{}
 	eg.Go(func() error {
-		l, err := net.Listen("tcp", addr)
+		l, err := net.Listen("tcp", config.Address)
 		if err != nil {
 			errortracking.CaptureErrWithStackTrace(err, errortracking.WithField("listener", "metrics"))
-			return fmt.Errorf("failed to listen on addr %s: %w", addr, err)
+			return fmt.Errorf("failed to listen on addr %s: %w", config.Address, err)
+		}
+
+		if config.TLSConfig != nil {
+			l = cryptotls.NewListener(l, config.TLSConfig)
 		}
 
 		monitoringOpts := []monitoring.Option{
diff --git a/internal/config/config.go b/internal/config/config.go
index f3650ffb..18fe33eb 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -1,7 +1,9 @@
 package config
 
 import (
+	"crypto/tls"
 	"encoding/base64"
+	"errors"
 	"fmt"
 	"os"
 	"strings"
@@ -23,6 +25,7 @@ type Config struct {
 	Server          Server
 	TLS             TLS
 	Zip             ZipServing
+	Metrics         Metrics
 
 	// These fields contain the raw strings passed for listen-http,
 	// listen-https, listen-proxy and listen-https-proxyv2 settings. It is used
@@ -39,7 +42,6 @@ type General struct {
 	Domain                string
 	MaxConns              int
 	MaxURILength          int
-	MetricsAddress        string
 	RedirectHTTP          bool
 	RootCertificate       []byte
 	RootDir               string
@@ -146,6 +148,16 @@ type Server struct {
 	ListenKeepAlive   time.Duration
 }
 
+type Metrics struct {
+	Address   string
+	TLSConfig *tls.Config
+}
+
+var (
+	errMetricsNoCertificate = errors.New("metrics certificate path must not be empty")
+	errMetricsNoKey         = errors.New("metrics private key path must not be empty")
+)
+
 func internalGitlabServerFromFlags() string {
 	if *internalGitLabServer != "" {
 		return *internalGitLabServer
@@ -186,13 +198,45 @@ func setGitLabAPISecretKey(secretFile string, config *Config) error {
 	return nil
 }
 
+func loadMetricsConfig() (metrics Metrics, err error) {
+	// don't validate anything if metrics are disabled
+	if *metricsAddress == "" {
+		return metrics, nil
+	}
+	metrics.Address = *metricsAddress
+
+	// no error when using HTTP
+	if *metricsCertificate == "" && *metricsKey == "" {
+		return metrics, nil
+	}
+
+	if *metricsCertificate == "" {
+		return metrics, errMetricsNoCertificate
+	}
+
+	if *metricsKey == "" {
+		return metrics, errMetricsNoKey
+	}
+
+	cert, err := tls.LoadX509KeyPair(*metricsCertificate, *metricsKey)
+	if err != nil {
+		return metrics, err
+	}
+
+	metrics.TLSConfig = &tls.Config{
+		Certificates: []tls.Certificate{cert},
+		MinVersion:   tls.VersionTLS12,
+	}
+
+	return metrics, nil
+}
+
 func loadConfig() (*Config, error) {
 	config := &Config{
 		General: General{
 			Domain:                     strings.ToLower(*pagesDomain),
 			MaxConns:                   *maxConns,
 			MaxURILength:               *maxURILength,
-			MetricsAddress:             *metricsAddress,
 			RedirectHTTP:               *redirectHTTP,
 			RootDir:                    *pagesRoot,
 			StatusPath:                 *pagesStatus,
@@ -276,6 +320,11 @@ func loadConfig() (*Config, error) {
 
 	var err error
 
+	// Validating and populating Metrics config
+	if config.Metrics, err = loadMetricsConfig(); err != nil {
+		return nil, err
+	}
+
 	// Populating remaining General settings
 	for _, file := range []struct {
 		contents *[]byte
@@ -319,6 +368,8 @@ func LogConfig(config *Config) {
 		"listen-https-proxyv2":           listenHTTPSProxyv2,
 		"log-format":                     *logFormat,
 		"metrics-address":                *metricsAddress,
+		"metrics-certificate":            *metricsCertificate,
+		"metrics-key":                    *metricsKey,
 		"pages-domain":                   *pagesDomain,
 		"pages-root":                     *pagesRoot,
 		"pages-status":                   *pagesStatus,
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
new file mode 100644
index 00000000..f120d17d
--- /dev/null
+++ b/internal/config/config_test.go
@@ -0,0 +1,85 @@
+package config
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"gitlab.com/gitlab-org/gitlab-pages/internal/fixture"
+)
+
+func Test_loadMetricsConfig(t *testing.T) {
+	defaultMetricsAdress := ":9325"
+	defaultDir, defaultMetricsKey, defaultMetricsCertificate := setupHTTPSFixture(t)
+
+	tests := map[string]struct {
+		metricsAddress     string
+		metricsCertificate string
+		metricsKey         string
+		expectedError      error
+	}{
+		"no metrics": {},
+		"http metrics": {
+			metricsAddress: defaultMetricsAdress,
+		},
+		"https metrics": {
+			metricsAddress:     defaultMetricsAdress,
+			metricsCertificate: defaultMetricsCertificate,
+			metricsKey:         defaultMetricsKey,
+		},
+		"https metrics no certificate": {
+			metricsAddress: defaultMetricsAdress,
+			metricsKey:     defaultMetricsKey,
+			expectedError:  errMetricsNoCertificate,
+		},
+		"https metrics no key": {
+			metricsAddress:     defaultMetricsAdress,
+			metricsCertificate: defaultMetricsCertificate,
+			expectedError:      errMetricsNoKey,
+		},
+		"https metrics invalid certificate path": {
+			metricsAddress:     defaultMetricsAdress,
+			metricsCertificate: filepath.Join(defaultDir, "domain.certificate.missing"),
+			metricsKey:         defaultMetricsKey,
+			expectedError:      os.ErrNotExist,
+		},
+		"https metrics invalid key path": {
+			metricsAddress:     defaultMetricsAdress,
+			metricsCertificate: defaultMetricsCertificate,
+			metricsKey:         filepath.Join(defaultDir, "domain.key.missing"),
+			expectedError:      os.ErrNotExist,
+		},
+	}
+	for name, tc := range tests {
+		t.Run(name, func(t *testing.T) {
+			metricsAddress = &tc.metricsAddress
+			metricsCertificate = &tc.metricsCertificate
+			metricsKey = &tc.metricsKey
+			_, err := loadMetricsConfig()
+			require.ErrorIs(t, err, tc.expectedError)
+		})
+	}
+}
+
+func setupHTTPSFixture(t *testing.T) (dir string, key string, cert string) {
+	t.Helper()
+
+	tmpDir := t.TempDir()
+
+	keyfile, err := os.CreateTemp(tmpDir, "https-fixture")
+	require.NoError(t, err)
+	key = keyfile.Name()
+	keyfile.Close()
+
+	certfile, err := os.CreateTemp(tmpDir, "https-fixture")
+	require.NoError(t, err)
+	cert = certfile.Name()
+	certfile.Close()
+
+	require.NoError(t, os.WriteFile(key, []byte(fixture.Key), 0644))
+	require.NoError(t, os.WriteFile(cert, []byte(fixture.Certificate), 0644))
+
+	return tmpDir, keyfile.Name(), certfile.Name()
+}
diff --git a/internal/config/flags.go b/internal/config/flags.go
index 23da47ed..88ebbb15 100644
--- a/internal/config/flags.go
+++ b/internal/config/flags.go
@@ -40,6 +40,8 @@ var (
 	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")
+	metricsCertificate      = flag.String("metrics-certificate", "", "The default path to file certificate to serve metrics requests")
+	metricsKey              = flag.String("metrics-key", "", "The default path to file private key to serve 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")
 	propagateCorrelationID  = flag.Bool("propagate-correlation-id", false, "Reuse existing Correlation-ID from the incoming request header `X-Request-ID` if present")
diff --git a/test/acceptance/metrics_test.go b/test/acceptance/metrics_test.go
index d1e8a1b6..193d21e7 100644
--- a/test/acceptance/metrics_test.go
+++ b/test/acceptance/metrics_test.go
@@ -1,6 +1,7 @@
 package acceptance_test
 
 import (
+	"crypto/tls"
 	"io"
 	"net/http"
 	"testing"
@@ -58,3 +59,27 @@ func TestPrometheusMetricsCanBeScraped(t *testing.T) {
 	require.Contains(t, string(body), "gitlab_pages_limit_listener_concurrent_conns")
 	require.Contains(t, string(body), "gitlab_pages_limit_listener_waiting_conns")
 }
+
+func TestMetricsHTTPSConnection(t *testing.T) {
+	keyFile, certFile := CreateHTTPSFixtureFiles(t)
+
+	RunPagesProcess(t,
+		withExtraArgument("metrics-address", ":42345"),
+		withExtraArgument("metrics-certificate", certFile),
+		withExtraArgument("metrics-key", keyFile),
+	)
+
+	tlsConfig := &tls.Config{InsecureSkipVerify: true}
+	transport := &http.Transport{TLSClientConfig: tlsConfig}
+	client := &http.Client{Transport: transport}
+
+	res, err := client.Get("https://127.0.0.1:42345/metrics")
+	require.NoError(t, err)
+	testhelpers.Close(t, res.Body)
+	require.Equal(t, http.StatusOK, res.StatusCode)
+
+	res, err = client.Get("http://127.0.0.1:42345/metrics")
+	require.NoError(t, err)
+	testhelpers.Close(t, res.Body)
+	require.Equal(t, http.StatusBadRequest, res.StatusCode)
+}