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

[aws-s3] Enforce AWS Foundational Security Best Practice #10969

Assignees
Labels
@aws-cdk/aws-s3 Related to Amazon S3 effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. in-progress This issue is being actively worked on. p2

Comments

@crashGoBoom
Copy link
Contributor

It would be nice to have the ability to enforce AWS Foundational Security Best Practice through CDK.

Use Case

When creating s3 buckets AWS FSBP should be followed.

Proposed Solution

      new s3.Bucket(stack, 'MyBucket', {
        enforceSecurityBestPractice: true,
      });

And behind the scenes it will block all public access and enforce data transmission over HTTPS.

A PR is ready to go which introduces a simple way of adding these controls for s3 buckets.

Other

This is really just to get the conversation started around enforcing the AWS FSBP controls as a default for CDK and perhaps this could be a first step into helping prevent AWS users from using defaults which are not secure.

  • 👋 I may be able to implement this feature request
  • ⚠️ This feature might incur a breaking change

This is a 🚀 Feature Request

@crashGoBoom crashGoBoom added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Oct 19, 2020
@github-actions github-actions bot added the @aws-cdk/aws-s3 Related to Amazon S3 label Oct 19, 2020
crashGoBoom added a commit to kindlyops/aws-cdk that referenced this issue Oct 19, 2020
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 added a commit to kindlyops/aws-cdk that referenced this issue Oct 19, 2020
This adds an option to enforce aws foundational best practices for s3 buckets.

Closes aws#10969
Signed-off-by: Christopher Mundus <chris@kindlyops.com>
@hoegertn
Copy link
Contributor

Instead of adding enforcement options to all resources I would love to have an aspect for this that traverses all resources in the stack.

@iliapolo iliapolo added effort/medium Medium work item – several days of effort p2 and removed needs-triage This issue or PR still needs to be triaged. labels Oct 20, 2020
@SomayaB SomayaB added the in-progress This issue is being actively worked on. label Oct 21, 2020
@crashGoBoom
Copy link
Contributor Author

crashGoBoom commented Oct 27, 2020

Instead of adding enforcement options to all resources I would love to have an aspect for this that traverses all resources in the stack.

This is a great idea and I think a good way to handle this may be by using Aspects. Here is a rough sketch:

class AWSFSBPChecker implements IAspect {
  public visit(node: IConstruct): void {
    // See that we're dealing with a CfnBucket
    if (node instanceof s3.CfnBucket) {
      // Check for compliance
      checkS3Compliance(node)
    }
  }
}

// Apply to the stack if AWS Foundational Security Best Practice is enabled
if(optionEnabled) {
  stack.node.applyAspect(new AWSFSBPChecker());
}

This would allow for a more flexible approach and update the checks in one spot as opposed to the individual resource. Feel free to let me know if you think using Aspects would work out or if there are any limitations you may know of and I will update my PR. Thanks!

@NetaNir
Copy link
Contributor

NetaNir commented Nov 18, 2020

Commented on the PR. I suggest we add the enforceSsl and remove the enforceSecurityBestPractice (see reasoning on the PR)
Aspect could definitely be used for this but Im not sure how will it look in the CDK (as opposed to in customer code or a higher level construct), will it be added as a separate capability like Tags? Would love to hear more about the API you are thinking of.

@NetaNir NetaNir added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 18, 2020
@crashGoBoom
Copy link
Contributor Author

Commented on the PR. I suggest we add the enforceSsl and remove the enforceSecurityBestPractice (see reasoning on the PR)
Aspect could definitely be used for this but Im not sure how will it look in the CDK (as opposed to in customer code or a higher level construct), will it be added as a separate capability like Tags? Would love to hear more about the API you are thinking of.

Hi! I updated my PR for the s3 bucket specific enforceSSL option. You raise some good questions as far as how this would be implemented across CDK. My original thought was it would be enabled per resource that is addressed with the AWS Foundational Security Best Practice with a public property along the lines of enforceSecurityBestPractice. Which in the case of S3 includes encryption, enforcing ssl requests and blocking public access. This would require adding the applicable enforceSecurityBestPractice property to each corresponding resource that falls under the AWS FSBP.

I am not 100% sure how Aspects would come into play besides maybe checking for compliance of the AWS FSBP and perhaps throwing an error or warning. I have some test code that checks for s3 compliance working here and it would only be a matter of figuring out where this code would live and how this gets enabled (assuming cli flag or maybe we change the error to warnings and it can be enabled by default?):

import { Annotations, IAspect, IConstruct, Tokenization } from "@aws-cdk/core";
import * as s3 from "@aws-cdk/aws-s3";

export class AWSFSBPChecker implements IAspect {
    public visit(node: IConstruct): void {
      // See that we're dealing with a CfnBucket
      if (node instanceof s3.CfnBucket) {
        // Check for compliance
        this.checkS3Compliance(node)
      }
    }

    private checkS3Compliance(node: s3.CfnBucket ) {
        // Check for encryption

        // https://docs.aws.amazon.com/securityhub/latest/userguide/securityhub-standards-fsbp-controls.html#fsbp-s3-4
        if (!node.bucketEncryption
                || (!Tokenization.isResolvable(node.bucketEncryption)
                && !node.bucketEncryption.serverSideEncryptionConfiguration)) {
            Annotations.of(node).addError('Bucket encryption is not enabled');
        }
        // Check for public access
        // https://docs.aws.amazon.com/securityhub/latest/userguide/securityhub-standards-fsbp-controls.html#fsbp-s3-1
        if (!node.publicAccessBlockConfiguration
            || (!Tokenization.isResolvable(node.publicAccessBlockConfiguration)
            && !node.publicAccessBlockConfiguration.blockPublicAcls)) {
            Annotations.of(node).addError('Bucket blockPublicAcls is not enabled');
        }
        if (!node.publicAccessBlockConfiguration
            || (!Tokenization.isResolvable(node.publicAccessBlockConfiguration)
            && !node.publicAccessBlockConfiguration.blockPublicPolicy)) {
            Annotations.of(node).addError('Bucket blockPublicPolicy is not enabled');
        }
        if (!node.publicAccessBlockConfiguration
            || (!Tokenization.isResolvable(node.publicAccessBlockConfiguration)
            && !node.publicAccessBlockConfiguration.ignorePublicAcls)) {
            Annotations.of(node).addError('Bucket ignorePublicAcls is not enabled');
        }
        if (!node.publicAccessBlockConfiguration
            || (!Tokenization.isResolvable(node.publicAccessBlockConfiguration)
            && !node.publicAccessBlockConfiguration.restrictPublicBuckets)) {
            Annotations.of(node).addError('Bucket restrictPublicBuckets is not enabled');
        }

    }
}

// Check stack for foundational security best practices if cli flag/config option is enabled
Aspects.of(someStack).add(new AWSFSBPChecker());

That being said I totally understand if the team feels that this is out of scope for CDK to provide these kinds of helpers. 👍

@github-actions github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Nov 20, 2020
@iliapolo iliapolo added effort/medium Medium work item – several days of effort feature-request A feature should be added or improved. p2 labels Nov 25, 2020
@mergify mergify bot closed this as completed in #12804 Feb 23, 2021
mergify bot pushed a commit that referenced this issue Feb 23, 2021
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*
@github-actions
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

This was referenced Mar 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment