diff --git a/config.praefect.toml.example b/config.praefect.toml.example index 99b83fdba1086efd86e4b285db67f12f96f0730e..640754a491da1cabc4b53a5c94a86b6143beb35a 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 2b9ea85d14eecb5ffab4ee455f607687981fb59d..e1165ca20cca535e31277247254b2524fa8dd46e 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 e4c7075f5e20dbecd805c2cefaab8f4a9af0294c..8663e8dcd23f834d30b31818acd0227be988f22f 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 9977e3ccd3ff4092f27909a3013a732c60b49feb..a8168880dc29f3f22ca3c507d1b273d7f0ed0993 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 3c28d64f06cef04c4123191c3b7f53f8020598b4..9c749af3c5a0aadb0473cdefd7ab99646fe02ca5 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 eee8b0b657a981ef59713e6fa5bab170805c5a0b..10ad0200e227d265fd51bad7bac93ae31d2cfc0e 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 3a4621d40b9d1c53f131acc8891ca39a47b18816..359a0d86d286d17cae8d2cb92a1542d9811c6e5c 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 6a16fb8fd8e22fa9b13b716059668b97acb44c22..a6d1092c5e340d945d9027590019af99babe3818 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 869597f01247b9da233dd6d862622832ccf2e64d..a2d7144b027f2fb0594a432888e5151cb4b52dab 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 eae1271dd2b4c590df0ad8f85caa25827ad1a976..0e1a314b9b27e02550e28f843128f4adc7f1605d 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 d347c5c7cd52705b1f0bb10d4aaf32b9e6b07d30..b374e7daa52783cb905c0add6e4bc0b90169eb30 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.