authz: support empty principals and fix rbac authenticated matcher (#4883)

* authz: support empty principals in SDK and fixes to rbac authenticated
matcher.

* Minor formatting

* Remove pointer from principals fields

* resolving comments

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 {

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",

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())
 	}
 }

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

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

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,