Feature file broken since 1.0.17

[anthonycolon25]

I had been testing with a plan.out.json file over the last several releases (at least up until 1.0.13). I was trying out a more complex tf script with 1.0.20 and had errors. In trying to troubleshoot I reverted back to 1.0.13 (same errors). I created ( #124) for that problem.

I went back to try my previous plan.out.json (the simple one) with 1.0.20 and that was broken as well. I ran some tests with several versions. Up to version 1.0.16 everything worked fine with the output json from the simpler script. Here are the results for other versions (with the same plan.out.json):

1.0.17:

$ terraform-compliance -p /target/tc-test2/tf-template/plan.out.json -f /target/tc-test2/features
Unable to find image 'eerkunt/terraform-compliance:1.0.17' locally
1.0.17: Pulling from eerkunt/terraform-compliance
fc7181108d40: Already exists 
01f3b6f74410: Already exists 
163142c4383b: Already exists 
eeed0da1db91: Already exists 
ea6d6fd23371: Already exists 
4a73e06379da: Pull complete 
Digest: sha256:b785a436dc81d15f903ab639ca9bee780bb666b469c7f1930610feda89b2cd18
Status: Downloaded newer image for eerkunt/terraform-compliance:1.0.17
terraform-compliance v1.0.17 initiated

* Features  : /target/tc-test2/features
* Plan File : /target/tc-test2/tf-template/plan.out.json

. Running tests.
Feature: Resources should be properly tagged  # /target/tc-test2/features/tags.feature
    In order to keep track of resource ownership
    As engineers
    We'll enforce tagging on all resources

    Scenario Outline: Ensure that my specific tags are defined
        Given I have resource that supports tags defined
        When it contains tags
        Then it must contain <tag_keys>
        And its value must match the "<pattern>" regex

    Examples:
        | tag_keys            | pattern                                                                  |
        | application         | ^([a-z]+[0-9]*)*[-]?([a-z0-9]+[-])*[a-z0-9]+$                            |
          TerraformComplianceInternalFailure: Unexpected value type <class 'dict'>. {'key': 'Name', 'propagate_at_launch': 'true', 'value': 'ptct-gsearch-dev-cluster'}
        | app_component       | ^([a-z]+[0-9]*)*[-]?([a-z0-9]+[-])*[a-z0-9]+$                            |
          TerraformComplianceInternalFailure: Unexpected value type <class 'dict'>. {'key': 'Name', 'propagate_at_launch': 'true', 'value': 'ptct-gsearch-dev-cluster'}
        | app_contacts        | ^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$                          |
          TerraformComplianceInternalFailure: Unexpected value type <class 'dict'>. {'key': 'Name', 'propagate_at_launch': 'true', 'value': 'ptct-gsearch-dev-cluster'}
        | app_environment     | ^(sandbox|dev|deva|devb|ita|itb|qa|pv|prod|poc|training)$                |
          TerraformComplianceInternalFailure: Unexpected value type <class 'dict'>. {'key': 'Name', 'propagate_at_launch': 'true', 'value': 'ptct-gsearch-dev-cluster'}

1.0.21:

$ terraform-compliance -p /target/tc-test2/tf-template/plan.out.json -f /target/tc-test2/features
terraform-compliance v1.0.21 initiated

* Features  : /target/tc-test2/features
* Plan File : /target/tc-test2/tf-template/plan.out.json

. Running tests.
Feature: Resources should be properly tagged  # /target/tc-test2/features/tags.feature
    In order to keep track of resource ownership
    As engineers
    We'll enforce tagging on all resources

    Scenario Outline: Ensure that my specific tags are defined
        Given I have resource that supports tags defined
        When it contains tags
        Then it must contain <tag_keys>
        And its value must match the "<pattern>" regex

    Examples:
        | tag_keys            | pattern                                                                  |
        | application         | ^([a-z]+[0-9]*)*[-]?([a-z0-9]+[-])*[a-z0-9]+$                            |
          Failure: aws_autoscaling_group.example (resource that supports tags) does not have application property.
        | app_component       | ^([a-z]+[0-9]*)*[-]?([a-z0-9]+[-])*[a-z0-9]+$                            |
          Failure: aws_autoscaling_group.example (resource that supports tags) does not have app_component property.
        | app_contacts        | ^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+.[a-zA-Z0-9-.]+$                          |
          Failure: aws_autoscaling_group.example (resource that supports tags) does not have app_contacts property.
        | app_environment     | ^(sandbox|dev|deva|devb|ita|itb|qa|pv|prod|poc|training)$                |
          Failure: aws_autoscaling_group.example (resource that supports tags) does not have app_environment property.

on 1.0.21 I also get additional errors that worked fine on 1.0.17:

Feature: Security Groups Rules should be used to protect services/instances  # /target/tc-test2/features/security_group_rule.feature
    In order to improve security
    As engineers
    We'll use AWS Security Group Rules as a Perimeter Defense

    Scenario Outline: Policy Structure
        Given I have AWS Security Group defined
        Then it must contain <policy_name>

    Examples:
        | policy_name |
        | ingress     |

    Scenario Outline: Well-known insecure protocol exposure on Public Network for ingress traffic
        Given I have AWS Security Group defined
        When it contains ingress
        Then it must not have <proto> protocol and port <portNumber> for 0.0.0.0/0

    Examples:
        | ProtocolName  | proto | portNumber |
        | HTTP          | tcp   | 443        |
          KeyError: 0
        | Telnet        | tcp   | 23         |
          KeyError: 0
        | SSH           | tcp   | 22         |
          KeyError: 0
        | MySQL         | tcp   | 3306       |
          KeyError: 0
        | MSSQL         | tcp   | 1443       |
          KeyError: 0
        | NetBIOS       | tcp   | 139        |
          KeyError: 0
        | RDP           | tcp   | 3389       |
          KeyError: 0
        | Jenkins Slave | tcp   | 50000      |
          KeyError: 0

I am attaching the output json file.
plan.out.json.txt

[eerkunt]

Fix has been introduced in 1.0.22.

Can you please have a try ? Build is started, must be there in few minutes.

[anthonycolon25]

Tested my simple terraform script with 1.0.22. The errors at the top are now gone. The scenario outline for protocol/port/cidr verification still has some problems.

If I change the aws_security_group in my terraform to something like the following:

### Creating Security Group for EC2
resource "aws_security_group" "instance" {
  name = "${module.tagsmod.id}"
  ingress {
    from_port   = 8080
    to_port     = 8080
    protocol    = "tcp"
    cidr_blocks = ["0.0.0.0/0"]
  }
  ingress {
    from_port   = 22
    to_port     = 22
    protocol    = "tcp"
    # cidr_blocks = ["0.0.0.0/0"]
    cidr_blocks = ["10.0.0.0/8"]
  }

  tags = "${module.tagsmod2.tags}"
#   tags = {
#     Name = "staging"
#     role = "sgrole"
#     BusinessUnit = "IT"
#   }
}

It should fail with the following scenario outline:

Scenario Outline: Well-known insecure protocol exposure on Public Network for ingress traffic
    Given I have AWS Security Group defined
  	When it contains ingress
    Then it must not have <proto> protocol and port <portNumber> for 10.0.0.0/8

  Examples:
    | ProtocolName | proto | portNumber |
    | HTTP         | tcp   | 443        |
    | Telnet       | tcp   | 23         |
    | SSH          | tcp   | 22         |
    | MySQL        | tcp   | 3306       |
    | MSSQL        | tcp   | 1443       |
    | NetBIOS      | tcp   | 139        |
    | RDP          | tcp   | 3389       |
    | Jenkins Slave| tcp   | 50000      |

But it passes the test. I believe it has to do with having multiple ingress blocks. When I remove the first one ingress block it fails the test (as it should). If I have the two ingress blocks but invert the order it still incorrectly passes the test.

Also, in trying to troubleshoot that issue I tried changing the scenario outline to use "must" instead of "must not" as follows:

  Scenario Outline: Well-known insecure protocol exposure on Public Network for ingress traffic
    Given I have AWS Security Group defined
  	When it contains ingress
    Then it must have <proto> protocol and port <portNumber> for 10.0.0.0/8

  Examples:
    | ProtocolName | proto | portNumber |
    | HTTP         | tcp   | 443        |
    | Telnet       | tcp   | 23         |
    | SSH          | tcp   | 22         |
    | MySQL        | tcp   | 3306       |
    | MSSQL        | tcp   | 1443       |
    | NetBIOS      | tcp   | 139        |
    | RDP          | tcp   | 3389       |
    | Jenkins Slave| tcp   | 50000      |

and got the following error:

$ terraform-compliance -p /target/tc-test2/tf-template/plan.out.json -f /target/tc-test2/features
terraform-compliance v1.0.22 initiated

* Features  : /target/tc-test2/features
* Plan File : /target/tc-test2/tf-template/plan.out.json

. Running tests.
Error: Cannot find step definition for step 'Then it must have tcp protocol and port 443 for 10.0.0.0/8' in /target/tc-test2/features/security_group_rule.feature:29

Error Oracle says:
There is no step defintion for 'Then it must have tcp protocol and port 443 for 10.0.0.0/8'.
All steps should be declared in a module located in /usr/local/lib/python3.7/site-packages/terraform_compliance/steps.
For example you could do:

@step(r"Then it must have tcp protocol and port 443 for 10.0.0.0/8")
def my_step(step):
    raise NotImplementedError("This step is not implemented yet")

Should these be new issues??

[eerkunt]

Let me answer both issues at once.

1. Multiple ingress may cause this problem, never tried really. Very good find! Will have a look and fix. 🎉

2. According to the documentation, step definitions are like below.

BDD Conditions	Step Sentence	Parameters
THEN	it must {condition} have {proto} protocol and port {port} for {cidr}	{condition}: only,not proto: tcp, udp port: integer port number (or a port range by using - delimeter between port ranges [e.g. 80-84]) cidr: IP/Cidr

Thus, if you change your step from ;

Then it must have tcp protocol and port 443 for 10.0.0.0/8

to

Then it must only have tcp protocol and port 443 for 10.0.0.0/8

it should work.

[anthonycolon25]

Thank you for the quick reply. I actually looked at the documentation and misread it. :(

I just tested briefly with the only condition. I guess that is only usable within a scenario, not a scenario outline?

For example could I do something like "the only ingress rules allowed are tcp on port 80 and tcp on port 443 for 2.2.2.2/8 " ?

[eerkunt]

You can also use for both Scenario or Scenario Outline. The only difference between these two is, on Scenario you need to define the values within the steps, while in Scenario Outline you can use Examples with variables. For more information, you can have a look on radish-bdd documentation

Here is the test that should work for your previous scenario ;

  Scenario: Ensure we only allow a port range for ingress rule
    Given I have AWS Security Group defined
    When it contains ingress
    Then it must only have tcp protocol and port 22 for 10.0.0.0/8

and the results ;

    Scenario: Ensure we only allow a port range for ingress rule
        Given I have AWS Security Group defined
        When it contains ingress
        Then it must only have tcp protocol and port 22 for 10.0.0.0/8
          Failure: tcp/8080-8080 ports are defined for ['0.0.0.0/0'] network. Must be limited to tcp/22

which is valid (but the error message is misleading a bit, it should have said Must be limited to tcp/22 and 10.0.0.0/8) because tcp/8080 is defined for 0.0.0.0/0 within the terraform code, which 10.0.0.0/8 is a subnet of 0.0.0.0/0 and scenario fails.

If we create a Scenario like this ;

 Scenario: Ensure we only allow a port range for ingress rule
   Given I have AWS Security Group defined
   When it contains ingress
   Then it must only have tcp protocol and port 8080 for 0.0.0.0/0

Then the tests will pass, because the other port defined in SGs are tcp/22 for 10.0.0.0/8 which is not a subset of 0.0.0.0/0

[anthonycolon25]

Sorry but I don't fully understand the explanation.

I understand the first portion where we are setting a rule that only tcp/22 is allowed for 10.0.0.0/8, but in another ingress block declaration we are allowing tcp/8080 for 0.0.0.0/0. Since 0.0.0.0/0 basically means all IP addresses (including 10.0.0.0/8) the the test should fail.

In you other scenario we are setting a rule that only tcp/8080 is allowed for 0.0.0.0/0 (which should mean all IPs). If I then have the other ingress block allowing tcp/22 for 10.0.0.0/8 shouldn't that fail as well??

Isn't 10.0.0.0/8 a subset of 0.0.0.0/0?

[eerkunt]

Yes it is, but you need to switch the logic :)

Then it must only have tcp protocol and port 8080 for 0.0.0.0/0

This means, for 0.0.0.0/0 cidr ( which is everything just like you said), there must be only 8080/tcp defined. Thus, the logic must check if nothing else is defined for 0.0.0.0/0.

A sample use case could be an SG attached to an ELB, allowing 8080/tcp from Public Internet (0.0.0.0/0 in this case). So another SG rule defining 22/tcp for 10.0.0.0/8 does not create a risk.

On the opposite, if my ELB only allows 22/tcp from 10.0.0.0/8, then an SG rule defining 8080/tcp for 10.0.0.0/8 would also match the same rule and that ELB will allow 8080/tcp and 22/tcp from 10.0.0.0/8

[anthonycolon25]

Still not clear for me but I don't really have that use case at the moment so I won't worry too much about it for now :)

[eerkunt]

You are not alone. I had quite hard times to understand while implementing it.

The use case is quite limited on only usage for SG rules.

[eerkunt]

Closing the issue, since the initial problem is fixed :)

Please don't hesitate to re-open it, if you still have problem with the same issue.

Thanks again 🎉

[anthonycolon25]

Will you address the multiple ingress blocks in another issue?

[eerkunt]

It is already released :) Please try with the latest version, was a busy evening :)

[anthonycolon25]

Just tested with 1.0.24. Works good.

Thank you for your support!!

[eerkunt]

Glad it is working! 🎉

Thanks for reporting it. :)

Closing the issue.