resolve comments

grpc · Feb 4, 2023 · 54f7019 · 54f7019
1 parent 4cf19f9
commit 54f7019
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 51 deletions.
diff --git a/security/advancedtls/advancedtls.go b/security/advancedtls/advancedtls.go
@@ -162,19 +162,6 @@ const (
 	SkipVerification
 )
 
-// TLSVersionOptions contains the TLS version requirements for a client/server.
-type TLSVersionOptions struct {
-	// 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
-}
-
 // ClientOptions contains the fields needed to be filled by the client.
 type ClientOptions struct {
 	// IdentityOptions is OPTIONAL on client side. This field only needs to be
@@ -197,9 +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
-	// VersionOptions contains the requirements for the TLS version being used.
-	// If not set, the default values would be used.
-	VersionOptions TLSVersionOptions
+	// 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.
@@ -221,9 +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
-	// VersionOptions contains the requirements for the TLS version being used.
-	// If not set, the default values would be used.
-	VersionOptions TLSVersionOptions
+	// 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) {
@@ -241,16 +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.VersionOptions.MinVersion > o.VersionOptions.MaxVersion {
+	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.VersionOptions.MinVersion,
-		MaxVersion:         o.VersionOptions.MaxVersion,
+		MinVersion:         o.MinVersion,
+		MaxVersion:         o.MaxVersion,
 	}
 	// Propagate root-certificate-related fields in tls.Config.
 	switch {
@@ -317,7 +316,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.VersionOptions.MinVersion > o.VersionOptions.MaxVersion {
+	if o.MinVersion > o.MaxVersion {
 		return nil, fmt.Errorf("the minimum TLS version is larger than the maximum TLS version")
 	}
 	clientAuth := tls.NoClientCert
@@ -329,8 +328,8 @@ func (o *ServerOptions) config() (*tls.Config, error) {
 	}
 	config := &tls.Config{
 		ClientAuth: clientAuth,
-		MinVersion: o.VersionOptions.MinVersion,
-		MaxVersion: o.VersionOptions.MaxVersion,
+		MinVersion: o.MinVersion,
+		MaxVersion: o.MaxVersion,
 	}
 	// Propagate root-certificate-related fields in tls.Config.
 	switch {

diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go
@@ -91,7 +91,8 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) {
 		clientVType     VerificationType
 		IdentityOptions IdentityCertificateOptions
 		RootOptions     RootCertificateOptions
-		VersionOptions  TLSVersionOptions
+		MinVersion      uint16
+		MaxVersion      uint16
 	}{
 		{
 			desc:        "Skip default verification and provide no root credentials",
@@ -124,11 +125,9 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) {
 			},
 		},
 		{
-			desc: "Invalid min/max TLS versions",
-			VersionOptions: TLSVersionOptions{
-				MinVersion: tls.VersionTLS13,
-				MaxVersion: tls.VersionTLS12,
-			},
+			desc:       "Invalid min/max TLS versions",
+			MinVersion: tls.VersionTLS13,
+			MaxVersion: tls.VersionTLS12,
 		},
 	}
 	for _, test := range tests {
@@ -138,7 +137,8 @@ func (s) TestClientOptionsConfigErrorCases(t *testing.T) {
 				VType:           test.clientVType,
 				IdentityOptions: test.IdentityOptions,
 				RootOptions:     test.RootOptions,
-				VersionOptions:  test.VersionOptions,
+				MinVersion:      test.MinVersion,
+				MaxVersion:      test.MaxVersion,
 			}
 			_, err := clientOptions.config()
 			if err == nil {
@@ -154,7 +154,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) {
 		clientVType     VerificationType
 		IdentityOptions IdentityCertificateOptions
 		RootOptions     RootCertificateOptions
-		VersionOptions  TLSVersionOptions
+		MinVersion      uint16
+		MaxVersion      uint16
 	}{
 		{
 			desc:        "Use system default if no fields in RootCertificateOptions is specified",
@@ -169,10 +170,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) {
 			IdentityOptions: IdentityCertificateOptions{
 				IdentityProvider: fakeProvider{pt: provTypeIdentity},
 			},
-			VersionOptions: TLSVersionOptions{
-				MinVersion: tls.VersionTLS12,
-				MaxVersion: tls.VersionTLS13,
-			},
+			MinVersion: tls.VersionTLS12,
+			MaxVersion: tls.VersionTLS13,
 		},
 	}
 	for _, test := range tests {
@@ -182,7 +181,8 @@ func (s) TestClientOptionsConfigSuccessCases(t *testing.T) {
 				VType:           test.clientVType,
 				IdentityOptions: test.IdentityOptions,
 				RootOptions:     test.RootOptions,
-				VersionOptions:  test.VersionOptions,
+				MinVersion:      test.MinVersion,
+				MaxVersion:      test.MaxVersion,
 			}
 			clientConfig, err := clientOptions.config()
 			if err != nil {
@@ -207,7 +207,8 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) {
 		serverVType       VerificationType
 		IdentityOptions   IdentityCertificateOptions
 		RootOptions       RootCertificateOptions
-		VersionOptions    TLSVersionOptions
+		MinVersion        uint16
+		MaxVersion        uint16
 	}{
 		{
 			desc:              "Skip default verification and provide no root credentials",
@@ -246,11 +247,9 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) {
 			},
 		},
 		{
-			desc: "Invalid min/max TLS versions",
-			VersionOptions: TLSVersionOptions{
-				MinVersion: tls.VersionTLS13,
-				MaxVersion: tls.VersionTLS12,
-			},
+			desc:       "Invalid min/max TLS versions",
+			MinVersion: tls.VersionTLS13,
+			MaxVersion: tls.VersionTLS12,
 		},
 	}
 	for _, test := range tests {
@@ -261,7 +260,8 @@ func (s) TestServerOptionsConfigErrorCases(t *testing.T) {
 				RequireClientCert: test.requireClientCert,
 				IdentityOptions:   test.IdentityOptions,
 				RootOptions:       test.RootOptions,
-				VersionOptions:    test.VersionOptions,
+				MinVersion:        test.MinVersion,
+				MaxVersion:        test.MaxVersion,
 			}
 			_, err := serverOptions.config()
 			if err == nil {
@@ -278,7 +278,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) {
 		serverVType       VerificationType
 		IdentityOptions   IdentityCertificateOptions
 		RootOptions       RootCertificateOptions
-		VersionOptions    TLSVersionOptions
+		MinVersion        uint16
+		MaxVersion        uint16
 	}{
 		{
 			desc:              "Use system default if no fields in RootCertificateOptions is specified",
@@ -300,10 +301,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) {
 					return nil, nil
 				},
 			},
-			VersionOptions: TLSVersionOptions{
-				MinVersion: tls.VersionTLS12,
-				MaxVersion: tls.VersionTLS13,
-			},
+			MinVersion: tls.VersionTLS12,
+			MaxVersion: tls.VersionTLS13,
 		},
 	}
 	for _, test := range tests {
@@ -314,7 +313,8 @@ func (s) TestServerOptionsConfigSuccessCases(t *testing.T) {
 				RequireClientCert: test.requireClientCert,
 				IdentityOptions:   test.IdentityOptions,
 				RootOptions:       test.RootOptions,
-				VersionOptions:    test.VersionOptions,
+				MinVersion:        test.MinVersion,
+				MaxVersion:        test.MaxVersion,
 			}
 			serverConfig, err := serverOptions.config()
 			if err != nil {