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

Role option to set alias name to Service Account's "namespace/name" instead of uid #103

Closed
wants to merge 7 commits into from

Conversation

damonearp
Copy link
Contributor

Overview

Adds a Role Configuration Option to allow using the serviceAccount.namespace()/serviceAccount.name() to be the alias name instead of the serviceAccount.uid().

This option allows tying Entities to Kubernetes service accounts, before they exists, and even after they been deleted and recreated. In a practical sense it allows deploying testing/staging namespaces and allowing vault to manage the credentials without needing to update vault with new service account uids every time a deployment occurs.

The UI user's experience is also improved as the Entities>Alias list is now human readable. The user now knows what service account the alias is for without having to click and inspect the metadata of every single alias. Furthermore as namespaces get deleted and recreated the list of Entities and Aliases does not continue to grow.

Design of Change

The path_role.go implementation was updated to include this new option in the UI as well as visible in the vault read|write auth/kubernetes/role/$ROLE commands. And path_login.go was updated to check if the new option is true and if so use the namespace and service account name separated by a / instead of the uid.

The / was chosen over the existing - used in the DisplayName as the former is an invalid character in a Kubernetes namespace. A . could work as well.

The name of the option HumanReadableAlias is surely not the best, any feedback on a better name is welcome. I've noticed in other parts of this plugin disable_ seems to be the preferred flag/boolean prefix. Perhaps DisableUidAlias?

Finally enabling this option can open up security concerns, as a service account being deleted and recreated will correspond to the same alias and entity. Perhaps a warning should be added outlining this fact? Any suggestion on how to add such a warning would be appreciated.

Related Issues/Pull Requests

#39 Using - as the separator for DisplayName

Contributor Checklist

[ x ] Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
This is a plugin specific option. As such it isn't clear more documentation is needed.
[ ] Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
Change has been tested by using vault write sys/plugins/catalog/auth/kubernetes sha256=$SHA256SUM command=vault-plugin-auth-kubernetes to overwrite the builtin version on vault 1.6.2. Please indicate what information is required to show that it has been tested.
[ x ] Backwards compatible
This change defaults to the current behavior and as such doesn't affect any existing infrastructure.

@hashicorp-cla
Copy link

hashicorp-cla commented Feb 11, 2021

CLA assistant check
All committers have signed the CLA.

@Nick-Triller
Copy link

I would be happy about this feature. Some of our Kubernetes users recreate service accounts often which leads to new ServiceAccount UIDs. Each UID leads a new entity and uses a license. Our Vault Enterprise license count is heavily skewed because of this.

@EvertonSA
Copy link

are we going to merge this?

@benashz benashz self-requested a review August 26, 2021 17:50
@benashz benashz self-assigned this Aug 26, 2021
@benashz
Copy link
Contributor

benashz commented Sep 3, 2021

@damonearp we have continued your PR in #110.

Thank you for your contribution.

@benashz benashz closed this Sep 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants