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

add hierarchical path handling to static role endpoints #102

Merged
merged 9 commits into from
May 8, 2024

Conversation

fairclothjm
Copy link
Contributor

@fairclothjm fairclothjm commented May 3, 2024

This PR adds hierarchical path handling to the following APIs:

This allows creating a static role name with an arbitrary number of forward slashes. For example,

$ vault write ldap/static-role/org/platform/dev \
    username="user3" \
    rotation_period="5m"

Where org/platform/dev is the role name. Creds can be read and rotated using the same role name and the respective API's. For example,

$ vault read ldap/static-cred/org/secure
Key                    Value
---                    -----
dn                     n/a
last_password          a3sQ6OkmXKt2dtx22kAt36YLkkxLsg4RmhMZCLYCBCbvvv67ILROaOokdCaGPEAE
last_vault_rotation    2024-05-03T16:39:27.174164-05:00
password               ECf7ZoxfDxGuJEYZrzgzTffSIDI4tx5TojBR9wuEGp8bqUXbl4Kr9eAgPjmizcvg
rotation_period        5m
ttl                    4m58s
username               user2

$ vault write -f ldap/rotate-role/org/secure

Most importantly, this allows us to perform LIST operations to query the available roles. For example,

$ vault list ldap/static-role/org/
Keys
----
platform/
secure

$ vault list ldap/static-role/org/platform
Keys
----
dev

@fairclothjm fairclothjm requested a review from a team May 3, 2024 21:43
Copy link

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

PR is looking good! Had a couple initial questions

path_static_roles.go Outdated Show resolved Hide resolved
path_static_roles.go Show resolved Hide resolved
path_static_roles_test.go Show resolved Hide resolved
Copy link
Contributor

@benashz benashz left a comment

Choose a reason for hiding this comment

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

Looking good. I had a few minor suggestions, questions, and nits that you might want to address.

path_static_roles.go Outdated Show resolved Hide resolved
path_static_roles.go Show resolved Hide resolved
path_static_roles.go Outdated Show resolved Hide resolved
path_static_roles.go Outdated Show resolved Hide resolved
path_static_roles.go Outdated Show resolved Hide resolved
path_static_creds_test.go Outdated Show resolved Hide resolved
path_static_roles.go Outdated Show resolved Hide resolved
path_static_roles_test.go Show resolved Hide resolved
path_static_roles_test.go Show resolved Hide resolved
path_static_roles_test.go Show resolved Hide resolved
@fairclothjm
Copy link
Contributor Author

@benashz @vinay-gopalan I have addressed the bulk of your comments. Any remaining test improvements I will do once we get past feature freeze.

Copy link

@vinay-gopalan vinay-gopalan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for all the clarifying comments, I am fine with additional test improvements being handled after feature freeze

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

4 participants