advancedtls: Rename custom verification function APIs #7140

gtcooke94 · 2024-04-17T20:40:27Z

This change renames the APIs for injecting a custom verification function such that the names are clearer that it is occurs post-handshake with already built chains and does not replace the base chain building and chain verification that the underlying security library does.

I've left type aliases to the old names and marked them as deprecated.

(To add to release notes? - In advancedtls, change CustomVerificationFunc to PostHandshakeVerificationFunc, VerificationFuncParams to HandshakeVerificationInfo, and VerificationResults to PostHandshakeVerificationResults.)

RELEASE NOTES: none

security/advancedtls/advancedtls.go

erm-g · 2024-04-18T17:16:43Z

security/advancedtls/examples/credential_reloading_from_files/client/main.go

@@ -76,8 +76,8 @@ func main() {
 		IdentityOptions: advancedtls.IdentityCertificateOptions{
 			IdentityProvider: identityProvider,
 		},
-		VerifyPeer: func(params *advancedtls.VerificationFuncParams) (*advancedtls.VerificationResults, error) {
-			return &advancedtls.VerificationResults{}, nil
+		VerifyPeer: func(params *advancedtls.HandshakeVerificationInfo) (*advancedtls.PostHandshakeVerificationResults, error) {


Looking at both 'main.go' examples it looks like 'xOptions' is used more here, so I'm leaning towards 'HandshakeVerificationOptions'

See other comment

erm-g

LGTM, let's ask Doug to have the final look.

security/advancedtls/advancedtls.go

dfawley · 2024-04-19T16:25:13Z

security/advancedtls/advancedtls.go

+// PostHandshakeVerificationFunc returns nil
+// if the authorization fails; otherwise returns an empty struct.


I don't understand this.

The convention is that functions reutrn <zero value AKA nil here>, error or error and _, nil on success. If nil, nil is illegal, then that makes the API confusing and hard to use -- in which case you should have it return a value instead of a pointer.

It returns nil, error in the case the authorization fails, <currently empty struct>, nil on success

We discussed and don't think there is harm in keeping the struct even though it's currently unused - there's a good chance it could be in the future and it's already implemented as such

OK - it's a little unusual but should be OK. If that's what you want you should make sure you're panicking if you call it and the function ever returns nil, nil.

I don't think there's a compelling reason to return a pointer vs. a struct if that is what is making this unclear, but that would represent a breaking behavior change. Looking at it, I don't think nil, nil is illegal, it just means there's no information to pass back out.

I added some text to the comment to make it a little more clear.

That being said, looking at it again today with fresh eyes, it's called here in our code. And the struct is always thrown away. So I'm rethinking the value of keeping this in the API. Will discuss with my team and add another comment soon.

It says "otherwise returns an empty struct" which implies that nil, nil is invalid. If nil, nil is valid, then you can just delete this part of the documentation. Returning <zero>, error or _, nil is just how regular Go conventions always work.

Ah I see, I'll remove that part and then this is resolved? I think that it's more pain that its worth to remove the struct return

SGTM. Maybe PostHandshakeVerificationFunc should return an error if the authorization should fail or If PostHandshakeVerificationFunc returns an error, authorization fails or something? (Something a user implements shouldn't be documented as "this is what it does" but "this is what you should make it do" or "this is how the library responds based on what it does".)

Re-worded, PTAL

dfawley · 2024-04-19T16:26:03Z

security/advancedtls/advancedtls.go

+// verification has been completed.
+// PostHandshakeVerificationFunc returns nil
+// if the authorization fails; otherwise returns an empty struct.
+type PostHandshakeVerificationFunc func(params *HandshakeVerificationInfo) (*PostHandshakeVerificationResults, error)


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.

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.

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?

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.

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).

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.

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

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

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

Renamed VerifyPeer in the settings struct, PTAL

dfawley · 2024-04-23T15:55:04Z

Can you please resolve the conflicts as well? Thanks!

gtcooke94 · 2024-04-23T16:54:23Z

Fixed merge conflicts, should be all good

dfawley · 2024-04-23T17:12:15Z

security/advancedtls/advancedtls.go

+	// 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.


VType->VerificationType?

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

dfawley · 2024-04-23T17:18:23Z

security/advancedtls/advancedtls.go

+	// 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.


VerificationType

dfawley · 2024-04-23T17:19:16Z

security/advancedtls/advancedtls.go

+	// 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


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 }

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"

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

gtcooke94 added 4 commits April 17, 2024 20:19

rename

5c7819f

replace usage of CustomVerificationFunc

79b35eb

replace usages

e1248b0

rename the results struct

0d6185d

gtcooke94 requested a review from erm-g April 17, 2024 20:41

gtcooke94 added Type: Internal Cleanup Refactors, etc Type: Security A bug or other problem affecting security labels Apr 17, 2024

gtcooke94 added this to the 1.64 Release milestone Apr 17, 2024

aranjans assigned erm-g Apr 18, 2024

erm-g requested changes Apr 18, 2024

View reviewed changes

erm-g self-requested a review April 18, 2024 18:09

gtcooke94 requested a review from dfawley April 18, 2024 18:10

erm-g approved these changes Apr 18, 2024

View reviewed changes

aranjans assigned dfawley Apr 19, 2024

dfawley reviewed Apr 19, 2024

View reviewed changes

dfawley assigned gtcooke94 and unassigned dfawley and erm-g Apr 19, 2024

address PR comments - deprecation comment structure

53323db

gtcooke94 assigned dfawley and unassigned gtcooke94 Apr 19, 2024

gtcooke94 requested a review from dfawley April 19, 2024 17:39

dfawley assigned gtcooke94 and unassigned dfawley Apr 19, 2024

gtcooke94 assigned dfawley and unassigned gtcooke94 Apr 19, 2024

dfawley assigned gtcooke94 and unassigned dfawley Apr 22, 2024

gtcooke94 added 2 commits April 22, 2024 20:43

Add more description to PostHandshakeVerificationFunc

5920acc

rename VerifyPeer option to AdditionalPeerVerification

010d89e

gtcooke94 added 2 commits April 23, 2024 15:48

clean up PostHandshakeVerificationFunc documentation

718e858

wording

6bcc5f8

dfawley assigned dfawley and unassigned gtcooke94 Apr 23, 2024

gtcooke94 added 2 commits April 23, 2024 16:35

merge master

e6f065e

missed saving a file

84aa942

dfawley reviewed Apr 23, 2024

View reviewed changes

dfawley assigned gtcooke94 and unassigned dfawley Apr 23, 2024

gtcooke94 added 2 commits April 23, 2024 17:55

VType -> VerificationType after merge

52dbf58

reverse precendence of deprecated option

bb4d7b2

gtcooke94 assigned dfawley Apr 23, 2024

gtcooke94 requested a review from dfawley April 23, 2024 17:59

dfawley approved these changes Apr 23, 2024

View reviewed changes

dfawley removed their assignment Apr 23, 2024

gtcooke94 merged commit d75b5e2 into grpc:master Apr 23, 2024
12 checks passed

arjan-bal mentioned this pull request Apr 24, 2024

advancedtls: fix docstring for VerificationResults #7168

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

advancedtls: Rename custom verification function APIs #7140

advancedtls: Rename custom verification function APIs #7140

gtcooke94 commented Apr 17, 2024 •

edited

Loading

erm-g Apr 18, 2024

gtcooke94 Apr 18, 2024

erm-g left a comment

dfawley Apr 19, 2024

gtcooke94 Apr 19, 2024

dfawley Apr 22, 2024

gtcooke94 Apr 22, 2024

dfawley Apr 23, 2024

gtcooke94 Apr 23, 2024

dfawley Apr 23, 2024

gtcooke94 Apr 23, 2024 •

edited

Loading

dfawley Apr 19, 2024

gtcooke94 Apr 19, 2024

dfawley Apr 19, 2024

gtcooke94 Apr 19, 2024

dfawley Apr 22, 2024

gtcooke94 Apr 23, 2024

gtcooke94 Apr 23, 2024

dfawley commented Apr 23, 2024

gtcooke94 commented Apr 23, 2024

dfawley Apr 23, 2024

gtcooke94 Apr 23, 2024

dfawley Apr 23, 2024

gtcooke94 Apr 23, 2024

dfawley Apr 23, 2024

gtcooke94 Apr 23, 2024

dfawley Apr 23, 2024

		// PostHandshakeVerificationFunc returns nil
		// if the authorization fails; otherwise returns an empty struct.

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

advancedtls: Rename custom verification function APIs #7140

advancedtls: Rename custom verification function APIs #7140

Conversation

gtcooke94 commented Apr 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erm-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtcooke94 Apr 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfawley commented Apr 23, 2024

gtcooke94 commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtcooke94 commented Apr 17, 2024 •

edited

Loading

gtcooke94 Apr 23, 2024 •

edited

Loading