Respond to PR comments

pkg/teams/team_map_test_invalid_arn.json

@@ -9,7 +9,8 @@
         },
         {
           "environment": "prod",
-          "id": "account 2"
+          "id": "account 2",
+          "roleArn": "arn:aws:iam::000000000011:role/CustomRole"
         }
       ],
       "name": "Test Team 1"

pkg/teams/teams.go

@@ -19,6 +19,14 @@ func (e *duplicateAccountIDError) Error() string {
 	return e.message
 }
 
+type invalidRoleARNError struct {
+	message string
+}
+
+func (e *invalidRoleARNError) Error() string {
+	return e.message
+}
+
 // Teams is a struct describing the format we expect in the JSON file
 // describing the team mappings
 type Teams struct {
@@ -34,9 +42,9 @@ type Team struct {
 
 // Account is a struct describing a single account for a team
 type Account struct {
-	ID              string `json:"id"`
-	Environment     string `json:"environment"`
-	RoleARN string `json:"roleArn"`
+	ID          string `json:"id"`
+	Environment string `json:"environment"`
+	RoleARN     string `json:"roleArn"`
 }
 
 // ParseTeamMap takes a path to a team mapping JSON file, reads the file, and returns a Go map of Accounts to team names
@@ -106,7 +114,9 @@ func (t *Teams) accountsToTeamNames() (map[Account]string, error) {
 			}
 
 			if !arn.IsARN(account.RoleARN) {
-				return nil, fmt.Errorf("invalid role ARN override for account %s: %s Input must be a valid Role ARN", account.ID, account.RoleARN)
+				return nil, &invalidRoleARNError{
+					message: fmt.Sprintf("invalid role ARN for account %s: %s Input must be a valid Role ARN", account.ID, account.RoleARN),
+				}
 			}
 			a[account] = team.Name
 		}

pkg/teams/teams_test.go

@@ -4,7 +4,6 @@ import (
 	"errors"
 	"fmt"
 	"reflect"
-	"strings"
 	"testing"
 )
 
@@ -27,17 +26,17 @@ func TestParseTeamMap(t *testing.T) {
 
 	// this test checks that a duplicate account ID is caught
 	_, err = ParseTeamMap("team_map_test_duplicate.json")
-	fmt.Printf("err: %v", err)
+	fmt.Printf("err: %v\n", err)
 	var duplicateAccountIDError *duplicateAccountIDError
 	if err == nil || !errors.As(err, &duplicateAccountIDError) {
 		t.Error("ERROR: didn't get expected error for duplicate account ID", err)
 	}
 
 	// Test invalid ARN
 	_, err = ParseTeamMap("team_map_test_invalid_arn.json")
-	if err == nil {
-		t.Error("ERROR: expected error for invalid ARN, but got nil")
-	} else if !strings.Contains(err.Error(), "invalid role ARN for account") {
-		t.Errorf("ERROR: unexpected error message for invalid ARN: %s", err)
+	fmt.Printf("err: %v\n", err)
+	var invalidRoleARNError *invalidRoleARNError
+	if err == nil || !errors.As(err, &invalidRoleARNError) {
+		t.Error("ERROR: didn't get expected error for invalid Role ARN", err)
 	}
 }

terraform/README.md

@@ -1,2 +1,2 @@
 /collector contains the actual collector application so that we can deploy to verify customer issues.
-/ecr contains the terraform that sets up the infrastrcture to deploy the image and share it to customers
+/ecr contains the terraform that sets up the infrastructure to deploy the image and share it to customers

terraform/collector/main.tf

@@ -1,6 +1,6 @@
 ########## Building in us-east-1 ##########
 provider "aws" {
-  region = "us-east-1"
+  region              = "us-east-1"
   allowed_account_ids = ["037370603820"]
 
   default_tags {
@@ -29,22 +29,47 @@ terraform {
 ########## Create s3 bucket for storing the collected findings ##########
 resource "aws_s3_bucket" "security_hub_collector" {
   bucket = var.security_hub_collector_results_bucket_name
-  #acl    = "private"
+}
 
-  #lifecycle_rule {
-  #  enabled = true
+resource "aws_s3_bucket_server_side_encryption_configuration" "security_hub_collector" {
+  bucket = aws_s3_bucket.security_hub_collector.id
 
-   # expiration {
-   #   days = 10
-   # }
-  #}
+  rule {
+    apply_server_side_encryption_by_default {
+      sse_algorithm = "AES256"
+    }
+  }
+}
 
-  tags = {
-    Automation  = "Terraform"
-    Environment = "dev"
+resource "aws_s3_bucket_lifecycle_configuration" "security_hub_collector" {
+  bucket = aws_s3_bucket.security_hub_collector.id
+
+  rule {
+    id     = "security-hub-collector"
+    status = "Enabled"
+
+    expiration {
+      days = 90
+    }
   }
 }
 
+resource "aws_s3_bucket_public_access_block" "security_hub_collector" {
+  bucket = aws_s3_bucket.security_hub_collector.id
+
+  # Block new public ACLs and uploading public objects
+  block_public_acls = true
+
+  # Retroactively remove public access granted through public ACLs
+  ignore_public_acls = true
+
+  # Block new public bucket policies
+  block_public_policy = true
+
+  # Retroactively block public and cross-account access if bucket has public policies
+  restrict_public_buckets = true
+}
+
 resource "aws_s3_bucket_policy" "security_hub_collector_bucket_policy" {
   bucket = aws_s3_bucket.security_hub_collector.id
   policy = jsonencode({
@@ -62,19 +87,19 @@ resource "aws_s3_bucket_policy" "security_hub_collector_bucket_policy" {
         ]
       },
       {
-        Action    = "s3:*"
+        Action = "s3:*"
         Condition = {
-            Bool = {
-              "aws:SecureTransport" = "false",
-            }
+          Bool = {
+            "aws:SecureTransport" = "false",
+          }
         }
         Effect    = "Deny"
         Principal = "*"
-        Resource  = [
-              aws_s3_bucket.security_hub_collector.arn,
-            "${aws_s3_bucket.security_hub_collector.arn}/*",
+        Resource = [
+          aws_s3_bucket.security_hub_collector.arn,
+          "${aws_s3_bucket.security_hub_collector.arn}/*",
         ]
-        Sid       = "AllowSSLRequestsOnly"
+        Sid = "AllowSSLRequestsOnly"
       }
     ]
   })
@@ -87,30 +112,30 @@ resource "aws_cloudwatch_log_group" "aws-scanner-inspec" {
 }
 
 resource "aws_ecs_cluster" "security_hub_collector_runner" {
-  name = "security_hub_collector"
-    }
+  name = "security-hub-collector"
+}
 
 ########## Use the securityhub collector runner module ##########
 module "security_hub_collector_runner" {
-  source      = "/home/acremins/corbalt/github.com/Enterprise-CMCS/mac-fc-security-hub-collector-ecs-runner"
+  source = "/home/acremins/corbalt/github.com/Enterprise-CMCS/mac-fc-security-hub-collector-ecs-runner"
   #source      = "github.com/CMSgov/security-hub-collector-ecs-runner?ref=9b76aea273ce9c27c50257c10b23ae921ab99416" TODO: Remove hardcoded path and update ref once security-hub-collector-ecs-runner PR is merged
-  app_name    = "security-hub"
-  environment = "dev"
-  task_name      = "scheduled-collector"
-  repo_arn       = "arn:aws:ecr:us-east-1:037370603820:repository/security-hub-collector"
-  repo_url       = "037370603820.dkr.ecr.us-east-1.amazonaws.com/security-hub-collector"
-  repo_tag       = "d93a473"
-  ecs_vpc_id     = var.ecs_vpc_id
-  ecs_subnet_ids = var.ecs_subnet_ids
+  app_name                  = "security-hub"
+  environment               = "dev"
+  task_name                 = "scheduled-collector"
+  repo_arn                  = "arn:aws:ecr:us-east-1:037370603820:repository/security-hub-collector"
+  repo_url                  = "037370603820.dkr.ecr.us-east-1.amazonaws.com/security-hub-collector"
+  repo_tag                  = "d93a473"
+  ecs_vpc_id                = var.ecs_vpc_id
+  ecs_subnet_ids            = var.ecs_subnet_ids
   schedule_task_expression  = var.schedule_task_expression
   logs_cloudwatch_group_arn = aws_cloudwatch_log_group.aws-scanner-inspec.arn
   ecs_cluster_arn           = aws_ecs_cluster.security_hub_collector_runner.arn
-  output_path       = var.output_path  //optional
-  s3_results_bucket = var.security_hub_collector_results_bucket_name
-  s3_key            = var.s3_key //optional
-  assign_public_ip  = var.assign_public_ip
-  role_path         = "/delegatedadmin/developer/"
-  permissions_boundary = "arn:aws:iam::037370603820:policy/cms-cloud-admin/developer-boundary-policy"
-  team_map = base64encode(file("${path.module}/team_map.json"))
-  scheduled_task_state = "ENABLED" #Set to DISABLED to stop scheduled execution
+  output_path               = var.output_path //optional
+  s3_results_bucket         = var.security_hub_collector_results_bucket_name
+  s3_key                    = var.s3_key //optional
+  assign_public_ip          = var.assign_public_ip
+  role_path                 = "/delegatedadmin/developer/"
+  permissions_boundary      = "arn:aws:iam::037370603820:policy/cms-cloud-admin/developer-boundary-policy"
+  team_map                  = base64encode(file("${path.module}/team_map.json"))
+  scheduled_task_state      = "DISABLED" #Set to DISABLED to stop scheduled execution
 }

terraform/collector/team_map.json.example

@@ -0,0 +1,19 @@
+{
+  "teams": [
+    {
+      "accounts": [
+        {
+          "id": "XXXXXXXXX",
+          "environment": "dev",
+          "roleArn": "arn:aws:iam::XXXXXXXXX:role/security-hub-collector"
+        },
+        {
+          "id": "YYYYYYYYY",
+          "environment": "impl",
+          "roleArn": "arn:aws:iam::YYYYYYYYY:role/security-hub-collector"
+        }
+      ],
+      "name": "My Team"
+    }
+  ]
+}

terraform/collector/terraform.tfvars

@@ -1,7 +1,7 @@
 ecs_vpc_id                                 = "vpc-07f4de56f6970729d"
-ecs_subnet_ids                             = ["subnet-06bbdc0b680091dd1","subnet-02d08271e8ac413b0"]
+ecs_subnet_ids                             = ["subnet-06bbdc0b680091dd1", "subnet-02d08271e8ac413b0"]
 security_hub_collector_results_bucket_name = "securityhub-collector-results-037370603820s"
-schedule_task_expression                   =  "cron(*/5 * * * ? *)"
+schedule_task_expression                   = "cron(*/5 * * * ? *)"
 output_path                                = ""
 s3_key                                     = ""
 assume_role                                = "security-hub-collector"

terraform/collector/variables.tf

@@ -1,9 +1,44 @@
-variable "ecs_vpc_id" {}
-variable "ecs_subnet_ids" {}
-variable "security_hub_collector_results_bucket_name" {}
-variable "schedule_task_expression" {}
-variable "output_path" {}
-variable "s3_key" {}
-variable "assume_role" {}
-variable "aws_cloudwatch_log_group_name" {}
-variable "assign_public_ip" {}
+variable "ecs_vpc_id" {
+  description = "The ID of the VPC where the ECS tasks will run"
+  type        = string
+}
+
+variable "ecs_subnet_ids" {
+  description = "A list of subnet IDs where the ECS tasks will be placed"
+  type        = list(string)
+}
+
+variable "security_hub_collector_results_bucket_name" {
+  description = "The name of the S3 bucket where Security Hub collector results will be stored"
+  type        = string
+}
+
+variable "schedule_task_expression" {
+  description = "The schedule expression for when the ECS task should run (e.g., cron or rate expression)"
+  type        = string
+}
+
+variable "output_path" {
+  description = "The path where output files will be saved"
+  type        = string
+}
+
+variable "s3_key" {
+  description = "The S3 key (path) where files will be stored in the S3 bucket"
+  type        = string
+}
+
+variable "assume_role" {
+  description = "The ARN of the IAM role to be assumed by the ECS task"
+  type        = string
+}
+
+variable "aws_cloudwatch_log_group_name" {
+  description = "The name of the CloudWatch log group where ECS task logs will be sent"
+  type        = string
+}
+
+variable "assign_public_ip" {
+  description = "Whether to assign a public IP address to the ECS task"
+  type        = bool
+}