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

authz: add conversion of json to RBAC Audit Logging config #6192

Merged
merged 25 commits into from Apr 26, 2023

Conversation

gtcooke94
Copy link
Contributor

@gtcooke94 gtcooke94 commented Apr 14, 2023

Add functionality in RBAC translator to convert the new Audit Logging configs in envoy from JSON to their associated envoy proto structs. Overall a pretty simple addition, the most complicated section being that we allow a dynamic/unknown structure in the audit logging config, so converting that around was a little bit messy (particularly in the tests). Ended up adding the convenience function anyPbHelper so that I could create them in struct literals for the tests.

Also had to add the new hash of envoy with the new proto structs to go.mod which created a big change in go.sum.

See gRFC A59 for more details.

RELEASE NOTES:

  • authz: add conversion of json to RBAC Audit Logging config

@rockspore
Copy link
Contributor

Seems like many tests are failing. Please fix.

We need to make sure incorrect logger configs can be rejected before going too far. In this authorization policy case, it may be fine to validate the config with the logger builder parser later. But in xDS, that stage (when RBACs are already generated, meaning the HTTP filters are possibly already ACKed) might be too late.

]
}
}`,
wantErr: `failed to unmarshal policy`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Before anything else, I just wanted to point out I am generally not happy with this kind of error messages as a user, because it's barely actionable. And I know existing test cases have this as well because it's from the json decoder. And unless we reinvent that decoder somewhat, we will have to live with this.

Anyway, I think there is no action item here. Just wanted to complain about it xD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The errors are a at least a little more descriptive, the test is just doing prefix checking on the messages

}],
"audit_logging_options": {
"audit_condition": "ON_DENY_AND_ALLOW",
"audit_loggers": [
Copy link
Contributor

Choose a reason for hiding this comment

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

When the test case doesn't need this field, I'd recommend omit it for readability. It would also confirm that the absence of loggers is fine.

Same applies below when you test bad audit conditions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is the absence of loggers fine? I assumed no - what does it mean to have an audit condition but no audit logger to log that condition?
I figured both were required

Copy link
Contributor

Choose a reason for hiding this comment

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

I replied in other comments. We shouldn't require any of the fields to be present. No loggers with audit condition means nothing happens but there is nothing wrong with the config. Likewise for logger with no condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But if there is no logger then the audit condition also means nothing? From a user perspective I think that would be confusing - what's the use case for allowing it? If there is none, I think allowing an audit condition with no logger is just silencing a mistake/misconfiguration

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd be leaning toward disallowing these if audit logging options is more of a separate config from the authz policy. Given that it's part of the policy, I'd prefer to keep the authorization going when the audit logging does nothing for these types of configs. If we failed here, it would not update the authorization engine at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

BTW, this is consistent with how the xDS APIs were defined. We won't NACK the RBAC there under the same situation and audit condition is an enum there so it will always default to none. At least the xDS part was explicitly discussed before and it's good to stay consistent here I think.

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'll implement it this way to be consistent, but I think we're setting up users for silent mistakes

No loggers with audit condition means nothing happens but there is nothing wrong with the config. Likewise for logger with no condition.

To me, if I explicitly write a config that does nothing because I missed another part of the config, that's a mistake that I'd rather not silently hide. Allowing it makes for a very difficult mistake for a user to find to find with no meaningful upside since nothing happens if only one is passed. I suppose allowing the authz policy to pass is a sort-of upside, but I still think we're allowing a broken config.

Copy link
Contributor Author

@gtcooke94 gtcooke94 Apr 17, 2023

Choose a reason for hiding this comment

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

So to clarify

  1. Audit condition but no logger - fine
  2. Logger but empty audit condition - fine
  3. Logger and invalid audit condition (not an empty string, but something that doesn't match the enum) - fail (or should this pass as NONE with the default)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the above cases are correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I've changed the code and test cases to follow this behavior, PTAL

authz/rbac_translator_test.go Outdated Show resolved Hide resolved
]
}
}`,
wantErr: `AuditLogger config present but missing AuditCondition`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we need to reject this. Audit condition should fall back to none and it simply means loggers will not be invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in other thread, keeping open to track

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

@@ -266,6 +281,48 @@ func parseRules(rules []rule, prefixName string) (map[string]*v3rbacpb.Policy, e
return policies, nil
}

func parseAuditLoggingOptions(options auditLoggingOptions) (*v3rbacpb.RBAC_AuditLoggingOptions, error) {
optionsRbac := v3rbacpb.RBAC_AuditLoggingOptions{}
if options.AuditCondition == "" && len(options.AuditLoggers) == 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Audit condition should default to none if not present. No loggers should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed in other thread, keeping open to track

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PTAL

authz/rbac_translator.go Outdated Show resolved Hide resolved
authz/rbac_translator.go Show resolved Hide resolved
authz/rbac_translator.go Outdated Show resolved Hide resolved
@@ -283,22 +340,28 @@ func translatePolicy(policyStr string) ([]*v3rbacpb.RBAC, error) {
if len(policy.AllowRules) == 0 {
return nil, fmt.Errorf(`"allow_rules" is not present`)
}
auditLoggers, err := parseAuditLoggingOptions(policy.AuditLoggingOptions)
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a mapping from this audit condition to the conditions of deny and allow RBACs. Because we only want the same RPC to be logged at most once.

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 don't quite get what you wrote here and how it applies to this line, can you go into more detail?

Copy link
Contributor

Choose a reason for hiding this comment

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

Basically, you cannot use the audit condition from the policy in both RBACs, because we need to make sure one RPC can at most be logged one time.

There is a table in the gRFC. As an example, if we want to audit on allow, then the deny RBAC should have no audit condition (I just realized right now that in fact, it doesn't even need to hold any loggers in this case), and the allow RBAC will have ON_ALLOW. The evaluation order is always deny -> allow, and short-circuited if applicable.

Certainly you don't have to change this line to fix it. But this was where I realized the problem.

Copy link
Contributor Author

@gtcooke94 gtcooke94 Apr 18, 2023

Choose a reason for hiding this comment

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

So I just need some additional logic here that constructs two separate audit logging configs to add to each RBAC with different conditions per the table and the input AuditCondition?

Copy link
Contributor

Choose a reason for hiding this comment

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

That's correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I've changed to this, it was a sizeable code change in the tests because I made every test have both an ALLOW and DENY filter to be able to test that they are mapped correctly

authz/rbac_translator.go Outdated Show resolved Hide resolved
authz/rbac_translator.go Outdated Show resolved Hide resolved
@arvindbr8 arvindbr8 added this to the 1.56 Release milestone Apr 18, 2023
Copy link
Contributor

@rockspore rockspore left a comment

Choose a reason for hiding this comment

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

As mentioned offline, while it's fine to leave the logger config parsing and validation by the logger builder to the chained engine construction. xDS impl will have to parse the configs before that stage to decide whether to NACK the RBAC filter. It means in the xDS flow the configs would be parsed twice. This is not necessarily a problem but worth thinking about and perhaps taking care of here.

authz/rbac_translator.go Outdated Show resolved Hide resolved
@gtcooke94 gtcooke94 requested a review from dfawley April 19, 2023 16:14
@gtcooke94
Copy link
Contributor Author

@dfawley can you or someone else on the go team take a look at this?

@dfawley dfawley requested review from easwars and removed request for dfawley April 19, 2023 18:38
@dfawley
Copy link
Contributor

dfawley commented Apr 19, 2023

@easwars do you have any time to review this for style?

@easwars easwars changed the title Add conversion of json to RBAC Audit Logging config authz: add conversion of json to RBAC Audit Logging config Apr 19, 2023
authz/rbac_translator.go Outdated Show resolved Hide resolved
examples/go.mod Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

There was recent PR which updated all go mod and sum files after the 1.55 branch cut. Please ensure that you rebase to master, and ensure that you don't overwrite any changes made there. Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it was causing merge conflicts for me - I did a merge of master instead of a rebase to keep the history of the PR intact. Is there an easy way to make sure I don't overwrite any of those changes, or can we trust the merge?

authz/rbac_translator.go Outdated Show resolved Hide resolved
authz/rbac_translator.go Show resolved Hide resolved
authz/rbac_translator.go Outdated Show resolved Hide resolved
authz/rbac_translator_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@easwars easwars left a comment

Choose a reason for hiding this comment

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

LGTM modulo a few minor nits.

authz/rbac_translator.go Outdated Show resolved Hide resolved
}
customConfig, err := anypb.New(config.Config)
if err != nil {
return nil, nil, fmt.Errorf("error parsing custom audit logger config: %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: does it make sense to include the erroring config in the error string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The custom config is a user-defined json object so it could be very large, would we want to write it out in the error message?

authz/rbac_translator.go Outdated Show resolved Hide resolved
authz/rbac_translator.go Show resolved Hide resolved
@easwars easwars assigned gtcooke94 and unassigned easwars Apr 25, 2023
@gtcooke94 gtcooke94 merged commit e853dbf into grpc:master Apr 26, 2023
11 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants