Skip to content

Commit 76ae56c

Browse files
tofikwestclaude
andauthored
fix(cloud-security): add missing logGroupName to CreateLogGroup remediation (#3515)
* fix(cloud-security): add missing logGroupName to CreateLogGroup remediation ## Problem Auto-remediation for the CloudTrail/CloudWatch Logs compliance finding fails silently. The CreateLogGroup API call rejects the request because no log group name is supplied, leaving the resource non-compliant. Customers who fall back to manual remediation steps also report difficulty completing them independently. ## Root cause The CreateLogGroupCommand in the auto-remediation flow does not enforce or supply the required logGroupName parameter. Unlike PutMetricFilter (which has both a required-params check and a deterministic name injector), CreateLogGroup lacks these guards. The parameter reaches the AWS API as undefined, causing rejection. ## Fix Add logGroupName to REQUIRED_PARAMS in aws-command-executor.ts alongside an injector that defaults the name to aws-cloudtrail-logs, mirroring the pattern already established for PutMetricFilter. This ensures the CreateLogGroup call always includes a valid name without manual configuration. ## Explicitly NOT touched Manual remediation step clarity (addressed separately if needed). Parser or orchestration logic outside cloud-security scope. Any other compliance remediations. ## Verification Added regression test asserting that CreateLogGroup remediation includes a valid logGroupName parameter and passes validation. Unit tests for aws-command-executor and the injector logic pass locally ✅ * fix(bug-todo): address cubic review Address review findings. * fix(cloud-security): fail-fast on missing log group name; no false-success backfill (CS-787) Two gaps in the CreateLogGroup remediation fix: 1. Add CreateLogGroupCommand: ['logGroupName'] to REQUIRED_PARAMS. A CreateLogGroup with no name was reaching AWS and failing with an opaque "Member must not be null", silently failing the whole remediation. This also protects every sibling remediation that mints a log group (Step Functions, SSM, Transfer Family), which the CloudTrail-only name backfill deliberately skips. 2. The name backfill only fills a name it can DERIVE from the trail's log-group ARN (guarantees the created group matches what UpdateTrail links to = complete fix). When no step carries a parseable CloudWatchLogsLogGroupArn, nothing links the trail to the group, so defaulting the name created an orphan log group and reported a FALSE success on a still-non-compliant finding. Now it leaves the step unfilled so REQUIRED_PARAMS rejects it and the fix falls back honestly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 1940f06 commit 76ae56c

4 files changed

Lines changed: 335 additions & 1 deletion

File tree

‎apps/api/src/cloud-security/aws-command-executor.spec.ts‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,30 @@ describe('validatePlanSteps — REQUIRED_PARAMS', () => {
3939
);
4040
});
4141

42+
it('reports a clear error when CreateLogGroupCommand is missing logGroupName (CS-787 fail-fast)', () => {
43+
const errors = validatePlanSteps([
44+
step({ service: 'logs', command: 'CreateLogGroupCommand', params: {} }),
45+
]);
46+
expect(errors).toEqual(
47+
expect.arrayContaining([
48+
expect.stringMatching(
49+
/CreateLogGroupCommand\): Required param "logGroupName" is missing or empty/,
50+
),
51+
]),
52+
);
53+
});
54+
55+
it('does NOT error when CreateLogGroupCommand has a logGroupName', () => {
56+
const errors = validatePlanSteps([
57+
step({
58+
service: 'logs',
59+
command: 'CreateLogGroupCommand',
60+
params: { logGroupName: 'aws-cloudtrail-logs' },
61+
}),
62+
]);
63+
expect(errors.filter((e) => e.includes('logGroupName'))).toHaveLength(0);
64+
});
65+
4266
it('does NOT error when AWSServiceName is populated', () => {
4367
const errors = validatePlanSteps([
4468
step({

‎apps/api/src/cloud-security/aws-command-executor.ts‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -166,6 +166,12 @@ export const REQUIRED_PARAMS: Record<string, readonly string[]> = {
166166
PutBucketPolicyCommand: ['Bucket', 'Policy'],
167167
CreateTrailCommand: ['Name', 'S3BucketName'],
168168
PutMetricFilterCommand: ['logGroupName', 'filterName', 'metricTransformations'],
169+
// A CreateLogGroup with no name is rejected by AWS with an opaque "Member must
170+
// not be null" error, silently failing the whole remediation (CS-787). Fail
171+
// fast with a clear message here. This covers CloudTrail AND every sibling
172+
// remediation that mints a log group (Step Functions, SSM Session Manager,
173+
// Transfer Family), which the CloudTrail-only name backfill deliberately skips.
174+
CreateLogGroupCommand: ['logGroupName'],
169175
};
170176

171177
/**

‎apps/api/src/cloud-security/plan-normalizer-aws-edge-cases.spec.ts‎

Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -143,3 +143,199 @@ describe('normalizeFixPlan — AWS remediation edge cases', () => {
143143
expect(result.fixSteps[0].params).toEqual({ GroupName: 'default' });
144144
});
145145
});
146+
147+
// CS-787: the "CloudTrail not integrated with CloudWatch Logs" auto-fix mints a
148+
// new log group, so the AI must invent the name. When it omits/empties
149+
// logGroupName, AWS rejects CreateLogGroup with "Member must not be null" and
150+
// the whole fix falls back to manual steps. The normalizer must guarantee a
151+
// valid name.
152+
describe('normalizeFixPlan — CreateLogGroup logGroupName backfill (CS-787)', () => {
153+
const createLogGroupStep = (params: Record<string, unknown>): AwsCommandStep =>
154+
makeStep({ service: 'logs', command: 'CreateLogGroupCommand', params });
155+
156+
const updateTrailStep = (logGroupArn: string): AwsCommandStep =>
157+
makeStep({
158+
service: 'cloudtrail',
159+
command: 'UpdateTrailCommand',
160+
params: {
161+
Name: 'compai-trail',
162+
CloudWatchLogsLogGroupArn: logGroupArn,
163+
CloudWatchLogsRoleArn:
164+
'arn:aws:iam::123456789012:role/CompAI-CloudTrailDelivery',
165+
},
166+
});
167+
168+
// A CloudTrail trail step with NO usable log-group ARN: the plan is still a
169+
// CloudTrail integration fix, but there is nothing to derive the name from and
170+
// nothing that will link the trail to a group. The normalizer must NOT invent a
171+
// name here — doing so would create an orphan log group and report a false
172+
// success on a still-non-compliant finding (CS-787). It leaves the step unfilled
173+
// so REQUIRED_PARAMS validation rejects it and the fix fails honestly.
174+
const updateTrailStepMissingArn = (): AwsCommandStep =>
175+
makeStep({
176+
service: 'cloudtrail',
177+
command: 'UpdateTrailCommand',
178+
params: { Name: 'compai-trail' },
179+
});
180+
181+
it('leaves an OMITTED logGroupName unfilled when no trail ARN exists (no orphan group / false success — CS-787)', () => {
182+
const plan = makePlan({
183+
fixSteps: [createLogGroupStep({}), updateTrailStepMissingArn()],
184+
});
185+
186+
const result = normalizeFixPlan(plan);
187+
188+
// Nothing to link the trail to → do NOT invent a name; leave it for
189+
// REQUIRED_PARAMS validation to reject rather than create an orphan group.
190+
expect(result.fixSteps[0].params.logGroupName).toBeUndefined();
191+
});
192+
193+
it('leaves a blank logGroupName unchanged when no trail ARN exists', () => {
194+
const plan = makePlan({
195+
fixSteps: [
196+
createLogGroupStep({ logGroupName: ' ' }),
197+
updateTrailStepMissingArn(),
198+
],
199+
});
200+
201+
const result = normalizeFixPlan(plan);
202+
203+
expect(result.fixSteps[0].params.logGroupName).toBe(' ');
204+
});
205+
206+
it('derives the name from the trail step ARN so CreateLogGroup matches what UpdateTrail links to', () => {
207+
const plan = makePlan({
208+
fixSteps: [
209+
createLogGroupStep({ logGroupName: '' }),
210+
updateTrailStep(
211+
'arn:aws:logs:us-east-1:123456789012:log-group:/compai/cloudtrail:*',
212+
),
213+
],
214+
});
215+
216+
const result = normalizeFixPlan(plan);
217+
218+
expect(result.fixSteps[0].params.logGroupName).toBe('/compai/cloudtrail');
219+
});
220+
221+
it('preserves a logGroupName the AI already supplied', () => {
222+
const plan = makePlan({
223+
fixSteps: [
224+
createLogGroupStep({ logGroupName: 'my-custom-cloudtrail-lg' }),
225+
updateTrailStep(
226+
'arn:aws:logs:us-east-1:123456789012:log-group:something-else:*',
227+
),
228+
],
229+
});
230+
231+
const result = normalizeFixPlan(plan);
232+
233+
expect(result.fixSteps[0].params.logGroupName).toBe(
234+
'my-custom-cloudtrail-lg',
235+
);
236+
});
237+
238+
it('also backfills a CreateLogGroup step that appears in rollback steps (from the trail ARN)', () => {
239+
const plan = makePlan({
240+
rollbackSteps: [
241+
createLogGroupStep({}),
242+
updateTrailStep(
243+
'arn:aws:logs:us-east-1:123456789012:log-group:/compai/cloudtrail:*',
244+
),
245+
],
246+
});
247+
248+
const result = normalizeFixPlan(plan);
249+
250+
expect(result.rollbackSteps[0].params.logGroupName).toBe('/compai/cloudtrail');
251+
});
252+
253+
it('is a no-op for plans without a CreateLogGroup step', () => {
254+
const plan = makePlan({
255+
fixSteps: [
256+
makeStep({ service: 's3', command: 'PutPublicAccessBlockCommand' }),
257+
],
258+
});
259+
260+
const result = normalizeFixPlan(plan);
261+
262+
expect(result.fixSteps).toEqual(plan.fixSteps);
263+
});
264+
265+
// The CloudTrail default must NOT leak into other logging remediations. Many
266+
// findings mint a log group via CreateLogGroupCommand (Step Functions, SSM
267+
// Session Manager, Transfer Family, …) but reference it under their own name;
268+
// stamping them with `aws-cloudtrail-logs` would create a group that doesn't
269+
// match the sibling step that consumes it.
270+
it('does NOT inject the CloudTrail default into a Step Functions logging remediation', () => {
271+
const plan = makePlan({
272+
fixSteps: [
273+
createLogGroupStep({}),
274+
makeStep({
275+
service: 'sfn',
276+
command: 'UpdateStateMachineCommand',
277+
params: {
278+
stateMachineArn:
279+
'arn:aws:states:us-east-1:123456789012:stateMachine:my-sm',
280+
loggingConfiguration: {
281+
level: 'ALL',
282+
includeExecutionData: true,
283+
destinations: [
284+
{
285+
cloudWatchLogsLogGroup: {
286+
logGroupArn:
287+
'arn:aws:logs:us-east-1:123456789012:log-group:/aws/vendedlogs/states/my-sm:*',
288+
},
289+
},
290+
],
291+
},
292+
},
293+
}),
294+
],
295+
});
296+
297+
const result = normalizeFixPlan(plan);
298+
299+
expect(result.fixSteps[0].params.logGroupName).toBeUndefined();
300+
});
301+
302+
it('does NOT inject the CloudTrail default into an SSM Session Manager logging remediation', () => {
303+
const plan = makePlan({
304+
fixSteps: [
305+
createLogGroupStep({}),
306+
makeStep({
307+
service: 'ssm',
308+
command: 'UpdateDocumentCommand',
309+
params: {
310+
Name: 'SSM-SessionManagerRunShell',
311+
Content: JSON.stringify({
312+
inputs: { cloudWatchLogGroupName: '/aws/ssm/session-logs' },
313+
}),
314+
},
315+
}),
316+
],
317+
});
318+
319+
const result = normalizeFixPlan(plan);
320+
321+
expect(result.fixSteps[0].params.logGroupName).toBeUndefined();
322+
});
323+
324+
it('leaves the name unfilled for a CloudTrail plan (detected via CreateTrailCommand) that carries no log-group ARN', () => {
325+
const plan = makePlan({
326+
fixSteps: [
327+
createLogGroupStep({}),
328+
makeStep({
329+
service: 'cloudtrail',
330+
command: 'CreateTrailCommand',
331+
params: { Name: 'compai-trail', S3BucketName: 'compai-logs' },
332+
}),
333+
],
334+
});
335+
336+
const result = normalizeFixPlan(plan);
337+
338+
// No ARN anywhere → nothing links the trail to the group → don't invent a name.
339+
expect(result.fixSteps[0].params.logGroupName).toBeUndefined();
340+
});
341+
});

‎apps/api/src/cloud-security/plan-normalizer.ts‎

Lines changed: 109 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import type { AwsCommandStep, FixPlan } from './ai-remediation.prompt';
2+
import { logGroupNameFromArn } from './providers/aws/cloudwatch.adapter';
23

34
/**
45
* Maps an AWS service prefix (as it appears in `AwsCommandStep.service`)
@@ -40,6 +41,18 @@ const EC2_SECURITY_GROUP_COMMANDS = new Set([
4041
]);
4142
const S3_ACL_COMMANDS = new Set(['PutBucketAclCommand']);
4243
const S3_ACL_PERMISSIONS = new Set(['s3:PutBucketAcl']);
44+
const CREATE_LOG_GROUP_COMMAND = 'CreateLogGroupCommand';
45+
const CLOUDTRAIL_SERVICE = 'cloudtrail';
46+
/**
47+
* Commands that operate on a CloudTrail trail. A step whose `service` is
48+
* CloudTrail — or that runs one of these commands — is what marks a plan as the
49+
* CloudTrail→CloudWatch Logs integration fix, the ONLY remediation the
50+
* `aws-cloudtrail-logs` default is correct for.
51+
*/
52+
const CLOUDTRAIL_TRAIL_COMMANDS = new Set([
53+
'CreateTrailCommand',
54+
'UpdateTrailCommand',
55+
]);
4356

4457
export interface NormalizeFixPlanContext {
4558
resourceId?: string | null;
@@ -73,7 +86,9 @@ function normalizeStepList(
7386
securityGroupId: string | null,
7487
): AwsCommandStep[] {
7588
return backfillSecurityGroupParams(
76-
removeUnsupportedS3AclSteps(backfillServiceLinkedRoleParams(steps)),
89+
backfillLogGroupName(
90+
removeUnsupportedS3AclSteps(backfillServiceLinkedRoleParams(steps)),
91+
),
7792
securityGroupId,
7893
);
7994
}
@@ -168,3 +183,96 @@ function inferServiceLinkedRolePrincipal(
168183
}
169184
return null;
170185
}
186+
187+
/**
188+
* Backfill `logGroupName` on `CreateLogGroupCommand` steps the AI left empty.
189+
*
190+
* The "CloudTrail not integrated with CloudWatch Logs" remediation has to MINT
191+
* a brand-new log group — the finding carries no existing log-group name
192+
* (none exists yet), so the model must invent one. When it omits/empties
193+
* `logGroupName`, AWS rejects the call with "Member must not be null / value
194+
* null at logGroupName", the single step-repair fails, and the whole auto-fix
195+
* silently falls back to manual steps. This is the same failure class as the
196+
* SLR `AWSServiceName` backfill above.
197+
*
198+
* Resolve the name deterministically: prefer the log group the plan's trail
199+
* step already targets (parsed from its `CloudWatchLogsLogGroupArn`) so the
200+
* group we create matches what UpdateTrail links to; otherwise fall back to the
201+
* canonical default. Only a missing/empty value is filled — a name the AI
202+
* supplied is left untouched.
203+
*
204+
* Scope: this ONLY runs for the CloudTrail integration plan itself (detected by
205+
* a CloudTrail trail step). Other logging remediations (SSM Session Manager,
206+
* Step Functions, Transfer Family, …) also mint a log group via
207+
* `CreateLogGroupCommand`, but each references it under its own name — stamping
208+
* them with `aws-cloudtrail-logs` would create a group that doesn't match the
209+
* sibling step that consumes it.
210+
*/
211+
function backfillLogGroupName(steps: AwsCommandStep[]): AwsCommandStep[] {
212+
const needsBackfill = steps.some(
213+
(step) =>
214+
step.command === CREATE_LOG_GROUP_COMMAND &&
215+
!hasNonEmptyString(step.params?.logGroupName),
216+
);
217+
if (!needsBackfill || !isCloudTrailIntegrationPlan(steps)) return steps;
218+
219+
// Only backfill the name we can DERIVE from the log-group ARN the trail step
220+
// actually links to — that guarantees the group we create matches what
221+
// UpdateTrail points at, i.e. a COMPLETE integration. If no step carries a
222+
// parseable CloudWatchLogsLogGroupArn, nothing in the plan will link the trail
223+
// to the group, so inventing a default name would create an orphan log group
224+
// and report a FALSE success on a still-non-compliant finding (CS-787 cubic P1).
225+
// Leave it unfilled instead: the CreateLogGroup step then fails REQUIRED_PARAMS
226+
// validation and the fix falls back honestly rather than pretending it worked.
227+
const resolved = inferCloudTrailLogGroupName(steps);
228+
if (!resolved) return steps;
229+
230+
return steps.map((step) => {
231+
if (
232+
step.command !== CREATE_LOG_GROUP_COMMAND ||
233+
hasNonEmptyString(step.params?.logGroupName)
234+
) {
235+
return step;
236+
}
237+
return {
238+
...step,
239+
params: { ...(step.params ?? {}), logGroupName: resolved },
240+
};
241+
});
242+
}
243+
244+
/**
245+
* True when the plan operates on a CloudTrail trail — the signal that a minted
246+
* log group is destined for CloudTrail delivery and the `aws-cloudtrail-logs`
247+
* default (the name the console proposes for trail log delivery) is correct.
248+
* Keyed on the CloudTrail SDK client suffix or a trail command so it also
249+
* matches plans that only create/update the trail without a parseable ARN.
250+
*/
251+
function isCloudTrailIntegrationPlan(steps: AwsCommandStep[]): boolean {
252+
return steps.some(
253+
(step) =>
254+
step.service === CLOUDTRAIL_SERVICE ||
255+
CLOUDTRAIL_TRAIL_COMMANDS.has(step.command),
256+
);
257+
}
258+
259+
/**
260+
* Derive the CloudWatch Logs log-group NAME the plan's CloudTrail step points
261+
* at, from any step carrying a `CloudWatchLogsLogGroupArn`. Keeps the created
262+
* log group consistent with the trail integration. Returns null when no step
263+
* references a parseable log-group ARN.
264+
*/
265+
function inferCloudTrailLogGroupName(steps: AwsCommandStep[]): string | null {
266+
for (const step of steps) {
267+
const arn = step.params?.CloudWatchLogsLogGroupArn;
268+
if (typeof arn === 'string') {
269+
const name = logGroupNameFromArn(arn);
270+
if (name) return name;
271+
}
272+
}
273+
return null;
274+
}
275+
276+
function hasNonEmptyString(value: unknown): boolean {
277+
return typeof value === 'string' && value.trim().length > 0;
278+
}

0 commit comments

Comments
 (0)