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.

Sign up for GitHub

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

Jump to bottom

feat(s3-deployment): implement new signContent option #24713

Merged
merged 2 commits into from
Apr 20, 2023
Merged

feat(s3-deployment): implement new signContent option #24713

merged 2 commits into from
Apr 20, 2023

Conversation

AMZN-hgoffin
Copy link
Contributor

This adds a new boolean option 'signPayload' to the BucketDeployment construct, so that it can be used with buckets whose IAM policy denies PutObject when x-amz-content-sha256 is not set properly. (See https://docs.aws.amazon.com/AmazonS3/latest/API/bucket-policy-s3-sigv4-conditions.html for example constructions that use this IAM condition key.)

Closes #24711.

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

@github-actions github-actions bot added feature-request A feature should be added or improved. p2 labels Mar 21, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team March 21, 2023 04:01
@github-actions github-actions bot added the beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK label Mar 21, 2023
aws-cdk-automation
aws-cdk-automation previously requested changes Mar 21, 2023
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

@AMZN-hgoffin AMZN-hgoffin changed the title feat(aws-s3-deployment: implement new option for signed content payload feat(aws-s3-deployment): implement new signContent option Mar 21, 2023
@AMZN-hgoffin AMZN-hgoffin changed the title feat(aws-s3-deployment): implement new signContent option feat(s3-deployment): implement new signContent option Mar 21, 2023
@AMZN-hgoffin
Copy link
Contributor Author

AMZN-hgoffin commented Mar 21, 2023 •
edited
Loading

(Removed keyword): I do not believe that this commit requires a new integration test because this feature is a property of the custom resource lambda and not a CloudFormation feature. The unit tests provide 100% coverage by

  1. verifying that this config option sets a new environment variable properly
  2. verifying that the python handler interprets the environment variable and turns it into an "aws configure set ..." command
  3. verifying that the "aws configure set" command is not executed by any other existing handler paths

@aws-cdk-automation aws-cdk-automation added the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 21, 2023
@corymhall
Copy link
Contributor

Exemption Request: I do not believe that this commit requires a new integration test because this feature is a property of the custom resource lambda and not a CloudFormation feature. The unit tests provide 100% coverage by

@AMZN-hgoffin because the feature is a property of the custom resource lambda is exactly why we do need an integration test. We need an integration test that creates a bucket with the specific bucket policy in order to confirm that these changes allow the user to write to the bucket.

@AMZN-hgoffin
Copy link
Contributor Author

Exemption Request: I do not believe that this commit requires a new integration test because this feature is a property of the custom resource lambda and not a CloudFormation feature. The unit tests provide 100% coverage by

@AMZN-hgoffin because the feature is a property of the custom resource lambda is exactly why we do need an integration test. We need an integration test that creates a bucket with the specific bucket policy in order to confirm that these changes allow the user to write to the bucket.

Understood, I will put together a complete integration test. I have entirely too much faith in the aws-cli and botocore test suites, I suppose :)

@aws-cdk-automation aws-cdk-automation removed the pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. label Mar 21, 2023
@AMZN-hgoffin AMZN-hgoffin force-pushed the signpayload branch 2 times, most recently from 91b2955 to 9136983 Compare March 21, 2023 16:23
@aws-cdk-automation aws-cdk-automation dismissed their stale review March 21, 2023 16:25

✅ Updated pull request passes all PRLinter validations. Dissmissing previous PRLinter review.

@AMZN-hgoffin AMZN-hgoffin force-pushed the signpayload branch 3 times, most recently from 569f96e to f4f9dec Compare March 22, 2023 09:32
TheRealAmazonKendra
TheRealAmazonKendra previously requested changes Mar 28, 2023
View reviewed changes
Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution! Please see my comments inline.

packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment-signcontent.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-s3-deployment/test/integ.bucket-deployment-signcontent.ts Outdated Show resolved Hide resolved
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment
@aws-cdk-automation
Copy link
Collaborator

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

@mergify mergify bot dismissed TheRealAmazonKendra’s stale review April 2, 2023 07:48

Pull request has been modified.

@AMZN-hgoffin
Copy link
Contributor Author

I think I've addressed the feedback with better comments in the integration test. I rebuilt the change with the new CDK directory structure. Please let me know if there's anything else required to get this functionality landed!

corymhall
corymhall previously approved these changes Apr 19, 2023
View reviewed changes
@mergify
Copy link
Contributor

mergify bot commented Apr 19, 2023

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

@mergify mergify bot dismissed corymhall’s stale review April 19, 2023 17:55

Pull request has been modified.

@AMZN-hgoffin
feat(s3-deployment): implement new signContent option
d5bf7b3 
IAM resource policy may prohibit PutObject calls with unsigned payloads.
This enables a new 'signContent: true' option to enable sigv4 signing of
a computed x-amz-content-sha256 header. The option directly corresponds
to the CLI option `aws configure set s3.payload_signing_enabled true`.
@AMZN-hgoffin
Copy link
Contributor Author

AMZN-hgoffin commented Apr 19, 2023 •
edited
Loading

Auto-merge was refusing to merge because of workflow configuration files being edited on main since my branch, and auto-merge not having permission to modify workflow files in my remote branch. I have rebased the change to latest main. Diff is identical.

@corymhall corymhall self-assigned this Apr 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2023

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

@gitpod-io
Copy link

gitpod-io bot commented Apr 20, 2023

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: 6ce67e9
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 5a836cb into aws:main Apr 20, 2023
@mergify
Copy link
Contributor

mergify bot commented Apr 20, 2023

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

@filletofish filletofish mentioned this pull request Jul 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@corymhall corymhall corymhall approved these changes

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

@TheRealAmazonKendra TheRealAmazonKendra Awaiting requested review from TheRealAmazonKendra

Assignees

@corymhall corymhall

Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK feature-request A feature should be added or improved. p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

aws-s3-deployment: support signed payloads with x-amz-content-sha256 header
4 participants
@AMZN-hgoffin @corymhall @aws-cdk-automation @TheRealAmazonKendra