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

MongoDB database secret engine - rotate-root - Only default to admin if DB blank #23240

Merged
merged 6 commits into from
Oct 4, 2023

Conversation

conor-mccullough
Copy link
Contributor

@conor-mccullough conor-mccullough commented Sep 22, 2023

Currently the MongoDB plugin does not allow for rotate-root actions unless the user exists already in the admin database.

While fine for some use-cases, many environments do not allow true root-level access to MongoDB and separate admins must administer their own databases within Mongo (similar to Vault's namespaces), and it is not unreasonable to expect that teams administering Vault will not be MongoDB admins and therefore not have "full" root access to their target databases.

@conor-mccullough conor-mccullough requested a review from a team as a code owner September 22, 2023 11:10
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 22, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@github-actions
Copy link

CI Results:
All Go tests succeeded! ✅

@ram-parameswaran ram-parameswaran changed the title Only default to admin if DB blank MongoDB database secret engine - Only default to admin if DB blank Sep 22, 2023
@ram-parameswaran ram-parameswaran changed the title MongoDB database secret engine - Only default to admin if DB blank MongoDB database secret engine - rotate-root - Only default to admin if DB blank Sep 22, 2023
@ltcarbonell ltcarbonell self-requested a review September 25, 2023 14:00
@conor-mccullough
Copy link
Contributor Author

Added backport tags. I'm not sure if this requires a changelog entry as it is quite minor - if so please let me know.

@fairclothjm
Copy link
Contributor

@conor-mccullough Thanks for the PR! Yes, I think we should add a changelog entry for this behavior. Is this fixing a reported bug? Or is it only an enhancement?

Are there any tests we can add/update to test this behavior?

@conor-mccullough
Copy link
Contributor Author

conor-mccullough commented Sep 27, 2023

Thanks for the feedback @fairclothjm, I'm adding the changelog and am in the process of including a couple of minor tests. They will hopefully come in the next day or so.

In regards to it being a bug/enhancement - I would argue it's a bug, since this looks and feels like something that may have initially been introduced accidentally. This was raised for an enterprise customer, I am happy to provide more details (internal tracking etc) privately if needed.

@fairclothjm
Copy link
Contributor

@conor-mccullough Thanks! Sounds like marking it as a bug in the changelog will do then.

@vercel
Copy link

vercel bot commented Oct 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
vault ⬜️ Ignored (Inspect) Visit Preview Oct 4, 2023 0:22am

@conor-mccullough
Copy link
Contributor Author

Hi @fairclothjm - submitted with some basic tests and the changelog entry added.

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!

@fairclothjm fairclothjm added this to the 1.15.1 milestone Oct 3, 2023
@conor-mccullough
Copy link
Contributor Author

Thanks @fairclothjm ! Merging is currently blocked - are you able to merge or is there anything I can do on my end to get this merged & closed out?

Looping in @ltcarbonell and @jasonodonnell for a bit of additional visibility.

@fairclothjm
Copy link
Contributor

@conor-mccullough Is there a reason we are backporting all the way to 1.11? Usually we do n-2 so that would be back to 1.13

@conor-mccullough
Copy link
Contributor Author

@fairclothjm Human error - I've removed the two labels for 1.11 and 1.12. Thanks!

@fairclothjm
Copy link
Contributor

@conor-mccullough Thanks, I will go ahead and close the 1.11 and 1.12 backport PRs and approve and merge the others.

@fairclothjm
Copy link
Contributor

@conor-mccullough FYI all backports and merged and this is wrapped up. Thanks for the help here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants