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(core): Access Denied using legacy synthesizer with new bootstrap #9831

Merged
merged 10 commits into from
Aug 20, 2020

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Aug 19, 2020

We always intended the FileAsset KMS Key to be transparently
usable by any IAM identity allowed to read from and write to the
FileAsset Bucket. We incorrectly implemented this, however.

We used to use the following key policy:

     - Action: [...]
       Principal: { AWS: "123456789012" }
       Condition:
         StringEquals:
           kms:ViaService: Fn::Sub: s3.${AWS::Region}.amazonaws.com

And this was intended to mean "any identity from the given account". That is not how KMS interprets it, though.

Principal: { AWS: "123456789012" } is equivalent to Principal: { AWS: "arn:aws:iam::123456789012:root" },
and arn:aws:iam::123456789012:root is a principal which is treated
in a special way by KMS, and it means "use the caller's IAM Identity Policy instead".

So while I was under the impression that it was strictly necessary for
KMS usage permissions to exist both on the key and on the identity, this
is only true if you use the arn:aws:iam::123456789012:root principal.

The correct way to express the condition we had intended to express was
instead to use a condition called kms:CallerAccount in combination
with the principal *:

     - Action: [...]
       Principal: { AWS: "*" }
       Condition:
         StringEquals:
           kms:CallerAccount: "123456789012"
           kms:ViaService: Fn::Sub: s3.${AWS::Region}.amazonaws.com

This PR changes the key policy in the bootstrap resources to use the
policy that we always had intended. This now gets rid of the requirement
for IAM identities to list kms:Decrypt in their role policy, and so
gets rid of the requirement for them to know the KMS key ARN.

This makes the stack synthesized by the legacy stack synthesizer work
with the new bootstrap stack, and also removes the need for the new
synthesizer to import the KMS key ARN using Fn::ImportValue.


However, the new stack synthesizer does now require that you have
the newest bootstrap stack installed, and since templates are likely
deployed using a pipeline, the CLI is not involved to do the
MINIMUM_BOOTSTRAP_STACK version check.

Originally I had intended to use the version Export to add version
checking to the template, but that doesn't actually work for 2 reasons:

  • Fn::ImportValue can only occur in a limited set of positions in
    the CloudFormation template.
  • If an Export is used by a Stack, it cannot be changed anymore. That
    means that even if we had done the check using Fn::ImportValue,
    users wouldn't have been allowed to update the bootstrap stack
    anymore.

What we should have done from the start, and what this PR
introduces, is storing the bootstrap stack version in an SSM Parameter
Store Parameter. This value can be inspected in a CloudFormation
Rules section, which will produce a readable error message
about why the template cannot be deployed.

Any assertion failure reasons will be reported on a
ROLLBACK_IN_PROGRESS event, so classify those appropriately
in the stack monitor so the error message gets displayed.

Fixes #9607.

BREAKING CHANGE: (cdk-pipelines) users of CDK Pipelines (and other users
of the new stack synthesizer) will need to update their bootstrap stack
by running cdk bootstrap with the new CLI. Until they do, deployments
will fail with the error: Unable to fetch parameters [/aws-cdk-bootstrap/hnb659fds/version]


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

We always intended the FileAsset KMS Key to be transparently
usable by any IAM identity allowed to read from and write to the
FileAsset Bucket. We incorrectly implemented this, however.

We used to use the following key policy:

```
     - Action: [...]
       Principal: { AWS: "123456789012" }
       Condition:
         StringEquals:
           kms:ViaService: Fn::Sub: s3.${AWS::Region}.amazonaws.com
```

And this was intended to mean "any identity from the given account". That is *not* how KMS interprets it, though.

`Principal: { AWS: "123456789012" }` is equivalent to `Principal: { AWS: "arn:aws:iam::123456789012:root" }`,
and `arn:aws:iam::123456789012:root` is a principal which is treated
in a special way by KMS, and it means "look at IAM Identity Policy of the requester instead".

So while I was under the impression that it was strictly necessary for
KMS usage permissions to exist both on the key and on the identity, this
is only true if you use the `arn:aws:iam::123456789012:root` principal.

The correct way to express the condition we had intended to express was
instead to use a condition called `kms:CallerAccount` in combination
with the principal `*`:

```
     - Action: [...]
       Principal: { AWS: "*" }
       Condition:
         StringEquals:
           kms:CallerAccount: "123456789012"
           kms:ViaService: Fn::Sub: s3.${AWS::Region}.amazonaws.com
```

This PR changes the key policy in the bootstrap resources to use the
policy that we always had intended. This now gets rid of the requirement
for IAM identities to list `kms:Decrypt` in their role policy, and so
gets rid of the requirement for them to know the KMS key ARN.

This makes the stack synthesized by the legacy stack synthesizer work
with the new bootstrap stack, and also removes the need for the new
synthesizer to import the KMS key ARN using `Fn::ImportValue`.

---

However, the new stack synthesizer *does* now require that you have
the newest bootstrap stack installed, and since templates are likely
deployed using a pipeline, the CLI is not involved to do the
`MINIMUM_BOOTSTRAP_STACK` version check.

Originally I had intended to use the version `Export` to add version
checking to the template, but that doesn't actually work for 2 reasons:

- `Fn::ImportValue` can only occur in a limited set of positions in
  the CloudFormation template.
- If an `Export` is used by a Stack, it cannot be changed anymore. That
  means that even if we had done the check using `Fn::ImportValue`,
  users wouldn't have been allowed to update the bootstrap stack
  anymore.

What we should have done from the start, and what this PR
introduces, is storing the bootstrap stack version in an SSM Parameter
Store Parameter. This value can be inspected in a CloudFormation
**Rules** section, which will produce a readable error message
about why the template cannot be deployed.

Any assertion failure reasons will be reported on a
`ROLLBACK_IN_PROGRESS` event, so classify those appropriately
in the stack monitor so the error message gets displayed.

Fixes#9607.

BREAKING CHANGES(cdk-pipelines): users of CDK Pipelines (and other users
of the new stack synthesizer) will need to update their bootstrap stack
by running `cdk bootstrap` with the new CLI. Until they do, deployments
will fail with the error: `Unable to fetch parameters
[/CdkBootstrap/hnb659fds/Version]`
@rix0rrr 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 19, 2020
@rix0rrr rix0rrr requested review from skinny85, eladb and a team August 19, 2020 13:13
@rix0rrr rix0rrr self-assigned this Aug 19, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Aug 19, 2020
@rix0rrr rix0rrr requested a review from NetaNir August 19, 2020 13:26
Copy link
Contributor

@eladb eladb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should synthesize the bootstrap template from a CDK app (still maintain it as a template and use L1s, but I don't see a good reason not to use CDK to define it and avoid duplicates and stuff).

@@ -373,10 +388,9 @@ Outputs:
Description: The name of the ECR repository which hosts docker image assets
Value:
Fn::Sub: "${ContainerAssetsRepository}"
# The Output is used by the CLI to verify the version of the bootstrap resources.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this to be validated both by CFN and the CLI? Maybe we can omit the CLI now?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI also does the check before uploading assets and, in the future, before querying context.

Even if we start reading the SSM parameter today (which is not a bad idea) we'll still need the the CLI to support old bootstrap stacks.

@rix0rrr rix0rrr requested a review from a team August 19, 2020 14:15
@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2020

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify
Copy link
Contributor

mergify bot commented Aug 20, 2020

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

@mergify mergify bot merged commit 960ef12 into master Aug 20, 2020
@mergify mergify bot deleted the huijbers/proper-bootstrap-key-policy branch August 20, 2020 14:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. pr/requires-two-approvers This PR is critical (e.g., security, broadly-impacting) and requires 2 approvers to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[cdk-pipelines] shellable and s3-deployments in a legacy stack don't work with new bootstrap resources
4 participants