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 static credential support to RabbitMQ secret engine #21644

Closed
wants to merge 25 commits into from

Conversation

f4z3r
Copy link
Contributor

@f4z3r f4z3r commented Jul 7, 2023

I will add the API documentation and the general documentation to this PR in the coming days.

@f4z3r f4z3r requested a review from a team July 7, 2023 05:57
@f4z3r f4z3r requested a review from a team as a code owner July 10, 2023 12:40
website/content/docs/secrets/rabbitmq.mdx Outdated Show resolved Hide resolved
website/content/docs/secrets/rabbitmq.mdx Outdated Show resolved Hide resolved
website/content/docs/secrets/rabbitmq.mdx Outdated Show resolved Hide resolved
website/content/docs/secrets/rabbitmq.mdx Outdated Show resolved Hide resolved
website/content/docs/secrets/rabbitmq.mdx Outdated Show resolved Hide resolved
f4z3r and others added 3 commits August 14, 2023 06:59
Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>
Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>
Co-authored-by: Sarah Chavis <62406755+schavis@users.noreply.github.com>
@f4z3r
Copy link
Contributor Author

f4z3r commented Aug 14, 2023

Thanks for the review @schavis !

@f4z3r f4z3r requested a review from schavis August 14, 2023 05:03
@@ -91,6 +108,21 @@ the proper permission, it can generate credentials.
such that trusted operators can manage the role definitions, and both users
and applications are restricted in the credentials they are allowed to read.

1. Alternatively, request credentials for a static role by reading from the
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
1. Alternatively, request credentials for a static role by reading from the
Alternatively, request credentials for a static role by reading from the

Missed this one in the first review, apologies. No need to number steps if there's only one step :)

@schavis schavis requested a review from a team as a code owner August 17, 2023 19:40
@schavis schavis enabled auto-merge (squash) August 17, 2023 19:40
@austingebauer
Copy link
Contributor

Hi, @f4z3r! This looks like a reasonable feature to add to the RabbitMQ secrets engine. However, we're starting to notice a pattern around how static role credential rotation is taking form across the various secrets engines (e.g., database, LDAP, AWS) that support it. The priority queue based rotation mechanism is being copy-pasted around. We want to take the time to look at how we can introduce rotation mechanisms to the various secrets engines without this copy-pasting. Ideally, it would be much easier for secrets engine developers to provide secrets rotation without having to maintain the mechanism that performs the rotation.

For that reason, we'd like to hold off on merging this. If it's important for your use case, I can help provide some information for how you could run this as an external plugin in your Vault deployment. Happy to hear your thoughts.

@f4z3r
Copy link
Contributor Author

f4z3r commented Aug 18, 2023

Hi @austingebauer , this definitely makes sense. Especially considering that there are quite a few little elements that one needs to consider that are not super obvious directly from the code when one looks at it (what happens in the event of a crash of Vault, how high can a rotation delay end up being, etc). It should be noted though, that as far as I remember the AWS static credentials do not use a priority queue. I tried to base the RMQ implementation off of that initially, but had some issues with rotations being massively delayed.

We are currently not yet relying on tihs for our use case, but might want to utilise such features in the future.

If you are open for some help on the refactoring, I would be happy to support.

@austingebauer
Copy link
Contributor

Thanks for understanding, @f4z3r! I don't have specific details to share yet, but I appreciate you offering to help out.

I'm going to close this PR since it's not something we'll merge. It would be neat to revisit this once we have something available to hook into.

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.

3 participants