fix(bootstrap): same-account modern bootstrapping still requires policy ARNs #9867

rix0rrr · 2020-08-20T12:20:53Z

The current bootstrapping experience has sharp edges. It requires you
to pass --cloudformation-execution-policies in all cases, even if
you just want a "simple", same-account bootstrap stack. If you bootstrap
your own accounts, you probably don't have a centralized SecOps
team that is limiting you, and you're probably intending to deploy
everything using the CDK, which means you're looking for AdministratorAccess.

In effect, you are forced to constantly Google and copy/paste the ARN
for AdministratorAccess (because who can remember that)
and it's a bad experience.

In the case of bootstrapping an account just for use "by itself",
however (which is the 90% use case for bootstrapping),
the trust boundary is very clear and there's no risk of privilege
escalation between accounts.

Add an optimization where in the case of a "simple", non-cross account
bootstrap, we'll default to the AdministratorAccess ARN for you.

Once you establish --trust with another account, we'll still
force you to spell out the execution policy you'll want to use though.

Fixes #8571.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

The current bootstrapping experience has sharp edges. It requires you to pass `--cloudformation-execution-policies`. If you don't pass any, it will successfully deploy a bootstrap stack that doesn't actually work. In effect, you are forced to constantly look up and copy/paste the ARN for `AdministratorAccess` (because that's effectively what people need to use anyway). We don't want to default to this policy for them, because it equates to handing over the keys to your account to another account, and you need to be fully aware that you're doing that. In the case of bootstrapping an account just for use "by itself", however (which is the 90% use case for bootstrapping), the trust boundary is very clear and there's no risk of privilege escalation between accounts (there might still be risk of privilege escalation between different people using different IAM users in the same AWS account, but that is not a recommended setup). Add an optimization where in the case of a "simple", non-cross account bootstrap, we'll default to the AdministratorAccess ARN for you. Once you establish `--trust` with another account, we'll still force you to spell out the execution policy you'll want to use though. Fixes #8571. Also in this PR: - The `blockPublicAccessConfiguration` would be reset on every re-bootstrap, instead of reusing the previous setting. - The `enableTerminationProtection` would be reset on every re-bootstrap, instead of reusing the previous setting.

hoegertn · 2020-08-20T14:46:21Z

@rix0rrr Do you have a timeline when this will be the default bootstrap when executing cdk bootstrap?

rix0rrr · 2020-08-20T15:04:08Z

@rix0rrr Do you have a timeline when this will be the default bootstrap when executing cdk bootstrap?

When "enough" people have tried it and used it that we can confidently recommend everyone use it in favor of the old way.

Also I would like to implement context queries and bake that for a while, and then if we haven't had issues in--say--a month, I'd feel comfortable flipping the switch.

hoegertn · 2020-08-20T15:08:09Z

@rix0rrr Do you have a timeline when this will be the default bootstrap when executing cdk bootstrap?

When "enough" people have tried it and used it that we can confidently recommend everyone use it in favor of the old way.

Also I would like to implement context queries and bake that for a while, and then if we haven't had issues in--say--a month, I'd feel comfortable flipping the switch.

What do you mean by "context queries"? I am using the new bootstrap and it is perfectly possible to use lookups. I thought only pipelines will not work but normal cdk deploy or synth work perfectly.

rix0rrr · 2020-08-20T15:52:43Z

Well the context queries don't use the new bootstrapping roles yet, which is mostly fine if you stay in-account but no longer fine once you try to do it cross-account.

njlynch

Approving as-is with two minor and one slightly less minor comment. Expecting these to be fixed before a second approval is given, so not explicitly tagging pr/do-not-merge.

njlynch · 2020-08-21T10:46:01Z

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

+  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>\'.`);


Suggested change

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>\'.`);

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>'.`);

njlynch · 2020-08-21T10:46:41Z

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

+    // Would leave AdministratorAccess policies with a trust relationship, without the user explicitly
+    // approving the trust policy.
+    const implicitPolicy = `arn:${await current.partition()}:iam::aws:policy/AdministratorAccess`;
+    warning(`Defaulting execution policy to '${implicitPolicy}'. Pass \'--cloudformation-execution-policies\' to customize.`);


Suggested change

warning(`Defaulting execution policy to '${implicitPolicy}'. Pass \'--cloudformation-execution-policies\' to customize.`);

warning(`Defaulting execution policy to '${implicitPolicy}'. Pass '--cloudformation-execution-policies' to customize.`);

njlynch · 2020-08-21T11:01:45Z

packages/aws-cdk/lib/api/bootstrap/deploy-bootstrap.ts

+    return deployStack({
+      stack: assembly.getStackByName(this.toolkitStackName),
+      resolvedEnvironment: this.resolvedEnvironment,
+      sdk: await this.sdkProvider.forEnvironment(this.resolvedEnvironment, Mode.ForWriting),


Suggested change

sdk: await this.sdkProvider.forEnvironment(this.resolvedEnvironment, Mode.ForWriting),

sdk: await this.sdk,

skinny85

I have no problems with this change from a security perspective.

@rix0rrr one question. Do I see correctly that this PR contains 2 changes? One is the "defaulting to AdminAccess" part, and the other is corrected validation that takes parameters into account when performing it?

If that's the case, any chance of splitting these into 2 PRs? It's pretty hard to understand exactly what's happening here... For example, I'm pretty sure the changes in toolkit-info.ts and cdk.ts are not related to the defaulting, but I'm not 100% sure...?

gitpod-io · 2020-10-23T08:21:39Z

rix0rrr · 2020-10-23T09:00:32Z

@rix0rrr one question. Do I see correctly that this PR contains 2 changes? One is the "defaulting to AdminAccess" part, and the other is corrected validation that takes parameters into account when performing it?

Yeah it's old, and I long ago extracted the other part into a separate PR. Updated it now.

skinny85

Overall looks fine, but some of the code is a little mysterious to me still.

skinny85 · 2020-10-23T18:42:07Z

packages/aws-cdk/lib/api/bootstrap/bootstrap-template.yaml

+            # The CLI will prevent this case from occurring
+            - Ref: AWS::NoValue
+            # The CLI will advertise that we picked this implicitly
+            - - Fn::Sub: "arn:${AWS::Partition}:iam::aws:policy/AdministratorAccess"


I hope you've verified this is the correct way to have a one-element array in YAML... this format is still a mystery to me.

I know. It is correct though:

- - Fn::Sub: '....' ▲ ▲ ▲ │ │ └─── This makes the list element an object │ │ │ └─── This starts a new list (because there's no bullet underneath it's a single element) │ └───── This is the Else branch of the { Fn::If: [Cond, Then, Else] }

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

skinny85 · 2020-10-23T18:55:10Z

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

+      // we inferred it or whether the user told us, and the sequence:
+      //
+      // $ cdk bootstrap
+      // $ cdk bootstrap --trust 1234


Sorry, I don't understand this comment... cdk bootstrap --trust 1234 should fail, right? Because we require passing --cloudformation-execution-policies when you pass --trust?

I think the missing piece of context here might be that:

$ cdk bootstrap --cloudformation-execution-policies arn:aws:.../BloopAccess $ cdk bootstrap --trust 1234

Should be valid, while:

$ cdk bootstrap $ cdk bootstrap --trust 1234

Should not.

Hmmm... I'm starting to get a little uneasy about how complicated this is becoming.

For example, the following sequence:

$ cdk bootstrap --trust 1234 --cloudformation-execution-policies SomeLimitedPolicy $ cdk bootstrap --trust 9876 --cloudformation-execution-policies AdminPolicy

Is it actually obvious to the user that with the second command they've just changed the trust to account 1234 from a limited policy to admin access?

packages/aws-cdk/lib/api/bootstrap/bootstrap-environment.ts

skinny85 · 2020-10-23T19:04:10Z

packages/aws-cdk/test/api/bootstrap2.test.ts

+        CloudFormationExecutionPolicies: '',
+      }),
+    }));
+  });


I feel like we're missing a test for the positive case (no --trust or --cloudformation-execution-policies passed) that checks the template? Maybe an integ test?

rix0rrr · 2020-10-28T14:32:12Z

Approved by AppSec

mergify · 2020-10-28T14:32:40Z

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-cdk-automation · 2020-10-28T15:00:01Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: cf5bd7f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2020-10-28T15:00:28Z

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).

rix0rrr added the pr/requires-two-approvers This PR is critical (e.g., security, broadly-impacting) and requires 2 approvers to be merged. label Aug 20, 2020

rix0rrr requested review from skinny85, eladb and a team August 20, 2020 12:20

rix0rrr self-assigned this Aug 20, 2020

mergify bot added the contribution/core This is a PR that came from AWS. label Aug 20, 2020

njlynch approved these changes Aug 21, 2020

View reviewed changes

Review fixes

6343e7c

rix0rrr added the pr/do-not-merge This PR should not be merged at this time. label Aug 25, 2020

rix0rrr requested a review from a team August 25, 2020 08:40

skinny85 reviewed Oct 8, 2020

View reviewed changes

Merge remote-tracking branch 'origin/master' into huijbers/auto-admin

dec74d5

rix0rrr added 2 commits October 23, 2020 10:46

Remember that we have to not actually reify the default

12cd1e5

oooone more test

f2c4861

rix0rrr changed the title ~~fix(bootstrap): default to Admin permissions for same-account access~~ fix(bootstrap): same-account modern bootstrapping still requires policy ARNs Oct 23, 2020

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Oct 23, 2020

rix0rrr requested a review from skinny85 October 23, 2020 09:00

skinny85 added the pr/do-not-merge This PR should not be merged at this time. label Oct 23, 2020

skinny85 reviewed Oct 23, 2020

View reviewed changes

rix0rrr added 2 commits October 26, 2020 09:38

Review comments

27d3314

Add integ test

680730c

rix0rrr requested a review from skinny85 October 26, 2020 09:15

rix0rrr removed the pr/do-not-merge This PR should not be merged at this time. label Oct 28, 2020

rix0rrr removed the pr/requires-two-approvers This PR is critical (e.g., security, broadly-impacting) and requires 2 approvers to be merged. label Oct 28, 2020

Merge branch 'master' into huijbers/auto-admin

cf5bd7f

mergify bot merged commit f5ab374 into master Oct 28, 2020

mergify bot deleted the huijbers/auto-admin branch October 28, 2020 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(bootstrap): same-account modern bootstrapping still requires policy ARNs #9867

fix(bootstrap): same-account modern bootstrapping still requires policy ARNs #9867

rix0rrr commented Aug 20, 2020 •

edited

hoegertn commented Aug 20, 2020

rix0rrr commented Aug 20, 2020

hoegertn commented Aug 20, 2020

rix0rrr commented Aug 20, 2020

njlynch left a comment

njlynch Aug 21, 2020

njlynch Aug 21, 2020

njlynch Aug 21, 2020

skinny85 left a comment

gitpod-io bot commented Oct 23, 2020 •

edited

rix0rrr commented Oct 23, 2020

skinny85 left a comment

skinny85 Oct 23, 2020

rix0rrr Oct 26, 2020

skinny85 Oct 23, 2020

rix0rrr Oct 26, 2020

skinny85 Oct 26, 2020

skinny85 Oct 23, 2020

rix0rrr commented Oct 28, 2020

mergify bot commented Oct 28, 2020

aws-cdk-automation commented Oct 28, 2020

mergify bot commented Oct 28, 2020

	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>\'.`);
	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>'.`);

	warning(`Defaulting execution policy to '${implicitPolicy}'. Pass \'--cloudformation-execution-policies\' to customize.`);
	warning(`Defaulting execution policy to '${implicitPolicy}'. Pass '--cloudformation-execution-policies' to customize.`);

	sdk: await this.sdkProvider.forEnvironment(this.resolvedEnvironment, Mode.ForWriting),
	sdk: await this.sdk,

fix(bootstrap): same-account modern bootstrapping still requires policy ARNs #9867

fix(bootstrap): same-account modern bootstrapping still requires policy ARNs #9867

Conversation

rix0rrr commented Aug 20, 2020 • edited

hoegertn commented Aug 20, 2020

rix0rrr commented Aug 20, 2020

hoegertn commented Aug 20, 2020

rix0rrr commented Aug 20, 2020

njlynch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skinny85 left a comment

Choose a reason for hiding this comment

gitpod-io bot commented Oct 23, 2020 • edited

rix0rrr commented Oct 23, 2020

skinny85 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rix0rrr commented Oct 28, 2020

mergify bot commented Oct 28, 2020

aws-cdk-automation commented Oct 28, 2020

AWS CodeBuild CI Report

mergify bot commented Oct 28, 2020

rix0rrr commented Aug 20, 2020 •

edited

gitpod-io bot commented Oct 23, 2020 •

edited