From befc29de931d6b4edb336bda7fb39621d01755e0 Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Mon, 6 May 2024 12:59:03 -0400 Subject: [PATCH] advancedTLS: Rename {Min/Max}Version to {Min/Max}TLSVersion (#7173) * rename `MinVersion` and `MaxVersion` to `MinTLSVersion` and `MaxTLSVersion` --- security/advancedtls/advancedtls.go | 90 +++++++++++++++---- .../advancedtls_integration_test.go | 8 +- security/advancedtls/advancedtls_test.go | 34 +++++-- 3 files changed, 104 insertions(+), 28 deletions(-) diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go index 2e8efe51521d..d7aa4f41be01 100644 --- a/security/advancedtls/advancedtls.go +++ b/security/advancedtls/advancedtls.go @@ -215,14 +215,26 @@ type ClientOptions struct { // It could be nil if such checks are not needed. RevocationOptions *RevocationOptions // 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. + // + // Deprecated: use MinTLSVersion instead. 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. + // + // Deprecated: use MaxTLSVersion instead. MaxVersion uint16 + // MinTLSVersion contains the minimum TLS version that is acceptable. + // The value should be set using tls.VersionTLSxx from https://pkg.go.dev/crypto/tls + // 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. This + // default may be changed over time affecting backwards compatibility. + MinTLSVersion uint16 + // MaxTLSVersion contains the maximum TLS version that is acceptable. + // The value should be set using tls.VersionTLSxx from https://pkg.go.dev/crypto/tls + // By default, the maximum version supported by this package is used, + // which is currently TLS 1.3. This default may be changed over time + // affecting backwards compatibility. + MaxTLSVersion uint16 // serverNameOverride is for testing only. If set to a non-empty string, it // will override the virtual host name of authority (e.g. :authority header // field) in requests and the target hostname used during server cert @@ -263,14 +275,26 @@ type ServerOptions struct { // It could be nil if such checks are not needed. RevocationOptions *RevocationOptions // 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. + // + // Deprecated: use MinTLSVersion instead. 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. + // + // Deprecated: use MaxTLSVersion instead. MaxVersion uint16 + // MinTLSVersion contains the minimum TLS version that is acceptable. + // The value should be set using tls.VersionTLSxx from https://pkg.go.dev/crypto/tls + // 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. This + // default may be changed over time affecting backwards compatibility. + MinTLSVersion uint16 + // MaxTLSVersion contains the maximum TLS version that is acceptable. + // The value should be set using tls.VersionTLSxx from https://pkg.go.dev/crypto/tls + // By default, the maximum version supported by this package is used, + // which is currently TLS 1.3. This default may be changed over time + // affecting backwards compatibility. + MaxTLSVersion uint16 } func (o *ClientOptions) config() (*tls.Config, error) { @@ -285,6 +309,15 @@ func (o *ClientOptions) config() (*tls.Config, error) { if o.VType != CertAndHostVerification { o.VerificationType = o.VType } + // TODO(gtcooke94) MinVersion and MaxVersion are deprected, eventually + // remove this block. This is a temporary fallback to ensure that if the + // refactored names aren't set we use the old names. + if o.MinTLSVersion == 0 { + o.MinTLSVersion = o.MinVersion + } + if o.MaxTLSVersion == 0 { + o.MaxTLSVersion = o.MaxVersion + } if o.VerificationType == SkipVerification && o.AdditionalPeerVerification == nil { return nil, fmt.Errorf("client needs to provide custom verification mechanism if choose to skip default verification") } @@ -299,16 +332,24 @@ 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 { + if o.MinTLSVersion > o.MaxTLSVersion { return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version") } + // If the MinTLSVersion isn't set, default to 1.2 + if o.MinTLSVersion == 0 { + o.MinTLSVersion = tls.VersionTLS12 + } + // If the MaxTLSVersion isn't set, default to 1.3 + if o.MaxTLSVersion == 0 { + o.MaxTLSVersion = tls.VersionTLS13 + } 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, + MinVersion: o.MinTLSVersion, + MaxVersion: o.MaxTLSVersion, } // Propagate root-certificate-related fields in tls.Config. switch { @@ -372,6 +413,15 @@ func (o *ServerOptions) config() (*tls.Config, error) { if o.VType != CertAndHostVerification { o.VerificationType = o.VType } + // TODO(gtcooke94) MinVersion and MaxVersion are deprected, eventually + // remove this block. This is a temporary fallback to ensure that if the + // refactored names aren't set we use the old names. + if o.MinTLSVersion == 0 { + o.MinTLSVersion = o.MinVersion + } + if o.MaxTLSVersion == 0 { + o.MaxTLSVersion = o.MaxVersion + } if o.RequireClientCert && o.VerificationType == SkipVerification && o.AdditionalPeerVerification == nil { return nil, fmt.Errorf("server needs to provide custom verification mechanism if choose to skip default verification, but require client certificate(s)") } @@ -386,7 +436,7 @@ 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 { + if o.MinTLSVersion > o.MaxTLSVersion { return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version") } clientAuth := tls.NoClientCert @@ -396,10 +446,18 @@ func (o *ServerOptions) config() (*tls.Config, error) { // buildVerifyFunc. clientAuth = tls.RequireAnyClientCert } + // If the MinTLSVersion isn't set, default to 1.2 + if o.MinTLSVersion == 0 { + o.MinTLSVersion = tls.VersionTLS12 + } + // If the MaxTLSVersion isn't set, default to 1.3 + if o.MaxTLSVersion == 0 { + o.MaxTLSVersion = tls.VersionTLS13 + } config := &tls.Config{ ClientAuth: clientAuth, - MinVersion: o.MinVersion, - MaxVersion: o.MaxVersion, + MinVersion: o.MinTLSVersion, + MaxVersion: o.MaxTLSVersion, } // 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 ca080c956b97..d3c5d143bd9d 100644 --- a/security/advancedtls/advancedtls_integration_test.go +++ b/security/advancedtls/advancedtls_integration_test.go @@ -908,8 +908,8 @@ func (s) TestTLSVersions(t *testing.T) { }, RequireClientCert: false, VerificationType: CertAndHostVerification, - MinVersion: test.serverMinVersion, - MaxVersion: test.serverMaxVersion, + MinTLSVersion: test.serverMinVersion, + MaxTLSVersion: test.serverMaxVersion, } serverTLSCreds, err := NewServerCreds(serverOptions) if err != nil { @@ -930,8 +930,8 @@ func (s) TestTLSVersions(t *testing.T) { RootCACerts: cs.ClientTrust1, }, VerificationType: CertAndHostVerification, - MinVersion: test.clientMinVersion, - MaxVersion: test.clientMaxVersion, + MinTLSVersion: test.clientMinVersion, + MaxTLSVersion: test.clientMaxVersion, } clientTLSCreds, err := NewClientCreds(clientOptions) if err != nil { diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go index e77e0f5e9819..7b1ee7fee1cd 100644 --- a/security/advancedtls/advancedtls_test.go +++ b/security/advancedtls/advancedtls_test.go @@ -138,8 +138,8 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) { VerificationType: test.clientVerificationType, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, - MinVersion: test.MinVersion, - MaxVersion: test.MaxVersion, + MinTLSVersion: test.MinVersion, + MaxTLSVersion: test.MaxVersion, } _, err := clientOptions.config() if err == nil { @@ -196,8 +196,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { VerificationType: test.clientVerificationType, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, - MinVersion: test.MinVersion, - MaxVersion: test.MaxVersion, + MinTLSVersion: test.MinVersion, + MaxTLSVersion: test.MaxVersion, } clientConfig, err := clientOptions.config() if err != nil { @@ -211,6 +211,24 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) { t.Fatalf("Failed to assign system-provided certificates on the client side.") } } + if test.MinVersion != 0 { + if clientConfig.MinVersion != test.MinVersion { + t.Fatalf("Failed to assign min tls version.") + } + } else { + if clientConfig.MinVersion != tls.VersionTLS12 { + t.Fatalf("Default min tls version not set correctly") + } + } + if test.MaxVersion != 0 { + if clientConfig.MaxVersion != test.MaxVersion { + t.Fatalf("Failed to assign max tls version.") + } + } else { + if clientConfig.MaxVersion != tls.VersionTLS13 { + t.Fatalf("Default max tls version not set correctly") + } + } }) } } @@ -275,8 +293,8 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) { RequireClientCert: test.requireClientCert, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, - MinVersion: test.MinVersion, - MaxVersion: test.MaxVersion, + MinTLSVersion: test.MinVersion, + MaxTLSVersion: test.MaxVersion, } _, err := serverOptions.config() if err == nil { @@ -342,8 +360,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) { RequireClientCert: test.requireClientCert, IdentityOptions: test.IdentityOptions, RootOptions: test.RootOptions, - MinVersion: test.MinVersion, - MaxVersion: test.MaxVersion, + MinTLSVersion: test.MinVersion, + MaxTLSVersion: test.MaxVersion, } serverConfig, err := serverOptions.config() if err != nil {