Red PR: Demonstrate compliance gaps (no baseline fixes)#3
Open
joserodriguezthe1 wants to merge 4 commits into
Open
Red PR: Demonstrate compliance gaps (no baseline fixes)#3joserodriguezthe1 wants to merge 4 commits into
joserodriguezthe1 wants to merge 4 commits into
Conversation
ⓘ You've reached your Qodo monthly free-tier limit. Reviews pause until next month — upgrade your plan to continue now, or link your paid account if you already have one. |
📝 WalkthroughWalkthroughThis pull request adds a comprehensive CMMC compliance framework consisting of five OPA/Rego policies that enforce cloud security controls, corresponding Terraform infrastructure to implement those controls, encrypted evidence storage with immutable compliance records, and a GitHub Actions pipeline that validates compliance on every pull request and push to main. ChangesCMMC Compliance Framework and Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (3)
policies/tests/sc1311_cmk_encryption_test.rego (1)
23-29: ⚡ Quick winAdd a failing fixture for
sse_algorithm: "AES256".This closes an important gap between “missing encryption” and “wrong encryption type”.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@policies/tests/sc1311_cmk_encryption_test.rego` around lines 23 - 29, Add a failing fixture where the S3 resource explicitly uses SSE with sse_algorithm set to "AES256": update (or add) a noncompliant_input object in policies/tests/sc1311_cmk_encryption_test.rego so the resource with address "aws_s3_bucket.uploads" includes a server_side_encryption_configuration -> rule -> apply_server_side_encryption_by_default object containing sse_algorithm: "AES256" (so the test covers wrong encryption type vs missing encryption).policies/tests/ac315_least_privilege_test.rego (1)
26-31: ⚡ Quick winAdd regression tests for nested modules and
Effect: Denycases.Current tests won’t prevent future reintroduction of bypasses/false positives in this policy.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@policies/tests/ac315_least_privilege_test.rego` around lines 26 - 31, Add regression tests that exercise nested module structures and explicit Effect: "Deny" outputs so the policy cannot be bypassed; extend policies/tests/ac315_least_privilege_test.rego by adding new test cases (e.g., test_nested_module_noncompliant and test_effect_deny_noncompliant) that invoke ac315.deny with inputs containing nested module declarations and resources marked with Effect: "Deny", assert that some msg in ac315.deny with those inputs and that contains(msg, "AC.L2-3.1.5") holds, and also add matching compliant inputs to assert count(ac315.deny) == 0 for nested/compliant scenarios to prevent regressions.policies/tests/sc138_tls_required_test.rego (1)
12-19: ⚡ Quick winStrengthen the compliant fixture to validate actual TLS enforcement logic.
Presence of a bucket policy alone is insufficient; add policy-content checks to avoid false passes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@policies/tests/sc138_tls_required_test.rego` around lines 12 - 19, The fixture for aws_s3_bucket_policy.uploads_tls only declares a policy resource but doesn't include a policy document, so tests can false-pass; update the fixture (aws_s3_bucket_policy.uploads_tls) to include a realistic "policy" or "policy_document" JSON that asserts TLS enforcement (e.g., a Deny statement for Principal:"*" on s3 actions/resources with Condition Bool {"aws:SecureTransport":"false"} or an equivalent aws:SecureTransport check), ensuring the policy references the bucket via the existing expressions.bucket reference to validate actual TLS enforcement logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/grc-gate.yml:
- Around line 10-14: Remove the unnecessary GitHub Actions permission
"pull-requests: write" from the workflow permissions block (keeping only the
least-privilege keys currently in use: "id-token: write" and "contents: read");
locate the permissions mapping that includes "pull-requests: write" and either
delete that entry or set it to "pull-requests: none" so the workflow no longer
grants write access to pull requests.
- Around line 47-50: The "Install tfsec" step currently only downloads the
binary but doesn't execute or persist scan results; modify that step (or add
immediately after) to run tfsec against the repo (e.g., tfsec --format json -o
tfsec.json ./), let its exit code surface so the job fails on findings, and then
upload tfsec.json as evidence using actions/upload-artifact (artifact name like
"tfsec-results", path tfsec.json) so results are persisted for the gate; ensure
you reference the existing "Install tfsec" step/ID when chaining these commands
so the binary is used and no continue-on-error masks failures.
In `@policies/ac315_least_privilege.rego`:
- Around line 19-21: The policy currently checks wildcard_actions against every
statement and may flag Deny statements; restrict the check to only statements
with Effect == "Allow" by adding a guard that filters statements to those where
stmt.Effect == "Allow" before iterating over stmt.Action (e.g., in the rule that
uses policy.Statement, ensure you require stmt.Effect == "Allow" prior to the
some action in to_array(stmt.Action) and wildcard_actions[action] checks).
- Around line 16-17: The rule currently only checks
input.planned_values.root_module.resources and misses resources in nested
modules; add a helper (e.g., get_all_resources(module) or
collect_resources(module)) that returns module.resources plus resources from
module.child_modules recursively, then replace the comprehension "some r in
input.planned_values.root_module.resources" with "some r in
get_all_resources(input.planned_values.root_module)" (or equivalent) and keep
the existing filter r.type == "aws_iam_role_policy".
In `@policies/sc1311_cmk_encryption.rego`:
- Around line 13-22: The rule accesses r.expressions.rule[0] unsafely which can
fail if rule is missing or empty; update the deny clause to first check that
r.expressions.rule exists and is a non-empty array (e.g., ensure some idx with
r.expressions.rule[idx]) and only then extract
apply_server_side_encryption_by_default and sse_algorithm, so modify the
predicate in deny contains msg (referencing r.expressions.rule,
apply_server_side_encryption_by_default, and sse_algorithm) to guard array
access and skip resources that don't have those nested fields.
In `@terraform/baseline.tf`:
- Around line 94-99: The current egress block in the Lambda security group
allows 0.0.0.0/0 on all ports; change it to enforce least privilege by removing
the wide allow-all rule and adding explicit egress rules for only required
destinations (e.g., HTTPS port 443 to S3, DynamoDB, and KMS service CIDR ranges
or service-specific prefixes), or alternatively remove internet egress entirely
and create/attach VPC endpoints (aws_vpc_endpoint) for S3, DynamoDB and KMS and
update the security group egress to only allow traffic to those endpoints;
update the existing egress block (and any aws_security_group_rule resources)
accordingly so only the necessary ports/protocols and destination
CIDRs/endpoints are permitted.
In `@terraform/cloudtrail.tf`:
- Around line 15-20: The server-side encryption block for
aws_s3_bucket_server_side_encryption_configuration "trail" is using AES256;
replace it with SSE-KMS by creating or referencing a CMK (e.g.,
aws_kms_key.cloudtrail) and update the rule in the "trail" resource to set
sse_algorithm = "aws:kms" and kms_master_key_id = aws_kms_key.cloudtrail.arn;
ensure the CMK policy/grants allow CloudTrail to use the key and that
aws_s3_bucket.trail has the encryption configuration pointing to the KMS key.
- Line 7: The audit-log S3 bucket currently uses force_destroy = true which
allows destructive deletion; change this to disallow destructive deletes by
removing or setting force_destroy = false and add a lifecycle block with
prevent_destroy = true on the S3 bucket resource (the audit-log / CloudTrail
bucket resource that contains force_destroy) so Terraform refuses
accidental/dangerous deletions; ensure the resource that currently declares
force_destroy also includes lifecycle { prevent_destroy = true } to enforce
immutability of audit evidence.
In `@terraform/evidence_vault.tf`:
- Around line 21-32: The Object Lock default_retention in
aws_s3_bucket_object_lock_configuration.evidence is set to 1 day which is too
short; change the retention to a multi-year value or parameterize it: replace
the default_retention days = 1 with either years = <n> (e.g., years = 7) or
reference a new variable (e.g., var.evidence_retention_years) and set that
variable to the required number of years (1–7+ as required by policy), keeping
mode = "GOVERNANCE" and leaving depends_on as-is.
In `@terraform/kms.tf`:
- Line 7: The KMS key resource's deletion_window_in_days is set to 7 which is
too short for production; update the aws_kms_key resource(s) that contain the
deletion_window_in_days attribute (and any matching occurrences like the one at
lines referenced as 24-24) to a minimum of 30 days (or a configurable variable
default >=30) so production keys protecting PHI meet compliance and allow
recovery time; ensure you update any variable defaults and
documentation/comments that reference the old 7-day window.
- Around line 5-15: The aws_kms_key "phi" resource lacks an explicit key policy;
create and attach a least-privilege policy by building a policy document (e.g.,
data "aws_iam_policy_document" "phi_kms_policy") that explicitly grants only the
required principals (specific IAM roles/users and AWS services like
kms:Encrypt/Decrypt for S3/DynamoDB service principals or your application
roles) and denies broad access, then supply that JSON to the aws_kms_key "phi"
policy attribute (or use aws_kms_key_policy) so the CMK no longer relies on the
AWS default policy and enforces restricted usage and admin permissions. Ensure
you reference and include the same principal ARNs your system uses and include a
separate admin/steward principal with kms:* for key management only.
---
Nitpick comments:
In `@policies/tests/ac315_least_privilege_test.rego`:
- Around line 26-31: Add regression tests that exercise nested module structures
and explicit Effect: "Deny" outputs so the policy cannot be bypassed; extend
policies/tests/ac315_least_privilege_test.rego by adding new test cases (e.g.,
test_nested_module_noncompliant and test_effect_deny_noncompliant) that invoke
ac315.deny with inputs containing nested module declarations and resources
marked with Effect: "Deny", assert that some msg in ac315.deny with those inputs
and that contains(msg, "AC.L2-3.1.5") holds, and also add matching compliant
inputs to assert count(ac315.deny) == 0 for nested/compliant scenarios to
prevent regressions.
In `@policies/tests/sc1311_cmk_encryption_test.rego`:
- Around line 23-29: Add a failing fixture where the S3 resource explicitly uses
SSE with sse_algorithm set to "AES256": update (or add) a noncompliant_input
object in policies/tests/sc1311_cmk_encryption_test.rego so the resource with
address "aws_s3_bucket.uploads" includes a server_side_encryption_configuration
-> rule -> apply_server_side_encryption_by_default object containing
sse_algorithm: "AES256" (so the test covers wrong encryption type vs missing
encryption).
In `@policies/tests/sc138_tls_required_test.rego`:
- Around line 12-19: The fixture for aws_s3_bucket_policy.uploads_tls only
declares a policy resource but doesn't include a policy document, so tests can
false-pass; update the fixture (aws_s3_bucket_policy.uploads_tls) to include a
realistic "policy" or "policy_document" JSON that asserts TLS enforcement (e.g.,
a Deny statement for Principal:"*" on s3 actions/resources with Condition Bool
{"aws:SecureTransport":"false"} or an equivalent aws:SecureTransport check),
ensuring the policy references the bucket via the existing expressions.bucket
reference to validate actual TLS enforcement logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 52acea8c-5c93-4d3c-abb9-475441fc9930
⛔ Files ignored due to path filters (8)
oscal/oscal/dist/assessment-plans/.keepis excluded by!**/dist/**oscal/oscal/dist/assessment-results/.keepis excluded by!**/dist/**oscal/oscal/dist/catalogs/.keepis excluded by!**/dist/**oscal/oscal/dist/component-definitions/.keepis excluded by!**/dist/**oscal/oscal/dist/mapping-collections/.keepis excluded by!**/dist/**oscal/oscal/dist/plan-of-action-and-milestones/.keepis excluded by!**/dist/**oscal/oscal/dist/profiles/.keepis excluded by!**/dist/**oscal/oscal/dist/system-security-plans/.keepis excluded by!**/dist/**
📒 Files selected for processing (28)
.github/workflows/grc-gate.ymlREADME.mdWRITEUP.mdoscal/oscal/.trestle/.keeposcal/oscal/.trestle/config.inioscal/oscal/assessment-plans/.keeposcal/oscal/assessment-results/.keeposcal/oscal/catalogs/.keeposcal/oscal/component-definitions/.keeposcal/oscal/mapping-collections/.keeposcal/oscal/plan-of-action-and-milestones/.keeposcal/oscal/profiles/.keeposcal/oscal/system-security-plans/.keeppolicies/ac315_least_privilege.regopolicies/au331_audit_logging.regopolicies/mp389_versioning.regopolicies/sc1311_cmk_encryption.regopolicies/sc138_tls_required.regopolicies/tests/ac315_least_privilege_test.regopolicies/tests/au331_audit_logging_test.regopolicies/tests/mp389_versioning_test.regopolicies/tests/sc1311_cmk_encryption_test.regopolicies/tests/sc138_tls_required_test.regoscripts/verify-evidence.shterraform/baseline.tfterraform/cloudtrail.tfterraform/evidence_vault.tfterraform/kms.tf
Comment on lines
+10
to
+14
| permissions: | ||
| id-token: write | ||
| contents: read | ||
| pull-requests: write | ||
|
|
There was a problem hiding this comment.
Reduce GitHub token permissions to least privilege.
pull-requests: write is not used here and increases blast radius unnecessarily.
Suggested fix
permissions:
id-token: write
contents: read
- pull-requests: write📝 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.
Suggested change
| permissions: | |
| id-token: write | |
| contents: read | |
| pull-requests: write | |
| permissions: | |
| id-token: write | |
| contents: read | |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/grc-gate.yml around lines 10 - 14, Remove the unnecessary
GitHub Actions permission "pull-requests: write" from the workflow permissions
block (keeping only the least-privilege keys currently in use: "id-token: write"
and "contents: read"); locate the permissions mapping that includes
"pull-requests: write" and either delete that entry or set it to "pull-requests:
none" so the workflow no longer grants write access to pull requests.
Comment on lines
+47
to
+50
| - name: Install tfsec | ||
| run: | | ||
| curl -fsSL https://github.com/aquasecurity/tfsec/releases/download/v1.28.14/tfsec-linux-amd64 \ | ||
| -o /usr/local/bin/tfsec && chmod +x /usr/local/bin/tfsec |
There was a problem hiding this comment.
tfsec is installed but not part of the gate.
Right now this step adds runtime but no control signal; execute it and persist results in evidence.
Suggested fix
- name: Install tfsec
run: |
curl -fsSL https://github.com/aquasecurity/tfsec/releases/download/v1.28.14/tfsec-linux-amd64 \
-o /usr/local/bin/tfsec && chmod +x /usr/local/bin/tfsec
+ - name: Step 2a - tfsec scan
+ working-directory: ${{ env.TF_WORKING_DIR }}
+ run: |
+ tfsec --format json --out $GITHUB_WORKSPACE/evidence/tfsec-results.json .📝 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.
Suggested change
| - name: Install tfsec | |
| run: | | |
| curl -fsSL https://github.com/aquasecurity/tfsec/releases/download/v1.28.14/tfsec-linux-amd64 \ | |
| -o /usr/local/bin/tfsec && chmod +x /usr/local/bin/tfsec | |
| - name: Install tfsec | |
| run: | | |
| curl -fsSL https://github.com/aquasecurity/tfsec/releases/download/v1.28.14/tfsec-linux-amd64 \ | |
| -o /usr/local/bin/tfsec && chmod +x /usr/local/bin/tfsec | |
| - name: Step 2a - tfsec scan | |
| working-directory: ${{ env.TF_WORKING_DIR }} | |
| run: | | |
| tfsec --format json --out $GITHUB_WORKSPACE/evidence/tfsec-results.json . |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/grc-gate.yml around lines 47 - 50, The "Install tfsec"
step currently only downloads the binary but doesn't execute or persist scan
results; modify that step (or add immediately after) to run tfsec against the
repo (e.g., tfsec --format json -o tfsec.json ./), let its exit code surface so
the job fails on findings, and then upload tfsec.json as evidence using
actions/upload-artifact (artifact name like "tfsec-results", path tfsec.json) so
results are persisted for the gate; ensure you reference the existing "Install
tfsec" step/ID when chaining these commands so the binary is used and no
continue-on-error masks failures.
Comment on lines
+16
to
+17
| some r in input.planned_values.root_module.resources | ||
| r.type == "aws_iam_role_policy" |
There was a problem hiding this comment.
Policy misses IAM policies declared in child modules.
The rule only inspects root_module.resources, so wildcard actions inside nested modules bypass this control.
Suggested fix
+iam_role_policies contains r if {
+ walk(input.planned_values.root_module, [_, r])
+ r.type == "aws_iam_role_policy"
+}
+
deny contains msg if {
- some r in input.planned_values.root_module.resources
- r.type == "aws_iam_role_policy"
+ some r in iam_role_policies
policy := json.unmarshal(r.values.policy)
some stmt in policy.Statement
some action in to_array(stmt.Action)📝 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.
Suggested change
| some r in input.planned_values.root_module.resources | |
| r.type == "aws_iam_role_policy" | |
| iam_role_policies contains r if { | |
| walk(input.planned_values.root_module, [_, r]) | |
| r.type == "aws_iam_role_policy" | |
| } | |
| deny contains msg if { | |
| some r in iam_role_policies | |
| policy := json.unmarshal(r.values.policy) | |
| some stmt in policy.Statement | |
| some action in to_array(stmt.Action) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policies/ac315_least_privilege.rego` around lines 16 - 17, The rule currently
only checks input.planned_values.root_module.resources and misses resources in
nested modules; add a helper (e.g., get_all_resources(module) or
collect_resources(module)) that returns module.resources plus resources from
module.child_modules recursively, then replace the comprehension "some r in
input.planned_values.root_module.resources" with "some r in
get_all_resources(input.planned_values.root_module)" (or equivalent) and keep
the existing filter r.type == "aws_iam_role_policy".
Comment on lines
+19
to
+21
| some stmt in policy.Statement | ||
| some action in to_array(stmt.Action) | ||
| wildcard_actions[action] |
There was a problem hiding this comment.
Filter wildcard checks to Effect: Allow statements only.
Current logic can flag defensive Deny statements and fail compliant policies.
Suggested fix
policy := json.unmarshal(r.values.policy)
some stmt in policy.Statement
+ upper(stmt.Effect) == "ALLOW"
some action in to_array(stmt.Action)
wildcard_actions[action]📝 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.
Suggested change
| some stmt in policy.Statement | |
| some action in to_array(stmt.Action) | |
| wildcard_actions[action] | |
| some stmt in policy.Statement | |
| upper(stmt.Effect) == "ALLOW" | |
| some action in to_array(stmt.Action) | |
| wildcard_actions[action] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policies/ac315_least_privilege.rego` around lines 19 - 21, The policy
currently checks wildcard_actions against every statement and may flag Deny
statements; restrict the check to only statements with Effect == "Allow" by
adding a guard that filters statements to those where stmt.Effect == "Allow"
before iterating over stmt.Action (e.g., in the rule that uses policy.Statement,
ensure you require stmt.Effect == "Allow" prior to the some action in
to_array(stmt.Action) and wildcard_actions[action] checks).
Comment on lines
+13
to
+22
| deny contains msg if { | ||
| some r in input.configuration.root_module.resources | ||
| r.type == "aws_s3_bucket_server_side_encryption_configuration" | ||
| algo := r.expressions.rule[0].apply_server_side_encryption_by_default[0].sse_algorithm.constant_value | ||
| algo != "aws:kms" | ||
| msg := sprintf( | ||
| "[SC.L2-3.13.11] %s: S3 bucket uses %s instead of aws:kms with a CMK. Remediation: set sse_algorithm = aws:kms and provide kms_master_key_id.", | ||
| [r.address, algo] | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add defensive check for rule array access.
Line 16 accesses r.expressions.rule[0] without verifying the array exists or has elements. If rule is empty, undefined, or not an array, the policy evaluation will fail rather than gracefully handling the missing configuration.
🛡️ Proposed fix to add defensive checks
deny contains msg if {
some r in input.configuration.root_module.resources
r.type == "aws_s3_bucket_server_side_encryption_configuration"
+ count(r.expressions.rule) > 0
+ r.expressions.rule[0].apply_server_side_encryption_by_default
algo := r.expressions.rule[0].apply_server_side_encryption_by_default[0].sse_algorithm.constant_value
algo != "aws:kms"📝 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.
Suggested change
| deny contains msg if { | |
| some r in input.configuration.root_module.resources | |
| r.type == "aws_s3_bucket_server_side_encryption_configuration" | |
| algo := r.expressions.rule[0].apply_server_side_encryption_by_default[0].sse_algorithm.constant_value | |
| algo != "aws:kms" | |
| msg := sprintf( | |
| "[SC.L2-3.13.11] %s: S3 bucket uses %s instead of aws:kms with a CMK. Remediation: set sse_algorithm = aws:kms and provide kms_master_key_id.", | |
| [r.address, algo] | |
| ) | |
| } | |
| deny contains msg if { | |
| some r in input.configuration.root_module.resources | |
| r.type == "aws_s3_bucket_server_side_encryption_configuration" | |
| count(r.expressions.rule) > 0 | |
| r.expressions.rule[0].apply_server_side_encryption_by_default | |
| algo := r.expressions.rule[0].apply_server_side_encryption_by_default[0].sse_algorithm.constant_value | |
| algo != "aws:kms" | |
| msg := sprintf( | |
| "[SC.L2-3.13.11] %s: S3 bucket uses %s instead of aws:kms with a CMK. Remediation: set sse_algorithm = aws:kms and provide kms_master_key_id.", | |
| [r.address, algo] | |
| ) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@policies/sc1311_cmk_encryption.rego` around lines 13 - 22, The rule accesses
r.expressions.rule[0] unsafely which can fail if rule is missing or empty;
update the deny clause to first check that r.expressions.rule exists and is a
non-empty array (e.g., ensure some idx with r.expressions.rule[idx]) and only
then extract apply_server_side_encryption_by_default and sse_algorithm, so
modify the predicate in deny contains msg (referencing r.expressions.rule,
apply_server_side_encryption_by_default, and sse_algorithm) to guard array
access and skip resources that don't have those nested fields.
|
|
||
| resource "aws_s3_bucket" "trail" { | ||
| bucket = "${local.name_prefix}-cloudtrail-${local.suffix}" | ||
| force_destroy = true |
There was a problem hiding this comment.
Do not allow destructive deletion of the audit-log bucket.
force_destroy = true permits wiping CloudTrail evidence, which is a compliance blocker for audit integrity.
Suggested fix
resource "aws_s3_bucket" "trail" {
bucket = "${local.name_prefix}-cloudtrail-${local.suffix}"
- force_destroy = true
+ force_destroy = false
+
+ lifecycle {
+ prevent_destroy = true
+ }📝 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.
Suggested change
| force_destroy = true | |
| force_destroy = false | |
| lifecycle { | |
| prevent_destroy = true | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/cloudtrail.tf` at line 7, The audit-log S3 bucket currently uses
force_destroy = true which allows destructive deletion; change this to disallow
destructive deletes by removing or setting force_destroy = false and add a
lifecycle block with prevent_destroy = true on the S3 bucket resource (the
audit-log / CloudTrail bucket resource that contains force_destroy) so Terraform
refuses accidental/dangerous deletions; ensure the resource that currently
declares force_destroy also includes lifecycle { prevent_destroy = true } to
enforce immutability of audit evidence.
Comment on lines
+15
to
+20
| resource "aws_s3_bucket_server_side_encryption_configuration" "trail" { | ||
| bucket = aws_s3_bucket.trail.id | ||
| rule { | ||
| apply_server_side_encryption_by_default { | ||
| sse_algorithm = "AES256" | ||
| } |
There was a problem hiding this comment.
Use SSE-KMS for CloudTrail logs instead of SSE-S3.
For this compliance profile, AES256 is weaker than CMK-backed encryption and can create control drift.
Suggested fix
resource "aws_s3_bucket_server_side_encryption_configuration" "trail" {
bucket = aws_s3_bucket.trail.id
rule {
apply_server_side_encryption_by_default {
- sse_algorithm = "AES256"
+ sse_algorithm = "aws:kms"
+ kms_master_key_id = aws_kms_key.cloudtrail.arn
}
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/cloudtrail.tf` around lines 15 - 20, The server-side encryption
block for aws_s3_bucket_server_side_encryption_configuration "trail" is using
AES256; replace it with SSE-KMS by creating or referencing a CMK (e.g.,
aws_kms_key.cloudtrail) and update the rule in the "trail" resource to set
sse_algorithm = "aws:kms" and kms_master_key_id = aws_kms_key.cloudtrail.arn;
ensure the CMK policy/grants allow CloudTrail to use the key and that
aws_s3_bucket.trail has the encryption configuration pointing to the KMS key.
Comment on lines
+21
to
+32
| resource "aws_s3_bucket_object_lock_configuration" "evidence" { | ||
| bucket = aws_s3_bucket.evidence.id | ||
|
|
||
| rule { | ||
| default_retention { | ||
| mode = "GOVERNANCE" | ||
| days = 1 | ||
| } | ||
| } | ||
|
|
||
| depends_on = [aws_s3_bucket_versioning.evidence] | ||
| } |
There was a problem hiding this comment.
Evidence retention period is insufficient for compliance.
The Object Lock retention is set to only 1 day, which is far too short for compliance evidence that must satisfy audit and accountability requirements. CMMC AU.L2-3.3.1 and typical regulatory frameworks require audit evidence retention for 1-7 years depending on the specific compliance regime.
📦 Proposed fix to increase retention period
rule {
default_retention {
mode = "GOVERNANCE"
- days = 1
+ days = 2555 # 7 years
}
}📝 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.
Suggested change
| resource "aws_s3_bucket_object_lock_configuration" "evidence" { | |
| bucket = aws_s3_bucket.evidence.id | |
| rule { | |
| default_retention { | |
| mode = "GOVERNANCE" | |
| days = 1 | |
| } | |
| } | |
| depends_on = [aws_s3_bucket_versioning.evidence] | |
| } | |
| resource "aws_s3_bucket_object_lock_configuration" "evidence" { | |
| bucket = aws_s3_bucket.evidence.id | |
| rule { | |
| default_retention { | |
| mode = "GOVERNANCE" | |
| days = 2555 # 7 years | |
| } | |
| } | |
| depends_on = [aws_s3_bucket_versioning.evidence] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/evidence_vault.tf` around lines 21 - 32, The Object Lock
default_retention in aws_s3_bucket_object_lock_configuration.evidence is set to
1 day which is too short; change the retention to a multi-year value or
parameterize it: replace the default_retention days = 1 with either years = <n>
(e.g., years = 7) or reference a new variable (e.g.,
var.evidence_retention_years) and set that variable to the required number of
years (1–7+ as required by policy), keeping mode = "GOVERNANCE" and leaving
depends_on as-is.
Comment on lines
+5
to
+15
| resource "aws_kms_key" "phi" { | ||
| description = "CMK for Acme Health PHI data - S3 and DynamoDB" | ||
| deletion_window_in_days = 7 | ||
| enable_key_rotation = true | ||
|
|
||
| tags = { | ||
| Name = "${local.name_prefix}-phi-cmk" | ||
| ControlRef = "SC.L2-3.13.11" | ||
| DataClass = "phi" | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Consider defining explicit KMS key policies.
Neither KMS key has an explicit key policy defined, which means they will use AWS's default key policy. For CMMC compliance and defense-in-depth, explicitly define key policies that enforce least-privilege access and restrict key usage to authorized principals only.
Also applies to: 22-31
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/kms.tf` around lines 5 - 15, The aws_kms_key "phi" resource lacks
an explicit key policy; create and attach a least-privilege policy by building a
policy document (e.g., data "aws_iam_policy_document" "phi_kms_policy") that
explicitly grants only the required principals (specific IAM roles/users and AWS
services like kms:Encrypt/Decrypt for S3/DynamoDB service principals or your
application roles) and denies broad access, then supply that JSON to the
aws_kms_key "phi" policy attribute (or use aws_kms_key_policy) so the CMK no
longer relies on the AWS default policy and enforces restricted usage and admin
permissions. Ensure you reference and include the same principal ARNs your
system uses and include a separate admin/steward principal with kms:* for key
management only.
|
|
||
| resource "aws_kms_key" "phi" { | ||
| description = "CMK for Acme Health PHI data - S3 and DynamoDB" | ||
| deletion_window_in_days = 7 |
There was a problem hiding this comment.
Increase KMS key deletion window for production compliance.
The deletion_window_in_days = 7 is dangerously short for production KMS keys protecting PHI and compliance evidence. AWS recommends a minimum of 30 days to prevent accidental data loss and allow sufficient time for key recovery.
🔐 Proposed fix to increase deletion window
resource "aws_kms_key" "phi" {
description = "CMK for Acme Health PHI data - S3 and DynamoDB"
- deletion_window_in_days = 7
+ deletion_window_in_days = 30
enable_key_rotation = true resource "aws_kms_key" "evidence" {
description = "CMK for evidence vault encryption"
- deletion_window_in_days = 7
+ deletion_window_in_days = 30
enable_key_rotation = trueAlso applies to: 24-24
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@terraform/kms.tf` at line 7, The KMS key resource's deletion_window_in_days
is set to 7 which is too short for production; update the aws_kms_key
resource(s) that contain the deletion_window_in_days attribute (and any matching
occurrences like the one at lines referenced as 24-24) to a minimum of 30 days
(or a configurable variable default >=30) so production keys protecting PHI meet
compliance and allow recovery time; ensure you update any variable defaults and
documentation/comments that reference the old 7-day window.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
New Features
Tests
Chores