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.

Sign up for GitHub

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

Jump to bottom

Vault - Support default TTLs #33

Closed
wants to merge 6 commits into from
Closed

Vault - Support default TTLs #33

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c98c47a
Add support for the Vault Database credential engine
kppullin-nt Nov 19, 2021
1bf5663
Vault lease - Use the existing 'response' instead of making another c…
kppullin-nt Dec 2, 2021
51a0be0
Vault - Support default TTLs
kppullin-nt Dec 2, 2021
1527fc8
Merge pull request #1 from kppullin/vault-database-engine
kppullin-nt Dec 6, 2021
fde97de
Merge branch 'master' into kv2-lease-ttl
kppullin-nt Dec 6, 2021
2d42ebe
Set the TTL to `None` if the duration is `0`
kppullin-nt Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions src/main/scala/io/lenses/connect/secrets/config/VaultProviderConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ object VaultProviderConfig {
val VAULT_CLIENT_PEM: String = "vault.client.pem"
val VAULT_PEM: String = "vault.pem"
val VAULT_ENGINE_VERSION = "vault.engine.version"
val VAULT_SECRET_TTL_DEFAULT: String = "vault.secret.ttl.default"
val AUTH_METHOD: String = "vault.auth.method"

val VAULT_TRUSTSTORE_LOC: String =
Expand Down Expand Up @@ -137,6 +138,13 @@ object VaultProviderConfig {
Importance.HIGH,
"KV Secrets Engine version of the Vault server instance. Defaults to 2"
)
.define(
VAULT_SECRET_TTL_DEFAULT,
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"
Copy link

Choose a reason for hiding this comment

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

The referenced issue is closed, I suggest the following wording to keep things simple:

Suggested change
"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

)
// auth mode
.define(
AUTH_METHOD,
Expand Down
3 changes: 3 additions & 0 deletions src/main/scala/io/lenses/connect/secrets/config/VaultProviderSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ case class VaultSettings(
pem: String,
clientPem: String,
engineVersion: Int = 2,
secretTtlDefault: Int = 0,
appRole: Option[AppRole],
awsIam: Option[AwsIam],
gcp: Option[Gcp],
Expand All @@ -72,6 +73,7 @@ object VaultSettings extends StrictLogging {
val pem = config.getString(VaultProviderConfig.VAULT_PEM)
val clientPem = config.getString(VaultProviderConfig.VAULT_CLIENT_PEM)
val engineVersion = config.getInt(VaultProviderConfig.VAULT_ENGINE_VERSION)
val secretTtlDefault = config.getInt(VaultProviderConfig.VAULT_SECRET_TTL_DEFAULT)

val authMode = VaultAuthMethod.withNameOpt(
config.getString(VaultProviderConfig.AUTH_METHOD).toUpperCase
Expand Down Expand Up @@ -121,6 +123,7 @@ object VaultSettings extends StrictLogging {
pem = pem,
clientPem = clientPem,
engineVersion = engineVersion,
secretTtlDefault = secretTtlDefault,
appRole = appRole,
awsIam = awsIam,
gcp = gcp,
Expand Down
32 changes: 27 additions & 5 deletions src/main/scala/io/lenses/connect/secrets/providers/VaultSecretProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@

package io.lenses.connect.secrets.providers

import java.nio.file.FileSystems
import java.nio.file.Paths
import java.time.OffsetDateTime
import java.util
Expand All @@ -15,6 +14,7 @@ import _root_.io.lenses.connect.secrets.config.VaultProviderConfig
import _root_.io.lenses.connect.secrets.config.VaultSettings
import _root_.io.lenses.connect.secrets.connect._
import com.bettercloud.vault.Vault
import com.bettercloud.vault.response.LogicalResponse
import io.lenses.connect.secrets.async.AsyncFunctionLoop
import io.lenses.connect.secrets.io.FileWriterOnce
import io.lenses.connect.secrets.utils.EncodingAndId
Expand All @@ -30,7 +30,6 @@ import scala.util.Try

class VaultSecretProvider() extends ConfigProvider with VaultHelper {

private val separator: String = FileSystems.getDefault.getSeparator
private var settings: VaultSettings = _
private var vaultClient: Option[Vault] = None
private var tokenRenewal: Option[AsyncFunctionLoop] = None
Expand Down Expand Up @@ -122,7 +121,7 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {

logger.info(s"Looking up value at [$path]")
val fileWriter = new FileWriterOnce(Paths.get(settings.fileDir, path))
Try(vaultClient.get.logical().read(path)) match {
Try(readSecretFromVault(path)) match {
case Success(response) =>
if (response.getRestResponse.getStatus != 200) {
throw new ConnectException(
Expand All @@ -133,8 +132,15 @@ class VaultSecretProvider() extends ConfigProvider with VaultHelper {
)
}

val ttl = Option(vaultClient.get.logical().read(path).getLeaseDuration) match {
case Some(duration) => Some(now.plusSeconds(duration))
val ttl = Option(response.getLeaseDuration) match {
case Some(duration) =>
if (duration != 0) {
Some(now.plusSeconds(duration))
} else if (settings.secretTtlDefault > 0) {
Some(now.plusSeconds(settings.secretTtlDefault))
} else {
None
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be Some(now) given that previously with a duration, we returned now.plusSeconds(duration)?

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
case None => None
}

Expand Down Expand Up @@ -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
Copy link

Choose a reason for hiding this comment

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

This comment can be removed I think, it states what the code does line by line without adding value.

// vault client to obtain this secret. Otherwise, fall back to the default key-vaule engine.
if (path.startsWith("database/creds/")) {
Copy link

Choose a reason for hiding this comment

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

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.

val parts = path.split("/")

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>'")
Copy link

Choose a reason for hiding this comment

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

Suggested change
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>'")

}
} else {
vaultClient.get.logical().read(path)
}
}
}