Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add per-rule granular error suppression #1826

Merged
merged 8 commits into from
Nov 7, 2024
63 changes: 63 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,69 @@ You would see the following error on synth/deploy

</details>

## Suppressing Rule Validation Failures

When a rule validation fails it is handled similarly to a rule violation, and can be suppressed in the same manner. The `ID` for a rule failure is `CdkNagValidationFailure`.

If a rule is suppressed in a non-granular manner (i.e. `appliesTo` is not set, see example 1 above) then validation failures on that rule are also suppressed.

Validation failure suppression respects any applied [Suppression Ignore Conditions](#conditionally-ignoring-suppressions)

<details>
<summary>Example 1) Suppress all Validation Failures on a Resource</summary>

```typescript
import { SecurityGroup, Vpc, Peer, Port } from 'aws-cdk-lib/aws-ec2';
import { Stack, StackProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { NagSuppressions } from 'cdk-nag';

export class CdkTestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);
const test = new SecurityGroup(this, 'test', {
vpc: new Vpc(this, 'vpc'),
});
test.addIngressRule(Peer.anyIpv4(), Port.allTraffic());
NagSuppressions.addResourceSuppressions(test, [
{ id: 'CdkNagValidationFailure', reason: 'lorem ipsum' },
]);
}
}
```

</details>

<details>
<summary>Example 2) Granular Suppression of Validation Failures</summary>
Validation failures can be suppressed for individual rules by using `appliesTo` to list the desired rules

```typescript
import { SecurityGroup, Vpc, Peer, Port } from 'aws-cdk-lib/aws-ec2';
import { Stack, StackProps } from 'aws-cdk-lib';
import { Construct } from 'constructs';
import { NagSuppressions } from 'cdk-nag';

export class CdkTestStack extends Stack {
constructor(scope: Construct, id: string, props?: StackProps) {
super(scope, id, props);
const test = new SecurityGroup(this, 'test', {
vpc: new Vpc(this, 'vpc'),
});
test.addIngressRule(Peer.anyIpv4(), Port.allTraffic());
NagSuppressions.addResourceSuppressions(test, [
{
id: 'CdkNagValidationFailure',
reason: 'lorem ipsum',
appliesTo: ['AwsSolutions-L1'],
},
]);
}
}
```

</details>

## Suppressing `aws-cdk-lib/pipelines` Violations

The [aws-cdk-lib/pipelines.CodePipeline](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.pipelines.CodePipeline.html) construct and its child constructs are not guaranteed to be "Visited" by `Aspects`, as they are not added during the "Construction" phase of the [cdk lifecycle](https://docs.aws.amazon.com/cdk/v2/guide/apps.html#lifecycle). Because of this behavior, you may experience problems such as rule violations not appearing or the inability to suppress violations on these constructs.
Expand Down
4 changes: 2 additions & 2 deletions src/nag-logger.ts
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ export class AnnotationLogger implements INagLogger {
const information = `'${data.ruleId}' threw an error during validation. This is generally caused by a parameter referencing an intrinsic function. You can suppress the "${VALIDATION_FAILURE_ID}" to get rid of this error. For more details enable verbose logging.'`;
const message = this.createMessage(
VALIDATION_FAILURE_ID,
'',
data.ruleId,
information,
data.errorMessage,
this.verbose
Expand All @@ -171,7 +171,7 @@ export class AnnotationLogger implements INagLogger {
if (this.logIgnores === true) {
const message = this.createMessage(
this.suppressionId,
'',
data.ruleId,
`${VALIDATION_FAILURE_ID} was triggered but suppressed.`,
data.errorSuppressionReason,
this.verbose
Expand Down
20 changes: 16 additions & 4 deletions src/nag-pack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -199,11 +199,12 @@ export abstract class NagPack implements IAspect {
} catch (error) {
const reason = this.ignoreRule(
allSuppressions,
VALIDATION_FAILURE_ID,
ruleId,
'',
params.node,
params.level,
params.ignoreSuppressionCondition
params.ignoreSuppressionCondition,
true
);
if (reason) {
this.loggers.forEach((t) =>
Expand All @@ -230,6 +231,7 @@ export abstract class NagPack implements IAspect {
* @param ruleId The id of the rule to ignore.
* @param resource The resource being evaluated.
* @param findingId The id of the finding that is being checked.
* @param validationFailure Whether the rule is being checked due to a validation failure.
* @returns The reason the rule was ignored, or an empty string.
*/
protected ignoreRule(
Expand All @@ -238,10 +240,20 @@ export abstract class NagPack implements IAspect {
findingId: string,
resource: CfnResource,
level: NagMessageLevel,
ignoreSuppressionCondition?: INagSuppressionIgnore
ignoreSuppressionCondition?: INagSuppressionIgnore,
validationFailure: boolean = false
): string {
for (let suppression of suppressions) {
if (NagSuppressionHelper.doesApply(suppression, ruleId, findingId)) {
if (
NagSuppressionHelper.doesApply(suppression, ruleId, findingId) ||
// If this is marked as an exception, also check for a suppression on VALIDATION_FAILURE_ID
(validationFailure &&
NagSuppressionHelper.doesApply(
suppression,
VALIDATION_FAILURE_ID,
ruleId
))
) {
const ignoreMessage = new SuppressionIgnoreOr(
this.userGlobalSuppressionIgnore ?? new SuppressionIgnoreNever(),
this.packGlobalSuppressionIgnore ?? new SuppressionIgnoreNever(),
Expand Down
189 changes: 39 additions & 150 deletions test/Conditions.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import {
SuppressionIgnoreOr,
} from '../src';
import { TestPack } from './rules/utils';
import { expectMessages } from './test-utils';

describe('Rule Suppression Condition Core Functionality', () => {
const IGNORE = new SuppressionIgnoreAlways('IGNORED.');
Expand All @@ -37,22 +38,10 @@ describe('Rule Suppression Condition Core Functionality', () => {
Aspects.of(stack).add(testPack);
new CfnResource(stack, 'nice', { type: 'AWS::Infinidash::Meme' });
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining(
'was ignored for the following reason(s)'
),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition'],
notContaining: ['was ignored for the following reason(s)'],
});
});
test('Not ignored with suppression', () => {
const testPack = new TestPack(
Expand All @@ -74,22 +63,12 @@ describe('Rule Suppression Condition Core Functionality', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining(
'was ignored for the following reason(s)'
),
}),
})
);
expectMessages(messages, {
notContaining: [
'Test-Condition',
'was ignored for the following reason(s)',
],
});
});
test('Ignored no suppression', () => {
const testPack = new TestPack(
Expand All @@ -105,22 +84,10 @@ describe('Rule Suppression Condition Core Functionality', () => {
new CfnResource(stack, 'nice', { type: 'AWS::Infinidash::Meme' });
Aspects.of(stack).add(testPack);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining(
'was ignored for the following reason(s)'
),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition'],
notContaining: ['was ignored for the following reason(s)'],
});
});
test('Ignored with suppression', () => {
const testPack = new TestPack(
Expand All @@ -142,22 +109,9 @@ describe('Rule Suppression Condition Core Functionality', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining(
'was ignored for the following reason(s)'
),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition', 'was ignored for the following reason(s)'],
});
});
});

Expand Down Expand Up @@ -189,20 +143,9 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('IGNORED.\n\tIGNORED.'),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition', 'IGNORED.\n\tIGNORED.'],
});
});
test('Should Not Ignore Suppression', () => {
const testPack = new TestPack(
Expand All @@ -224,20 +167,9 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('IGNORED.'),
}),
})
);
expectMessages(messages, {
notContaining: ['Test-Condition', 'IGNORED.'],
});
});
});
describe('IgnoreOr', () => {
Expand All @@ -261,20 +193,9 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('IGNORED.'),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition', 'IGNORED.'],
});
});
test('Should Not Ignore Suppression', () => {
const testPack = new TestPack(
Expand All @@ -296,22 +217,12 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining(
'was ignored for the following reason(s)'
),
}),
})
);
expectMessages(messages, {
notContaining: [
'Test-Condition',
'was ignored for the following reason(s)',
],
});
});
});
describe('SuppressionIgnoreErrors', () => {
Expand All @@ -336,20 +247,9 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('categorized as an ERROR'),
}),
})
);
expectMessages(messages, {
containing: ['Test-Condition', 'categorized as an ERROR'],
});
});
test('Should Not Ignore Suppression', () => {
const testPack = new TestPack(
Expand All @@ -372,20 +272,9 @@ describe('Prebuilt Rule Suppression Conditions', () => {
},
]);
const messages = SynthUtils.synthesize(stack).messages;
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('Test-Condition'),
}),
})
);
expect(messages).not.toContainEqual(
expect.objectContaining({
entry: expect.objectContaining({
data: expect.stringContaining('categorized as an ERROR'),
}),
})
);
expectMessages(messages, {
notContaining: ['Test-Condition', 'categorized as an ERROR'],
});
});
});
});
Loading
Loading