Skip to content

[Feedback requested] New rule on S3 has VPC endpoint enabled. #130

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

Closed
jongogogo opened this issue Feb 25, 2019 · 12 comments
Closed

[Feedback requested] New rule on S3 has VPC endpoint enabled. #130

jongogogo opened this issue Feb 25, 2019 · 12 comments
Labels
feedback requested This rule has detailed requirements ready for in-depth review

Comments

@jongogogo
Copy link
Contributor

jongogogo commented Feb 25, 2019

New rule to verify that the S3 has VPC endpoint enabled.

Please provide comments on the gherkin if any.


Description
   Check whether VPC Endpoint is enabled for each VPC to access Amazon S3.

Trigger
   Periodic (because VPC Configuration Item does not record Endpoint status)

Reports on:
   AWS::EC2::VPC

Rule Parameters:
   None

Scenarios:
  Scenario 1:
  Given: No VPC is present
   Then: Return NOT_APPLICABLE

  Scenario 2:
  Given: At least one VPC is present
    And: The S3 service is not present in the list of "ServiceName" on DescribeVpcEndpoints API
   Then: Return NON_COMPLIANT on this VPC

  Scenario 3:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is not "Available"
   Then: Return NON_COMPLIANT on this VPC

  Scenario 4:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is "Available"
   Then: Return COMPLIANT on this VPC

DescribeVpcEndpoints API doc

@jongogogo
Copy link
Contributor Author

@dboyd13 following your request.

@jongogogo jongogogo added the feedback requested This rule has detailed requirements ready for in-depth review label Mar 23, 2019
@bksarthak
Copy link
Contributor

Description
   Check whether VPC Endpoint is enabled for each VPC to access Amazon S3.

Trigger
   Periodic (because VPC Configuration Item does not record Endpoint status)

Reports on:
   AWS::::Account

Rule Parameters:
   None

Scenarios:
  Scenario 1:
  Given: No VPC endpoints are present
   Then: Return NOT_APPLICABLE

  Scenario 2:
  Given: At least one VPC is present
    And: The S3 service is not present in the list of "ServiceName" on DescribeVpcEndpoints API
   Then: Return NON_COMPLIANT for the account

  Scenario 3:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is not "Available"
   Then: Return NON_COMPLIANT for the account

  Scenario 4:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is "Available"
   Then: Return COMPLIANT for the account

Suggesting this version of the Gherkin for the check. Also wanted to check if there. Please confirm if this looks correct.

@jongogogo
Copy link
Contributor Author

The goal is to check that all VPCs have a S3 endpoint.

The problem with the Gherkin you posted is:

  • You don't address directly the "per-VPC" high level requirement.
  • Scenario 1 is not making much sense in the context of the high-level requirements. It would actually be NON_COMPLIANT on every VPC.

I'd suggest to comment on the original Gherkin first.

@bksarthak
Copy link
Contributor

New rule to verify that the S3 has VPC endpoint enabled.

Please provide comments on the gherkin if any.

Description
   Check whether VPC Endpoint is enabled for each VPC to access Amazon S3.

Trigger
   Periodic (because VPC Configuration Item does not record Endpoint status)

Reports on:
   AWS::EC2::VPC

Rule Parameters:
   None

Scenarios:
  Scenario 1:
  Given: No VPC is present
   Then: Return NOT_APPLICABLE

  Scenario 2:
  Given: At least one VPC is present
    And: The S3 service is not present in the list of "ServiceName" on DescribeVpcEndpoints API
   Then: Return NON_COMPLIANT on this VPC

  Scenario 3:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is not "Available"
   Then: Return NON_COMPLIANT on this VPC

  Scenario 4:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is "Available"
   Then: Return COMPLIANT on this VPC

DescribeVpcEndpoints API doc

I'd need one more API call to make this work. I'm planning to do a describe_vpcs() so that I can create a list of VPCs and then compare with describe_vpc_endpoints() Vpc values.

@jongogogo
Copy link
Contributor Author

Good with that

@bksarthak
Copy link
Contributor

New rule to verify that the S3 has VPC endpoint enabled.

Please provide comments on the gherkin if any.

Description
   Check whether VPC Endpoint is enabled for each VPC to access Amazon S3.

Trigger
   Periodic (because VPC Configuration Item does not record Endpoint status)

Reports on:
   AWS::EC2::VPC

Rule Parameters:
   None

Scenarios:
  Scenario 1:
  Given: No VPC is present
   Then: Return NOT_APPLICABLE

  Scenario 2:
  Given: At least one VPC is present
    And: The S3 service is not present in the list of "ServiceName" on DescribeVpcEndpoints API
   Then: Return NON_COMPLIANT on this VPC

  Scenario 3:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is not "Available"
   Then: Return NON_COMPLIANT on this VPC

  Scenario 4:
  Given: At least one VPC is present
    And: The S3 service is present in the "ServiceName" key on DescribeVpcEndpoints API
    And: The "VpcEndpointState" key value is "Available"
   Then: Return COMPLIANT on this VPC

DescribeVpcEndpoints API doc

Small edit to the first Gherkin scenario
Given: No VPC is present while using the describe_vpcs() API call

Code is ready and working as expected.

@bksarthak
Copy link
Contributor

Code and Unit tests ready and tested. The code has one minor issue on Either all return statements in a function should return an expression, or none of them should. (inconsistent-return-statements)
Would need some help on fixing this linting issue

Current Lint score -- 9.91/10 (code file); 10/10 (unit test file)

Please advise if I should initiate a PR.

@shikharj05
Copy link

@bksarthak If you have the code ready, you can start a PR. We will review it. I can help you with the pylint score, you can refer this issue: pylint-dev/pylint#1267 or check this site for inconsistent-return-statements: http://pylint.pycqa.org/en/latest/whatsnew/1.8.html

If nothing helps, we can work over it via the PR.

@shikharj05
Copy link

@jongogogo @bksarthak I would like to add an additional check to ensure at least one Route table is associated with the Amazon S3 endpoint. We can get it easily from the 'RouteTableIds' key in the output of describe_vpc_endpoints() API call.
Let me know your thoughts on this.

@bksarthak
Copy link
Contributor

bksarthak commented Apr 11, 2019

I think adding that check doesn't really check anything special apart from seeing the route table is attached or not. Considering the describe_vpc_endpoints() just gives rtb name and not if there are any subnets attached to that rtb, the rtb can be associated to zero subnets and yet pass the test.
@jongogogo let me know if this makes sense?

@jongogogo @bksarthak I would like to add an additional check to ensure at least one Route table is associated with the Amazon S3 endpoint. We can get it easily from the 'RouteTableIds' key in the output of describe_vpc_endpoints() API call.
Let me know your thoughts on this.

@jongogogo
Copy link
Contributor Author

#194

@jongogogo
Copy link
Contributor Author

@shikharj05 @bksarthak it is a good idea for a new rule. I would not put it into this one. It creates too much additional checks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback requested This rule has detailed requirements ready for in-depth review
Projects
None yet
Development

No branches or pull requests

3 participants