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.

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

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

Merged
merged 5 commits into from Nov 9, 2020

Conversation

@yihuazhang
Copy link
Contributor

@yihuazhang yihuazhang commented Oct 28, 2020

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

@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Oct 28, 2020

@dfawley Do you mind taking a look?

@dfawley
Copy link
Contributor

@dfawley dfawley commented Oct 28, 2020

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
Copy link
Contributor

@dfawley dfawley commented Oct 28, 2020

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

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

@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Oct 28, 2020

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
Copy link
Contributor

@dfawley dfawley commented Oct 28, 2020

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
Copy link
Contributor Author

@yihuazhang yihuazhang commented Oct 28, 2020

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
Copy link
Contributor

@dfawley dfawley commented Oct 29, 2020

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:

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 yihuazhang force-pushed the security_level_fix branch from 9ef1e2b to 386b266 Oct 30, 2020
@yihuazhang 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
@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Oct 30, 2020

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:

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
Copy link
Contributor

@dfawley dfawley commented Oct 30, 2020

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.

@dfawley dfawley added this to the 1.34 Release milestone Oct 30, 2020
@dfawley 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
Copy link
Contributor Author

@yihuazhang yihuazhang commented Nov 2, 2020

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.

@yihuazhang yihuazhang force-pushed the security_level_fix branch from f5c5fae to 4e0b213 Nov 2, 2020
@dfawley
Copy link
Contributor

@dfawley dfawley commented Nov 2, 2020

GetSecurityLevel

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

@yihuazhang yihuazhang force-pushed the security_level_fix branch from 4e0b213 to 21ae702 Nov 3, 2020
@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Nov 3, 2020

GetSecurityLevel

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

SG. I removed GetSecurityLevel. PTAL

@dfawley dfawley self-assigned this Nov 5, 2020
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()")
}
Copy link
Contributor

@dfawley dfawley Nov 6, 2020

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
}

Copy link
Contributor Author

@yihuazhang yihuazhang Nov 7, 2020

Ah that's much more concise. Done!

@@ -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 {
Copy link
Contributor

@dfawley dfawley Nov 6, 2020

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

Copy link
Contributor Author

@yihuazhang yihuazhang Nov 7, 2020

Sure, consider it done!

@dfawley dfawley assigned yihuazhang and unassigned dfawley Nov 6, 2020
@yihuazhang yihuazhang removed their assignment Nov 7, 2020
dfawley
dfawley approved these changes Nov 7, 2020
@@ -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 {
Copy link
Contributor

@dfawley dfawley Nov 7, 2020

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
easwars approved these changes Nov 9, 2020
Copy link
Contributor

@easwars easwars left a comment

LGTM

@easwars easwars removed their assignment Nov 9, 2020
@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Nov 9, 2020

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

@yihuazhang
Copy link
Contributor Author

@yihuazhang yihuazhang commented Nov 9, 2020

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

@yihuazhang yihuazhang removed their assignment Nov 9, 2020
@easwars
Copy link
Contributor

@easwars easwars commented Nov 9, 2020

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.

@dfawley dfawley merged commit aeb0479 into grpc:master Nov 9, 2020
11 checks passed
davidkhala added a commit to Hyperledger-TWGC/grpc that referenced this issue Dec 7, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants