feat: Hashicorp Vault static role credentials#1130
Conversation
1294ef5 to
6900dc6
Compare
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
|
||
| Ok(VaultToken { | ||
| token: auth.auth.client_token, | ||
| expires_at: SystemTime::now() + Duration::from_secs(auth.auth.lease_duration), |
There was a problem hiding this comment.
You kind of want to use Instant here because its guaranteed to never be affected but clock skew.
There was a problem hiding this comment.
Are you sure you meant here for the vault auth token, or did you actually mean in the token cache?
There was a problem hiding this comment.
Good question. So, if the expiration date is external - agreed, let's use system time or parsed time. If it's our own, e.g., we pull a duration from somewhere and add it to now(), let's use Instant because that will guarantee, in the unlikely scenario someone updates the clock, that the token cache won't blow up.
There was a problem hiding this comment.
Changed the auth token and refresh_at cache field to Instant 👍
I made the expire_at field, used by RDS and Azure, optional to avoid this cumbersome logic in the vault code
expire_at: SystemTime::now() + ttl
refresh_at: Instant:now() + ttl
Let me know what you think
|
Is the static vs dynamic credentials a known Vault thing that we can point people to, e.g., in their docs? Just trying to understand if this is something you guys developed internally or is more of an "industry standard" (quotes for dramatic effect, I don't think Vault is that ubiquitous). |
Both are native Vault features and are documented here: |
|
Sounds good. Will review asap |
| // Resolve any Vault static role entries to plaintext before starting | ||
| // the auth exchange. MD5, SCRAM, and plain all need the actual | ||
| // password, so this must happen before the first message is sent. | ||
| let resolved; |
There was a problem hiding this comment.
I understood what happens here, but maybe there is a cleaner way to write this? e.g., if vault, do x, else do y, or maybe move this into its own testable method?
There was a problem hiding this comment.
Moved the logic in the auth::vault crate and moved the call before check_password
Should look way cleaner now
There was a problem hiding this comment.
Looks good, just a few naming convention updates and a bit of code cleanup. Happy to merge this afterwards.
We should also start running tests for this in CI (i.e. integration tests with actual vault server) if we can. Maybe as a follow up PR, just to make sure we don't break anything here going forward. This is definitely more complex than RDS IAM and friends.
|
Also, I just found this: https://docs.rs/vaultrs/latest/vaultrs/ Any reason to not use an existing vault client and implement our own? I get that it's just an HTTP API with JWT, so pretty simple, but still worth considering. |
Didn't want to import an entire library for just two endpoint calls, but happy to switch to it if you prefer 👍 |
2200b1c to
f9e9f7a
Compare
f9e9f7a to
4ede7bd
Compare
All good. I just wanted to confirm |
Hey, it's me again 😃
Summary
client_vault_path) and backend PgDog-to-Postgres connections (server_auth = "vault_static"+backend_vault_path)server_auth = "vault"→"vault_dynamic"andvault_path→backend_vault_pathfor clarity/symmetry withclient_vault_path. Both old names still parse via#[serde(alias = ...)], no config breaking changeserver_user/name), not generated by Vault, and there's no proactive percentage-based refreshrefresh_at.pgdog/src/auth/vault.rs, reused by bothpgdog/src/backend/auth/vault.rsandpgdog/src/frontend/client/mod.rs