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 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 6 commits
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
36 changes: 30 additions & 6 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,8 @@ type VerificationResults = PostHandshakeVerificationResults
// PostHandshakeVerificationFunc is the function defined by users to perform
// custom verification checks after chain building and regular handshake
// verification has been completed.
// PostHandshakeVerificationFunc returns nil
// if the authorization fails; otherwise returns an empty struct.
// PostHandshakeVerificationFunc should return (nil, error) if the authorization
// should fail, with the error containing information on why it failed.
type PostHandshakeVerificationFunc func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error)
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: PeerVerificationFunc (and all associated types)? It's a shorter name and (maybe?) is named after the intent of the thing vs. when it happens.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to make sure that we are differentiating between verification happening after the normal chain building and verification that happens by default (which this is), and overriding the base chain building and verification itself.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow all this.

What is "post handshake verification" exactly? And you're saying this can be used to replace the default behavior? What is the default behavior? Do you have any examples of what users would want to do with this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's two main different ways users could desire to customize verification behavior.
During verification we takes the peer's untrusted chain and build a chain from it to a trusted root.

  1. A user could want to fully replace this chain building behavior (rarer, more difficult use-case that is not supported but might be in the future).
  2. A user could also just want to do additional checks after the regular default chain building (this is a much more common use-case). We want to preemptively make sure the naming for (2) does not get it confused with (1), as we see them as fully different features with different use cases.

A concrete example of (2) is doing additional check on the hostname of the peer's cert.
A concrete example of (1) would be fully changing the process by which you build a chain to a trusted root using other information, for example SPIFFE IDs.

Copy link
Member

Choose a reason for hiding this comment

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

What's the API for (2)? VerifyPeer?

So is it the case that if you specify this callback then VerifyPeer isn't called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes the API for (2) is VerifyPeer. When set it eventually cascades to here -

grpc-go/security/advancedtls/advancedtls.go

Lines 581 to 582 in 34de5cf

if c.verifyFunc != nil {
_, err := c.verifyFunc(&VerificationFuncParams{

And VerifyPeer is of type PostHandshakeVerificationFunc, I think renaming this option from VerifyPeer to something more clear is probably belongs in this PR as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed VerifyPeer in the settings struct, PTAL


// CustomVerificationFunc is the function defined by users to perform custom
Expand Down Expand Up @@ -186,10 +186,17 @@ type ClientOptions struct {
// IdentityOptions is OPTIONAL on client side. This field only needs to be
// set if mutual authentication is required on server side.
IdentityOptions IdentityCertificateOptions
// AdditionalPeerVerification is a custom verification check after certificate signature
// check.
// If this is set, we will perform this customized check after doing the
// normal check(s) indicated by setting VType.
Copy link
Member

Choose a reason for hiding this comment

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

VType->VerificationType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I think I lost some in the merge, good catch

AdditionalPeerVerification PostHandshakeVerificationFunc
// VerifyPeer is a custom verification check after certificate signature
// check.
// If this is set, we will perform this customized check after doing the
// normal check(s) indicated by setting VType.
//
// Deprecated: use AdditionalPeerVerification instead.
VerifyPeer PostHandshakeVerificationFunc
// RootOptions is OPTIONAL on client side. If not set, we will try to use the
// default trust certificates in users' OS system.
Expand Down Expand Up @@ -225,10 +232,17 @@ type ClientOptions struct {
type ServerOptions struct {
// IdentityOptions is REQUIRED on server side.
IdentityOptions IdentityCertificateOptions
// AdditionalPeerVerification is a custom verification check after certificate signature
// check.
// If this is set, we will perform this customized check after doing the
// normal check(s) indicated by setting VType.
Copy link
Member

Choose a reason for hiding this comment

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

VerificationType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch

AdditionalPeerVerification PostHandshakeVerificationFunc
// VerifyPeer is a custom verification check after certificate signature
// check.
// If this is set, we will perform this customized check after doing the
// normal check(s) indicated by setting VType.
//
// Deprecated: use AdditionalPeerVerification instead.
VerifyPeer PostHandshakeVerificationFunc
// RootOptions is OPTIONAL on server side. This field only needs to be set if
// mutual authentication is required(RequireClientCert is true).
Expand Down Expand Up @@ -258,13 +272,18 @@ type ServerOptions struct {
}

func (o *ClientOptions) config() (*tls.Config, error) {
// TODO(gtcooke94) Remove this block when remove o.VerifyPeer
// Set AdditionalPeerVerification if the user is still using VerifyPeer.
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?" 😆

}
// 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 {
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 @@ -340,13 +359,18 @@ func (o *ClientOptions) config() (*tls.Config, error) {
}

func (o *ServerOptions) config() (*tls.Config, error) {
// TODO(gtcooke94) Remove this block when remove o.VerifyPeer
// Set AdditionalPeerVerification if the user is still using VerifyPeer.
if o.VerifyPeer != nil {
o.AdditionalPeerVerification = o.VerifyPeer
}
// 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 {
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 @@ -621,7 +645,7 @@ func NewClientCreds(o *ClientOptions) (credentials.TransportCredentials, error)
config: conf,
isClient: true,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.VerifyPeer,
verifyFunc: o.AdditionalPeerVerification,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
Expand All @@ -640,7 +664,7 @@ func NewServerCreds(o *ServerOptions) (credentials.TransportCredentials, error)
config: conf,
isClient: false,
getRootCAs: o.RootOptions.GetRootCertificates,
verifyFunc: o.VerifyPeer,
verifyFunc: o.AdditionalPeerVerification,
verificationType: o.VerificationType,
revocationConfig: o.RevocationConfig,
}
Expand Down
45 changes: 23 additions & 22 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,20 @@ 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 PostHandshakeVerificationFunc
clientVType VerificationType
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc PostHandshakeVerificationFunc
serverVType VerificationType
desc string
clientCert []tls.Certificate
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc PostHandshakeVerificationFunc
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
}{
// Test Scenarios:
// At initialization(stage = 0), client will be initialized with cert
Expand Down Expand Up @@ -317,9 +318,9 @@ 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 *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
switch stage.read() {
case 0, 2:
Expand All @@ -345,9 +346,9 @@ func (s) TestEnd2End(t *testing.T) {
RootCACerts: test.serverRoot,
GetRootCertificates: test.serverGetRoot,
},
RequireClientCert: true,
VerifyPeer: test.serverVerifyFunc,
VerificationType: test.serverVerificationType,
RequireClientCert: true,
AdditionalPeerVerification: test.serverVerifyFunc,
VerificationType: test.serverVerificationType,
}
serverTLSCreds, err := NewServerCreds(serverOptions)
if err != nil {
Expand All @@ -368,7 +369,7 @@ func (s) TestEnd2End(t *testing.T) {
Certificates: test.clientCert,
GetIdentityCertificatesForClient: test.clientGetCert,
},
VerifyPeer: test.clientVerifyFunc,
AdditionalPeerVerification: test.clientVerifyFunc,
RootOptions: RootCertificateOptions{
RootCACerts: test.clientRoot,
GetRootCertificates: test.clientGetRoot,
Expand Down Expand Up @@ -635,7 +636,7 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
RootProvider: serverRootProvider,
},
RequireClientCert: true,
VerifyPeer: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
AdditionalPeerVerification: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
VerificationType: CertVerification,
Expand All @@ -658,7 +659,7 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
IdentityOptions: IdentityCertificateOptions{
IdentityProvider: clientIdentityProvider,
},
VerifyPeer: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
AdditionalPeerVerification: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
RootOptions: RootCertificateOptions{
Expand Down
14 changes: 7 additions & 7 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 @@ -822,10 +822,10 @@ func (s) TestClientServerHandshake(t *testing.T) {
GetRootCertificates: test.serverGetRoot,
RootProvider: test.serverRootProvider,
},
RequireClientCert: test.serverMutualTLS,
VerifyPeer: test.serverVerifyFunc,
VerificationType: test.serverVerificationType,
RevocationConfig: test.serverRevocationConfig,
RequireClientCert: test.serverMutualTLS,
AdditionalPeerVerification: test.serverVerifyFunc,
VerificationType: test.serverVerificationType,
RevocationConfig: test.serverRevocationConfig,
}
go func(done chan credentials.AuthInfo, lis net.Listener, serverOptions *ServerOptions) {
serverRawConn, err := lis.Accept()
Expand Down Expand Up @@ -861,7 +861,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
GetIdentityCertificatesForClient: test.clientGetCert,
IdentityProvider: test.clientIdentityProvider,
},
VerifyPeer: test.clientVerifyFunc,
AdditionalPeerVerification: test.clientVerifyFunc,
RootOptions: RootCertificateOptions{
RootCACerts: test.clientRoot,
GetRootCertificates: test.clientGetRoot,
Expand Down
2 changes: 1 addition & 1 deletion security/advancedtls/examples/credential_reloading_from_files/client/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func main() {
IdentityOptions: advancedtls.IdentityCertificateOptions{
IdentityProvider: identityProvider,
},
VerifyPeer: func(params *advancedtls.HandshakeVerificationInfo) (*advancedtls.PostHandshakeVerificationResults, error) {
AdditionalPeerVerification: func(params *advancedtls.HandshakeVerificationInfo) (*advancedtls.PostHandshakeVerificationResults, error) {
return &advancedtls.PostHandshakeVerificationResults{}, nil
},
RootOptions: advancedtls.RootCertificateOptions{
Expand Down
2 changes: 1 addition & 1 deletion security/advancedtls/examples/credential_reloading_from_files/server/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ func main() {
RootProvider: rootProvider,
},
RequireClientCert: true,
VerifyPeer: func(params *advancedtls.HandshakeVerificationInfo) (*advancedtls.PostHandshakeVerificationResults, error) {
AdditionalPeerVerification: func(params *advancedtls.HandshakeVerificationInfo) (*advancedtls.PostHandshakeVerificationResults, error) {
// This message is to show the certificate under the hood is actually reloaded.
fmt.Printf("Client common name: %s.\n", params.Leaf.Subject.CommonName)
return &advancedtls.PostHandshakeVerificationResults{}, nil
Expand Down
Loading