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

Proper way to do negative testing #156

Closed
joubin opened this issue Sep 8, 2019 · 13 comments
Closed

Proper way to do negative testing #156

joubin opened this issue Sep 8, 2019 · 13 comments
Labels
bug enhancement waiting for confirmation Workaround/Fix applied, waiting for confirmation

Comments

@joubin
Copy link

joubin commented Sep 8, 2019

Feature: EC2 containers should not have a role attached to them unless threat model requires it
  In order to improve security
  As engineers
  We'll not attach IAM Roles to EC2's unless otherwise Threat Modeled


  Scenario: Fail if an EC2 has an IAM Role Attached to it
    Given I have AWS EC2 instance defined
    When it contains iam_instance_profile
  	Then the scenario should fail

Is there a good way to do the above. Basically, if an AWS EC2 Instance have a IAM Role attached, the test should fail.

@eerkunt
Copy link
Member

eerkunt commented Sep 8, 2019

Hello,

You structured the test correctly, but unfortunately some terraform-compliance internals fails while mounting iam_role to an aws_instance resource because terraform is reporting the change in a reverse way.

Thus, it is possible to create a test starting to search for iam_roles first, then check for aws_instance property. It doesn't sound ideal, and potentially prone to error on some edge cases.

Looking into this right now.

@eerkunt
Copy link
Member

eerkunt commented Sep 8, 2019

Fixed in d0aa707, will release new version in few minutes.

@eerkunt
Copy link
Member

eerkunt commented Sep 8, 2019

Could you please have a try with https://github.com/eerkunt/terraform-compliance/releases/tag/1.0.49 version.

Here is the test I have used ;

terraform file
data "aws_iam_policy_document" "attached_policy" {
  statement {
    actions = ["sts:AssumeRole"]
    principals {
      identifiers = ["ec2.amazonaws.com"]
      type = "Service"
    }
  }
}

resource "aws_iam_role" "attached_role" {
  name = "instance-attached-role"
  assume_role_policy = data.aws_iam_policy_document.attached_policy.json
}

resource "aws_instance" "my_instance_with_attached_role" {
  ami                  = "some-ami"
  instance_type        = "t2.micro"
  iam_instance_profile = aws_iam_role.attached_role.arn
}
bdd feature
Feature: EC2 containers should not have a role attached to them unless threat model requires it
  In order to improve security
  As engineers
  We'll not attach IAM Roles to EC2's unless otherwise Threat Modeled

  Scenario: Fail if an EC2 has an IAM Role Attached to it
    Given I have AWS EC2 instance defined
    When it contains aws_iam_role
    Then the scenario should fail

@eerkunt eerkunt added the waiting for confirmation Workaround/Fix applied, waiting for confirmation label Sep 8, 2019
@joubin
Copy link
Author

joubin commented Sep 8, 2019

Thank you for making the change so quickly!

I'm getting recursion error now on the same feature files and tf files as before

I had to upgrade terraform to 12.8 to support 1.0.49.

Here are the commands I'm using

function terraform {
    docker run --rm -v $(pwd):/app/ -w /app/ --e "AWS_ACCESS_KEY_ID=$AWSKEYID" -e "AWS_SECRET_ACCESS_KEY=$AWSACCESSKEY" -e "TF_VAR_SSH_PUB=$TF_VAR_SSH_PUB" -e "TF_VAR_SSH_PRI=$TF_VAR_SSH_PRI" -i -t hashicorp/terraform:0.12.8 "$@";
}

function terraform-compliance { 
    docker run --rm -v $(pwd):/target -e "AWS_ACCESS_KEY_ID=$AWSKEYID" -e "AWS_SECRET_ACCESS_KEY=$AWSACCESSKEY" -i -t eerkunt/terraform-compliance:1.0.49 "$@"; 
}

Here is what I'm running

terraform plan --out=plan.out && terraform-compliance -f Security/ -p plan.out 

and here is what's coming out

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

    Scenario: Ensure all resources have tags
        Given I have resource that supports tags defined
        Then it must contain tags
          RecursionError: maximum recursion depth exceeded while calling a Python object
        And its value must not be null
...

@eerkunt
Copy link
Member

eerkunt commented Sep 9, 2019

Hmm this is weird, is it possible to share your plan.out if there is nothing confidential ?

@eerkunt eerkunt added bug require more data Further data is required to initiate debugging/troubleshooting and removed waiting for confirmation Workaround/Fix applied, waiting for confirmation labels Sep 9, 2019
@eerkunt
Copy link
Member

eerkunt commented Sep 9, 2019

It looks like this A->B and B->A resource mounting is breaking too many things. Removing the release for now.

@eerkunt
Copy link
Member

eerkunt commented Sep 9, 2019

Will fix this with a more proper solution and then release a dev release for you to test.

@joubin
Copy link
Author

joubin commented Sep 10, 2019

No worries. However, here is my main.tf file for now.

provider "aws" {
  region = "${var.aws_region}"

}



resource "aws_key_pair" "deployer" {
  key_name   = "${var.project}-${var.environment}-${var.application}-SSHKey-GitLab"
  public_key = "${var.SSH_PUB}"

}



resource "aws_security_group" "instance" {
  name = "terraform-example-instance"
  
  ingress {
    from_port   = 80
    to_port     = 80
    protocol    = "tcp"
    cidr_blocks = ["1.2.3.4/32"]
  }

  ingress {
    from_port   = 22
    to_port     = 22
    protocol    = "tcp"
    cidr_blocks = ["1.2.3.4/32"]
  }
  
  egress {
    from_port   = 0
    to_port     = 0
    protocol    = "-1"
    cidr_blocks = ["0.0.0.0/0"]
  }

  tags = {
    Name = "allow_all"
  }

}



resource "aws_instance" "demo-prod-AppOne" {
  ami           = "ami-04b762b4289fba92b"
  instance_type = "t2.micro"
  vpc_security_group_ids = [aws_security_group.instance.id]
  key_name = "${var.project}-${var.environment}-${var.application}-SSHKey-GitLab"
  iam_instance_profile = "EC2-S3-READONLY"
  user_data = <<-EOF
#!/bin/bash
yum update -y
amazon-linux-extras install -y lamp-mariadb10.2-php7.2 php7.2
yum install -y httpd mariadb-server
systemctl start httpd
systemctl enable httpd
usermod -a -G apache ec2-user
chown -R ec2-user:apache /var/www
chmod 2775 /var/www
find /var/www -type d -exec chmod 2775 {} \;
find /var/www -type f -exec chmod 0664 {} \;
echo "<?php phpinfo(); ?>" > /var/www/html/phpinfo.php
cat /tmp/index.php > /var/www/html/index.php
EOF

    provisioner "file" {
      source      = "./app/index.php"
      destination = "/tmp/index.php"
      connection {
        type = "ssh"
        user = "ec2-user"
        private_key = "${var.SSH_PRI}"
        host = aws_instance.demo-prod-AppOne.public_ip
      }
    }

    tags = {
      Name        = "${var.project}-${var.environment}-${var.application}",
      Env2         = "Prod"
    }
  }



  output "public_ip" {
    value       = aws_instance.demo-prod-AppOne.public_ip
    description = "The public IP of the web server"
  }

@eerkunt
Copy link
Member

eerkunt commented Sep 10, 2019

Thanks 🎉 This will help a lot.

@eerkunt eerkunt added enhancement and removed require more data Further data is required to initiate debugging/troubleshooting enhancement labels Sep 10, 2019
@eerkunt
Copy link
Member

eerkunt commented Sep 15, 2019

Fix is introduced in 1f8774f, will be released with few more other fixes.

@eerkunt eerkunt added the ready for release Fix/Enhancement is implemented, will be released in next release cycle. label Sep 15, 2019
@eerkunt
Copy link
Member

eerkunt commented Sep 16, 2019

1.0.50 is released 🎉

Please have a try and let me know for your case.

@eerkunt eerkunt added waiting for confirmation Workaround/Fix applied, waiting for confirmation and removed ready for release Fix/Enhancement is implemented, will be released in next release cycle. labels Sep 16, 2019
@eerkunt
Copy link
Member

eerkunt commented Sep 23, 2019

@joubin any luck ?

@eerkunt
Copy link
Member

eerkunt commented Oct 24, 2019

Assuming this issue as resolved. Please dont hesitate to re-open it of the problem still occurs.

Thanks 🎉

@eerkunt eerkunt closed this as completed Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug enhancement waiting for confirmation Workaround/Fix applied, waiting for confirmation
Projects
None yet
Development

No branches or pull requests

2 participants