Checkov #108
Checkov #108
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughA new Terraform configuration file has been added, defining several AWS resources with intentional security vulnerabilities for testing. The file sets up local variables, configures the AWS provider with hardcoded credentials, and specifies required Terraform and AWS provider versions. It declares two security groups—one with minimal restrictions and another with overly permissive rules—and an S3 bucket that is publicly accessible. The configuration includes comments and instructions for running security scans using Checkov. Changes
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
sampleTerraformFile.tf (2)
1-8: Refactor documentation comments to README
Consider moving the Checkov invocation examples and reference URL out of the Terraform file and into a dedicated README or CI configuration. This keeps the Terraform code focused and avoids cluttering resource files with procedural docs.
30-49: Refine egress rules and clean up description
- The default egress allows all outbound traffic, which may be broader than necessary.
- The description has an extra trailing space.
You might tighten the egress to required ports and remove the trailing space:
resource "aws_security_group" "this" { name = local.sg_name - description = "Security group " + description = "Security group" vpc_id = local.aws_vpc_id ... egress { - from_port = 0 - to_port = 0 - protocol = "-1" + from_port = 443 + to_port = 443 + protocol = "tcp" cidr_blocks = ["0.0.0.0/0"] } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sampleTerraformFile.tf(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Security Checks
sampleTerraformFile.tf
[error] 24-24: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by malicious adversaries. Use environment variables or secure vaults to provide credentials. (terraform.aws.security.aws-provider-static-credentials.aws-provider-static-credentials)
🪛 CircleCI: semgrep
sampleTerraformFile.tf
[error] 24-24: terraform.aws.security.aws-provider-static-credentials.aws-provider-static-credentials: A hard-coded credential was detected. Storing credentials in source code risks secrets being leaked. Use environment variables or secure vaults to provide credentials.
🔇 Additional comments (1)
sampleTerraformFile.tf (1)
97-105: Terraform block versions look good
Therequired_versionand provider version constraints are appropriately set.
| locals { | ||
| sg_name = "checkov-test" | ||
| aws_vpc_id = "vpc-#####" #enter vpc id here | ||
| cidr_block = ["0.0.0.0/0"] | ||
| from_port = "80" | ||
| to_port = "80" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce correct types for port variables
Terraform expects numeric values for from_port and to_port. Defining them as strings will cause type mismatches or implicit conversions. Convert these to numbers for clarity and safety.
Apply this diff:
locals {
sg_name = "checkov-test"
aws_vpc_id = "vpc-#####" #enter vpc id here
cidr_block = ["0.0.0.0/0"]
- from_port = "80"
- to_port = "80"
+ from_port = 80
+ to_port = 80
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| locals { | |
| sg_name = "checkov-test" | |
| aws_vpc_id = "vpc-#####" #enter vpc id here | |
| cidr_block = ["0.0.0.0/0"] | |
| from_port = "80" | |
| to_port = "80" | |
| } | |
| locals { | |
| sg_name = "checkov-test" | |
| aws_vpc_id = "vpc-#####" #enter vpc id here | |
| cidr_block = ["0.0.0.0/0"] | |
| from_port = 80 | |
| to_port = 80 | |
| } |
| provider "aws" { | ||
| region = "us-east-1" | ||
| access_key = "AKIA123456789EXAMPLE" | ||
| secret_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" | ||
| } |
There was a problem hiding this comment.
Avoid hard-coded AWS credentials
Embedding access_key and secret_key in code poses a critical security risk. Use environment variables, shared credentials files, or Terraform variables with sensitive inputs instead.
Consider this approach:
provider "aws" {
- region = "us-east-1"
- access_key = "AKIA123456789EXAMPLE"
- secret_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY"
+ region = var.aws_region
+ # Credentials picked up from environment or shared credentials file
}
variable "aws_region" {
type = string
default = "us-east-1"
}Also define aws_access_key and aws_secret_key as sensitive variables or rely on AWS provider defaults.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| provider "aws" { | |
| region = "us-east-1" | |
| access_key = "AKIA123456789EXAMPLE" | |
| secret_key = "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY" | |
| } | |
| provider "aws" { | |
| region = var.aws_region | |
| # Credentials picked up from environment or shared credentials file | |
| } | |
| variable "aws_region" { | |
| type = string | |
| default = "us-east-1" | |
| } |
🧰 Tools
🪛 GitHub Actions: Security Checks
[error] 24-24: A hard-coded credential was detected. It is not recommended to store credentials in source-code, as this risks secrets being leaked and used by malicious adversaries. Use environment variables or secure vaults to provide credentials. (terraform.aws.security.aws-provider-static-credentials.aws-provider-static-credentials)
🪛 CircleCI: semgrep
[error] 24-24: terraform.aws.security.aws-provider-static-credentials.aws-provider-static-credentials: A hard-coded credential was detected. Storing credentials in source code risks secrets being leaked. Use environment variables or secure vaults to provide credentials.
| resource "aws_security_group" "insecure" { | ||
| name = "insecure-sg" | ||
| description = "Insecure SG exposing all TCP ports to the world" | ||
| vpc_id = local.aws_vpc_id | ||
|
|
||
| ingress { | ||
| description = "Allow all TCP traffic" | ||
| from_port = 0 | ||
| to_port = 65535 | ||
| protocol = "tcp" | ||
| cidr_blocks = ["0.0.0.0/0"] | ||
| } | ||
|
|
||
| egress { | ||
| description = "Allow all outbound traffic" | ||
| from_port = 0 | ||
| to_port = 0 | ||
| protocol = "-1" | ||
| cidr_blocks = ["0.0.0.0/0"] | ||
| } | ||
| } |
There was a problem hiding this comment.
Dangerous: Insecure security group exposing all TCP ports
This resource opens every TCP port to the internet, constituting a major security vulnerability. If this is only for testing, guard it behind a toggle or isolate it in a dedicated test workspace.
Example toggle using a variable:
+variable "enable_insecure_sg" {
+ type = bool
+ default = false
+}
resource "aws_security_group" "insecure" {
+ count = var.enable_insecure_sg ? 1 : 0
name = "insecure-sg"
description = "Insecure SG exposing all TCP ports to the world"
vpc_id = local.aws_vpc_id
...
}| resource "aws_s3_bucket" "public" { | ||
| bucket = "checkov-public-bucket-demo-12345" | ||
| acl = "public-read" # Vulnerability: Bucket is publicly readable | ||
|
|
||
| versioning { | ||
| enabled = false | ||
| } | ||
|
|
||
| # Intentionally not configuring block public access to expose potential risk | ||
| website { | ||
| index_document = "index.html" | ||
| } | ||
| } |
There was a problem hiding this comment.
Secure public S3 bucket configuration
A public-read ACL without block public access settings leaves this bucket exposed. Recommend removing the ACL, enabling block public access, and using a bucket policy for controlled access.
Apply this diff:
resource "aws_s3_bucket" "public" {
bucket = "checkov-public-bucket-demo-12345"
- acl = "public-read"
+ # acl = "public-read" # tightened for security
+
+ block_public_acls = true
+ block_public_policy = true
+ ignore_public_acls = true
+ restrict_public_buckets = true
versioning {
enabled = false
}
website {
index_document = "index.html"
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resource "aws_s3_bucket" "public" { | |
| bucket = "checkov-public-bucket-demo-12345" | |
| acl = "public-read" # Vulnerability: Bucket is publicly readable | |
| versioning { | |
| enabled = false | |
| } | |
| # Intentionally not configuring block public access to expose potential risk | |
| website { | |
| index_document = "index.html" | |
| } | |
| } | |
| resource "aws_s3_bucket" "public" { | |
| bucket = "checkov-public-bucket-demo-12345" | |
| # acl = "public-read" # tightened for security | |
| block_public_acls = true | |
| block_public_policy = true | |
| ignore_public_acls = true | |
| restrict_public_buckets = true | |
| versioning { | |
| enabled = false | |
| } | |
| website { | |
| index_document = "index.html" | |
| } | |
| } |
|
@alexcrtestapp full review |
2 similar comments
|
@alexcrtestapp full review |
|
@alexcrtestapp full review |
✅ Actions performedFull review triggered. |
|
@alexcrtestapp full review |
Summary by CodeRabbit