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

Review vault role / user creation #2556

Open
Ardiea opened this issue Jul 17, 2024 · 0 comments
Open

Review vault role / user creation #2556

Ardiea opened this issue Jul 17, 2024 · 0 comments
Assignees
Labels
Design Infrastructure Tasks related to infrastructure needed to power other services Maintenance

Comments

@Ardiea
Copy link
Member

Ardiea commented Jul 17, 2024

Description/Context

4:04 PM sar  Yeah think as long as we don't have the grant definitions, then we should be good as those are the statements that are causing the error
4:06 PM tmacey Looking at it more, it seems like the static role definition is for when you have already defined the role and just want to rotate credentials. There's no parameter for defining the creation statements to allow for specifying the grants.
4:21 PM sar Can it be broken down into multiple roles?
4:22 PM tmacey Yeah, we would need to have one role for admins, one for app users, and one for readonly
4:23 PM sar We kinda have that though. I was thinking more in terms of the app role or whatever role that's throwing the error. Can that be broken down further
4:25 PM tmacey So, the issue is that the app user explicitly grants permissions every time it gets created. We need to move those grants to the role definition, and then the user creation permissions are just "grant <role> to <user>;"
4:28 PM sar But if the problem is the overloaded grants, then wouldn't the error be thrown when trying to create that role with all the grant statements
4:32 PM tmacey The problem is that the grants are getting run too many times (I think)
4:32 PM Also, I think the revoke statements aren't all working right, which is leaving lots of excess user objects laying around
4:34 PM Basically, we need to do a thorough and holistic review of our Postgres permissioning. At this point it's mostly a cobbled together set of "I got it to work?" fixes that have accrued over time.
4:34 PM sar I'm not sure about that, cause i think when i ran into this, I tried running a script that would run the grants sequentially and that seemed to have worked, but u can't do that in the vault creation statement
4:34 PM tmacey I think you can.
4:34 PM If you look at the UI, it accepts a list of statements
4:34 PM We just have it all as a single blob because of historical reasons.
4:35 PM I think we can turn our "one big string" into an array
4:35 PM sar Oh great, then maybe that would be the way to solve it. But yeah I agree, we need to look over the entire approach

https://github.com/mitodl/ol-infrastructure/blob/main/src/ol_infrastructure/lib/vault.py#L19-L98

Plan/Design

@Ardiea Ardiea added Maintenance Infrastructure Tasks related to infrastructure needed to power other services Design labels Jul 17, 2024
@Ardiea Ardiea self-assigned this Jul 18, 2024
Ardiea added a commit that referenced this issue Aug 15, 2024
* Inital commit of vault db credential fixups for postgres.

* Fixing vault permissions after db cred change.

* Refactor vault database secret code to support an array of statements rather than just one. Also added support for renew and rollback statements.

* Updated to split role_statments out into their own lines.

* Revert changes to concourse database secret look ups.

* Fixing template

* Fixing revoke statements.

* Removed access to the public schema for the reverse_etl role.

* Added notice raises for untested pathways that create roles. Removed some questional grants from the revoke statement of an admin user.

* fix: Split role management from user creation

With all of the role creation/permission settings statements we are still hitting the
"line too long" error in Postgres. This splits the role definitions back into separate
blocks to avoid that error.

* fix: Remove approle revocation statements to avoid accidental removal

---------

Co-authored-by: Tobias Macey <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Design Infrastructure Tasks related to infrastructure needed to power other services Maintenance
Projects
None yet
Development

No branches or pull requests

1 participant