-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Conversation
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]`
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.
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. |
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.
Do we really need this to be validated both by CFN and the CLI? Maybe we can omit the CLI now?
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.
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.
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). |
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:
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 toPrincipal: { AWS: "arn:aws:iam::123456789012:root" }
,and
arn:aws:iam::123456789012:root
is a principal which is treatedin 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 combinationwith the principal
*
: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 sogets 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 versionchecking to the template, but that doesn't actually work for 2 reasons:
Fn::ImportValue
can only occur in a limited set of positions inthe CloudFormation template.
Export
is used by a Stack, it cannot be changed anymore. Thatmeans 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 appropriatelyin 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, deploymentswill 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