grpc: implement WithInsecure() using the insecure package (#4718)

clientconn.go

@@ -83,13 +83,13 @@ var (
 	// errTransportCredsAndBundle indicates that creds bundle is used together
 	// with other individual Transport Credentials.
 	errTransportCredsAndBundle = errors.New("grpc: credentials.Bundle may not be used with individual TransportCredentials")
-	// errTransportCredentialsMissing indicates that users want to transmit security
+	// errNoTransportCredsInBundle indicated that the configured creds bundle
-	// information (e.g., OAuth2 token) which requires secure connection on an insecure
+	// returned a transport credentials which was nil.
-	// connection.
+	errNoTransportCredsInBundle = errors.New("grpc: credentials.Bundle must return non-nil transport credentials")
+	// errTransportCredentialsMissing indicates that users want to transmit
+	// security information (e.g., OAuth2 token) which requires secure
+	// connection on an insecure connection.
 	errTransportCredentialsMissing = errors.New("grpc: the credentials require transport level security (use grpc.WithTransportCredentials() to set)")
-	// errCredentialsConflict indicates that grpc.WithTransportCredentials()
-	// and grpc.WithInsecure() are both called for a connection.
-	errCredentialsConflict = errors.New("grpc: transport credentials are set for an insecure connection (grpc.WithTransportCredentials() and grpc.WithInsecure() are both called)")
 )
 
 const (
@@ -177,17 +177,20 @@ func DialContext(ctx context.Context, target string, opts ...DialOption) (conn *
 		cc.csMgr.channelzID = cc.channelzID
 	}
 
-	if !cc.dopts.insecure {
 	if cc.dopts.copts.TransportCredentials == nil && cc.dopts.copts.CredsBundle == nil {
 		return nil, errNoTransportSecurity
 	}
 	if cc.dopts.copts.TransportCredentials != nil && cc.dopts.copts.CredsBundle != nil {
 		return nil, errTransportCredsAndBundle
 	}
-	} else {
+	if cc.dopts.copts.CredsBundle != nil && cc.dopts.copts.CredsBundle.TransportCredentials() == nil {
-		if cc.dopts.copts.TransportCredentials != nil || cc.dopts.copts.CredsBundle != nil {
+		return nil, errNoTransportCredsInBundle
-			return nil, errCredentialsConflict
 	}
+	transportCreds := cc.dopts.copts.TransportCredentials
+	if transportCreds == nil {
+		transportCreds = cc.dopts.copts.CredsBundle.TransportCredentials()
+	}
+	if transportCreds.Info().SecurityProtocol == "insecure" {
 		for _, cd := range cc.dopts.copts.PerRPCCredentials {
 			if cd.RequireTransportSecurity() {
 				return nil, errTransportCredentialsMissing

clientconn_test.go

@@ -490,29 +490,52 @@ func (s) TestDialContextFailFast(t *testing.T) {
 }
 
 // securePerRPCCredentials always requires transport security.
-type securePerRPCCredentials struct{}
+type securePerRPCCredentials struct {
-
+	credentials.PerRPCCredentials
-func (c securePerRPCCredentials) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) {
-	return nil, nil
 }
 
 func (c securePerRPCCredentials) RequireTransportSecurity() bool {
 	return true
 }
 
+type fakeBundleCreds struct {
+	credentials.Bundle
+	transportCreds credentials.TransportCredentials
+}
+
+func (b *fakeBundleCreds) TransportCredentials() credentials.TransportCredentials {
+	return b.transportCreds
+}
+
 func (s) TestCredentialsMisuse(t *testing.T) {
-	tlsCreds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "x.test.example.com")
+	// Use of no transport creds and no creds bundle must fail.
+	if _, err := Dial("passthrough:///Non-Existent.Server:80"); err != errNoTransportSecurity {
+		t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errNoTransportSecurity)
+	}
+
+	// Use of both transport creds and creds bundle must fail.
+	creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "x.test.example.com")
 	if err != nil {
 		t.Fatalf("Failed to create authenticator %v", err)
 	}
-	// Two conflicting credential configurations
+	dopts := []DialOption{
-	if _, err := Dial("passthrough:///Non-Existent.Server:80", WithTransportCredentials(tlsCreds), WithBlock(), WithInsecure()); err != errCredentialsConflict {
+		WithTransportCredentials(creds),
-		t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errCredentialsConflict)
+		WithCredentialsBundle(&fakeBundleCreds{transportCreds: creds}),
 	}
-	// security info on insecure connection
+	if _, err := Dial("passthrough:///Non-Existent.Server:80", dopts...); err != errTransportCredsAndBundle {
-	if _, err := Dial("passthrough:///Non-Existent.Server:80", WithPerRPCCredentials(securePerRPCCredentials{}), WithBlock(), WithInsecure()); err != errTransportCredentialsMissing {
+		t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errTransportCredsAndBundle)
+	}
+
+	// Use of perRPC creds requiring transport security over an insecure
+	// transport must fail.
+	if _, err := Dial("passthrough:///Non-Existent.Server:80", WithPerRPCCredentials(securePerRPCCredentials{}), WithInsecure()); err != errTransportCredentialsMissing {
 		t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errTransportCredentialsMissing)
 	}
+
+	// Use of a creds bundle with nil transport credentials must fail.
+	if _, err := Dial("passthrough:///Non-Existent.Server:80", WithCredentialsBundle(&fakeBundleCreds{})); err != errNoTransportCredsInBundle {
+		t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errTransportCredsAndBundle)
+	}
 }
 
 func (s) TestWithBackoffConfigDefault(t *testing.T) {

credentials/credentials.go

@@ -178,8 +178,18 @@ type TransportCredentials interface {
 //
 // This API is experimental.
 type Bundle interface {
+	// TransportCredentials returns the transport credentials from the Bundle.
+	//
+	// Implementations must return non-nil transport credentials. If transport
+	// security is not needed by the Bundle, implementations may choose to
+	// return insecure.NewCredentials().
 	TransportCredentials() TransportCredentials
+
+	// PerRPCCredentials returns the per-RPC credentials from the Bundle.
+	//
+	// May be nil if per-RPC credentials are not needed.
 	PerRPCCredentials() PerRPCCredentials
+
 	// NewWithMode should make a copy of Bundle, and switch mode. Modifying the
 	// existing Bundle may cause races.
 	//

credentials/insecure/insecure.go

@@ -33,6 +33,9 @@ import (
 )
 
 // NewCredentials returns a credentials which disables transport security.
+//
+// Note that using this credentials with per-RPC credentials which require
+// transport security is incompatible and will cause grpc.Dial() to fail.
 func NewCredentials() credentials.TransportCredentials {
 	return insecureTC{}
 }

dialoptions.go

@@ -27,6 +27,7 @@ import (
 	"google.golang.org/grpc/backoff"
 	"google.golang.org/grpc/balancer"
 	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/internal"
 	internalbackoff "google.golang.org/grpc/internal/backoff"
 	"google.golang.org/grpc/internal/transport"
@@ -49,7 +50,6 @@ type dialOptions struct {
 	bs              internalbackoff.Strategy
 	block           bool
 	returnLastError bool
-	insecure        bool
 	timeout         time.Duration
 	scChan          <-chan ServiceConfig
 	authority       string
@@ -298,11 +298,17 @@ func WithReturnConnectionError() DialOption {
 }
 
 // WithInsecure returns a DialOption which disables transport security for this
-// ClientConn. Note that transport security is required unless WithInsecure is
+// ClientConn. Under the hood, it uses insecure.NewCredentials().
-// set.
+//
+// Note that using this DialOption with per-RPC credentials (through
+// WithCredentialsBundle or WithPerRPCCredentials) which require transport
+// security is incompatible and will cause grpc.Dial() to fail.
+//
+// Deprecated: use insecure.NewCredentials() instead.
+// Will be supported throughout 1.x.
 func WithInsecure() DialOption {
 	return newFuncDialOption(func(o *dialOptions) {
-		o.insecure = true
+		o.copts.TransportCredentials = insecure.NewCredentials()
 	})
 }
 

test/creds_test.go

@@ -31,6 +31,7 @@ import (
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/connectivity"
 	"google.golang.org/grpc/credentials"
+	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/resolver"
 	"google.golang.org/grpc/resolver/manual"
@@ -52,7 +53,7 @@ type testCredsBundle struct {
 
 func (c *testCredsBundle) TransportCredentials() credentials.TransportCredentials {
 	if c.mode == bundlePerRPCOnly {
-		return nil
+		return insecure.NewCredentials()
 	}
 
 	creds, err := credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "x.test.example.com")

test/insecure_creds_test.go

@@ -144,25 +144,7 @@ func (s) TestInsecureCreds(t *testing.T) {
 	}
 }
 
-func (s) TestInsecureCredsWithPerRPCCredentials(t *testing.T) {
+func (s) TestInsecureCreds_WithPerRPCCredentials_AsCallOption(t *testing.T) {
-	tests := []struct {
-		desc                      string
-		perRPCCredsViaDialOptions bool
-		perRPCCredsViaCallOptions bool
-	}{
-		{
-			desc:                      "send PerRPCCredentials via DialOptions",
-			perRPCCredsViaDialOptions: true,
-			perRPCCredsViaCallOptions: false,
-		},
-		{
-			desc:                      "send PerRPCCredentials via CallOptions",
-			perRPCCredsViaDialOptions: false,
-			perRPCCredsViaCallOptions: true,
-		},
-	}
-	for _, test := range tests {
-		t.Run(test.desc, func(t *testing.T) {
 	ss := &stubserver.StubServer{
 		EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) {
 			return &testpb.Empty{}, nil
@@ -184,13 +166,7 @@ func (s) TestInsecureCredsWithPerRPCCredentials(t *testing.T) {
 	defer cancel()
 
 	dopts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())}
-			if test.perRPCCredsViaDialOptions {
+	copts := []grpc.CallOption{grpc.PerRPCCredentials(testLegacyPerRPCCredentials{})}
-				dopts = append(dopts, grpc.WithPerRPCCredentials(testLegacyPerRPCCredentials{}))
-			}
-			copts := []grpc.CallOption{}
-			if test.perRPCCredsViaCallOptions {
-				copts = append(copts, grpc.PerRPCCredentials(testLegacyPerRPCCredentials{}))
-			}
 	cc, err := grpc.Dial(addr, dopts...)
 	if err != nil {
 		t.Fatalf("grpc.Dial(%q) failed: %v", addr, err)
@@ -200,8 +176,34 @@ func (s) TestInsecureCredsWithPerRPCCredentials(t *testing.T) {
 	const wantErr = "transport: cannot send secure credentials on an insecure connection"
 	c := testpb.NewTestServiceClient(cc)
 	if _, err = c.EmptyCall(ctx, &testpb.Empty{}, copts...); err == nil || !strings.Contains(err.Error(), wantErr) {
-				t.Fatalf("InsecureCredsWithPerRPCCredentials/send_PerRPCCredentials_via_CallOptions  = %v; want %s", err, wantErr)
+		t.Fatalf("insecure credentials with per-RPC credentials requiring transport security returned error: %v; want %s", err, wantErr)
-			}
+	}
-		})
+}
+
+func (s) TestInsecureCreds_WithPerRPCCredentials_AsDialOption(t *testing.T) {
+	ss := &stubserver.StubServer{
+		EmptyCallF: func(_ context.Context, _ *testpb.Empty) (*testpb.Empty, error) {
+			return &testpb.Empty{}, nil
+		},
+	}
+
+	s := grpc.NewServer(grpc.Creds(insecure.NewCredentials()))
+	defer s.Stop()
+	testpb.RegisterTestServiceServer(s, ss)
+
+	lis, err := net.Listen("tcp", "localhost:0")
+	if err != nil {
+		t.Fatalf("net.Listen(tcp, localhost:0) failed: %v", err)
+	}
+	go s.Serve(lis)
+
+	addr := lis.Addr().String()
+	dopts := []grpc.DialOption{
+		grpc.WithTransportCredentials(insecure.NewCredentials()),
+		grpc.WithPerRPCCredentials(testLegacyPerRPCCredentials{}),
+	}
+	const wantErr = "the credentials require transport level security"
+	if _, err := grpc.Dial(addr, dopts...); err == nil || !strings.Contains(err.Error(), wantErr) {
+		t.Fatalf("grpc.Dial(%q) returned err %v, want: %v", addr, err, wantErr)
 	}
 }