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

[vtadmin] [experimental] add per-api RBAC implementation #8515

Merged
merged 6 commits into from
Jul 21, 2021

Conversation

ajm188
Copy link
Contributor

@ajm188 ajm188 commented Jul 21, 2021

Description

This PR introduces an experimental feature to vtadmin, namely per-api scoped role-based access control. See the package documentation in rbac.go for more details around the principles behind the design. I plan to continue to iterate on this within our deployment, and I'll follow up with more unit tests and documentation as I do so.

Related Issue(s)

Checklist

  • Tests were added or are not required
  • Documentation was added or is not required

Deployment Notes

Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Signed-off-by: Andrew Mason <amason@slack-corp.com>
Copy link
Contributor

@doeg doeg left a comment

Choose a reason for hiding this comment

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

Given that we already went over the feedback I had out-of-band + this works great internally, it looks good to me!

authz = opts.RBAC.GetAuthorizer()

if authn != nil {
opts.GRPCOpts.StreamInterceptors = append(opts.GRPCOpts.StreamInterceptors, rbac.AuthenticationStreamInterceptor(authn))
Copy link
Contributor

Choose a reason for hiding this comment

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

I like dis. So clean.

// stream and request contexts, respectively.
//
// Returning an error from the authenticator will fail the request. To
// denote an authenticated request, return (nil, nil) instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Such nice docs always ❤️

*/

/*
Package rbac provides role-based access control for vtadmin API endpoints.
Copy link
Contributor

Choose a reason for hiding this comment

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

chef kiss

@ajm188 ajm188 merged commit 792b091 into vitessio:main Jul 21, 2021
@ajm188 ajm188 deleted the am_vtadmin_rbac branch July 21, 2021 22:53
// AuthenticateHTTP returns an actor given an http.Request.
//
// Returning an error from the authenticator will fail the request. To
// denote an authenticated request, return (nil, nil) instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

"To denote an authenticated request without an actor, return (nil, nil) instead" might be more clear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ohhh this is a typo. it should be "to denote an unauthenticated request, return (nil, nil)"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants