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

AWS — added feature to read secrets from other accounts #57

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

AWS — added feature to read secrets from other accounts #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fc207e1
parsing account ID from secret and building ARN
Sep 1, 2023
713ed2a
new if logic
Sep 1, 2023
a51c36a
added altRegion to AWS config
Sep 7, 2023
d073f8e
added alternate region to config
Sep 8, 2023
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
2 changes: 1 addition & 1 deletion project/Settings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ object Settings extends Dependencies {
val scala213 = "2.13.10"
val scala3 = "3.2.2"

val nextVersion = "2.1.7"
val nextVersion = "2.2.1"
val artifactVersion = {
sys.env.get("LENSES_TAG_NAME") match {
case Some(tag) => tag
Expand Down
8 changes: 8 additions & 0 deletions secret-provider/src/main/scala/io/lenses/connect/secrets/config/AWSProviderConfig.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ object AWSProviderConfig {
val AWS_SECRET_KEY: String = "aws.secret.key"
val AUTH_METHOD: String = "aws.auth.method"
val ENDPOINT_OVERRIDE: String = "aws.endpoint.override"
val AWS_CROSS_ACCOUNT_REGION: String = "aws.cross.account.region"
Copy link
Contributor

Choose a reason for hiding this comment

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

I general I'm not sure I understand the need for the alt.region property as

  • it is always used wherever region is used and simply overrides it - I think this can be removed in favour of just changing the existing region property in connector configuration?

  • the condition that activates the new configuration is actually only activated based on the presence of an $ in the secret id, so I don't think any configuration is necessary here.


val config: ConfigDef = new ConfigDef()
.define(
Expand Down Expand Up @@ -54,6 +55,13 @@ object AWSProviderConfig {
| Default is 'credentials'
|""".stripMargin,
)
.define(
AWS_CROSS_ACCOUNT_REGION,
Type.STRING,
"",
Importance.MEDIUM,
"AWS region the Secrets manager is in when reading from alternate account",
)
.define(
WRITE_FILES,
Type.BOOLEAN,
Expand Down
4 changes: 4 additions & 0 deletions secret-provider/src/main/scala/io/lenses/connect/secrets/config/AWSProviderSettings.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ case class AWSProviderSettings(
fileWriterOpts: Option[FileWriterOptions],
defaultTtl: Option[Duration],
endpointOverride: Option[String],
altRegion: String
Copy link
Contributor

Choose a reason for hiding this comment

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

altRegion would need to be made an Option so as not to cause existing users to have to change their settings

)

import io.lenses.connect.secrets.config.AbstractConfigExtensions._
Expand All @@ -39,6 +40,8 @@ object AWSProviderSettings {
val authMode =
getAuthenticationMethod(configs.getString(AWSProviderConfig.AUTH_METHOD))

val altRegion = configs.getString("aws.cross.account.region")
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of using the string aws.cross.account.region here, it would be better to refer to the constant you defined AWS_CROSS_ACCOUNT_REGION


if (authMode == AuthMode.CREDENTIALS) {
if (accessKey.isEmpty)
throw new ConnectException(
Expand All @@ -59,6 +62,7 @@ object AWSProviderSettings {
defaultTtl =
Option(configs.getLong(SECRET_DEFAULT_TTL).toLong).filterNot(_ == 0L).map(Duration.of(_, ChronoUnit.MILLIS)),
endpointOverride,
altRegion = altRegion
)
}
}
24 changes: 20 additions & 4 deletions secret-provider/src/main/scala/io/lenses/connect/secrets/providers/AWSHelper.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ import scala.util.Try
class AWSHelper(
client: SecretsManagerClient,
defaultTtl: Option[Duration],
fileWriterCreateFn: () => Option[FileWriter],
region: String,
altRegion: String,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be made an Option

fileWriterCreateFn: () => Option[FileWriter]
)(
implicit
clock: Clock,
Expand All @@ -48,12 +50,26 @@ class AWSHelper(
private val objectMapper = new ObjectMapper()

// get the key value and ttl in the specified secret
override def lookup(secretId: String): Either[Throwable, ValueWithTtl[Map[String, String]]] =
override def lookup(secretId: String): Either[Throwable, ValueWithTtl[Map[String, String]]] = {
val secretName = getSecretName(secretId)
for {
secretTtl <- getTTL(secretId)
secretValue <- getSecretValue(secretId)
secretTtl <- getTTL(secretName)
secretValue <- getSecretValue(secretName)
parsedSecretValue <- parseSecretValue(secretValue)
} yield ValueWithTtl(secretTtl, parsedSecretValue)
}

private def getSecretName(secretId: String): String = {
val hasAccount = secretId.indexOf("$")
Copy link
Contributor

Choose a reason for hiding this comment

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

could this be

val hasAccount = secret.contains("*")

Copy link
Contributor

Choose a reason for hiding this comment

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

This variable is named as a boolean (hasAccount). This and the > 1 condition below should be merged so this is a boolean variable.

if (hasAccount > -1) {
val secret_region = if (hasAccount > -1 && altRegion.length > 0) altRegion else region
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use underscores in variable names, for consistency these should be camelCase

Copy link
Contributor

Choose a reason for hiding this comment

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

the hasAccount > -1 condition here is unnecessary as you're already in a block

`if (hasAccount > -1)

val secret_array = secretId.split("\\$")
s"arn:aws:secretsmanager:${secret_region}:${secret_array(0)}:secret:${secret_array(1)}"
}
else {
secretId
}
}

// determine the ttl for the secret
def getTTL(
Expand Down
2 changes: 2 additions & 0 deletions secret-provider/src/main/scala/io/lenses/connect/secrets/providers/AWSSecretProvider.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ class AWSSecretProvider(testClient: Option[SecretsManagerClient]) extends Config
val awsClient = testClient.getOrElse(createClient(settings))
val helper = new AWSHelper(awsClient,
settings.defaultTtl,
settings.region,
settings.altRegion,
fileWriterCreateFn = () => settings.fileWriterOpts.map(_.createFileWriter()),
)
secretProvider = Some(
Expand Down