- Notifications
You must be signed in to change notification settings - Fork4.5k
authz: Rbac engine audit logging#6225
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
Add punctuation to commentsCo-authored-by: Arvind Bright <arvind.bright100@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM!
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM!
Uh oh!
There was an error while loading.Please reload this page.
internal/xds/rbac/converter.go Outdated
| func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig) (audit.Logger, error) { | ||
| if loggerConfig.AuditLogger.TypedConfig == nil { | ||
| return nil, fmt.Errorf("AuditLogger TypedConfig cannot be nil") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does this get plumbed back to the user somehow? I traced it back a couple levels and it's never augmented or changed. I don't think this would be a useful error message for a user. Similar with all the other errors in this function / PR - please make sure that the failure modes are understood and that the errors will make sense where they appear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
It ends up bubbling up through theNewEngine func
Looks like that is called inauthz.NewStatic(authzPolicy string) by the user or via xds config plumbing.
I think these errors make sense in the realm of the user getting them back from calling `authz.NewStatic(.), what do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The authz policy is a string and you're giving them errors about nil pointers. That doesn't seem right."missing required field: TypedConfig" or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
That makes sense, I'll reword the errors to match that. Will comment back once I push that commit
gtcooke94May 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Reworded some, I think the others left still make sense? PTAL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
When the user callsNewStatic, are they ending up here? I thought they wouldn't be using the xds protos in that case, andTypedConfig wouldn't even be a thing for them?
gtcooke94May 16, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes - the way the golang was already structured it all pushes into these protos in the end - therbac_engine.go file is ininternal/xds and was already written to use RBAC protos directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Does that mean this error won't ever go back out to users ofNewStatic then, because we ensure the structure is right when we come through that path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In theNewStatic case it will have gone throughtranslatePolicy inrbac_translator.go.
This converts from a string policy to the rbac proto.
Then that is passed along toNewChainEngine.
So checks that happen inrbac_translator.go will be guaranteed to be good by the time this path gets toNewChainEngine.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is one more change I just realized.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Got more comments on a few subtle things that I didn't notice earlier. Sorry for any inconvenience.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
authz/rbac_translator.go Outdated
| @@ -304,12 +304,9 @@ func (options *auditLoggingOptions) toProtos() (allow *v3rbacpb.RBAC_AuditLoggin | |||
| for i := range options.AuditLoggers { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
for _, config := range options.AuditLoggers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Changed, I think this was a holdover from when this was structured differently.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Looks like this is why?
range var config copies lock: google.golang.org/grpc/authz.auditLogger contains google.golang.org/protobuf/types/known/structpb.Struct contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex
Should this be[]*auditLogger then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Oh yes, I remember now that is why I had done that - changed to[]*auditLogger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Two minor comments. Otherwise LGTM. Thanks!
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
This PR adds the functionality to actually do audit logging in
rbac_engine.goand associated tests for that functionality.In addition, the audit logging spec (https://github.com/grpc/proposal/pull/346/files) requires that we have the overall policy name. It's not currently passed through the RBAC to the engine level, so we need to pipe it around. So, this PR contains some basic plumbing for that value.
In XDS, this piping will be complex and need to be its own PR. For now, XDS has a TODO and passes an empty string. This is not a loss or change of functionality since currently it is not passed anyways.
RELEASE NOTES: none