From dd767416a6b9d4c7c49c17f94e13b94857cc08c0 Mon Sep 17 00:00:00 2001 From: Easwar Swaminathan Date: Tue, 9 Nov 2021 15:42:07 -0800 Subject: [PATCH] grpc: implement WithInsecure() using the insecure package (#4718) --- clientconn.go | 37 ++++++----- clientconn_test.go | 43 ++++++++++--- credentials/credentials.go | 10 +++ credentials/insecure/insecure.go | 3 + dialoptions.go | 14 ++-- test/creds_test.go | 3 +- test/insecure_creds_test.go | 106 ++++++++++++++++--------------- 7 files changed, 132 insertions(+), 84 deletions(-) diff --git a/clientconn.go b/clientconn.go index 5a9e7d75..972ff1a6 100644 --- a/clientconn.go +++ b/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 - // information (e.g., OAuth2 token) which requires secure connection on an insecure - // connection. + // errNoTransportCredsInBundle indicated that the configured creds bundle + // returned a transport credentials which was nil. + 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.TransportCredentials != nil || cc.dopts.copts.CredsBundle != nil { - return nil, errCredentialsConflict - } + 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 + } + if cc.dopts.copts.CredsBundle != nil && cc.dopts.copts.CredsBundle.TransportCredentials() == nil { + return nil, errNoTransportCredsInBundle + } + 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 diff --git a/clientconn_test.go b/clientconn_test.go index d276c7b5..ef6a6808 100644 --- a/clientconn_test.go +++ b/clientconn_test.go @@ -490,29 +490,52 @@ func (s) TestDialContextFailFast(t *testing.T) { } // securePerRPCCredentials always requires transport security. -type securePerRPCCredentials struct{} - -func (c securePerRPCCredentials) GetRequestMetadata(ctx context.Context, uri ...string) (map[string]string, error) { - return nil, nil +type securePerRPCCredentials struct { + credentials.PerRPCCredentials } 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 - if _, err := Dial("passthrough:///Non-Existent.Server:80", WithTransportCredentials(tlsCreds), WithBlock(), WithInsecure()); err != errCredentialsConflict { - t.Fatalf("Dial(_, _) = _, %v, want _, %v", err, errCredentialsConflict) + dopts := []DialOption{ + WithTransportCredentials(creds), + WithCredentialsBundle(&fakeBundleCreds{transportCreds: creds}), } - // security info on insecure connection - if _, err := Dial("passthrough:///Non-Existent.Server:80", WithPerRPCCredentials(securePerRPCCredentials{}), WithBlock(), WithInsecure()); err != errTransportCredentialsMissing { + if _, err := Dial("passthrough:///Non-Existent.Server:80", dopts...); err != errTransportCredsAndBundle { + 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) { diff --git a/credentials/credentials.go b/credentials/credentials.go index a6711075..96ff1877 100644 --- a/credentials/credentials.go +++ b/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. // diff --git a/credentials/insecure/insecure.go b/credentials/insecure/insecure.go index c4fa27c9..22a8f996 100644 --- a/credentials/insecure/insecure.go +++ b/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{} } diff --git a/dialoptions.go b/dialoptions.go index 22d626f1..063f1e90 100644 --- a/dialoptions.go +++ b/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 -// set. +// ClientConn. Under the hood, it uses insecure.NewCredentials(). +// +// 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() }) } diff --git a/test/creds_test.go b/test/creds_test.go index 6b3fc2a4..d886220d 100644 --- a/test/creds_test.go +++ b/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") diff --git a/test/insecure_creds_test.go b/test/insecure_creds_test.go index 791cf650..9c925e47 100644 --- a/test/insecure_creds_test.go +++ b/test/insecure_creds_test.go @@ -144,64 +144,66 @@ func (s) TestInsecureCreds(t *testing.T) { } } -func (s) TestInsecureCredsWithPerRPCCredentials(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, +func (s) TestInsecureCreds_WithPerRPCCredentials_AsCallOption(t *testing.T) { + ss := &stubserver.StubServer{ + EmptyCallF: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) { + return &testpb.Empty{}, nil }, } - 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 - }, - } - s := grpc.NewServer(grpc.Creds(insecure.NewCredentials())) - defer s.Stop() - testpb.RegisterTestServiceServer(s, ss) + 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) + 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() - ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) - defer cancel() + addr := lis.Addr().String() + ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout) + defer cancel() - dopts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())} - if test.perRPCCredsViaDialOptions { - 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) - } - defer cc.Close() + dopts := []grpc.DialOption{grpc.WithTransportCredentials(insecure.NewCredentials())} + copts := []grpc.CallOption{grpc.PerRPCCredentials(testLegacyPerRPCCredentials{})} + cc, err := grpc.Dial(addr, dopts...) + if err != nil { + t.Fatalf("grpc.Dial(%q) failed: %v", addr, err) + } + defer cc.Close() - 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) - } - }) + 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("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) } }