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.

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

[Audit Logging] Authz policy support for audit logging #32944

Merged
merged 77 commits into from May 2, 2023

Conversation

rockspore
Copy link
Contributor

Add audit condition and audit logger config into grpc_core::Rbac. Support translation of audit logging options from authz policy to it.

Audit logging options in authz policy looks like:

{
  "audit_logging_options": {
    "audit_condition": "ON_DENY",
    "audit_loggers": [
      {
        "name": "logger",
        "config": {},
        "is_optional": false
      }
    ]
  }
}

which is consistent with what's in the xDS RBAC proto but a little flattened.

@rockspore
Copy link
Contributor Author

I really like the test coverage but have few more suggesions:

  1. add tests for rbac_policy.cc , especially for Rbac::ToString() - we added extra fields there and I think it's worth testing the resulting strings
  2. for rbac_translator_test.cc - let's add a test for an empty audit_logger_options ({}) and for the situation when it's a customer's logger and we don't know if the config has a correct value or not (like "config":{"bad_value_but_we_can't_check_it":"ok"}
  3. are we planning to create e2e tests where we can check the real output later?
  1. In one of my tests, I added a prefix checking of the ToString(). I don't want to add coverage for the existing behavior of ToString(), which is not directly called anywhere, and possibly only used for logging purpose somewhere (if any). I suppose there was a reason why tests didn't exist in the first place for it.
  2. Added a test for empty logging options. Also added coverage by checking Config.ToString() which I have modified to store the config json.
  3. e2e test is definitely required. We will do that once the last PR to complete the feature is done.

Copy link
Contributor

@gtcooke94 gtcooke94 left a comment

Choose a reason for hiding this comment

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

This LGTM

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

This looks good overall! Comments are mostly minor.

src/core/ext/filters/rbac/rbac_service_config_parser.cc Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_policy.h Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_policy.h Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_policy.h Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_policy.cc Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_translator.cc Outdated Show resolved Hide resolved
src/core/lib/security/authorization/rbac_translator.cc Outdated Show resolved Hide resolved
test/core/security/rbac_translator_test.cc Outdated Show resolved Hide resolved
@rockspore rockspore changed the title [Audit Logging] Authz policy support for audit logging [Audit Logging] Authz policy support for audit logging. May 2, 2023
@rockspore rockspore requested a review from markdroth May 2, 2023 17:24
Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great! Feel free to merge after addressing the remaining comments.

Rbac::Rbac(Rbac::Action action, std::map<std::string, Policy> policies,
absl::string_view name)
: action(action), policies(std::move(policies)), name(name) {}
Rbac::Rbac(absl::string_view name, Rbac::Action action,
Copy link
Member

Choose a reason for hiding this comment

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

Might as well take the name parameter as std::string instead of absl::string_view, since we're making a copy anyway. That way, if a caller happens to have a temp string that they don't need to keep, they can pass it in with std::move() and avoid the copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the tip!

@@ -394,7 +392,8 @@ ParseAuditLogger(const Json& json, size_t pos) {
absl::StrFormat("\"audit_loggers[%d].name\" is not a string.", pos));
}
absl::string_view name = it->second.string();
Json config = Json::Object();
// The config defaults to an empty object.
Json config = Json::FromObject(Json::Object());
Copy link
Member

Choose a reason for hiding this comment

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

Can just say Json::FromObject({}). The compiler is smart enough to know the type of the {}.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed.

@rockspore rockspore changed the title [Audit Logging] Authz policy support for audit logging. [Audit Logging] Authz policy support for audit logging May 2, 2023
@rockspore rockspore merged commit 3541ef5 into grpc:master May 2, 2023
63 of 64 checks passed
@rockspore rockspore deleted the rbac_policy branch May 2, 2023 22:51
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label May 3, 2023
paulosjca pushed a commit to paulosjca/grpc that referenced this pull request May 4, 2023
Add audit condition and audit logger config into `grpc_core::Rbac`.
Support translation of audit logging options from authz policy to it.

Audit logging options in authz policy looks like:
```json
{
  "audit_logging_options": {
    "audit_condition": "ON_DENY",
    "audit_loggers": [
      {
        "name": "logger",
        "config": {},
        "is_optional": false
      }
    ]
  }
}
```
which is consistent with what's in the xDS RBAC proto but a little
flattened.

---------

Co-authored-by: rockspore <rockspore@users.noreply.github.com>
wanlin31 pushed a commit that referenced this pull request May 18, 2023
Add audit condition and audit logger config into `grpc_core::Rbac`.
Support translation of audit logging options from authz policy to it.

Audit logging options in authz policy looks like:
```json
{
  "audit_logging_options": {
    "audit_condition": "ON_DENY",
    "audit_loggers": [
      {
        "name": "logger",
        "config": {},
        "is_optional": false
      }
    ]
  }
}
```
which is consistent with what's in the xDS RBAC proto but a little
flattened.

---------

Co-authored-by: rockspore <rockspore@users.noreply.github.com>
@yijiem 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 erm-g removed the release notes: yes Indicates if PR needs to be in release notes label Jun 12, 2023
@yijiem yijiem added the release notes: no Indicates if PR should not be in release notes label Jun 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/low imported Specifies if the PR has been imported to the internal repository lang/core per-call-memory/neutral per-channel-memory/neutral release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants