AWS — added feature to read secrets from other accounts #57
Conversation
There was a problem hiding this comment.
Thank you for the contribution, it's much appreciated to add features to the AWS Secret Provider.
My comments:
- the new configuration property seems unnecessary as I would have thought simply changing the region in configuration should work.
- there are no unit tests for these changes.
Please reach out if you want to discuss or if I can help you in any way.
| @@ -39,6 +40,8 @@ object AWSProviderSettings { | |||
| val authMode = | |||
| getAuthenticationMethod(configs.getString(AWSProviderConfig.AUTH_METHOD)) | |||
|
|
|||
| val altRegion = configs.getString("aws.cross.account.region") | |||
There was a problem hiding this comment.
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
| @@ -23,6 +23,7 @@ case class AWSProviderSettings( | |||
| fileWriterOpts: Option[FileWriterOptions], | |||
| defaultTtl: Option[Duration], | |||
| endpointOverride: Option[String], | |||
| altRegion: String | |||
There was a problem hiding this comment.
altRegion would need to be made an Option so as not to cause existing users to have to change their settings
| @@ -38,7 +38,9 @@ import scala.util.Try | |||
| class AWSHelper( | |||
| client: SecretsManagerClient, | |||
| defaultTtl: Option[Duration], | |||
| fileWriterCreateFn: () => Option[FileWriter], | |||
| region: String, | |||
| altRegion: String, | |||
There was a problem hiding this comment.
Should be made an Option
| private def getSecretName(secretId: String): String = { | ||
| val hasAccount = secretId.indexOf("$") | ||
| if (hasAccount > -1) { | ||
| val secret_region = if (hasAccount > -1 && altRegion.length > 0) altRegion else region |
There was a problem hiding this comment.
we don't use underscores in variable names, for consistency these should be camelCase
| private def getSecretName(secretId: String): String = { | ||
| val hasAccount = secretId.indexOf("$") | ||
| if (hasAccount > -1) { | ||
| val secret_region = if (hasAccount > -1 && altRegion.length > 0) altRegion else region |
There was a problem hiding this comment.
the hasAccount > -1 condition here is unnecessary as you're already in a block
`if (hasAccount > -1)
| } | ||
|
|
||
| private def getSecretName(secretId: String): String = { | ||
| val hasAccount = secretId.indexOf("$") |
There was a problem hiding this comment.
could this be
val hasAccount = secret.contains("*")
| } | ||
|
|
||
| private def getSecretName(secretId: String): String = { | ||
| val hasAccount = secretId.indexOf("$") |
There was a problem hiding this comment.
This variable is named as a boolean (hasAccount). This and the > 1 condition below should be merged so this is a boolean variable.
| @@ -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" | |||
There was a problem hiding this comment.
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.
|
@caltangelo We are looking to release a new version soon. Are you able to address Davids comments? |
By prefixing the secret name with the account ID and a dollar sign ($), the provider will now construct the full secret ARN before calling getSecretValue(), therefore enabling the AWS API to find secrets located in any account that the configured user is authorized to access.
e.g.
${aws:databases/credentials:password}will call getSecretValue('databases/credentials'), defaulting to the configured account, whereas${aws:123456789012$databases/credentials:password}will provide the full ARN ofarn:aws:secretsmanager:us-east-1:123456789012:secret:databases/credentialsIf the cross-account secret is located in a different region, you can specify that via the
aws.cross.account.regionconfiguration.