Merge branch 'jc/tls-key' into 'master'

gitaly_config: Allow passing in TLS key directly

Closes #4973

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/6285

Merged-by: Justin Tobler <jtobler@gitlab.com>
Approved-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: Justin Tobler <jtobler@gitlab.com>
Reviewed-by: karthik nayak <knayak@gitlab.com>
Co-authored-by: John Cai <jcai@gitlab.com>

8 files changed, 107 insertions, 34 deletions

diff --git a/internal/git/smudge/config_test.go b/internal/git/smudge/config_test.go
index 22e8a3414..7885ef06d 100644
--- a/internal/git/smudge/config_test.go
+++ b/internal/git/smudge/config_test.go
@@ -168,5 +168,5 @@ func TestConfig_Environment(t *testing.T) {
 
 	env, err := cfg.Environment()
 	require.NoError(t, err)
-	require.Equal(t, `GITALY_LFS_SMUDGE_CONFIG={"gl_repository":"repo","gitlab":{"url":"https://example.com","relative_url_root":"gitlab","http_settings":{"read_timeout":1,"user":"user","password":"correcthorsebatterystaple","ca_file":"/ca/file","ca_path":"/ca/path"},"secret_file":"/secret/path","secret":"secret_token"},"tls":{"cert_path":"/cert/path","key_path":"/key/path"},"driver_type":0}`, env)
+	require.Equal(t, `GITALY_LFS_SMUDGE_CONFIG={"gl_repository":"repo","gitlab":{"url":"https://example.com","relative_url_root":"gitlab","http_settings":{"read_timeout":1,"user":"user","password":"correcthorsebatterystaple","ca_file":"/ca/file","ca_path":"/ca/path"},"secret_file":"/secret/path","secret":"secret_token"},"tls":{"cert_path":"/cert/path","key_path":"/key/path","key":""},"driver_type":0}`, env)
 }
diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go
index 4e79ecff2..084dc95cc 100644
--- a/internal/gitaly/config/config.go
+++ b/internal/gitaly/config/config.go
@@ -118,17 +118,29 @@ type Cfg struct {
 type TLS struct {
 	CertPath string `toml:"certificate_path,omitempty" json:"cert_path"`
 	KeyPath  string `toml:"key_path,omitempty" json:"key_path"`
+	Key      string `toml:"key,omitempty" json:"key"`
 }
 
 // Validate runs validation on all fields and compose all found errors.
 func (t TLS) Validate() error {
-	if t.CertPath == "" && t.KeyPath == "" {
+	if t.CertPath == "" && t.KeyPath == "" && t.Key == "" {
 		return nil
 	}
 
+	if t.Key != "" && t.KeyPath != "" {
+		return cfgerror.NewValidationError(
+			errors.New("key_path and key cannot both be set"),
+			"key_path",
+			"key",
+		)
+	}
+
 	errs := cfgerror.New().
-		Append(cfgerror.FileExists(t.CertPath), "certificate_path").
-		Append(cfgerror.FileExists(t.KeyPath), "key_path")
+		Append(cfgerror.FileExists(t.CertPath), "certificate_path")
+
+	if t.Key == "" {
+		errs = errs.Append(cfgerror.FileExists(t.KeyPath), "key_path")
+	}
 
 	if len(errs) != 0 {
 		// In case of problems with files attempt to load
@@ -136,21 +148,49 @@ func (t TLS) Validate() error {
 		return errs.AsError()
 	}
 
-	if _, err := tls.LoadX509KeyPair(t.CertPath, t.KeyPath); err != nil {
-		if strings.Contains(err.Error(), "in certificate input") {
-			return cfgerror.NewValidationError(err, "certificate_path")
-		}
+	if _, err := t.Certificate(); err != nil {
+		var field string
 
-		if strings.Contains(err.Error(), "in key input") {
-			return cfgerror.NewValidationError(err, "key_path")
+		if strings.Contains(err.Error(), "in certificate input") ||
+			strings.Contains(err.Error(), "certificate_path") {
+			field = "certificate_path"
+		} else if t.Key != "" {
+			field = "key"
+		} else {
+			field = "key_path"
 		}
 
-		return cfgerror.NewValidationError(err)
+		return cfgerror.NewValidationError(err, field)
 	}
 
 	return nil
 }
 
+// Certificate gets the certificate with the certificate path and either the key
+// path or the key.
+func (t TLS) Certificate() (tls.Certificate, error) {
+	if t.Key != "" {
+		certPEMBlock, err := os.ReadFile(t.CertPath)
+		if err != nil {
+			return tls.Certificate{}, fmt.Errorf("reading certificate_path: %w", err)
+		}
+
+		cert, err := tls.X509KeyPair(certPEMBlock, []byte(t.Key))
+		if err != nil {
+			return tls.Certificate{}, fmt.Errorf("loading x509 keypair: %w", err)
+		}
+
+		return cert, nil
+	}
+
+	cert, err := tls.LoadX509KeyPair(t.CertPath, t.KeyPath)
+	if err != nil {
+		return tls.Certificate{}, fmt.Errorf("loading x509 keypair: %w", err)
+	}
+
+	return cert, nil
+}
+
 // GitlabShell contains the settings required for executing `gitlab-shell`
 type GitlabShell struct {
 	Dir string `toml:"dir" json:"dir"`
diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go
index df136d263..45d08096a 100644
--- a/internal/gitaly/config/config_test.go
+++ b/internal/gitaly/config/config_test.go
@@ -1751,6 +1751,9 @@ func TestTLS_Validate(t *testing.T) {
 	const certPath = "../server/testdata/gitalycert.pem"
 	const keyPath = "../server/testdata/gitalykey.pem"
 
+	key, err := os.ReadFile(keyPath)
+	require.NoError(t, err)
+
 	tmpDir := testhelper.TempDir(t)
 	tmpFile := filepath.Join(tmpDir, "file")
 	require.NoError(t, os.WriteFile(tmpFile, []byte("I am not a certificate"), perm.SharedFile))
@@ -1773,6 +1776,12 @@ func TestTLS_Validate(t *testing.T) {
 			},
 		},
 		{
+			name: "valid, pass in key directly",
+			setup: func(t *testing.T) TLS {
+				return TLS{CertPath: certPath, Key: string(key)}
+			},
+		},
+		{
 			name: "no cert path, bad key path",
 			setup: func(t *testing.T) TLS {
 				return TLS{CertPath: "", KeyPath: "somewhere"}
@@ -1794,25 +1803,51 @@ func TestTLS_Validate(t *testing.T) {
 				return TLS{CertPath: tmpFile, KeyPath: keyPath}
 			},
 			expectedErr: cfgerror.NewValidationError(
-				errors.New("tls: failed to find any PEM data in certificate input"),
+				errors.New("loading x509 keypair: tls: failed to find any PEM data in certificate input"),
 				"certificate_path",
 			),
 		},
 		{
-			name: "bad key",
+			name: "bad keypath",
 			setup: func(t *testing.T) TLS {
 				return TLS{CertPath: certPath, KeyPath: "config_test.go"}
 			},
 			expectedErr: cfgerror.NewValidationError(
-				errors.New("tls: failed to find any PEM data in key input"),
+				errors.New("loading x509 keypair: tls: failed to find any PEM data in key input"),
+				"key_path",
+			),
+		},
+		{
+			name: "bad key",
+			setup: func(t *testing.T) TLS {
+				return TLS{CertPath: certPath, Key: "this is not a valid key"}
+			},
+			expectedErr: cfgerror.NewValidationError(
+				errors.New("loading x509 keypair: tls: failed to find any PEM data in key input"),
+				"key",
+			),
+		},
+		{
+			name: "key and key_path both set",
+			setup: func(t *testing.T) TLS {
+				return TLS{CertPath: certPath, Key: "key data", KeyPath: "path/to/key"}
+			},
+			expectedErr: cfgerror.NewValidationError(
+				errors.New("key_path and key cannot both be set"),
 				"key_path",
+				"key",
 			),
 		},
 	} {
 		t.Run(tc.name, func(t *testing.T) {
 			tlsCfg := tc.setup(t)
 			err := tlsCfg.Validate()
-			require.Equal(t, tc.expectedErr, err)
+			if err == nil {
+				require.NoError(t, err)
+				return
+			}
+
+			require.Equal(t, tc.expectedErr.Error(), err.Error())
 		})
 	}
 }
diff --git a/internal/gitaly/server/server.go b/internal/gitaly/server/server.go
index b30ac1c83..672cef9de 100644
--- a/internal/gitaly/server/server.go
+++ b/internal/gitaly/server/server.go
@@ -69,7 +69,7 @@ func (s *GitalyServerFactory) New(secure bool, opts ...Option) (*grpc.Server, er
 	// If tls config is specified attempt to extract tls options and use it
 	// as a grpc.ServerOption
 	if secure {
-		cert, err := tls.LoadX509KeyPair(s.cfg.TLS.CertPath, s.cfg.TLS.KeyPath)
+		cert, err := s.cfg.TLS.Certificate()
 		if err != nil {
 			return nil, fmt.Errorf("error reading certificate and key paths: %w", err)
 		}
diff --git a/internal/gitlab/client/httpclient.go b/internal/gitlab/client/httpclient.go
index b15643f4a..bd37de947 100644
--- a/internal/gitlab/client/httpclient.go
+++ b/internal/gitlab/client/httpclient.go
@@ -35,21 +35,18 @@ type HTTPClient struct {
 }
 
 type httpClientCfg struct {
-	keyPath, certPath string
-	caFile, caPath    string
+	caFile, caPath string
+	cert           *tls.Certificate
 }
 
-func (hcc httpClientCfg) HaveCertAndKey() bool { return hcc.keyPath != "" && hcc.certPath != "" }
-
 // HTTPClientOpt provides options for configuring an HttpClient
 type HTTPClientOpt func(*httpClientCfg)
 
 // WithClientCert will configure the HttpClient to provide client certificates
 // when connecting to a server.
-func WithClientCert(certPath, keyPath string) HTTPClientOpt {
+func WithClientCert(cert *tls.Certificate) HTTPClientOpt {
 	return func(hcc *httpClientCfg) {
-		hcc.keyPath = keyPath
-		hcc.certPath = certPath
+		hcc.cert = cert
 	}
 }
 
@@ -155,12 +152,8 @@ func buildHTTPSTransport(hcc httpClientCfg, gitlabURL string) (*http.Transport,
 		MinVersion: tls.VersionTLS12,
 	}
 
-	if hcc.HaveCertAndKey() {
-		cert, err := tls.LoadX509KeyPair(hcc.certPath, hcc.keyPath)
-		if err != nil {
-			return nil, "", err
-		}
-		tlsConfig.Certificates = []tls.Certificate{cert}
+	if hcc.cert != nil {
+		tlsConfig.Certificates = []tls.Certificate{*hcc.cert}
 	}
 
 	transport := &http.Transport{
diff --git a/internal/gitlab/http_client.go b/internal/gitlab/http_client.go
index 094c52a76..c00efca85 100644
--- a/internal/gitlab/http_client.go
+++ b/internal/gitlab/http_client.go
@@ -43,8 +43,13 @@ func NewHTTPClient(
 	}
 
 	var opts []client.HTTPClientOpt
-	if tlsCfg.CertPath != "" && tlsCfg.KeyPath != "" {
-		opts = append(opts, client.WithClientCert(tlsCfg.CertPath, tlsCfg.KeyPath))
+	if tlsCfg.CertPath != "" && tlsCfg.KeyPath != "" ||
+		tlsCfg.Key != "" {
+		cert, err := tlsCfg.Certificate()
+		if err != nil {
+			return nil, fmt.Errorf("getting certificate: %w", err)
+		}
+		opts = append(opts, client.WithClientCert(&cert))
 	}
 
 	httpClient, err := client.NewHTTPClientWithOpts(
diff --git a/internal/praefect/server_factory.go b/internal/praefect/server_factory.go
index d02d89bc0..eae1271dd 100644
--- a/internal/praefect/server_factory.go
+++ b/internal/praefect/server_factory.go
@@ -72,7 +72,7 @@ func (s *ServerFactory) Create(secure bool) (*grpc.Server, error) {
 		return s.insecure[len(s.insecure)-1], nil
 	}
 
-	cert, err := tls.LoadX509KeyPair(s.deps.Config.TLS.CertPath, s.deps.Config.TLS.KeyPath)
+	cert, err := s.deps.Config.TLS.Certificate()
 	if err != nil {
 		return nil, fmt.Errorf("load certificate key pair: %w", err)
 	}
diff --git a/internal/praefect/server_factory_test.go b/internal/praefect/server_factory_test.go
index a0513bb79..ab1624847 100644
--- a/internal/praefect/server_factory_test.go
+++ b/internal/praefect/server_factory_test.go
@@ -313,7 +313,7 @@ func TestServerFactory(t *testing.T) {
 		praefectServerFactory := setupPraefectServerFactory(badTLSKeyPath)
 
 		err := praefectServerFactory.Serve(nil, true)
-		require.EqualError(t, err, "load certificate key pair: open invalid: no such file or directory")
+		require.EqualError(t, err, "load certificate key pair: loading x509 keypair: open invalid: no such file or directory")
 	})
 
 	t.Run("tls cert path invalid", func(t *testing.T) {
@@ -322,6 +322,6 @@ func TestServerFactory(t *testing.T) {
 		praefectServerFactory := setupPraefectServerFactory(badTLSCertPath)
 
 		err := praefectServerFactory.Serve(nil, true)
-		require.EqualError(t, err, "load certificate key pair: open invalid: no such file or directory")
+		require.EqualError(t, err, "load certificate key pair: loading x509 keypair: open invalid: no such file or directory")
 	})
 }