Skip to content

advancedtls: Rename VType #7149

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

Merged
merged 4 commits into from
Apr 19, 2024
Merged

advancedtls: Rename VType #7149

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
30d2680
renamed VType to VerificationType and add deprecation note
gtcooke94 Apr 18, 2024
24e5133
renamed server VType and all usages
gtcooke94 Apr 18, 2024
d33c06e
add tests for deprecated VType cascade
gtcooke94 Apr 18, 2024
1a0d60d
Fix deprecated notice
gtcooke94 Apr 19, 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
40 changes: 31 additions & 9 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,11 @@ type ClientOptions struct {
// RootOptions is OPTIONAL on client side. If not set, we will try to use the
// default trust certificates in users' OS system.
RootOptions RootCertificateOptions
// VerificationType defines what type of server verification is done. See
// the `VerificationType` enum for the different options.
// Default: CertAndHostVerification
VerificationType VerificationType
// DEPRECATED: Renamed to `VerificationType`
// VType is the verification type on the client side.
Copy link
Member

@dfawley dfawley Apr 19, 2024

Choose a reason for hiding this comment

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

The proper way to docstring this is:

// VType is blah blah
//
// Deprecated: use VerificationType instead.
VType VerificationType

Copy link
Contributor Author

@gtcooke94 gtcooke94 Apr 19, 2024

Choose a reason for hiding this comment

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

Fixed

VType VerificationType
// RevocationConfig is the configurations for certificate revocation checks.
Expand Down Expand Up @@ -210,6 +215,11 @@ type ServerOptions struct {
RootOptions RootCertificateOptions
// If the server want the client to send certificates.
RequireClientCert bool
// VerificationType defines what type of client verification is done. See
// the `VerificationType` enum for the different options.
// Default: CertAndHostVerification
VerificationType VerificationType
// DEPRECATED: Renamed to `VerificationType`
Copy link
Member

@dfawley dfawley Apr 19, 2024

Choose a reason for hiding this comment

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

As above.

Copy link
Contributor Author

@gtcooke94 gtcooke94 Apr 19, 2024

Choose a reason for hiding this comment

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

Fixed

// VType is the verification type on the server side.
VType VerificationType
// RevocationConfig is the configurations for certificate revocation checks.
Expand All @@ -227,7 +237,13 @@ type ServerOptions struct {
}

func (o *ClientOptions) config() (*tls.Config, error) {
if o.VType == SkipVerification && o.VerifyPeer == nil {
// TODO(gtcooke94). VType is deprecated, eventually remove this block. This
// will ensure that users still explicitly setting `VType` will get the
// setting to the right place.
if o.VType != CertAndHostVerification {
o.VerificationType = o.VType
}
if o.VerificationType == SkipVerification && o.VerifyPeer == nil {
return nil, fmt.Errorf("client needs to provide custom verification mechanism if choose to skip default verification")
}
// Make sure users didn't specify more than one fields in
Expand Down Expand Up @@ -271,7 +287,7 @@ func (o *ClientOptions) config() (*tls.Config, error) {
default:
// No root certificate options specified by user. Use the certificates
// stored in system default path as the last resort.
if o.VType != SkipVerification {
if o.VerificationType != SkipVerification {
systemRootCAs, err := x509.SystemCertPool()
if err != nil {
return nil, err
Expand Down Expand Up @@ -303,7 +319,13 @@ func (o *ClientOptions) config() (*tls.Config, error) {
}

func (o *ServerOptions) config() (*tls.Config, error) {
if o.RequireClientCert && o.VType == SkipVerification && o.VerifyPeer == nil {
// TODO(gtcooke94). VType is deprecated, eventually remove this block. This
// will ensure that users still explicitly setting `VType` will get the
// setting to the right place.
if o.VType != CertAndHostVerification {
o.VerificationType = o.VType
}
if o.RequireClientCert && o.VerificationType == SkipVerification && o.VerifyPeer == nil {
return nil, fmt.Errorf("server needs to provide custom verification mechanism if choose to skip default verification, but require client certificate(s)")
}
// Make sure users didn't specify more than one fields in
Expand Down Expand Up @@ -351,7 +373,7 @@ func (o *ServerOptions) config() (*tls.Config, error) {
default:
// No root certificate options specified by user. Use the certificates
// stored in system default path as the last resort.
if o.VType != SkipVerification && o.RequireClientCert {
if o.VerificationType != SkipVerification && o.RequireClientCert {
systemRootCAs, err := x509.SystemCertPool()
if err != nil {
return nil, err
Expand Down Expand Up @@ -395,7 +417,7 @@ type advancedTLSCreds struct {
verifyFunc CustomVerificationFunc
getRootCAs func(params *GetRootCAsParams) (*GetRootCAsResults, error)
isClient bool
vType VerificationType
verificationType VerificationType
revocationConfig *RevocationConfig
}

Expand Down Expand Up @@ -495,7 +517,7 @@ func buildVerifyFunc(c *advancedTLSCreds,
}
rawCertList[i] = cert
}
if c.vType == CertAndHostVerification || c.vType == CertVerification {
if c.verificationType == CertAndHostVerification || c.verificationType == CertVerification {
// perform possible trust credential reloading and certificate check
rootCAs := c.config.RootCAs
if !c.isClient {
Expand Down Expand Up @@ -527,7 +549,7 @@ func buildVerifyFunc(c *advancedTLSCreds,
opts.Intermediates.AddCert(cert)
}
// Perform default hostname check if specified.
if c.isClient && c.vType == CertAndHostVerification && serverName != "" {
if c.isClient && c.verificationType == CertAndHostVerification && serverName != "" {
parsedName, _, err := net.SplitHostPort(serverName)
if err != nil {
// If the serverName had no host port or if the serverName cannot be
Expand Down Expand Up @@ -579,7 +601,7 @@ func NewClientCreds(o *ClientOptions) (credentials.TransportCredentials, error)
isClient: true,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.VerifyPeer,
vType: o.VType,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos)
Expand All @@ -598,7 +620,7 @@ func NewServerCreds(o *ServerOptions) (credentials.TransportCredentials, error)
isClient: false,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.VerifyPeer,
vType: o.VType,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos)
Expand Down
104 changes: 52 additions & 52 deletions security/advancedtls/advancedtls_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,19 +138,19 @@ func (s) TestEnd2End(t *testing.T) {
}
stage := &stageInfo{}
for _, test := range []struct {
desc string
clientCert []tls.Certificate
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc CustomVerificationFunc
clientVType VerificationType
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc CustomVerificationFunc
serverVType VerificationType
desc string
clientCert []tls.Certificate
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc CustomVerificationFunc
clientVerificationType VerificationType
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc CustomVerificationFunc
serverVerificationType VerificationType
}{
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand Down Expand Up @@ -178,8 +178,8 @@ func (s) TestEnd2End(t *testing.T) {
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
clientVType: CertVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
clientVerificationType: CertVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
serverGetRoot: func(params *GetRootCAsParams) (*GetRootCAsResults, error) {
switch stage.read() {
case 0, 1:
Expand All @@ -191,7 +191,7 @@ func (s) TestEnd2End(t *testing.T) {
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
serverVType: CertVerification,
serverVerificationType: CertVerification,
},
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand Down Expand Up @@ -219,7 +219,7 @@ func (s) TestEnd2End(t *testing.T) {
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
clientVType: CertVerification,
clientVerificationType: CertVerification,
serverGetCert: func(*tls.ClientHelloInfo) ([]*tls.Certificate, error) {
switch stage.read() {
case 0:
Expand All @@ -232,7 +232,7 @@ func (s) TestEnd2End(t *testing.T) {
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
serverVType: CertVerification,
serverVerificationType: CertVerification,
},
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand Down Expand Up @@ -284,7 +284,7 @@ func (s) TestEnd2End(t *testing.T) {
}
return nil, fmt.Errorf("custom authz check fails")
},
clientVType: CertVerification,
clientVerificationType: CertVerification,
serverGetCert: func(*tls.ClientHelloInfo) ([]*tls.Certificate, error) {
switch stage.read() {
case 0:
Expand All @@ -297,7 +297,7 @@ func (s) TestEnd2End(t *testing.T) {
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
serverVType: CertVerification,
serverVerificationType: CertVerification,
},
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand All @@ -317,9 +317,9 @@ func (s) TestEnd2End(t *testing.T) {
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
clientVType: CertVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
serverRoot: cs.ServerTrust1,
clientVerificationType: CertVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
serverRoot: cs.ServerTrust1,
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
switch stage.read() {
case 0, 2:
Expand All @@ -330,7 +330,7 @@ func (s) TestEnd2End(t *testing.T) {
return nil, fmt.Errorf("custom authz check fails")
}
},
serverVType: CertVerification,
serverVerificationType: CertVerification,
},
} {
test := test
Expand All @@ -347,7 +347,7 @@ func (s) TestEnd2End(t *testing.T) {
},
RequireClientCert: true,
VerifyPeer: test.serverVerifyFunc,
VType: test.serverVType,
VerificationType: test.serverVerificationType,
}
serverTLSCreds, err := NewServerCreds(serverOptions)
if err != nil {
Expand All @@ -373,7 +373,7 @@ func (s) TestEnd2End(t *testing.T) {
RootCACerts: test.clientRoot,
GetRootCertificates: test.clientGetRoot,
},
VType: test.clientVType,
VerificationType: test.clientVerificationType,
}
clientTLSCreds, err := NewClientCreds(clientOptions)
if err != nil {
Expand Down Expand Up @@ -638,7 +638,7 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
VerifyPeer: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
},
VType: CertVerification,
VerificationType: CertVerification,
}
serverTLSCreds, err := NewServerCreds(serverOptions)
if err != nil {
Expand All @@ -664,7 +664,7 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
RootOptions: RootCertificateOptions{
RootProvider: clientRootProvider,
},
VType: CertVerification,
VerificationType: CertVerification,
}
clientTLSCreds, err := NewClientCreds(clientOptions)
if err != nil {
Expand Down Expand Up @@ -731,34 +731,34 @@ func (s) TestDefaultHostNameCheck(t *testing.T) {
t.Fatalf("cs.LoadCerts() failed, err: %v", err)
}
for _, test := range []struct {
desc string
clientRoot *x509.CertPool
clientVType VerificationType
serverCert []tls.Certificate
serverVType VerificationType
expectError bool
desc string
clientRoot *x509.CertPool
clientVerificationType VerificationType
serverCert []tls.Certificate
serverVerificationType VerificationType
expectError bool
}{
// Client side sets vType to CertAndHostVerification, and will do
// default hostname check. Server uses a cert without "localhost" or
// "127.0.0.1" as common name or SAN names, and will hence fail.
{
desc: "Bad default hostname check",
clientRoot: cs.ClientTrust1,
clientVType: CertAndHostVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
serverVType: CertAndHostVerification,
expectError: true,
desc: "Bad default hostname check",
clientRoot: cs.ClientTrust1,
clientVerificationType: CertAndHostVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
serverVerificationType: CertAndHostVerification,
expectError: true,
},
// Client side sets vType to CertAndHostVerification, and will do
// default hostname check. Server uses a certificate with "localhost" as
// common name, and will hence pass the default hostname check.
{
desc: "Good default hostname check",
clientRoot: cs.ClientTrust1,
clientVType: CertAndHostVerification,
serverCert: []tls.Certificate{cs.ServerPeerLocalhost1},
serverVType: CertAndHostVerification,
expectError: false,
desc: "Good default hostname check",
clientRoot: cs.ClientTrust1,
clientVerificationType: CertAndHostVerification,
serverCert: []tls.Certificate{cs.ServerPeerLocalhost1},
serverVerificationType: CertAndHostVerification,
expectError: false,
},
} {
test := test
Expand All @@ -769,7 +769,7 @@ func (s) TestDefaultHostNameCheck(t *testing.T) {
Certificates: test.serverCert,
},
RequireClientCert: false,
VType: test.serverVType,
VerificationType: test.serverVerificationType,
}
serverTLSCreds, err := NewServerCreds(serverOptions)
if err != nil {
Expand All @@ -789,7 +789,7 @@ func (s) TestDefaultHostNameCheck(t *testing.T) {
RootOptions: RootCertificateOptions{
RootCACerts: test.clientRoot,
},
VType: test.clientVType,
VerificationType: test.clientVerificationType,
}
clientTLSCreds, err := NewClientCreds(clientOptions)
if err != nil {
Expand Down Expand Up @@ -907,7 +907,7 @@ func (s) TestTLSVersions(t *testing.T) {
Certificates: []tls.Certificate{cs.ServerPeerLocalhost1},
},
RequireClientCert: false,
VType: CertAndHostVerification,
VerificationType: CertAndHostVerification,
MinVersion: test.serverMinVersion,
MaxVersion: test.serverMaxVersion,
}
Expand All @@ -929,9 +929,9 @@ func (s) TestTLSVersions(t *testing.T) {
RootOptions: RootCertificateOptions{
RootCACerts: cs.ClientTrust1,
},
VType: CertAndHostVerification,
MinVersion: test.clientMinVersion,
MaxVersion: test.clientMaxVersion,
VerificationType: CertAndHostVerification,
MinVersion: test.clientMinVersion,
MaxVersion: test.clientMaxVersion,
}
clientTLSCreds, err := NewClientCreds(clientOptions)
if err != nil {
Expand Down
Loading