Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

authz: End2End test for AuditLogger #6304

Merged
merged 23 commits into from Jun 1, 2023
Merged

authz: End2End test for AuditLogger #6304

Changes from 1 commit
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
5778913
Draft of e2e test
erm-g May 18, 2023
cebf932
No Audit, Audit on Allow and Deny
erm-g May 18, 2023
d405ab2
Audit on Allow, Audit on Deny
erm-g May 19, 2023
0baf0e6
fix typo
erm-g May 19, 2023
b569c67
SPIFFE related testing
erm-g May 22, 2023
135565a
SPIFFE Id validation and certs creation script
erm-g May 22, 2023
c19e304
Address PR comments
erm-g May 23, 2023
1e7fcc4
Wrap tests using grpctest.Tester
erm-g May 23, 2023
e5991f2
Address PR comments
erm-g May 23, 2023
620d990
Change package name to authz_test to fit other end2end tests
erm-g May 23, 2023
cba398c
Add licence header, remove SPIFFE slice
erm-g May 24, 2023
de59ce5
Licence year change
erm-g May 24, 2023
9a8ee49
Address PR comments part 1
erm-g May 26, 2023
36bc552
Address PR comments part 2
erm-g May 26, 2023
b255385
Address PR comments part 3
erm-g May 26, 2023
7777c5b
Address PR comments final part
erm-g May 29, 2023
191fdf6
Drop newline for a brace
erm-g May 29, 2023
80d38b4
Address PR comments, fix outdated function comment
erm-g May 30, 2023
46fda00
Address PR comments
erm-g May 31, 2023
a02960d
Fix typo
erm-g May 31, 2023
56eba6e
Remove unused var
erm-g May 31, 2023
cb01a30
Address PR comment, change most test error handling to Errorf
erm-g Jun 1, 2023
4f30f15
Address PR comments
erm-g Jun 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
43 changes: 31 additions & 12 deletions authz/audit/audit_logging_test.go
View file
Open in desktop
Expand Up @@ -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
}
Expand All @@ -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) {
Expand Down Expand Up @@ -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{
Expand Down Expand Up @@ -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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inline this please, since it's so simple:

if _, err := client.UnaryCall(ctx, &testpb.SimpleRequest{}); err != nil {
	t.Fatalf("Unexpected error from UnaryCall: %v", err)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also need to check if the err is a permission denied one - it's a valid scenario for the test (we count deny as well).
I think I can rely on code only but still it'll be 2 LOCs

_, err = client.UnaryCall(ctx, &testpb.SimpleRequest{})
if errCode := status.Code(err); errCode != permissionDeniedStatus.Code() && errCode != codes.OK {
	t.Errorf("Call failed:%v", err)
}

Or I can create a helper function which takes SimpleResponse, error and returns back error code - smth like

if errCode := errorCodeExtractor(client.UnaryCall(ctx, &testpb.SimpleRequest{})); errCode != permissionDeniedStatus.Code() && errCode != codes.OK {
	t.Errorf("Call failed:%v", err)
}

WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is more of a problem of your test design.. You're using a table driven test but things are sufficiently different between iterations that it leads to these sorts of problems.

If you want to not care which one should happen:

if _, err := UnaryCall(); err != nil && status.Code(err) != codes.PermissionDenied {
	t.Fatalf()
}

Or if you want to require no error when unary should not be denied and vice-versa:

_, err := UnaryCall()
if test.wantAuthzOutcomes[false] > 1 /* LOL */ && status.Code(err) != codes.PermissionDenied {
	t.Fatalf()
} else if test.wantAuthzOutcomes[false] < 2 /* also LOL */ && err != nil {
	t.Fatalf()
}

Realistically, the latter is how your code should behave, but it's ugly. Maybe the test should be reworked a bit or split into multiple tests, as this seems wrong.

Copy link
Contributor Author

@erm-g erm-g May 31, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this is more of a problem of your test design.. You're using a table driven test but things are sufficiently different between iterations that it leads to these sorts of problems.

My idea is to have a set of authz policies with various audit_condition values. In order to test each audit_condition I need to make a few calls and then check if audit logger was invoked as expected. That's why I think that iterations are common enough to use a table driven approach. From my perspective

if _, err := UnaryCall(); err != nil && status.Code(err) != codes.PermissionDenied {
	t.Fatalf()
}

covers all the cases (we want to error test if a call returns an err but it's not a permission denied one).

Maybe the test should be reworked a bit or split into multiple tests, as this seems wrong.

What'd be your suggestion for refactoring? I think that splitting it will result in very similar tests with different err handling for permission denied cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My idea is to have a set of authz policies with various audit_condition values. In order to test each audit_condition I need to make a few calls and then check if audit logger was invoked as expected. That's why I think that iterations are common enough to use a table driven approach. From my perspective

Maybe add a codes.Code for the expected result from the Unary and Streaming RPC calls to the table?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I think it makes the test more readable. PTAL

_, err = client.UnaryCall(ctx, &testpb.SimpleRequest{})
validateCallResult(t, err)
stream, _ := client.StreamingInputCall(ctx)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please check the error.

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)
Expand All @@ -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},
Expand All @@ -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,
Expand All @@ -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()
Expand All @@ -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 {
Expand All @@ -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)
}
}