[Audit Logging] End2end test for audit logging in authorization policy #33196

rockspore · 2023-05-19T16:55:09Z

I generated a new client key and cert where a Spiffe ID is added as the URI SAN. As such, we are able to test the audit log contains the principal correctly.

As for the test strategy, a test logger is registered to check the log count only and the log content is verified in details via the built-in stdout logger.

Update: I switched to use the test logger to verify the log content and removed stdout logger here because one the failure of RBE Windows Debug C/C++.

Update again: Refactored the test logger in a util such that the authz engine test also uses the same logger. Subsequently, xDS e2e test will also use it.

gtcooke94

A few initial comments

src/core/tsi/test_creds/README

test/cpp/end2end/BUILD

test/cpp/end2end/grpc_authz_end2end_test.cc

rockspore · 2023-05-19T18:34:01Z

Bazel RBE Windows Debug C/C++ has been consistently failing and it seems like the captured stdout failed to be unmarshaled into a valid json. This is difficult to troubleshoot as I don't have a windows machine (and running RBE Windows locally needs it as well).

I think I will switch to use the test logger to verify the log content and have to ditch stdout logger here.

test/cpp/end2end/grpc_authz_end2end_test.cc

gtcooke94

LGTM (assuming all tests are good, etc)

test/cpp/end2end/grpc_authz_end2end_test.cc

erm-g

LGTM

erm-g

Would it make sense to name it authz_audit_logging_utils?

Automated fix for refs/heads/authz-e2e

rockspore · 2023-05-24T15:21:23Z

Would it make sense to name it authz_audit_logging_utils?

We have public headers simply named grpc_audit_logging.h or audit_logging.h so I don' think it's strictly necessary. But let me know if you feel strongly about making this clear and I am open to changing it accordingly. @erm-g

erm-g · 2023-05-24T16:19:05Z

Would it make sense to name it authz_audit_logging_utils?

We have public headers simply named grpc_audit_logging.h or audit_logging.h so I don' think it's strictly necessary. But let me know if you feel strongly about making this clear and I am open to changing it accordingly. @erm-g

Yeah in this case I think I'm fine

markdroth

This looks good! My comments are all fairly minor, so feel free to merge after addressing.

markdroth · 2023-05-24T17:36:41Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -82,9 +93,15 @@ class GrpcAuthzEnd2EndTest : public ::testing::Test {
    channel_options.watch_identity_key_cert_pairs();
    channel_options.watch_root_certs();
    channel_creds_ = grpc::experimental::TlsCredentials(channel_options);
+


Nit: Please remove unnecessary blank lines within functions.

Same thing throughout.

markdroth · 2023-05-24T17:38:16Z

test/cpp/end2end/grpc_authz_end2end_test.cc

+  grpc::Status status;
+
+  ClientContext context1;
+  // Matches the allow rule.


Please split each of these cases into their own TEST_F() blocks, as per go/tott/649.

Same for all of these tests.

Done. Thanks for the link!

markdroth · 2023-05-24T17:44:47Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -798,6 +1120,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  grpc::Status status = SendRpc(channel, &context1, &resp1);
  EXPECT_TRUE(status.ok());
  EXPECT_EQ(resp1.message(), kMessage);
+  ASSERT_EQ(audit_logs_.size(), 1);


Suggest using EXPECT_THAT(audit_logs_, ::testing::ElementsAre(...)) here. That way, if the check fails, we won't bail out early.

markdroth · 2023-05-24T17:45:25Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -823,6 +1150,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  status = SendRpc(channel, &context2, &resp2);
  EXPECT_TRUE(status.ok());
  EXPECT_EQ(resp2.message(), kMessage);
+  EXPECT_EQ(audit_logs_.size(), 2);


Suggest using ElementsAre() here as well. Note that you can say audit_logs_.clear() after the previous pass to clear out the entry you've already checked.

markdroth · 2023-05-24T17:46:19Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -871,6 +1212,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  EXPECT_EQ(status.error_code(), grpc::StatusCode::PERMISSION_DENIED);
  EXPECT_EQ(status.error_message(), "Unauthorized RPC request rejected.");
  EXPECT_TRUE(resp3.message().empty());
+  EXPECT_EQ(audit_logs_.size(), 3);


rockspore

Thanks for the review, Mark!

rockspore · 2023-05-24T19:20:05Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -82,9 +93,15 @@ class GrpcAuthzEnd2EndTest : public ::testing::Test {
    channel_options.watch_identity_key_cert_pairs();
    channel_options.watch_root_certs();
    channel_creds_ = grpc::experimental::TlsCredentials(channel_options);
+


rockspore · 2023-05-24T19:20:19Z

test/cpp/end2end/grpc_authz_end2end_test.cc

+  grpc::Status status;
+
+  ClientContext context1;
+  // Matches the allow rule.


Done. Thanks for the link!

rockspore · 2023-05-24T19:20:31Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -798,6 +1120,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  grpc::Status status = SendRpc(channel, &context1, &resp1);
  EXPECT_TRUE(status.ok());
  EXPECT_EQ(resp1.message(), kMessage);
+  ASSERT_EQ(audit_logs_.size(), 1);


rockspore · 2023-05-24T19:20:36Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -823,6 +1150,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  status = SendRpc(channel, &context2, &resp2);
  EXPECT_TRUE(status.ok());
  EXPECT_EQ(resp2.message(), kMessage);
+  EXPECT_EQ(audit_logs_.size(), 2);


rockspore · 2023-05-24T19:20:43Z

test/cpp/end2end/grpc_authz_end2end_test.cc

@@ -871,6 +1212,11 @@ TEST_F(GrpcAuthzEnd2EndTest, FileWatcherRecoversFromFailure) {
  EXPECT_EQ(status.error_code(), grpc::StatusCode::PERMISSION_DENIED);
  EXPECT_EQ(status.error_message(), "Unauthorized RPC request rejected.");
  EXPECT_TRUE(resp3.message().empty());
+  EXPECT_EQ(audit_logs_.size(), 3);


grpc#33196) I generated a new client key and cert where a Spiffe ID is added as the URI SAN. As such, we are able to test the audit log contains the principal correctly. Update: I switched to use the test logger to verify the log content and removed stdout logger here because one the failure of [RBE Windows Debug C/C++](https://source.cloud.google.com/results/invocations/c3187f41-bb1f-44b3-b2b1-23f38e47386d). Update again: Refactored the test logger in a util such that the authz engine test also uses the same logger. Subsequently, xDS e2e test will also use it. --------- Co-authored-by: rockspore <rockspore@users.noreply.github.com>

rockspore added 2 commits May 19, 2023 16:47

end2end test for audit logging in authz policy

c554691

correct openssl command

162ecb2

rockspore requested a review from erm-g May 19, 2023 16:55

github-actions bot added lang/c++ lang/core labels May 19, 2023

rockspore requested a review from gtcooke94 May 19, 2023 16:55

rockspore added the release notes: no Indicates if PR should not be in release notes label May 19, 2023

remove extra include

4faa5fd

grpc-checks bot added per-call-memory/neutral per-channel-memory/neutral bloat/none labels May 19, 2023

gtcooke94 reviewed May 19, 2023

View reviewed changes

rockspore added 2 commits May 19, 2023 17:43

address PR comments

74b97ce

name in README.

1801a96

rockspore marked this pull request as draft May 19, 2023 18:34

rockspore added 2 commits May 19, 2023 18:56

verify log content in test logger

f302fa9

remove extra include

bda164f

rockspore marked this pull request as ready for review May 19, 2023 19:38

rockspore requested a review from gtcooke94 May 19, 2023 19:38

rockspore added the kokoro:force-run label May 19, 2023

grpc-kokoro removed the kokoro:force-run label May 19, 2023

gtcooke94 reviewed May 19, 2023

View reviewed changes

address PR comments

328224b

rockspore requested a review from gtcooke94 May 19, 2023 20:51

gtcooke94 approved these changes May 19, 2023

View reviewed changes

rockspore requested a review from markdroth May 19, 2023 21:06

erm-g requested changes May 22, 2023

View reviewed changes

test/cpp/end2end/grpc_authz_end2end_test.cc Show resolved Hide resolved

add test case with no logging

94af09e

rockspore requested a review from erm-g May 22, 2023 21:39

erm-g approved these changes May 23, 2023

View reviewed changes

rockspore added 5 commits May 23, 2023 20:01

make test logger as test util

648f581

Merge branch 'master' of github.com:grpc/grpc into authz-e2e

12a385d

newlines

7ecb862

sanity

86eed3b

more sanity

71d239f

erm-g reviewed May 23, 2023

View reviewed changes

rockspore and others added 3 commits May 23, 2023 21:39

Automated change: Fix sanity tests

aa29dd5

Merge pull request #31 from rockspore/create-pull-request/patch-71d239f

8636a49

Automated fix for refs/heads/authz-e2e

clang tidy

1bef4ca

markdroth approved these changes May 24, 2023

View reviewed changes

rockspore added 2 commits May 24, 2023 18:14

address PR comments

1ae9566

clang tidy

1be579d

rockspore commented May 24, 2023

View reviewed changes

rockspore merged commit de9d398 into grpc:master May 24, 2023
64 of 65 checks passed

copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 25, 2023

yijiem added release notes: yes Indicates if PR needs to be in release notes and removed release notes: no Indicates if PR should not be in release notes labels May 31, 2023

erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023

yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Audit Logging] End2end test for audit logging in authorization policy #33196

[Audit Logging] End2end test for audit logging in authorization policy #33196

rockspore commented May 19, 2023 •

edited

gtcooke94 left a comment

rockspore commented May 19, 2023

gtcooke94 left a comment •

edited

erm-g left a comment

erm-g left a comment

rockspore commented May 24, 2023

erm-g commented May 24, 2023

markdroth left a comment

markdroth May 24, 2023

rockspore May 24, 2023

markdroth May 24, 2023

rockspore May 24, 2023

markdroth May 24, 2023

rockspore May 24, 2023

markdroth May 24, 2023

rockspore May 24, 2023

markdroth May 24, 2023

rockspore May 24, 2023

rockspore left a comment

rockspore May 24, 2023

rockspore May 24, 2023

rockspore May 24, 2023

rockspore May 24, 2023

rockspore May 24, 2023

[Audit Logging] End2end test for audit logging in authorization policy #33196

[Audit Logging] End2end test for audit logging in authorization policy #33196

Conversation

rockspore commented May 19, 2023 • edited

gtcooke94 left a comment

Choose a reason for hiding this comment

rockspore commented May 19, 2023

gtcooke94 left a comment • edited

Choose a reason for hiding this comment

erm-g left a comment

Choose a reason for hiding this comment

erm-g left a comment

Choose a reason for hiding this comment

rockspore commented May 24, 2023

erm-g commented May 24, 2023

markdroth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockspore left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rockspore commented May 19, 2023 •

edited

gtcooke94 left a comment •

edited