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

VAULT-9208: updating rotate role to handle case sensitivity #118

Merged
merged 4 commits into from
Oct 14, 2024

Conversation

tvo0813
Copy link
Contributor

@tvo0813 tvo0813 commented Oct 9, 2024

Overview

This update improves OpenLDAP by allowing role rotation without case sensitivity concerns, enabling users to rotate roles using any letter case.

Related Issues/Pull Requests

Contributor Checklist

  • Add relevant docs to upstream Vault repository, or sufficient reasoning why docs won’t be added yet
  • Add output for any tests not ran in CI to the PR description (eg, acceptance tests)
  • Backwards compatible

@tvo0813
Copy link
Contributor Author

tvo0813 commented Oct 10, 2024

All go tests passed and tested on Vault v1.18.0
image

@tvo0813 tvo0813 changed the title VAULT-9208: updating rotate role to not be case sensitive VAULT-9208: updating rotate role to handle case sensitivity Oct 11, 2024
@ltcarbonell ltcarbonell requested review from ltcarbonell, austingebauer and a team and removed request for austingebauer and a team October 11, 2024 17:51
Copy link

@ltcarbonell ltcarbonell left a comment

Choose a reason for hiding this comment

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

LGTM. I would like to see a 👍 from Eco before we merge it.

Copy link
Contributor

@fairclothjm fairclothjm left a comment

Choose a reason for hiding this comment

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

This feels like it could be a backwards incompatible change. What happens to roles that were created before this change? After we update the plugin, will auto rotation and manual rotation continue to work as expected? What about read requests?

@ryancragun
Copy link

I think a better approach would be to utilize fieldsForType() to set the rotate field type to match static role field type.

@fairclothjm
Copy link
Contributor

Good point at @ryancragun! I didn't realize the roles were using framework.TypeLowerCaseString which explains some of my confusion with this behavior. I think I am less concerned about this being backwards compatible now but it would be good to verify anyway.

@tvo0813
Copy link
Contributor Author

tvo0813 commented Oct 14, 2024

Updated to use fieldsForType() to set the rotate field type. Tested and was successfully able to rotate the roles

current:

tin.vo@tin vault-plugin-secrets-openldap % vault write ldap/config binddn=cn=admin,dc=hashicorp,dc=com bindpass=tin url=ldap://localhost:389
Success! Data written to: ldap/config
tin.vo@tin vault-plugin-secrets-openldap % v write ldap/static-role/tiNVo dn='cn=tin,ou=users,dc=hashicorp,dc=com' username='tin' rotation_period="24h"
Success! Data written to: ldap/static-role/tiNVo
tin.vo@tin vault-plugin-secrets-openldap % v list ldap/static-role
Keys
----
tinvo
tin.vo@tin vault-plugin-secrets-openldap % v read ldap/static-role/tinvo
Key                    Value
---                    -----
dn                     cn=tin,ou=users,dc=hashicorp,dc=com
last_vault_rotation    2024-10-14T11:25:00.21149-07:00
rotation_period        24h
username               tin
tin.vo@tin vault-plugin-secrets-openldap % v write -f ldap/rotate-role/tinvo
Success! Data written to: ldap/rotate-role/tinvo
tin.vo@tin vault-plugin-secrets-openldap % v write -f ldap/rotate-role/tiNVo
Error writing data to ldap/rotate-role/tiNVo: Error making API request.

URL: PUT http://127.0.0.1:8200/v1/ldap/rotate-role/tiNVo
Code: 400. Errors:

* role doesn't exist: tiNVo

after change:

tin.vo@tin vault-plugin-secrets-openldap % vault write vault-plugin-secrets-openldap/config binddn=cn=admin,dc=hashicorp,dc=com bindpass=tin url=ldap://localhost:389
Success! Data written to: vault-plugin-secrets-openldap/config
tin.vo@tin vault-plugin-secrets-openldap % v write vault-plugin-secrets-openldap/static-role/tinVO dn='cn=tin,ou=users,dc=hashicorp,dc=com' username='tin' rotation_period="24h"
Success! Data written to: vault-plugin-secrets-openldap/static-role/tinVO
tin.vo@tin vault-plugin-secrets-openldap % v list vault-plugin-secrets-openldap/static-role
Keys
----
tinvo
tin.vo@tin vault-plugin-secrets-openldap % v read vault-plugin-secrets-openldap/static-role/tinvo
Key                    Value
---                    -----
dn                     cn=tin,ou=users,dc=hashicorp,dc=com
last_vault_rotation    2024-10-14T11:32:18.061378-07:00
rotation_period        24h
username               tin
tin.vo@tin vault-plugin-secrets-openldap % v write -f vault-plugin-secrets-openldap/static-role/tinVO
Success! Data written to: vault-plugin-secrets-openldap/static-role/tinVO
tin.vo@tin vault-plugin-secrets-openldap % v write -f vault-plugin-secrets-openldap/static-role/tiNVo
Success! Data written to: vault-plugin-secrets-openldap/static-role/tiNVo
tin.vo@tin vault-plugin-secrets-openldap % v write -f vault-plugin-secrets-openldap/static-role/tinvo
Success! Data written to: vault-plugin-secrets-openldap/static-role/tinvo

Copy link
Contributor

@fairclothjm fairclothjm 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 @tvo0813! Could we also add a CHANGELOG entry?

@tvo0813 tvo0813 force-pushed the VAULT-9208_rotated_account_case_sensitive branch from 81986fd to 1e40a9c Compare October 14, 2024 18:56
@tvo0813 tvo0813 merged commit ecd418c into main Oct 14, 2024
4 checks passed
@fairclothjm fairclothjm deleted the VAULT-9208_rotated_account_case_sensitive branch October 25, 2024 15:44
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.

4 participants