credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels #3995

yihuazhang · 2020-10-28T13:13:56Z

This PR attempts to add the per-callcheck discussed in #3974.

yihuazhang · 2020-10-28T13:23:06Z

@dfawley Do you mind taking a look?

dfawley · 2020-10-28T16:36:16Z

I think there might be an issue with pointer-vs-struct receiver. Try making the insecure creds return &Info{} instead. (Also: I think that should be unexported -- AKA info.)

dfawley · 2020-10-28T16:37:11Z

(Also: I think that should be unexported -- AKA info.)

Or even: just delete it and return &CommonAuthInfo{...} directly?

yihuazhang · 2020-10-28T17:59:55Z

Thanks a lot @dfawley. Using &Info{} solves the problem.

I also tried directly returning &CommonAuthInfo{...} but it fails because the return type requires AuthInfo. Regarding the exposability, Info is also used in insecure_creds_test.go which is a part of test package.

If we decide to use &Info{}, then we probably need to modify all other credentials as well. There is also another problem that Peer struct now contains a pointer to AuthInfo which may break existing users if they want to perform type casting.

dfawley · 2020-10-28T18:33:30Z

I also tried directly returning &CommonAuthInfo{...} but it fails because the return type requires AuthInfo.

We could make this implement AuthInfo easily enough. I'm not sure anyone ever calls AuthType() except our tests. I honestly don't know why it exists. Maybe for theoretical informational logging?

Info is also used in insecure_creds_test.go which is a part of test package.

We could change this.

If we decide to use &Info{}, then we probably need to modify all other credentials as well. There is also another problem that Peer struct now contains a pointer to AuthInfo which may break existing users if they want to perform type casting.

I don't understand this. Why modify all other credentials? What does Peer.AuthInfo have to do with this?

yihuazhang · 2020-10-28T18:55:53Z

PCMIIW, for other credentials, shouldn't we also return a pointer to a corresponding AuthInfo for ClientHandshake and ServerHandshake, similar to what we have done for insecure creds? Otherwise, they do not trigger the core logic of security level comparison.

Regarding peer.AuthInfo, if we return &Info{}, Peer will contain a pointer to AuthInfo, instead of AuthInfo itself. So if we want to do type casting similar to here, it will return an error asking to cast it to *insecure.Info.

dfawley · 2020-10-29T21:20:31Z

PCMIIW, for other credentials, shouldn't we also return a pointer to a corresponding AuthInfo for ClientHandshake and ServerHandshake, similar to what we have done for insecure creds? Otherwise, they do not trigger the core logic of security level comparison.

Hmm, I think the fix for everyone here might be to make GetCommonAuthInfo use a non-pointer receiver here:

grpc-go/credentials/credentials.go

Line 95 in 89faf1c

func (c *CommonAuthInfo) GetCommonAuthInfo() *CommonAuthInfo {

We should also be testing our creds implementations to make sure AuthInfo().GetCommonAuthInfo() works if we are not already.

Regarding peer.AuthInfo, if we return &Info{}, Peer will contain a pointer to AuthInfo, instead of AuthInfo itself. So if we want to do type casting similar to here, it will return an error asking to cast it to *insecure.Info.

No, if we need to return &Info{} then we'll also need to make the AuthType method a pointer receiver. But if we change what I suggested above than we can return Info{} and leave this alone.

We could make this implement AuthInfo easily enough.

Actually, let's not do this. But I think we should unexport Info since nobody should ever need it. The tests don't truly need it either.

yihuazhang · 2020-10-30T23:00:14Z

PCMIIW, for other credentials, shouldn't we also return a pointer to a corresponding AuthInfo for ClientHandshake and ServerHandshake, similar to what we have done for insecure creds? Otherwise, they do not trigger the core logic of security level comparison.

Hmm, I think the fix for everyone here might be to make GetCommonAuthInfo use a non-pointer receiver here:

grpc-go/credentials/credentials.go

Line 95 in 89faf1c

func (c *CommonAuthInfo) GetCommonAuthInfo() *CommonAuthInfo {

Thanks Doug. The problem is resolved after following your suggestion!

We should also be testing our creds implementations to make sure AuthInfo().GetCommonAuthInfo() works if we are not already.

Agreed. There are 6 credentials to be tested. Do you want me to fix them in this PR or a separate one? I am fine with either way.

Regarding peer.AuthInfo, if we return &Info{}, Peer will contain a pointer to AuthInfo, instead of AuthInfo itself. So if we want to do type casting similar to here, it will return an error asking to cast it to *insecure.Info.

No, if we need to return &Info{} then we'll also need to make the AuthType method a pointer receiver. But if we change what I suggested above than we can return Info{} and leave this alone.

We could make this implement AuthInfo easily enough.

Actually, let's not do this. But I think we should unexport Info since nobody should ever need it. The tests don't truly need it either.

I am wondering if we should keep it as it is (keep exporting) to make it consistent with other credentials (TLS)? Also, if we unexport it, every time we want to retrieve a security level from AuthInfo, we need to create a dummy interface containing GetCommonAuthInfo() and cast AuthInfo to it (similar to what we have done here), which seems to require more work than just casting to Info and directly access the security level. HDYT?

dfawley · 2020-10-30T23:12:02Z

Do you want me to fix them in this PR or a separate one? I am fine with either way.

Whatever is easier for you is fine.

I am wondering if we should keep it as it is (keep exporting) to make it consistent with other credentials (TLS)?

Unless there is useful information in it, there should be no reason to export it.

Also, if we unexport it, every time we want to retrieve a security level from AuthInfo, we need to create a dummy interface containing GetCommonAuthInfo() and cast AuthInfo to it (similar to what we have done here), which seems to require more work than just casting to Info and directly access the security level. HDYT?

Really we don't expect anybody to be doing this, though, besides CheckSecurityLevel, right? And to be a generic "figure out creds security level" will require this type assertion. We can also add either another helper to credentials or an interface definition with GetCommonAuthInfo if it turns out to be useful.

yihuazhang · 2020-11-02T02:11:15Z

Do you want me to fix them in this PR or a separate one? I am fine with either way.

Whatever is easier for you is fine.

SG. Let me handle them in a separate PR.

I am wondering if we should keep it as it is (keep exporting) to make it consistent with other credentials (TLS)?

Unless there is useful information in it, there should be no reason to export it.

Also, if we unexport it, every time we want to retrieve a security level from AuthInfo, we need to create a dummy interface containing GetCommonAuthInfo() and cast AuthInfo to it (similar to what we have done here), which seems to require more work than just casting to Info and directly access the security level. HDYT?

Really we don't expect anybody to be doing this, though, besides CheckSecurityLevel, right? And to be a generic "figure out creds security level" will require this type assertion. We can also add either another helper to credentials or an interface definition with GetCommonAuthInfo if it turns out to be useful.

That makes sense. Let me add a helper function GetSecurityLevel(ai AuthInfo) to return a corresponding security level and unexport Info for both insecure and local credentials.

dfawley · 2020-11-02T18:28:46Z

GetSecurityLevel

Let's not add this until it's necessary. CheckSecurityLevel seems sufficient to me for now.

yihuazhang · 2020-11-03T16:58:32Z

GetSecurityLevel

Let's not add this until it's necessary. CheckSecurityLevel seems sufficient to me for now.

SG. I removed GetSecurityLevel. PTAL

credentials/credentials.go

dfawley · 2020-11-06T18:28:21Z

credentials/local/local_test.go

+		type internalInfo interface {
+			GetCommonAuthInfo() credentials.CommonAuthInfo
+		}
+		if info, ok := clientAuthInfo.(internalInfo); ok {
+			clientSecLevel = info.GetCommonAuthInfo().SecurityLevel
+		} else {
+			return credentials.Invalid, fmt.Errorf("Error at client-side: client's AuthInfo does not implement GetCommonAuthInfo()")
+		}


I'd say make a helper:

func getSecurityLevel(ai AuthInfo) credentials.SecurityLevel { if c, ok := ai.(interface { GetCommonAuthInfo() credentials.CommonAuthInfo }); ok { return c.GetCommonAuthInfo().SecurityLevel } return credentials.InvalidSecurityLevel // Let's rename this enum value; it's not specific enough to be exported }

Ah that's much more concise. Done!

dfawley · 2020-11-06T18:32:04Z

internal/transport/http2_client.go

@@ -231,9 +231,22 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
 		contextWithHandshakeInfo := internal.NewClientHandshakeInfoContext.(func(context.Context, credentials.ClientHandshakeInfo) context.Context)
 		connectCtx = contextWithHandshakeInfo(connectCtx, credentials.ClientHandshakeInfo{Attributes: addr.Attributes})
 		conn, authInfo, err = transportCreds.ClientHandshake(connectCtx, addr.ServerName, conn)
+		type internalInfo interface {


Hmmmmm, maybe CheckSecurityLevel should accept an AuthInfo instead of a Context. Does this cause too many changes?

Sure, consider it done!

dfawley · 2020-11-07T03:22:49Z

internal/transport/http2_client.go

@@ -241,7 +241,7 @@ func newHTTP2Client(connectCtx, ctx context.Context, addr resolver.Address, opts
 			if cd.RequireTransportSecurity() {
 				if ci, ok := authInfo.(internalInfo); ok {
 					secLevel := ci.GetCommonAuthInfo().SecurityLevel
-					if secLevel != credentials.Invalid && secLevel < credentials.PrivacyAndIntegrity {
+					if secLevel != credentials.InvalidSecurityLevel && secLevel < credentials.PrivacyAndIntegrity {


I was thinking we could use CheckSecurityeLevel here, but I see this is different from CheckSecurityLevel because Invalid is not okay here. LGTM, but please move internalInfo so it's defined immediately above to limit its scope (or use it inline).

easwars

LGTM

yihuazhang · 2020-11-09T17:22:40Z

Thanks a lot for the review, @dfawley and @easwars.

yihuazhang · 2020-11-09T18:08:40Z

It seems the test failure in xds_server_integration_test.go is not related to this PR. Is it a known issue?

easwars · 2020-11-09T18:12:58Z

It seems the test failure in xds_server_integration_test.go is not related to this PR. Is it a known issue?

Yes, that failure is not related. I filed an issue to track that: #4022.
Thanks.

…urity levels (grpc#3995)

yihuazhang force-pushed the security_level_fix branch from 9ef1e2b to 386b266 Compare October 30, 2020 22:49

yihuazhang changed the title ~~Fix the interaction between legacy PerRPCCredentials with RequireTransportSecurity and security level (DO NOT SUBMIT)~~ Fix the interaction between legacy PerRPCCredentials with RequireTransportSecurity and security level Oct 30, 2020

dfawley added the Type: Bug label Oct 30, 2020

dfawley added this to the 1.34 Release milestone Oct 30, 2020

dfawley changed the title ~~Fix the interaction between legacy PerRPCCredentials with RequireTransportSecurity and security level~~ credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels Oct 30, 2020

yihuazhang force-pushed the security_level_fix branch from f5c5fae to 4e0b213 Compare November 2, 2020 02:18

fix PerRPCCredentials w/RequireTransportSecurity and security levels

21ae702

yihuazhang force-pushed the security_level_fix branch from 4e0b213 to 21ae702 Compare November 3, 2020 16:57

fix a lint error

fe9161c

easwars reviewed Nov 3, 2020

View reviewed changes

credentials/credentials.go Show resolved Hide resolved

dfawley self-assigned this Nov 5, 2020

dfawley reviewed Nov 6, 2020

View reviewed changes

dfawley assigned yihuazhang and unassigned dfawley Nov 6, 2020

yihuazhang added 2 commits November 6, 2020 16:21

address Doug's comments

b91dfca

sync to upstream

d668845

yihuazhang removed their assignment Nov 7, 2020

dfawley approved these changes Nov 7, 2020

View reviewed changes

dfawley assigned easwars and yihuazhang Nov 7, 2020

easwars approved these changes Nov 9, 2020

View reviewed changes

easwars removed their assignment Nov 9, 2020

make internalInfo inline

51a2458

yihuazhang removed their assignment Nov 9, 2020

dfawley merged commit aeb0479 into grpc:master Nov 9, 2020

davidkhala pushed a commit to Hyperledger-TWGC/grpc that referenced this pull request Dec 7, 2020

credentials: fix PerRPCCredentials w/RequireTransportSecurity and sec…

14db5e4

…urity levels (grpc#3995)

github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels #3995

credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels #3995

yihuazhang commented Oct 28, 2020 •

edited

yihuazhang commented Oct 28, 2020

dfawley commented Oct 28, 2020

dfawley commented Oct 28, 2020

yihuazhang commented Oct 28, 2020

dfawley commented Oct 28, 2020

yihuazhang commented Oct 28, 2020

dfawley commented Oct 29, 2020

yihuazhang commented Oct 30, 2020 •

edited

dfawley commented Oct 30, 2020

yihuazhang commented Nov 2, 2020 •

edited

dfawley commented Nov 2, 2020

yihuazhang commented Nov 3, 2020

dfawley Nov 6, 2020

yihuazhang Nov 7, 2020

dfawley Nov 6, 2020

yihuazhang Nov 7, 2020

dfawley Nov 7, 2020

easwars left a comment

yihuazhang commented Nov 9, 2020

yihuazhang commented Nov 9, 2020

easwars commented Nov 9, 2020

credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels #3995

credentials: fix PerRPCCredentials w/RequireTransportSecurity and security levels #3995

Conversation

yihuazhang commented Oct 28, 2020 • edited

yihuazhang commented Oct 28, 2020

dfawley commented Oct 28, 2020

dfawley commented Oct 28, 2020

yihuazhang commented Oct 28, 2020

dfawley commented Oct 28, 2020

yihuazhang commented Oct 28, 2020

dfawley commented Oct 29, 2020

yihuazhang commented Oct 30, 2020 • edited

dfawley commented Oct 30, 2020

yihuazhang commented Nov 2, 2020 • edited

dfawley commented Nov 2, 2020

yihuazhang commented Nov 3, 2020

dfawley Nov 6, 2020

Choose a reason for hiding this comment

yihuazhang Nov 7, 2020

Choose a reason for hiding this comment

dfawley Nov 6, 2020

Choose a reason for hiding this comment

yihuazhang Nov 7, 2020

Choose a reason for hiding this comment

dfawley Nov 7, 2020

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

yihuazhang commented Nov 9, 2020

yihuazhang commented Nov 9, 2020

easwars commented Nov 9, 2020

yihuazhang commented Oct 28, 2020 •

edited

yihuazhang commented Oct 30, 2020 •

edited

yihuazhang commented Nov 2, 2020 •

edited