Open mdesmarest opened 3 years ago
Kudbettin
commented
3 years ago @mdesmarest,
Thanks for reporting this bug! Here's my take on it: #417
The default value of encrypted in ebs_block_device is false in the docs. However, in the plan file, the default value is true, which causes the issue.
mdesmarest
commented
3 years ago @Kudbettin
Thanks for getting back to me quickly!
I understand your reference, but am confused at how you determined the default value, as the terraform docs. here aws_instance indicate that the default value for ebs_block_device and root_block_device are both encrypted = false . I understand the logic of how the stash builds, but I am not sure why a default value would be true when the docs say its false. Can you elaborate on how you are filling in your default values as in your example you reference encrypted=true . This runs counter to the terraform docs.
Being able to account for situations where known after apply are in the plan vs static values assigned helps in two ways
if the default is false, and we are enforcing maximized declaration of optional parameters for code clarity and drift detection, we want to make sure that a hardcoded value is present. Flagging on a known after apply where there should be a value declared would be very beneficial, but we need to get clarity on how you are drawing the default values you are pulling in. I think being able to account for what is known after apply as simply a consequence of an absent parameter vs a true known after apply because it is only known after apply. I see why this depends less on the plan and more on the code blocks.
I understand the limitations here, and your tool is the reason we moved from resource block scanners like TFsec and Checkov, they couldnt evaluate resource blocks as they relate to a plan as a whole. I can now clearly see the gap where resource blocks play a role where a plan file does not.
3.A lot of my enforcement of tests still stems from a need to determine the difference between a create and an update resource to better enforce things like encryption on newly deployed resources vs flagging on a resource being updated for tags getting flagged for encryption, since encrypting root_block_device via terraform would result in a destroy and rebuild of a block_device. This would not be a great thing.
also, on one of my resources, where I do not declare a root_block_device at all,
eerkunt
commented
3 years ago I think we fixed this issue on 1.3.8. Could you please have a try and let us know ? 🙏
mdesmarest
commented
3 years ago @eerkunt @Kudbettin YES! this works, see below for confirmation.
This is exactly what we need. Thanks so much!!! I see your changes in terraform.py as well as helper.py. After posting my issue, I took a look into the code and saw how you were populating the values via the after block in resource changes portion of the plan json. I see you were using the resource_changes block flagging if actions != ['delete']: here: Link I am trying to work on leveraging the same filtering to only populate the resources with actions =['create'] on a fork now. Instead of populating values, I want to remove all resources that are not flagged ascreate` This seems entirely possible to do, maybe with an additional output flag to run as an option. Is this something on your board? I realize it may be a heavy lift, but I can see this pattern of use as very compelling for many. It's the last major and most important barrier to a full deploy pattern for us. Since this new update fixes all of our false negatives, we can now write our full feature set.
Much of what we are running Terraform-Compliance against, is living infrastructure where additions are being made as well as full new deployments. We can't fail on resources if the fix is resource destroying/recreation, but want to focus on the resources at deployment where we have the most flexibility to flag failures. We plan to run terraform-compliance against state files to clear up legacy compliance issues that require changes outside of terraform, but need to heavily enforce on github repos where we have junior level devs deploying code.
We are running with Atlantis via github PRs, feeding a 1 liner for terraform-compliance directly into the plan step via the atlantis.yaml and passing the planfile that Atlantis produces directly into terraform-compliance. We are looking to enforce checks via protected github branches, with Terraform-compliance influencing the pass/fail of the Atlantis check and outputting our results in the same gitbot message Atlantis uses. I'd be happy to show you how we sort and format our output, and how we are using terraform-compliance. Thanks to the updated silent mode we are now very contextual! This whole deploy pattern works, we just need to scan on create only as stated above.
TEST OF PR CHANGES
FEATURES
Feature: ec2 root_block_device must be declared and encrypted set to true. Full Resolution notes at: github.com/CORP/tfcompliance/all/ec2root_block_encrypt.feature
Fixes: declare and configure root_block_device on each non bastion EC2 instance and declare encrypted=true
Background:Check for the presence of aws ec2 instances not named bastion
Given I have aws_instance defined
When its name is not bastion
Scenario: Root block device not declared
Then it must have root_block_device
Scenario: root block device encrypted must be declared
When it has root_block_device
Then it must have root_block_device
And it must have encrypted
Scenario: root block device encrypted must be declared and set to true
When it has root_block_device
Then it must have root_block_device
When it has encrypted
Then it must have encrypted
And its value must be trueFeature: ec2 ebs_block_device must have encrypted declared and set to true. Full Resolution notes at: github.com/CORP/tfcompliance/all/ec2ebs_block_encrypt.feature
Fixes: declare and configure ebs_block_device on each non bastion EC2 instance and declare encrypted=true
Background:Check for the presence of aws ec2 instances not named bastion
Given I have aws_instance defined
When its name is not bastion
Scenario: ebs_block_device encrypted must be declared
When it has ebs_block_device
Then it must have ebs_block_device
And it must have encrypted
Scenario: ebs_block_device encrypted must be declared and set to true
When it has ebs_block_device
Then it must have ebs_block_device
When it has encrypted
Then it must have encrypted
And its value must be trueTERRAFORM RESOURCES
resource "aws_instance" "root_block_encrypted_FALSE" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
volume_size = 200
encrypted = false
}
ebs_block_device {
device_name = "/dev/sdb"
encrypted = true
}
}
#root_block_device is missing the encrypted field
resource "aws_instance" "root_block_encrypted_field_missing_ebs_true" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
volume_size = 200
}
ebs_block_device {
device_name = "/dev/sdb"
encrypted = true
}
}
#root_block_device is missing the encrypted field
resource "aws_instance" "root_block_encrypted_field_missing_ebs_false" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
volume_size = 200
}
ebs_block_device {
device_name = "/dev/sdb"
encrypted = false
}
}
#root_block_volume entirely missing
resource "aws_instance" "root_block_missing" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
ebs_block_device {
device_name = "/dev/sdb"
encrypted = true
}
}
#root_block_volume declared and set to true, ebs encrypted not declared
resource "aws_instance" "ebs_block_not_declared" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
encrypted = true
}
}
#root_block_volume declared and set to true, ebs encrypted not declared
resource "aws_instance" "ebs_encrypted_not_present" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
encrypted = true
}
ebs_block_device {
device_name = "/dev/sdg"
}
}
resource "aws_instance" "ebs_encrypted_FALSE" {
ami = "ami-003634241a8fcdec0"
instance_type = "t2.medium"
root_block_device {
volume_size = 200
encrypted = true
}
ebs_block_device {
device_name = "/dev/sdg"
encrypted = false
}
}RESULTS
Feature: ec2 root_block_device must be declared and encrypted set to true.
Fixes: declare and configure root_block_device on each non bastion EC2 instance and declare encrypted=true
Background: Check for the presence of aws ec2 instances not named bastion
Given I have aws_instance defined
When its name is not bastion
Scenario: Root block device not declared
Given I have aws_instance defined
When its name is not bastion
Failure: aws_instance.root_block_missing (aws_instance) does not have root_block_device property.
Then it must have root_block_device
Failure:
Scenario: root block device encrypted must be declared
Given I have aws_instance defined
When its name is not bastion
When it has root_block_device
Then it must have root_block_device
Failure: aws_instance.root_block_encrypted_field_missing_ebs_false (aws_instance) does not have encrypted property.
Failure: aws_instance.root_block_encrypted_field_missing_ebs_true (aws_instance) does not have encrypted property.
And it must have encrypted
Failure:
Scenario: root block device encrypted must be declared and set to true
Given I have aws_instance defined
When its name is not bastion
When it has root_block_device
Then it must have root_block_device
When it has encrypted
Then it must have encrypted
Failure: encrypted property in aws_instance.root_block_encrypted_FALSE resource does not match with ^true$ case insensitive regex. It is set to False.
And its value must be true
Failure:
Feature: ec2 ebs_block_device must have encrypted declared and set to true.
Background: Check for the presence of aws ec2 instances not named bastion
Given I have aws_instance defined
When its name is not bastion
Scenario: ebs_block_device encrypted must be declared
Given I have aws_instance defined
When its name is not bastion
When it has ebs_block_device
Then it must have ebs_block_device
Failure: aws_instance.ebs_encrypted_not_present (aws_instance) does not have encrypted property.
And it must have encrypted
Failure:
Scenario: ebs_block_device encrypted must be declared and set to true
Given I have aws_instance defined
When its name is not bastion
When it has ebs_block_device
Then it must have ebs_block_device
When it has encrypted
Then it must have encrypted
Failure: encrypted property in aws_instance.ebs_encrypted_FALSE resource does not match with ^true$ case insensitive regex. It is set to False.
Failure: encrypted property in aws_instance.root_block_encrypted_field_missing_ebs_false resource does not match with ^true$ case insensitive regex. It is set to False.
And its value must be true
Failure:
2 features (0 passed, 2 failed)
5 scenarios (0 passed, 5 failed)
17 steps (12 passed, 5 failed)
Run 1607428302 finished within a moment
Please see full code and trouble shoot on 410, but the logic with how the encrypted field gets added to root_block_device or ebs_block_device when when or the other is null, the default value should be false and this is confirmed in the json file generated from terraform plan, however Terraform compliance is passing the value of one to the other, if the other is left blank. This has caused issues with rule writing and false positives and false negatives. Thanks!
410
Scenario: root_block_device must be configured on all non bastion hosts Given I have aws_instance defined When its name is not bastion Then it must have root_block_device And it must have encrypted <skips here And its value must be true < Skips here
When you look at the actual plan JSON this is not correct, Terraform compliance is parsing incorrectly: Resource block:
ACTUAL JSON OUTPUT FOR RESOURCE:
https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#encrypted. Default for encrypted field is false
A parsing issue also occurs for This piece of terraform code, ebs_block_device encryption is not declared in block:
STASH SHOWS INCORRECT, shows ebs_block_device encrypted = true https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/instance#encrypted default is also FALSE on terraform registry
Plan JSON OUTPUT ALSO CONCURS. that terraform compliance is not passing the correct values to the stash: