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

gtcooke94 · 2023-04-14T15:53:04Z

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 · 2023-04-14T16:24:57Z

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.

rockspore · 2023-04-14T16:29:26Z

authz/rbac_translator_test.go

+					]
+				}
+			}`,
+			wantErr: `failed to unmarshal policy`,


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

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

authz/rbac_translator_test.go

rockspore · 2023-04-14T20:29:18Z

authz/rbac_translator_test.go

+				}],
+				"audit_logging_options": {
+					"audit_condition": "ON_DENY_AND_ALLOW",
+					"audit_loggers": [


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.

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

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.

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

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.

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.

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.

So to clarify

Audit condition but no logger - fine

Logger but empty audit condition - fine

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)

Yes, the above cases are correct.

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

authz/rbac_translator_test.go

rockspore · 2023-04-14T20:32:13Z

authz/rbac_translator_test.go

+					]
+				}
+			}`,
+			wantErr: `AuditLogger config present but missing AuditCondition`,


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.

Discussed in other thread, keeping open to track

rockspore · 2023-04-14T20:35:58Z

authz/rbac_translator.go

@@ -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 {


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

Discussed in other thread, keeping open to track

authz/rbac_translator.go

rockspore · 2023-04-14T20:41:31Z

authz/rbac_translator.go

@@ -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)


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.

I don't quite get what you wrote here and how it applies to this line, can you go into more detail?

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.

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?

That's correct.

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

rockspore

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

gtcooke94 · 2023-04-19T16:14:49Z

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

dfawley · 2023-04-19T18:38:58Z

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

authz/rbac_translator.go

easwars · 2023-04-19T21:11:37Z

examples/go.mod

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.

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

authz/rbac_translator_test.go

easwars

LGTM modulo a few minor nits.

authz/rbac_translator.go

easwars · 2023-04-25T16:55:17Z

authz/rbac_translator.go

+		}
+		customConfig, err := anypb.New(config.Config)
+		if err != nil {
+			return nil, nil, fmt.Errorf("error parsing custom audit logger config: %v", err)


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

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

Add conversion of json to RBAC Audit Logging config

8b9f59a

gtcooke94 requested a review from rockspore April 14, 2023 15:53

rockspore reviewed Apr 14, 2023

View reviewed changes

authz/rbac_translator_test.go Show resolved Hide resolved

gtcooke94 added 5 commits April 14, 2023 19:01

Swap to passing around references instead of copied values

56ab1cd

Change v3 to v3corepb

0f3b6e7

go mod tidy compat=1.17

6bff58b

go mod tidy in examples

9b3ab47

mod tidy

bbafb89

rockspore reviewed Apr 14, 2023

View reviewed changes

rockspore requested changes Apr 14, 2023

View reviewed changes

gtcooke94 added 7 commits April 14, 2023 21:16

go mod tidy compat=1.17

5dad66e

more tidy

73b1390

replace panic with t.fatal

27cfe85

return nil when error

c8751d3

lowercase start of error message

441fb5c

remove redundant change

2326e9f

Fix test logging hard-coding problem

abcb93f

gtcooke94 added Type: Feature New features or improvements in behavior Type: Security A bug or other problem affecting security labels Apr 17, 2023

rockspore reviewed Apr 18, 2023

View reviewed changes

authz/rbac_translator.go Outdated Show resolved Hide resolved

arvindbr8 assigned gtcooke94 Apr 18, 2023

gtcooke94 added 2 commits April 18, 2023 18:46

Changed behavior of missing configs per PR discussion

3f57e69

Change parser to use map from generated proto go file

b7863ee

arvindbr8 added this to the 1.56 Release milestone Apr 18, 2023

rockspore reviewed Apr 18, 2023

View reviewed changes

authz/rbac_translator.go Outdated Show resolved Hide resolved

gtcooke94 added 3 commits April 18, 2023 21:23

ALLOW and DENY filter should get separate audit logger config

48a875e

Small change remove and else

edf40f1

Construct and allow and denyproto from the beginning

551ced9

gtcooke94 added 3 commits April 19, 2023 14:53

Merge branch 'master' into AuditLoggingRBACTranslator

1718c45

compat 1.17 in examples

dad7293

more 1.17 compat

7b9e96e

rockspore approved these changes Apr 19, 2023

View reviewed changes

gtcooke94 requested a review from dfawley April 19, 2023 16:14

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

dfawley assigned easwars Apr 19, 2023

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

easwars reviewed Apr 19, 2023

View reviewed changes

gtcooke94 added 2 commits April 20, 2023 16:32

Address PR comments

1f4c0c0

Swap so we have to hav an empty custom config, no nil allowed

ec7b6bd

arvindbr8 unassigned gtcooke94 Apr 21, 2023

arvindbr8 requested a review from easwars April 21, 2023 18:53

easwars approved these changes Apr 25, 2023

View reviewed changes

easwars assigned gtcooke94 and unassigned easwars Apr 25, 2023

gtcooke94 added 2 commits April 25, 2023 17:18

Change variable names

392b6b1

Remove newline

601be9a

gtcooke94 merged commit e853dbf into grpc:master Apr 26, 2023
11 checks passed

gtcooke94 mentioned this pull request Apr 27, 2023

authz: Rbac engine audit logging #6225

Merged

github-actions bot locked as resolved and limited conversation to collaborators Oct 24, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gtcooke94 commented Apr 14, 2023 •

edited by easwars

rockspore commented Apr 14, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 14, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 14, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 14, 2023

rockspore Apr 14, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 17, 2023

gtcooke94 Apr 17, 2023 •

edited

rockspore Apr 18, 2023

gtcooke94 Apr 18, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 17, 2023

gtcooke94 Apr 18, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 17, 2023

gtcooke94 Apr 18, 2023

rockspore Apr 14, 2023

gtcooke94 Apr 17, 2023

rockspore Apr 18, 2023

gtcooke94 Apr 18, 2023 •

edited

rockspore Apr 18, 2023

gtcooke94 Apr 18, 2023

rockspore left a comment

gtcooke94 commented Apr 19, 2023

dfawley commented Apr 19, 2023

easwars Apr 19, 2023

gtcooke94 Apr 20, 2023

easwars left a comment

easwars Apr 25, 2023

gtcooke94 Apr 25, 2023

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

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

Conversation

gtcooke94 commented Apr 14, 2023 • edited by easwars

rockspore commented Apr 14, 2023

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

gtcooke94 Apr 17, 2023 • edited

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

Choose a reason for hiding this comment

gtcooke94 Apr 18, 2023 • edited

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

gtcooke94 commented Apr 19, 2023

dfawley commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

easwars left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gtcooke94 commented Apr 14, 2023 •

edited by easwars

gtcooke94 Apr 17, 2023 •

edited

gtcooke94 Apr 18, 2023 •

edited