From 5778913ba76087bbdf7aa29f0f92b512484b3acb Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Thu, 18 May 2023 15:46:08 +0000 Subject: [PATCH 01/23] Draft of e2e test --- authz/grpc_audit_end2end_test.go | 101 +++++++++++++++++++++++++++++++ 1 file changed, 101 insertions(+) create mode 100644 authz/grpc_audit_end2end_test.go diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go new file mode 100644 index 00000000000..faa10755b1e --- /dev/null +++ b/authz/grpc_audit_end2end_test.go @@ -0,0 +1,101 @@ +package authz + +import ( + "context" + "net" + "testing" + "time" + + "google.golang.org/grpc" + "google.golang.org/grpc/credentials" + "google.golang.org/grpc/testdata" + + testgrpc "google.golang.org/grpc/interop/grpc_testing" + testpb "google.golang.org/grpc/interop/grpc_testing" +) + +type testServer struct { + testgrpc.UnimplementedTestServiceServer +} + +func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{}, nil +} + +func TestStdoutLogger(t *testing.T) { + authzPolicy := `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_UnaryCall", + "request": + { + "paths": + [ + "/grpc.testing.TestService/UnaryCall" + ] + } + } + ], + "deny_rules": [ + { + "name": "deny_policy_1", + "source": { + "principals":[ + "spiffe://foo.abc" + ] + } + } + ], + "audit_logging_options": { + "audit_condition": "ON_ALLOW", + "audit_loggers": [ + { + "name": "stdout_logger", + "config": {}, + "is_optional": false + } + ] + } + }` + + // Start a gRPC server with gRPC authz unary server interceptor. + i, _ := 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() + testgrpc.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 := testgrpc.NewTestServiceClient(clientConn) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + // Verifying authorization decision. + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil { + t.Fatalf("client.UnaryCall(_, _) = %v; want nil", err) + } + +} From cebf932bde984bc0ff562fc27615912f88febe17 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Thu, 18 May 2023 22:11:45 +0000 Subject: [PATCH 02/23] No Audit, Audit on Allow and Deny --- authz/grpc_audit_end2end_test.go | 224 +++++++++++++++++++++++++------ 1 file changed, 180 insertions(+), 44 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index faa10755b1e..53f46e23d82 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -2,11 +2,14 @@ package authz import ( "context" + "encoding/json" + "io" "net" "testing" "time" "google.golang.org/grpc" + "google.golang.org/grpc/authz/audit" "google.golang.org/grpc/credentials" "google.golang.org/grpc/testdata" @@ -22,8 +25,43 @@ func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) ( return &testpb.SimpleResponse{}, nil } -func TestStdoutLogger(t *testing.T) { - authzPolicy := `{ +type statAuditLogger struct { + Stat map[bool]int +} + +func (s *statAuditLogger) Log(event *audit.Event) { + if event.Authorized { + s.Stat[true]++ + } else { + s.Stat[false]++ + } +} + +type loggerBuilder struct { + Stat map[bool]int +} + +func (loggerBuilder) Name() string { + return "stat_logger" +} +func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { + return &statAuditLogger{ + Stat: lb.Stat, + } +} + +func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerConfig, error) { + return nil, nil +} + +func TestAuditLoggers(t *testing.T) { + tests := map[string]struct { + authzPolicy string + wantAllows int + wantDenies int + }{ + "No audit": { + authzPolicy: `{ "name": "authz", "allow_rules": [ @@ -38,64 +76,162 @@ func TestStdoutLogger(t *testing.T) { } } ], - "deny_rules": [ + "audit_logging_options": { + "audit_condition": "NONE", + "audit_loggers": [ + { + "name": "stat_logger", + "config": {}, + "is_optional": false + } + ] + } + }`, + wantAllows: 0, + wantDenies: 0, + }, + "Allow Unary Deny Streaming": { + authzPolicy: `{ + "name": "authz", + "allow_rules": + [ { - "name": "deny_policy_1", - "source": { - "principals":[ - "spiffe://foo.abc" + "name": "allow_all", + "request": { + "paths": + [ + "*" + ] + } + } + ], + "deny_rules": + [ + { + "name": "deny_all", + "request": { + "paths": + [ + "/grpc.testing.TestService/StreamingInputCall" ] } } ], "audit_logging_options": { - "audit_condition": "ON_ALLOW", + "audit_condition": "ON_DENY_AND_ALLOW", "audit_loggers": [ { - "name": "stdout_logger", + "name": "stat_logger", "config": {}, "is_optional": false } ] } - }` - - // Start a gRPC server with gRPC authz unary server interceptor. - i, _ := 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() - testgrpc.RegisterTestServiceServer(s, &testServer{}) - - lis, err := net.Listen("tcp", "localhost:0") - if err != nil { - t.Fatalf("error listening: %v", err) + }`, + wantAllows: 2, + wantDenies: 1, + }, } - go s.Serve(lis) + //authzPolicy := `{ + // "name": "authz", + // "allow_rules": + // [ + // { + // "name": "allow_UnaryCall", + // "request": + // { + // "paths": + // [ + // "/grpc.testing.TestService/UnaryCall" + // ] + // } + // } + // ], + // "deny_rules": [ + // { + // "name": "deny_policy_1", + // "source": { + // "principals":[ + // "spiffe://foo.abc" + // ] + // } + // } + // ], + // "audit_logging_options": { + // "audit_condition": "ON_ALLOW", + // "audit_loggers": [ + // { + // "name": "stat_logger", + // "config": {}, + // "is_optional": false + // } + // ] + // } + // }` - // 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 := testgrpc.NewTestServiceClient(clientConn) + for name, test := range tests { + t.Run(name, func(t *testing.T) { + lb := &loggerBuilder{ + Stat: make(map[bool]int), + } + audit.RegisterLoggerBuilder(lb) + // Start a gRPC server with gRPC authz unary server interceptor. + i, _ := NewStatic(test.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), + grpc.ChainStreamInterceptor(i.StreamInterceptor)) + defer s.Stop() + testgrpc.RegisterTestServiceServer(s, &testServer{}) - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) - defer cancel() + lis, err := net.Listen("tcp", "localhost:0") + if err != nil { + t.Fatalf("error listening: %v", err) + } + go s.Serve(lis) - // Verifying authorization decision. - if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil { - t.Fatalf("client.UnaryCall(_, _) = %v; want nil", err) - } + // 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 := testgrpc.NewTestServiceClient(clientConn) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + + client.UnaryCall(ctx, &testpb.SimpleRequest{}) + client.UnaryCall(ctx, &testpb.SimpleRequest{}) + stream, err := client.StreamingInputCall(ctx) + if err != nil { + t.Fatalf("failed StreamingInputCall err: %v", err) + } + req := &testpb.StreamingInputCallRequest{ + Payload: &testpb.Payload{ + Body: []byte("hi"), + }, + } + if err := stream.Send(req); err != nil && err != io.EOF { + t.Fatalf("failed stream.Send err: %v", err) + } + stream.CloseAndRecv() + + if lb.Stat[true] != test.wantAllows { + t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.Stat[true]) + } + if lb.Stat[false] != test.wantDenies { + t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.Stat[false]) + } + }) + } } From d405ab20d8c327272280bc9089cdb09e4a3fd8aa Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Fri, 19 May 2023 01:07:23 +0000 Subject: [PATCH 03/23] Audit on Allow, Audit on Deny --- authz/grpc_audit_end2end_test.go | 129 +++++++++++++++++-------------- 1 file changed, 71 insertions(+), 58 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 53f46e23d82..d2f823b0fc7 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -10,9 +10,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/authz/audit" - "google.golang.org/grpc/credentials" - "google.golang.org/grpc/testdata" - + "google.golang.org/grpc/credentials/insecure" testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" ) @@ -26,10 +24,12 @@ func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) ( } type statAuditLogger struct { - Stat map[bool]int + Stat map[bool]int + stdoutLogger audit.Logger } func (s *statAuditLogger) Log(event *audit.Event) { + s.stdoutLogger.Log(event) if event.Authorized { s.Stat[true]++ } else { @@ -46,7 +46,8 @@ func (loggerBuilder) Name() string { } func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ - Stat: lb.Stat, + Stat: lb.Stat, + stdoutLogger: audit.GetLoggerBuilder("stdout_logger").Build(nil), } } @@ -90,7 +91,7 @@ func TestAuditLoggers(t *testing.T) { wantAllows: 0, wantDenies: 0, }, - "Allow Unary Deny Streaming": { + "Allow All Deny Streaming - Audit All": { authzPolicy: `{ "name": "authz", "allow_rules": @@ -131,87 +132,98 @@ func TestAuditLoggers(t *testing.T) { wantAllows: 2, wantDenies: 1, }, + "Allow Unary - Audit Allow": { + authzPolicy: `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_UnaryCall", + "request": + { + "paths": + [ + "/grpc.testing.TestService/UnaryCall" + ] + } + } + ], + "audit_logging_options": { + "audit_condition": "ON_ALLOW", + "audit_loggers": [ + { + "name": "stat_logger", + "config": {}, + "is_optional": false + } + ] + } + }`, + wantAllows: 2, + wantDenies: 0, + }, + "Allow Typo - Audit Deny": { + authzPolicy: `{ + "name": "authz", + "allow_rules": + [ + { + "name": "allow_UnaryCall", + "request": + { + "paths": + [ + "/grpc.testing.TestService/UnaryCall_Z" + ] + } + } + ], + "audit_logging_options": { + "audit_condition": "ON_DENY", + "audit_loggers": [ + { + "name": "stat_logger", + "config": {}, + "is_optional": false + } + ] + } + }`, + wantAllows: 0, + wantDenies: 3, + }, } - //authzPolicy := `{ - // "name": "authz", - // "allow_rules": - // [ - // { - // "name": "allow_UnaryCall", - // "request": - // { - // "paths": - // [ - // "/grpc.testing.TestService/UnaryCall" - // ] - // } - // } - // ], - // "deny_rules": [ - // { - // "name": "deny_policy_1", - // "source": { - // "principals":[ - // "spiffe://foo.abc" - // ] - // } - // } - // ], - // "audit_logging_options": { - // "audit_condition": "ON_ALLOW", - // "audit_loggers": [ - // { - // "name": "stat_logger", - // "config": {}, - // "is_optional": false - // } - // ] - // } - // }` for name, test := range tests { t.Run(name, func(t *testing.T) { + // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors lb := &loggerBuilder{ Stat: make(map[bool]int), } audit.RegisterLoggerBuilder(lb) - // Start a gRPC server with gRPC authz unary server interceptor. i, _ := NewStatic(test.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), grpc.ChainStreamInterceptor(i.StreamInterceptor)) defer s.Stop() testgrpc.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)) + clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) if err != nil { t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err) } defer clientConn.Close() client := testgrpc.NewTestServiceClient(clientConn) - ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() + //Make 2 unary calls and 1 streaming call client.UnaryCall(ctx, &testpb.SimpleRequest{}) client.UnaryCall(ctx, &testpb.SimpleRequest{}) - stream, err := client.StreamingInputCall(ctx) if err != nil { t.Fatalf("failed StreamingInputCall err: %v", err) @@ -226,6 +238,7 @@ func TestAuditLoggers(t *testing.T) { } stream.CloseAndRecv() + //Compare expected number of allows/denies with content of internal map of statAuditLogger if lb.Stat[true] != test.wantAllows { t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.Stat[true]) } From 0baf0e6b3bcb85e6c1551d2e1cb64fbcfa1362a5 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Fri, 19 May 2023 01:19:16 +0000 Subject: [PATCH 04/23] fix typo --- authz/grpc_audit_end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index d2f823b0fc7..09550e457be 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -55,7 +55,7 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon return nil, nil } -func TestAuditLoggers(t *testing.T) { +func TestAuditLogger(t *testing.T) { tests := map[string]struct { authzPolicy string wantAllows int From b569c67da412f244f2a359a1ac6cadf420341ee7 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Mon, 22 May 2023 05:11:22 +0000 Subject: [PATCH 05/23] SPIFFE related testing --- authz/grpc_audit_end2end_test.go | 49 +++++++++++++++++- testdata/x509/client_with_spiffe_cert.pem | 33 +++++++++++++ testdata/x509/client_with_spiffe_key.pem | 52 ++++++++++++++++++++ testdata/x509/client_with_spiffe_openssl.cnf | 17 +++++++ 4 files changed, 149 insertions(+), 2 deletions(-) create mode 100644 testdata/x509/client_with_spiffe_cert.pem create mode 100644 testdata/x509/client_with_spiffe_key.pem create mode 100644 testdata/x509/client_with_spiffe_openssl.cnf diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 09550e457be..23595455714 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -2,17 +2,23 @@ package authz import ( "context" + "crypto/tls" + "crypto/x509" "encoding/json" "io" "net" + "os" "testing" "time" "google.golang.org/grpc" "google.golang.org/grpc/authz/audit" - "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/credentials" testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" + "google.golang.org/grpc/testdata" + + _ "google.golang.org/grpc/authz/audit/stdout" ) type testServer struct { @@ -202,7 +208,27 @@ func TestAuditLogger(t *testing.T) { } audit.RegisterLoggerBuilder(lb) i, _ := NewStatic(test.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 := os.ReadFile(testdata.Path("x509/client_ca_cert.pem")) + if err != nil { + t.Fatalf("os.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), grpc.ChainStreamInterceptor(i.StreamInterceptor)) defer s.Stop() @@ -212,7 +238,26 @@ func TestAuditLogger(t *testing.T) { t.Fatalf("error listening: %v", err) } go s.Serve(lis) - clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + + cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) + if err != nil { + t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) + } + ca, err = os.ReadFile(testdata.Path("x509/server_ca_cert.pem")) + if err != nil { + t.Fatalf("os.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) } diff --git a/testdata/x509/client_with_spiffe_cert.pem b/testdata/x509/client_with_spiffe_cert.pem new file mode 100644 index 00000000000..b982fcbe554 --- /dev/null +++ b/testdata/x509/client_with_spiffe_cert.pem @@ -0,0 +1,33 @@ +-----BEGIN CERTIFICATE----- +MIIFxzCCA6+gAwIBAgICA+gwDQYJKoZIhvcNAQELBQAwUDELMAkGA1UEBhMCVVMx +CzAJBgNVBAgMAkNBMQwwCgYDVQQHDANTVkwxDTALBgNVBAoMBGdSUEMxFzAVBgNV +BAMMDnRlc3QtY2xpZW50X2NhMB4XDTIzMDUyMjA1MDA1NVoXDTMzMDUxOTA1MDA1 +NVowTjELMAkGA1UEBhMCVVMxCzAJBgNVBAgMAkNBMQwwCgYDVQQHDANTVkwxDTAL +BgNVBAoMBGdSUEMxFTATBgNVBAMMDHRlc3QtY2xpZW50MTCCAiIwDQYJKoZIhvcN +AQEBBQADggIPADCCAgoCggIBANXyLXGYzQFwLGwjzkeuo/y41voDH1Y9J+ee4qJU +OFuMKKXx5ai7n7dik4//J12OqJbbr416cFkKmcojwwbAdncXMV58EF82Bt8QRov0 +Vtoio/wxlyRlxDlVYwr56W+0EVP9Q+kzA/dTnMgOQYIeSix96CUQRy8XDu1YX3rk +fiUkND9xxuQw8OXi3LXguv/lilLVC/lXiXwa0RWEgMZZU2S1/lAElAG3aZuuWULG +K+PpKPuqkcptbUPCvNN1eUs9/D82aoFuqRCmpTC+7bUO+SJSggpUHcgTbXT9i6OO +9eR0ijcaQjtb0Y6ro+Cv60YOnlGC8It3KoY2SxioyqdceRUohqs4T4hjBEckzz11 +AC0Pj0Gp4NJPcOY68EjhD5rvncn76RRr3z2XZpd+2Nz+Fldxk/aaejfdgqs9lo1g +C+aP+nk9oqSpFAc+rpHsblLZehUur/FHhenn1pYWqkSJsAG0sFW4sDHATRIfva3c +HNHB5kBzruGymywBGO0xOw7+s5XzPiNnbXT5FBY1rKG7RwlqdtDh6LWJRHmEblWV +tPHNiY+rrStv0rN7Hk/YKcSXd5JiTjk3GXjO1YJJVEraEWHlxzdGy+xu/m0iJLed +pxZwuxxdZ/Q2+Ht+X9pO2DsW8BQFbddCwbooxKygwSlmHCN1gRSWqWMZY5nzsxxY +tic9AgMBAAGjgawwgakwDAYDVR0TAQH/BAIwADAdBgNVHQ4EFgQUyiXne0d3g9zN +gjhCdl2d9ykxIfgwDgYDVR0PAQH/BAQDAgXgMBYGA1UdJQEB/wQMMAoGCCsGAQUF +BwMCMDEGA1UdEQQqMCiGJnNwaWZmZTovL2Zvby5iYXIuY29tL2NsaWVudC93b3Jr +bG9hZC8xMB8GA1UdIwQYMBaAFOr3a0MblN9W9Opu7VsDn3crpoDCMA0GCSqGSIb3 +DQEBCwUAA4ICAQB3pt3mLXDDcReko9eTFahkNyU2zGP7CSi1RcgfP1aJDLBTjePb +JUhoY14tSpOGSliXWscXbNveW+Yk+tB411r8SJuXIkaYZM2BJQDWFzL7aLfAQSx5 +rf8tHVyKO89uBoQtgEnxZp9BFhBp/b2y5DLrZWjM6W9s21C9S9UIFjD8UwrKicNH +HGxIC6AZ6yc0x2Nsq/KW1IZ6HDueZRB4tud75wwkPVpS5fb+XqIJEBP7lgYrJjGp +aLLxV2vn1kX2/qbH31hhWVpNyPkpFsT+IbkPFLDyQoZKHbewD6M56+KBRTTENETQ +hFLgJB0HiICJ2I6cqw1UbDJMJFkcnThsuI8Wg9dxZ+OffYeZ5bnFCVIg0WUi9oMK +JDXZAqYDwBaQHyNszaYzZ5VE2Gd/K8PEDevW4RblI+vAOamIM5w1DjQHWf7n1byt +nGwnxt4IQ5vwlrdX3FDcEkhacHdcniX/FTpYrfOistPh+QpBAvA92DG1CbAf2nKY +yXLx+Ho7tUEBGioU4XvRHccwumfatf5z+JO/EvIi2yWd1tanl5J3o/sSs9ixJfx4 +aSuM+zAwf8EM+YGqYMCZ896+T6/r7NAg+YIDYu1K5b5QqYyPanqNqUf9VTR4oQ4v ++jdb5PkujXbjENvkAhNbUyUbQJ+IU0KHm3/sdhRPN5tuc9C+BTSQvlmKkw== +-----END CERTIFICATE----- diff --git a/testdata/x509/client_with_spiffe_key.pem b/testdata/x509/client_with_spiffe_key.pem new file mode 100644 index 00000000000..6adcdc3122c --- /dev/null +++ b/testdata/x509/client_with_spiffe_key.pem @@ -0,0 +1,52 @@ +-----BEGIN PRIVATE KEY----- +MIIJQwIBADANBgkqhkiG9w0BAQEFAASCCS0wggkpAgEAAoICAQDV8i1xmM0BcCxs +I85HrqP8uNb6Ax9WPSfnnuKiVDhbjCil8eWou5+3YpOP/yddjqiW26+NenBZCpnK +I8MGwHZ3FzFefBBfNgbfEEaL9FbaIqP8MZckZcQ5VWMK+elvtBFT/UPpMwP3U5zI +DkGCHkosfeglEEcvFw7tWF965H4lJDQ/ccbkMPDl4ty14Lr/5YpS1Qv5V4l8GtEV +hIDGWVNktf5QBJQBt2mbrllCxivj6Sj7qpHKbW1DwrzTdXlLPfw/NmqBbqkQpqUw +vu21DvkiUoIKVB3IE210/YujjvXkdIo3GkI7W9GOq6Pgr+tGDp5RgvCLdyqGNksY +qMqnXHkVKIarOE+IYwRHJM89dQAtD49BqeDST3DmOvBI4Q+a753J++kUa989l2aX +ftjc/hZXcZP2mno33YKrPZaNYAvmj/p5PaKkqRQHPq6R7G5S2XoVLq/xR4Xp59aW +FqpEibABtLBVuLAxwE0SH72t3BzRweZAc67hspssARjtMTsO/rOV8z4jZ210+RQW +Nayhu0cJanbQ4ei1iUR5hG5VlbTxzYmPq60rb9Kzex5P2CnEl3eSYk45Nxl4ztWC +SVRK2hFh5cc3Rsvsbv5tIiS3nacWcLscXWf0Nvh7fl/aTtg7FvAUBW3XQsG6KMSs +oMEpZhwjdYEUlqljGWOZ87McWLYnPQIDAQABAoICAAY5tM7QZm67R9+hrxfw4f6x +ljfSLXBB+U5JFkko8DbhvjEN9+PQCda5PJf9EbUsOIWjQNl6DZjZsR3rqnog0ZGn +kB0yuPs8RDjrbVIXOwu/5EurWb2KZIpSjL4+BWflsndiMD6x6FSjDzXXDFrv7LKc +u0uQzLF3F00avDSEP5NvGUIbWnE7Z1cZIdj9ABQAJuVAI8gOnwaIdTsODv02jjGp +BgxoBbKDFsSb7yb9QzuvhizEitd8FajaGsqAaZYh6JwiRjkb8jl0z+u6MoqJNACm +q/gG+JLg1deIpS6OM2OBbKAr2G+HvXJMVklsdQkl1b+DcuJsBkW/gLHn/3WdQDyq +t9sB8XrUx3S5dy6oroj9tcrwwlpUPbx3/37BX7OEn/NlIWZojI62hGexoFaggu3O +Jg0JJYH8THlQqs9G/oXmRTQKngse2FLEIPePie9vIIvokiQtG4T6miOK+6QmxYZq +H+KPT8AQG8j7AEexo4is1mEayapmTxftIYANOLFaT82BhsUOZksa698Sz7k1Cf/o +MSFn6CocGLflMEzdBqEq0wbBkdeuKUKlXG3ztXlqElN1xFdbzkaP/Tzl1ooq3kLR +0cLBCJNrHxTo1tuW4qTn+s4GLHpM4PE4YZgMehVWtyRFBb7mrSXsESw03POvulBx +65vA86DtCPm/jVuC5lQBAoIBAQD8IWDHYtQnvn/za6etc0fc7eTyq1jmoS/gh33y +eHaY6IccUN2LXCxgTJYwmfy57De58Im5AcOnkgGvw2Hw2i6+A5i4tKkyCa9awW4A +M20QOnyQpF+9uiIqGzI72skpfH20SvgTstTFtgGr3UBOqTfcApL+1X4guvGnY+Cx +uHUAPzbis9G3CNOWb4iiLhUcBnTDZyB3MPM4S1U8E5JLFo86+va6gbqdBP4ac+KH +08XDk/z6ohp9p796o6IiBQyZEsVaYLCrzjSOXeFfE5Fyj2z53oGlws+/PdhXKo02 +3++zRESiLVuGbCmAN17nKwDbZu9kFfGNP2WdwhJt9Yey91I9AoIBAQDZOsXWNkyP +zoDcSrvJznMPFcwQCbMNyU7A+axXpAsxDqn16AQj5/t1PWqufjRSdC7gVUWFcQ2K +ldUHkNyGtqHVCcNpqsMZJT51NlgTOl1A3GLnmm+tAiMCcr7aySeNnlj08fW278Ek +fnxpgUqGtXjTFpArULSFdZulXNPAP85ZDBburJtdhMfiT3GyQ1iRZcXkzsUVzNU1 +nGGk0jtCodlzQKiz3/aHO63G0GAjtdPuXpzGm7nBJSgLD0GabkCdkHDFULOaraYy +t1zsCsg7tQWa4KGRDNkcJKzoz3zf1sI4g87UJceGoXdB+mfluyKtnFhqjFalFW8Y +14Yb8YYdYHkBAoIBAC1pZaED7+poqWsSjNT02pC0WHRM4GpJxfHO9aRihhnsZ8l1 +1zFunJ+Lq9F9KsPiA/d9l5C2/KKF7b/WlSFoatrWkv9RqtfUXr0d8c4fdRljL2Rt +9sCZceXbmCSnt2u9fHaouh3yK9ige5SU+Swx1lnOLOOxWFJU2Ymot6PK8Wfl+uDC +OpeZA2MpG5b6bdrqXsWDIZnWOzh8eRGlBMh5e7rH0QCutQnrCEmDbd3BCvG7Cemq +oNLZD+fq6Rzvg+FePCWXHLsVHOo3how1XhEgPCSVKwzMFdcAMKMiiuTDWM0VEreT +K9T+TktFrdY9LJ5X3+5K9YLXVFohxmf/vT1CxpECggEBAIfegeVU+xgrYl/nAoPb +9A1oZcVWO78QvYhn4YrDmRhrApVDNGu86oPPEU3otBMqhjNcQmqPZpfa1W6xBa3g +x2H3hFkwLG0q5WDsx7PnGnK6JcaUyurcXkdmu8ceb/XdJ+i0+ioc1aJc1rYq3xFY +qiTlhPECvpaHE/4fDHa/sfHyZNmN7nNU3KzJYeTMyLXQgTF2vsC+6FBq6ovrzpMD +pn224I35NDorcqrapHdRgCgk10xGFK4g7mXUegT8lr+2m0JfEqdZm403MRCWQd1O +gR35CDUwYw9+RQQs2v8qVTqB/riklKK5lV0YISoInU0XcBncg0koGd/g1gneTDNN +pwECggEBAM4sDCCPplzbyd0yXLGo9P3RYIsNFnKnIm0YGRPrevBaiux3Qhr7Whpi +eV04BJ7Q58Z2WFzPFMhdXU45y4c6jIbmikdplEW1TASgXxOTvTfhg8P8ljdLPx+R +3CvQi4BPkJ3ZtYrHLKXKF/9aseyHLlSzuNUAJ6H0YxVi0tmzCFG82SWcFOzhR2Ec +cWDptGTRt9YY+Eo5rhPYbX/s8fCcW2u9QGnRnX35F8vJOp8Q7eCONIaN6faV4Yos +1wk6WXjZfDgEdjxmrnqXrgxdup82uD4Q1agmkxAjPl/9frLtHMW87Y0OixJb/Sve +eSCMKThlBQ57WubHTi2TbFBVKph/rP0= +-----END PRIVATE KEY----- diff --git a/testdata/x509/client_with_spiffe_openssl.cnf b/testdata/x509/client_with_spiffe_openssl.cnf new file mode 100644 index 00000000000..cf96f271d4a --- /dev/null +++ b/testdata/x509/client_with_spiffe_openssl.cnf @@ -0,0 +1,17 @@ +[req] +distinguished_name = req_distinguished_name +attributes = req_attributes + +[req_distinguished_name] + +[req_attributes] + +[test_client] +basicConstraints = critical,CA:FALSE +subjectKeyIdentifier = hash +keyUsage = critical,nonRepudiation,digitalSignature,keyEncipherment +extendedKeyUsage = critical,clientAuth +subjectAltName = @alt_names + +[alt_names] +URI = spiffe://foo.bar.com/client/workload/1 \ No newline at end of file From 135565ab9d8ed288b77300cdb3b3ad8efa2e4d62 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Mon, 22 May 2023 15:30:44 +0000 Subject: [PATCH 06/23] SPIFFE Id validation and certs creation script --- authz/grpc_audit_end2end_test.go | 17 +++++++++++++---- testdata/x509/create.sh | 19 +++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 23595455714..1424ae4ca44 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -32,6 +32,7 @@ func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) ( type statAuditLogger struct { Stat map[bool]int stdoutLogger audit.Logger + SpiffeIds []string } func (s *statAuditLogger) Log(event *audit.Event) { @@ -41,10 +42,12 @@ func (s *statAuditLogger) Log(event *audit.Event) { } else { s.Stat[false]++ } + s.SpiffeIds = append(s.SpiffeIds, event.Principal) } type loggerBuilder struct { - Stat map[bool]int + Stat map[bool]int + SpiffeIds []string } func (loggerBuilder) Name() string { @@ -61,6 +64,8 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon return nil, nil } +const spiffeId = "spiffe://foo.bar.com/client/workload/1" + func TestAuditLogger(t *testing.T) { tests := map[string]struct { authzPolicy string @@ -208,7 +213,6 @@ func TestAuditLogger(t *testing.T) { } audit.RegisterLoggerBuilder(lb) i, _ := NewStatic(test.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) @@ -226,7 +230,6 @@ func TestAuditLogger(t *testing.T) { Certificates: []tls.Certificate{cert}, ClientCAs: certPool, }) - s := grpc.NewServer( grpc.Creds(creds), grpc.ChainUnaryInterceptor(i.UnaryInterceptor), @@ -239,6 +242,7 @@ func TestAuditLogger(t *testing.T) { } go s.Serve(lis) + // Setup gRPC test client with certificates containing a SPIFFE Id cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) if err != nil { t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) @@ -256,7 +260,6 @@ func TestAuditLogger(t *testing.T) { 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) @@ -290,6 +293,12 @@ func TestAuditLogger(t *testing.T) { if lb.Stat[false] != test.wantDenies { t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.Stat[false]) } + //Compare recorded SPIFFE Ids with the value from cert + for _, id := range lb.SpiffeIds { + if id != spiffeId { + t.Errorf("Unexpected SPIFFE Id, want %v got %v", spiffeId, id) + } + } }) } } diff --git a/testdata/x509/create.sh b/testdata/x509/create.sh index 2cbc971fb8d..f7b116d84b7 100755 --- a/testdata/x509/create.sh +++ b/testdata/x509/create.sh @@ -128,5 +128,24 @@ openssl req -x509 \ -addext "subjectAltName = URI:spiffe://foo.bar.com/client/workload/1, URI:https://bar.baz.com/client" \ -sha256 +# Generate a cert with SPIFFE ID using client_with_spiffe_openssl.cnf +openssl req -new \ + -key client_with_spiffe_key.pem \ + -out client_with_spiffe_csr.pem \ + -subj /C=US/ST=CA/L=SVL/O=gRPC/CN=test-client1/ \ + -config ./client_with_spiffe_openssl.cnf \ + -reqexts test_client +openssl x509 -req \ + -in client_with_spiffe_csr.pem \ + -CAkey client_ca_key.pem \ + -CA client_ca_cert.pem \ + -days 3650 \ + -set_serial 1000 \ + -out client_with_spiffe_cert.pem \ + -extfile ./client_with_spiffe_openssl.cnf \ + -extensions test_client \ + -sha256 +openssl verify -verbose -CAfile client_with_spiffe_cert.pem + # Cleanup the CSRs. rm *_csr.pem From c19e304a28f13689dbbb95376af846a70e024d21 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Tue, 23 May 2023 02:54:55 +0000 Subject: [PATCH 07/23] Address PR comments --- authz/grpc_audit_end2end_test.go | 61 ++++++++++++++++++++++++-------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 1424ae4ca44..a5acf913772 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -8,9 +8,11 @@ import ( "io" "net" "os" + "strconv" "testing" "time" + "github.com/google/go-cmp/cmp" "google.golang.org/grpc" "google.golang.org/grpc/authz/audit" "google.golang.org/grpc/credentials" @@ -30,24 +32,29 @@ func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) ( } type statAuditLogger struct { - Stat map[bool]int - stdoutLogger audit.Logger - SpiffeIds []string + AuthzDescisionStat map[bool]int //Map to hold the counts of authorization decisions + EventContent map[string]string //Map to hold event fields in key:value fashion + SpiffeIds []string //Slice to hold collected SPIFFE IDs } func (s *statAuditLogger) Log(event *audit.Event) { - s.stdoutLogger.Log(event) if event.Authorized { - s.Stat[true]++ + s.AuthzDescisionStat[true]++ } else { - s.Stat[false]++ + s.AuthzDescisionStat[false]++ } s.SpiffeIds = append(s.SpiffeIds, event.Principal) + s.EventContent["rpc_method"] = event.FullMethodName + s.EventContent["principal"] = event.Principal + s.EventContent["policy_name"] = event.PolicyName + s.EventContent["matched_rule"] = event.MatchedRule + s.EventContent["authorized"] = strconv.FormatBool(event.Authorized) } type loggerBuilder struct { - Stat map[bool]int - SpiffeIds []string + AuthzDescisionStat map[bool]int + EventContent map[string]string + SpiffeIds []string } func (loggerBuilder) Name() string { @@ -55,8 +62,8 @@ func (loggerBuilder) Name() string { } func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ - Stat: lb.Stat, - stdoutLogger: audit.GetLoggerBuilder("stdout_logger").Build(nil), + AuthzDescisionStat: lb.AuthzDescisionStat, + EventContent: lb.EventContent, } } @@ -136,6 +143,10 @@ func TestAuditLogger(t *testing.T) { "name": "stat_logger", "config": {}, "is_optional": false + }, + { + "name": "stdout_logger", + "is_optional": false } ] } @@ -209,7 +220,8 @@ func TestAuditLogger(t *testing.T) { t.Run(name, func(t *testing.T) { // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors lb := &loggerBuilder{ - Stat: make(map[bool]int), + AuthzDescisionStat: make(map[bool]int), + EventContent: make(map[string]string), } audit.RegisterLoggerBuilder(lb) i, _ := NewStatic(test.authzPolicy) @@ -287,11 +299,11 @@ func TestAuditLogger(t *testing.T) { stream.CloseAndRecv() //Compare expected number of allows/denies with content of internal map of statAuditLogger - if lb.Stat[true] != test.wantAllows { - t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.Stat[true]) + if lb.AuthzDescisionStat[true] != test.wantAllows { + t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.AuthzDescisionStat[true]) } - if lb.Stat[false] != test.wantDenies { - t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.Stat[false]) + if lb.AuthzDescisionStat[false] != test.wantDenies { + t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.AuthzDescisionStat[false]) } //Compare recorded SPIFFE Ids with the value from cert for _, id := range lb.SpiffeIds { @@ -299,6 +311,25 @@ func TestAuditLogger(t *testing.T) { t.Errorf("Unexpected SPIFFE Id, want %v got %v", spiffeId, id) } } + //Special case - compare event fields with expected values from authz policy + if name == `Allow All Deny Streaming - Audit All` { + if diff := cmp.Diff(lb.EventContent, generateEventAsMap()); diff != "" { + t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) + } + } }) } } + +// generateEvent produces an map contaning audit.Event fields. +// It's used to compare captured audit.Event with the matched rule during +// `Allow All Deny Streaming - Audit All` scenario (authz_deny_all rule) +func generateEventAsMap() map[string]string { + return map[string]string{ + "rpc_method": "/grpc.testing.TestService/StreamingInputCall", + "principal": "spiffe://foo.bar.com/client/workload/1", + "policy_name": "authz", + "matched_rule": "authz_deny_all", + "authorized": "false", + } +} From 1e7fcc402557e4351331049e8f2403e273edf599 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Tue, 23 May 2023 03:20:38 +0000 Subject: [PATCH 08/23] Wrap tests using grpctest.Tester --- authz/grpc_audit_end2end_test.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index a5acf913772..ee2edec178b 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -16,6 +16,7 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/authz/audit" "google.golang.org/grpc/credentials" + "google.golang.org/grpc/internal/grpctest" testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" "google.golang.org/grpc/testdata" @@ -23,6 +24,14 @@ import ( _ "google.golang.org/grpc/authz/audit/stdout" ) +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { + grpctest.RunSubTests(t, s{}) +} + type testServer struct { testgrpc.UnimplementedTestServiceServer } @@ -73,7 +82,7 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon const spiffeId = "spiffe://foo.bar.com/client/workload/1" -func TestAuditLogger(t *testing.T) { +func (s) TestAuditLogger(t *testing.T) { tests := map[string]struct { authzPolicy string wantAllows int From e5991f23479e378dfaea9c12eaf2db7d0f3e61c7 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Tue, 23 May 2023 20:36:44 +0000 Subject: [PATCH 09/23] Address PR comments --- authz/grpc_audit_end2end_test.go | 75 ++++++++++++++------------------ 1 file changed, 33 insertions(+), 42 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index ee2edec178b..c3895f5773b 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -41,17 +41,13 @@ func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) ( } type statAuditLogger struct { - AuthzDescisionStat map[bool]int //Map to hold the counts of authorization decisions - EventContent map[string]string //Map to hold event fields in key:value fashion - SpiffeIds []string //Slice to hold collected SPIFFE IDs + AuthzDescisionStat map[bool]int // Map to hold the counts of authorization decisions + EventContent map[string]string // Map to hold event fields in key:value fashion + SpiffeIds []string // Slice to hold collected SPIFFE IDs } func (s *statAuditLogger) Log(event *audit.Event) { - if event.Authorized { - s.AuthzDescisionStat[true]++ - } else { - s.AuthzDescisionStat[false]++ - } + s.AuthzDescisionStat[event.Authorized]++ s.SpiffeIds = append(s.SpiffeIds, event.Principal) s.EventContent["rpc_method"] = event.FullMethodName s.EventContent["principal"] = event.Principal @@ -61,9 +57,9 @@ func (s *statAuditLogger) Log(event *audit.Event) { } type loggerBuilder struct { - AuthzDescisionStat map[bool]int - EventContent map[string]string - SpiffeIds []string + AuthDecisionStat map[bool]int + EventContent map[string]string + SpiffeIds []string } func (loggerBuilder) Name() string { @@ -71,7 +67,7 @@ func (loggerBuilder) Name() string { } func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ - AuthzDescisionStat: lb.AuthzDescisionStat, + AuthzDescisionStat: lb.AuthDecisionStat, EventContent: lb.EventContent, } } @@ -84,9 +80,10 @@ const spiffeId = "spiffe://foo.bar.com/client/workload/1" func (s) TestAuditLogger(t *testing.T) { tests := map[string]struct { - authzPolicy string - wantAllows int - wantDenies int + authzPolicy string + wantAllows int + wantDenies int + eventContent map[string]string }{ "No audit": { authzPolicy: `{ @@ -162,6 +159,13 @@ func (s) TestAuditLogger(t *testing.T) { }`, wantAllows: 2, wantDenies: 1, + eventContent: map[string]string{ + "rpc_method": "/grpc.testing.TestService/StreamingInputCall", + "principal": "spiffe://foo.bar.com/client/workload/1", + "policy_name": "authz", + "matched_rule": "authz_deny_all", + "authorized": "false", + }, }, "Allow Unary - Audit Allow": { authzPolicy: `{ @@ -227,10 +231,10 @@ func (s) TestAuditLogger(t *testing.T) { for name, test := range tests { t.Run(name, func(t *testing.T) { - // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors + // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors. lb := &loggerBuilder{ - AuthzDescisionStat: make(map[bool]int), - EventContent: make(map[string]string), + AuthDecisionStat: make(map[bool]int), + EventContent: make(map[string]string), } audit.RegisterLoggerBuilder(lb) i, _ := NewStatic(test.authzPolicy) @@ -263,7 +267,7 @@ func (s) TestAuditLogger(t *testing.T) { } go s.Serve(lis) - // Setup gRPC test client with certificates containing a SPIFFE Id + // Setup gRPC test client with certificates containing a SPIFFE Id. cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) if err != nil { t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) @@ -290,7 +294,7 @@ func (s) TestAuditLogger(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - //Make 2 unary calls and 1 streaming call + // Make 2 unary calls and 1 streaming call. client.UnaryCall(ctx, &testpb.SimpleRequest{}) client.UnaryCall(ctx, &testpb.SimpleRequest{}) stream, err := client.StreamingInputCall(ctx) @@ -307,38 +311,25 @@ func (s) TestAuditLogger(t *testing.T) { } stream.CloseAndRecv() - //Compare expected number of allows/denies with content of internal map of statAuditLogger - if lb.AuthzDescisionStat[true] != test.wantAllows { - t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.AuthzDescisionStat[true]) + // Compare expected number of allows/denies with content of internal map of statAuditLogger. + if lb.AuthDecisionStat[true] != test.wantAllows { + t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.AuthDecisionStat[true]) } - if lb.AuthzDescisionStat[false] != test.wantDenies { - t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.AuthzDescisionStat[false]) + if lb.AuthDecisionStat[false] != test.wantDenies { + t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.AuthDecisionStat[false]) } - //Compare recorded SPIFFE Ids with the value from cert + // Compare recorded SPIFFE Ids with the value from cert. for _, id := range lb.SpiffeIds { if id != spiffeId { t.Errorf("Unexpected SPIFFE Id, want %v got %v", spiffeId, id) } } - //Special case - compare event fields with expected values from authz policy - if name == `Allow All Deny Streaming - Audit All` { - if diff := cmp.Diff(lb.EventContent, generateEventAsMap()); diff != "" { + // Compare event fields with expected values from authz policy. + if test.eventContent != nil { + if diff := cmp.Diff(lb.EventContent, test.eventContent); diff != "" { t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) } } }) } } - -// generateEvent produces an map contaning audit.Event fields. -// It's used to compare captured audit.Event with the matched rule during -// `Allow All Deny Streaming - Audit All` scenario (authz_deny_all rule) -func generateEventAsMap() map[string]string { - return map[string]string{ - "rpc_method": "/grpc.testing.TestService/StreamingInputCall", - "principal": "spiffe://foo.bar.com/client/workload/1", - "policy_name": "authz", - "matched_rule": "authz_deny_all", - "authorized": "false", - } -} From 620d9909ed1045b1ffaa323eb31b7f5c0c37ac05 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Tue, 23 May 2023 20:47:53 +0000 Subject: [PATCH 10/23] Change package name to authz_test to fit other end2end tests --- authz/grpc_audit_end2end_test.go | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index c3895f5773b..3088583a3ce 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -1,4 +1,4 @@ -package authz +package authz_test import ( "context" @@ -14,6 +14,7 @@ import ( "github.com/google/go-cmp/cmp" "google.golang.org/grpc" + "google.golang.org/grpc/authz" "google.golang.org/grpc/authz/audit" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" @@ -24,22 +25,10 @@ import ( _ "google.golang.org/grpc/authz/audit/stdout" ) -type s struct { - grpctest.Tester -} - -func Test(t *testing.T) { +func TestAudit(t *testing.T) { grpctest.RunSubTests(t, s{}) } -type testServer struct { - testgrpc.UnimplementedTestServiceServer -} - -func (s *testServer) UnaryCall(ctx context.Context, req *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { - return &testpb.SimpleResponse{}, nil -} - type statAuditLogger struct { AuthzDescisionStat map[bool]int // Map to hold the counts of authorization decisions EventContent map[string]string // Map to hold event fields in key:value fashion @@ -237,7 +226,7 @@ func (s) TestAuditLogger(t *testing.T) { EventContent: make(map[string]string), } audit.RegisterLoggerBuilder(lb) - i, _ := NewStatic(test.authzPolicy) + i, _ := authz.NewStatic(test.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) From cba398c452e7702c03b0707009efb53c748c0540 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 24 May 2023 16:00:44 +0000 Subject: [PATCH 11/23] Add licence header, remove SPIFFE slice --- authz/grpc_audit_end2end_test.go | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 3088583a3ce..8d182701783 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -1,3 +1,21 @@ +/* + * + * Copyright 2021 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 authz_test import ( @@ -37,7 +55,6 @@ type statAuditLogger struct { func (s *statAuditLogger) Log(event *audit.Event) { s.AuthzDescisionStat[event.Authorized]++ - s.SpiffeIds = append(s.SpiffeIds, event.Principal) s.EventContent["rpc_method"] = event.FullMethodName s.EventContent["principal"] = event.Principal s.EventContent["policy_name"] = event.PolicyName @@ -48,7 +65,6 @@ func (s *statAuditLogger) Log(event *audit.Event) { type loggerBuilder struct { AuthDecisionStat map[bool]int EventContent map[string]string - SpiffeIds []string } func (loggerBuilder) Name() string { @@ -65,8 +81,6 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon return nil, nil } -const spiffeId = "spiffe://foo.bar.com/client/workload/1" - func (s) TestAuditLogger(t *testing.T) { tests := map[string]struct { authzPolicy string @@ -307,12 +321,6 @@ func (s) TestAuditLogger(t *testing.T) { if lb.AuthDecisionStat[false] != test.wantDenies { t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.AuthDecisionStat[false]) } - // Compare recorded SPIFFE Ids with the value from cert. - for _, id := range lb.SpiffeIds { - if id != spiffeId { - t.Errorf("Unexpected SPIFFE Id, want %v got %v", spiffeId, id) - } - } // Compare event fields with expected values from authz policy. if test.eventContent != nil { if diff := cmp.Diff(lb.EventContent, test.eventContent); diff != "" { From de59ce537c4c87b84013ae44ef3a74d47f99b61f Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 24 May 2023 16:20:07 +0000 Subject: [PATCH 12/23] Licence year change --- authz/grpc_audit_end2end_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/grpc_audit_end2end_test.go b/authz/grpc_audit_end2end_test.go index 8d182701783..bd5717cabcc 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/grpc_audit_end2end_test.go @@ -1,6 +1,6 @@ /* * - * Copyright 2021 gRPC authors. + * Copyright 2023 gRPC authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. From 9a8ee492b72c14230c9b658d3735830e31be7dc9 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Fri, 26 May 2023 04:03:44 +0000 Subject: [PATCH 13/23] Address PR comments part 1 --- ..._end2end_test.go => audit_logging_test.go} | 74 +++++++++---------- 1 file changed, 36 insertions(+), 38 deletions(-) rename authz/{grpc_audit_end2end_test.go => audit_logging_test.go} (80%) diff --git a/authz/grpc_audit_end2end_test.go b/authz/audit_logging_test.go similarity index 80% rename from authz/grpc_audit_end2end_test.go rename to authz/audit_logging_test.go index bd5717cabcc..493e9fc6248 100644 --- a/authz/grpc_audit_end2end_test.go +++ b/authz/audit_logging_test.go @@ -48,32 +48,32 @@ func TestAudit(t *testing.T) { } type statAuditLogger struct { - AuthzDescisionStat map[bool]int // Map to hold the counts of authorization decisions - EventContent map[string]string // Map to hold event fields in key:value fashion - SpiffeIds []string // Slice to hold collected SPIFFE IDs + authDecisionStat map[bool]int // Map to hold the counts of authorization decisions + lastEventContent map[string]string // Map to hold event fields in key:value fashion } func (s *statAuditLogger) Log(event *audit.Event) { - s.AuthzDescisionStat[event.Authorized]++ - s.EventContent["rpc_method"] = event.FullMethodName - s.EventContent["principal"] = event.Principal - s.EventContent["policy_name"] = event.PolicyName - s.EventContent["matched_rule"] = event.MatchedRule - s.EventContent["authorized"] = strconv.FormatBool(event.Authorized) + s.authDecisionStat[event.Authorized]++ + s.lastEventContent["rpc_method"] = event.FullMethodName + s.lastEventContent["principal"] = event.Principal + s.lastEventContent["policy_name"] = event.PolicyName + s.lastEventContent["matched_rule"] = event.MatchedRule + s.lastEventContent["authorized"] = strconv.FormatBool(event.Authorized) } type loggerBuilder struct { - AuthDecisionStat map[bool]int - EventContent map[string]string + authDecisionStat map[bool]int + lastEventContent map[string]string } func (loggerBuilder) Name() string { return "stat_logger" } + func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ - AuthzDescisionStat: lb.AuthDecisionStat, - EventContent: lb.EventContent, + authDecisionStat: lb.authDecisionStat, + lastEventContent: lb.lastEventContent, } } @@ -81,7 +81,14 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon return nil, nil } +// TestAuditLogger examines audit logging invocations using four different authorization policies. +// It covers scenarios including a disabled audit, auditing both 'allow' and 'deny' outcomes, +// and separately auditing 'allow' and 'deny' outcomes. +// Additionally, it checks if SPIFFE ID from a certificate is propagated correctly. func (s) TestAuditLogger(t *testing.T) { + // Each test data entry contains an authz policy for a grpc server, + // how many 'allow' and 'deny' outcomes we expect (each test case makes 2 unary calls and one client-streaming call), + // and a structure to check if the audit.Event fields are properly populated. tests := map[string]struct { authzPolicy string wantAllows int @@ -91,14 +98,12 @@ func (s) TestAuditLogger(t *testing.T) { "No audit": { authzPolicy: `{ "name": "authz", - "allow_rules": - [ + "allow_rules": [ { "name": "allow_UnaryCall", "request": { - "paths": - [ + "paths": [ "/grpc.testing.TestService/UnaryCall" ] } @@ -121,20 +126,17 @@ func (s) TestAuditLogger(t *testing.T) { "Allow All Deny Streaming - Audit All": { authzPolicy: `{ "name": "authz", - "allow_rules": - [ + "allow_rules": [ { "name": "allow_all", "request": { - "paths": - [ + "paths": [ "*" ] } } ], - "deny_rules": - [ + "deny_rules": [ { "name": "deny_all", "request": { @@ -173,14 +175,12 @@ func (s) TestAuditLogger(t *testing.T) { "Allow Unary - Audit Allow": { authzPolicy: `{ "name": "authz", - "allow_rules": - [ + "allow_rules": [ { "name": "allow_UnaryCall", "request": { - "paths": - [ + "paths": [ "/grpc.testing.TestService/UnaryCall" ] } @@ -203,14 +203,12 @@ func (s) TestAuditLogger(t *testing.T) { "Allow Typo - Audit Deny": { authzPolicy: `{ "name": "authz", - "allow_rules": - [ + "allow_rules": [ { "name": "allow_UnaryCall", "request": { - "paths": - [ + "paths": [ "/grpc.testing.TestService/UnaryCall_Z" ] } @@ -236,8 +234,8 @@ func (s) TestAuditLogger(t *testing.T) { t.Run(name, func(t *testing.T) { // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors. lb := &loggerBuilder{ - AuthDecisionStat: make(map[bool]int), - EventContent: make(map[string]string), + authDecisionStat: make(map[bool]int), + lastEventContent: make(map[string]string), } audit.RegisterLoggerBuilder(lb) i, _ := authz.NewStatic(test.authzPolicy) @@ -315,15 +313,15 @@ func (s) TestAuditLogger(t *testing.T) { stream.CloseAndRecv() // Compare expected number of allows/denies with content of internal map of statAuditLogger. - if lb.AuthDecisionStat[true] != test.wantAllows { - t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.AuthDecisionStat[true]) + if lb.authDecisionStat[true] != test.wantAllows { + t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.authDecisionStat[true]) } - if lb.AuthDecisionStat[false] != test.wantDenies { - t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.AuthDecisionStat[false]) + if lb.authDecisionStat[false] != test.wantDenies { + t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.authDecisionStat[false]) } // Compare event fields with expected values from authz policy. if test.eventContent != nil { - if diff := cmp.Diff(lb.EventContent, test.eventContent); diff != "" { + if diff := cmp.Diff(lb.lastEventContent, test.eventContent); diff != "" { t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) } } From 36bc5524796b9ba51aae0f9b938db112c6408f04 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Fri, 26 May 2023 12:03:05 +0000 Subject: [PATCH 14/23] Address PR comments part 2 --- authz/audit_logging_test.go | 143 +++++++++++++++++++----------------- 1 file changed, 74 insertions(+), 69 deletions(-) diff --git a/authz/audit_logging_test.go b/authz/audit_logging_test.go index 493e9fc6248..1e55ba70559 100644 --- a/authz/audit_logging_test.go +++ b/authz/audit_logging_test.go @@ -48,12 +48,12 @@ func TestAudit(t *testing.T) { } type statAuditLogger struct { - authDecisionStat map[bool]int // Map to hold the counts of authorization decisions - lastEventContent map[string]string // Map to hold event fields in key:value fashion + authzDecisionStat map[bool]int // Map to hold the counts of authorization decisions + lastEventContent map[string]string // Map to hold event fields in key:value fashion } func (s *statAuditLogger) Log(event *audit.Event) { - s.authDecisionStat[event.Authorized]++ + s.authzDecisionStat[event.Authorized]++ s.lastEventContent["rpc_method"] = event.FullMethodName s.lastEventContent["principal"] = event.Principal s.lastEventContent["policy_name"] = event.PolicyName @@ -62,8 +62,8 @@ func (s *statAuditLogger) Log(event *audit.Event) { } type loggerBuilder struct { - authDecisionStat map[bool]int - lastEventContent map[string]string + authzDecisionStat map[bool]int + lastEventContent map[string]string } func (loggerBuilder) Name() string { @@ -72,8 +72,8 @@ func (loggerBuilder) Name() string { func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ - authDecisionStat: lb.authDecisionStat, - lastEventContent: lb.lastEventContent, + authzDecisionStat: lb.authzDecisionStat, + lastEventContent: lb.lastEventContent, } } @@ -89,13 +89,14 @@ func (s) TestAuditLogger(t *testing.T) { // Each test data entry contains an authz policy for a grpc server, // how many 'allow' and 'deny' outcomes we expect (each test case makes 2 unary calls and one client-streaming call), // and a structure to check if the audit.Event fields are properly populated. - tests := map[string]struct { - authzPolicy string - wantAllows int - wantDenies int - eventContent map[string]string + tests := []struct { + name string + authzPolicy string + wantAuthzOutcomes map[bool]int + eventContent map[string]string }{ - "No audit": { + { + name: "No audit", authzPolicy: `{ "name": "authz", "allow_rules": [ @@ -120,10 +121,10 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAllows: 0, - wantDenies: 0, + wantAuthzOutcomes: map[bool]int{true: 0, false: 0}, }, - "Allow All Deny Streaming - Audit All": { + { + name: "Allow All Deny Streaming - Audit All", authzPolicy: `{ "name": "authz", "allow_rules": [ @@ -162,8 +163,7 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAllows: 2, - wantDenies: 1, + wantAuthzOutcomes: map[bool]int{true: 2, false: 1}, eventContent: map[string]string{ "rpc_method": "/grpc.testing.TestService/StreamingInputCall", "principal": "spiffe://foo.bar.com/client/workload/1", @@ -172,7 +172,8 @@ func (s) TestAuditLogger(t *testing.T) { "authorized": "false", }, }, - "Allow Unary - Audit Allow": { + { + name: "Allow Unary - Audit Allow", authzPolicy: `{ "name": "authz", "allow_rules": [ @@ -197,10 +198,10 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAllows: 2, - wantDenies: 0, + wantAuthzOutcomes: map[bool]int{true: 2, false: 0}, }, - "Allow Typo - Audit Deny": { + { + name: "Allow Typo - Audit Deny", authzPolicy: `{ "name": "authz", "allow_rules": [ @@ -225,39 +226,22 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAllows: 0, - wantDenies: 3, + wantAuthzOutcomes: map[bool]int{true: 0, false: 3}, }, } - for name, test := range tests { - t.Run(name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors. lb := &loggerBuilder{ - authDecisionStat: make(map[bool]int), - lastEventContent: make(map[string]string), + authzDecisionStat: map[bool]int{true: 0, false: 0}, + lastEventContent: make(map[string]string), } audit.RegisterLoggerBuilder(lb) i, _ := authz.NewStatic(test.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 := os.ReadFile(testdata.Path("x509/client_ca_cert.pem")) - if err != nil { - t.Fatalf("os.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.Creds(loadServerCreds(t)), grpc.ChainUnaryInterceptor(i.UnaryInterceptor), grpc.ChainStreamInterceptor(i.StreamInterceptor)) defer s.Stop() @@ -269,24 +253,7 @@ func (s) TestAuditLogger(t *testing.T) { go s.Serve(lis) // Setup gRPC test client with certificates containing a SPIFFE Id. - cert, err = tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) - if err != nil { - t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) - } - ca, err = os.ReadFile(testdata.Path("x509/server_ca_cert.pem")) - if err != nil { - t.Fatalf("os.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)) + clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(loadClientCreds(t))) if err != nil { t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err) } @@ -313,11 +280,8 @@ func (s) TestAuditLogger(t *testing.T) { stream.CloseAndRecv() // Compare expected number of allows/denies with content of internal map of statAuditLogger. - if lb.authDecisionStat[true] != test.wantAllows { - t.Errorf("Allow case failed, want %v got %v", test.wantAllows, lb.authDecisionStat[true]) - } - if lb.authDecisionStat[false] != test.wantDenies { - t.Errorf("Deny case failed, want %v got %v", test.wantDenies, lb.authDecisionStat[false]) + if diff := cmp.Diff(lb.authzDecisionStat, test.wantAuthzOutcomes); diff != "" { + t.Fatalf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff) } // Compare event fields with expected values from authz policy. if test.eventContent != nil { @@ -328,3 +292,44 @@ func (s) TestAuditLogger(t *testing.T) { }) } } + +func loadServerCreds(t *testing.T) credentials.TransportCredentials { + 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 := os.ReadFile(testdata.Path("x509/client_ca_cert.pem")) + if err != nil { + t.Fatalf("os.ReadFile(x509/client_ca_cert.pem) failed: %v", err) + } + certPool := x509.NewCertPool() + if !certPool.AppendCertsFromPEM(ca) { + t.Fatal("failed to append certificates") + } + return credentials.NewTLS(&tls.Config{ + ClientAuth: tls.RequireAndVerifyClientCert, + Certificates: []tls.Certificate{cert}, + ClientCAs: certPool, + }) +} + +func loadClientCreds(t *testing.T) credentials.TransportCredentials { + cert, err := tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) + if err != nil { + t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) + } + ca, err := os.ReadFile(testdata.Path("x509/server_ca_cert.pem")) + if err != nil { + t.Fatalf("os.ReadFile(x509/server_ca_cert.pem) failed: %v", err) + } + roots := x509.NewCertPool() + if !roots.AppendCertsFromPEM(ca) { + t.Fatal("failed to append certificates") + } + return credentials.NewTLS(&tls.Config{ + Certificates: []tls.Certificate{cert}, + RootCAs: roots, + ServerName: "x.test.example.com", + }) + +} From b255385f5c9cf14f8725a17eb5792a289c3d2b41 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Fri, 26 May 2023 22:13:10 +0000 Subject: [PATCH 15/23] Address PR comments part 3 --- authz/{ => audit}/audit_logging_test.go | 135 +++++++++++++----------- testdata/x509/create.sh | 22 ++-- 2 files changed, 87 insertions(+), 70 deletions(-) rename authz/{ => audit}/audit_logging_test.go (73%) diff --git a/authz/audit_logging_test.go b/authz/audit/audit_logging_test.go similarity index 73% rename from authz/audit_logging_test.go rename to authz/audit/audit_logging_test.go index 1e55ba70559..7d11105ecd8 100644 --- a/authz/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -16,7 +16,7 @@ * */ -package authz_test +package audit_test import ( "context" @@ -26,7 +26,6 @@ import ( "io" "net" "os" - "strconv" "testing" "time" @@ -36,6 +35,7 @@ import ( "google.golang.org/grpc/authz/audit" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" + "google.golang.org/grpc/internal/stubserver" testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" "google.golang.org/grpc/testdata" @@ -43,27 +43,27 @@ import ( _ "google.golang.org/grpc/authz/audit/stdout" ) -func TestAudit(t *testing.T) { +type s struct { + grpctest.Tester +} + +func Test(t *testing.T) { grpctest.RunSubTests(t, s{}) } type statAuditLogger struct { - authzDecisionStat map[bool]int // Map to hold the counts of authorization decisions - lastEventContent map[string]string // Map to hold event fields in key:value fashion + authzDecisionStat map[bool]int // Map to hold the counts of authorization decisions + lastEvent *audit.Event // Map to hold event fields in key:value fashion } func (s *statAuditLogger) Log(event *audit.Event) { s.authzDecisionStat[event.Authorized]++ - s.lastEventContent["rpc_method"] = event.FullMethodName - s.lastEventContent["principal"] = event.Principal - s.lastEventContent["policy_name"] = event.PolicyName - s.lastEventContent["matched_rule"] = event.MatchedRule - s.lastEventContent["authorized"] = strconv.FormatBool(event.Authorized) + *s.lastEvent = *event } type loggerBuilder struct { authzDecisionStat map[bool]int - lastEventContent map[string]string + lastEvent *audit.Event } func (loggerBuilder) Name() string { @@ -73,7 +73,7 @@ func (loggerBuilder) Name() string { func (lb *loggerBuilder) Build(audit.LoggerConfig) audit.Logger { return &statAuditLogger{ authzDecisionStat: lb.authzDecisionStat, - lastEventContent: lb.lastEventContent, + lastEvent: lb.lastEvent, } } @@ -93,7 +93,7 @@ func (s) TestAuditLogger(t *testing.T) { name string authzPolicy string wantAuthzOutcomes map[bool]int - eventContent map[string]string + eventContent *audit.Event }{ { name: "No audit", @@ -102,8 +102,7 @@ func (s) TestAuditLogger(t *testing.T) { "allow_rules": [ { "name": "allow_UnaryCall", - "request": - { + "request": { "paths": [ "/grpc.testing.TestService/UnaryCall" ] @@ -141,8 +140,7 @@ func (s) TestAuditLogger(t *testing.T) { { "name": "deny_all", "request": { - "paths": - [ + "paths": [ "/grpc.testing.TestService/StreamingInputCall" ] } @@ -164,12 +162,12 @@ func (s) TestAuditLogger(t *testing.T) { } }`, wantAuthzOutcomes: map[bool]int{true: 2, false: 1}, - eventContent: map[string]string{ - "rpc_method": "/grpc.testing.TestService/StreamingInputCall", - "principal": "spiffe://foo.bar.com/client/workload/1", - "policy_name": "authz", - "matched_rule": "authz_deny_all", - "authorized": "false", + eventContent: &audit.Event{ + FullMethodName: "/grpc.testing.TestService/StreamingInputCall", + Principal: "spiffe://foo.bar.com/client/workload/1", + PolicyName: "authz", + MatchedRule: "authz_deny_all", + Authorized: false, }, }, { @@ -179,8 +177,7 @@ func (s) TestAuditLogger(t *testing.T) { "allow_rules": [ { "name": "allow_UnaryCall", - "request": - { + "request": { "paths": [ "/grpc.testing.TestService/UnaryCall" ] @@ -230,22 +227,38 @@ func (s) TestAuditLogger(t *testing.T) { }, } + serverCreds := loadServerCreds(t) + clientCreds := loadClientCreds(t) + + ss := &stubserver.StubServer{ + UnaryCallF: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { + return &testpb.SimpleResponse{}, nil + }, + FullDuplexCallF: func(stream testgrpc.TestService_FullDuplexCallServer) error { + _, err := stream.Recv() + if err != io.EOF { + return err + } + return nil + }, + } + for _, test := range tests { t.Run(test.name, func(t *testing.T) { // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors. lb := &loggerBuilder{ authzDecisionStat: map[bool]int{true: 0, false: 0}, - lastEventContent: make(map[string]string), + lastEvent: &audit.Event{}, } audit.RegisterLoggerBuilder(lb) i, _ := authz.NewStatic(test.authzPolicy) s := grpc.NewServer( - grpc.Creds(loadServerCreds(t)), + grpc.Creds(serverCreds), grpc.ChainUnaryInterceptor(i.UnaryInterceptor), grpc.ChainStreamInterceptor(i.StreamInterceptor)) defer s.Stop() - testgrpc.RegisterTestServiceServer(s, &testServer{}) + testgrpc.RegisterTestServiceServer(s, ss) lis, err := net.Listen("tcp", "localhost:0") if err != nil { t.Fatalf("error listening: %v", err) @@ -253,7 +266,7 @@ func (s) TestAuditLogger(t *testing.T) { go s.Serve(lis) // Setup gRPC test client with certificates containing a SPIFFE Id. - clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(loadClientCreds(t))) + clientConn, err := grpc.Dial(lis.Addr().String(), grpc.WithTransportCredentials(clientCreds)) if err != nil { t.Fatalf("grpc.Dial(%v) failed: %v", lis.Addr().String(), err) } @@ -262,21 +275,15 @@ func (s) TestAuditLogger(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - // Make 2 unary calls and 1 streaming call. client.UnaryCall(ctx, &testpb.SimpleRequest{}) client.UnaryCall(ctx, &testpb.SimpleRequest{}) stream, err := client.StreamingInputCall(ctx) - if err != nil { - t.Fatalf("failed StreamingInputCall err: %v", err) - } req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ Body: []byte("hi"), }, } - if err := stream.Send(req); err != nil && err != io.EOF { - t.Fatalf("failed stream.Send err: %v", err) - } + stream.Send(req) stream.CloseAndRecv() // Compare expected number of allows/denies with content of internal map of statAuditLogger. @@ -285,7 +292,7 @@ func (s) TestAuditLogger(t *testing.T) { } // Compare event fields with expected values from authz policy. if test.eventContent != nil { - if diff := cmp.Diff(lb.lastEventContent, test.eventContent); diff != "" { + if diff := cmp.Diff(lb.lastEvent, test.eventContent); diff != "" { t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) } } @@ -293,19 +300,11 @@ func (s) TestAuditLogger(t *testing.T) { } } +// loadServerCreds constructs TLS containing server certs and CA func loadServerCreds(t *testing.T) credentials.TransportCredentials { - 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 := os.ReadFile(testdata.Path("x509/client_ca_cert.pem")) - if err != nil { - t.Fatalf("os.ReadFile(x509/client_ca_cert.pem) failed: %v", err) - } - certPool := x509.NewCertPool() - if !certPool.AppendCertsFromPEM(ca) { - t.Fatal("failed to append certificates") - } + t.Helper() + cert := loadKeys(t, "x509/server1_cert.pem", "x509/server1_key.pem") + certPool := loadCaCerts(t, "x509/client_ca_cert.pem") return credentials.NewTLS(&tls.Config{ ClientAuth: tls.RequireAndVerifyClientCert, Certificates: []tls.Certificate{cert}, @@ -313,23 +312,41 @@ func loadServerCreds(t *testing.T) credentials.TransportCredentials { }) } +// loadClientCreds constructs TLS containing client certs and CA func loadClientCreds(t *testing.T) credentials.TransportCredentials { - cert, err := tls.LoadX509KeyPair(testdata.Path("x509/client_with_spiffe_cert.pem"), testdata.Path("x509/client_with_spiffe_key.pem")) + t.Helper() + cert := loadKeys(t, "x509/client_with_spiffe_cert.pem", "x509/client_with_spiffe_key.pem") + roots := loadCaCerts(t, "x509/server_ca_cert.pem") + return credentials.NewTLS(&tls.Config{ + Certificates: []tls.Certificate{cert}, + RootCAs: roots, + ServerName: "x.test.example.com", + }) + +} + +// loadCaCerts loads X509 key pair from the provided file paths. +// It is used for loading both client and server certificates for the test +func loadKeys(t *testing.T, certPath, key string) tls.Certificate { + t.Helper() + cert, err := tls.LoadX509KeyPair(testdata.Path(certPath), testdata.Path(key)) if err != nil { - t.Fatalf("tls.LoadX509KeyPair(x509/client1_cert.pem, x509/client1_key.pem) failed: %v", err) + t.Fatalf("tls.LoadX509KeyPair(%q, %q) failed: %v", certPath, key, err) } - ca, err := os.ReadFile(testdata.Path("x509/server_ca_cert.pem")) + return cert +} + +// loadCaCerts loads CA certificates and constructs x509.CertPool +// It is used for loading both client and server CAs for the test +func loadCaCerts(t *testing.T, certPath string) *x509.CertPool { + t.Helper() + ca, err := os.ReadFile(testdata.Path(certPath)) if err != nil { - t.Fatalf("os.ReadFile(x509/server_ca_cert.pem) failed: %v", err) + t.Fatalf("os.ReadFile(%q) failed: %v", certPath, err) } roots := x509.NewCertPool() if !roots.AppendCertsFromPEM(ca) { t.Fatal("failed to append certificates") } - return credentials.NewTLS(&tls.Config{ - Certificates: []tls.Certificate{cert}, - RootCAs: roots, - ServerName: "x.test.example.com", - }) - + return roots } diff --git a/testdata/x509/create.sh b/testdata/x509/create.sh index f7b116d84b7..378bd10cf24 100755 --- a/testdata/x509/create.sh +++ b/testdata/x509/create.sh @@ -130,20 +130,20 @@ openssl req -x509 \ # Generate a cert with SPIFFE ID using client_with_spiffe_openssl.cnf openssl req -new \ - -key client_with_spiffe_key.pem \ - -out client_with_spiffe_csr.pem \ + -key client_with_spiffe_key.pem \ + -out client_with_spiffe_csr.pem \ -subj /C=US/ST=CA/L=SVL/O=gRPC/CN=test-client1/ \ - -config ./client_with_spiffe_openssl.cnf \ + -config ./client_with_spiffe_openssl.cnf \ -reqexts test_client -openssl x509 -req \ - -in client_with_spiffe_csr.pem \ - -CAkey client_ca_key.pem \ - -CA client_ca_cert.pem \ - -days 3650 \ - -set_serial 1000 \ - -out client_with_spiffe_cert.pem \ +openssl x509 -req \ + -in client_with_spiffe_csr.pem \ + -CAkey client_ca_key.pem \ + -CA client_ca_cert.pem \ + -days 3650 \ + -set_serial 1000 \ + -out client_with_spiffe_cert.pem \ -extfile ./client_with_spiffe_openssl.cnf \ - -extensions test_client \ + -extensions test_client \ -sha256 openssl verify -verbose -CAfile client_with_spiffe_cert.pem From 7777c5bd912514634287e439ed2a45e9fbfba5ca Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Mon, 29 May 2023 01:40:07 +0000 Subject: [PATCH 16/23] Address PR comments final part --- authz/audit/audit_logging_test.go | 30 ++++++++++++++++-------------- 1 file changed, 16 insertions(+), 14 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 7d11105ecd8..93fc838caf9 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -81,14 +81,16 @@ func (*loggerBuilder) ParseLoggerConfig(config json.RawMessage) (audit.LoggerCon return nil, nil } -// TestAuditLogger examines audit logging invocations using four different authorization policies. -// It covers scenarios including a disabled audit, auditing both 'allow' and 'deny' outcomes, -// and separately auditing 'allow' and 'deny' outcomes. -// Additionally, it checks if SPIFFE ID from a certificate is propagated correctly. +// TestAuditLogger examines audit logging invocations using four different +// authorization policies. It covers scenarios including a disabled audit, +// auditing both 'allow' and 'deny' outcomes, and separately auditing 'allow' +// and 'deny' outcomes. Additionally, it checks if SPIFFE ID from a certificate +// is propagated correctly. func (s) TestAuditLogger(t *testing.T) { // Each test data entry contains an authz policy for a grpc server, - // how many 'allow' and 'deny' outcomes we expect (each test case makes 2 unary calls and one client-streaming call), - // and a structure to check if the audit.Event fields are properly populated. + // how many 'allow' and 'deny' outcomes we expect (each test case makes 2 + // unary calls and one client-streaming call), and a structure to check if + // the audit.Event fields are properly populated. tests := []struct { name string authzPolicy string @@ -226,10 +228,9 @@ func (s) TestAuditLogger(t *testing.T) { wantAuthzOutcomes: map[bool]int{true: 0, false: 3}, }, } - + //Construct the credentials for the tests and the stub server serverCreds := loadServerCreds(t) clientCreds := loadClientCreds(t) - ss := &stubserver.StubServer{ UnaryCallF: func(ctx context.Context, in *testpb.SimpleRequest) (*testpb.SimpleResponse, error) { return &testpb.SimpleResponse{}, nil @@ -242,10 +243,10 @@ func (s) TestAuditLogger(t *testing.T) { return nil }, } - for _, test := range tests { t.Run(test.name, func(t *testing.T) { - // Setup test statAuditLogger, gRPC test server with authzPolicy, unary and stream interceptors. + // Setup test statAuditLogger, gRPC test server with authzPolicy, unary + // and stream interceptors. lb := &loggerBuilder{ authzDecisionStat: map[bool]int{true: 0, false: 0}, lastEvent: &audit.Event{}, @@ -261,7 +262,7 @@ func (s) TestAuditLogger(t *testing.T) { testgrpc.RegisterTestServiceServer(s, ss) lis, err := net.Listen("tcp", "localhost:0") if err != nil { - t.Fatalf("error listening: %v", err) + t.Fatalf("Error listening: %v", err) } go s.Serve(lis) @@ -277,7 +278,7 @@ func (s) TestAuditLogger(t *testing.T) { client.UnaryCall(ctx, &testpb.SimpleRequest{}) client.UnaryCall(ctx, &testpb.SimpleRequest{}) - stream, err := client.StreamingInputCall(ctx) + stream, _ := client.StreamingInputCall(ctx) req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ Body: []byte("hi"), @@ -286,7 +287,8 @@ func (s) TestAuditLogger(t *testing.T) { stream.Send(req) stream.CloseAndRecv() - // Compare expected number of allows/denies with content of internal map of statAuditLogger. + // Compare expected number of allows/denies with content of the internal + // map of statAuditLogger. if diff := cmp.Diff(lb.authzDecisionStat, test.wantAuthzOutcomes); diff != "" { t.Fatalf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff) } @@ -346,7 +348,7 @@ func loadCaCerts(t *testing.T, certPath string) *x509.CertPool { } roots := x509.NewCertPool() if !roots.AppendCertsFromPEM(ca) { - t.Fatal("failed to append certificates") + t.Fatal("Failed to append certificates") } return roots } From 191fdf63bcc2a38fdf15eed8face7bb67f46b004 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Mon, 29 May 2023 03:24:02 +0000 Subject: [PATCH 17/23] Drop newline for a brace --- authz/audit/audit_logging_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 93fc838caf9..f2ec7be953d 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -206,8 +206,7 @@ func (s) TestAuditLogger(t *testing.T) { "allow_rules": [ { "name": "allow_UnaryCall", - "request": - { + "request": { "paths": [ "/grpc.testing.TestService/UnaryCall_Z" ] From 80d38b4365f98f68ec73e01048e2e35d5a03ce9f Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Tue, 30 May 2023 21:49:12 +0000 Subject: [PATCH 18/23] Address PR comments, fix outdated function comment --- authz/audit/audit_logging_test.go | 43 ++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 12 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index f2ec7be953d..1d3b4e6ee7d 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -33,16 +33,20 @@ import ( "google.golang.org/grpc" "google.golang.org/grpc/authz" "google.golang.org/grpc/authz/audit" + "google.golang.org/grpc/codes" "google.golang.org/grpc/credentials" "google.golang.org/grpc/internal/grpctest" "google.golang.org/grpc/internal/stubserver" testgrpc "google.golang.org/grpc/interop/grpc_testing" testpb "google.golang.org/grpc/interop/grpc_testing" + "google.golang.org/grpc/status" "google.golang.org/grpc/testdata" _ "google.golang.org/grpc/authz/audit/stdout" ) +var permissionDeniedStatus = status.New(codes.PermissionDenied, "unauthorized RPC request rejected") + type s struct { grpctest.Tester } @@ -53,7 +57,7 @@ func Test(t *testing.T) { type statAuditLogger struct { authzDecisionStat map[bool]int // Map to hold the counts of authorization decisions - lastEvent *audit.Event // Map to hold event fields in key:value fashion + lastEvent *audit.Event // Field to store last received event } func (s *statAuditLogger) Log(event *audit.Event) { @@ -227,7 +231,7 @@ func (s) TestAuditLogger(t *testing.T) { wantAuthzOutcomes: map[bool]int{true: 0, false: 3}, }, } - //Construct the credentials for the tests and the stub server + // Construct the credentials for the tests and the stub server serverCreds := loadServerCreds(t) clientCreds := loadClientCreds(t) ss := &stubserver.StubServer{ @@ -275,23 +279,26 @@ func (s) TestAuditLogger(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - client.UnaryCall(ctx, &testpb.SimpleRequest{}) - client.UnaryCall(ctx, &testpb.SimpleRequest{}) + _, err = client.UnaryCall(ctx, &testpb.SimpleRequest{}) + validateCallResult(t, err) + _, err = client.UnaryCall(ctx, &testpb.SimpleRequest{}) + validateCallResult(t, err) stream, _ := client.StreamingInputCall(ctx) req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ Body: []byte("hi"), }, } - stream.Send(req) - stream.CloseAndRecv() + validateCallResult(t, stream.Send(req)) + _, err = stream.CloseAndRecv() + validateCallResult(t, err) // Compare expected number of allows/denies with content of the internal // map of statAuditLogger. if diff := cmp.Diff(lb.authzDecisionStat, test.wantAuthzOutcomes); diff != "" { t.Fatalf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff) } - // Compare event fields with expected values from authz policy. + // Compare last event received by statAuditLogger with expected event. if test.eventContent != nil { if diff := cmp.Diff(lb.lastEvent, test.eventContent); diff != "" { t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) @@ -305,7 +312,7 @@ func (s) TestAuditLogger(t *testing.T) { func loadServerCreds(t *testing.T) credentials.TransportCredentials { t.Helper() cert := loadKeys(t, "x509/server1_cert.pem", "x509/server1_key.pem") - certPool := loadCaCerts(t, "x509/client_ca_cert.pem") + certPool := loadCACerts(t, "x509/client_ca_cert.pem") return credentials.NewTLS(&tls.Config{ ClientAuth: tls.RequireAndVerifyClientCert, Certificates: []tls.Certificate{cert}, @@ -317,7 +324,7 @@ func loadServerCreds(t *testing.T) credentials.TransportCredentials { func loadClientCreds(t *testing.T) credentials.TransportCredentials { t.Helper() cert := loadKeys(t, "x509/client_with_spiffe_cert.pem", "x509/client_with_spiffe_key.pem") - roots := loadCaCerts(t, "x509/server_ca_cert.pem") + roots := loadCACerts(t, "x509/server_ca_cert.pem") return credentials.NewTLS(&tls.Config{ Certificates: []tls.Certificate{cert}, RootCAs: roots, @@ -326,7 +333,7 @@ func loadClientCreds(t *testing.T) credentials.TransportCredentials { } -// loadCaCerts loads X509 key pair from the provided file paths. +// loadKeys loads X509 key pair from the provided file paths. // It is used for loading both client and server certificates for the test func loadKeys(t *testing.T, certPath, key string) tls.Certificate { t.Helper() @@ -337,9 +344,9 @@ func loadKeys(t *testing.T, certPath, key string) tls.Certificate { return cert } -// loadCaCerts loads CA certificates and constructs x509.CertPool +// loadCACerts loads CA certificates and constructs x509.CertPool // It is used for loading both client and server CAs for the test -func loadCaCerts(t *testing.T, certPath string) *x509.CertPool { +func loadCACerts(t *testing.T, certPath string) *x509.CertPool { t.Helper() ca, err := os.ReadFile(testdata.Path(certPath)) if err != nil { @@ -351,3 +358,15 @@ func loadCaCerts(t *testing.T, certPath string) *x509.CertPool { } return roots } + +// validateCallResult checks if the error resulting from making a call can be +// ignored. It is used for both unary and streaming calls in this test. +func validateCallResult(t *testing.T, err error) { + t.Helper() + if err == nil || err == io.EOF { + return + } + if errStatus := status.Convert(err); errStatus.Code() != permissionDeniedStatus.Code() || errStatus.Message() != permissionDeniedStatus.Message() { + t.Errorf("Call failed:%v", err) + } +} From 46fda00fe91949a990be441f754ed32d6b34e074 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 31 May 2023 19:37:48 +0000 Subject: [PATCH 19/23] Address PR comments --- authz/audit/audit_logging_test.go | 58 +++++++++++++++++-------------- 1 file changed, 31 insertions(+), 27 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 1d3b4e6ee7d..509ccfc53a2 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -94,12 +94,15 @@ func (s) TestAuditLogger(t *testing.T) { // Each test data entry contains an authz policy for a grpc server, // how many 'allow' and 'deny' outcomes we expect (each test case makes 2 // unary calls and one client-streaming call), and a structure to check if - // the audit.Event fields are properly populated. + // the audit.Event fields are properly populated. Additionally, we specify + // directly which authz outcome we expect from each type of call. tests := []struct { - name string - authzPolicy string - wantAuthzOutcomes map[bool]int - eventContent *audit.Event + name string + authzPolicy string + wantAuthzOutcomes map[bool]int + eventContent *audit.Event + wantUnaryCallCode codes.Code + wantStreamingCallCode codes.Code }{ { name: "No audit", @@ -126,7 +129,9 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAuthzOutcomes: map[bool]int{true: 0, false: 0}, + wantAuthzOutcomes: map[bool]int{true: 0, false: 0}, + wantUnaryCallCode: codes.OK, + wantStreamingCallCode: codes.PermissionDenied, }, { name: "Allow All Deny Streaming - Audit All", @@ -175,6 +180,8 @@ func (s) TestAuditLogger(t *testing.T) { MatchedRule: "authz_deny_all", Authorized: false, }, + wantUnaryCallCode: codes.OK, + wantStreamingCallCode: codes.PermissionDenied, }, { name: "Allow Unary - Audit Allow", @@ -201,7 +208,9 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAuthzOutcomes: map[bool]int{true: 2, false: 0}, + wantAuthzOutcomes: map[bool]int{true: 2, false: 0}, + wantUnaryCallCode: codes.OK, + wantStreamingCallCode: codes.PermissionDenied, }, { name: "Allow Typo - Audit Deny", @@ -228,7 +237,9 @@ func (s) TestAuditLogger(t *testing.T) { ] } }`, - wantAuthzOutcomes: map[bool]int{true: 0, false: 3}, + wantAuthzOutcomes: map[bool]int{true: 0, false: 3}, + wantUnaryCallCode: codes.PermissionDenied, + wantStreamingCallCode: codes.PermissionDenied, }, } // Construct the credentials for the tests and the stub server @@ -279,19 +290,24 @@ func (s) TestAuditLogger(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) defer cancel() - _, err = client.UnaryCall(ctx, &testpb.SimpleRequest{}) - validateCallResult(t, err) - _, err = client.UnaryCall(ctx, &testpb.SimpleRequest{}) - validateCallResult(t, err) + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode { + t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) + } + if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode { + t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) + } stream, _ := client.StreamingInputCall(ctx) req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ Body: []byte("hi"), }, } - validateCallResult(t, stream.Send(req)) - _, err = stream.CloseAndRecv() - validateCallResult(t, err) + if err := stream.Send(req); err != nil && err != io.EOF { + t.Errorf("stream.Send failed:%v", err) + } + if _, err := stream.CloseAndRecv(); status.Code(err) != test.wantStreamingCallCode { + t.Fatalf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantUnaryCallCode) + } // Compare expected number of allows/denies with content of the internal // map of statAuditLogger. @@ -358,15 +374,3 @@ func loadCACerts(t *testing.T, certPath string) *x509.CertPool { } return roots } - -// validateCallResult checks if the error resulting from making a call can be -// ignored. It is used for both unary and streaming calls in this test. -func validateCallResult(t *testing.T, err error) { - t.Helper() - if err == nil || err == io.EOF { - return - } - if errStatus := status.Convert(err); errStatus.Code() != permissionDeniedStatus.Code() || errStatus.Message() != permissionDeniedStatus.Message() { - t.Errorf("Call failed:%v", err) - } -} From a02960d7a98b4030a44f0951f78625189fd74b4e Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 31 May 2023 19:39:00 +0000 Subject: [PATCH 20/23] Fix typo --- authz/audit/audit_logging_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 509ccfc53a2..0a43ffb6473 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -306,7 +306,7 @@ func (s) TestAuditLogger(t *testing.T) { t.Errorf("stream.Send failed:%v", err) } if _, err := stream.CloseAndRecv(); status.Code(err) != test.wantStreamingCallCode { - t.Fatalf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantUnaryCallCode) + t.Fatalf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode) } // Compare expected number of allows/denies with content of the internal From 56eba6e93ce7f261b59ce37a1a2c035e72963360 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 31 May 2023 19:51:08 +0000 Subject: [PATCH 21/23] Remove unused var --- authz/audit/audit_logging_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 0a43ffb6473..030fe3533d5 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -45,8 +45,6 @@ import ( _ "google.golang.org/grpc/authz/audit/stdout" ) -var permissionDeniedStatus = status.New(codes.PermissionDenied, "unauthorized RPC request rejected") - type s struct { grpctest.Tester } From cb01a30fbc625630ed5d2fd42b0991b38cb58af3 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Thu, 1 Jun 2023 15:33:54 +0000 Subject: [PATCH 22/23] Address PR comment, change most test error handling to Errorf --- authz/audit/audit_logging_test.go | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index 030fe3533d5..dcff6665256 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -289,12 +289,15 @@ func (s) TestAuditLogger(t *testing.T) { defer cancel() if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode { - t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) + t.Errorf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) } if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); status.Code(err) != test.wantUnaryCallCode { - t.Fatalf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) + t.Errorf("Unexpected UnaryCall fail: got %v want %v", err, test.wantUnaryCallCode) + } + stream, err := client.StreamingInputCall(ctx) + if err != nil { + t.Errorf("StreamingInputCall failed:%v", err) } - stream, _ := client.StreamingInputCall(ctx) req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ Body: []byte("hi"), @@ -304,18 +307,18 @@ func (s) TestAuditLogger(t *testing.T) { t.Errorf("stream.Send failed:%v", err) } if _, err := stream.CloseAndRecv(); status.Code(err) != test.wantStreamingCallCode { - t.Fatalf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode) + t.Errorf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode) } // Compare expected number of allows/denies with content of the internal // map of statAuditLogger. if diff := cmp.Diff(lb.authzDecisionStat, test.wantAuthzOutcomes); diff != "" { - t.Fatalf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff) + t.Errorf("Authorization decisions do not match\ndiff (-got +want):\n%s", diff) } // Compare last event received by statAuditLogger with expected event. if test.eventContent != nil { if diff := cmp.Diff(lb.lastEvent, test.eventContent); diff != "" { - t.Fatalf("Unexpected message\ndiff (-got +want):\n%s", diff) + t.Errorf("Unexpected message\ndiff (-got +want):\n%s", diff) } } }) From 4f30f1577a04d3f6694f7ced4ba2093f4b20c256 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Thu, 1 Jun 2023 17:17:44 +0000 Subject: [PATCH 23/23] Address PR comments --- authz/audit/audit_logging_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/authz/audit/audit_logging_test.go b/authz/audit/audit_logging_test.go index dcff6665256..e3a4ef25b02 100644 --- a/authz/audit/audit_logging_test.go +++ b/authz/audit/audit_logging_test.go @@ -296,7 +296,7 @@ func (s) TestAuditLogger(t *testing.T) { } stream, err := client.StreamingInputCall(ctx) if err != nil { - t.Errorf("StreamingInputCall failed:%v", err) + t.Fatalf("StreamingInputCall failed:%v", err) } req := &testpb.StreamingInputCallRequest{ Payload: &testpb.Payload{ @@ -304,7 +304,7 @@ func (s) TestAuditLogger(t *testing.T) { }, } if err := stream.Send(req); err != nil && err != io.EOF { - t.Errorf("stream.Send failed:%v", err) + t.Fatalf("stream.Send failed:%v", err) } if _, err := stream.CloseAndRecv(); status.Code(err) != test.wantStreamingCallCode { t.Errorf("Unexpected stream.CloseAndRecv fail: got %v want %v", err, test.wantStreamingCallCode)