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.

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

fix(bootstrapping): --cloudformation-execution-policies not checked #10337

Merged
merged 5 commits into from
Sep 14, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions packages/aws-cdk/bin/cdk.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,8 @@ async function parseCommandLineArguments() {
.option('public-access-block-configuration', { type: 'boolean', desc: 'Block public access configuration on CDK toolkit bucket (enabled by default) ', default: undefined })
.option('tags', { type: 'array', alias: 't', desc: 'Tags to add for the stack (KEY=VALUE)', nargs: 1, requiresArg: true, default: [] })
.option('execute', { type: 'boolean', desc: 'Whether to execute ChangeSet (--no-execute will NOT execute the ChangeSet)', default: true })
.option('trust', { type: 'array', desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated)', default: [], nargs: 1, requiresArg: true, hidden: true })
.option('cloudformation-execution-policies', { type: 'array', desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment. Required if --trust was passed (may be repeated)', default: [], nargs: 1, requiresArg: true, hidden: true })
.option('trust', { type: 'array', desc: 'The AWS account IDs that should be trusted to perform deployments into this environment (may be repeated, modern bootstrapping only)', default: [], nargs: 1, requiresArg: true })
.option('cloudformation-execution-policies', { type: 'array', desc: 'The Managed Policy ARNs that should be attached to the role performing deployments into this environment (may be repeated, modern bootstrapping only)', default: [], nargs: 1, requiresArg: true })
.option('force', { alias: 'f', type: 'boolean', desc: 'Always bootstrap even if it would downgrade template version', default: false })
.option('termination-protection', { type: 'boolean', default: undefined, desc: 'Toggle CloudFormation termination protection on the bootstrap stacks' })
.option('show-template', { type: 'boolean', desc: 'Instead of actual bootstrapping, print the current CLI\'s bootstrapping template to stdout for customization.', default: false })
Expand Down
6 changes: 4 additions & 2 deletions packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,8 +88,10 @@ export class Bootstrapper {
const trustedAccounts = params.trustedAccounts ?? current.parameters.TrustedAccounts?.split(',') ?? [];
const cloudFormationExecutionPolicies = params.cloudFormationExecutionPolicies ?? current.parameters.CloudFormationExecutionPolicies?.split(',') ?? [];

if (trustedAccounts.length > 0 && cloudFormationExecutionPolicies.length === 0) {
throw new Error(`You need to pass '--cloudformation-execution-policies' when trusting other accounts using '--trust' (${trustedAccounts}). Try a managed policy of the form 'arn:aws:iam::aws:policy/<PolicyName>'.`);
// To prevent user errors, require --cfn-exec-policies always
// (Hopefully until we get https://github.com/aws/aws-cdk/pull/9867 approved)
if (cloudFormationExecutionPolicies.length === 0) {
throw new Error('Please pass \'--cloudformation-execution-policies\' to specify deployment permissions. Try a managed policy of the form \'arn:aws:iam::aws:policy/<PolicyName>\'.');
}
// Remind people what the current settings are
info(`Trusted accounts: ${trustedAccounts.length > 0 ? trustedAccounts.join(', ') : '(none)'}`);
Expand Down
35 changes: 30 additions & 5 deletions packages/aws-cdk/test/api/bootstrap2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ describe('Bootstrapping v2', () => {
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
bucketName: 'my-bucket-name',
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

Expand All @@ -46,6 +47,7 @@ describe('Bootstrapping v2', () => {
test('passes the KMS key ID as a CFN parameter', async () => {
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
kmsKeyId: 'my-kms-key-id',
},
});
Expand All @@ -61,6 +63,7 @@ describe('Bootstrapping v2', () => {
test('passes false to PublicAccessBlockConfiguration', async () => {
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
publicAccessBlockConfiguration: false,
},
});
Expand All @@ -79,7 +82,7 @@ describe('Bootstrapping v2', () => {
},
}))
.rejects
.toThrow(/--cloudformation-execution-policies.*--trust/);
.toThrow(/--cloudformation-execution-policies/);
});

test('allow adding trusted account if there was already a policy on the stack', async () => {
Expand All @@ -104,7 +107,11 @@ describe('Bootstrapping v2', () => {
version: 999,
};

await expect(bootstrapper.bootstrapEnvironment(env, sdk, {}))
await expect(bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
}))
.rejects.toThrow('Not downgrading existing bootstrap stack');
});

Expand All @@ -114,7 +121,11 @@ describe('Bootstrapping v2', () => {
template = args.stack.template;
});

await bootstrapper.bootstrapEnvironment(env, sdk, {});
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

const exports = Object.values(template.Outputs ?? {})
.filter((o: any) => o.Export !== undefined)
Expand All @@ -127,7 +138,11 @@ describe('Bootstrapping v2', () => {
});

test('stack is not termination protected by default', async () => {
await bootstrapper.bootstrapEnvironment(env, sdk);
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({
stack: expect.objectContaining({
Expand All @@ -139,6 +154,9 @@ describe('Bootstrapping v2', () => {
test('stack is termination protected when option is set', async () => {
await bootstrapper.bootstrapEnvironment(env, sdk, {
terminationProtection: true,
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({
Expand All @@ -153,7 +171,11 @@ describe('Bootstrapping v2', () => {
EnableTerminationProtection: true,
});

await bootstrapper.bootstrapEnvironment(env, sdk, {});
await bootstrapper.bootstrapEnvironment(env, sdk, {
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({
stack: expect.objectContaining({
Expand All @@ -169,6 +191,9 @@ describe('Bootstrapping v2', () => {

await bootstrapper.bootstrapEnvironment(env, sdk, {
terminationProtection: false,
parameters: {
cloudFormationExecutionPolicies: ['arn:policy'],
},
});

expect(mockDeployStack).toHaveBeenCalledWith(expect.objectContaining({
Expand Down
3 changes: 2 additions & 1 deletion packages/aws-cdk/test/integ/cli/bootstrapping.integtest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ integTest('upgrade legacy bootstrap stack to new bootstrap stack while in use',
await fixture.cdk(['bootstrap',
'--toolkit-stack-name', bootstrapStackName,
'--bootstrap-bucket-name', newBootstrapBucketName,
'--qualifier', fixture.qualifier], {
'--qualifier', fixture.qualifier,
'--cloudformation-execution-policies', 'arn:aws:iam::aws:policy/AdministratorAccess'], {
modEnv: {
CDK_NEW_BOOTSTRAP: '1',
},
Expand Down