cli

cli copied to clipboard

Description : I am testing the example for "Only selected ports should be publicly open", where the engine checks for only a comma seperated list of ports allowed to a cidr. The rule is incorrectly failing when a rule exists for an allowed 2 digit port, and the rule is failing with an assertion error when a 3 digit port is included in the list of ports.

To Reproduce 1.

resource "aws_security_group" "allow_tls_ingress_inline" {
  name        = "allow_tls_ingress_inline"

  ingress {
    from_port   = 80
    to_port     = 80
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]
  }

  ingress {
    from_port   = 53
    to_port     = 53
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]
  }
}

2. default

3. python package

4. Scenario: Only selected ports should be publicly open ingress Given I have AWS Security Group defined When it contains ingress Then it must only have tcp protocol and port 53,80 for 0.0.0.0/0 Failure: tcp/53-53 ports are defined for ['0.0.0.0/0'] network. Must be limited to tcp/53,80 and 0.0.0.0/0

   Scenario: Only selected ports should be publicly open ingress Given I have AWS Security Group defined When it contains ingress Then it must only have tcp protocol and port 53,80,123,443 for 0.0.0.0/0 AssertionError: Port range is defined incorrectly within the Scenario. Define it 123-80 instead of 80-123.

  Scenario : Only selected ports should be publicly open ingress
      Given I have AWS Security Group defined
      When it contains ingress
      Then it must only have tcp protocol and port 53,80 for 0.0.0.0/0

  Scenario : Only selected ports should be publicly open ingress
      Given I have AWS Security Group defined
      When it contains ingress
      Then it must only have tcp protocol and port 53,80,123,443 for 0.0.0.0/0

Expected behavior : I expect the test to pass b/c the only ports allowed for the cidr are given in the comma seperated allowed list

Tested versions :

• <terraform-compliance version (1.0.55)>
• <terraform version (0.12.10)>
• <python runtime version, if running as a python package (3.7.3)>

Thanks for the issue. I agree there is a problem there and found out why the tool is failing on this specific scenario.

We have to implement Feature->HCL and HCL->Feature tests for Security Groups since must only have requires this kind of a check. Currently it doesn't do it and it only checks HCL->Feature while iterating over all SGs.

I am going to refactor the step and the helper functions that is required for Security Groups checking.

Just giving an update, we are about to finalise the refactoring of the Security Groups feature that will not only fix this problem but also will be extendable for other scenarios.

awesome, thanks for the update

Hi @mjseid , Can you please have a try with the new release ? 🎉

this works now if I only have security groups which match the exact cidr and ports listed. It still fails on rules which should not be in scope (different cidr) or rules which have a subset of the allowed ports but not all of the allowed ports.

Updated resources and rules below

resource "aws_security_group" "allow_tls_ingress_inline" {
  name        = "allow_tls_ingress_inline"

  ingress {
    from_port   = 80
    to_port     = 80
    protocol    = "tcp"
    cidr_blocks     = ["0.0.0.0/0"]
  }

  ingress {
    from_port   = 53
    to_port     = 53
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]
  }
}

resource "aws_security_group" "allow_ssh_ingress_inline" {
  name        = "allow_ssh_ingress_inline"

  ingress {
    from_port   = 22
    to_port     = 22
    protocol    = "tcp"
    cidr_blocks     = ["10.0.0.0/8"]
  }
}

  Scenario : Only selected ports should be publicly open ingress
      Given I have AWS Security Group defined
      When it contains ingress
      Then it must only have tcp protocol and port 53,80 for 0.0.0.0/0

  Scenario : Only selected ports should be publicly open ingress
      Given I have AWS Security Group defined
      When it contains ingress
      Then it must only have tcp protocol and port 53,80,123,443 for 0.0.0.0/0

Results with version 1.1.5

    Scenario: Only selected ports should be publicly open ingress
        Given I have AWS Security Group defined
        When it contains ingress
			Failure: tcp/(80,443,123,53) ports are not defined within 0.0.0.0/0 network in aws_security_group.allow_ssh_ingress_inline.
			         None of the ports given defined within 0.0.0.0/0 network in aws_security_group.allow_ssh_ingress_inline.
			Failure: tcp/(123,443) ports are not defined within 0.0.0.0/0 network in aws_security_group.allow_tls_ingress_inline.
        Then it must only have tcp protocol and port 53,80,123,443 for 0.0.0.0/0
          Failure: 

Hmm, according to the documents it is following the rules ?

Because you are using must only condition in your tests, the test will have a look on both ;

• exact Ports and a supernet of that CIDR (or exact CIDR)

exists on a Security Group.

Your test on the given example are failing because ;

tcp/(80,443,123,53) ports are not defined within 0.0.0.0/0 network in aws_security_group.allow_ssh_ingress_inline.
None of the ports given defined within 0.0.0.0/0 network in aws_security_group.allow_ssh_ingress_inline.
tcp/(123,443) ports are not defined within 0.0.0.0/0 network in aws_security_group.allow_tls_ingress_inline.

• ssh_ingress_inline only have tcp/22 for 10.0.0.0/8. Which ports does not match and CIDR is a subset, not a superset.
• In allow_tls_ingress_inline tcp/123 and tcp/443 is not defined for 0.0.0.0/0. It matches the other ports, but these are the ports that does not exist.

What was your exact expectation ?

Just after reading your message, I think I need to implement a similar step to ;

Then it must only have tcp protocol and port 53,80,123,443 for 0.0.0.0/0

like

Then all must have tcp protocol and port 53,80,123,443 for 0.0.0.0/0

which will be iterated over all values at once, instead of individiuals.

Yes it does seem to follow the reference you linked based on the must only condition.

I was hoping for a way to look at all my security_group rules and if the CIDR of a rule is 0.0.0.0/0 then the port(s) for that rule must be a subset of the allowed list, not necessarily the full list. Also if the CIDR isn't 0.0.0.0/0 then it should pass.

This would support the ability to have many groups verified with a single scenario. For example a group which allows port 22 only to 0.0.0.0/0 and different group which allows port 443 only to 0.0.0.0/0 would be covered by a single step that says and port 22,443 for 0.0.0.0/0

Hi,

You can do it actually, in a way. Instead of must only, you can just say must, but again the port & cidr superset matching will be executed per security group. Thus, if that group has 35 rules, all those rules will be evaluated as one.

Unfortunately, this is not the case if you have 2 security groups, since evaluation is based on the individual security group hence the GIVEN step is ;

Given I have AWS Security Group defined

IMHO, as I wrote one message above, we may have something like ;

Then all must have tcp protocol and port 53,80,123,443 for 0.0.0.0/0

and that it -> all subject within the step can trigger an evaluation for all security groups at considered as one.

or maybe better as a Scenario Outline;

Then it must contain tcp protocol port <port> for <cidr>

where it will search that port for those CIDRs and you can also use Examples as a list, like in this example.

Let me know your thoughts please ?

If I use the current must condition then I'm still constrained by requiring all 4 ports in every group which has the 0.0.0.0/0 CIDR.

The same if I use the Scenario Outline, where it will look at every group and give failures for groups with only 1 of the 4 ports or failures for groups which don't have the 0.0.0.0/0 CIDR defined at all.

I guess my wish list would be the ability to filter more so its only looking at groups with 0.0.0.0/0 rules and then also only requiring at least 1 of the given ports within the superset of ports in the group. Something like:

     Given I have AWS Security Group defined
      When it contains ingress
      And its cidr_blocks is 0.0.0.0/0
      Then it must only contain tcp protocol and port 53,80,123,443 for 0.0.0.0/0

I will create a new step for going over ALL security groups ( not one by one ) ;

Then all must only contain tcp protocol and port 53,80,123,443 for 0.0.0.0/0

Where it -> all change will apply tests for ALL security groups instead of just one.

Hello, I was just wondering if there is any update on this? We're just doing an initial proof-of-concept of using your project, and are very interested in it, but this specific check (ensuring that only a certain list of port numbers can be open to 0.0.0.0/0) is one of the first things we'd like to do.