Skip to content

Commit

Permalink
feat: add per-rule granular error suppression (#1826)
Browse files Browse the repository at this point in the history 
Fixes #1818

This does slightly change some behaviours, specifically by suppressing errors for suppressed rules. I'm torn as to whether this is a sufficiently large behavioural change to cause an issue, though as a rule I feel it is a better behaviour and cant think of a scenario where someone would have been relying on a suppressed rule throwing an error.
  • Loading branch information
@cgatt
cgatt authored Nov 7, 2024
1 parent 6c566e2 commit 86917b3
Show file tree
Hide file tree
Showing 7 changed files with 421 additions and 341 deletions.
63 changes: 63 additions & 0 deletions README.md
View file
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
View file
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
View file
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
View file
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

0 comments on commit 86917b3

Please sign in to comment.