Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

advancedTLS: Rename {Min/Max}Version to {Min/Max}TLSVersion #7173

Merged
merged 6 commits into from
May 6, 2024
Merged

advancedTLS: Rename {Min/Max}Version to {Min/Max}TLSVersion #7173

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c21abb4
rename
gtcooke94 Apr 25, 2024
c79a583
set the min and max tls version defaults
gtcooke94 Apr 30, 2024
e7fb344
add tls version defaults to server options too
gtcooke94 Apr 30, 2024
62ad803
add tests for default tls versions
gtcooke94 Apr 30, 2024
db4e77c
fix copy paste errors
gtcooke94 May 3, 2024
b57509f
add note about using tls.VersionTLSxx
gtcooke94 May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 70 additions & 16 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,14 +216,24 @@ type ClientOptions struct {
// 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.
//
// 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.
// 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.
Comment on lines +229 to +231
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we reserving the right to override the user's setting? Like if they set 1.0 and we decide to set the min to 1.2, then we'll automatically upgrade them? Or no?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - if we decide to change the minimum supported version from 1.0 to 1.2, I think we'd add in a check that throws an error rather than override the user setting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine. It seems like when they made a change like this in Go's tls package they added an env var so users could get the old 1.0-default behavior back (just FYI).

Also, I assume this is supposed to be a tls.VersionTLSxx value? (I wish they had made a type name for it.) That might be worth documenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could explore adding an env var like that down the line if it ever happens that we need it
Added notes in the docs about using tls.VersionTLSxx

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added notes in the docs about using tls.VersionTLSxx

What doc? The godocs? If so, I think you forgot to push the change. It seems worth documenting in these comments, since this isn't inside the tls package, but I won't argue too strongly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops I didn't actually push the change to my remote branch, done now

MinTLSVersion uint16
// MaxTLSVersion 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. This default may be changed over time
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
// 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
Expand Down Expand Up @@ -264,14 +274,24 @@ type ServerOptions struct {
// 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.
//
// 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.
// 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
gtcooke94 marked this conversation as resolved.
Show resolved Hide resolved
// MaxTLSVersion 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. This default may be changed over time
// affecting backwards compatibility.
MaxTLSVersion uint16
}

func (o *ClientOptions) config() (*tls.Config, error) {
Expand All @@ -286,6 +306,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")
}
Expand All @@ -300,16 +329,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 {
Expand Down Expand Up @@ -373,6 +410,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)")
}
Expand All @@ -387,7 +433,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
Expand All @@ -397,10 +443,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 {
Expand Down
8 changes: 4 additions & 4 deletions security/advancedtls/advancedtls_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down
34 changes: 26 additions & 8 deletions security/advancedtls/advancedtls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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")
}
}
})
}
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
Loading