credentials: local creds implementation #3517

yihuazhang · 2020-04-10T21:13:19Z

This PR implements gRPC Go local credentials.

The local credentials should be used in either a UDS and local TCP connection. The former will be associated with the security level PrigvacyAndIntegrity while the latter is associated with NoSecurity. Notice that local credentials will be used to migrate the call sites of WithInsecure used in local connections.

yihuazhang · 2020-04-10T21:15:24Z

@dfawley, could you please review this PR?

@jiangtaoli2016 F.Y.I.

credentials/local/local.go

credentials/local/local_test.go

credentials/local/local.go

credentials/local/local_test.go

credentials/local/local.go

credentials/local/local_test.go

yihuazhang · 2020-04-13T15:13:04Z

Thanks @easwars for the review! All comments are addressed.

credentials/local/local_test.go

easwars

Changes look good to me. Please do wait for either Doug or Jiangtao to approve as well. Thank you.

credentials/local/local_test.go

yihuazhang · 2020-04-15T00:17:16Z

Friendly ping @dfawley

dfawley

Could you please also add a test in grpc/test as an end-to-end test? I.e. grpc.Dial with these credentials on localhost, uds, and maybe (if possible) a non-127-loopback address to make sure they work as expected. Thanks!

credentials/local/local.go

dfawley · 2020-04-17T19:55:03Z

credentials/local/local.go

+func getSecurityLevel(network, addr string) (credentials.SecurityLevel, error) {
+	switch {
+	// Local TCP connection
+	case strings.HasPrefix(addr, "127."), strings.HasPrefix(addr, "localhost:"), strings.HasPrefix(addr, "[::1]:"):


Is addr the address as passed to net.Dial?

If so, the following will not work:

/etc/hosts: 127.0.0.1 workstation foo.go: grpc.Dial("workstation", local.NewCredentials())

Maybe this is fine, but I wanted to point that out. If there's a way to retrieve the actual IP address, that would be better.

No, it's not the address passed to net.dial but the one derived from RemoteAddr().String() of an established net.Conn, which I believe is the actual IP address.

If that is the case, then is "localhost:" needed?

yihuazhang · 2020-04-22T01:15:40Z

Thanks @dfawley for the review. All comments are addressed.

In end2end_test.go, I skipped TestServerMaxHeaderListSizeClientIntentionalViolation and TestClientMaxHeaderListSizeServerIntentionalViolation for local credentials as it results in timeout for UDS connections and I could not figure out why. Could you please shed some lights on it?

dfawley · 2020-04-22T16:10:51Z

Please resolve the conflicts in your branch to unblock travis.

dfawley · 2020-04-22T16:16:31Z

test/end2end_test.go

@@ -626,6 +662,9 @@ func (te *test) listenAndServe(ts testpb.TestServiceServer, listen func(network,
 			te.t.Fatalf("Failed to generate credentials %v", err)
 		}
 		sopts = append(sopts, grpc.Creds(creds))
+	case "local":
+		creds := local.NewCredentials()


Nit: remove local variable

dfawley · 2020-04-22T16:16:36Z

test/end2end_test.go

@@ -803,6 +842,9 @@ func (te *test) configDial(opts ...grpc.DialOption) ([]grpc.DialOption, string)
 			te.t.Fatalf("Failed to load credentials: %v", err)
 		}
 		opts = append(opts, grpc.WithTransportCredentials(creds))
+	case "local":
+		creds := local.NewCredentials()


Nit: remove local variable

test/end2end_test.go

credentials/local/local.go

dfawley · 2020-04-22T16:42:50Z

credentials/local/local.go

+	return "local"
+}
+
+// localTC is the credentials required to eatablish a local connection.


dfawley · 2020-04-22T16:44:30Z

credentials/local/local.go

+func getSecurityLevel(network, addr string) (credentials.SecurityLevel, error) {
+	switch {
+	// Local TCP connection
+	case strings.HasPrefix(addr, "127."), strings.HasPrefix(addr, "localhost:"), strings.HasPrefix(addr, "[::1]:"):


If that is the case, then is "localhost:" needed?

dfawley · 2020-04-22T16:46:21Z

credentials/local/local.go

+		return credentials.PrivacyAndIntegrity, nil
+	// Not a local connection and should fail
+	default:
+		return credentials.Invalid, fmt.Errorf("credentials: local credentials should be used in a local connection")


Maybe: "local credentials rejected connection to non-local address %q", addr

dfawley · 2020-04-22T16:51:05Z

test/end2end_test.go

 	tcpClearRREnv = env{name: "tcp-clear", network: "tcp", balancer: "round_robin"}
 	tcpTLSRREnv   = env{name: "tcp-tls", network: "tcp", security: "tls", balancer: "round_robin"}
 	handlerEnv    = env{name: "handler-tls", network: "tcp", security: "tls", httpHandler: true, balancer: "round_robin"}
 	noBalancerEnv = env{name: "no-balancer", network: "tcp", security: "tls"}
-	allEnv        = []env{tcpClearEnv, tcpTLSEnv, tcpClearRREnv, tcpTLSRREnv, handlerEnv, noBalancerEnv}
+	allEnv        = []env{tcpClearEnv, tcpTLSEnv, tcpLocalEnv, tcpClearRREnv, tcpTLSRREnv, handlerEnv, noBalancerEnv}


Let's leave this out of allEnv entirely and just test it specifically in one test case.

yihuazhang · 2020-04-22T17:21:22Z

Thanks @dfawley. All comments are addressed. PTAL

dfawley · 2020-04-22T17:45:23Z

test/end2end_test.go

@@ -421,6 +448,7 @@ func (e env) dialer(addr string, timeout time.Duration) (net.Conn, error) {
 var (
 	tcpClearEnv   = env{name: "tcp-clear-v1-balancer", network: "tcp", balancer: "v1"}
 	tcpTLSEnv     = env{name: "tcp-tls-v1-balancer", network: "tcp", security: "tls", balancer: "v1"}
+	tcpLocalEnv   = env{name: "tcp-local-v1-balancer", network: "tcp", listenerAddr: "[::1]:0", security: "local", balancer: "v1"}


There is now no test that uses this. (And I think that's OK.)

Please add an end-to-end test specifically for the local credentials. It should ideally:

use local creds on client and server with a TCP dial to "localhost" that succeeds

use local creds on client and server with a UDS connection that succeeds

use local creds on client and server with a loopback-but-non-127.* TCP dial that fails (observe failure on client, then use WithInsecure on client and observe failure on server)

~~I'm not sure how to do that last one in a portable way, but it should be possible if you limit it to linux systems.~~
EDIT: by wrapping the listener (which wraps the returned conns) and the dialer, this should be fairly simple.

I think you can just do all this ~manually using a stubServer, and not modify the global testServer.

menghanl · 2020-04-30T20:49:39Z

Ping, please fix the conflicts and add tests. Thanks

yihuazhang · 2020-04-30T21:38:38Z

Sorry, I was distracted by another task. I will get back to it early next week.

yihuazhang · 2020-05-10T19:39:20Z

Sorry for the long delay. I just added E2E tests, and PTAL.

dfawley · 2020-05-13T16:21:02Z

credentials/local/local_test.go

+			}
+			got, err := serverAndClientHandshake(lis)
+			if got != tc.want {
+				t.Fatalf("ServerAndClientHandshake(%s, %s) returned %s but want %s. Error: %v", tc.testNetwork, tc.testAddr, got.String(), tc.want.String(), err)


Go style:

t.Fatalf("serverAndClientHandshake(%s, %s) = %v, %v; want %v, nil", tc.testNetwork, tc.testAddr, got, err, want)

dfawley · 2020-05-13T16:24:51Z

test/end2end_test.go

@@ -214,18 +215,43 @@ func (s *testServer) UnaryCall(ctx context.Context, in *testpb.SimpleRequest) (*
 	if s.security != "" {
 		// Check Auth info
 		var authType, serverName string
+		var secLevel credentials.SecurityLevel


Are these changes still necessary? If at all possible, we should avoid any changes to testServer. Use a stubServer instead.

I will revert the changes in end2end_test.go, and instead added the security level check (which is all we changed in end2end_test.go) to local_creds_test.go.

dfawley · 2020-05-13T16:29:26Z

test/local_creds_test.go

+	go s.Serve(lis)
+
+	var cc *grpc.ClientConn
+	if network == "unix" {


switch network { case "unix": ... case "tcp": ... default: return fmt.Errorf("unsupported network %q", network) }

Thanks for the suggestion. Done.

dfawley · 2020-05-13T16:31:45Z

test/local_creds_test.go

+
+	var cc *grpc.ClientConn
+	if network == "unix" {
+		cc, err = grpc.Dial("passthrough:///"+address, grpc.WithTransportCredentials(local.NewCredentials()), grpc.WithContextDialer(


passthrough is the default; do you need this?

No it's not needed. I removed it.

dfawley · 2020-05-13T16:33:08Z

test/local_creds_test.go

+}
+
+func (s) TestLocalhost(t *testing.T) {
+	err := testE2ESucceed("tcp", "localhost:0")


Nit:

if err := ...; err != nil {

dfawley · 2020-05-13T16:37:50Z

test/local_creds_test.go

+	if err != nil {
+		return nil, err
+	}
+	return connWrapper{c, remoteAddrs[1]}, nil


Similar, but unfortunately a little uglier:

func spoofDialer(addr string) func(string, time.Duration) (net.Conn, error) { return func(t string, d time.Duration) (net.Conn, error) { c, err := net.DialTimeout("tcp", t, d) if err != nil { return nil, err } return connWrapper{c, addr}, nil } } ``

Aha, good suggestion!

dfawley · 2020-05-13T16:40:38Z

test/local_creds_test.go

+	if useLocal {
+		cc, err = grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(local.NewCredentials()), grpc.WithDialer(dialer))
+	} else {
+		cc, err = grpc.Dial(lis.Addr().String(), grpc.WithInsecure(), grpc.WithDialer(dialer))
+	}


Idea: pass dopts []grpc.DialOption as a parameter to this function, then:

cc, err := grpc.Dial(lis.Addr().String(), append(dopts, grpc.WithDialer(dialer))...)

Good point. The logic will be simpler if we do that. Thanks.

dfawley · 2020-05-13T16:42:55Z

test/local_creds_test.go

+	// Use local creds at client-side which should lead to client-side failure.
+	err := testE2EFail(true /*useLocal*/)
+	if err == nil || !strings.Contains(err.Error(), "local credentials rejected connection to non-local address") {
+		t.Fatalf("testE2EFail(%v) = _; want security handshake fails, %v", false, err)


This looks wrong. You have the returned error being printed as the desired error.

Good catch. The test is revised.

dfawley · 2020-05-13T16:44:41Z

test/local_creds_test.go

+	// Use insecure at client-side which should lead to server-side failure.
+	err := testE2EFail(false /*useLocal*/)
+	if err == nil {
+		t.Fatalf("testE2EFail(%v) = _; want security handshake fails, %v", true, err)


Same. What kind of error do we expect to see here?

I think we should expected connection closed error. I updated the test.

dfawley · 2020-05-13T16:45:06Z

test/local_creds_test.go

+func (s) TestClientFail(t *testing.T) {
+	// Use local creds at client-side which should lead to client-side failure.
+	err := testE2EFail(true /*useLocal*/)
+	if err == nil || !strings.Contains(err.Error(), "local credentials rejected connection to non-local address") {


Should we also check status.Code(err)?

yihuazhang · 2020-05-13T22:09:53Z

All comments are addressed. PTAL.

dfawley · 2020-05-14T16:27:55Z

FYI: travis seems to be failing:

local_creds_test.go:216: testE2EFail() = rpc error: code = Unavailable desc = write tcp 127.0.0.1:34484->127.0.0.1:45229: write: broken pipe; want rpc error: code = Unavailable desc = connection closed

Seems like there's raciness determining what kind of error appears to the client when this happens.

dfawley · 2020-05-14T16:32:48Z

test/local_creds_test.go

+			switch network {
+			case "unix":
+				if secLevel != credentials.PrivacyAndIntegrity {
+					return nil, status.Errorf(codes.Unauthenticated, "Wrong security level: got %q, want %q", secLevel.String(), credentials.PrivacyAndIntegrity.String())


I am pretty sure fmt.Sprintf (which status.Errorf uses) will call .String() for you in this case (with %q) - won't it?

Yes, you are right. Thanks for the suggestion. The code is updated.

dfawley · 2020-05-14T16:34:10Z

test/local_creds_test.go

 			func(ctx context.Context, addr string) (net.Conn, error) {
 				return net.Dial("unix", address)
 			}))
-	} else {
+	case "tcp":
 		cc, err = grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(local.NewCredentials()))


Can this also dial address to be consistent with the above?

By dialing address it will dial with port 0, which is incorrect. I updated the test to use lisAddr in both unix and tcp cases.

dfawley · 2020-05-14T16:34:21Z

test/local_creds_test.go

 	}
-
 	if err != nil {
 		return fmt.Errorf("Failed to dial server: %v, %v", err, lis.Addr().String())


same: address ?

I updated it to use lisAddr instead.

dfawley · 2020-05-14T16:37:03Z

test/local_creds_test.go

+	if status.Code(got) == status.Code(want) && strings.Contains(status.Convert(got).Message(), status.Convert(want).Message()) {
+		return true
+	}
+	return false


Simplify: return <condition>

dfawley · 2020-05-14T19:54:11Z

test/local_creds_test.go

@@ -48,11 +49,11 @@ func testE2ESucceed(network, address string) error {
 			switch network {
 			case "unix":
 				if secLevel != credentials.PrivacyAndIntegrity {
-					return nil, status.Errorf(codes.Unauthenticated, "Wrong security level: got %q, want %q", secLevel.String(), credentials.PrivacyAndIntegrity.String())
+					return nil, status.Errorf(codes.Unauthenticated, fmt.Sprintf("Wrong security level: got %q, want %q", secLevel, credentials.PrivacyAndIntegrity))


status.Errorf uses fmt.Sprintf already. Remove this call here.

dfawley · 2020-05-14T19:55:07Z

test/local_creds_test.go

+		if err != nil {
+			return fmt.Errorf("Failed to parse listener address: %v", err)
+		}
+		lisAddr = "localhost:" + port


Can you not do: lisAddr = lis.Addr().String() and remove the parsing?

dfawley · 2020-05-14T19:57:19Z

test/local_creds_test.go

+	want := status.Error(codes.Unavailable, "")
 	if err := testE2EFail(opts); !isExpected(err, want) {


Simplify:

if err := testE2EFail(opts); status.Code(err) != codes.Unavailable {

yihuazhang · 2020-05-18T14:48:39Z

Friendly ping @dfawley

dfawley · 2020-05-19T19:32:27Z

test/local_creds_test.go

+	testpb "google.golang.org/grpc/test/grpc_testing"
+)
+
+func testE2ESucceed(network, address string) error {


I know this is in a file called local_creds_test.go, but it's in a huge testing package. Please give this a name that includes LocalCreds. Same for everything global in this file, especially the Test____ functions.

yihuazhang · 2020-05-20T22:46:56Z

Thanks a lot Doug for the detailed review! Really appreciate it.

This reverts commit 9eb3e7d.

easwars requested changes Apr 10, 2020

View reviewed changes

menghanl reviewed Apr 10, 2020

View reviewed changes

credentials/local/local.go Outdated Show resolved Hide resolved

credentials/local/local_test.go Outdated Show resolved Hide resolved

yihuazhang force-pushed the local_creds branch from f6f52c9 to 8b0cfea Compare April 13, 2020 15:12

easwars added the Type: Feature New features or improvements in behavior label Apr 13, 2020

easwars added this to the 1.30 Release milestone Apr 13, 2020

easwars requested changes Apr 13, 2020

View reviewed changes

easwars approved these changes Apr 13, 2020

View reviewed changes

credentials/local/local_test.go Outdated Show resolved Hide resolved

credentials/local/local_test.go Outdated Show resolved Hide resolved

menghanl assigned dfawley Apr 16, 2020

dfawley reviewed Apr 17, 2020

View reviewed changes

dfawley reviewed Apr 22, 2020

View reviewed changes

credentials/local/local.go Outdated Show resolved Hide resolved

dfawley reviewed Apr 22, 2020

View reviewed changes

credentials/local/local.go Outdated Show resolved Hide resolved

dfawley reviewed Apr 22, 2020

View reviewed changes

dfawley assigned yihuazhang and unassigned dfawley Apr 22, 2020

yihuazhang removed their assignment Apr 22, 2020

dfawley reviewed Apr 22, 2020

View reviewed changes

dfawley assigned yihuazhang Apr 22, 2020

dfawley added the Status: Requires Reporter Clarification label May 7, 2020

Fix conflicts with upstream master

f15d4ae

yihuazhang force-pushed the local_creds branch from 937ab5a to f15d4ae Compare May 8, 2020 20:10

add end2end test

9a865e9

fix a test failure

9482a13

yihuazhang removed their assignment May 11, 2020

menghanl assigned dfawley May 12, 2020

dfawley reviewed May 13, 2020

View reviewed changes

dfawley assigned yihuazhang and unassigned dfawley May 13, 2020

address doug's 2nd round comments

b81b36c

yihuazhang removed their assignment May 13, 2020

dfawley reviewed May 14, 2020

View reviewed changes

yihuazhang added 2 commits May 14, 2020 10:35

address doug's 3rd round of comments

876fdb9

fix travis error

e417e60

dfawley reviewed May 14, 2020

View reviewed changes

yihuazhang added 2 commits May 14, 2020 13:02

2nd attempt to fix travis error

11d7567

Address doug's 4th round of comments

3533a21

dfawley reviewed May 19, 2020

View reviewed changes

give a more meaningfule name to functions

2f261a3

yihuazhang mentioned this pull request May 20, 2020

Python experimental channel credential functions grpc/grpc#23000

Closed

dfawley approved these changes May 20, 2020

View reviewed changes

dfawley changed the title ~~Local credential implementations~~ credentials: local creds implementation May 20, 2020

dfawley merged commit 9eb3e7d into grpc:master May 20, 2020

yihuazhang added a commit to yihuazhang/grpc-go that referenced this pull request Jun 1, 2020

Revert "credentials: local creds implementation (grpc#3517)"

1878d6e

This reverts commit 9eb3e7d.

This was referenced Jun 1, 2020

Revert "credentials: local creds implementation" #3655

Closed

credentials/local: fix incorrect binding of listener port #3656

Merged

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

		want := status.Error(codes.Unavailable, "")
		if err := testE2EFail(opts); !isExpected(err, want) {

credentials: local creds implementation #3517

credentials: local creds implementation #3517

Conversation

yihuazhang commented Apr 10, 2020 • edited Loading

yihuazhang commented Apr 10, 2020

yihuazhang commented Apr 13, 2020

easwars left a comment

Choose a reason for hiding this comment

yihuazhang commented Apr 15, 2020

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

yihuazhang commented Apr 22, 2020

dfawley commented Apr 22, 2020

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 Apr 22, 2020

dfawley Apr 22, 2020 • edited Loading

Choose a reason for hiding this comment

menghanl commented Apr 30, 2020

yihuazhang commented Apr 30, 2020

yihuazhang commented May 10, 2020

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

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

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 May 13, 2020

dfawley commented May 14, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuazhang commented May 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yihuazhang commented May 20, 2020

yihuazhang commented Apr 10, 2020 •

edited

Loading

dfawley Apr 22, 2020 •

edited

Loading