diff --git a/authz/rbac_translator.go b/authz/rbac_translator.go index 039d76bc99d9..4a790b1a702c 100644 --- a/authz/rbac_translator.go +++ b/authz/rbac_translator.go @@ -155,14 +155,20 @@ func parsePrincipalNames(principalNames []string) []*v3rbacpb.Principal { } func parsePeer(source peer) (*v3rbacpb.Principal, error) { - if len(source.Principals) > 0 { - return principalOr(parsePrincipalNames(source.Principals)), nil + if source.Principals == nil { + return &v3rbacpb.Principal{ + Identifier: &v3rbacpb.Principal_Any{ + Any: true, + }, + }, nil } - return &v3rbacpb.Principal{ - Identifier: &v3rbacpb.Principal_Any{ - Any: true, - }, - }, nil + if len(source.Principals) == 0 { + return &v3rbacpb.Principal{ + Identifier: &v3rbacpb.Principal_Authenticated_{ + Authenticated: &v3rbacpb.Principal_Authenticated{}, + }}, nil + } + return principalOr(parsePrincipalNames(source.Principals)), nil } func parsePaths(paths []string) []*v3rbacpb.Permission { diff --git a/authz/rbac_translator_test.go b/authz/rbac_translator_test.go index 9a883e9d78d5..9b88362ea90b 100644 --- a/authz/rbac_translator_test.go +++ b/authz/rbac_translator_test.go @@ -205,6 +205,32 @@ func TestTranslatePolicy(t *testing.T) { }, }, }, + "empty principal field": { + authzPolicy: `{ + "name": "authz", + "allow_rules": [{ + "name": "allow_authenticated", + "source": {"principals":[]} + }] + }`, + wantPolicies: []*v3rbacpb.RBAC{ + { + Action: v3rbacpb.RBAC_ALLOW, + Policies: map[string]*v3rbacpb.Policy{ + "authz_allow_authenticated": { + Principals: []*v3rbacpb.Principal{ + {Identifier: &v3rbacpb.Principal_Authenticated_{ + Authenticated: &v3rbacpb.Principal_Authenticated{}, + }}, + }, + Permissions: []*v3rbacpb.Permission{ + {Rule: &v3rbacpb.Permission_Any{Any: true}}, + }, + }, + }, + }, + }, + }, "unknown field": { authzPolicy: `{"random": 123}`, wantErr: "failed to unmarshal policy", diff --git a/authz/sdk_end2end_test.go b/authz/sdk_end2end_test.go index 093b2bb437d2..79fa379bceac 100644 --- a/authz/sdk_end2end_test.go +++ b/authz/sdk_end2end_test.go @@ -20,6 +20,8 @@ package authz_test import ( "context" + "crypto/tls" + "crypto/x509" "io" "io/ioutil" "net" @@ -30,10 +32,12 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/authz" "google.golang.org/grpc/codes" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/metadata" "google.golang.org/grpc/status" pb "google.golang.org/grpc/test/grpc_testing" + "google.golang.org/grpc/testdata" ) type testServer struct { @@ -69,7 +73,7 @@ var sdkTests = map[string]struct { md metadata.MD wantStatus *status.Status }{ - "DeniesRpcMatchInDenyNoMatchInAllow": { + "DeniesRPCMatchInDenyNoMatchInAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -112,7 +116,7 @@ var sdkTests = map[string]struct { md: metadata.Pairs("key-abc", "val-abc"), wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), }, - "DeniesRpcMatchInDenyAndAllow": { + "DeniesRPCMatchInDenyAndAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -142,7 +146,7 @@ var sdkTests = map[string]struct { }`, wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), }, - "AllowsRpcNoMatchInDenyMatchInAllow": { + "AllowsRPCNoMatchInDenyMatchInAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -179,7 +183,7 @@ var sdkTests = map[string]struct { md: metadata.Pairs("key-xyz", "val-xyz"), wantStatus: status.New(codes.OK, ""), }, - "DeniesRpcNoMatchInDenyAndAllow": { + "DeniesRPCNoMatchInDenyAndAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -209,7 +213,7 @@ var sdkTests = map[string]struct { }`, wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), }, - "AllowsRpcEmptyDenyMatchInAllow": { + "AllowsRPCEmptyDenyMatchInAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -238,7 +242,7 @@ var sdkTests = map[string]struct { }`, wantStatus: status.New(codes.OK, ""), }, - "DeniesRpcEmptyDenyNoMatchInAllow": { + "DeniesRPCEmptyDenyNoMatchInAllow": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -257,6 +261,45 @@ var sdkTests = map[string]struct { }`, wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), }, + "DeniesRPCRequestWithPrincipalsFieldOnUnauthenticatedConnection": { + authzPolicy: `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_TestServiceCalls", + "source": { + "principals": + [ + "foo" + ] + }, + "request": { + "paths": + [ + "/grpc.testing.TestService/*" + ] + } + } + ] + }`, + wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), + }, + "DeniesRPCRequestWithEmptyPrincipalsOnUnauthenticatedConnection": { + authzPolicy: `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_authenticated", + "source": { + "principals": [] + } + } + ] + }`, + wantStatus: status.New(codes.PermissionDenied, "unauthorized RPC request rejected"), + }, } func (s) TestSDKStaticPolicyEnd2End(t *testing.T) { @@ -315,6 +358,136 @@ func (s) TestSDKStaticPolicyEnd2End(t *testing.T) { } } +func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnTLSAuthenticatedConnection(t *testing.T) { + authzPolicy := `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_authenticated", + "source": { + "principals": [] + } + } + ] + }` + // Start a gRPC server with SDK unary server interceptor. + i, _ := authz.NewStatic(authzPolicy) + creds, err := credentials.NewServerTLSFromFile(testdata.Path("x509/server1_cert.pem"), testdata.Path("x509/server1_key.pem")) + if err != nil { + t.Fatalf("failed to generate credentials: %v", err) + } + s := grpc.NewServer( + grpc.Creds(creds), + grpc.ChainUnaryInterceptor(i.UnaryInterceptor)) + defer s.Stop() + pb.RegisterTestServiceServer(s, &testServer{}) + + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("error listening: %v", err) + } + go s.Serve(lis) + + // Establish a connection to the server. + creds, err = credentials.NewClientTLSFromFile(testdata.Path("x509/server_ca_cert.pem"), "x.test.example.com") + if err != nil { + t.Fatalf("failed to load credentials: %v", err) + } + clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(creds)) + if err != nil { + t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err) + } + defer clientConn.Close() + client := pb.NewTestServiceClient(clientConn) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Verifying authorization decision. + if _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}); err != nil { + t.Fatalf("client.UnaryCall(_, _) = %v; want nil", err) + } +} + +func (s) TestSDKAllowsRPCRequestWithEmptyPrincipalsOnMTLSAuthenticatedConnection(t *testing.T) { + authzPolicy := `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_authenticated", + "source": { + "principals": [] + } + } + ] + }` + // Start a gRPC server with SDK unary server interceptor. + i, _ := authz.NewStatic(authzPolicy) + cert, err := tls.LoadX509KeyPair(testdata.Path("x509/server1_cert.pem"), testdata.Path("x509/server1_key.pem")) + if err != nil { + t.Fatalf("tls.LoadX509KeyPair(x509/server1_cert.pem, x509/server1_key.pem) failed: %v", err) + } + ca, err := ioutil.ReadFile(testdata.Path("x509/client_ca_cert.pem")) + if err != nil { + t.Fatalf("ioutil.ReadFile(x509/client_ca_cert.pem) failed: %v", err) + } + certPool := x509.NewCertPool() + if !certPool.AppendCertsFromPEM(ca) { + t.Fatal("failed to append certificates") + } + creds := credentials.NewTLS(&tls.Config{ + ClientAuth: tls.RequireAndVerifyClientCert, + Certificates: []tls.Certificate{cert}, + ClientCAs: certPool, + }) + s := grpc.NewServer( + grpc.Creds(creds), + grpc.ChainUnaryInterceptor(i.UnaryInterceptor)) + defer s.Stop() + pb.RegisterTestServiceServer(s, &testServer{}) + + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("error listening: %v", err) + } + go s.Serve(lis) + + // Establish a connection to the server. + cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client1_cert.pem"), testdata.Path("x509/client1_key.pem")) + if err != nil { + t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) + } + ca, err = ioutil.ReadFile(testdata.Path("x509/server_ca_cert.pem")) + if err != nil { + t.Fatalf("ioutil.ReadFile(x509/server_ca_cert.pem) failed: %v", err) + } + roots := x509.NewCertPool() + if !roots.AppendCertsFromPEM(ca) { + t.Fatal("failed to append certificates") + } + creds = credentials.NewTLS(&tls.Config{ + Certificates: []tls.Certificate{cert}, + RootCAs: roots, + ServerName: "x.test.example.com", + }) + clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(creds)) + if err != nil { + t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err) + } + defer clientConn.Close() + client := pb.NewTestServiceClient(clientConn) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Verifying authorization decision. + if _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}); err != nil { + t.Fatalf("client.UnaryCall(_, _) = %v; want nil", err) + } +} + func (s) TestSDKFileWatcherEnd2End(t *testing.T) { for name, test := range sdkTests { t.Run(name, func(t *testing.T) { @@ -387,7 +560,7 @@ func retryUntil(ctx context.Context, tsc pb.TestServiceClient, want *status.Stat } func (s) TestSDKFileWatcher_ValidPolicyRefresh(t *testing.T) { - valid1 := sdkTests["DeniesRpcMatchInDenyAndAllow"] + valid1 := sdkTests["DeniesRPCMatchInDenyAndAllow"] file := createTmpPolicyFile(t, "valid_policy_refresh", []byte(valid1.authzPolicy)) i, _ := authz.NewFileWatcher(file, 100*time.Millisecond) defer i.Close() @@ -419,23 +592,23 @@ func (s) TestSDKFileWatcher_ValidPolicyRefresh(t *testing.T) { // Verifying authorization decision. _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}) if got := status.Convert(err); got.Code() != valid1.wantStatus.Code() || got.Message() != valid1.wantStatus.Message() { - t.Fatalf("error want:{%v} got:{%v}", valid1.wantStatus.Err(), got.Err()) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got.Err(), valid1.wantStatus.Err()) } // Rewrite the file with a different valid authorization policy. - valid2 := sdkTests["AllowsRpcEmptyDenyMatchInAllow"] + valid2 := sdkTests["AllowsRPCEmptyDenyMatchInAllow"] if err := ioutil.WriteFile(file, []byte(valid2.authzPolicy), os.ModePerm); err != nil { t.Fatalf("ioutil.WriteFile(%q) failed: %v", file, err) } // Verifying authorization decision. if got := retryUntil(ctx, client, valid2.wantStatus); got != nil { - t.Fatalf("error want:{%v} got:{%v}", valid2.wantStatus.Err(), got) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got, valid2.wantStatus.Err()) } } func (s) TestSDKFileWatcher_InvalidPolicySkipReload(t *testing.T) { - valid := sdkTests["DeniesRpcMatchInDenyAndAllow"] + valid := sdkTests["DeniesRPCMatchInDenyAndAllow"] file := createTmpPolicyFile(t, "invalid_policy_skip_reload", []byte(valid.authzPolicy)) i, _ := authz.NewFileWatcher(file, 20*time.Millisecond) defer i.Close() @@ -467,7 +640,7 @@ func (s) TestSDKFileWatcher_InvalidPolicySkipReload(t *testing.T) { // Verifying authorization decision. _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}) if got := status.Convert(err); got.Code() != valid.wantStatus.Code() || got.Message() != valid.wantStatus.Message() { - t.Fatalf("error want:{%v} got:{%v}", valid.wantStatus.Err(), got.Err()) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got.Err(), valid.wantStatus.Err()) } // Skips the invalid policy update, and continues to use the valid policy. @@ -481,12 +654,12 @@ func (s) TestSDKFileWatcher_InvalidPolicySkipReload(t *testing.T) { // Verifying authorization decision. _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}) if got := status.Convert(err); got.Code() != valid.wantStatus.Code() || got.Message() != valid.wantStatus.Message() { - t.Fatalf("error want:{%v} got:{%v}", valid.wantStatus.Err(), got.Err()) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got.Err(), valid.wantStatus.Err()) } } func (s) TestSDKFileWatcher_RecoversFromReloadFailure(t *testing.T) { - valid1 := sdkTests["DeniesRpcMatchInDenyAndAllow"] + valid1 := sdkTests["DeniesRPCMatchInDenyAndAllow"] file := createTmpPolicyFile(t, "recovers_from_reload_failure", []byte(valid1.authzPolicy)) i, _ := authz.NewFileWatcher(file, 100*time.Millisecond) defer i.Close() @@ -518,7 +691,7 @@ func (s) TestSDKFileWatcher_RecoversFromReloadFailure(t *testing.T) { // Verifying authorization decision. _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}) if got := status.Convert(err); got.Code() != valid1.wantStatus.Code() || got.Message() != valid1.wantStatus.Message() { - t.Fatalf("error want:{%v} got:{%v}", valid1.wantStatus.Err(), got.Err()) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got.Err(), valid1.wantStatus.Err()) } // Skips the invalid policy update, and continues to use the valid policy. @@ -532,17 +705,17 @@ func (s) TestSDKFileWatcher_RecoversFromReloadFailure(t *testing.T) { // Verifying authorization decision. _, err = client.UnaryCall(ctx, &pb.SimpleRequest{}) if got := status.Convert(err); got.Code() != valid1.wantStatus.Code() || got.Message() != valid1.wantStatus.Message() { - t.Fatalf("error want:{%v} got:{%v}", valid1.wantStatus.Err(), got.Err()) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got.Err(), valid1.wantStatus.Err()) } // Rewrite the file with a different valid authorization policy. - valid2 := sdkTests["AllowsRpcEmptyDenyMatchInAllow"] + valid2 := sdkTests["AllowsRPCEmptyDenyMatchInAllow"] if err := ioutil.WriteFile(file, []byte(valid2.authzPolicy), os.ModePerm); err != nil { t.Fatalf("ioutil.WriteFile(%q) failed: %v", file, err) } // Verifying authorization decision. if got := retryUntil(ctx, client, valid2.wantStatus); got != nil { - t.Fatalf("error want:{%v} got:{%v}", valid2.wantStatus.Err(), got) + t.Fatalf("client.UnaryCall(_, _) = %v; want = %v", got, valid2.wantStatus.Err()) } } diff --git a/internal/xds/rbac/matchers.go b/internal/xds/rbac/matchers.go index 28dabf465919..6129a292d23e 100644 --- a/internal/xds/rbac/matchers.go +++ b/internal/xds/rbac/matchers.go @@ -395,13 +395,13 @@ func newAuthenticatedMatcher(authenticatedMatcherConfig *v3rbacpb.Principal_Auth } func (am *authenticatedMatcher) match(data *rpcData) bool { - // Represents this line in the RBAC documentation = "If unset, it applies to - // any user that is authenticated" (see package-level comments). An - // authenticated downstream in a stateful TLS connection will have to - // provide a certificate to prove their identity. Thus, you can simply check - // if there is a certificate present. + if data.authType != "tls" { + // Connection is not authenticated. + return false + } if am.stringMatcher == nil { - return len(data.certs) != 0 + // Allows any authenticated user. + return true } // "If there is no client certificate (thus no SAN nor Subject), check if "" // (empty string) matches. If it matches, the principal_name is said to diff --git a/internal/xds/rbac/rbac_engine.go b/internal/xds/rbac/rbac_engine.go index a25f9cfdeefa..ecb8512ac51a 100644 --- a/internal/xds/rbac/rbac_engine.go +++ b/internal/xds/rbac/rbac_engine.go @@ -187,10 +187,12 @@ func newRPCData(ctx context.Context) (*rpcData, error) { return nil, fmt.Errorf("error parsing local address: %v", err) } + var authType string var peerCertificates []*x509.Certificate if pi.AuthInfo != nil { tlsInfo, ok := pi.AuthInfo.(credentials.TLSInfo) if ok { + authType = pi.AuthInfo.AuthType() peerCertificates = tlsInfo.State.PeerCertificates } } @@ -201,6 +203,7 @@ func newRPCData(ctx context.Context) (*rpcData, error) { fullMethod: mn, destinationPort: uint32(dp), localAddr: conn.LocalAddr(), + authType: authType, certs: peerCertificates, }, nil } @@ -219,6 +222,8 @@ type rpcData struct { destinationPort uint32 // localAddr is the address that the RPC is being sent to. localAddr net.Addr + // authType is the type of authentication e.g. "tls". + authType string // certs are the certificates presented by the peer during a TLS // handshake. certs []*x509.Certificate diff --git a/internal/xds/rbac/rbac_engine_test.go b/internal/xds/rbac/rbac_engine_test.go index 17832458209a..e2e5d98c2a88 100644 --- a/internal/xds/rbac/rbac_engine_test.go +++ b/internal/xds/rbac/rbac_engine_test.go @@ -916,6 +916,20 @@ func (s) TestChainEngine(t *testing.T) { fullMethod: "some method", peerInfo: &peer.Peer{ Addr: &addr{ipAddress: "0.0.0.0"}, + AuthInfo: credentials.TLSInfo{ + State: tls.ConnectionState{ + PeerCertificates: []*x509.Certificate{ + { + URIs: []*url.URL{ + { + Host: "cluster.local", + Path: "/ns/default/sa/admin", + }, + }, + }, + }, + }, + }, }, }, wantStatusCode: codes.OK,