credentials: create API for transport security level information #3214

yihuazhang · 2019-11-27T00:59:28Z

This PR does the following:

Define a tri-state security level (SecurityNone, IntegrityOnly and PrivacyAndIntegrity) indicating the protection level of an established channel.
Create ConnAuthInfo type struct that is a wrapper of credentials.AuthInfo and also contains the security level of an established channel. Existing TransportCredentials implementations are modified to return ConnAuthInfo upon finishing the handshake.
Associate a security level to each credentials.PerRPCCredentials implementations (JWT, Oauth2).
Enforce the security level check when attempting to send PerRPCCredentials on a channel. For backward-compatibility we sill preserve the original checking behavior (i.e., using RequreTransportSecurity()).

yihuazhang · 2019-12-05T22:13:42Z

Friendly ping :)

dfawley

Please make sure travis is green. From the error it seems like gofmt wasn't run on grpclb_test. You can run vet.sh locally before pushing updates. (I'd recommend running via VET_SKIP_PROTO=1 vet.sh to skip some slower checks that shouldn't affect you.)

jiangtaoli2016

Overall looks good. One question:

In clientconn.go, it checks if any PerRPCCredentials require transport security, and fails early.
Do we also want to check SecurityLevel there as well?

jiangtaoli2016 · 2019-12-10T22:53:47Z

credentials/credentials.go

+// EnableSecurityLevelCheck checks if security level examination should be enabled. It will be enabled
+// only if 1) AuthInfo is an instance of ConnAuthInfo and 2) rpcCred implements
+// MinimumSecurityLevel interface.
+func EnableSecurityLevelCheck(authInfo AuthInfo, rpcCred PerRPCCredentials) bool {


ShouldCheckSecurityLevel maybe a better name?

Hi Jiangtao, we decided not to do a centralized check, but let each PerRPCCredentials to implement its own check in GetRequestMetadata().

yihuazhang · 2019-12-13T21:02:14Z

@dfawley The PR is updated per our off-line discussion. Could you please take a look?

credentials/credentials.go

dfawley · 2019-12-13T21:33:32Z

credentials/credentials.go

+
+// CommonAuthInfo contains authenticated information common to AuthInfo implementations.
+type CommonAuthInfo struct {
+	Level SecurityLevel


Nit: SecurityLevel SecurityLevel. IMO "Level" by itself is not distinct enough.

yihuazhang · 2019-12-17T17:44:05Z

@dfawley The comment is addressed. PTAL. Also, the failed tests do not seem to relate to my PR.

dfawley · 2019-12-18T21:03:16Z

credentials/credentials_test.go

+	return "testAuthInfo"
+}
+
+func TestCheckSecurityLevel(t *testing.T) {


Please use table-style tests:

testCases := []struct{ authLevel SecurityLevel testLevel SecurityLevel want bool }{{ authLevel: PrivacyAndIntegrity, testLevel: PrivacyAndIntegrity, want: true }, { authLevel: IntegrityOnly, testLevel: PrivacyAndIntegrity, want: false } .... } for _, tc := range testCases { // make testAuthInfo, call CheckSecurityLevel, check result }

This doesn't have to be exhaustive, but should check a few different cases.

credentials/tls.go

credentials/credentials.go

dfawley · 2019-12-18T21:54:52Z

credentials/credentials.go

+}
+
+// CommonInfo returns the pointer to CommonAuthInfo struct.
+func (c *CommonAuthInfo) CommonInfo() *CommonAuthInfo {


My mistake: s/CommonInfo/CommonAuthInfo/g please. CommonInfo() doesn't seem sufficient.

Per our offline discussion, the name is changed to commonAuthInfo()

credentials/tls.go

yihuazhang · 2019-12-19T17:13:13Z

Thanks Doug for the comments and I am currently working on addressing them. I will let you know once it's ready.

yihuazhang · 2019-12-20T01:47:23Z

All comments are addressed, PTAL. @dfawley

credentials/tls.go

dfawley · 2019-12-20T16:59:00Z

credentials/credentials.go

+}
+
+// CommonAuthInfo returns the pointer to CommonAuthInfo struct.
+func (c *CommonAuthInfo) commonAuthInfo() *CommonAuthInfo {


Sorry, bad suggestion on my part. I think it will make sense to allow users to access this in an AuthInfo, if present. GetCommonAuthInfo should be fine. It seems the accessor-style name is unavoidable here.

No worries. Renaming is done in the next commit.

dfawley · 2019-12-20T16:59:47Z

credentials/credentials.go

+	case PrivacyAndIntegrity:
+		return "PrivacyAndIntegrity"
+	}
+	return ""


fmt.Sprintf("invalid SecurityLevel: %v", int(s))

Done in the next commit.

credentials/credentials.go

dfawley · 2019-12-20T17:04:50Z

credentials/credentials.go

@@ -50,6 +51,49 @@ type PerRPCCredentials interface {
 	RequireTransportSecurity() bool
 }

+// SecurityLevel defines the protection level on an established channel.


Throughout: please don't use "channel". gRPC-Go does not use that term. You can use "connection" for an individual connection or "ClientConn" for a grpc.ClientConn connected to potentially many backends.

Done in the next commit.

yihuazhang · 2019-12-20T19:01:11Z

All comments are addressed (again :) PTAL.

dfawley · 2019-12-20T20:44:59Z

FYI: travis is red:

+misspell -error .
credentials/credentials.go:189:114: "compatability" is a misspelling of "compatibility"

dfawley

A few more small things, then I think this should be good to go.

dfawley · 2019-12-20T20:48:16Z

credentials/credentials.go

@@ -127,6 +169,8 @@ type Bundle interface {
 type RequestInfo struct {
 	// The method passed to Invoke or NewStream for this RPC. (For proto methods, this has the format "/some.Service/Method")
 	Method string
+	// AuthInfo contains the information resulted from a security handshake (TransportCredentials.ClientHandshake, TransportCredentials.ServerHandshake)


s/resulted//.

dfawley · 2019-12-20T20:53:42Z

credentials/oauth/oauth.go

@@ -42,6 +42,9 @@ func (ts TokenSource) GetRequestMetadata(ctx context.Context, uri ...string) (ma
 	if err != nil {
 		return nil, err
 	}
+	if !credentials.CheckSecurityLevel(ctx, credentials.PrivacyAndIntegrity) {
+		return nil, fmt.Errorf("connection is not secure enough to transfer TokenSource PerRPCCredentials which requires PrivacyAndIntegrity")


Hmmmm.. ideally this would show more information, namely the SecurityLevel of the connection.

CheckSecurityLevel could return an error instead of bool to facilitate this.

Maybe something like fmt.Errorf("requires SecurityLevel %v; connection has %v", requiredLevel, hasLevel)? And this can wrap: fmt.Errorf("unable to transfer TokenSource PerRPCCredentials: %v", err)?

Good point. CheckSecurityLevel now returns error instead of bool, and more complete error messages are printed in oauth.go.

credentials/credentials.go

dfawley · 2019-12-20T22:42:53Z

credentials/credentials.go

+	}
+	ri, _ := RequestInfoFromContext(ctx)
+	if ri.AuthInfo == nil {
+		return errors.New("RequestInfo does not contain AuthInfo struct")


I think the only real way to get here is if ctx does not contain a RequestInfo (i.e. it was called with the wrong context). I would say "unable to obtain SecurityLevel from context".

yihuazhang · 2019-12-21T00:03:24Z

Thanks much @dfawley for detailed reviews!

yihuazhang · 2020-01-02T21:02:19Z

@dfawley Do you have any other comments on the PR?

dfawley

Please add "experimental" comments on SecurityLevel and CommonAuthInfo, otherwise LGTM.

yihuazhang force-pushed the security_level_negotiation branch from 4ebef55 to f12549f Compare November 27, 2019 01:05

cesarghali requested review from dfawley and jiangtaoli2016 December 4, 2019 17:46

cesarghali added no release notes Type: Feature New features or improvements in behavior labels Dec 4, 2019

menghanl assigned dfawley Dec 5, 2019

dfawley reviewed Dec 9, 2019

View reviewed changes

jiangtaoli2016 reviewed Dec 10, 2019

View reviewed changes

yihuazhang force-pushed the security_level_negotiation branch from 815bfb3 to e501cd9 Compare December 13, 2019 19:20

dfawley reviewed Dec 13, 2019

View reviewed changes

dfawley reviewed Dec 18, 2019

View reviewed changes

dfawley assigned yihuazhang and unassigned dfawley Dec 19, 2019

yihuazhang force-pushed the security_level_negotiation branch from 59d169c to 3a03528 Compare December 20, 2019 01:46

dfawley reviewed Dec 20, 2019

View reviewed changes

yihuazhang changed the title ~~Enforce security level negotiation between call credentials and channel~~ Enforce security level negotiation between call credentials and connection Dec 20, 2019

dfawley reviewed Dec 20, 2019

View reviewed changes

add security level negotation

bf6cafe

yihuazhang force-pushed the security_level_negotiation branch from bef9360 to bf6cafe Compare December 21, 2019 00:02

dfawley reviewed Jan 7, 2020

View reviewed changes

Add experimental comment

431178d

dfawley approved these changes Jan 7, 2020

View reviewed changes

dfawley changed the title ~~Enforce security level negotiation between call credentials and connection~~ credentials: create API for transport security level information Jan 7, 2020

dfawley merged commit 4346c59 into grpc:master Jan 7, 2020

dfawley removed the no release notes label Jan 7, 2020

dfawley added this to the 1.27 Release milestone Jan 7, 2020

tbg mentioned this pull request May 15, 2020

kvserver: authN and authZ for incoming tenant connections cockroachdb/cockroach#47898

Closed

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: create API for transport security level information #3214

credentials: create API for transport security level information #3214

yihuazhang commented Nov 27, 2019 •

edited

Loading

yihuazhang commented Dec 5, 2019

dfawley left a comment

jiangtaoli2016 left a comment

jiangtaoli2016 Dec 10, 2019

yihuazhang Dec 13, 2019

yihuazhang commented Dec 13, 2019

dfawley Dec 13, 2019

yihuazhang commented Dec 17, 2019

dfawley Dec 18, 2019

dfawley Dec 18, 2019

yihuazhang Dec 19, 2019

yihuazhang Dec 20, 2019

yihuazhang commented Dec 19, 2019

yihuazhang commented Dec 20, 2019

dfawley Dec 20, 2019

yihuazhang Dec 20, 2019

dfawley Dec 20, 2019

yihuazhang Dec 20, 2019

dfawley Dec 20, 2019

yihuazhang Dec 20, 2019

yihuazhang commented Dec 20, 2019

dfawley commented Dec 20, 2019

dfawley left a comment

dfawley Dec 20, 2019

yihuazhang Dec 20, 2019

dfawley Dec 20, 2019

yihuazhang Dec 20, 2019

dfawley Dec 20, 2019

yihuazhang Dec 21, 2019

yihuazhang commented Dec 21, 2019

yihuazhang commented Jan 2, 2020

dfawley left a comment

credentials: create API for transport security level information #3214

credentials: create API for transport security level information #3214

Conversation

yihuazhang commented Nov 27, 2019 • edited Loading

yihuazhang commented Dec 5, 2019

dfawley left a comment

Choose a reason for hiding this comment

jiangtaoli2016 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuazhang commented Dec 13, 2019

Choose a reason for hiding this comment

yihuazhang commented Dec 17, 2019

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

yihuazhang commented Dec 19, 2019

yihuazhang commented Dec 20, 2019

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

yihuazhang commented Dec 20, 2019

dfawley commented Dec 20, 2019

dfawley 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

yihuazhang commented Dec 21, 2019

yihuazhang commented Jan 2, 2020

dfawley left a comment

Choose a reason for hiding this comment

yihuazhang commented Nov 27, 2019 •

edited

Loading