diff options
author | Pavlo Strokov <pstrokov@gitlab.com> | 2020-06-18 08:55:10 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2020-06-18 08:55:10 +0300 |
commit | 4290807efcf2de64d5c1e8abce15399361da42f9 (patch) | |
tree | f4ca8746b1d16059416eda50a2ad6e2dfb138943 | |
parent | 15031e59ac28fe540191d450feaf0684688c0b28 (diff) |
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
-rw-r--r-- | client/dial_test.go | 18 | ||||
-rw-r--r-- | config.toml.example | 2 | ||||
-rw-r--r-- | internal/server/server.go | 6 | ||||
-rw-r--r-- | internal/service/register.go | 4 | ||||
-rw-r--r-- | internal/service/repository/fork_test.go | 81 | ||||
-rw-r--r-- | internal/testhelper/testhelper.go | 89 | ||||
-rw-r--r-- | internal/x509/pool_darwin.go | 5 |
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 } |