-
Notifications
You must be signed in to change notification settings - Fork 63
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
set auth alias custom metadata to service account annotations #226
set auth alias custom metadata to service account annotations #226
Conversation
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.
Looking good! Not a complete review, but I left a couple of minor WIP thoughts
service_account_getter.go
Outdated
|
||
// Use the configured TokenReviewer JWT as the bearer | ||
if w.config.TokenReviewerJWT == "" { | ||
return nil, errors.New("service account lookup failed: TokenReviewer JWT needs to be configured to retrieve service accounts") |
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.
Is there any reason we can't use the client's own token like we do for other API requests? It's not my favourite feature, but I would favour consistency and shared code to achieve that if possible.
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.
Generally LGTM - I think there is one major issue with backwards compatibility, but the remaining comments are minor suggestions/questions
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 test looks great! I left a few nits and style comments, but I think there is 1 bug that is worth addressing.
backend.go
Outdated
// Note Vault client or the token reviewer service account needs | ||
// to be configured with the permission to get service account | ||
// annotations from Kubernetes API | ||
useAnnotationsAsAliasMetadata bool |
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.
I think this should be a member of kubeConfig
instead. The lifecycle of fields in kubeAuthBackend
matches the lifecycle of the plugin process, but this value is tied to the most recent write to the config endpoint instead.
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.
Thank you for pointing this out!
integrationtest/integration_test.go
Outdated
t.Fatalf("Expected to enable kv secrets engine but got: %v", err) | ||
} | ||
|
||
cleanup := func() { |
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.
I think this can probably be simplified with t.Cleanup too?
Co-authored-by: Tom Proctor <[email protected]>
Co-authored-by: Tom Proctor <[email protected]>
Co-authored-by: Tom Proctor <[email protected]>
Co-authored-by: Tom Proctor <[email protected]>
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! 🎉
Co-authored-by: Tom Proctor <[email protected]>
… github.com:hashicorp/vault-plugin-auth-kubernetes into VAULT-1665-add-custom-metadata-from-k8s-annotations
👋🏽 Just wanted to point out for future readers: the |
Overview
This PR sets auth alias custom metadata to service account annotations. Users can create policy rules that use custom entity/alias metadata and have this metadata be supplied from Kubernetes annotations on the ServiceAccount.
Design of Change
Extra permission configuration for installations is required to allow to get annotations from Kubernetes. A new
use_annotations_as_alias_metadata
config option is added for users to explicitly opt-in to ensure existing installations still work if this option is opted out.Only annotations with the prefix
vault.hashicorp.com/alias-metadata-
of the client token'sassociated service account will be added to the auth alias's metadata upon a login request. For example, if an annotation "vault.hashicorp.com/alias-metadata-foo" is configured, "foo" with its value will be added.
NOTE that Vault will need permission to read service accounts from the Kubernetes API.
Example:
Related Issues/Pull Requests
Contributor Checklist
My Docs PR vault #24941 Link