From 10190e8b16ef3db613562a78ee054b86ce258b7c Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Tue, 1 Apr 2025 14:02:06 +0300 Subject: [PATCH] Allow configuring minimum offered TLS version Gitaly and Praefect currently hard code the minimum TLS version offered to TLS 1.2. This commit makes it possible to configure the minimum TLS version offered to clients. This allows for running Gitaly in environments that require a later TLS version to be used than TLS 1.2. --- config.praefect.toml.example | 2 + config.toml.example | 2 + internal/gitaly/config/config.go | 38 ++++++++++++++ internal/gitaly/config/config_test.go | 42 +++++++++++++++ internal/gitaly/server/server.go | 2 +- internal/gitaly/server/server_factory_test.go | 51 ++++++++++++++++--- internal/praefect/config/config.go | 1 + internal/praefect/config/config_test.go | 8 ++- .../testdata/config.overwritedefaults.toml | 3 ++ internal/praefect/server_factory.go | 2 +- internal/testhelper/certs.go | 9 +++- 11 files changed, 148 insertions(+), 12 deletions(-) diff --git a/config.praefect.toml.example b/config.praefect.toml.example index 99b83fdba1..640754a491 100644 --- a/config.praefect.toml.example +++ b/config.praefect.toml.example @@ -8,6 +8,8 @@ listen_addr = "127.0.0.1:2305" # [tls] # certificate_path = '/home/git/cert.cert' # key_path = '/home/git/key.pem' +# Minimum offered TLS version. Options: ["TLS 1.2", "TLS 1.3"]. +# min_version = "TLS 1.2" # # Praefect can listen on a socket when placed on the same machine as all clients # socket_path = "/home/git/gitlab/tmp/sockets/private/praefect.socket" diff --git a/config.toml.example b/config.toml.example index 2b9ea85d14..e1165ca20c 100644 --- a/config.toml.example +++ b/config.toml.example @@ -38,6 +38,8 @@ bin_dir = "/home/git/gitaly/_build/bin" # certificate_path = '/home/git/cert.cert' # # Path to the key. # key_path = '/home/git/key.pem' +# Minimum offered TLS version. Options: ["TLS 1.2", "TLS 1.3"]. +# min_version = "TLS 1.2" # # Git settings # [git] diff --git a/internal/gitaly/config/config.go b/internal/gitaly/config/config.go index e4c7075f5e..8663e8dcd2 100644 --- a/internal/gitaly/config/config.go +++ b/internal/gitaly/config/config.go @@ -165,13 +165,50 @@ type TimeoutConfig struct { UploadArchiveNegotiation duration.Duration `json:"upload_archive_negotiation,omitempty" toml:"upload_archive_negotiation,omitempty"` } +// TLSVersion represents a version of the TLS protocol. +type TLSVersion uint16 + +// UnmarshalText unmarshals a string representation of a version. +func (v *TLSVersion) UnmarshalText(data []byte) error { + switch string(data) { + case "TLS 1.2": + *v = tls.VersionTLS12 + case "TLS 1.3": + *v = tls.VersionTLS13 + default: + return fmt.Errorf("unsupported TLS version: %q", data) + } + + return nil +} + +// MarshalText returns the human readable text representation of the protocol version. +func (v TLSVersion) MarshalText() ([]byte, error) { + if v == 0 { + // tls.VersionName below returns the protocol number as 0x0000 if it's unconfigured. + // Special case so omitempty applies correctly and the field is not marshaled out when + // unconfigured. + return nil, nil + } + + return []byte(tls.VersionName(v.ProtocolVersion())), nil +} + +// ProtocolVersion returns the version as used by the protocol. +func (v TLSVersion) ProtocolVersion() uint16 { return uint16(v) } + // TLS configuration type TLS struct { CertPath string `json:"cert_path" toml:"certificate_path,omitempty"` KeyPath string `json:"key_path" toml:"key_path,omitempty"` Key string `json:"key" toml:"key,omitempty"` + // MinVersion configures the minimum offered TLS version. + MinVersion TLSVersion `json:"min_version,omitempty" toml:"min_version,omitempty"` } +// NewTLS returns a new TLS configuration with defaults configured. +func NewTLS() TLS { return TLS{MinVersion: tls.VersionTLS12} } + // Validate runs validation on all fields and compose all found errors. func (t TLS) Validate() error { if t.CertPath == "" && t.KeyPath == "" && t.Key == "" { @@ -703,6 +740,7 @@ func Load(file io.Reader) (Cfg, error) { Prometheus: prometheus.DefaultConfig(), PackObjectsCache: defaultPackObjectsCacheConfig(), PackObjectsLimiting: defaultPackObjectsLimiting(), + TLS: NewTLS(), } if err := toml.NewDecoder(file).Decode(&cfg); err != nil { diff --git a/internal/gitaly/config/config_test.go b/internal/gitaly/config/config_test.go index 9977e3ccd3..a8168880dc 100644 --- a/internal/gitaly/config/config_test.go +++ b/internal/gitaly/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "crypto/tls" "errors" "fmt" "net/url" @@ -59,6 +60,7 @@ func TestLoadEmptyConfig(t *testing.T) { Prometheus: prometheus.DefaultConfig(), PackObjectsCache: defaultPackObjectsCacheConfig(), PackObjectsLimiting: defaultPackObjectsLimiting(), + TLS: NewTLS(), } require.NoError(t, expectedCfg.Sanitize()) @@ -70,6 +72,45 @@ func TestLoadEmptyConfig(t *testing.T) { require.Equal(t, expectedCfg, cfg) } +func TestLoadTLS(t *testing.T) { + for _, tc := range []struct { + desc string + configuredVersion string + expectedProtocolVersion uint16 + expectedErrorMessage string + }{ + { + desc: "invalid version", + configuredVersion: "TLS 1.1", + expectedErrorMessage: `load toml: toml: unsupported TLS version: "TLS 1.1"`, + }, + { + desc: "TLS 1.2", + configuredVersion: "TLS 1.2", + expectedProtocolVersion: tls.VersionTLS12, + }, + { + desc: "TLS 1.3", + configuredVersion: "TLS 1.3", + expectedProtocolVersion: tls.VersionTLS13, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + cfg, err := Load(strings.NewReader(fmt.Sprintf(` + [tls] + min_version = %q + `, tc.configuredVersion))) + if tc.expectedErrorMessage != "" { + require.EqualError(t, err, tc.expectedErrorMessage) + return + } + + require.NoError(t, err) + require.Equal(t, tc.expectedProtocolVersion, cfg.TLS.MinVersion.ProtocolVersion()) + }) + } +} + func TestTimeout(t *testing.T) { t.Parallel() @@ -267,6 +308,7 @@ func TestLoadConfigCommand(t *testing.T) { Prometheus: prometheus.DefaultConfig(), PackObjectsCache: defaultPackObjectsCacheConfig(), PackObjectsLimiting: defaultPackObjectsLimiting(), + TLS: NewTLS(), } require.NoError(t, cfg.Sanitize()) modify(cfg) diff --git a/internal/gitaly/server/server.go b/internal/gitaly/server/server.go index 3c28d64f06..9c749af3c5 100644 --- a/internal/gitaly/server/server.go +++ b/internal/gitaly/server/server.go @@ -79,7 +79,7 @@ func (s *GitalyServerFactory) New(external, secure bool, opts ...Option) (*grpc. transportCredentials = credentials.NewTLS(&tls.Config{ Certificates: []tls.Certificate{cert}, - MinVersion: tls.VersionTLS12, + MinVersion: s.cfg.TLS.MinVersion.ProtocolVersion(), CipherSuites: secureCiphers, }) } diff --git a/internal/gitaly/server/server_factory_test.go b/internal/gitaly/server/server_factory_test.go index eee8b0b657..10ad0200e2 100644 --- a/internal/gitaly/server/server_factory_test.go +++ b/internal/gitaly/server/server_factory_test.go @@ -2,6 +2,7 @@ package server import ( "context" + "crypto/tls" "errors" "net" "os" @@ -20,6 +21,7 @@ import ( "golang.org/x/sync/errgroup" "google.golang.org/grpc" "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/health" healthpb "google.golang.org/grpc/health/grpc_health_v1" @@ -31,11 +33,11 @@ func TestGitalyServerFactory(t *testing.T) { grp, ctx := errgroup.WithContext(ctx) t.Cleanup(func() { assert.NoError(t, grp.Wait()) }) - checkHealth := func(t *testing.T, sf *GitalyServerFactory, schema, addr string, cert *testhelper.Certificate) healthpb.HealthClient { + newHealthClient := func(t *testing.T, sf *GitalyServerFactory, schema, addr string, creds credentials.TransportCredentials) healthpb.HealthClient { t.Helper() var cc *grpc.ClientConn - if cert != nil { + if creds != nil { srv, err := sf.CreateExternal(true) require.NoError(t, err) t.Cleanup(srv.Stop) @@ -46,8 +48,6 @@ func TestGitalyServerFactory(t *testing.T) { require.NoError(t, err) grp.Go(func() error { return srv.Serve(listener) }) - creds := cert.TransportCredentials(t) - cc, err = grpc.NewClient(listener.Addr().String(), grpc.WithTransportCredentials(creds)) require.NoError(t, err) } else { @@ -69,7 +69,12 @@ func TestGitalyServerFactory(t *testing.T) { } t.Cleanup(func() { assert.NoError(t, cc.Close()) }) - healthClient := healthpb.NewHealthClient(cc) + return healthpb.NewHealthClient(cc) + } + + checkHealth := func(t *testing.T, sf *GitalyServerFactory, schema, addr string, creds credentials.TransportCredentials) healthpb.HealthClient { + healthClient := newHealthClient(t, sf, schema, addr, creds) + resp, err := healthClient.Check(ctx, &healthpb.HealthCheckRequest{}) require.NoError(t, err) require.Equal(t, healthpb.HealthCheckResponse_SERVING, resp.GetStatus()) @@ -109,7 +114,41 @@ func TestGitalyServerFactory(t *testing.T) { ) t.Cleanup(sf.Stop) - checkHealth(t, sf, starter.TLS, "localhost:0", &certificate) + checkHealth(t, sf, starter.TLS, "localhost:0", certificate.TransportCredentials(t)) + }) + + t.Run("secure with minimum protocol version", func(t *testing.T) { + certificate := testhelper.GenerateCertificate(t) + + cfg := testcfg.Build(t, testcfg.WithBase(config.Cfg{TLS: config.TLS{ + CertPath: certificate.CertPath, + KeyPath: certificate.KeyPath, + MinVersion: tls.VersionTLS13, + }})) + + sf := NewGitalyServerFactory( + cfg, + testhelper.SharedLogger(t), + backchannel.NewRegistry(), + cache.New(cfg, config.NewLocator(cfg), testhelper.SharedLogger(t)), + nil, + TransactionMiddleware{}, + ) + t.Cleanup(sf.Stop) + + t.Run("unsupported client", func(t *testing.T) { + tlsCfg := certificate.TLSConfig(t) + tlsCfg.MaxVersion = tls.VersionTLS12 + + client := newHealthClient(t, sf, starter.TLS, "localhost:0", credentials.NewTLS(tlsCfg)) + + _, err := client.Check(ctx, &healthpb.HealthCheckRequest{}) + testhelper.RequireGrpcError(t, status.Error(codes.Unavailable, `connection error: desc = "transport: authentication handshake failed: remote error: tls: protocol version not supported"`), err) + }) + + t.Run("supported client", func(t *testing.T) { + checkHealth(t, sf, starter.TLS, "localhost:0", certificate.TransportCredentials(t)) + }) }) t.Run("all services must be stopped", func(t *testing.T) { diff --git a/internal/praefect/config/config.go b/internal/praefect/config/config.go index 3a4621d40b..359a0d86d2 100644 --- a/internal/praefect/config/config.go +++ b/internal/praefect/config/config.go @@ -324,6 +324,7 @@ func FromReader(reader io.Reader) (Config, error) { RepositoriesCleanup: DefaultRepositoriesCleanup(), Yamux: DefaultYamuxConfig(), Logging: DefaultLoggingConfig(), + TLS: config.NewTLS(), } if err := toml.NewDecoder(reader).Decode(conf); err != nil { return Config{}, err diff --git a/internal/praefect/config/config_test.go b/internal/praefect/config/config_test.go index 6a16fb8fd8..a6d1092c5e 100644 --- a/internal/praefect/config/config_test.go +++ b/internal/praefect/config/config_test.go @@ -2,6 +2,7 @@ package config import ( "bytes" + "crypto/tls" "errors" "fmt" "os" @@ -268,8 +269,9 @@ func TestConfigParsing(t *testing.T) { expected: Config{ TLSListenAddr: "0.0.0.0:2306", TLS: config.TLS{ - CertPath: "/home/git/cert.cert", - KeyPath: "/home/git/key.pem", + CertPath: "/home/git/cert.cert", + KeyPath: "/home/git/key.pem", + MinVersion: tls.VersionTLS12, }, Logging: log.Config{ Level: "info", @@ -381,6 +383,7 @@ func TestConfigParsing(t *testing.T) { BackgroundVerification: DefaultBackgroundVerificationConfig(), Yamux: DefaultYamuxConfig(), Logging: DefaultLoggingConfig(), + TLS: config.TLS{MinVersion: tls.VersionTLS13}, }, }, { @@ -405,6 +408,7 @@ func TestConfigParsing(t *testing.T) { BackgroundVerification: DefaultBackgroundVerificationConfig(), Yamux: DefaultYamuxConfig(), Logging: DefaultLoggingConfig(), + TLS: config.NewTLS(), }, }, { diff --git a/internal/praefect/config/testdata/config.overwritedefaults.toml b/internal/praefect/config/testdata/config.overwritedefaults.toml index 869597f012..a2d7144b02 100644 --- a/internal/praefect/config/testdata/config.overwritedefaults.toml +++ b/internal/praefect/config/testdata/config.overwritedefaults.toml @@ -17,3 +17,6 @@ monitor_interval = "10s" check_interval = "1s" run_interval = "4s" repositories_in_batch = 11 + +[tls] +min_version = "TLS 1.3" diff --git a/internal/praefect/server_factory.go b/internal/praefect/server_factory.go index eae1271dd2..0e1a314b9b 100644 --- a/internal/praefect/server_factory.go +++ b/internal/praefect/server_factory.go @@ -86,7 +86,7 @@ func (s *ServerFactory) Create(secure bool) (*grpc.Server, error) { s.secure = append(s.secure, s.createGRPC(credentials.NewTLS(&tls.Config{ Certificates: []tls.Certificate{cert}, - MinVersion: tls.VersionTLS12, + MinVersion: s.deps.Config.TLS.MinVersion.ProtocolVersion(), CipherSuites: secureCiphers, }))) diff --git a/internal/testhelper/certs.go b/internal/testhelper/certs.go index d347c5c7cd..b374e7daa5 100644 --- a/internal/testhelper/certs.go +++ b/internal/testhelper/certs.go @@ -103,10 +103,15 @@ func GenerateCertificate(tb testing.TB) Certificate { // TransportCredentials creates new transport credentials that contain the generated certificates. func (c Certificate) TransportCredentials(tb testing.TB) credentials.TransportCredentials { - return credentials.NewTLS(&tls.Config{ + return credentials.NewTLS(c.TLSConfig(tb)) +} + +// TLSConfig returns TLS configuration that contains the certificate. +func (c Certificate) TLSConfig(tb testing.TB) *tls.Config { + return &tls.Config{ RootCAs: c.CertPool(tb), MinVersion: tls.VersionTLS12, - }) + } } // CertPool creates a new certificate pool containing the certificate. -- GitLab