-
Notifications
You must be signed in to change notification settings - Fork 4k
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.
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
feat: check for accidental exposure of secrets #19543
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, I agree with the change, design, and the feature flag. I have a few questions and comments.
packages/@aws-cdk/aws-ec2/test/integ.vpn-pre-shared-key-token.ts
Outdated
Show resolved
Hide resolved
function isSecretValue(x: any): x is SecretValue { | ||
return typeof x === 'object' && x && 'unsafeUnwrap' in x; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a static function on SecretValue
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only if we need it in more than one place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm okay. If we don't put it into SecretValue
right now, I think we risk other people re-implementing the same thing in other places.
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
Co-authored-by: Madeline Kusters <80541297+madeline-k@users.noreply.github.com>
…huijbers/secret-values
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving, but added the do-not-merge label in case you want to move isSecretValue()
to SecretValue
.
function isSecretValue(x: any): x is SecretValue { | ||
return typeof x === 'object' && x && 'unsafeUnwrap' in x; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm okay. If we don't put it into SecretValue
right now, I think we risk other people re-implementing the same thing in other places.
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Introduces the following changes to secrets handling: - Deprecate `SecretValue.plainText()` in favor of `SecretValue.unsafePlainText()` to highlight its issues - Introduce `SecretValue.resourceAttribute()` for cases where we know we want to use a value produced at deployment time - Introduce a new feature flag (`@aws-cdk/core:checkSecretUsage`) which, when enabled, will make it impossible to use secrets without explicitly unwrapping them first (`secretValue.unsafeUnwrap`). This prevents people from naively sticking secrets into vulnerable locations like environment variables. - Deprecate `secretsmanager.SecretStringValueBeta1` in favor of just accepting a `SecretValue`. Since this behavior would constitute a breaking change, it will only be enabled if the feature flag `@aws-cdk/core:checkSecretUsage` is turned on. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Introduces the following changes to secrets handling: - Deprecate `SecretValue.plainText()` in favor of `SecretValue.unsafePlainText()` to highlight its issues - Introduce `SecretValue.resourceAttribute()` for cases where we know we want to use a value produced at deployment time - Introduce a new feature flag (`@aws-cdk/core:checkSecretUsage`) which, when enabled, will make it impossible to use secrets without explicitly unwrapping them first (`secretValue.unsafeUnwrap`). This prevents people from naively sticking secrets into vulnerable locations like environment variables. - Deprecate `secretsmanager.SecretStringValueBeta1` in favor of just accepting a `SecretValue`. Since this behavior would constitute a breaking change, it will only be enabled if the feature flag `@aws-cdk/core:checkSecretUsage` is turned on. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Introduces the following changes to secrets handling:
SecretValue.plainText()
in favor ofSecretValue.unsafePlainText()
to highlight its issuesSecretValue.resourceAttribute()
for cases where we know we want to use a value produced at deployment time@aws-cdk/core:checkSecretUsage
) which, when enabled, will make it impossible to use secrets without explicitly unwrapping them first (secretValue.unsafeUnwrap
). This prevents people from naively sticking secrets into vulnerable locations like environment variables.secretsmanager.SecretStringValueBeta1
in favor of just accepting aSecretValue
.Since this behavior would constitute a breaking change, it will only be enabled if the feature flag
@aws-cdk/core:checkSecretUsage
is turned on.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license