Merge branch 'po-tls-custom-ca' into 'master'

Fix forking with custom CA in RPC CreateFork

Closes #1567

See merge request gitlab-org/gitaly!1658

11 files changed, 228 insertions, 43 deletions

diff --git a/changelogs/unreleased/po-tls-custom-ca.yml b/changelogs/unreleased/po-tls-custom-ca.yml
new file mode 100644
index 000000000..c895344e6
--- /dev/null
+++ b/changelogs/unreleased/po-tls-custom-ca.yml
@@ -0,0 +1,5 @@
+---
+title: Fix forking with custom CA in RPC CreateFork
+merge_request: 1658
+author:
+type: fixed
diff --git a/client/dial.go b/client/dial.go
index befd73942..48e082ca9 100644
--- a/client/dial.go
+++ b/client/dial.go
@@ -11,6 +11,7 @@ import (
 
 	"net/url"
 
+	gitaly_x509 "gitlab.com/gitlab-org/gitaly/internal/x509"
 	"google.golang.org/grpc"
 )
 
@@ -41,7 +42,7 @@ func Dial(rawAddress string, connOpts []grpc.DialOption) (*grpc.ClientConn, erro
 			return nil, err
 		}
 
-		certPool, err := systemCertPool()
+		certPool, err := gitaly_x509.SystemCertPool()
 		if err != nil {
 			return nil, err
 		}
diff --git a/client/pool.go b/client/pool.go
deleted file mode 100644
index e4d216427..000000000
--- a/client/pool.go
+++ /dev/null
@@ -1,8 +0,0 @@
-// +build !darwin
-
-package client
-
-import "crypto/x509"
-
-// systemCertPool has an override on macOS.
-func systemCertPool() (*x509.CertPool, error) { return x509.SystemCertPool() }
diff --git a/internal/service/repository/fetch_test.go b/internal/service/repository/fetch_test.go
index 1939dd633..43500dc2e 100644
--- a/internal/service/repository/fetch_test.go
+++ b/internal/service/repository/fetch_test.go
@@ -199,3 +199,19 @@ func runFullServer(t *testing.T) (*grpc.Server, string) {
 
 	return server, "unix://" + serverSocketPath
 }
+
+func runFullSecureServer(t *testing.T) (*grpc.Server, string, testhelper.Cleanup) {
+	server := serverPkg.NewSecure(repository.RubyServer)
+	listener, addr := testhelper.GetLocalhostListener(t)
+
+	errQ := make(chan error)
+
+	go func() { errQ <- server.Serve(listener) }()
+
+	cleanup := func() {
+		server.Stop()
+		require.NoError(t, <-errQ)
+	}
+
+	return server, "tls://" + addr, cleanup
+}
diff --git a/internal/service/repository/fork.go b/internal/service/repository/fork.go
index 314c39be3..765e261ba 100644
--- a/internal/service/repository/fork.go
+++ b/internal/service/repository/fork.go
@@ -10,6 +10,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	gitaly_x509 "gitlab.com/gitlab-org/gitaly/internal/x509"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/labkit/tracing"
 	"google.golang.org/grpc/codes"
@@ -75,6 +76,10 @@ func (s *server) CreateFork(ctx context.Context, req *gitalypb.CreateForkRequest
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_TOKEN=%s", sourceRepositoryGitalyToken),
 		fmt.Sprintf("GIT_SSH_COMMAND=%s upload-pack", gitalySSHPath),
+		// Pass through the SSL_CERT_* variables that indicate which
+		// system certs to trust
+		fmt.Sprintf("%s=%s", gitaly_x509.SSLCertDir, os.Getenv(gitaly_x509.SSLCertDir)),
+		fmt.Sprintf("%s=%s", gitaly_x509.SSLCertFile, os.Getenv(gitaly_x509.SSLCertFile)),
 	}
 	env = envInjector(ctx, env)
 
diff --git a/internal/service/repository/fork_test.go b/internal/service/repository/fork_test.go
index 9e595bb3c..d15a6f637 100644
--- a/internal/service/repository/fork_test.go
+++ b/internal/service/repository/fork_test.go
@@ -1,61 +1,102 @@
 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"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/service/repository"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
+	gitaly_x509 "gitlab.com/gitlab-org/gitaly/internal/x509"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/metadata"
 )
 
 func TestSuccessfulCreateForkRequest(t *testing.T) {
-	server, serverSocketPath := runFullServer(t)
-	defer server.Stop()
+	for _, tt := range []struct {
+		name   string
+		secure bool
+	}{
+		{name: "secure", secure: true},
+		{name: "insecure"},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			var (
+				server           *grpc.Server
+				serverSocketPath string
+				client           gitalypb.RepositoryServiceClient
+				conn             *grpc.ClientConn
+			)
+
+			if tt.secure {
+				testPool, sslCleanup := injectCustomCATestCerts(t)
+				defer sslCleanup()
+
+				var serverCleanup testhelper.Cleanup
+				_, serverSocketPath, serverCleanup = runFullSecureServer(t)
+				defer serverCleanup()
+
+				client, conn = repository.NewSecureRepoClient(t, serverSocketPath, testPool)
+				defer conn.Close()
+			} else {
+				server, serverSocketPath = runFullServer(t)
+				defer server.Stop()
 
-	client, conn := repository.NewRepositoryClient(t, serverSocketPath)
-	defer conn.Close()
+				client, conn = repository.NewRepositoryClient(t, serverSocketPath)
+				defer conn.Close()
+			}
 
-	ctxOuter, cancel := testhelper.Context()
-	defer cancel()
+			ctxOuter, cancel := testhelper.Context()
+			defer cancel()
 
-	md := testhelper.GitalyServersMetadata(t, serverSocketPath)
-	ctx := metadata.NewOutgoingContext(ctxOuter, md)
+			md := testhelper.GitalyServersMetadata(t, serverSocketPath)
+			ctx := metadata.NewOutgoingContext(ctxOuter, md)
 
-	testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
-	defer cleanupFn()
+			testRepo, _, cleanupFn := testhelper.NewTestRepo(t)
+			defer cleanupFn()
 
-	forkedRepo := &gitalypb.Repository{
-		RelativePath: "forks/test-repo-fork.git",
-		StorageName:  testRepo.StorageName,
-	}
+			forkedRepo := &gitalypb.Repository{
+				RelativePath: "forks/test-repo-fork.git",
+				StorageName:  testRepo.StorageName,
+			}
 
-	forkedRepoPath, err := helper.GetPath(forkedRepo)
-	require.NoError(t, err)
-	require.NoError(t, os.RemoveAll(forkedRepoPath))
+			forkedRepoPath, err := helper.GetPath(forkedRepo)
+			require.NoError(t, err)
+			require.NoError(t, os.RemoveAll(forkedRepoPath))
 
-	req := &gitalypb.CreateForkRequest{
-		Repository:       forkedRepo,
-		SourceRepository: testRepo,
-	}
+			req := &gitalypb.CreateForkRequest{
+				Repository:       forkedRepo,
+				SourceRepository: testRepo,
+			}
 
-	_, err = client.CreateFork(ctx, req)
-	require.NoError(t, err)
+			_, err = client.CreateFork(ctx, req)
+			require.NoError(t, err)
 
-	testhelper.MustRunCommand(t, nil, "git", "-C", forkedRepoPath, "fsck")
+			testhelper.MustRunCommand(t, nil, "git", "-C", forkedRepoPath, "fsck")
 
-	remotes := testhelper.MustRunCommand(t, nil, "git", "-C", forkedRepoPath, "remote")
-	require.NotContains(t, string(remotes), "origin")
+			remotes := testhelper.MustRunCommand(t, nil, "git", "-C", forkedRepoPath, "remote")
+			require.NotContains(t, string(remotes), "origin")
 
-	info, err := os.Lstat(path.Join(forkedRepoPath, "hooks"))
-	require.NoError(t, err)
-	require.NotEqual(t, 0, info.Mode()&os.ModeSymlink)
+			info, err := os.Lstat(path.Join(forkedRepoPath, "hooks"))
+			require.NoError(t, err)
+			require.NotEqual(t, 0, info.Mode()&os.ModeSymlink)
+		})
+	}
 }
 
 func TestFailedCreateForkRequestDueToExistingTarget(t *testing.T) {
@@ -118,3 +159,81 @@ 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 := new(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,
+		}),
+	)
+
+	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())
+
+	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()))
+	}
+
+	pool := x509.NewCertPool()
+	require.True(t, pool.AppendCertsFromPEM(caPEMBytes.Bytes()))
+
+	return pool, cleanup
+}
diff --git a/internal/service/repository/testhelper_test.go b/internal/service/repository/testhelper_test.go
index 4c3600da8..8c9d73d7a 100644
--- a/internal/service/repository/testhelper_test.go
+++ b/internal/service/repository/testhelper_test.go
@@ -1,6 +1,7 @@
 package repository
 
 import (
+	"crypto/x509"
 	"log"
 	"net"
 	"os"
@@ -10,6 +11,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
+	"gitlab.com/gitlab-org/gitaly/client"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/server/auth"
@@ -17,6 +19,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/credentials"
 	"google.golang.org/grpc/reflection"
 )
 
@@ -44,6 +47,22 @@ func newRepositoryClient(t *testing.T, serverSocketPath string) (gitalypb.Reposi
 var NewRepositoryClient = newRepositoryClient
 var RunRepoServer = runRepoServer
 
+func newSecureRepoClient(t *testing.T, serverSocketPath string, pool *x509.CertPool) (gitalypb.RepositoryServiceClient, *grpc.ClientConn) {
+	connOpts := []grpc.DialOption{
+		grpc.WithTransportCredentials(credentials.NewClientTLSFromCert(pool, "")),
+		grpc.WithPerRPCCredentials(gitalyauth.RPCCredentials(testhelper.RepositoryAuthToken)),
+	}
+
+	conn, err := client.Dial(serverSocketPath, connOpts)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	return gitalypb.NewRepositoryServiceClient(conn), conn
+}
+
+var NewSecureRepoClient = newSecureRepoClient
+
 func runRepoServer(t *testing.T) (*grpc.Server, string) {
 	streamInt := []grpc.StreamServerInterceptor{auth.StreamServerInterceptor(config.Config.Auth)}
 	unaryInt := []grpc.UnaryServerInterceptor{auth.UnaryServerInterceptor(config.Config.Auth)}
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index ba12a90d4..b18070fc1 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -8,6 +8,7 @@ import (
 	"fmt"
 	"io"
 	"io/ioutil"
+	"net"
 	"os"
 	"os/exec"
 	"path"
@@ -279,6 +280,17 @@ func GetTemporaryGitalySocketFileName() string {
 	return name
 }
 
+// GetLocalhostListener listens on the next available TCP port and returns
+// the listener and the localhost address (host:port) string.
+func GetLocalhostListener(t testing.TB) (net.Listener, string) {
+	l, err := net.Listen("tcp", "localhost:0")
+	require.NoError(t, err)
+
+	addr := fmt.Sprintf("localhost:%d", l.Addr().(*net.TCPAddr).Port)
+
+	return l, addr
+}
+
 // ConfigureRuby configures Ruby settings for test purposes at run time.
 func ConfigureRuby() error {
 	if dir := os.Getenv("GITALY_TEST_RUBY_DIR"); len(dir) > 0 {
diff --git a/internal/x509/common.go b/internal/x509/common.go
new file mode 100644
index 000000000..8ca72d208
--- /dev/null
+++ b/internal/x509/common.go
@@ -0,0 +1,8 @@
+package x509
+
+// SSLCertDir and SSLCertFile are the environment variables that specify the
+// location to add system root certificates.
+const (
+	SSLCertDir  = "SSL_CERT_DIR"
+	SSLCertFile = "SSL_CERT_FILE"
+)
diff --git a/internal/x509/pool.go b/internal/x509/pool.go
new file mode 100644
index 000000000..487f0ee54
--- /dev/null
+++ b/internal/x509/pool.go
@@ -0,0 +1,8 @@
+// +build !darwin
+
+package x509
+
+import "crypto/x509"
+
+// SystemCertPool has an override on macOS.
+func SystemCertPool() (*x509.CertPool, error) { return x509.SystemCertPool() }
diff --git a/client/pool_darwin.go b/internal/x509/pool_darwin.go
index 62589ce28..c982d013f 100644
--- a/client/pool_darwin.go
+++ b/internal/x509/pool_darwin.go
@@ -1,4 +1,4 @@
-package client
+package x509
 
 import (
 	"crypto/x509"
@@ -7,12 +7,12 @@ import (
 	"path"
 )
 
-// systemCertPool circumvents the fact that Go on macOS does not support
+// SystemCertPool circumvents the fact that Go on macOS does not support
 // SSL_CERT_{DIR,FILE}.
-func systemCertPool() (*x509.CertPool, error) {
+func SystemCertPool() (*x509.CertPool, error) {
 	var certPem []byte
 
-	if f := os.Getenv("SSL_CERT_FILE"); len(f) > 0 {
+	if f := os.Getenv(SSLCertFile); len(f) > 0 {
 		pem, err := ioutil.ReadFile(f)
 		if err != nil {
 			return nil, err
@@ -22,7 +22,7 @@ func systemCertPool() (*x509.CertPool, error) {
 		certPem = append(certPem, pem...)
 	}
 
-	if d := os.Getenv("SSL_CERT_DIR"); len(d) > 0 {
+	if d := os.Getenv(SSLCertDir); len(d) > 0 {
 		entries, err := ioutil.ReadDir(d)
 		if err != nil {
 			return nil, err