Skip to content

Commit

Permalink
feat: check for accidental exposure of secrets (#19543)
Browse files Browse the repository at this point in the history
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*
  • Loading branch information
rix0rrr authored Apr 11, 2022
1 parent e217c79 commit 789e8d2
Show file tree
Hide file tree
Showing 76 changed files with 475 additions and 220 deletions.
2 changes: 1 addition & 1 deletion packages/@aws-cdk/app-delivery/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ const source = new codepipeline_actions.GitHubSourceAction({
output: sourceOutput,
owner: 'myName',
repo: 'myRepo',
oauthToken: cdk.SecretValue.plainText('secret'),
oauthToken: cdk.SecretValue.unsafePlainText('secret'),
});
pipeline.addStage({
stageName: 'source',
Expand Down
2 changes: 1 addition & 1 deletion packages/@aws-cdk/app-delivery/test/integ.cicd.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ const source = new cpactions.GitHubSourceAction({
actionName: 'GitHub',
owner: 'awslabs',
repo: 'aws-cdk',
oauthToken: cdk.SecretValue.plainText('DummyToken'),
oauthToken: cdk.SecretValue.unsafePlainText('DummyToken'),
trigger: cpactions.GitHubTrigger.POLL,
output: sourceOutput,
});
Expand Down
6 changes: 3 additions & 3 deletions packages/@aws-cdk/aws-amplify/lib/app.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export interface SourceCodeProviderConfig {
* to create webhook and read-only deploy key. OAuth token is not stored.
*
* Either `accessToken` or `oauthToken` must be specified if `repository`
* is sepcified.
* is specified.
*
* @default - do not use a token
*/
Expand Down Expand Up @@ -223,7 +223,7 @@ export class App extends Resource implements IApp, iam.IGrantable {
const sourceCodeProviderOptions = props.sourceCodeProvider?.bind(this);

const app = new CfnApp(this, 'Resource', {
accessToken: sourceCodeProviderOptions?.accessToken?.toString(),
accessToken: sourceCodeProviderOptions?.accessToken?.unsafeUnwrap(), // Safe usage
autoBranchCreationConfig: props.autoBranchCreation && {
autoBranchCreationPatterns: props.autoBranchCreation.patterns,
basicAuthConfig: props.autoBranchCreation.basicAuth
Expand All @@ -247,7 +247,7 @@ export class App extends Resource implements IApp, iam.IGrantable {
environmentVariables: Lazy.any({ produce: () => renderEnvironmentVariables(this.environmentVariables) }, { omitEmptyArray: true }),
iamServiceRole: role.roleArn,
name: props.appName || this.node.id,
oauthToken: sourceCodeProviderOptions?.oauthToken?.toString(),
oauthToken: sourceCodeProviderOptions?.oauthToken?.unsafeUnwrap(), // Safe usage
repository: sourceCodeProviderOptions?.repository,
customHeaders: props.customResponseHeaders ? renderCustomResponseHeaders(props.customResponseHeaders) : undefined,
});
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-amplify/lib/basic-auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export class BasicAuth {
if (this.props.password) {
return {
...config,
password: this.props.password.toString(),
password: this.props.password.unsafeUnwrap(), // Safe usage
};
}

Expand All @@ -103,7 +103,7 @@ export class BasicAuth {
});
return {
...config,
password: secret.secretValueFromJson('password').toString(),
password: secret.secretValueFromJson('password').unsafeUnwrap(),
};
}
}
22 changes: 11 additions & 11 deletions packages/@aws-cdk/aws-amplify/test/app.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ test('create an app connected to a GitHub repository', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
buildSpec: codebuild.BuildSpec.fromObjectToYaml({
version: '1.0',
Expand Down Expand Up @@ -70,7 +70,7 @@ test('create an app connected to a GitLab repository', () => {
sourceCodeProvider: new amplify.GitLabSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
buildSpec: codebuild.BuildSpec.fromObject({
version: '1.0',
Expand Down Expand Up @@ -194,9 +194,9 @@ test('with basic auth from credentials', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.plainText('password')),
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.unsafePlainText('password')),
});

// THEN
Expand All @@ -215,7 +215,7 @@ test('with basic auth from generated password', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
basicAuth: amplify.BasicAuth.fromGeneratedPassword('username'),
});
Expand Down Expand Up @@ -254,7 +254,7 @@ test('with env vars', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
environmentVariables: {
key1: 'value1',
Expand Down Expand Up @@ -283,7 +283,7 @@ test('with custom rules', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
customRules: [
{
Expand Down Expand Up @@ -322,7 +322,7 @@ test('with SPA redirect', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
customRules: [amplify.CustomRule.SINGLE_PAGE_APPLICATION_REDIRECT],
});
Expand All @@ -345,7 +345,7 @@ test('with auto branch creation', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
autoBranchCreation: {
environmentVariables: {
Expand Down Expand Up @@ -384,7 +384,7 @@ test('with auto branch deletion', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
autoBranchDeletion: true,
});
Expand All @@ -401,7 +401,7 @@ test('with custom headers', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
customResponseHeaders: [
{
Expand Down
4 changes: 2 additions & 2 deletions packages/@aws-cdk/aws-amplify/test/branch.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ beforeEach(() => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
});
Expand All @@ -38,7 +38,7 @@ test('create a branch', () => {
test('with basic auth from credentials', () => {
// WHEN
app.addBranch('dev', {
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.plainText('password')),
basicAuth: amplify.BasicAuth.fromCredentials('username', SecretValue.unsafePlainText('password')),
});

// THEN
Expand Down
14 changes: 7 additions & 7 deletions packages/@aws-cdk/aws-amplify/test/domain.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ test('create a domain', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
const prodBranch = app.addBranch('master');
Expand Down Expand Up @@ -71,7 +71,7 @@ test('map a branch to the domain root', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
const prodBranch = app.addBranch('master');
Expand Down Expand Up @@ -111,7 +111,7 @@ test('throws at synthesis without subdomains', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});

Expand All @@ -129,7 +129,7 @@ test('auto subdomain all branches', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
const prodBranch = app.addBranch('master');
Expand Down Expand Up @@ -163,7 +163,7 @@ test('auto subdomain some branches', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
});
const prodBranch = app.addBranch('master');
Expand Down Expand Up @@ -195,7 +195,7 @@ test('auto subdomain with IAM role', () => {
sourceCodeProvider: new amplify.GitHubSourceCodeProvider({
owner: 'aws',
repository: 'aws-cdk',
oauthToken: SecretValue.plainText('secret'),
oauthToken: SecretValue.unsafePlainText('secret'),
}),
role: iam.Role.fromRoleArn(
stack,
Expand Down Expand Up @@ -230,4 +230,4 @@ test('auto subdomain with IAM role', () => {
],
},
});
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ new cdk.CfnOutput(stack, 'TESTACCESSKEYID', {
});

new cdk.CfnOutput(stack, 'TESTSECRETACCESSKEY', {
value: userAccessKey.secretAccessKey.toString(),
value: userAccessKey.secretAccessKey.unsafeUnwrap(),
});

new cdk.CfnOutput(stack, 'TESTREGION', {
Expand Down
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-codebuild/lib/source-credentials.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ export class GitHubSourceCredentials extends Resource {
new CfnSourceCredential(this, 'Resource', {
serverType: 'GITHUB',
authType: 'PERSONAL_ACCESS_TOKEN',
token: props.accessToken.toString(),
token: props.accessToken.unsafeUnwrap(), // Safe usage
});
}
}
Expand Down Expand Up @@ -60,7 +60,7 @@ export class GitHubEnterpriseSourceCredentials extends Resource {
new CfnSourceCredential(this, 'Resource', {
serverType: 'GITHUB_ENTERPRISE',
authType: 'PERSONAL_ACCESS_TOKEN',
token: props.accessToken.toString(),
token: props.accessToken.unsafeUnwrap(), // Safe usage
});
}
}
Expand Down Expand Up @@ -92,8 +92,8 @@ export class BitBucketSourceCredentials extends Resource {
new CfnSourceCredential(this, 'Resource', {
serverType: 'BITBUCKET',
authType: 'BASIC_AUTH',
username: props.username.toString(),
token: props.password.toString(),
username: props.username.unsafeUnwrap(), // Safe usage
token: props.password.unsafeUnwrap(), // Safe usage
});
}
}
8 changes: 4 additions & 4 deletions packages/@aws-cdk/aws-codebuild/test/project.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ describe('GitHub source', () => {

// WHEN
new codebuild.GitHubSourceCredentials(stack, 'GitHubSourceCredentials', {
accessToken: cdk.SecretValue.plainText('my-access-token'),
accessToken: cdk.SecretValue.unsafePlainText('my-access-token'),
});

// THEN
Expand Down Expand Up @@ -239,7 +239,7 @@ describe('GitHub Enterprise source', () => {

// WHEN
new codebuild.GitHubEnterpriseSourceCredentials(stack, 'GitHubEnterpriseSourceCredentials', {
accessToken: cdk.SecretValue.plainText('my-access-token'),
accessToken: cdk.SecretValue.unsafePlainText('my-access-token'),
});

// THEN
Expand Down Expand Up @@ -277,8 +277,8 @@ describe('BitBucket source', () => {

// WHEN
new codebuild.BitBucketSourceCredentials(stack, 'BitBucketSourceCredentials', {
username: cdk.SecretValue.plainText('my-username'),
password: cdk.SecretValue.plainText('password'),
username: cdk.SecretValue.unsafePlainText('my-username'),
password: cdk.SecretValue.unsafePlainText('password'),
});

// THEN
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,8 @@ export class AlexaSkillDeployAction extends Action {
return {
configuration: {
ClientId: this.props.clientId,
ClientSecret: this.props.clientSecret,
RefreshToken: this.props.refreshToken,
ClientSecret: this.props.clientSecret.unsafeUnwrap(), // Safe usage
RefreshToken: this.props.refreshToken.unsafeUnwrap(), // Safe usage
SkillId: this.props.skillId,
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ export class GitHubSourceAction extends Action {
new codepipeline.CfnWebhook(scope, 'WebhookResource', {
authentication: 'GITHUB_HMAC',
authenticationConfiguration: {
secretToken: this.props.oauthToken.toString(),
secretToken: this.props.oauthToken.unsafeUnwrap(), // Safe usage
},
filters: [
{
Expand All @@ -152,7 +152,7 @@ export class GitHubSourceAction extends Action {
Owner: this.props.owner,
Repo: this.props.repo,
Branch: this.props.branch || 'master',
OAuthToken: this.props.oauthToken.toString(),
OAuthToken: this.props.oauthToken.unsafeUnwrap(),
PollForSourceChanges: this.props.trigger === GitHubTrigger.POLL,
},
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -218,14 +218,14 @@ function addCodeDeployECSCodePipeline(stack: cdk.Stack, props: cpactions.CodeDep
new cpactions.GitHubSourceAction({
actionName: 'GitHub',
output: props.taskDefinitionTemplateInput || props.taskDefinitionTemplateFile!.artifact,
oauthToken: cdk.SecretValue.plainText('secret'),
oauthToken: cdk.SecretValue.unsafePlainText('secret'),
owner: 'awslabs',
repo: 'aws-cdk',
}),
new cpactions.GitHubSourceAction({
actionName: 'GitHub2',
output: props.appSpecTemplateInput || props.appSpecTemplateFile!.artifact,
oauthToken: cdk.SecretValue.plainText('secret'),
oauthToken: cdk.SecretValue.unsafePlainText('secret'),
owner: 'awslabs',
repo: 'aws-cdk-2',
}),
Expand Down
Loading

0 comments on commit 789e8d2

Please sign in to comment.