-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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 | ||
} | ||
|
||
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" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, since the interaction we want (reject |
||
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) | ||
} | ||
}) | ||
} | ||
} |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
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
and
grpc-go/internal/transport/http2_client.go
Lines 559 to 562 in 9519eff
+
grpc-go/internal/transport/http2_client.go
Lines 226 to 237 in 9519eff
In both cases, the presence of
TransportCredentials
is enough to make thePerRPCCredentials
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?There was a problem hiding this comment.
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
orRequireTransportSecurity() = true
can be communicated over channels created with insecure credentials. I think we should add the check you suggested above, and probably inhttp2_client.go
whereGetRequestMetadata()
is called?There was a problem hiding this comment.
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 toCheckSecurityLevel()
API incredentials.go
There was a problem hiding this comment.
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 theTransportCredentials
don't report their security level until after the connection is established. So the only place we can do this is inhttp2_client.go
, and it will have to be a connection failure and not a channel-creation failure.Also, do we require
!=NoSecurity
or==PrivacyAndIntegrity
ifPerRPCCredentials.RequireTransportSecurity()==true
? Let's move this conversation to #3974.