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

Jump to bottom

advancedtls: Rename custom verification function APIs #7140

Merged
merged 13 commits into from
Apr 23, 2024
Merged

advancedtls: Rename custom verification function APIs #7140

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5c7819f
rename
gtcooke94 Apr 17, 2024
79b35eb
replace usage of CustomVerificationFunc
gtcooke94 Apr 17, 2024
e1248b0
replace usages
gtcooke94 Apr 17, 2024
0d6185d
rename the results struct
gtcooke94 Apr 17, 2024
53323db
address PR comments - deprecation comment structure
gtcooke94 Apr 19, 2024
5920acc
Add more description to PostHandshakeVerificationFunc
gtcooke94 Apr 22, 2024
010d89e
rename VerifyPeer option to AdditionalPeerVerification
gtcooke94 Apr 23, 2024
718e858
clean up PostHandshakeVerificationFunc documentation
gtcooke94 Apr 23, 2024
6bcc5f8
wording
gtcooke94 Apr 23, 2024
e6f065e
merge master
gtcooke94 Apr 23, 2024
84aa942
missed saving a file
gtcooke94 Apr 23, 2024
52dbf58
VType -> VerificationType after merge
gtcooke94 Apr 23, 2024
bb4d7b2
reverse precendence of deprecated option
gtcooke94 Apr 23, 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
Prev Previous commit
Next Next commit
merge master
  • Loading branch information
@gtcooke94
gtcooke94 committed Apr 23, 2024
commit e6f065ee85f061772763bc85540c57fca60c166c
20 changes: 16 additions & 4 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -277,7 +277,13 @@ func (o *ClientOptions) config() (*tls.Config, error) {
if o.VerifyPeer != nil {
o.AdditionalPeerVerification = o.VerifyPeer
Copy link
Member

Choose a reason for hiding this comment

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

if o.AdditionalPeerVerification != nil { return <err> }?

Or ignore the old deprecated field VerifyPeer instead by reversing it:

if o.AdditionalPeerVerification == nil {
	o.AdditionalPeerVerification = o.VerifyPeer
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not required, so the error return I think doesn't make the most sense.

The reverse works too, I guess it just matters for the precedence - I was going with "if the old field is set they probably haven't migrated", but the bottom is good too as a "if the new field isn't set take whatever is in the old field"

Copy link
Member

Choose a reason for hiding this comment

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

The error would be "you set two fields that mean the same thing, one of which is deprecated: what were you thinking?" 😆

}
if o.VType == SkipVerification && o.AdditionalPeerVerification == 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.AdditionalPeerVerification == 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 @@ -358,7 +364,13 @@ func (o *ServerOptions) config() (*tls.Config, error) {
if o.VerifyPeer != nil {
o.AdditionalPeerVerification = o.VerifyPeer
}
if o.RequireClientCert && o.VType == SkipVerification && o.AdditionalPeerVerification == 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.AdditionalPeerVerification == 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 @@ -634,7 +646,7 @@ func NewClientCreds(o *ClientOptions) (credentials.TransportCredentials, error)
isClient: true,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.AdditionalPeerVerification,
vType: o.VType,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos)
Expand All @@ -653,7 +665,7 @@ func NewServerCreds(o *ServerOptions) (credentials.TransportCredentials, error)
isClient: false,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.AdditionalPeerVerification,
vType: o.VType,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
tc.config.NextProtos = credinternal.AppendH2ToNextProtos(tc.config.NextProtos)
Expand Down
26 changes: 21 additions & 5 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,34 @@ func (s) TestEnd2End(t *testing.T) {
}
stage := &stageInfo{}
for _, test := range []struct {
<<<<<<< HEAD
desc string
clientCert []tls.Certificate
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc PostHandshakeVerificationFunc
clientVType VerificationType
clientVerificationType VerificationType
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc PostHandshakeVerificationFunc
serverVType VerificationType
serverVerificationType VerificationType
=======
desc string
clientCert []tls.Certificate
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc CustomVerificationFunc
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc CustomVerificationFunc
>>>>>>> master
}{
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand Down Expand Up @@ -317,9 +332,10 @@ func (s) TestEnd2End(t *testing.T) {
clientVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, 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) {
serverVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
switch stage.read() {
case 0, 2:
Expand Down Expand Up @@ -347,7 +363,7 @@ func (s) TestEnd2End(t *testing.T) {
},
RequireClientCert: true,
AdditionalPeerVerification: test.serverVerifyFunc,
VType: test.serverVType,
VerificationType: test.serverVerificationType,
}
serverTLSCreds, err := NewServerCreds(serverOptions)
if err != nil {
Expand Down
6 changes: 3 additions & 3 deletions security/advancedtls/advancedtls_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc PostHandshakeVerificationFunc
clientVType VerificationType
clientVerificationType VerificationType
clientRootProvider certprovider.Provider
clientIdentityProvider certprovider.Provider
clientRevocationConfig *RevocationConfig
Expand All @@ -443,7 +443,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc PostHandshakeVerificationFunc
serverVType VerificationType
serverVerificationType VerificationType
serverRootProvider certprovider.Provider
serverIdentityProvider certprovider.Provider
serverRevocationConfig *RevocationConfig
Expand Down Expand Up @@ -824,7 +824,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
},
RequireClientCert: test.serverMutualTLS,
AdditionalPeerVerification: test.serverVerifyFunc,
VType: test.serverVType,
VerificationType: test.serverVerificationType,
RevocationConfig: test.serverRevocationConfig,
}
go func(done chan credentials.AuthInfo, lis net.Listener, serverOptions *ServerOptions) {
Expand Down
Loading
You are viewing a condensed version of this merge commit. You can view the full changes here.