aws / aws-cdk

The AWS Cloud Development Kit is a framework for defining cloud infrastructure in code
https://aws.amazon.com/cdk
Apache License 2.0
11.56k stars 3.87k forks source link

(core): make SecretValue.unsafePlainText() harder to use #20033

Open rix0rrr opened 2 years ago

rix0rrr commented 2 years ago

Describe the feature

By request of the Secrets Manager team, use of SecretValue.unsafePlainText() needs to be discouraged even more.

We'll add a feature flag:

{
  "@aws-cdk/core.preventUnsafePlaintextSecrets": true
}

That users will have to explicitly turn off to be able to call the plaintext constructor for SecretValue.

While we're at it, let's also record metadata into the resulting CFN template that an unsafe secret was rendered literally into the template.

Use Case

Make sure that users don't commit secrets to source control.

Proposed Solution

No response

Other Information

No response

Acknowledgements

CDK version used

-

Environment details (OS name and version, etc.)

-

laurelmay commented 2 years ago

Based on my understanding of the current implementations of secretsmanager.Secret and cdk.SecretValue, this may make it impossible to create a JSON object (as shown in the SecretStringValueBeta1.fromToken docs when this Feature Flag is enabled.

SecretValue.resourceAttribute is far more strict than secretsmanager.SecretStringValueBeta1.fromToken was as the former uses Tokenization.reverseCompleteString whereas the latter just checks Token.isUnresolved.

The most direct translation would seem like it'd be something along the lines of:

const user = new iam.User(this, 'User');
const accessKey = new iam.AccessKey(this, 'AccessKey', { user });
const secretValue = secretsmanager.Secret(this, 'Secret', {
  secretStringValue: cdk.SecretValue.resourceAttribute(JSON.stringify({
    username: user.userName,
    database: 'foo',
    password: accessKey.secretAccessKey.unsafeUnwrap(),
  }))
});

Which fails because the JSON object isn't entirely a Token, it's a concatenation (with the braces, commas, keys, and such). Therefore, the only possible translation of that example leverages unsafePlainText:

const secretValue = secretsmanager.Secret(this, 'Secret', {
  secretStringValue: cdk.SecretValue.unsafePlainText(JSON.stringify({
    username: user.userName,
    database: 'foo',
    password: accessKey.secretAccessKey.unsafeUnwrap(),
  }))
});

Storing JSON objects in secrets is a very common practice (and, in fact, is the default in the Console). It may be counterintuitive for users with a non-simple-string Secret to need to flip a feature flag that would also enable truly dangerous behavior.

ntachukwu commented 2 years ago

I see the solution provided by the merged PR above is no longer part of the codebase. Why is that?