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 11 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
75 changes: 59 additions & 16 deletions security/advancedtls/advancedtls.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,10 +35,10 @@ import (
credinternal "google.golang.org/grpc/internal/credentials"
)

// VerificationFuncParams contains parameters available to users when
// implementing CustomVerificationFunc.
// HandshakeVerificationInfo contains information about a handshake needed for
// verification for use when implementing the `PostHandshakeVerificationFunc`
// The fields in this struct are read-only.
type VerificationFuncParams struct {
type HandshakeVerificationInfo struct {
dfawley marked this conversation as resolved.
Show resolved Hide resolved
// The target server name that the client connects to when establishing the
// connection. This field is only meaningful for client side. On server side,
// this field would be an empty string.
Expand All @@ -54,17 +54,36 @@ type VerificationFuncParams struct {
Leaf *x509.Certificate
}

// VerificationResults contains the information about results of
// CustomVerificationFunc.
// VerificationResults is an empty struct for now. It may be extended in the
// VerificationFuncParams contains parameters available to users when
// implementing CustomVerificationFunc.
// The fields in this struct are read-only.
//
// Deprecated: use HandshakeVerificationInfo instead.
type VerificationFuncParams = HandshakeVerificationInfo

// PostHandshakeVerificationResults contains the information about results of
// PostHandshakeVerificationFunc.
// PostHandshakeVerificationResults is an empty struct for now. It may be extended in the
// future to include more information.
type VerificationResults struct{}
type PostHandshakeVerificationResults struct{}

// Deprecated: use PostHandshakeVerificationResults instead.
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 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
// verification check.
// CustomVerificationFunc returns nil if the authorization fails; otherwise
// returns an empty struct.
type CustomVerificationFunc func(params *VerificationFuncParams) (*VerificationResults, error)
//
// Deprecated: use PostHandshakeVerificationFunc instead.
type CustomVerificationFunc = PostHandshakeVerificationFunc

// GetRootCAsParams contains the parameters available to users when
// implementing GetRootCAs.
Expand Down Expand Up @@ -167,11 +186,18 @@ 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.
VerifyPeer CustomVerificationFunc
//
// 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.
RootOptions RootCertificateOptions
Expand Down Expand Up @@ -206,11 +232,18 @@ 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.
VerifyPeer CustomVerificationFunc
//
// 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).
RootOptions RootCertificateOptions
Expand Down Expand Up @@ -239,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 @@ -321,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 @@ -416,7 +459,7 @@ func (o *ServerOptions) config() (*tls.Config, error) {
// using TLS.
type advancedTLSCreds struct {
config *tls.Config
verifyFunc CustomVerificationFunc
verifyFunc PostHandshakeVerificationFunc
getRootCAs func(params *GetRootCAsParams) (*GetRootCAsResults, error)
isClient bool
verificationType VerificationType
Expand Down Expand Up @@ -579,7 +622,7 @@ func buildVerifyFunc(c *advancedTLSCreds,
}
// Perform custom verification check if specified.
if c.verifyFunc != nil {
_, err := c.verifyFunc(&VerificationFuncParams{
_, err := c.verifyFunc(&HandshakeVerificationInfo{
ServerName: serverName,
RawCerts: rawCerts,
VerifiedChains: chains,
Expand All @@ -602,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 @@ -621,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
53 changes: 27 additions & 26 deletions security/advancedtls/advancedtls_integration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,14 @@ func (s) TestEnd2End(t *testing.T) {
clientGetCert func(*tls.CertificateRequestInfo) (*tls.Certificate, error)
clientRoot *x509.CertPool
clientGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
clientVerifyFunc CustomVerificationFunc
clientVerifyFunc PostHandshakeVerificationFunc
clientVerificationType VerificationType
serverCert []tls.Certificate
serverGetCert func(*tls.ClientHelloInfo) ([]*tls.Certificate, error)
serverRoot *x509.CertPool
serverGetRoot func(params *GetRootCAsParams) (*GetRootCAsResults, error)
serverVerifyFunc CustomVerificationFunc
serverVerifyFunc PostHandshakeVerificationFunc
serverVType VerificationType
serverVerificationType VerificationType
}{
// Test Scenarios:
Expand All @@ -175,8 +176,8 @@ func (s) TestEnd2End(t *testing.T) {
}
},
clientRoot: cs.ClientTrust1,
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
clientVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
clientVerificationType: CertVerification,
serverCert: []tls.Certificate{cs.ServerCert1},
Expand All @@ -188,8 +189,8 @@ func (s) TestEnd2End(t *testing.T) {
return &GetRootCAsResults{TrustCerts: cs.ServerTrust2}, nil
}
},
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
serverVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
serverVerificationType: CertVerification,
},
Expand All @@ -216,8 +217,8 @@ func (s) TestEnd2End(t *testing.T) {
return &GetRootCAsResults{TrustCerts: cs.ClientTrust2}, nil
}
},
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
clientVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
clientVerificationType: CertVerification,
serverGetCert: func(*tls.ClientHelloInfo) ([]*tls.Certificate, error) {
Expand All @@ -229,8 +230,8 @@ func (s) TestEnd2End(t *testing.T) {
}
},
serverRoot: cs.ServerTrust1,
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
serverVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
serverVerificationType: CertVerification,
},
Expand Down Expand Up @@ -258,7 +259,7 @@ func (s) TestEnd2End(t *testing.T) {
return &GetRootCAsResults{TrustCerts: cs.ClientTrust2}, nil
}
},
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
clientVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
if len(params.RawCerts) == 0 {
return nil, fmt.Errorf("no peer certs")
}
Expand All @@ -280,7 +281,7 @@ func (s) TestEnd2End(t *testing.T) {
}
}
if authzCheck {
return &VerificationResults{}, nil
return &PostHandshakeVerificationResults{}, nil
}
return nil, fmt.Errorf("custom authz check fails")
},
Expand All @@ -294,8 +295,8 @@ func (s) TestEnd2End(t *testing.T) {
}
},
serverRoot: cs.ServerTrust1,
serverVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
serverVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
serverVerificationType: CertVerification,
},
Expand All @@ -314,16 +315,16 @@ func (s) TestEnd2End(t *testing.T) {
desc: "TestServerCustomVerification",
clientCert: []tls.Certificate{cs.ClientCert1},
clientRoot: cs.ClientTrust1,
clientVerifyFunc: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
clientVerifyFunc: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
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:
return &VerificationResults{}, nil
return &PostHandshakeVerificationResults{}, nil
case 1:
return nil, fmt.Errorf("custom authz check fails")
default:
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,8 +636,8 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
RootProvider: serverRootProvider,
},
RequireClientCert: true,
VerifyPeer: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
AdditionalPeerVerification: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
VerificationType: CertVerification,
}
Expand All @@ -658,8 +659,8 @@ func (s) TestPEMFileProviderEnd2End(t *testing.T) {
IdentityOptions: IdentityCertificateOptions{
IdentityProvider: clientIdentityProvider,
},
VerifyPeer: func(params *VerificationFuncParams) (*VerificationResults, error) {
return &VerificationResults{}, nil
AdditionalPeerVerification: func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error) {
return &PostHandshakeVerificationResults{}, nil
},
RootOptions: RootCertificateOptions{
RootProvider: clientRootProvider,
Expand Down
Loading
Loading