-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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: custom resources log sensitive ResponseURL
field
#20899
Conversation
ResponseURL
inputsResponseURL
field
ResponseURL
fieldResponseURL
field
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.
Is there any way to automate this? Mainly this comes down to:
- How do we know this is all the places that need to be updated?
- How do we know future custom resources won't log the response url?
I've thought about that a little but nothing obvious came up. Do you have a good idea? I thought new CRs would mostly be created by looking at/copy pasting from existing ones, so that would get the fix in there. And provider framework-based CRs automatically get the fix because there it's code where I can enforce the user payload doesn't even contain the ResponseURL. For the rest... docs I guess, but I was very unsure where to put it. There are too many docs in too many scattered places that I'm not even sure someone would read it. And in the mean time, I'd rather just get the fix out. |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork). |
Thank you for contributing! Your pull request will be updated from main 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 |
@Mergifyio backport v1-main |
✅ Backports have been created
|
All custom resource implementations start by logging the input `event`, so that issues will be easier to diagnose. Part of the payload of this event is the `ResponseURL` field, which is a pre-signed S3 URL where CloudFormation expects the success/failure result of the Custom Resource. By logging the `ResponseURL` to CloudWatch, we open an attack vector where an attacker who has access to read CloudWatch URLs can race the Custom Resource implementation to write a fake response to the presigned S3 URL, thereby faking the result of the CR (making the deployment fail when it should have succeeded, or make it succeed when it should have failed). Remove the `ResponseURL` from all logging output, and have the Custom Resource Provider remove it from the Payload that gets sent to the user function as well. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit 6b4f92f) # Conflicts: # packages/@aws-cdk/aws-dynamodb/test/global-replicas-provisioned.integ.snapshot/asset.5d88959fad6bed204d22b24bf15826b8c7591c586a60a313e54f1948d9cdf80f/index.js # packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.lit.integ.snapshot/aws-ecs-integ-ecs.assets.json # packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.lit.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/app-mesh-proxy-config.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/app-mesh-proxy-config.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/bottlerocket.integ.snapshot/aws-ecs-integ-bottlerocket.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/bottlerocket.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/capacity-provider.integ.snapshot/integ-ec2-capacity-provider.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/capacity-provider.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/clb-host-nw.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/clb-host-nw.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/cloudmap-container-port.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/cloudmap-container-port.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/aws-ecs-integ.template.json # packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/exec-command.integ.snapshot/aws-ecs-integ-exec-command.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/exec-command.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/firelens-s3-config.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/firelens-s3-config.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/graviton-bottlerocket.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/graviton-bottlerocket.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/graviton.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/graviton.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/lb-awsvpc-nw.integ.snapshot/aws-ecs-integ.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/lb-awsvpc-nw.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/lb-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/lb-bridge-nw.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/sd-awsvpc-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/sd-awsvpc-nw.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/sd-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/sd-bridge-nw.integ.snapshot/tree.json # packages/@aws-cdk/aws-ecs/test/ec2/spot-drain.integ.snapshot/aws-ecs-integ-spot.assets.json # packages/@aws-cdk/aws-ecs/test/ec2/spot-drain.integ.snapshot/tree.json # packages/@aws-cdk/aws-events-targets/test/ecs/event-ec2-task.lit.integ.snapshot/aws-ecs-integ-ecs.assets.json # packages/@aws-cdk/aws-events-targets/test/ecs/event-ec2-task.lit.integ.snapshot/tree.json # packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-run-task.integ.snapshot/aws-sfn-tasks-ecs-ec2-integ.assets.json # packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-run-task.integ.snapshot/tree.json # packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-task.integ.snapshot/aws-ecs-integ2.assets.json # packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-task.integ.snapshot/tree.json # packages/aws-cdk/does-not-exist.json
Follow-up to #20899, missed a couple of spots. Marking this a `chore` instead of a `fix` since the previous commit will already show up in the CHANGELOG and both this and #20899 will go into the same release. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Follow-up to #20899, missed a couple of spots. Marking this a `chore` instead of a `fix` since the previous commit will already show up in the CHANGELOG and both this and #20899 will go into the same release. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* (cherry picked from commit d55ad0e) # Conflicts: # packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts # packages/@aws-cdk/pipelines/test/pipeline-security.integ.snapshot/PipelineSecurityStack.assets.json # packages/@aws-cdk/pipelines/test/pipeline-security.integ.snapshot/manifest.json
This is an automatic backport of pull request #20899 done by [Mergify](https://mergify.com). Cherry-pick of 6b4f92f has failed: ``` On branch mergify/bp/v1-main/pr-20899 Your branch is up to date with 'origin/v1-main'. You are currently cherry-picking commit 6b4f92f. (fix conflicts and run "git cherry-pick --continue") (use "git cherry-pick --skip" to skip this patch) (use "git cherry-pick --abort" to cancel the cherry-pick operation) Changes to be committed: modified: packages/@aws-cdk/aws-dynamodb/lib/replica-handler/index.ts modified: packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/manifest.json modified: packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.lit.integ.snapshot/aws-ecs-integ-ecs.template.json modified: packages/@aws-cdk/aws-ecs/lib/drain-hook/lambda-source/index.py modified: packages/@aws-cdk/aws-ecs/test/ec2/app-mesh-proxy-config.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/bottlerocket.integ.snapshot/aws-ecs-integ-bottlerocket.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/capacity-provider.integ.snapshot/integ-ec2-capacity-provider.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/clb-host-nw.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/cloudmap-container-port.integ.snapshot/aws-ecs-integ.template.json deleted: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/asset.8ad7bbf8be94e05d569da95ddb82511dcc959f25054825394cbb86028ccd1b6a.zip deleted: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/asset.be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824/__entrypoint__.js deleted: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/asset.be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824/index.d.ts deleted: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/asset.be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824/index.js deleted: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/asset.be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824/index.ts modified: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/manifest.json modified: packages/@aws-cdk/aws-ecs/test/ec2/exec-command.integ.snapshot/aws-ecs-integ-exec-command.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/firelens-s3-config.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/graviton-bottlerocket.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/graviton.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/lb-awsvpc-nw.integ.snapshot/aws-ecs-integ.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/lb-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/sd-awsvpc-nw.integ.snapshot/aws-ecs-integ-ecs.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/sd-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.template.json modified: packages/@aws-cdk/aws-ecs/test/ec2/spot-drain.integ.snapshot/aws-ecs-integ-spot.template.json modified: packages/@aws-cdk/aws-eks-legacy/lib/cluster-resource/index.py modified: packages/@aws-cdk/aws-eks-legacy/lib/helm-chart/index.py modified: packages/@aws-cdk/aws-eks-legacy/lib/k8s-resource/index.py modified: packages/@aws-cdk/aws-eks/lib/kubectl-handler/apply/__init__.py modified: packages/@aws-cdk/aws-eks/lib/kubectl-handler/get/__init__.py modified: packages/@aws-cdk/aws-eks/lib/kubectl-handler/helm/__init__.py modified: packages/@aws-cdk/aws-eks/lib/kubectl-handler/index.py modified: packages/@aws-cdk/aws-eks/lib/kubectl-handler/patch/__init__.py modified: packages/@aws-cdk/aws-events-targets/lib/aws-api-handler/index.ts modified: packages/@aws-cdk/aws-events-targets/test/ecs/event-ec2-task.lit.integ.snapshot/aws-ecs-integ-ecs.template.json modified: packages/@aws-cdk/aws-logs/lib/log-retention-provider/index.ts modified: packages/@aws-cdk/aws-stepfunctions-tasks/lib/eval-nodejs-handler/index.ts modified: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-run-task.integ.snapshot/aws-sfn-tasks-ecs-ec2-integ.template.json modified: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-task.integ.snapshot/aws-ecs-integ2.template.json modified: packages/@aws-cdk/custom-resources/lib/aws-custom-resource/runtime/index.ts modified: packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/cfn-response.ts modified: packages/@aws-cdk/custom-resources/lib/provider-framework/runtime/framework.ts modified: packages/@aws-cdk/integ-tests/lib/assertions/providers/lambda-handler/base.ts modified: packages/@aws-cdk/triggers/lib/lambda/index.ts Unmerged paths: (use "git add/rm <file>..." as appropriate to mark resolution) deleted by us: packages/@aws-cdk/aws-dynamodb/test/global-replicas-provisioned.integ.snapshot/asset.5d88959fad6bed204d22b24bf15826b8c7591c586a60a313e54f1948d9cdf80f/index.js deleted by us: packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs-patterns/test/ec2/multiple-application-load-balanced-ecs-service.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.lit.integ.snapshot/aws-ecs-integ-ecs.assets.json both modified: packages/@aws-cdk/aws-ecs-patterns/test/ec2/scheduled-ecs-task.lit.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/app-mesh-proxy-config.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/app-mesh-proxy-config.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/bottlerocket.integ.snapshot/aws-ecs-integ-bottlerocket.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/bottlerocket.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/capacity-provider.integ.snapshot/integ-ec2-capacity-provider.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/capacity-provider.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/clb-host-nw.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/clb-host-nw.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/cloudmap-container-port.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/cloudmap-container-port.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/aws-ecs-integ.template.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/environment-file.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/exec-command.integ.snapshot/aws-ecs-integ-exec-command.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/exec-command.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/firelens-s3-config.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/firelens-s3-config.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/graviton-bottlerocket.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/graviton-bottlerocket.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/graviton.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/graviton.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/lb-awsvpc-nw.integ.snapshot/aws-ecs-integ.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/lb-awsvpc-nw.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/lb-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/lb-bridge-nw.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/sd-awsvpc-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/sd-awsvpc-nw.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/sd-bridge-nw.integ.snapshot/aws-ecs-integ-ecs.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/sd-bridge-nw.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-ecs/test/ec2/spot-drain.integ.snapshot/aws-ecs-integ-spot.assets.json both modified: packages/@aws-cdk/aws-ecs/test/ec2/spot-drain.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-events-targets/test/ecs/event-ec2-task.lit.integ.snapshot/aws-ecs-integ-ecs.assets.json both modified: packages/@aws-cdk/aws-events-targets/test/ecs/event-ec2-task.lit.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-run-task.integ.snapshot/aws-sfn-tasks-ecs-ec2-integ.assets.json both modified: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-run-task.integ.snapshot/tree.json deleted by us: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-task.integ.snapshot/aws-ecs-integ2.assets.json both modified: packages/@aws-cdk/aws-stepfunctions-tasks/test/ecs/ec2-task.integ.snapshot/tree.json both modified: packages/aws-cdk/does-not-exist.json ``` To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally --- <details> <summary>Mergify commands and options</summary> <br /> More conditions and actions can be found in the [documentation](https://docs.mergify.com/). You can also trigger Mergify actions by commenting on this pull request: - `@Mergifyio refresh` will re-evaluate the rules - `@Mergifyio rebase` will rebase this PR on its base branch - `@Mergifyio update` will merge the base branch into this PR - `@Mergifyio backport <destination>` will backport this PR on `<destination>` branch Additionally, on Mergify [dashboard](https://dashboard.mergify.com/) you can: - look at your merge queues - generate the Mergify configuration with the config editor. Finally, you can contact us on https://mergify.com </details>
All custom resource implementations start by logging the input `event`, so that issues will be easier to diagnose. Part of the payload of this event is the `ResponseURL` field, which is a pre-signed S3 URL where CloudFormation expects the success/failure result of the Custom Resource. By logging the `ResponseURL` to CloudWatch, we open an attack vector where an attacker who has access to read CloudWatch URLs can race the Custom Resource implementation to write a fake response to the presigned S3 URL, thereby faking the result of the CR (making the deployment fail when it should have succeeded, or make it succeed when it should have failed). Remove the `ResponseURL` from all logging output, and have the Custom Resource Provider remove it from the Payload that gets sent to the user function as well. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Follow-up to aws#20899, missed a couple of spots. Marking this a `chore` instead of a `fix` since the previous commit will already show up in the CHANGELOG and both this and aws#20899 will go into the same release. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
See aws/aws-cdk#20899 for details
See aws/aws-cdk#20899 for details
All custom resource implementations start by logging the input
event
, so that issues will be easier to diagnose.Part of the payload of this event is the
ResponseURL
field, which is a pre-signed S3 URL where CloudFormation expects the success/failure result of the Custom Resource. By logging theResponseURL
to CloudWatch, we open an attack vector where an attacker who has access to read CloudWatch URLs can race the Custom Resource implementation to write a fake response to the presigned S3 URL, thereby faking the result of the CR (making the deployment fail when it should have succeeded, or make it succeed when it should have failed).Remove the
ResponseURL
from all logging output, and have the Custom Resource Provider remove it from the Payload that gets sent to the user function as well.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license