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(aws-s3): adds s3 bucket aws fsbp option #10970

Closed
wants to merge 14 commits into from
Closed

feat(aws-s3): adds s3 bucket aws fsbp option #10970

wants to merge 14 commits into from

Conversation

crashGoBoom
Copy link
Contributor

This adds an option to enforce aws foundational best practices for s3 buckets.

Closes #10969
Signed-off-by: Christopher Mundus chris@kindlyops.com

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

@gitpod-io
Copy link

gitpod-io bot commented Oct 19, 2020 •
edited
Loading

@mergify
Copy link
Contributor

mergify bot commented Oct 19, 2020

Title does not follow the guidelines of Conventional Commits. Please adjust title before merge.

@crashGoBoom
feat(aws-s3): adds s3 bucket AWS FSBP option
bd5b894 
This adds an option to enforce aws foundational best practices for s3 buckets.

Closes aws#10969
Signed-off-by: Christopher Mundus <chris@kindlyops.com>
@crashGoBoom crashGoBoom changed the title feat(aws-s3) adds s3 bucket AWS FSBP option feat(aws-s3): adds s3 bucket AWS FSBP option Oct 19, 2020
@crashGoBoom crashGoBoom changed the title feat(aws-s3): adds s3 bucket AWS FSBP option feat(aws-s3): adds s3 bucket aws fsbp option Oct 19, 2020
@NetaNir
Copy link
Contributor

NetaNir commented Nov 18, 2020

Hi @ crashGoBoom!

Generally we prefer to encode best practices as defaults in the L2 constructs. Individual properties, such as the the enforceSsl, should be exposed as properties and have sane defaults, allowing users to opt out from specific configurations. If you agree, I suggest adding enforceSsl as property and removing the enforceSecurityBestPractice property.

@NetaNir NetaNir self-requested a review November 18, 2020 19:30
NetaNir
NetaNir previously requested changes Nov 18, 2020
View reviewed changes
packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
@crashGoBoom
Refactor based on PR suggestions
72df7d2 
Removes enforceSecurityBestPractice property and only
uses SSL statement enforcement.

Signed-off-by: Christopher Mundus <chris@kindlyops.com>
@mergify mergify bot dismissed NetaNir’s stale review November 19, 2020 14:41

Pull request has been modified.

@crashGoBoom
Adds enforce ssl changes
419ef7c 
Signed-off-by: Christopher Mundus <chris@kindlyops.com>
@crashGoBoom
Merge branch 'master' into add_sec_helpers
9643368
NetaNir
NetaNir previously requested changes Nov 25, 2020
View reviewed changes
Copy link
Contributor

@NetaNir NetaNir left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good, one comment about the doc. Feel free to tag me so I can merge this once updated! Thanks!

packages/@aws-cdk/aws-s3/lib/bucket.ts Outdated Show resolved Hide resolved
@crashGoBoom
Refactor to update url for s3 documentation
191b3ba 
Signed-off-by: Christopher Mundus <chris@kindlyops.com>
@mergify mergify bot dismissed NetaNir’s stale review November 25, 2020 13:23

Pull request has been modified.

@crashGoBoom
Copy link
Contributor Author

Looks good, one comment about the doc. Feel free to tag me so I can merge this once updated! Thanks!

@NetaNir Hi! This has been updated. Let me know if there is anything else. Thanks so much!

@NetaNir NetaNir assigned NetaNir and unassigned iliapolo Dec 5, 2020
@NetaNir NetaNir added the pr-linter/exempt-readme The PR linter will not require README changes label Dec 5, 2020
@NetaNir
Copy link
Contributor

NetaNir commented Dec 5, 2020

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Dec 5, 2020

Command update: failure

Branch update failed
user doesn't have permission to update head repository
err-code: CB7F1

@NetaNir
Copy link
Contributor

NetaNir commented Dec 5, 2020

Hi @crashGoBoom see mergify comment above, you will need to update your branch so I can merge it.

@crashGoBoom
Copy link
Contributor Author

Hi @crashGoBoom see mergify comment above, you will need to update your branch so I can merge it.

Ok I just updated sorry about that! @NetaNir

NetaNir
NetaNir previously requested changes Dec 6, 2020
View reviewed changes
packages/@aws-cdk/aws-s3/test/bucket.test.ts Outdated Show resolved Hide resolved
@mergify mergify bot dismissed NetaNir’s stale review December 7, 2020 13:38

Pull request has been modified.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@crashGoBoom
Copy link
Contributor Author

@NetaNir ok looks like it should be ready to go now.

@mergify
Copy link
Contributor

mergify bot commented Dec 7, 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).

@crashGoBoom
Copy link
Contributor Author

Hmm, looks like this might have failed to allow access since this PR is coming from an ORG repo instead of a user repo. I am not seeing the options to allow changes to be pushed to this fork so I am guessing thats what it is. @NetaNir Any ideas if its possible to use this org fork or should this PR be closed and opened with my user instead of our org? Thanks and sorry for the hassle!

@crashGoBoom crashGoBoom mentioned this pull request Feb 1, 2021
Merged
@iliapolo
Copy link
Contributor

Closed in favor of #12804

@iliapolo iliapolo closed this Feb 21, 2021
mergify bot pushed a commit that referenced this pull request Feb 23, 2021
@crashGoBoom
feat(aws-s3): adds s3 bucket AWS FSBP option (#12804)
b9cdd52 
This adds an option to enforce ssl for s3 buckets.

Closes #10969

Signed-off-by: crashGoBoom <crashGoBoom@users.noreply.github.com>

Replaces the PR  #10970 as it was created with an ORG fork which is not compatible with the required option "Allow edits by maintainers". 

FYI @NetaNir 

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NetaNir NetaNir NetaNir approved these changes

Assignees

@NetaNir NetaNir

Labels
pr-linter/exempt-readme The PR linter will not require README changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[aws-s3] Enforce AWS Foundational Security Best Practice
4 participants
@crashGoBoom @NetaNir @aws-cdk-automation @iliapolo