Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

credentials/insecure: Implement insecure credentials. #3964

Merged
merged 1 commit into from Oct 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
74 changes: 74 additions & 0 deletions credentials/insecure/insecure.go
@@ -0,0 +1,74 @@
/*
*
* Copyright 2020 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

// Package insecure provides an implementation of the
// credentials.TransportCredentials interface which disables transport security.
//
// Experimental
//
// Notice: This package is EXPERIMENTAL and may be changed or removed in a
// later release.
package insecure

import (
"context"
"net"

"google.golang.org/grpc/credentials"
)

// NewCredentials returns a credentials which disables transport security.
func NewCredentials() credentials.TransportCredentials {
return insecureTC{}
}

// insecureTC implements the insecure transport credentials. The handshake
// methods simply return the passed in net.Conn and set the security level to
// NoSecurity.
type insecureTC struct{}

func (insecureTC) ClientHandshake(ctx context.Context, _ string, conn net.Conn) (net.Conn, credentials.AuthInfo, error) {
return conn, Info{credentials.CommonAuthInfo{SecurityLevel: credentials.NoSecurity}}, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this impact call creds? Will call creds not aware of security level be allowed? Should they be? What do the other languages do? @yihuazhang do you know offhand?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The security level of a channel is orthogonal to the security level of a call credential. A security level is an inherent property of both channel and call credential. For a call credential, its security level dictates the minimum security level a channel's security level should be in order to transfer the call credential. So in the case of insecure credentials, it can only transfer the call credentials having the security level - NoSecurity. The above behavior is consistent across all languages (C-core, Java, and Go). PLMK if you have more questions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a bunch of call credentials that were written before we had the "security levels" concept.

Are those supposed to work with insecure creds? I would assume anything that doesn't support security levels, because it is too old, should NOT be used with insecure creds. At least, if they have RequireTransportSecurity() = true.

Did we add a check somewhere:

if perRPCCreds.RequireTransportSecurity() && transportCreds.SecurityLevel() == INSECURE {
  // FAIL
}

I don't think we did. Here are the places I can see where we check RequireTransportSecurity:

grpc-go/clientconn.go

Lines 175 to 182 in 9519eff

if cc.dopts.copts.TransportCredentials != nil || cc.dopts.copts.CredsBundle != nil {
return nil, errCredentialsConflict
}
for _, cd := range cc.dopts.copts.PerRPCCredentials {
if cd.RequireTransportSecurity() {
return nil, errTransportCredentialsMissing
}
}

and

if callCreds := callHdr.Creds; callCreds != nil {
if !t.isSecure && callCreds.RequireTransportSecurity() {
return nil, status.Error(codes.Unauthenticated, "transport: cannot send secure credentials on an insecure connection")
}

+
if transportCreds != nil {
// gRPC, resolver, balancer etc. can specify arbitrary data in the
// Attributes field of resolver.Address, which is shoved into connectCtx
// and passed to the credential handshaker. This makes it possible for
// address specific arbitrary data to reach the credential handshaker.
contextWithHandshakeInfo := internal.NewClientHandshakeInfoContext.(func(context.Context, credentials.ClientHandshakeInfo) context.Context)
connectCtx = contextWithHandshakeInfo(connectCtx, credentials.ClientHandshakeInfo{Attributes: addr.Attributes})
conn, authInfo, err = transportCreds.ClientHandshake(connectCtx, addr.ServerName, conn)
if err != nil {
return nil, connectionErrorf(isTemporary(err), err, "transport: authentication handshake failed: %v", err)
}
isSecure = true

In both cases, the presence of TransportCredentials is enough to make the PerRPCCredentials happy. But that seems too weak now that we will have credentials that aren't actually providing security at all. Or does "insecure" credentials mean "also fake out call credentials so they think the connection is secure"? And if that is the case, these creds should return the max security level, shouldn't they?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point. I agree the current check (if we leave as it is now) is too weak because as you said, all PerRPCCredentials having either security level > NoSecurity or RequireTransportSecurity() = true can be communicated over channels created with insecure credentials. I think we should add the check you suggested above, and probably in http2_client.go where GetRequestMetadata() is called?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also consider a channel's Invalid security level for the purpose of backward compatibility in the above check. Similar to CheckSecurityLevel() API in credentials.go

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that all sounds right to me. But note that the check in clientconn.go can't be improved since the TransportCredentials don't report their security level until after the connection is established. So the only place we can do this is in http2_client.go, and it will have to be a connection failure and not a channel-creation failure.

Also, do we require !=NoSecurity or ==PrivacyAndIntegrity if PerRPCCredentials.RequireTransportSecurity()==true? Let's move this conversation to #3974.

}

func (insecureTC) ServerHandshake(conn net.Conn) (net.Conn, credentials.AuthInfo, error) {
return conn, Info{credentials.CommonAuthInfo{SecurityLevel: credentials.NoSecurity}}, nil
}

func (insecureTC) Info() credentials.ProtocolInfo {
return credentials.ProtocolInfo{SecurityProtocol: "insecure"}
}

func (insecureTC) Clone() credentials.TransportCredentials {
return insecureTC{}
}

func (insecureTC) OverrideServerName(string) error {
return nil
}

// Info contains the auth information for an insecure connection.
// It implements the AuthInfo interface.
type Info struct {
credentials.CommonAuthInfo
}

// AuthType returns the type of Info as a string.
func (Info) AuthType() string {
return "insecure"
}
124 changes: 124 additions & 0 deletions test/insecure_creds_test.go
@@ -0,0 +1,124 @@
/*
*
* Copyright 2020 gRPC authors.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
*/

package test

import (
"context"
"net"
"testing"
"time"

"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/credentials/insecure"
"google.golang.org/grpc/peer"
"google.golang.org/grpc/status"
testpb "google.golang.org/grpc/test/grpc_testing"
)

const defaultTestTimeout = 5 * time.Second

// TestInsecureCreds tests the use of insecure creds on the server and client
// side, and verifies that expect security level and auth info are returned.
// Also verifies that this credential can interop with existing `WithInsecure`
// DialOption.
func (s) TestInsecureCreds(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test that covers the interaction between insecure creds and call creds.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add the test as part of the other PR where we add the missing checks mentioned in the other comment thread?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, since the interaction we want (reject PerRPCCredentials that RequireTransportSecurity) isn't happening now.

tests := []struct {
desc string
clientInsecureCreds bool
serverInsecureCreds bool
}{
{
desc: "client and server insecure creds",
clientInsecureCreds: true,
serverInsecureCreds: true,
},
{
desc: "client only insecure creds",
clientInsecureCreds: true,
},
{
desc: "server only insecure creds",
serverInsecureCreds: true,
},
}

for _, test := range tests {
t.Run(test.desc, func(t *testing.T) {
ss := &stubServer{
emptyCall: func(ctx context.Context, in *testpb.Empty) (*testpb.Empty, error) {
if !test.serverInsecureCreds {
return &testpb.Empty{}, nil
}

pr, ok := peer.FromContext(ctx)
if !ok {
return nil, status.Error(codes.DataLoss, "Failed to get peer from ctx")
}
// Check security level.
info := pr.AuthInfo.(insecure.Info)
if at := info.AuthType(); at != "insecure" {
return nil, status.Errorf(codes.Unauthenticated, "Wrong AuthType: got %q, want insecure", at)
}
if secLevel := info.CommonAuthInfo.SecurityLevel; secLevel != credentials.NoSecurity {
return nil, status.Errorf(codes.Unauthenticated, "Wrong security level: got %q, want %q", secLevel, credentials.NoSecurity)
}
return &testpb.Empty{}, nil
},
}

sOpts := []grpc.ServerOption{}
if test.serverInsecureCreds {
sOpts = append(sOpts, grpc.Creds(insecure.NewCredentials()))
}
s := grpc.NewServer(sOpts...)
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()
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
cOpts := []grpc.DialOption{grpc.WithBlock()}
if test.clientInsecureCreds {
cOpts = append(cOpts, grpc.WithTransportCredentials(insecure.NewCredentials()))
} else {
cOpts = append(cOpts, grpc.WithInsecure())
}
cc, err := grpc.DialContext(ctx, addr, cOpts...)
if err != nil {
t.Fatalf("grpc.Dial(%q) failed: %v", addr, err)
}
defer cc.Close()

c := testpb.NewTestServiceClient(cc)
if _, err = c.EmptyCall(ctx, &testpb.Empty{}); err != nil {
t.Fatalf("EmptyCall(_, _) = _, %v; want _, <nil>", err)
}
})
}
}