terraform-compliance / cli

a lightweight, security focused, BDD test framework against terraform.
https://terraform-compliance.com
MIT License
1.36k stars 151 forks source link

step : Then it must contain fails to read a property #126

Closed vrbcntrl closed 5 years ago

vrbcntrl commented 5 years ago

Hi,

We are trying to run my below test in v 1.0.21 , but it fails with error : Failure: aws_elb.bar (aws_elb) does not have ssl_certificate_id property.

BDD is below:

Scenario: Ensure that aws-elb has listener ssl-cerificateid provided by Aws certificate Manager  

    Given I have AWS ELB resource defined
    When it contains listener
    Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

Results:

Scenario: Ensure that aws-elb has listener ssl-cerificateid provided by Aws certificate Manager
        Given I have AWS ELB resource defined
        When it contains listener
        Then it must contain ssl_certificate_id
          Failure: aws_elb.bar (aws_elb) does not have ssl_certificate_id property.
        And its value must match the ".*acm.*" regex

1 features (0 passed, 1 failed)
1 scenarios (0 passed, 1 failed)
4 steps (2 passed, 1 failed, 1 skipped)
Run 1562964747 finished within a moment

however if I change the BDD to

Given I have AWS ELB resource defined
    When it contains ssl_certificate_id
    #Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

Then its successfully executed as I expected

Scenario: Ensure that aws-elb has listener ssl-cerificateid provided by Aws certificate Manager
        Given I have AWS ELB resource defined
        When it contains ssl_certificate_id
        And its value must match the ".*acm.*" regex
          Failure: ssl_certificate_id property in aws_elb.bar resource does not match with .*acm.* regex. It is set to arn:aws:iam::123456789012:server-certificate/certName.

1 features (0 passed, 1 failed)
1 scenarios (0 passed, 1 failed)
3 steps (2 passed, 1 failed)
Run 1562965008 finished within a moment

I think the reason the first BDD failed is because line#134 in steps.py return empty values values = resource.get('values', resource.get('expressions', {})) when Then it must contain ssl_certificate_id step is executed

sample tf template is taken from : https://www.terraform.io/docs/providers/aws/r/elb.html

eerkunt commented 5 years ago

Assuming the terraform file is ;

resource "aws_elb" "bar" {
  name               = "foobar-terraform-elb"
  availability_zones = ["us-west-2a", "us-west-2b", "us-west-2c"]

  access_logs {
    bucket        = "foo"
    bucket_prefix = "bar"
    interval      = 60
  }

  listener {
    instance_port     = 8000
    instance_protocol = "http"
    lb_port           = 80
    lb_protocol       = "http"
  }

  listener {
    instance_port      = 8000
    instance_protocol  = "http"
    lb_port            = 443
    lb_protocol        = "https"
    ssl_certificate_id = "arn:aws:iam::123456789012:server-certificate/certName"
  }

  health_check {
    healthy_threshold   = 2
    unhealthy_threshold = 2
    timeout             = 3
    target              = "HTTP:8000/"
    interval            = 30
  }

  instances                   = ["some_id"]
  cross_zone_load_balancing   = true
  idle_timeout                = 400
  connection_draining         = true
  connection_draining_timeout = 400

  tags = {
    Name = "foobar-terraform-elb"
  }
}

as also given in https://www.terraform.io/docs/providers/aws/r/elb.html, then the functionality is working properly. Since in this hcl, there are 2 listeners defined and one of them does not have ssl_certificate_id

Thus, when you run

    Given I have AWS ELB resource defined
    When it contains listener
    Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

Then you are enforcing ALL listeners must have ssl_certificate_id. So this must be applicable to ALL findings.

On the other hand when you run ;

    Given I have AWS ELB resource defined
    When it contains ssl_certificate_id
    #Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

You are actually saying, if you find an ssl_certificate_id then it should have match with the regex. This is applicable to at least one finding.

vrbcntrl commented 5 years ago

I guess, my question is what if I have 2 ssl_certificateids configured in side 2 listners, then if the first ssl_certficateid is passed then the tool is not checking for the second ssl_certifcateid

assume I have the below configuration:

 listener {
    instance_port     = 8000
    instance_protocol = "http"
    lb_port           = 80
    lb_protocol       = "http"
 ssl_certificate_id = "arn:aws:acm::123456789012:server-certificate/certName"
  }

  listener {
    instance_port      = 8000
    instance_protocol  = "http"
    lb_port            = 443
    lb_protocol        = "https"
    ssl_certificate_id = "arn:aws:iam::123456789012:server-certificate/certName"
  }

also, wouldn't it be more accurate to read the property values by following its hierarchy?

For Example: i have ssl_certificateid in this tree

aws_elb ----> listner---->ssl_certificateid

but in the suggested rule, we are reading the ssl_certificateid using the search aws_elb--->ssl_certificateid

so...i am thinking which is the best way to search ssl_certificateid? or any parameter at that level

vrbcntrl commented 5 years ago

here is the plan json I was testing with

aws_elb.out.json.txt

the below tests produce different results on the same plan json with terraform-compliance 1.0.22 and terraform 0.12.1

test1: This test is PASSED

 Given I have AWS ELB resource defined
    When it contains listener
    Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

test2: This test is FAILED

 Given I have AWS ELB resource defined
    When it contains ssl_certificate_id
    Then its value must match the ".*acm.*" regex

so I am bit confused :(

eerkunt commented 5 years ago

Good finding. terraform-compliance was just finding the last found key (listener without having any ssl_certificate_id on your example).

This is fixed right now and I will push it in few minutes. There is also another detail in terraform that it looks like whenever ssl_certificate_id is not there, in plan.out.json actually it is set to ''. This was also messing up some problems. Also introduced a fix to that, too.

Additionally, you need to be a bit careful while using must, whenever must is used, instead of skipping the test, it will fail. Just saying..

I changed your feature tests like this ;

  Scenario: Ensure we are encryption ELBs via ACM.
    Given I have AWS ELB resource defined
    When it contains listener
    Then it must contain ssl_certificate_id
    And its value must match the ".*acm.*" regex

  Scenario: Ensure encryption flag is set on ELBs.
    Given I have AWS ELB resource defined
    When it contains listener
    Then it must contain ssl_certificate_id
    And its value must not be null

The tests will run properly on 1.0.23, few more minutes :)

vrbcntrl commented 5 years ago

Thanks for the fix and updated BDD. The tests works fine when I have one ssl_certoficateid does not match with my regex i.e ".acm." however, if my plan contains 2 ssl_certoficateids and if both are not maching the regex, the test result would show me FAILURE message for only one ssl_certoficateid.

Ex:

                            "listener": [
                            {
                                "instance_port": 8000,
                                "instance_protocol": "http",
                                "lb_port": 443,
                                "lb_protocol": "https",
                                "ssl_certificate_id": "arn:aws:iam::123456789012:server-certificate/certName"
                            },
                            {
                                "instance_port": 8080,
                                "instance_protocol": "http",
                                "lb_port": 443,
                                "lb_protocol": "https",
                                "ssl_certificate_id": "arn:aws:abc::123456789012:server-certificate/certName"
                            }
                        ]

results:

Feature: To test elb configuartion # D:\VA_Softwares\cloud\Terraform-compliance\CustomRules\rules-inprogress\elbtest\elb-heirarchy.feature

    Scenario: Ensure we are encrypting ELBs via ACM.
        Given I have AWS ELB resource defined
        When it contains listener
        Then it must contain ssl_certificate_id
        And its value must match the ".*acm.*" regex
          Failure: ssl_certificate_id property in aws_elb.bar resource does not match with .*acm.* regex. It is set to arn:aws:iam::123456789012:server-certificate/certName.

I think it would be great if we could print both ssl_certificateids in the FAILURE message some thing like this:

Failure: ssl_certificate_id property in aws_elb.bar resource does not match with .*acm.* regex. It is set to arn:aws:iam::123456789012:server-certificate/certName
ssl_certificate_id property in aws_elb.bar resource does not match with .*acm.* regex. It is set to arn:aws:abc::123456789012:server-certificate/certName

any thoughts?

vrbcntrl commented 5 years ago

BTW I have tested this with 1.0.25

eerkunt commented 5 years ago

That is an expected behaviour. All failures happens on first match.

terraform-compliance is not a linting tool, it is way of ensuring your terraform code is compliant. :) So it won't show you all the errors, but only show the first matches.

vrbcntrl commented 5 years ago

alright!! fair enough :)

Thanks for the fix.

eerkunt commented 5 years ago

Thanks for reporting 🎉

Closing the issue :)