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

core: set namespace within GeneratePasswordFromPolicy #12635

Merged
merged 4 commits into from Sep 27, 2021

Conversation

calvn
Copy link
Member

@calvn calvn commented Sep 24, 2021

This PR makes a change on dynamicSystemView.GeneratePasswordFromPolicy to set a namespace value on its incoming context based on the value that exists on its mount entry.

GeneratePasswordFromPolicy is accessible to plugins via framework.Backend.System(), but callback handlers on the plugin side don't carry over the namespace its context, thus leading to cases where namespace value is not set when called by plugins. The dynamic system view already has visibility into its namespace through its mountEntry so we can just set the context's namespace to the value from there.

Prior to this change, static role credentials rotations would error due to missing namespace information.

Auto-rotation failure:

2021-09-23T11:28:38.363-0700 [ERROR] secrets.openldap.openldap_084ff8e4.openldap.vault-plugin-secrets-openldap: unable to rotate credentials in periodic function: error="unable to generate password: rpc error: code = Internal desc = failed to generate password: failed to retrieve password policy: no namespace" timestamp=2021-09-23T11:28:38.363-0700

Manual rotation failure:

Error writing data to openldap/rotate-role/mary: Error making API request.

Namespace: test/ad1/
URL: PUT http://localhost:8200/v1/openldap/rotate-role/mary
Code: 500. Errors:

* 1 error occurred:
	* unable to generate password: rpc error: code = Internal desc = failed to generate password

@calvn calvn added this to the 1.9 milestone Sep 24, 2021
@calvn calvn requested review from pcman312, kalafut and a team September 24, 2021 23:48
@vercel vercel bot temporarily deployed to Preview – vault September 24, 2021 23:53 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook September 24, 2021 23:53 Inactive
Copy link
Contributor

@tomhjp tomhjp left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@calvn calvn merged commit e0cbb10 into main Sep 27, 2021
@calvn calvn deleted the sysview-gen-password-ns-fix branch September 27, 2021 16:08
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
calvn added a commit that referenced this pull request Sep 27, 2021
* core: set namespace from the sysview's mount entry on GeneratePasswordFromPolicy

* test: update TestDynamicSystemView to be ns-aware, update tests

* add changelog entry
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

3 participants