ClientHandshake to return AuthInfo #956

MakMukhi · 2016-10-28T22:49:43Z

Issue #934

menghanl · 2016-10-29T00:12:48Z

credentials/credentials.go

@@ -167,7 +167,7 @@ func (c *tlsCreds) ClientHandshake(ctx context.Context, addr string, rawConn net
 	}
 	// TODO(zhaoq): Omit the auth info for client now. It is more for


remove this TODO

menghanl · 2016-10-29T00:15:02Z

credentials/credentials_test.go

+
+func TestTLSClientHandshakeReturnsAuthInfo(t *testing.T) {
+	localPort := ":5050"
+	tlsDir := "../test/testdata/"


define this as a file level const
const tlsDir := ...

menghanl · 2016-10-29T00:16:09Z

credentials/credentials_test.go

+func TestTLSClientHandshakeReturnsAuthInfo(t *testing.T) {
+	localPort := ":5050"
+	tlsDir := "../test/testdata/"
+	lis, err := net.Listen("tcp", localPort)


do listen() on localhost:0 and get the listening address from lis

menghanl · 2016-10-29T00:19:00Z

credentials/credentials_test.go

+		}
+		serverAuthInfo = TLSInfo{serverConn.ConnectionState()}
+	}()
+	defer lis.Close()


Move this to the place right after listen().
If something fails before this line, lis will not be closed().

menghanl · 2016-10-29T00:20:46Z

credentials/credentials_test.go

+	tlsDir := "../test/testdata/"
+	lis, err := net.Listen("tcp", localPort)
+	if err != nil {
+		t.Fatalf("Failed to start local server. Listener error: %v", err)


You can simplify the error message to be just
t.Fatalf("Failed to listen: %v", err)

menghanl · 2016-10-29T00:21:37Z

credentials/credentials_test.go

+		defer func() {
+			done <- true
+		}()
+		serverRawConn, _ := lis.Accept()


check the error here?

menghanl · 2016-10-29T00:26:43Z

credentials/credentials_test.go

+	case <-done:
+		// wait until server has populated the serverAuthInfo struct.
+	}
+	if authInfo.AuthType() != serverAuthInfo.AuthType() {


TLSInfo.AuthType always returns "tls".
How about comparing something inside ConnectionState instead? like Version?

menghanl · 2016-10-29T00:29:02Z

credentials/credentials_test.go

+		t.Fatalf("Error on client while handshake. Error: %v", err)
+	}
+	select {
+	case <-done:


the select is not necessary, just <-done.

menghanl · 2016-10-29T00:29:37Z

credentials/credentials_test.go

+		serverAuthInfo = TLSInfo{serverConn.ConnectionState()}
+	}()
+	defer lis.Close()
+	conn, err := net.Dial("tcp", localPort)


defer close this connection

iamqizhao · 2016-10-31T18:33:13Z

credentials/credentials_test.go

+		}()
+		serverRawConn, err := lis.Accept()
+		if err != nil {
+			t.Fatalf("Server failed to accept connection: %v", err)


Use t.Errorf

iamqizhao · 2016-10-31T18:33:25Z

credentials/credentials_test.go

+		serverConn := tls.Server(serverRawConn, serverTLS.(*tlsCreds).config)
+		serverErr := serverConn.Handshake()
+		if serverErr != nil {
+			t.Fatalf("Error on server while handshake. Error: %v", serverErr)


iamqizhao · 2016-10-31T18:35:14Z

credentials/credentials_test.go

+	}()
+	conn, err := net.Dial("tcp", lis.Addr().String())
+	if err != nil {
+		t.Fatalf("Client failed to connect to local server. Error: %v", err)


print lis.Addr().String() instead of "local server".

iamqizhao · 2016-10-31T18:44:16Z

credentials/credentials_test.go

+}
+
+func TestTLSServerHandshakeReturnsAuthInfo(t *testing.T) {
+	lis, err := net.Listen("tcp", "localhost:0")


The above comments apply to this test case too.

iamqizhao · 2016-10-31T18:45:06Z

credentials/credentials_test.go

+		t.Fatalf("Error on client while handshake. Error: %v", err)
+	}
+	// wait until server has populated the serverAuthInfo struct.
+	<-done


if err := <-done; err != nil { return }

debug debug

iamqizhao

With those 2 wrappers I mentioned in the comments, the existing 2 tests are just slightly more than calling these 2 wrappers and you can easily add the 3rd test I just mentioned in our talk.

iamqizhao · 2016-10-31T23:30:15Z

credentials/credentials_test.go

+	if err != nil {
+		t.Fatalf("Error on client while handshake. Error: %v", err)
+	}
+	// wait until server has populated the serverAuthInfo struct.


.... populated the server AuthInfo or failed.

iamqizhao · 2016-10-31T23:45:37Z

credentials/credentials_test.go

+		t.Fatalf("Failed to create server TLS. Error: %v", err)
+	}
+	var serverAuthInfo TLSInfo
+	errChan := make(chan error)


make(chan error, 1) so that this goroutine can finish even though the test fails at line 101.

iamqizhao · 2016-10-31T23:46:45Z

credentials/credentials_test.go

+		return
+	}
+	if authInfo.(TLSInfo).State.Version != serverAuthInfo.State.Version {
+		t.Fatalf("c.ClientHandshake(_, %v, _) = %v, want %v.", lis.Addr().String(), authInfo, serverAuthInfo)


%s for list.Addr().String()

iamqizhao · 2016-10-31T23:51:36Z

credentials/credentials_test.go

+	if err != nil {
+		t.Fatalf("Failed to create server TLS. Error: %v", err)
+	}
+	var serverAuthInfo TLSInfo


Use tls.ConnectionState directly?

iamqizhao · 2016-10-31T23:52:07Z

credentials/credentials_test.go

+	if err != nil {
+		t.Fatalf("Error on client while handshake. Error: %v", err)
+	}
+	authInfo := TLSInfo{clientConn.ConnectionState()}


use tls.ConnectionState directly?

iamqizhao · 2016-11-01T00:02:15Z

credentials/credentials_test.go

+			t.Errorf("Error on server while handshake. Error: %v", sErr)
+			return
+		}
+		serverAuthInfo = TLSInfo{serverConn.ConnectionState()}


The body can be written in a separate function like:

func serverHandle(hs func(net.Conn) (net.Conn, AuthInfo, error)) { ... }

iamqizhao · 2016-11-01T00:04:43Z

credentials/credentials_test.go

+	// wait until server has populated the serverAuthInfo struct.
+	if err = <-errChan; err != nil {
+		return
+	}


and the above client side logic can be wrapped into a function too:

func clientHandle(hs func(context.Context, string, net.Conn) (net.Conn, AuthInfo, error))

iamqizhao · 2016-11-09T23:28:54Z

credentials/credentials_test.go

+func launchServer(t *testing.T, serverConnState *tls.ConnectionState, hs serverHandshake, errChan chan error) (string, error) {
+	lis, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Errorf("Failed to listen: %v", err)


you can t.Fatalf directly so that this function only needs to return lis.Addr().String().

iamqizhao · 2016-11-09T23:31:03Z

credentials/credentials_test.go

+func clientHandle(t *testing.T, hs func(net.Conn, string) (tls.ConnectionState, error), lisAddr string) (tls.ConnectionState, error) {
+	conn, err := net.Dial("tcp", lisAddr)
+	if err != nil {
+		t.Errorf("Client failed to connect to %s. Error: %v", lisAddr, err)


use t.Fatalf and this function only returns tls.ConnectionState.

iamqizhao · 2016-11-09T23:36:27Z

credentials/credentials_test.go

+	defer conn.Close()
+	clientConnState, err := hs(conn, lisAddr)
+	if err != nil {
+		t.Errorf("Error on client while handshake. Error: %v", err)


iamqizhao · 2016-11-09T23:37:44Z

credentials/credentials_test.go

+	return nil
+}
+
+// Client handshake implementation using gRPC.


iamqizhao · 2016-11-09T23:38:02Z

credentials/credentials_test.go

+	return clientConnState, err
+}
+
+// Server handshake implementation using gRPC.


iamqizhao · 2016-11-09T23:50:37Z

credentials/credentials_test.go

+	return authInfo.(TLSInfo).State, nil
+}
+
+// Server handshake implementation using tls.


remove the comment.

iamqizhao · 2016-11-09T23:51:08Z

credentials/credentials_test.go

+	return nil
+}
+
+// Client handskae implementation using tls.


remove the comment.

iamqizhao · 2016-11-09T23:55:31Z

credentials/credentials_test.go

+
+func TestClientHandshakeReturnsAuthInfo(t *testing.T) {
+	var serverConnState tls.ConnectionState
+	errChan := make(chan error, 1)


This is more like a channel to notify the completion of serverHandle. Can you rename it to "done"? Applies to all the tests you added.

iamqizhao · 2016-11-16T00:14:03Z

credentials/credentials_test.go

+	return lis.Addr().String()
+}
+
+// Is run in a seperate go routine.


goroutine is one word.

iamqizhao · 2016-11-16T00:17:40Z

credentials/credentials_test.go

+
+func TestClientHandshakeReturnsAuthInfo(t *testing.T) {
+	var serverConnState tls.ConnectionState
+	done := make(chan error, 1)


make this a channel of authInfo. The way launchServer does (&serverConnState) is more like C/C++ way instead of Go idiom. In this way, serverHandshake does not need the second param.

iamqizhao · 2016-11-17T01:53:04Z

credentials/credentials_test.go

+	lisAddr := launchServer(t, tlsServerHandshake, done)
+	clientConnState := clientHandle(t, gRPCClientHandshake, lisAddr)
+	// wait until server sends serverConnState or fails.
+	serverConnState := <-done


"done" should be closed if there is an error.

sAuthInfo, ok := <-done if !ok { t.Fatalf(....) }

iamqizhao · 2016-11-17T01:53:34Z

credentials/credentials_test.go

+	lisAddr := launchServer(t, gRPCServerHandshake, done)
+	clientConnState := clientHandle(t, tlsClientHandshake, lisAddr)
+	// wait until server sends serverConnState or fails.
+	serverConnState := <-done


iamqizhao · 2016-11-17T01:53:51Z

credentials/credentials_test.go

+	lisAddr := launchServer(t, gRPCServerHandshake, done)
+	clientConnState := clientHandle(t, gRPCClientHandshake, lisAddr)
+	// wait until server sends serverConnState or fails.
+	serverConnState := <-done


iamqizhao · 2016-11-17T01:55:49Z

credentials/credentials_test.go

+	defer lis.Close()
+	var serverConnState tls.ConnectionState
+	defer func() {
+		done <- serverConnState


done should be closed when there is an error (e.g., line 142 and 147).

iamqizhao · 2017-01-05T23:09:54Z

credentials/credentials_test.go

+
+const tlsDir = "../test/testdata/"
+
+type serverHandshake func(net.Conn) (tls.ConnectionState, error)


can we define it as security-protocol agnostic -- it should return AuthInfo instead of tls.ConnectionState?

iamqizhao · 2017-01-05T23:10:41Z

credentials/credentials_test.go

+type serverHandshake func(net.Conn) (tls.ConnectionState, error)
+
+func TestClientHandshakeReturnsAuthInfo(t *testing.T) {
+	done := make(chan tls.ConnectionState, 1)


ditto. should be chan AuthInfo?

this applies to the rest of the tests. Please fix.

iamqizhao · 2017-01-05T23:58:27Z

credentials/credentials_test.go

+func TestClientHandshakeReturnsAuthInfo(t *testing.T) {
+	done := make(chan AuthInfo, 1)
+	lisAddr := launchServer(t, tlsServerHandshake, done)
+	clientConnState := clientHandle(t, gRPCClientHandshake, lisAddr)


s/clientConnState/clientAuthInfo

iamqizhao · 2017-01-05T23:58:47Z

credentials/credentials_test.go

+	lisAddr := launchServer(t, tlsServerHandshake, done)
+	clientConnState := clientHandle(t, gRPCClientHandshake, lisAddr)
+	// wait until server sends serverConnState or fails.
+	serverConnState, ok := <-done


s/serverConnState/serverAuthInfo

applies to the other tests.

iamqizhao · 2017-01-06T01:56:30Z

credentials/credentials_test.go

+	}
+	_, serverAuthInfo, err := serverTLS.ServerHandshake(conn)
+	if err != nil {
+		return TLSInfo{}, err


return nil, err?

iamqizhao · 2017-01-06T01:56:37Z

credentials/credentials_test.go

+func gRPCServerHandshake(conn net.Conn) (AuthInfo, error) {
+	serverTLS, err := NewServerTLSFromFile(tlsDir+"server1.pem", tlsDir+"server1.key")
+	if err != nil {
+		return TLSInfo{}, err


return nil, err?

iamqizhao · 2017-01-06T01:56:51Z

credentials/credentials_test.go

+	clientTLS := NewTLS(&tls.Config{InsecureSkipVerify: true})
+	_, authInfo, err := clientTLS.ClientHandshake(context.Background(), lisAddr, conn)
+	if err != nil {
+		return TLSInfo{}, err


iamqizhao · 2017-01-06T01:56:58Z

credentials/credentials_test.go

+func tlsServerHandshake(conn net.Conn) (AuthInfo, error) {
+	cert, err := tls.LoadX509KeyPair(tlsDir+"server1.pem", tlsDir+"server1.key")
+	if err != nil {
+		return TLSInfo{}, err


iamqizhao · 2017-01-06T01:57:05Z

credentials/credentials_test.go

+	serverConn := tls.Server(conn, serverTLSConfig)
+	err = serverConn.Handshake()
+	if err != nil {
+		return TLSInfo{}, err


iamqizhao · 2017-01-06T01:57:12Z

credentials/credentials_test.go

+	clientConn := tls.Client(conn, clientTLSConfig)
+	err := clientConn.Handshake()
+	if err != nil {
+		return TLSInfo{}, err


iamqizhao · 2017-01-09T19:29:28Z

credentials/credentials_test.go

+func tlsClientHandshake(conn net.Conn, _ string) (AuthInfo, error) {
+	clientTLSConfig := &tls.Config{InsecureSkipVerify: true}
+	clientConn := tls.Client(conn, clientTLSConfig)
+	err := clientConn.Handshake()


if err := clientConn.Handshake(); err != nil { ... }

MakMukhi added 3 commits October 27, 2016 16:39

Initial commit

1165b1e

Initial commit 2

d1b12d3

minor update

0129b49

MakMukhi changed the title ~~Mmukhi tlsinfo~~ ClientHandshake to return AuthInfo Oct 28, 2016

MakMukhi added 3 commits October 28, 2016 16:03

goimport update

c62cf7b

resolved race condition

5b3192c

added test for TLSInfo on server side

ff332b6

menghanl reviewed Oct 29, 2016

View reviewed changes

Post review updates

915cb50

iamqizhao reviewed Oct 31, 2016

View reviewed changes

port review changes

6c58b32

debug debug

MakMukhi force-pushed the mmukhi_tlsinfo branch from 745ff04 to 6c58b32 Compare October 31, 2016 21:40

iamqizhao reviewed Nov 1, 2016

View reviewed changes

refactoring and added third function

9d3e997

iamqizhao reviewed Nov 9, 2016

View reviewed changes

post review changes

c980740

iamqizhao reviewed Nov 16, 2016

View reviewed changes

post review changes

6fdee01

iamqizhao reviewed Nov 17, 2016

View reviewed changes

post review updates

b792ae8

iamqizhao reviewed Jan 5, 2017

View reviewed changes

post review commit

16853da

iamqizhao reviewed Jan 5, 2017

View reviewed changes

MakMukhi added 5 commits January 5, 2017 16:09

post review commit

40952fe

post review update

1db9a22

post review update

848da09

post review update

49c5700

post review update

e7832cf

iamqizhao reviewed Jan 6, 2017

View reviewed changes

post review commit

ecc30a5

iamqizhao reviewed Jan 9, 2017

View reviewed changes

post review update

74f10a5

iamqizhao merged commit 4ed3489 into grpc:master Jan 9, 2017

tamalsaha mentioned this pull request Mar 23, 2017

Update grpc-go to 1.2.1 helm/helm#2172

Closed

MakMukhi deleted the mmukhi_tlsinfo branch May 4, 2018 02:07

lock bot locked as resolved and limited conversation to collaborators Oct 31, 2018

		@@ -167,7 +167,7 @@ func (c *tlsCreds) ClientHandshake(ctx context.Context, addr string, rawConn net
		}
		// TODO(zhaoq): Omit the auth info for client now. It is more for


		const tlsDir = "../test/testdata/"

		type serverHandshake func(net.Conn) (tls.ConnectionState, error)

ClientHandshake to return AuthInfo #956

ClientHandshake to return AuthInfo #956

Conversation

MakMukhi commented Oct 28, 2016

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

iamqizhao 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

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

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