-
Notifications
You must be signed in to change notification settings - Fork 2
Add GuardDuty malware scanning to storage module #258
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
Open
jrpbc
wants to merge
2
commits into
main
Choose a base branch
from
johan/storage-malware-scanning
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| variable "enable_threatdetection" { | ||
| description = "Whether to enable the ThreatDetection detector for the account" | ||
| type = bool | ||
| default = true | ||
| } | ||
|
|
||
| variable "threatdetection_finding_publishing_frequency" { | ||
| description = "The frequency of notifications sent for subsequent ThreatDetection finding occurrences" | ||
| type = string | ||
| default = "FIFTEEN_MINUTES" | ||
|
|
||
| validation { | ||
| condition = contains([ | ||
| "FIFTEEN_MINUTES", | ||
| "ONE_HOUR", | ||
| "SIX_HOURS" | ||
| ], var.threatdetection_finding_publishing_frequency) | ||
| error_message = "Finding publishing frequency must be one of: FIFTEEN_MINUTES, ONE_HOUR, SIX_HOURS." | ||
| } | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,10 +15,11 @@ resource "aws_s3_bucket_policy" "storage" { | |
| } | ||
|
|
||
| data "aws_iam_policy_document" "storage" { | ||
| # Require HTTPS connections | ||
| statement { | ||
| sid = "RestrictToTLSRequestsOnly" | ||
| effect = "Deny" | ||
| actions = ["s3:*"] | ||
| sid = "RestrictToTLSRequestsOnly" | ||
| effect = "Deny" | ||
| actions = ["s3:*"] | ||
|
Comment on lines
+20
to
+22
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These formatting changes seem unrelated, remove? |
||
| resources = [aws_s3_bucket.storage.arn] | ||
| principals { | ||
| type = "*" | ||
|
|
@@ -30,6 +31,26 @@ data "aws_iam_policy_document" "storage" { | |
| values = ["false"] | ||
| } | ||
| } | ||
|
|
||
| # Block access to objects tagged with malware threats | ||
| statement { | ||
| sid = "BlockMalwareThreats" | ||
| effect = "Deny" | ||
| actions = [ | ||
| "s3:GetObject", | ||
| "s3:GetObjectVersion" | ||
| ] | ||
| resources = ["${aws_s3_bucket.storage.arn}/*"] | ||
| principals { | ||
| type = "*" | ||
| identifiers = ["*"] | ||
| } | ||
| condition { | ||
| test = "StringEquals" | ||
| variable = "s3:ExistingObjectTag/GuardDutyMalwareScanStatus" | ||
| values = ["THREATS_FOUND"] | ||
| } | ||
| } | ||
| } | ||
|
|
||
| # Create policy for read/write access | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,139 @@ | ||
| # S3-specific GuardDuty malware protection | ||
| # This configures malware scanning for this specific S3 bucket | ||
|
|
||
| # Get current account ID | ||
| data "aws_caller_identity" "identity" {} | ||
|
|
||
| # Get current region | ||
| data "aws_region" "region" {} | ||
|
|
||
| # Configure S3 bucket for malware scanning | ||
| resource "aws_guardduty_malware_protection_plan" "s3_malware_protection" { | ||
| role = aws_iam_role.guardduty_malware_protection.arn | ||
|
|
||
| protected_resource { | ||
| s3_bucket { | ||
| bucket_name = aws_s3_bucket.storage.bucket | ||
| object_prefixes = [] # Scan all objects in the bucket | ||
| } | ||
| } | ||
|
|
||
| actions { | ||
| tagging { | ||
| status = "ENABLED" | ||
| } | ||
| } | ||
|
|
||
| depends_on = [ | ||
| aws_iam_role.guardduty_malware_protection, | ||
| aws_iam_role_policy.guardduty_malware_protection, | ||
| aws_s3_bucket.storage | ||
| ] | ||
| } | ||
|
|
||
| # IAM role for GuardDuty Malware Protection service | ||
| resource "aws_iam_role" "guardduty_malware_protection" { | ||
| name = "${var.name}-guardduty-malware-protection" | ||
|
|
||
| assume_role_policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Action = "sts:AssumeRole" | ||
| Effect = "Allow" | ||
| Principal = { | ||
| Service = "malware-protection-plan.guardduty.amazonaws.com" | ||
| } | ||
| Condition = { | ||
| StringEquals = { | ||
| "aws:SourceAccount" = data.aws_caller_identity.identity.account_id | ||
| } | ||
| } | ||
| } | ||
| ] | ||
| }) | ||
| } | ||
|
|
||
| # IAM policy for GuardDuty Malware Protection | ||
| resource "aws_iam_role_policy" "guardduty_malware_protection" { | ||
| name = "${var.name}-guardduty-malware-protection-policy" | ||
| role = aws_iam_role.guardduty_malware_protection.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetObject", | ||
| "s3:GetObjectTagging", | ||
| "s3:GetObjectVersion", | ||
| "s3:GetObjectVersionTagging", | ||
| "s3:PutObjectTagging", | ||
| "s3:PutObjectVersionTagging" | ||
| ] | ||
| Resource = [ | ||
| "${aws_s3_bucket.storage.arn}/*" | ||
| ] | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketLocation", | ||
| "s3:GetBucketVersioning", | ||
| "s3:ListBucket", | ||
| "s3:GetBucketOwnershipControls", | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:GetBucketPolicy", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketAcl", | ||
| "s3:GetBucketNotification", | ||
| "s3:GetBucketTagging" | ||
| ] | ||
| Resource = [ | ||
| aws_s3_bucket.storage.arn | ||
| ] | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "kms:Decrypt", | ||
| "kms:DescribeKey", | ||
| "kms:GenerateDataKey" | ||
| ] | ||
| Resource = [ | ||
| aws_kms_key.storage.arn | ||
| ] | ||
| Condition = { | ||
| StringEquals = { | ||
| "kms:ViaService" = "s3.${data.aws_region.region.name}.amazonaws.com" | ||
| } | ||
| } | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "iam:GetRole", | ||
| "sts:AssumeRole" | ||
| ] | ||
| Resource = [ | ||
| aws_iam_role.guardduty_malware_protection.arn | ||
| ] | ||
| }, | ||
| { | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "events:PutRule", | ||
| "events:PutTargets", | ||
| "events:DeleteRule", | ||
| "events:RemoveTargets", | ||
| "events:DescribeRule", | ||
| "events:ListTargetsByRule" | ||
| ] | ||
| Resource = [ | ||
| "arn:aws:events:${data.aws_region.region.name}:${data.aws_caller_identity.identity.account_id}:rule/DO-NOT-DELETE-AmazonGuardDutyMalwareProtectionS3-*" | ||
| ] | ||
| } | ||
| ] | ||
| }) | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # GuardDuty centralized module | ||
| # This module manages the account-wide GuardDuty detector for malware protection | ||
|
|
||
| # Create and enable GuardDuty detector (single detector for current region) | ||
| resource "aws_guardduty_detector" "main" { | ||
| # checkov:skip=CKV2_AWS_3:GuardDuty is enabled for this specific region/account - org-level management not required for single-account setup | ||
| enable = var.enable_detector | ||
| finding_publishing_frequency = var.finding_publishing_frequency | ||
| } | ||
|
|
||
| # TODO: When upgrading to AWS provider >= 5.7.0, uncomment the following for multi-region support: | ||
| # resource "aws_guardduty_detector" "main" { | ||
| # for_each = toset(var.utilized_regions) | ||
| # # checkov:skip=CKV2_AWS_3:GuardDuty is enabled for this specific region/account - org-level management not required for single-account setup | ||
| # enable = var.enable_detector | ||
| # region = each.value | ||
| # finding_publishing_frequency = var.finding_publishing_frequency | ||
| # } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,22 @@ | ||
| # Outputs for GuardDuty module | ||
|
|
||
| output "detector_id" { | ||
| description = "GuardDuty detector ID" | ||
| value = aws_guardduty_detector.main.id | ||
| } | ||
|
|
||
| output "detector_arn" { | ||
| description = "GuardDuty detector ARN" | ||
| value = aws_guardduty_detector.main.arn | ||
| } | ||
|
|
||
| # TODO: When upgrading to AWS provider >= 5.7.0, uncomment for multi-region support: | ||
| # output "detector_id" { | ||
| # description = "GuardDuty detector IDs by region" | ||
| # value = { for k, v in aws_guardduty_detector.main : k => v.id } | ||
| # } | ||
| # | ||
| # output "detector_arn" { | ||
| # description = "GuardDuty detector ARNs by region" | ||
| # value = { for k, v in aws_guardduty_detector.main : k => v.arn } | ||
| # } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| variable "enable_detector" { | ||
| description = "Whether to enable the GuardDuty detector" | ||
| type = bool | ||
| default = true | ||
| } | ||
|
|
||
| variable "finding_publishing_frequency" { | ||
| description = "The frequency of notifications sent for subsequent finding occurrences" | ||
| type = string | ||
| default = "FIFTEEN_MINUTES" | ||
|
|
||
| validation { | ||
| condition = contains([ | ||
| "FIFTEEN_MINUTES", | ||
| "ONE_HOUR", | ||
| "SIX_HOURS" | ||
| ], var.finding_publishing_frequency) | ||
| error_message = "Finding publishing frequency must be one of: FIFTEEN_MINUTES, ONE_HOUR, SIX_HOURS." | ||
| } | ||
| } | ||
|
|
||
| # TODO: When upgrading to AWS provider >= 5.7.0, uncomment for multi-region support: | ||
| # variable "utilized_regions" { | ||
| # description = <<-EOF | ||
| # List of AWS regions that GuardDuty should be enabled in. | ||
| # This should typically include all regions that are being used in the project, | ||
| # especially if there are resources in those regions that GuardDuty can monitor for security threats. | ||
| # EOF | ||
| # type = list(string) | ||
| # default = [] | ||
| # } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is account-wide, but region specific.
template-infraonly really supportsproject_config.default_region+us-east-1(for things that require existing there), but in the future we will have more greater support for multi-region configs.We could lay some ground work for that with a local var like
utilized_regions = distinct([local.region, 'us-east-1'])and create instances of the module for each region? Or have the module accept a list of regions.Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree. thoughts on adding
utilized_regionsas a list type variable, in project-config defaulting todefault_region? I just added it, and tried to use it within thethreatdectionmodule - had to revert back thethreatdetectionwork - the current Terraform AWS Provider we are using ("5.6.0") does not support specifyingregionon theaws_guardduty_detector. This was added on latest version of the AWS Terraform provider. Added TODO comments on the infra/modules/threatdetection.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please create issues in template-infra (if they don't exist) for:
Then link the latter here.