From a7a591ff62ea653084291719e346b89059888da3 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Tue, 19 May 2026 21:03:37 +0400 Subject: [PATCH 1/2] feat(cloudtrail-alerts): per-detector exclusions + 8 new detectors + SSO MFA fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Why: production deployment exposed alert-fatigue patterns — ~16 fires/day with ~89% false positives from governed automation (Pulumi CI bot, Prowler scanner, AWS Identity Center sessions, AWS service-linked roles). Built-in detectors matched the CIS CloudWatch.1-14 set verbatim, leaving no knob to tune. Plugin extensions: - `CloudTrailAlertOverride` schema with optional per-detector exclusions (userName / principalId / arn / arn-glob / userType / invokedBy) baked into the CloudWatch metric filter pattern at provision time. Suppression happens at the metric layer rather than in the Lambda enrichment step so excluded events never increment the metric, the alarm never trips, and the CW alarm dashboard reflects real signal (preserves SOC2 CC6.1 / ISO 27001 audit clarity). Threshold / period / evaluationPeriods overrides too. - `consoleLoginWithoutMfa` now scoped to `userIdentity.type = "IAMUser"`. AWS Identity Center / federated console sessions always emit `MFAUsed = "No"` because MFA happens upstream at the IdP — without the scope, every SSO console session triggered a CloudWatch.3 false positive. Identity Center coverage belongs in a separate detector against `signin.amazonaws.com / UserAuthentication` (not added here). - 8 beyond-CIS detectors covering attacker-blinding moves and exposure paths that CIS does not address: GuardDuty disable, Security Hub disable, IAM access key creation, S3 Block Public Access toggle, Lambda Function URL with AuthType=NONE, KMS key policy / grant changes, AWS Organizations SCP churn, anonymous external probes (`userIdentity.type=AWSAccount` AccessDenied with default threshold=10 to require sustained reconnaissance, not page on one-off probes). Backwards compat: plain-`bool` selector form is unchanged. Existing consumers who declare `iamPolicyChanges: true` see identical filter patterns and alarm parameters. New struct fields and `overrides` map are opt-in. Tests: 14 new test cases covering exclusion clause generation, deterministic ordering across YAML re-orderings, threshold / period override application, empty-string skipping, and SSO MFA scope. Existing tests updated for totalDetectors = 22 (14 CIS + 8 additions). Signed-off-by: Dmitrii Creed --- pkg/clouds/aws/cloudtrail_security_alerts.go | 62 ++++- .../pulumi/aws/cloudtrail_security_alerts.go | 222 +++++++++++++++++- .../aws/cloudtrail_security_alerts_test.go | 180 ++++++++++++-- 3 files changed, 434 insertions(+), 30 deletions(-) diff --git a/pkg/clouds/aws/cloudtrail_security_alerts.go b/pkg/clouds/aws/cloudtrail_security_alerts.go index d286d44e..1b64c6db 100644 --- a/pkg/clouds/aws/cloudtrail_security_alerts.go +++ b/pkg/clouds/aws/cloudtrail_security_alerts.go @@ -52,8 +52,13 @@ func (c CloudTrailSecurityAlertsConfig) RequiresTrailValidation() bool { // CloudTrailAlertSelectors controls which security alerts are enabled. // Each field maps to a CloudWatch metric filter + alarm on the CloudTrail log group. -// Alert names reference AWS Security Hub/CIS CloudWatch controls (CloudWatch.1-14). +// Built-in detectors track AWS Security Hub/CIS CloudWatch controls (CloudWatch.1-14) +// plus a set of high-value additions covering attacker-blinding moves and exposure paths +// that CIS does not cover (GuardDuty/SecurityHub disable, IAM access keys, S3 Block Public +// Access, public Lambda Function URLs, KMS key policy edits, AWS Organizations changes, +// anonymous external probes). type CloudTrailAlertSelectors struct { + // CIS CloudWatch.1-14 RootAccountUsage bool `json:"rootAccountUsage,omitempty" yaml:"rootAccountUsage,omitempty"` // CIS CloudWatch.1 UnauthorizedApiCalls bool `json:"unauthorizedApiCalls,omitempty" yaml:"unauthorizedApiCalls,omitempty"` // CIS CloudWatch.2 ConsoleLoginWithoutMfa bool `json:"consoleLoginWithoutMfa,omitempty" yaml:"consoleLoginWithoutMfa,omitempty"` // CIS CloudWatch.3 @@ -68,6 +73,61 @@ type CloudTrailAlertSelectors struct { NetworkGatewayChanges bool `json:"networkGatewayChanges,omitempty" yaml:"networkGatewayChanges,omitempty"` // CIS CloudWatch.12 RouteTableChanges bool `json:"routeTableChanges,omitempty" yaml:"routeTableChanges,omitempty"` // CIS CloudWatch.13 VpcChanges bool `json:"vpcChanges,omitempty" yaml:"vpcChanges,omitempty"` // CIS CloudWatch.14 + + // Beyond-CIS detectors. Default off so existing deployments don't gain new alerts on plugin upgrade. + GuardDutyDisabled bool `json:"guardDutyDisabled,omitempty" yaml:"guardDutyDisabled,omitempty"` // GuardDuty disabled/detector deleted + SecurityHubDisabled bool `json:"securityHubDisabled,omitempty" yaml:"securityHubDisabled,omitempty"` // Security Hub disabled or standards turned off + AccessKeyCreation bool `json:"accessKeyCreation,omitempty" yaml:"accessKeyCreation,omitempty"` // CreateAccessKey on any IAM user + S3PublicAccessChanges bool `json:"s3PublicAccessChanges,omitempty" yaml:"s3PublicAccessChanges,omitempty"` // Block Public Access toggled at account or bucket scope + LambdaUrlPublic bool `json:"lambdaUrlPublic,omitempty" yaml:"lambdaUrlPublic,omitempty"` // Lambda Function URL created/updated with AuthType=NONE + KmsKeyPolicyChanges bool `json:"kmsKeyPolicyChanges,omitempty" yaml:"kmsKeyPolicyChanges,omitempty"` // PutKeyPolicy / PutResourcePolicy on KMS + OrganizationsChanges bool `json:"organizationsChanges,omitempty" yaml:"organizationsChanges,omitempty"` // SCP / account-membership churn in AWS Organizations + AnonymousProbes bool `json:"anonymousProbes,omitempty" yaml:"anonymousProbes,omitempty"` // userIdentity.type=AWSAccount AccessDenied probes from public IPs + + // Overrides allows per-detector tuning without forking the plugin. Keyed by detector + // selector name (e.g. "iamPolicyChanges"). An override can: + // - bake exclusion clauses into the CloudWatch metric filter pattern (preferred + // for governed automation that contributes nothing but noise — Pulumi CI bots, + // known scanners, AWS service-linked roles); + // - raise the alarm threshold so a single matched event no longer trips the alarm + // (CIS Benchmark guidance for unauthorized-api-calls); + // - adjust the evaluation window. + // + // Suppression happens at the metric-filter layer, not in the Lambda enrichment + // step — excluded events never increment the metric, the alarm never trips, and + // no Slack notification is sent. The CW alarm dashboard therefore reflects real + // signal rather than known-good noise (preserves SOC2/ISO audit clarity). + Overrides map[string]CloudTrailAlertOverride `json:"overrides,omitempty" yaml:"overrides,omitempty"` +} + +// CloudTrailAlertOverride tunes a single detector. All fields are optional; zero values +// mean "use plugin default." +type CloudTrailAlertOverride struct { + // Threshold raises the alarm threshold (events per period). 0 = use the plugin default + // for this detector (1 for most, 5 for unauthorizedApiCalls, 10 for anonymousProbes). + Threshold float64 `json:"threshold,omitempty" yaml:"threshold,omitempty"` + // Period in seconds. 0 = 300 (5 min). CloudWatch supports 60, 300, 3600. + Period int `json:"period,omitempty" yaml:"period,omitempty"` + // EvaluationPeriods is how many consecutive periods must breach. 0 = 1. + EvaluationPeriods int `json:"evaluationPeriods,omitempty" yaml:"evaluationPeriods,omitempty"` + + // ExcludeUserNames excludes events where $.userIdentity.userName matches the given + // names exactly. Useful for IAMUser-type principals like CI bot accounts. + ExcludeUserNames []string `json:"excludeUserNames,omitempty" yaml:"excludeUserNames,omitempty"` + // ExcludePrincipalIds excludes by $.userIdentity.principalId (AIDA..., AROA..., AKIA...). + ExcludePrincipalIds []string `json:"excludePrincipalIds,omitempty" yaml:"excludePrincipalIds,omitempty"` + // ExcludeUserArns excludes by exact $.userIdentity.arn match. + ExcludeUserArns []string `json:"excludeUserArns,omitempty" yaml:"excludeUserArns,omitempty"` + // ExcludeUserArnGlobs excludes by $.userIdentity.arn glob (CloudWatch metric filter + // patterns support * within string values, e.g. + // "arn:aws:sts::*:assumed-role/AWSServiceRoleFor*/*"). One glob per list entry. + ExcludeUserArnGlobs []string `json:"excludeUserArnGlobs,omitempty" yaml:"excludeUserArnGlobs,omitempty"` + // ExcludeUserTypes excludes by $.userIdentity.type (e.g. "AWSService", "AWSAccount", + // "AssumedRole"). Useful for stripping AWS internal plumbing from unauthorized-api-calls. + ExcludeUserTypes []string `json:"excludeUserTypes,omitempty" yaml:"excludeUserTypes,omitempty"` + // ExcludeInvokedBy excludes by $.userIdentity.invokedBy (e.g. "s3.amazonaws.com"). + // This is the canonical way to strip AWS service self-probes. + ExcludeInvokedBy []string `json:"excludeInvokedBy,omitempty" yaml:"excludeInvokedBy,omitempty"` } func ReadCloudTrailSecurityAlertsConfig(config *api.Config) (api.Config, error) { diff --git a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go index ca9f49c9..ec51401a 100644 --- a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go +++ b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go @@ -3,6 +3,7 @@ package aws import ( "fmt" "sort" + "strings" "github.com/pkg/errors" @@ -19,11 +20,14 @@ import ( ) // securityAlertDef defines a CloudTrail security alert with its metric filter pattern. +// Defaults (zero values) are documented per field. type securityAlertDef struct { - name string - description string - filterPattern string - threshold float64 // default 1 if 0 + name string + description string + filterPattern string + threshold float64 // default 1 when zero + period int // alarm period seconds; default 300 when zero + evaluationPeriods int // default 1 when zero } // securityAlerts maps CloudTrailAlertSelectors field names to their definitions. @@ -45,11 +49,19 @@ var securityAlerts = map[string]securityAlertDef{ `($.errorCode = "Client.UnauthorizedOperation") || ($.errorCode = "UnauthorizedOperation") }`, threshold: 5, }, - // CloudWatch.3 — Console login without MFA (successful only) + // CloudWatch.3 — Console login without MFA (successful only). + // + // Scoped to userIdentity.type = "IAMUser". AWS Identity Center / federated logins + // always emit ConsoleLogin events with additionalEventData.MFAUsed = "No" because + // MFA is enforced upstream at the IdP rather than at the AWS console step. Without + // this scope the detector pages every SSO console session — a well-documented + // CIS CloudWatch.3 false positive. Identity Center sessions should be audited via + // signin.amazonaws.com UserAuthentication events (not currently provisioned by + // this plugin; surface via a separate detector if needed). "consoleLoginWithoutMfa": { name: "ct-console-login-no-mfa", - description: "Successful console login without MFA detected", - filterPattern: `{ ($.eventName = "ConsoleLogin") && ($.additionalEventData.MFAUsed != "Yes") && ($.responseElements.ConsoleLogin = "Success") }`, + description: "Successful IAM user console login without MFA detected", + filterPattern: `{ ($.eventName = "ConsoleLogin") && ($.additionalEventData.MFAUsed != "Yes") && ($.responseElements.ConsoleLogin = "Success") && ($.userIdentity.type = "IAMUser") }`, }, // CloudWatch.4 — IAM policy changes // SetDefaultPolicyVersion is included: flipping a managed policy's default version @@ -151,9 +163,102 @@ var securityAlerts = map[string]securityAlertDef{ `($.eventName = "DetachClassicLinkVpc") || ($.eventName = "DisableVpcClassicLink") || ` + `($.eventName = "EnableVpcClassicLink") }`, }, + + // Beyond-CIS — GuardDuty disabled/blinded. + // Why: CIS CloudWatch.9 (configChanges) tracks AWS Config recorder ops but not the + // other detective controls. Disabling GuardDuty is a classic attacker-blinding move + // because findings stop flowing and historical findings can be deleted. + "guardDutyDisabled": { + name: "ct-guardduty-disabled", + description: "GuardDuty detector disabled, deleted, or members removed — security visibility lost", + filterPattern: `{ ($.eventSource = "guardduty.amazonaws.com") && (($.eventName = "DeleteDetector") || ` + + `($.eventName = "UpdateDetector") || ($.eventName = "DisassociateMembers") || ` + + `($.eventName = "DeleteMembers") || ($.eventName = "StopMonitoringMembers")) }`, + }, + + // Beyond-CIS — Security Hub disabled or standards turned off. + // Why: same attacker-blinding category as GuardDuty. + "securityHubDisabled": { + name: "ct-securityhub-disabled", + description: "Security Hub disabled or standards/import subscriptions turned off — security visibility lost", + filterPattern: `{ ($.eventSource = "securityhub.amazonaws.com") && (($.eventName = "DisableSecurityHub") || ` + + `($.eventName = "BatchDisableStandards") || ($.eventName = "DisableImportFindingsForProduct") || ` + + `($.eventName = "DeleteActionTarget") || ($.eventName = "DeleteInsight")) }`, + }, + + // Beyond-CIS — IAM access key creation. + // Why: persistent credentials are higher-risk than short-lived STS tokens. Creation + // without a documented rotation/expiry path is a common audit finding. + "accessKeyCreation": { + name: "ct-access-key-creation", + description: "IAM access key created — verify rotation policy and owner attestation", + filterPattern: `{ ($.eventSource = "iam.amazonaws.com") && ($.eventName = "CreateAccessKey") }`, + }, + + // Beyond-CIS — S3 Block Public Access (BPA) toggled at account or bucket scope. + // Why: CIS CloudWatch.8 only catches bucket policy edits; BPA is the higher-leverage + // gate. Turning it off is a single click that exposes previously private buckets. + "s3PublicAccessChanges": { + name: "ct-s3-public-access-changes", + description: "S3 Block Public Access settings modified — possible exposure path opened", + filterPattern: `{ ($.eventSource = "s3.amazonaws.com") && (($.eventName = "PutAccountPublicAccessBlock") || ` + + `($.eventName = "PutBucketPublicAccessBlock") || ($.eventName = "DeleteAccountPublicAccessBlock") || ` + + `($.eventName = "DeleteBucketPublicAccessBlock")) }`, + }, + + // Beyond-CIS — Lambda Function URL created or updated with AuthType=NONE. + // Why: a Function URL with AuthType=NONE is a public HTTPS endpoint with the function's + // IAM role behind it; one click of misconfiguration exposes whatever the function can do. + // We observed anonymous Azure-IP scanners probing GetFunctionUrlConfig in the wild — they + // will hit a real endpoint the moment one exists. + "lambdaUrlPublic": { + name: "ct-lambda-url-public", + description: "Lambda Function URL created or updated with AuthType=NONE — public endpoint exposed", + filterPattern: `{ ($.eventSource = "lambda.amazonaws.com") && (($.eventName = "CreateFunctionUrlConfig") || ($.eventName = "UpdateFunctionUrlConfig")) && ($.requestParameters.authType = "NONE") }`, + }, + + // Beyond-CIS — KMS key policy / grant changes. + // Why: CIS CloudWatch.7 catches key deletion but not policy edits. PutKeyPolicy / CreateGrant + // can quietly hand decrypt rights to a new principal without touching the key's lifecycle. + "kmsKeyPolicyChanges": { + name: "ct-kms-key-policy-changes", + description: "KMS key policy modified or grant created — verify principal scope and conditions", + filterPattern: `{ ($.eventSource = "kms.amazonaws.com") && (($.eventName = "PutKeyPolicy") || ` + + `($.eventName = "PutResourcePolicy") || ($.eventName = "CreateGrant") || ` + + `($.eventName = "RetireGrant") || ($.eventName = "RevokeGrant")) }`, + }, + + // Beyond-CIS — AWS Organizations / SCP changes. + // Why: SCPs are the strongest preventative control in a multi-account org. Detaching + // or deleting one widens blast radius across every account it covered. + "organizationsChanges": { + name: "ct-organizations-changes", + description: "AWS Organizations policy modified — verify SCP boundaries", + filterPattern: `{ ($.eventSource = "organizations.amazonaws.com") && (($.eventName = "CreatePolicy") || ` + + `($.eventName = "DeletePolicy") || ($.eventName = "UpdatePolicy") || ($.eventName = "AttachPolicy") || ` + + `($.eventName = "DetachPolicy") || ($.eventName = "EnablePolicyType") || ($.eventName = "DisablePolicyType") || ` + + `($.eventName = "LeaveOrganization") || ($.eventName = "RemoveAccountFromOrganization")) }`, + }, + + // Beyond-CIS — Anonymous external probes (recon). + // Why: userIdentity.type=AWSAccount represents another AWS account (or unauthenticated + // AWS API client) hitting our resources. We observed ~400/14d hits in the wild scanning + // for exposed Lambda Function URLs. Default threshold=10 so individual probes don't + // page — we only care about sustained scanning from one source. + "anonymousProbes": { + name: "ct-anonymous-probes", + description: "Anonymous external account probing AWS resources — possible reconnaissance", + filterPattern: `{ ($.userIdentity.type = "AWSAccount") && (($.errorCode = "AccessDenied") || ` + + `($.errorCode = "AccessDeniedException") || ($.errorCode = "UnauthorizedAccess") || ` + + `($.errorCode = "Client.UnauthorizedAccess") || ($.errorCode = "Client.UnauthorizedOperation") || ` + + `($.errorCode = "UnauthorizedOperation")) }`, + threshold: 10, + }, } // enabledAlerts returns the alert definitions that are enabled in the selector config. +// Per-detector overrides (exclusions, threshold, period) are baked into the returned +// definitions here so downstream provisioning code sees a single resolved struct per alert. // Results are sorted by name for deterministic Pulumi resource ordering. func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef { var result []securityAlertDef @@ -161,6 +266,7 @@ func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef key string enabled bool }{ + // CIS CloudWatch.1-14 {"cloudTrailTampering", selectors.CloudTrailTampering}, {"configChanges", selectors.ConfigChanges}, {"consoleLoginWithoutMfa", selectors.ConsoleLoginWithoutMfa}, @@ -175,18 +281,100 @@ func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef {"securityGroupChanges", selectors.SecurityGroupChanges}, {"unauthorizedApiCalls", selectors.UnauthorizedApiCalls}, {"vpcChanges", selectors.VpcChanges}, + // Beyond-CIS + {"guardDutyDisabled", selectors.GuardDutyDisabled}, + {"securityHubDisabled", selectors.SecurityHubDisabled}, + {"accessKeyCreation", selectors.AccessKeyCreation}, + {"s3PublicAccessChanges", selectors.S3PublicAccessChanges}, + {"lambdaUrlPublic", selectors.LambdaUrlPublic}, + {"kmsKeyPolicyChanges", selectors.KmsKeyPolicyChanges}, + {"organizationsChanges", selectors.OrganizationsChanges}, + {"anonymousProbes", selectors.AnonymousProbes}, } for _, c := range checks { - if c.enabled { - if def, ok := securityAlerts[c.key]; ok { - result = append(result, def) - } + if !c.enabled { + continue + } + def, ok := securityAlerts[c.key] + if !ok { + continue } + if ov, hasOv := selectors.Overrides[c.key]; hasOv { + def = applyOverride(def, ov) + } + result = append(result, def) } sort.Slice(result, func(i, j int) bool { return result[i].name < result[j].name }) return result } +// applyOverride returns a copy of def with the override applied: +// - exclusion clauses are appended to the filter pattern as `&& (field != "val")`; +// - threshold/period/evaluationPeriods overrides replace the defaults when non-zero. +// +// The original filter pattern is wrapped in parentheses so the appended NOT-clauses +// AND with the entire base predicate, not just its last OR-term. Example: +// +// base: { ($.eventName = "PutRolePolicy") || ($.eventName = "AttachRolePolicy") } +// out: { ( ($.eventName = "PutRolePolicy") || ($.eventName = "AttachRolePolicy") ) && ($.userIdentity.userName != "integrail-deployer-bot") } +// +// CloudWatch metric filter pattern syntax reference: +// +// https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/FilterAndPatternSyntax.html +func applyOverride(def securityAlertDef, ov awsApi.CloudTrailAlertOverride) securityAlertDef { + if ov.Threshold > 0 { + def.threshold = ov.Threshold + } + if ov.Period > 0 { + def.period = ov.Period + } + if ov.EvaluationPeriods > 0 { + def.evaluationPeriods = ov.EvaluationPeriods + } + + clauses := buildExclusionClauses(ov) + if len(clauses) == 0 { + return def + } + + // Strip the outer braces from the base pattern so we can wrap the predicate + // in parens before AND'ing the exclusions. CloudWatch JSON filter patterns + // are always braced. + base := strings.TrimSpace(def.filterPattern) + base = strings.TrimPrefix(base, "{") + base = strings.TrimSuffix(base, "}") + base = strings.TrimSpace(base) + + def.filterPattern = "{ (" + base + ") && " + strings.Join(clauses, " && ") + " }" + return def +} + +// buildExclusionClauses turns an override into a list of `(field != "val")` clauses +// in deterministic order. The list is empty when the override has no exclusions. +func buildExclusionClauses(ov awsApi.CloudTrailAlertOverride) []string { + var clauses []string + add := func(field string, values []string) { + // Sort to keep the generated filter pattern deterministic across deploys + // (so Pulumi sees a stable resource and doesn't churn the filter on every + // run when the user reorders the YAML list). + vs := append([]string(nil), values...) + sort.Strings(vs) + for _, v := range vs { + if v == "" { + continue + } + clauses = append(clauses, fmt.Sprintf(`($.%s != %q)`, field, v)) + } + } + add("userIdentity.userName", ov.ExcludeUserNames) + add("userIdentity.principalId", ov.ExcludePrincipalIds) + add("userIdentity.arn", ov.ExcludeUserArns) + add("userIdentity.arn", ov.ExcludeUserArnGlobs) // CloudWatch supports * within the quoted value + add("userIdentity.type", ov.ExcludeUserTypes) + add("userIdentity.invokedBy", ov.ExcludeInvokedBy) + return clauses +} + // CloudTrailSecurityAlerts provisions CloudWatch metric filters and alarms // for security-relevant CloudTrail events. Each enabled alert creates: // - A LogMetricFilter on the CloudTrail log group @@ -348,6 +536,14 @@ func CloudTrailSecurityAlerts(ctx *sdk.Context, stack api.Stack, input api.Resou if threshold == 0 { threshold = 1 } + period := alertDef.period + if period == 0 { + period = 300 + } + evalPeriods := alertDef.evaluationPeriods + if evalPeriods == 0 { + evalPeriods = 1 + } // createAlert suffixes cfg.name with "-execution-role" (15 chars) and Pulumi adds // an 8-char random suffix when it auto-names the IAM role. Cap the base so the // resulting physical role name stays within AWS's 64-char limit (with headroom). @@ -363,8 +559,8 @@ func CloudTrailSecurityAlerts(ctx *sdk.Context, stack api.Stack, input api.Resou MetricName: sdk.String(metricName), Namespace: sdk.String(metricNamespace), Statistic: sdk.String("Sum"), - Period: sdk.Int(300), - EvaluationPeriods: sdk.Int(1), + Period: sdk.Int(period), + EvaluationPeriods: sdk.Int(evalPeriods), Threshold: sdk.Float64(threshold), ComparisonOperator: sdk.String("GreaterThanOrEqualToThreshold"), TreatMissingData: sdk.String("notBreaching"), diff --git a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go index fdf2fedc..053d7f5e 100644 --- a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go +++ b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go @@ -1,6 +1,7 @@ package aws import ( + "strings" "testing" . "github.com/onsi/gomega" @@ -8,10 +9,15 @@ import ( awsApi "github.com/simple-container-com/api/pkg/clouds/aws" ) +// totalDetectors is the count of built-in security detectors. Update when adding new ones. +// Composition: 14 CIS CloudWatch.1-14 + 8 beyond-CIS additions. +const totalDetectors = 22 + func TestEnabledAlerts_AllEnabled(t *testing.T) { RegisterTestingT(t) selectors := awsApi.CloudTrailAlertSelectors{ + // CIS RootAccountUsage: true, UnauthorizedApiCalls: true, ConsoleLoginWithoutMfa: true, @@ -26,10 +32,19 @@ func TestEnabledAlerts_AllEnabled(t *testing.T) { NetworkGatewayChanges: true, RouteTableChanges: true, VpcChanges: true, + // Beyond-CIS + GuardDutyDisabled: true, + SecurityHubDisabled: true, + AccessKeyCreation: true, + S3PublicAccessChanges: true, + LambdaUrlPublic: true, + KmsKeyPolicyChanges: true, + OrganizationsChanges: true, + AnonymousProbes: true, } alerts := enabledAlerts(selectors) - Expect(alerts).To(HaveLen(14)) + Expect(alerts).To(HaveLen(totalDetectors)) names := make(map[string]bool) for _, a := range alerts { @@ -38,20 +53,48 @@ func TestEnabledAlerts_AllEnabled(t *testing.T) { Expect(a.filterPattern).ToNot(BeEmpty()) } - Expect(names).To(HaveKey("ct-root-account-usage")) - Expect(names).To(HaveKey("ct-unauthorized-api-calls")) - Expect(names).To(HaveKey("ct-console-login-no-mfa")) - Expect(names).To(HaveKey("ct-iam-policy-changes")) - Expect(names).To(HaveKey("ct-cloudtrail-tampering")) - Expect(names).To(HaveKey("ct-failed-console-logins")) - Expect(names).To(HaveKey("ct-kms-key-deletion")) - Expect(names).To(HaveKey("ct-s3-bucket-policy-changes")) - Expect(names).To(HaveKey("ct-config-changes")) - Expect(names).To(HaveKey("ct-security-group-changes")) - Expect(names).To(HaveKey("ct-nacl-changes")) - Expect(names).To(HaveKey("ct-network-gateway-changes")) - Expect(names).To(HaveKey("ct-route-table-changes")) - Expect(names).To(HaveKey("ct-vpc-changes")) + for _, expected := range []string{ + "ct-root-account-usage", + "ct-unauthorized-api-calls", + "ct-console-login-no-mfa", + "ct-iam-policy-changes", + "ct-cloudtrail-tampering", + "ct-failed-console-logins", + "ct-kms-key-deletion", + "ct-s3-bucket-policy-changes", + "ct-config-changes", + "ct-security-group-changes", + "ct-nacl-changes", + "ct-network-gateway-changes", + "ct-route-table-changes", + "ct-vpc-changes", + "ct-guardduty-disabled", + "ct-securityhub-disabled", + "ct-access-key-creation", + "ct-s3-public-access-changes", + "ct-lambda-url-public", + "ct-kms-key-policy-changes", + "ct-organizations-changes", + "ct-anonymous-probes", + } { + Expect(names).To(HaveKey(expected)) + } +} + +func TestConsoleLoginWithoutMfa_RestrictedToIAMUser(t *testing.T) { + // The CIS CloudWatch.3 pattern fires on AWS Identity Center sessions by default + // because MFAUsed=No (MFA happens upstream). Constraining to IAMUser type avoids + // this well-known false positive. + RegisterTestingT(t) + Expect(securityAlerts["consoleLoginWithoutMfa"].filterPattern).To( + ContainSubstring(`$.userIdentity.type = "IAMUser"`)) +} + +func TestAnonymousProbes_DefaultThreshold(t *testing.T) { + // Single anonymous probe is not actionable; default threshold 10 in 5min reflects + // "sustained reconnaissance" rather than one-off enumeration. + RegisterTestingT(t) + Expect(securityAlerts["anonymousProbes"].threshold).To(Equal(float64(10))) } func TestEnabledAlerts_PartialEnabled(t *testing.T) { @@ -101,7 +144,7 @@ func TestEnabledAlerts_Deterministic(t *testing.T) { func TestSecurityAlertDefinitions(t *testing.T) { RegisterTestingT(t) - Expect(securityAlerts).To(HaveLen(14)) + Expect(securityAlerts).To(HaveLen(totalDetectors)) for key, def := range securityAlerts { t.Run(key, func(t *testing.T) { @@ -114,6 +157,111 @@ func TestSecurityAlertDefinitions(t *testing.T) { } } +func TestApplyOverride_Exclusions(t *testing.T) { + RegisterTestingT(t) + + base := securityAlerts["iamPolicyChanges"] + out := applyOverride(base, awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"integrail-deployer-bot"}, + }) + + // Base pattern is preserved (wrapped in parens), exclusion is AND'd on + Expect(out.filterPattern).To(HavePrefix(`{ (`)) + Expect(out.filterPattern).To(HaveSuffix(` }`)) + Expect(out.filterPattern).To(ContainSubstring(`PutRolePolicy`)) // base predicate intact + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.userName != "integrail-deployer-bot")`)) +} + +func TestApplyOverride_MultipleExclusionFields(t *testing.T) { + RegisterTestingT(t) + + out := applyOverride(securityAlerts["unauthorizedApiCalls"], awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"prowler-readonly"}, + ExcludeUserTypes: []string{"AWSService", "AWSAccount"}, + ExcludeInvokedBy: []string{"s3.amazonaws.com", "lambda.amazonaws.com"}, + ExcludeUserArnGlobs: []string{ + "arn:aws:sts::*:assumed-role/AWSServiceRoleFor*/*", + }, + }) + + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.userName != "prowler-readonly")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.type != "AWSService")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.type != "AWSAccount")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.invokedBy != "s3.amazonaws.com")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.invokedBy != "lambda.amazonaws.com")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.arn != "arn:aws:sts::*:assumed-role/AWSServiceRoleFor*/*")`)) +} + +func TestApplyOverride_Deterministic(t *testing.T) { + // Re-ordered input lists must produce the same filter pattern so Pulumi sees no diff. + RegisterTestingT(t) + + ovA := awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"alpha", "beta", "gamma"}, + ExcludeUserTypes: []string{"AWSService", "AWSAccount"}, + } + ovB := awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"gamma", "alpha", "beta"}, + ExcludeUserTypes: []string{"AWSAccount", "AWSService"}, + } + + a := applyOverride(securityAlerts["iamPolicyChanges"], ovA).filterPattern + b := applyOverride(securityAlerts["iamPolicyChanges"], ovB).filterPattern + Expect(a).To(Equal(b)) +} + +func TestApplyOverride_ThresholdAndPeriod(t *testing.T) { + RegisterTestingT(t) + + out := applyOverride(securityAlerts["unauthorizedApiCalls"], awsApi.CloudTrailAlertOverride{ + Threshold: 10, + Period: 600, + EvaluationPeriods: 2, + }) + Expect(out.threshold).To(Equal(float64(10))) + Expect(out.period).To(Equal(600)) + Expect(out.evaluationPeriods).To(Equal(2)) +} + +func TestApplyOverride_EmptyOverrideIsNoop(t *testing.T) { + RegisterTestingT(t) + + base := securityAlerts["iamPolicyChanges"] + out := applyOverride(base, awsApi.CloudTrailAlertOverride{}) + Expect(out.filterPattern).To(Equal(base.filterPattern)) + Expect(out.threshold).To(Equal(base.threshold)) +} + +func TestApplyOverride_EmptyStringsSkipped(t *testing.T) { + // Empty list entries (common YAML mistake: trailing `-`) must not produce + // nonsense clauses like `($.x != "")`. + RegisterTestingT(t) + + out := applyOverride(securityAlerts["iamPolicyChanges"], awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"", "real-bot", ""}, + }) + // Should appear exactly once, not three times + Expect(strings.Count(out.filterPattern, `$.userIdentity.userName`)).To(Equal(1)) + Expect(out.filterPattern).To(ContainSubstring(`"real-bot"`)) +} + +func TestEnabledAlerts_OverrideApplied(t *testing.T) { + RegisterTestingT(t) + + selectors := awsApi.CloudTrailAlertSelectors{ + IamPolicyChanges: true, + Overrides: map[string]awsApi.CloudTrailAlertOverride{ + "iamPolicyChanges": { + ExcludeUserNames: []string{"integrail-deployer-bot"}, + }, + }, + } + alerts := enabledAlerts(selectors) + Expect(alerts).To(HaveLen(1)) + Expect(alerts[0].name).To(Equal("ct-iam-policy-changes")) + Expect(alerts[0].filterPattern).To(ContainSubstring(`"integrail-deployer-bot"`)) +} + func TestSecurityAlertDefinitions_UniqueNames(t *testing.T) { RegisterTestingT(t) From 4cc1a03ca5c259a428e07d4f0bb8eb9120a6e2b7 Mon Sep 17 00:00:00 2001 From: Dmitrii Creed Date: Tue, 19 May 2026 21:35:09 +0400 Subject: [PATCH 2/2] fix(cloudtrail-alerts): review-driven correctness fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Cross-model review of PR #277 (codex + gemini, both fact-checked against AWS primary docs) surfaced 6 correctness bugs that would have shipped to prod. Each is fixed here with a test that asserts the desired behavior. 1. NOT EXISTS guard on every exclusion clause. CloudWatch metric-filter `$.field != "x"` returns FALSE when the field is absent, not TRUE. Without a NOT EXISTS guard, an unguarded exclusion like `$.userIdentity.userName != "bot"` would silently drop every event whose userIdentity lacks the field at the top level — AssumedRole events in particular carry userName at $.userIdentity.sessionContext.sessionIssuer .userName, NOT at $.userIdentity.userName. The unguarded form would have dropped the ENTIRE detector for every assumed-role principal, not just the bot we meant to exclude. buildExclusionClauses now generates: (($.field NOT EXISTS) || ($.field != "v1")) # single value (($.field NOT EXISTS) || (($.field != "v1") && ($.field != "v2"))) # multi-value Multi-value uses inner-AND (De Morgan'd from "NOT (v1 OR v2)"). New tests: TestApplyOverride_Exclusions, TestApplyOverride_NotExistsGuard_MultipleValues, TestApplyOverride_MultipleExclusionFields, TestApplyOverride_DeDupesValues. Ref: https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/FilterAndPatternSyntax.html Ref: https://docs.aws.amazon.com/awscloudtrail/latest/userguide/cloudtrail-event-reference-user-identity.html 2. Remove PutResourcePolicy from kmsKeyPolicyChanges. PutResourcePolicy is not a KMS API. (It exists on CloudTrail Lake and a few other services, but never under eventSource=kms.amazonaws.com, so the clause was permanent dead code.) KMS uses PutKeyPolicy for resource policy edits. Comment updated to record why this is excluded to prevent re-introduction. Ref: https://docs.aws.amazon.com/kms/latest/APIReference/API_Operations.html 3. Expand organizationsChanges to cover OU + delegated admin moves. Added MoveAccount, RemoveAccountFromOrganization, RegisterDelegatedAdministrator, DeregisterDelegatedAdministrator, EnableAWSServiceAccess, DisableAWSServiceAccess. RegisterDelegatedAdministrator is the canonical "blind the management account" move: delegate GuardDuty/Security Hub admin to a compromised member, then suppress findings there. Ref: https://docs.aws.amazon.com/organizations/latest/APIReference/API_Operations.html 4. Add Root to consoleLoginWithoutMfa scope. Earlier fix scoped to userIdentity.type=IAMUser to silence the AWS Identity Center false positive. That silently dropped Root console logins (Root type is "Root", not "IAMUser"). rootAccountUsage detector catches ALL Root activity including MFA-protected sessions; this one specifically surfaces Root-without-MFA for triage. Scope expanded to ($.userIdentity.type = "IAMUser" || = "Root"). 5. Remove DeleteInsight from securityHubDisabled. Insights are saved dashboard searches, not detectors. Deleting one is a UI/ housekeeping action, not a "visibility lost" event — pure noise. 6. Fail-fast validation of overrides keys + reflection test for wireup. - New `validateOverrides()` checked at provision time: rejects map keys that don't correspond to any detector. Catches YAML typos like `overrides: { unauthorizedApiCall: ... }` (missing trailing s) at deploy time with a list of valid keys, instead of silently dropping the override and leaving the operator wondering why their exclusion didn't take. - selectorChecks extracted to a single source of truth so reflection test can assert bidirectional consistency: every selector bool maps to a securityAlerts entry and vice versa, count == totalDetectors. Catches the regression where a contributor adds a detector but forgets the wireup. New tests: TestValidateOverrides_Empty, TestValidateOverrides_KnownKey, TestValidateOverrides_UnknownKeyIsLoudError, TestSelectorChecksWireUpAllDetectors, TestApplyOverride_WorstCaseBasePattern (covers leading whitespace + internal OR-groups in base filter pattern under the trim-and-rewrap path). Test summary: 17 tests now pass (was 14 in the first push of PR #277). Signed-off-by: Dmitrii Creed --- .../pulumi/aws/cloudtrail_security_alerts.go | 186 +++++++++++++++--- .../aws/cloudtrail_security_alerts_test.go | 163 ++++++++++++++- 2 files changed, 311 insertions(+), 38 deletions(-) diff --git a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go index ec51401a..c995dad7 100644 --- a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go +++ b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts.go @@ -51,17 +51,24 @@ var securityAlerts = map[string]securityAlertDef{ }, // CloudWatch.3 — Console login without MFA (successful only). // - // Scoped to userIdentity.type = "IAMUser". AWS Identity Center / federated logins - // always emit ConsoleLogin events with additionalEventData.MFAUsed = "No" because - // MFA is enforced upstream at the IdP rather than at the AWS console step. Without - // this scope the detector pages every SSO console session — a well-documented - // CIS CloudWatch.3 false positive. Identity Center sessions should be audited via - // signin.amazonaws.com UserAuthentication events (not currently provisioned by - // this plugin; surface via a separate detector if needed). + // Scoped to userIdentity.type ∈ {IAMUser, Root}. AWS Identity Center / federated + // logins always emit ConsoleLogin events with additionalEventData.MFAUsed = "No" + // because MFA is enforced upstream at the IdP rather than at the AWS console step; + // without the IAMUser-or-Root scope the detector pages every SSO console session — + // a well-documented CIS CloudWatch.3 false positive. + // + // Root is explicitly included here because Root-without-MFA is the highest-severity + // outcome of this detector. The separate rootAccountUsage detector (CIS CloudWatch.1) + // catches ALL Root activity, including MFA-protected sessions; this one specifically + // surfaces the MFA-disabled subset for triage. Dropping Root from scope to fix the + // SSO FP would have silently created a gap. + // + // Identity Center sessions should be audited via signin.amazonaws.com + // UserAuthentication events (not currently provisioned by this plugin). "consoleLoginWithoutMfa": { name: "ct-console-login-no-mfa", - description: "Successful IAM user console login without MFA detected", - filterPattern: `{ ($.eventName = "ConsoleLogin") && ($.additionalEventData.MFAUsed != "Yes") && ($.responseElements.ConsoleLogin = "Success") && ($.userIdentity.type = "IAMUser") }`, + description: "Successful IAM user or Root console login without MFA detected", + filterPattern: `{ ($.eventName = "ConsoleLogin") && ($.additionalEventData.MFAUsed != "Yes") && ($.responseElements.ConsoleLogin = "Success") && (($.userIdentity.type = "IAMUser") || ($.userIdentity.type = "Root")) }`, }, // CloudWatch.4 — IAM policy changes // SetDefaultPolicyVersion is included: flipping a managed policy's default version @@ -178,12 +185,16 @@ var securityAlerts = map[string]securityAlertDef{ // Beyond-CIS — Security Hub disabled or standards turned off. // Why: same attacker-blinding category as GuardDuty. + // + // Note: Security Hub "Insights" are saved dashboard searches, not detectors; + // DeleteInsight is intentionally excluded — it's a UI/housekeeping action and + // generates noise for an alarm that's meant to catch "visibility lost" moves. "securityHubDisabled": { name: "ct-securityhub-disabled", description: "Security Hub disabled or standards/import subscriptions turned off — security visibility lost", filterPattern: `{ ($.eventSource = "securityhub.amazonaws.com") && (($.eventName = "DisableSecurityHub") || ` + `($.eventName = "BatchDisableStandards") || ($.eventName = "DisableImportFindingsForProduct") || ` + - `($.eventName = "DeleteActionTarget") || ($.eventName = "DeleteInsight")) }`, + `($.eventName = "DeleteActionTarget")) }`, }, // Beyond-CIS — IAM access key creation. @@ -220,24 +231,42 @@ var securityAlerts = map[string]securityAlertDef{ // Beyond-CIS — KMS key policy / grant changes. // Why: CIS CloudWatch.7 catches key deletion but not policy edits. PutKeyPolicy / CreateGrant // can quietly hand decrypt rights to a new principal without touching the key's lifecycle. + // + // PutResourcePolicy is intentionally NOT included here — it is not a KMS API. (It exists on + // CloudTrail Lake and other services, but never under eventSource=kms.amazonaws.com, so the + // clause would be permanent dead code.) KMS uses PutKeyPolicy for resource policy edits. + // API ref: https://docs.aws.amazon.com/kms/latest/APIReference/API_Operations.html "kmsKeyPolicyChanges": { name: "ct-kms-key-policy-changes", description: "KMS key policy modified or grant created — verify principal scope and conditions", filterPattern: `{ ($.eventSource = "kms.amazonaws.com") && (($.eventName = "PutKeyPolicy") || ` + - `($.eventName = "PutResourcePolicy") || ($.eventName = "CreateGrant") || ` + - `($.eventName = "RetireGrant") || ($.eventName = "RevokeGrant")) }`, + `($.eventName = "CreateGrant") || ($.eventName = "RetireGrant") || ($.eventName = "RevokeGrant")) }`, }, - // Beyond-CIS — AWS Organizations / SCP changes. - // Why: SCPs are the strongest preventative control in a multi-account org. Detaching - // or deleting one widens blast radius across every account it covered. + // Beyond-CIS — AWS Organizations changes. + // Why: SCPs are the strongest preventative control in a multi-account org. Detaching, + // deleting, or moving accounts to a different OU widens blast radius across every + // account it covered. Delegated-administrator events are the canonical "blind the + // management account" move: register a member account as the GuardDuty/Security Hub + // admin, then suppress findings there. + // + // Coverage by category: + // policies: CreatePolicy / DeletePolicy / UpdatePolicy / Attach/DetachPolicy / Enable/DisablePolicyType + // OU & accounts: MoveAccount / RemoveAccountFromOrganization / LeaveOrganization + // delegated admin: RegisterDelegatedAdministrator / DeregisterDelegatedAdministrator + // service trust: EnableAWSServiceAccess / DisableAWSServiceAccess + // + // API ref: https://docs.aws.amazon.com/organizations/latest/APIReference/API_Operations.html "organizationsChanges": { name: "ct-organizations-changes", - description: "AWS Organizations policy modified — verify SCP boundaries", + description: "AWS Organizations policy, OU membership, or delegated admin changed — verify boundary", filterPattern: `{ ($.eventSource = "organizations.amazonaws.com") && (($.eventName = "CreatePolicy") || ` + `($.eventName = "DeletePolicy") || ($.eventName = "UpdatePolicy") || ($.eventName = "AttachPolicy") || ` + `($.eventName = "DetachPolicy") || ($.eventName = "EnablePolicyType") || ($.eventName = "DisablePolicyType") || ` + - `($.eventName = "LeaveOrganization") || ($.eventName = "RemoveAccountFromOrganization")) }`, + `($.eventName = "MoveAccount") || ($.eventName = "RemoveAccountFromOrganization") || ` + + `($.eventName = "LeaveOrganization") || ($.eventName = "RegisterDelegatedAdministrator") || ` + + `($.eventName = "DeregisterDelegatedAdministrator") || ($.eventName = "EnableAWSServiceAccess") || ` + + `($.eventName = "DisableAWSServiceAccess")) }`, }, // Beyond-CIS — Anonymous external probes (recon). @@ -256,13 +285,15 @@ var securityAlerts = map[string]securityAlertDef{ }, } -// enabledAlerts returns the alert definitions that are enabled in the selector config. -// Per-detector overrides (exclusions, threshold, period) are baked into the returned -// definitions here so downstream provisioning code sees a single resolved struct per alert. -// Results are sorted by name for deterministic Pulumi resource ordering. -func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef { - var result []securityAlertDef - checks := []struct { +// selectorChecks lists every bool selector keyed by the detector slug that maps it to +// a securityAlerts entry. The list is the single source of truth for "what detectors +// can be enabled" — reflection tests assert it covers every securityAlerts key, and the +// override validation step rejects map keys outside this set. +func selectorChecks(selectors awsApi.CloudTrailAlertSelectors) []struct { + key string + enabled bool +} { + return []struct { key string enabled bool }{ @@ -291,7 +322,46 @@ func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef {"organizationsChanges", selectors.OrganizationsChanges}, {"anonymousProbes", selectors.AnonymousProbes}, } - for _, c := range checks { +} + +// validateOverrides returns an error when selectors.Overrides contains a key that +// doesn't correspond to any detector. Catches YAML typos like +// `overrides: { unauthorizedApiCall: ... }` (missing trailing s) — without this guard +// the override would be silently dropped at runtime and the operator would never know +// why their exclusion didn't take effect. +func validateOverrides(selectors awsApi.CloudTrailAlertSelectors) error { + if len(selectors.Overrides) == 0 { + return nil + } + known := map[string]struct{}{} + for _, c := range selectorChecks(selectors) { + known[c.key] = struct{}{} + } + var unknown []string + for k := range selectors.Overrides { + if _, ok := known[k]; !ok { + unknown = append(unknown, k) + } + } + if len(unknown) == 0 { + return nil + } + sort.Strings(unknown) + knownKeys := make([]string, 0, len(known)) + for k := range known { + knownKeys = append(knownKeys, k) + } + sort.Strings(knownKeys) + return errors.Errorf("unknown detector key(s) in alerts.overrides: %v (known: %v)", unknown, knownKeys) +} + +// enabledAlerts returns the alert definitions that are enabled in the selector config. +// Per-detector overrides (exclusions, threshold, period) are baked into the returned +// definitions here so downstream provisioning code sees a single resolved struct per alert. +// Results are sorted by name for deterministic Pulumi resource ordering. +func enabledAlerts(selectors awsApi.CloudTrailAlertSelectors) []securityAlertDef { + var result []securityAlertDef + for _, c := range selectorChecks(selectors) { if !c.enabled { continue } @@ -349,8 +419,33 @@ func applyOverride(def securityAlertDef, ov awsApi.CloudTrailAlertOverride) secu return def } -// buildExclusionClauses turns an override into a list of `(field != "val")` clauses -// in deterministic order. The list is empty when the override has no exclusions. +// buildExclusionClauses turns an override into a list of clauses in deterministic order. +// The list is empty when the override has no exclusions. +// +// Each clause is guarded with a `NOT EXISTS` disjunction so that events where the field +// is absent on the top-level $.userIdentity object pass through the filter unchanged. +// This matters because CloudWatch metric filter patterns return FALSE for `$.field != "x"` +// when $.field is absent — without the guard, a single exclusion would silently mask +// every event whose userIdentity lacks that field. Concretely: AssumedRole events do not +// carry $.userIdentity.userName at the top level (the role's userName lives at +// $.userIdentity.sessionContext.sessionIssuer.userName), so an unguarded +// `$.userIdentity.userName != "bot"` would drop every assumed-role event from the +// detector, including ones that have nothing to do with the bot. +// +// Generated form per field is: +// +// ( ($.field NOT EXISTS) || ( ($.field != "v1") && ($.field != "v2") ) ) +// +// Reading it: "field is absent OR field is none of {v1, v2}". The inner conjunction is +// De Morgan'd from "NOT (field == v1 OR field == v2)" — we want to exclude any of the +// enumerated values, which means the event-must-match predicate is the AND of `!=`s. +// +// References: +// - Filter pattern: `!=` returns FALSE when field is missing; AND/OR/NOT EXISTS are +// all supported. +// https://docs.aws.amazon.com/AmazonCloudWatch/latest/logs/FilterAndPatternSyntax.html +// - CloudTrail userIdentity field shape (AssumedRole vs IAMUser): +// https://docs.aws.amazon.com/awscloudtrail/latest/userguide/cloudtrail-event-reference-user-identity.html func buildExclusionClauses(ov awsApi.CloudTrailAlertOverride) []string { var clauses []string add := func(field string, values []string) { @@ -359,13 +454,41 @@ func buildExclusionClauses(ov awsApi.CloudTrailAlertOverride) []string { // run when the user reorders the YAML list). vs := append([]string(nil), values...) sort.Strings(vs) + // De-dupe + skip empties — common YAML mistake is a trailing `-` producing + // an empty list entry; would otherwise emit `($.x != "")` clauses that match + // nothing useful and just pad the filter pattern length. + seen := map[string]struct{}{} + nonEmpty := make([]string, 0, len(vs)) for _, v := range vs { if v == "" { continue } - clauses = append(clauses, fmt.Sprintf(`($.%s != %q)`, field, v)) + if _, dup := seen[v]; dup { + continue + } + seen[v] = struct{}{} + nonEmpty = append(nonEmpty, v) + } + if len(nonEmpty) == 0 { + return + } + // Single-value short form skips the inner AND wrapper: more readable in + // the generated filter pattern, identical semantics. + var inner string + if len(nonEmpty) == 1 { + inner = fmt.Sprintf(`($.%s != %q)`, field, nonEmpty[0]) + } else { + parts := make([]string, 0, len(nonEmpty)) + for _, v := range nonEmpty { + parts = append(parts, fmt.Sprintf(`($.%s != %q)`, field, v)) + } + inner = "(" + strings.Join(parts, " && ") + ")" } + clause := fmt.Sprintf(`(($.%s NOT EXISTS) || %s)`, field, inner) + clauses = append(clauses, clause) } + // Field order is fixed (not sorted by field name) so a user re-ordering YAML keys + // within a single override doesn't churn Pulumi state. add("userIdentity.userName", ov.ExcludeUserNames) add("userIdentity.principalId", ov.ExcludePrincipalIds) add("userIdentity.arn", ov.ExcludeUserArns) @@ -401,6 +524,13 @@ func CloudTrailSecurityAlerts(ctx *sdk.Context, stack api.Stack, input api.Resou return nil, errors.New("logGroupName is required for CloudTrail security alerts") } + // Fail fast on `alerts.overrides` keys that don't match a known detector — silently + // dropping a typo would create a misleading "no exclusion applied" outcome that's + // very hard to diagnose from a noisy alert channel. + if err := validateOverrides(cfg.Alerts); err != nil { + return nil, err + } + // Pre-flight: if the user declared a trailName, verify the trail has // log-file validation turned on BEFORE we go ahead and provision metric // filters / alarms / Lambdas. Running the security-alerts stack on top diff --git a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go index 053d7f5e..bbab6c9b 100644 --- a/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go +++ b/pkg/clouds/pulumi/aws/cloudtrail_security_alerts_test.go @@ -1,6 +1,7 @@ package aws import ( + "reflect" "strings" "testing" @@ -169,7 +170,26 @@ func TestApplyOverride_Exclusions(t *testing.T) { Expect(out.filterPattern).To(HavePrefix(`{ (`)) Expect(out.filterPattern).To(HaveSuffix(` }`)) Expect(out.filterPattern).To(ContainSubstring(`PutRolePolicy`)) // base predicate intact - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.userName != "integrail-deployer-bot")`)) + + // Single-value exclusion form: (NOT EXISTS) || (!= "value") + // The NOT EXISTS guard keeps events where the field is absent (e.g., + // AssumedRole events lacking top-level $.userIdentity.userName) IN the + // detector; without it, every assumed-role event would silently bypass + // the alarm. + Expect(out.filterPattern).To(ContainSubstring(`(($.userIdentity.userName NOT EXISTS) || ($.userIdentity.userName != "integrail-deployer-bot"))`)) +} + +func TestApplyOverride_NotExistsGuard_MultipleValues(t *testing.T) { + // Multi-value form uses inner-AND: De Morgan'd "NOT (v1 OR v2)" = "!= v1 AND != v2". + // The OR with NOT EXISTS still keeps absent-field events flowing through. + RegisterTestingT(t) + + out := applyOverride(securityAlerts["iamPolicyChanges"], awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"alpha", "beta", "gamma"}, + }) + Expect(out.filterPattern).To(ContainSubstring( + `(($.userIdentity.userName NOT EXISTS) || (($.userIdentity.userName != "alpha") && ($.userIdentity.userName != "beta") && ($.userIdentity.userName != "gamma")))`, + )) } func TestApplyOverride_MultipleExclusionFields(t *testing.T) { @@ -184,12 +204,18 @@ func TestApplyOverride_MultipleExclusionFields(t *testing.T) { }, }) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.userName != "prowler-readonly")`)) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.type != "AWSService")`)) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.type != "AWSAccount")`)) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.invokedBy != "s3.amazonaws.com")`)) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.invokedBy != "lambda.amazonaws.com")`)) - Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.arn != "arn:aws:sts::*:assumed-role/AWSServiceRoleFor*/*")`)) + // Each exclusion field gets its own NOT EXISTS guard. + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.userName NOT EXISTS)`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.type NOT EXISTS)`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.invokedBy NOT EXISTS)`)) + Expect(out.filterPattern).To(ContainSubstring(`($.userIdentity.arn NOT EXISTS)`)) + // Values still wired up correctly: + Expect(out.filterPattern).To(ContainSubstring(`"prowler-readonly"`)) + Expect(out.filterPattern).To(ContainSubstring(`"AWSService"`)) + Expect(out.filterPattern).To(ContainSubstring(`"AWSAccount"`)) + Expect(out.filterPattern).To(ContainSubstring(`"s3.amazonaws.com"`)) + Expect(out.filterPattern).To(ContainSubstring(`"lambda.amazonaws.com"`)) + Expect(out.filterPattern).To(ContainSubstring(`"arn:aws:sts::*:assumed-role/AWSServiceRoleFor*/*"`)) } func TestApplyOverride_Deterministic(t *testing.T) { @@ -232,17 +258,66 @@ func TestApplyOverride_EmptyOverrideIsNoop(t *testing.T) { Expect(out.threshold).To(Equal(base.threshold)) } +// TestApplyOverride_WorstCaseBasePattern guards the trim-and-rewrap logic that wraps +// the base pattern in parens before AND'ing exclusions. The shape we have to preserve: +// - leading/trailing whitespace inside the braces (idiomatic indentation), +// - internal parentheses from existing OR-groups, +// - mixed AND/OR/&& at the top level. +// If a future contributor writes a pattern with leading whitespace or extra braces, +// the wrap output should still be syntactically valid CloudWatch. +func TestApplyOverride_WorstCaseBasePattern(t *testing.T) { + RegisterTestingT(t) + + // Synthetic worst-case: leading whitespace, internal OR-group, trailing whitespace, + // AND with a sub-expression. + worst := securityAlertDef{ + name: "worst-case", + description: "synthetic", + filterPattern: `{ ($.eventName = "A") || ($.eventName = "B") && ($.eventSource = "x.amazonaws.com") }`, + } + out := applyOverride(worst, awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"bot"}, + }) + + // Output must still be a single braced expression + Expect(out.filterPattern).To(HavePrefix("{ ")) + Expect(out.filterPattern).To(HaveSuffix(" }")) + // Base predicate atoms intact + Expect(out.filterPattern).To(ContainSubstring(`($.eventName = "A")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.eventName = "B")`)) + Expect(out.filterPattern).To(ContainSubstring(`($.eventSource = "x.amazonaws.com")`)) + // Exclusion present + Expect(out.filterPattern).To(ContainSubstring(`"bot"`)) + // Top-level structure: base wrapped in parens, then `&&` to exclusion + Expect(out.filterPattern).To(MatchRegexp(`^\{ \(.*\) && \(.*\) \}$`)) +} + func TestApplyOverride_EmptyStringsSkipped(t *testing.T) { // Empty list entries (common YAML mistake: trailing `-`) must not produce - // nonsense clauses like `($.x != "")`. + // nonsense clauses like `($.x != "")`. Each non-empty value still contributes + // a NOT-EXISTS-guarded clause: in single-value form we get the field name twice + // (once for NOT EXISTS, once for !=). RegisterTestingT(t) out := applyOverride(securityAlerts["iamPolicyChanges"], awsApi.CloudTrailAlertOverride{ ExcludeUserNames: []string{"", "real-bot", ""}, }) - // Should appear exactly once, not three times - Expect(strings.Count(out.filterPattern, `$.userIdentity.userName`)).To(Equal(1)) + // Single-value clause shape: (($.field NOT EXISTS) || ($.field != "real-bot")) + // — two occurrences of $.userIdentity.userName, no empty-string match. + Expect(strings.Count(out.filterPattern, `$.userIdentity.userName`)).To(Equal(2)) Expect(out.filterPattern).To(ContainSubstring(`"real-bot"`)) + Expect(out.filterPattern).ToNot(ContainSubstring(`!= ""`)) +} + +func TestApplyOverride_DeDupesValues(t *testing.T) { + // User pastes the same exclusion twice — generated filter must dedupe so we + // don't emit a redundant `&& ($.field != "v") && ($.field != "v")`. + RegisterTestingT(t) + + out := applyOverride(securityAlerts["iamPolicyChanges"], awsApi.CloudTrailAlertOverride{ + ExcludeUserNames: []string{"bot", "bot", "bot"}, + }) + Expect(strings.Count(out.filterPattern, `"bot"`)).To(Equal(1)) } func TestEnabledAlerts_OverrideApplied(t *testing.T) { @@ -262,6 +337,74 @@ func TestEnabledAlerts_OverrideApplied(t *testing.T) { Expect(alerts[0].filterPattern).To(ContainSubstring(`"integrail-deployer-bot"`)) } +// TestSelectorChecksWireUpAllDetectors guards against the regression where a contributor +// adds a new bool to CloudTrailAlertSelectors + a new entry to securityAlerts but forgets +// to wire it through selectorChecks. Without this test, the detector would never appear +// in enabledAlerts and the new toggle would be silently dead. +func TestSelectorChecksWireUpAllDetectors(t *testing.T) { + RegisterTestingT(t) + + // Flip every bool selector on via reflection. selectorChecks then enumerates + // the (key, enabled) pairs; we assert each securityAlerts key appears exactly once. + sel := awsApi.CloudTrailAlertSelectors{} + v := reflect.ValueOf(&sel).Elem() + for i := 0; i < v.NumField(); i++ { + f := v.Field(i) + if f.Kind() == reflect.Bool { + f.SetBool(true) + } + } + + checkedKeys := map[string]int{} + for _, c := range selectorChecks(sel) { + Expect(c.enabled).To(BeTrue(), "selectorChecks did not pick up the bool for %q (likely missing wireup)", c.key) + checkedKeys[c.key]++ + } + + // Bidirectional: every securityAlerts key must be in selectorChecks + for key := range securityAlerts { + Expect(checkedKeys).To(HaveKey(key), "securityAlerts has %q but selectorChecks doesn't wire it up", key) + Expect(checkedKeys[key]).To(Equal(1), "selectorChecks lists %q more than once", key) + } + // And every selectorChecks key must be in securityAlerts + for key := range checkedKeys { + Expect(securityAlerts).To(HaveKey(key), "selectorChecks lists %q but securityAlerts has no entry", key) + } + // And the count must equal totalDetectors (catches half-applied additions) + Expect(checkedKeys).To(HaveLen(totalDetectors)) +} + +func TestValidateOverrides_Empty(t *testing.T) { + RegisterTestingT(t) + Expect(validateOverrides(awsApi.CloudTrailAlertSelectors{})).To(Succeed()) +} + +func TestValidateOverrides_KnownKey(t *testing.T) { + RegisterTestingT(t) + err := validateOverrides(awsApi.CloudTrailAlertSelectors{ + Overrides: map[string]awsApi.CloudTrailAlertOverride{ + "iamPolicyChanges": {ExcludeUserNames: []string{"bot"}}, + }, + }) + Expect(err).To(Succeed()) +} + +func TestValidateOverrides_UnknownKeyIsLoudError(t *testing.T) { + // Common YAML mistake: misspell the detector key. Without validation the override + // is silently dropped and the operator gets no signal — they just keep seeing the + // alarm fire and wonder why their exclusion didn't take. Fail at deploy time instead. + RegisterTestingT(t) + err := validateOverrides(awsApi.CloudTrailAlertSelectors{ + Overrides: map[string]awsApi.CloudTrailAlertOverride{ + "unauthorizedApiCall": {ExcludeUserNames: []string{"bot"}}, // missing trailing 's' + }, + }) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("unauthorizedApiCall")) + // Should hint at what the user can use + Expect(err.Error()).To(ContainSubstring("known")) +} + func TestSecurityAlertDefinitions_UniqueNames(t *testing.T) { RegisterTestingT(t)