From 23c3143b55e173b6ae22648b78cbdcee8056ef58 Mon Sep 17 00:00:00 2001 From: ZhenLian Date: Tue, 15 Nov 2022 15:06:50 -0800 Subject: [PATCH] advancedtls: add min/max TLS version option Adding an "TlsVersionOption" for users to select their desired min/max TLS versions, if advanced TLS is used, per request by #5667 RELEASE NOTES: security/advancedtls: add min/max TLS version selection options --- security/advancedtls/advancedtls.go | 28 ++++ .../advancedtls_integration_test.go | 156 +++++++++++++++++- security/advancedtls/advancedtls_test.go | 30 ++++ 3 files changed, 206 insertions(+), 8 deletions(-) diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index 9a33bd583f6e..4b5d1f4825c9 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -184,6 +184,15 @@ type ClientOptions struct { // RevocationConfig is the configurations for certificate revocation checks. // It could be nil if such checks are not needed. RevocationConfig *RevocationConfig + // MinVersion contains the minimum TLS version that is acceptable. + // By default, TLS 1.2 is currently used as the minimum when acting as a + // client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum + // supported by this package, both as a client and as a server. + MinVersion uint16 + // MaxVersion contains the maximum TLS version that is acceptable. + // By default, the maximum version supported by this package is used, + // which is currently TLS 1.3. + MaxVersion uint16 } // ServerOptions contains the fields needed to be filled by the server. @@ -205,6 +214,15 @@ type ServerOptions struct { // RevocationConfig is the configurations for certificate revocation checks. // It could be nil if such checks are not needed. RevocationConfig *RevocationConfig + // MinVersion contains the minimum TLS version that is acceptable. + // By default, TLS 1.2 is currently used as the minimum when acting as a + // client, and TLS 1.0 when acting as a server. TLS 1.0 is the minimum + // supported by this package, both as a client and as a server. + MinVersion uint16 + // MaxVersion contains the maximum TLS version that is acceptable. + // By default, the maximum version supported by this package is used, + // which is currently TLS 1.3. + MaxVersion uint16 } func (o *ClientOptions) config() (*tls.Config, error) { @@ -222,11 +240,16 @@ func (o *ClientOptions) config() (*tls.Config, error) { if o.IdentityOptions.GetIdentityCertificatesForServer != nil { return nil, fmt.Errorf("GetIdentityCertificatesForServer cannot be specified on the client side") } + if o.MinVersion > o.MaxVersion { + return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version") + } config := &tls.Config{ ServerName: o.ServerNameOverride, // We have to set InsecureSkipVerify to true to skip the default checks and // use the verification function we built from buildVerifyFunc. InsecureSkipVerify: true, + MinVersion: o.MinVersion, + MaxVersion: o.MaxVersion, } // Propagate root-certificate-related fields in tls.Config. switch { @@ -293,6 +316,9 @@ func (o *ServerOptions) config() (*tls.Config, error) { if o.IdentityOptions.GetIdentityCertificatesForClient != nil { return nil, fmt.Errorf("GetIdentityCertificatesForClient cannot be specified on the server side") } + if o.MinVersion > o.MaxVersion { + return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version") + } clientAuth := tls.NoClientCert if o.RequireClientCert { // We have to set clientAuth to RequireAnyClientCert to force underlying @@ -302,6 +328,8 @@ func (o *ServerOptions) config() (*tls.Config, error) { } config := &tls.Config{ ClientAuth: clientAuth, + MinVersion: o.MinVersion, + MaxVersion: o.MaxVersion, } // Propagate root-certificate-related fields in tls.Config. switch { diff --git a/security/advancedtls/advancedtls_integration_test.go b/security/advancedtls/advancedtls_integration_test.go index d5a620d14f96..4cc2c0034fc8 100644 --- a/security/advancedtls/advancedtls_integration_test.go +++ b/security/advancedtls/advancedtls_integration_test.go @@ -731,13 +731,12 @@ func (s) TestDefaultHostNameCheck(t *testing.T) { t.Fatalf("cs.LoadCerts() failed, err: %v", err) } for _, test := range []struct { - desc string - clientRoot *x509.CertPool - clientVerifyFunc CustomVerificationFunc - clientVType VerificationType - serverCert []tls.Certificate - serverVType VerificationType - expectError bool + desc string + clientRoot *x509.CertPool + clientVType VerificationType + serverCert []tls.Certificate + serverVType VerificationType + expectError bool }{ // Client side sets vType to CertAndHostVerification, and will do // default hostname check. Server uses a cert without "localhost" or @@ -787,7 +786,6 @@ func (s) TestDefaultHostNameCheck(t *testing.T) { pb.RegisterGreeterServer(s, greeterServer{}) go s.Serve(lis) clientOptions := &ClientOptions{ - VerifyPeer: test.clientVerifyFunc, RootOptions: RootCertificateOptions{ RootCACerts: test.clientRoot, }, @@ -811,3 +809,145 @@ func (s) TestDefaultHostNameCheck(t *testing.T) { }) } } + +func (s) TestTLSVersions(t *testing.T) { + cs := &testutils.CertStore{} + if err := cs.LoadCerts(); err != nil { + t.Fatalf("cs.LoadCerts() failed, err: %v", err) + } + for _, test := range []struct { + desc string + expectError bool + clientMinVersion uint16 + clientMaxVersion uint16 + serverMinVersion uint16 + serverMaxVersion uint16 + }{ + // Client side sets TLS version that is higher than required from the server side. + { + desc: "Client TLS version higher than server", + clientMinVersion: tls.VersionTLS13, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS12, + expectError: true, + }, + // Server side sets TLS version that is higher than required from the client side. + { + desc: "Server TLS version higher than client", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS12, + serverMinVersion: tls.VersionTLS13, + serverMaxVersion: tls.VersionTLS13, + expectError: true, + }, + // Client and server set proper TLS versions. + { + desc: "Good TLS version settings", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS13, + expectError: false, + }, + { + desc: "Client 1.2 - 1.3 and server 1.2", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS12, + expectError: false, + }, + { + desc: "Client 1.2 - 1.3 and server 1.1 - 1.2", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS11, + serverMaxVersion: tls.VersionTLS12, + expectError: false, + }, + { + desc: "Client 1.2 - 1.3 and server 1.3", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS13, + serverMaxVersion: tls.VersionTLS13, + expectError: false, + }, + { + desc: "Client 1.2 - 1.2 and server 1.2 - 1.3", + clientMinVersion: tls.VersionTLS12, + clientMaxVersion: tls.VersionTLS12, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS13, + expectError: false, + }, + { + desc: "Client 1.1 - 1.2 and server 1.2 - 1.3", + clientMinVersion: tls.VersionTLS11, + clientMaxVersion: tls.VersionTLS12, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS13, + expectError: false, + }, + { + desc: "Client 1.3 and server 1.2 - 1.3", + clientMinVersion: tls.VersionTLS13, + clientMaxVersion: tls.VersionTLS13, + serverMinVersion: tls.VersionTLS12, + serverMaxVersion: tls.VersionTLS13, + expectError: false, + }, + } { + test := test + t.Run(test.desc, func(t *testing.T) { + // Start a server using ServerOptions in another goroutine. + serverOptions := &ServerOptions{ + IdentityOptions: IdentityCertificateOptions{ + Certificates: []tls.Certificate{cs.ServerPeerLocalhost1}, + }, + RequireClientCert: false, + VType: CertAndHostVerification, + MinVersion: test.serverMinVersion, + MaxVersion: test.serverMaxVersion, + } + serverTLSCreds, err := NewServerCreds(serverOptions) + if err != nil { + t.Fatalf("failed to create server creds: %v", err) + } + s := grpc.NewServer(grpc.Creds(serverTLSCreds)) + defer s.Stop() + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("failed to listen: %v", err) + } + defer lis.Close() + addr := fmt.Sprintf("localhost:%v", lis.Addr().(*net.TCPAddr).Port) + pb.RegisterGreeterServer(s, greeterServer{}) + go s.Serve(lis) + clientOptions := &ClientOptions{ + RootOptions: RootCertificateOptions{ + RootCACerts: cs.ClientTrust1, + }, + VType: CertAndHostVerification, + MinVersion: test.clientMinVersion, + MaxVersion: test.clientMaxVersion, + } + clientTLSCreds, err := NewClientCreds(clientOptions) + if err != nil { + t.Fatalf("clientTLSCreds failed to create: %v", err) + } + shouldFail := false + if test.expectError { + shouldFail = true + } + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() + conn, _, err := callAndVerifyWithClientConn(ctx, addr, "rpc call 1", clientTLSCreds, shouldFail) + if err != nil { + t.Fatal(err) + } + defer conn.Close() + }) + } +} diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go index 7092d46e60fa..afad25e7cb4b 100644 --- a/security/advancedtls/advancedtls_test.go +++ b/security/advancedtls/advancedtls_test.go @@ -91,6 +91,8 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) { clientVType VerificationType IdentityOptions IdentityCertificateOptions RootOptions RootCertificateOptions + MinVersion uint16 + MaxVersion uint16 }{ { desc: "Skip default verification and provide no root credentials", @@ -122,6 +124,11 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) { }, }, }, + { + desc: "Invalid min/max TLS versions", + MinVersion: tls.VersionTLS13, + MaxVersion: tls.VersionTLS12, + }, } for _, test := range tests { test := test @@ -130,6 +137,8 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) { VType: test.clientVType, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, + MinVersion: test.MinVersion, + MaxVersion: test.MaxVersion, } _, err := clientOptions.config() if err == nil { @@ -145,6 +154,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { clientVType VerificationType IdentityOptions IdentityCertificateOptions RootOptions RootCertificateOptions + MinVersion uint16 + MaxVersion uint16 }{ { desc: "Use system default if no fields in RootCertificateOptions is specified", @@ -159,6 +170,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { IdentityOptions: IdentityCertificateOptions{ IdentityProvider: fakeProvider{pt: provTypeIdentity}, }, + MinVersion: tls.VersionTLS12, + MaxVersion: tls.VersionTLS13, }, } for _, test := range tests { @@ -168,6 +181,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { VType: test.clientVType, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, + MinVersion: test.MinVersion, + MaxVersion: test.MaxVersion, } clientConfig, err := clientOptions.config() if err != nil { @@ -192,6 +207,8 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) { serverVType VerificationType IdentityOptions IdentityCertificateOptions RootOptions RootCertificateOptions + MinVersion uint16 + MaxVersion uint16 }{ { desc: "Skip default verification and provide no root credentials", @@ -229,6 +246,11 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) { }, }, }, + { + desc: "Invalid min/max TLS versions", + MinVersion: tls.VersionTLS13, + MaxVersion: tls.VersionTLS12, + }, } for _, test := range tests { test := test @@ -238,6 +260,8 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) { RequireClientCert: test.requireClientCert, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, + MinVersion: test.MinVersion, + MaxVersion: test.MaxVersion, } _, err := serverOptions.config() if err == nil { @@ -254,6 +278,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { serverVType VerificationType IdentityOptions IdentityCertificateOptions RootOptions RootCertificateOptions + MinVersion uint16 + MaxVersion uint16 }{ { desc: "Use system default if no fields in RootCertificateOptions is specified", @@ -275,6 +301,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { return nil, nil }, }, + MinVersion: tls.VersionTLS12, + MaxVersion: tls.VersionTLS13, }, } for _, test := range tests { @@ -285,6 +313,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { RequireClientCert: test.requireClientCert, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, + MinVersion: test.MinVersion, + MaxVersion: test.MaxVersion, } serverConfig, err := serverOptions.config() if err != nil {