Refactoring of TLS usage

In order to implement proper support of TLS connections
for the praefect we require some refactoring of the existing
codebase.
Mainly it is about moving the things into other places and
fixing `config.Config` global var usage where is it accessible
as parameter.

Part of: https://gitlab.com/gitlab-org/gitaly/-/issues/1698

7 files changed, 110 insertions, 95 deletions

diff --git a/client/dial_test.go b/client/dial_test.go
index d535767f4..2759e2dde 100644
--- a/client/dial_test.go
+++ b/client/dial_test.go
@@ -11,6 +11,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	gitaly_x509 "gitlab.com/gitlab-org/gitaly/internal/x509"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials"
@@ -109,8 +110,7 @@ func TestDial(t *testing.T) {
 			}
 
 			if tt.envSSLCertFile != "" {
-				restore := modifyEnvironment("SSL_CERT_FILE", tt.envSSLCertFile)
-				defer restore()
+				defer testhelper.ModifyEnvironment(t, gitaly_x509.SSLCertFile, tt.envSSLCertFile)()
 			}
 
 			err := doDialAndExecuteCall(tt.rawAddress)
@@ -220,20 +220,6 @@ func startListeners() (func(), map[string]string, error) {
 	}, connectionMap, nil
 }
 
-// modifyEnvironment will change an environment variable and return a func suitable
-// for `defer` to change the value back.
-func modifyEnvironment(key string, value string) func() {
-	oldValue, hasOldValue := os.LookupEnv(key)
-	os.Setenv(key, value)
-	return func() {
-		if hasOldValue {
-			os.Setenv(key, oldValue)
-		} else {
-			os.Unsetenv(key)
-		}
-	}
-}
-
 func emitProxyWarning() bool {
 	for _, key := range proxyEnvironmentKeys {
 		value := os.Getenv(key)
diff --git a/config.toml.example b/config.toml.example
index 85fbf6ba6..1332f6f9d 100644
--- a/config.toml.example
+++ b/config.toml.example
@@ -9,7 +9,7 @@ bin_dir = "/home/git/gitaly"
 
 # # Optional: listen on a TCP socket. This is insecure (no authentication)
 # listen_addr = "localhost:9999"
-# tls_listen_addr = "localhost:8888
+# tls_listen_addr = "localhost:8888"
 
 # # Optional: export metrics via Prometheus
 # prometheus_listen_addr = "localhost:9236"
diff --git a/internal/server/server.go b/internal/server/server.go
index 3ca647304..702b60027 100644
--- a/internal/server/server.go
+++ b/internal/server/server.go
@@ -88,7 +88,7 @@ func createNewServer(rubyServer *rubyserver.Server, gitlabAPI hook.GitlabAPI, cf
 			grpc_logrus.StreamServerInterceptor(logrusEntry),
 			sentryhandler.StreamLogHandler,
 			cancelhandler.Stream, // Should be below LogHandler
-			auth.StreamServerInterceptor(config.Config.Auth),
+			auth.StreamServerInterceptor(cfg.Auth),
 			lh.StreamInterceptor(), // Should be below auth handler to prevent v2 hmac tokens from timing out while queued
 			grpctracing.StreamServerTracingInterceptor(),
 			cache.StreamInvalidator(diskcache.LeaseKeyer{}, protoregistry.GitalyProtoPreregistered),
@@ -104,7 +104,7 @@ func createNewServer(rubyServer *rubyserver.Server, gitlabAPI hook.GitlabAPI, cf
 			grpc_logrus.UnaryServerInterceptor(logrusEntry),
 			sentryhandler.UnaryLogHandler,
 			cancelhandler.Unary, // Should be below LogHandler
-			auth.UnaryServerInterceptor(config.Config.Auth),
+			auth.UnaryServerInterceptor(cfg.Auth),
 			lh.UnaryInterceptor(), // Should be below auth handler to prevent v2 hmac tokens from timing out while queued
 			grpctracing.UnaryServerTracingInterceptor(),
 			cache.UnaryInvalidator(diskcache.LeaseKeyer{}, protoregistry.GitalyProtoPreregistered),
@@ -121,7 +121,7 @@ func createNewServer(rubyServer *rubyserver.Server, gitlabAPI hook.GitlabAPI, cf
 	// If tls config is specified attempt to extract tls options and use it
 	// as a grpc.ServerOption
 	if secure {
-		cert, err := tls.LoadX509KeyPair(config.Config.TLS.CertPath, config.Config.TLS.KeyPath)
+		cert, err := tls.LoadX509KeyPair(cfg.TLS.CertPath, cfg.TLS.KeyPath)
 		if err != nil {
 			log.Fatalf("error reading certificate and key paths: %v", err)
 		}
diff --git a/internal/service/register.go b/internal/service/register.go
index cb55fa24f..81f73ed88 100644
--- a/internal/service/register.go
+++ b/internal/service/register.go
@@ -72,8 +72,8 @@ func RegisterAll(grpcServer *grpc.Server, cfg config.Cfg, rubyServer *rubyserver
 	gitalypb.RegisterRemoteServiceServer(grpcServer, remote.NewServer(rubyServer))
 	gitalypb.RegisterServerServiceServer(grpcServer, server.NewServer(cfg.Storages))
 	gitalypb.RegisterObjectPoolServiceServer(grpcServer, objectpool.NewServer())
-	gitalypb.RegisterHookServiceServer(grpcServer, hook.NewServer(gitlabAPI, config.Config.Hooks))
-	gitalypb.RegisterInternalGitalyServer(grpcServer, internalgitaly.NewServer(config.Config.Storages))
+	gitalypb.RegisterHookServiceServer(grpcServer, hook.NewServer(gitlabAPI, cfg.Hooks))
+	gitalypb.RegisterInternalGitalyServer(grpcServer, internalgitaly.NewServer(cfg.Storages))
 
 	healthpb.RegisterHealthServer(grpcServer, health.NewServer())
 }
diff --git a/internal/service/repository/fork_test.go b/internal/service/repository/fork_test.go
index 7f603823b..b3f97de79 100644
--- a/internal/service/repository/fork_test.go
+++ b/internal/service/repository/fork_test.go
@@ -1,19 +1,11 @@
 package repository_test
 
 import (
-	"bytes"
-	"crypto/ecdsa"
-	"crypto/elliptic"
-	"crypto/rand"
 	"crypto/x509"
-	"encoding/pem"
-	"io"
 	"io/ioutil"
-	"math/big"
 	"os"
 	"path"
 	"testing"
-	"time"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
@@ -162,79 +154,24 @@ func TestFailedCreateForkRequestDueToExistingTarget(t *testing.T) {
 }
 
 func injectCustomCATestCerts(t *testing.T) (*x509.CertPool, testhelper.Cleanup) {
-	rootCA := &x509.Certificate{
-		SerialNumber: big.NewInt(1),
-		NotBefore:    time.Now(),
-		NotAfter:     time.Now().AddDate(0, 0, 1),
-		IsCA:         true,
-		DNSNames:     []string{"localhost"},
-	}
-
-	caKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	require.NoError(t, err)
-
-	caCert, err := x509.CreateCertificate(rand.Reader, rootCA, rootCA, &caKey.PublicKey, caKey)
-	require.NoError(t, err)
-
-	entityKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
-	require.NoError(t, err)
-
-	entityX509 := &x509.Certificate{
-		SerialNumber: big.NewInt(2),
-	}
-
-	entityCert, err := x509.CreateCertificate(
-		rand.Reader, rootCA, entityX509, &entityKey.PublicKey, caKey)
-	require.NoError(t, err)
-
-	certFile, err := ioutil.TempFile("", "")
-	require.NoError(t, err)
-	defer certFile.Close()
-
-	caPEMBytes := &bytes.Buffer{}
-	certPEMWriter := io.MultiWriter(certFile, caPEMBytes)
-
-	// create chained PEM file with CA and entity cert
-	for _, cert := range [][]byte{entityCert, caCert} {
-		require.NoError(t,
-			pem.Encode(certPEMWriter, &pem.Block{
-				Type:  "CERTIFICATE",
-				Bytes: cert,
-			}),
-		)
-	}
-
-	keyFile, err := ioutil.TempFile("", "")
-	require.NoError(t, err)
-	defer keyFile.Close()
-
-	entityKeyBytes, err := x509.MarshalECPrivateKey(entityKey)
-	require.NoError(t, err)
-
-	require.NoError(t,
-		pem.Encode(keyFile, &pem.Block{
-			Type:  "ECDSA PRIVATE KEY",
-			Bytes: entityKeyBytes,
-		}),
-	)
+	certFile, keyFile, removeCerts := testhelper.GenerateTestCerts(t)
 
 	oldTLSConfig := config.Config.TLS
 
-	config.Config.TLS.CertPath = certFile.Name()
-	config.Config.TLS.KeyPath = keyFile.Name()
-
-	oldSSLCert := os.Getenv(gitaly_x509.SSLCertFile)
-	os.Setenv(gitaly_x509.SSLCertFile, certFile.Name())
+	config.Config.TLS.CertPath = certFile
+	config.Config.TLS.KeyPath = keyFile
 
+	revertEnv := testhelper.ModifyEnvironment(t, gitaly_x509.SSLCertFile, certFile)
 	cleanup := func() {
 		config.Config.TLS = oldTLSConfig
-		os.Setenv(gitaly_x509.SSLCertFile, oldSSLCert)
-		require.NoError(t, os.Remove(certFile.Name()))
-		require.NoError(t, os.Remove(keyFile.Name()))
+		revertEnv()
+		removeCerts()
 	}
 
+	caPEMBytes, err := ioutil.ReadFile(certFile)
+	require.NoError(t, err)
 	pool := x509.NewCertPool()
-	require.True(t, pool.AppendCertsFromPEM(caPEMBytes.Bytes()))
+	require.True(t, pool.AppendCertsFromPEM(caPEMBytes))
 
 	return pool, cleanup
 }
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 200b46375..5cfe083f0 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -3,12 +3,18 @@ package testhelper
 import (
 	"bytes"
 	"context"
+	"crypto/ecdsa"
+	"crypto/elliptic"
+	"crypto/rand"
 	"crypto/sha256"
+	"crypto/x509"
 	"encoding/base64"
 	"encoding/json"
+	"encoding/pem"
 	"fmt"
 	"io"
 	"io/ioutil"
+	"math/big"
 	"net"
 	"os"
 	"os/exec"
@@ -805,3 +811,86 @@ func (m *mockAPI) PreReceive(glRepository string) (bool, error) {
 
 // GitlabAPIStub is a global mock that can be used in testing
 var GitlabAPIStub = &mockAPI{}
+
+// ModifyEnvironment will change an environment variable and return a func suitable
+// for `defer` to change the value back.
+func ModifyEnvironment(t testing.TB, key string, value string) func() {
+	t.Helper()
+
+	oldValue, hasOldValue := os.LookupEnv(key)
+	require.NoError(t, os.Setenv(key, value))
+	return func() {
+		if hasOldValue {
+			require.NoError(t, os.Setenv(key, oldValue))
+		} else {
+			require.NoError(t, os.Unsetenv(key))
+		}
+	}
+}
+
+// GenerateTestCerts creates a certificate that can be used to establish TLS protected TCP connection.
+// It returns paths to the file with the certificate and its private key.
+func GenerateTestCerts(t *testing.T) (string, string, Cleanup) {
+	t.Helper()
+
+	rootCA := &x509.Certificate{
+		SerialNumber:          big.NewInt(1),
+		NotBefore:             time.Now(),
+		NotAfter:              time.Now().AddDate(0, 0, 1),
+		BasicConstraintsValid: true,
+		IsCA:                  true,
+		DNSNames:              []string{"localhost", "0.0.0.0", "127.0.0.1", "::1"},
+		KeyUsage:              x509.KeyUsageCertSign,
+	}
+
+	caKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	caCert, err := x509.CreateCertificate(rand.Reader, rootCA, rootCA, &caKey.PublicKey, caKey)
+	require.NoError(t, err)
+
+	entityKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	entityX509 := &x509.Certificate{
+		SerialNumber: big.NewInt(2),
+	}
+
+	entityCert, err := x509.CreateCertificate(rand.Reader, rootCA, entityX509, &entityKey.PublicKey, caKey)
+	require.NoError(t, err)
+
+	certFile, err := ioutil.TempFile("", "")
+	require.NoError(t, err)
+	defer certFile.Close()
+
+	// create chained PEM file with CA and entity cert
+	for _, cert := range [][]byte{entityCert, caCert} {
+		require.NoError(t,
+			pem.Encode(certFile, &pem.Block{
+				Type:  "CERTIFICATE",
+				Bytes: cert,
+			}),
+		)
+	}
+
+	keyFile, err := ioutil.TempFile("", "")
+	require.NoError(t, err)
+	defer keyFile.Close()
+
+	entityKeyBytes, err := x509.MarshalECPrivateKey(entityKey)
+	require.NoError(t, err)
+
+	require.NoError(t,
+		pem.Encode(keyFile, &pem.Block{
+			Type:  "ECDSA PRIVATE KEY",
+			Bytes: entityKeyBytes,
+		}),
+	)
+
+	cleanup := func() {
+		require.NoError(t, os.Remove(certFile.Name()))
+		require.NoError(t, os.Remove(keyFile.Name()))
+	}
+
+	return certFile.Name(), keyFile.Name(), cleanup
+}
diff --git a/internal/x509/pool_darwin.go b/internal/x509/pool_darwin.go
index c982d013f..b898c686b 100644
--- a/internal/x509/pool_darwin.go
+++ b/internal/x509/pool_darwin.go
@@ -2,6 +2,7 @@ package x509
 
 import (
 	"crypto/x509"
+	"errors"
 	"io/ioutil"
 	"os"
 	"path"
@@ -48,6 +49,8 @@ func SystemCertPool() (*x509.CertPool, error) {
 		return nil, err
 	}
 
-	pool.AppendCertsFromPEM(certPem)
+	if !pool.AppendCertsFromPEM(certPem) {
+		return nil, errors.New("certificate(s) can't be added to the system pool")
+	}
 	return pool, nil
 }