Deny audit-trail tampering (CloudTrail, Config, GuardDuty, SecurityHub, S3, KMS)#1
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e18b70de79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "guardduty:DisassociateFromMasterAccount", "guardduty:StopMonitoringMembers", | ||
| "guardduty:DeletePublishingDestination", "guardduty:UpdatePublishingDestination", | ||
| "guardduty:DisassociateMembers", "guardduty:DeleteMembers", | ||
| "guardduty:UpdateMemberDetectors", |
There was a problem hiding this comment.
DenyAuditServiceTampering blocks only the deprecated *DisassociateFromMasterAccount actions, but not guardduty:DisassociateFromAdministratorAccount / securityhub:DisassociateFromAdministratorAccount. With PowerUserAccess still attached, the agent can use the newer APIs to detach from delegated administrators and bypass the intended centralized GuardDuty/Security Hub audit controls.
Useful? React with 👍 / 👎.
| "config:DeleteConfigurationRecorder", "config:StopConfigurationRecorder", | ||
| "config:PutConfigurationRecorder", | ||
| "config:DeleteDeliveryChannel", "config:PutDeliveryChannel", | ||
| "config:DeleteConfigRule", |
There was a problem hiding this comment.
The Config deny list blocks only customer-managed recorder mutations (DeleteConfigurationRecorder, PutConfigurationRecorder), but omits service-linked recorder APIs like config:DeleteServiceLinkedConfigurationRecorder and config:PutServiceLinkedConfigurationRecorder. In accounts that use service-linked recorders, the agent can still disable or replace recording despite this guardrail.
Useful? React with 👍 / 👎.
royosherove
force-pushed
the
fix/deny-audit-tampering
branch
2 times, most recently
from
cd4fc29 to
4d05d4d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d05d4d713
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "kms:PutResourcePolicy", "kms:DeleteResourcePolicy", | ||
| "kms:ImportKeyMaterial", "kms:DeleteImportedKeyMaterial" |
There was a problem hiding this comment.
DenyTrailKmsTampering includes kms:PutResourcePolicy and kms:DeleteResourcePolicy, but AWS KMS does not define those IAM actions (KMS policy management is kms:PutKeyPolicy). When trail_kms_key_arn is set, this makes the inline IAM policy invalid and aws_iam_role_policy.deny_guardrails creation/update fails with a malformed/invalid action error, so the guardrail cannot be deployed at all.
Useful? React with 👍 / 👎.
…b, S3, KMS)
PowerUserAccess permits the agent to blind the audit trail through
multiple paths. Adds explicit denies covering all of them.
policies/deny-guardrails.json — 4 new statements:
DenyCloudTrailTampering
StopLogging, DeleteTrail, UpdateTrail, PutEventSelectors,
PutInsightSelectors, Delete/UpdateEventDataStore.
DenyAuditServiceTampering
Config: Delete/Stop/Put ConfigurationRecorder, Delete/Put
DeliveryChannel (Put covers overwrite-as-delete: pointing the
recorder at a black-hole bucket), DeleteConfigRule.
GuardDuty: DeleteDetector, UpdateDetector (covers flipping a
detector to disabled / low-frequency without deleting it),
DisassociateFromMasterAccount, StopMonitoringMembers.
SecurityHub: DisableSecurityHub, DisassociateFromMasterAccount,
BatchDisableStandards, UpdateStandardsControl (covers
control-level disablement that bypasses DisableSecurityHub).
DenyTrailStorageTampering (scoped to TRAIL_BUCKET_NAME)
s3:DeleteBucket, Delete/PutBucketPolicy, PutBucketAcl,
PutBucketVersioning, PutLifecycleConfiguration, DeleteObject,
DeleteObjectVersion, PutObjectRetention, BypassGovernanceRetention.
Scoped to the trail bucket ARN so the agent can still operate on
other buckets normally.
DenyTrailKmsTampering (scoped to TRAIL_KMS_KEY_ARN)
kms:ScheduleKeyDeletion, DisableKey, PutKeyPolicy, CreateGrant,
RevokeGrant, CancelKeyDeletion. Scoped to the trail's CMK so the
agent can still manage other keys.
README.md
Adds TRAIL_BUCKET_NAME and TRAIL_KMS_KEY_ARN to the placeholder
table, with guidance for the unencrypted-trail case.
docs/policy-design.md
Expands security-property #4 from a one-liner to an itemized list
covering all four threat surfaces; line-wraps for diff/readability.
royosherove
force-pushed
the
fix/deny-audit-tampering
branch
from
4d05d4d to
63d9f88
Compare
1 participant
Closes the audit-trail tampering gap: under PowerUserAccess the agent could call
cloudtrail:StopLogging,DeleteTrail, etc., blinding the very audit mechanism the design relies on.What changed
Policy expansion — 11 deny statements, 132 deny actions:
DenyCloudTrailTampering(14) — Stop/Delete/Update trails, event-data-stores, channels, selectors, resource policies; alsoCreate*to block "stand up a competing trail" evasionDenyAuditServiceTampering(30) — Config/GuardDuty/SecurityHub recorders, members, filters, finding triage. Documented as intentional broad deny (triage = human/SOC task)DenyTrailStorageTampering(21) — trail S3 bucket: delete, policy/ACL/PAB/ownership, encryption, versioning, logging, lifecycle, replication, object-lock, notification redirect, website, object retention/legal-hold, governance bypass, PutObject (corrupt existing logs)DenyTrailKmsTampering(12) — trail CMK: ScheduleKeyDeletion, DisableKey, PutKeyPolicy, grants, alias retargeting, PutResourcePolicy, key-material importDenyRoleManagementOutsideAgentPathextended (+4) —iam:TagRole/UntagRole/UpdateRole/UpdateRoleDescriptionfor defense-in-depth parity withDenySelfEscalationDenySelfEscalationextended (+6) — closes the self-mutation hole opened by addingpath = var.iam_pathto the agent roleTerraform module mirrors all 4 new statements via
concat()over named locals; conditional inclusion gated ontrail_bucket_name/trail_kms_key_arnvars.Quality gates
trail_protection_acknowledged != true, plan/apply errors with a clear message/root, double-slash); strict KMS ARN format with multi-region key support.github/workflows/lint.yml) — JSON parse, substitution round-trip lint,terraform fmt/validate, per-Sid Action/NotAction parity check across all 3 policies (deny-guardrails, iam-scoped, permissions-boundary). Caught real Sid drift during development (AllowInstanceProfileManagementvsAllowInstanceProfileManagementUnderAgentPath).Repo cleanup
terraform/example.tf→terraform/examples/downstream-consumer.tf(was causingDuplicate variable declarationand breakingterraform validate).gitignore(was missing entirely)policy-design.mdwith Sid-summary tables linking to canonical sourcesBreaking changes
aws_iam_role.agentandaws_iam_instance_profile.agentnow setpath = var.iam_path(default/loki/). Existing deployments will recreate role + profile on apply, changing the role ARN. New deployments are unaffected. Required so the JSON template'srole/IAM_PATHAGENT_ROLE_NAMEplaceholder matches the actual role ARN.Partition support
Hardcoded to
aws(commercial regions). GovCloud / China not supported. TODO marker interraform/main.tfdocuments the 3 lock-in sites if/when partition support is added.Review trail
8 automated review loops + 2 architect reviews + 7 amendments. Final state: zero findings on both reviewers' last pass.