authz: support empty principals and fix rbac authenticated matcher

authz/rbac_translator.go

@@ -39,7 +39,7 @@ type header struct {
 }
 
 type peer struct {
-	Principals []string
+	Principals *[]string
 }
 
 type request struct {
@@ -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 {
@@ -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,138 @@ 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.
+	_, err = client.UnaryCall(ctx, &pb.SimpleRequest{})
+	if got := status.Convert(err); got.Code() != codes.OK {
+		t.Fatalf("error want:{%v} got:{%v}", codes.OK, got.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.
+	_, err = client.UnaryCall(ctx, &pb.SimpleRequest{})
+	if got := status.Convert(err); got.Code() != codes.OK {
+		t.Fatalf("error want:{%v} got:{%v}", codes.OK, got.Err())
+	}
+}
+
 func (s) TestSDKFileWatcherEnd2End(t *testing.T) {
 	for name, test := range sdkTests {
 		t.Run(name, func(t *testing.T) {

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.
+	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,