Vault - Support default TTLs #33
Conversation
Vault's KV2 engine does not return the `lease_duration` TTL value that is provided by the original KV1 engine. When this value is 0 or missing it effectively disables the caching support of the `VaultSecretProvider`. This commit adds support for configuring an optional default TTL to apply when the `lease_duration` value is 0. To maintain backwards compatibility the default value remains 0. Fixes lensesio#4 Ref: hashicorp/vault#6274
Vault database engine
There was a problem hiding this comment.
I am not a maintainer, however I am interested in seeing this PR move forward. Here are some suggestions. cc @andrewstevenson
| Type.INT, | ||
| 0, | ||
| Importance.MEDIUM, | ||
| "Default TTL for secrets returned from Vault. Defaults to 0 to not break existing configurations. Work around for the lack of `lease_duration` values returned from the Vault KV2 engine: https://github.com/hashicorp/vault/issues/6274" |
There was a problem hiding this comment.
The referenced issue is closed, I suggest the following wording to keep things simple:
| "Default TTL for secrets returned from Vault. Defaults to 0 to not break existing configurations. Work around for the lack of `lease_duration` values returned from the Vault KV2 engine: https://github.com/hashicorp/vault/issues/6274" | |
| "Default TTL for secrets returned by Vault. Defaults to 0 for backwards compatibility |
| } else if (settings.secretTtlDefault > 0) { | ||
| Some(now.plusSeconds(settings.secretTtlDefault)) | ||
| } else { | ||
| None |
There was a problem hiding this comment.
Shouldn't this be Some(now) given that previously with a duration, we returned now.plusSeconds(duration)?
There was a problem hiding this comment.
It's been a "long time" since I wrote this code (3 weeks! 😆 ) and some of the changes mixed in here were required when combined with other changes (#32 and #34). It was a bit of a mess to try to split these 3 PRs up into cleanly applicable units after the fact and it looks like other unrelated changes have also made their way in too, such as the /database/creds code.
If I recall correctly, returning None here is necessary when once the fix for making TTLs work is applied (#34), but you're likely correct that returning now.plusSeconds(duration) would work here. However, since I think this may all be moot without #34, as the TTLs don't work anyway, it may be better to combine the two PRs.
I lack time at the moment to untangle this, but will note that we've been running a patched version that combines #32, #33, and #34 for a few weeks now without issues (https://github.com/NextDeveloperTeam/secret-provider/commits/master).
Also thanks for all the feedback and hoping to find some time to revisit in more detail in the new year.
| @@ -162,4 +168,20 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper { | |||
| ) | |||
| } | |||
| } | |||
|
|
|||
| private def readSecretFromVault(path: String): LogicalResponse = { | |||
| // Check if the path looks like it's referencing the vault database engine. If so, use the `database()` api of the | |||
There was a problem hiding this comment.
This comment can be removed I think, it states what the code does line by line without adding value.
| if (parts.length == 3) { | ||
| vaultClient.get.database().creds(parts(2)) | ||
| } else { | ||
| throw new Exception("Database path is invalid. Path must be in the form 'database/creds/<role_name>'") |
There was a problem hiding this comment.
| throw new Exception("Database path is invalid. Path must be in the form 'database/creds/<role_name>'") | |
| throw new IllegalArgumentException("Database path is invalid, must be in the form 'database/creds/<role_name>'") |
| private def readSecretFromVault(path: String): LogicalResponse = { | ||
| // Check if the path looks like it's referencing the vault database engine. If so, use the `database()` api of the | ||
| // vault client to obtain this secret. Otherwise, fall back to the default key-vaule engine. | ||
| if (path.startsWith("database/creds/")) { |
There was a problem hiding this comment.
We may want to have a test case for database creds. A good place to start from would probably be the test case for kv secrets.
|
@kppullin Some similar functionality has been implemented in #47 . We have added the flag Do you want to check this and see if it fit your requirements, and if so close this one off? |
|
Closing as similar functionality was implemented by #47 . Thank you for the contribution! |
Vault's KV2 engine does not return the
lease_durationTTL valuethat is provided by the original KV1 engine. When this value is 0
or missing it effectively disables the caching support of the
VaultSecretProvider.This commit adds support for configuring an optional default TTL to apply
when the
lease_durationvalue is 0.To maintain backwards compatibility the default value remains 0.
Fixes #4
Ref: hashicorp/vault#6274