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: Rbac engine audit logging #6225

Merged
merged 106 commits into from
May 17, 2023
Merged

Conversation

gtcooke94
Copy link
Contributor

@gtcooke94 gtcooke94 commented Apr 26, 2023

This PR adds the functionality to actually do audit logging in rbac_engine.go and 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

Add punctuation to comments

Co-authored-by: Arvind Bright <arvind.bright100@gmail.com>
@arvindbr8 arvindbr8 added this to the 1.56 Release milestone May 10, 2023
@arvindbr8 arvindbr8 removed the request for review from easwars May 11, 2023 17:18
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.

LGTM!

internal/xds/rbac/converter_test.go Outdated Show resolved Hide resolved
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.

LGTM!

internal/xds/rbac/converter.go Outdated Show resolved Hide resolved

func buildLogger(loggerConfig *v3rbacpb.RBAC_AuditLoggingOptions_AuditLoggerConfig) (audit.Logger, error) {
if loggerConfig.AuditLogger.TypedConfig == nil {
return nil, fmt.Errorf("AuditLogger TypedConfig cannot be nil")
Copy link
Member

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.

Copy link
Contributor Author

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 the NewEngine func
Looks like that is called in authz.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?

Copy link
Member

@dfawley dfawley May 15, 2023

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@gtcooke94 gtcooke94 May 16, 2023

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

Copy link
Member

Choose a reason for hiding this comment

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

When the user calls NewStatic, are they ending up here? I thought they wouldn't be using the xds protos in that case, and TypedConfig wouldn't even be a thing for them?

Copy link
Contributor Author

@gtcooke94 gtcooke94 May 16, 2023

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 - the rbac_engine.go file is in internal/xds and was already written to use RBAC protos directly

Copy link
Member

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 of NewStatic then, because we ensure the structure is right when we come through that path?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the NewStatic case it will have gone through translatePolicy in rbac_translator.go.
This converts from a string policy to the rbac proto.
Then that is passed along to NewChainEngine.

So checks that happen in rbac_translator.go will be guaranteed to be good by the time this path gets to NewChainEngine.

internal/xds/rbac/converter.go Outdated Show resolved Hide resolved
internal/xds/rbac/converter.go Outdated Show resolved Hide resolved
internal/xds/rbac/rbac_engine.go Show resolved Hide resolved
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.

There is one more change I just realized.

internal/xds/rbac/rbac_engine.go Outdated Show resolved Hide resolved
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.

Got more comments on a few subtle things that I didn't notice earlier. Sorry for any inconvenience.

authz/rbac_translator.go Outdated Show resolved Hide resolved
internal/xds/rbac/converter.go Outdated Show resolved Hide resolved
@@ -304,12 +304,9 @@ func (options *auditLoggingOptions) toProtos() (allow *v3rbacpb.RBAC_AuditLoggin

for i := range options.AuditLoggers {
Copy link
Member

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?

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, I think this was a holdover from when this was structured differently.

Copy link
Member

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?

Copy link
Contributor Author

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

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.

Two minor comments. Otherwise LGTM. Thanks!

authz/rbac_translator.go Show resolved Hide resolved
internal/xds/rbac/rbac_engine.go Show resolved Hide resolved
@dfawley dfawley removed their assignment May 16, 2023
@gtcooke94 gtcooke94 merged commit 390c392 into grpc:master May 17, 2023
1 check passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Status: Requires Reporter Clarification Type: Security A bug or other problem affecting security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants