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

fix: Check standard logs in AwsSolutions-CFR3 #1877

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
fix: Check standard logs in AwsSolutions-CFR3
  • Loading branch information
georeeve committed Jan 14, 2025
commit 0e4648ce5dd0f47bd91e3e52e0bd49af57f73925
2 changes: 1 addition & 1 deletion package.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 30 additions & 1 deletion src/rules/cloudfront/CloudFrontDistributionAccessLogging.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,11 @@ import {
CfnDistribution,
CfnStreamingDistribution,
} from 'aws-cdk-lib/aws-cloudfront';
import { CfnDeliverySource } from 'aws-cdk-lib/aws-logs';
import { NagRuleCompliance, NagRules } from '../../nag-rules';
import { flattenCfnReference } from '../../utils/flatten-cfn-reference';

const pendingDistributions: CfnDistribution[] = [];

/**
* CloudFront distributions have access logging enabled
Expand All @@ -21,7 +25,10 @@ export default Object.defineProperty(
node.distributionConfig
);
if (distributionConfig.logging == undefined) {
return NagRuleCompliance.NON_COMPLIANT;
pendingDistributions.push(node);
// TODO: If there are no CfnDeliverySource defined, then mark as NON_COMPLIANT,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dontirun I'm wondering if you know of a way to check this? I've tried to use Template.fromStack(Stack.of(node)) and then find the resources that way, but that seems to really slow the tests down. Is there a simpler solution that I'm missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there might actually be a better solution here. Can we mark all of the CfnDistribution's as NOT_APPLICABLE, add them to a Map and then at the end of the Stack we evaluate and retrospectively mark them as COMPLIANT or NOT_COMPLIANT dependent on whether we've seen a CfnDeliverySource for them? Is there a method of doing that?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or maybe we just traverse the entire Stack when we see a CfnDistribution to check if any CfnDeliverySource's are present, similar to what we do in WAFv2LoggingEnabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've just pushed a commit to match the WAF rule, let me know if you think that's the right solution.

// otherwise mark as NOT_APPLICABLE to continue in the next step
return NagRuleCompliance.NOT_APPLICABLE;
}
return NagRuleCompliance.COMPLIANT;
} else if (node instanceof CfnStreamingDistribution) {
Expand All @@ -37,6 +44,28 @@ export default Object.defineProperty(
return NagRuleCompliance.NON_COMPLIANT;
}
return NagRuleCompliance.COMPLIANT;
} else if (node instanceof CfnDeliverySource) {
return pendingDistributions.every((distributionNode) => {
const distributionArn = flattenCfnReference(
Stack.of(distributionNode).resolve(
Stack.of(distributionNode).formatArn({
service: 'cloudfront',
region: '',
resource: 'distribution',
resourceName: distributionNode.attrId,
})
)
).replace('.Id', '');
const deliverySourceArn = flattenCfnReference(
Stack.of(node).resolve(node.resourceArn)
);
const logType = Stack.of(node).resolve(node.logType);
return (
distributionArn === deliverySourceArn && logType === 'ACCESS_LOGS'
);
})
? NagRuleCompliance.COMPLIANT
: NagRuleCompliance.NON_COMPLIANT;
} else {
return NagRuleCompliance.NOT_APPLICABLE;
}
Expand Down
45 changes: 45 additions & 0 deletions test/rules/CloudFront.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,12 @@ import {
S3Origin,
S3BucketOrigin,
} from 'aws-cdk-lib/aws-cloudfront-origins';
import {
CfnDelivery,
CfnDeliveryDestination,
CfnDeliverySource,
LogGroup,
} from 'aws-cdk-lib/aws-logs';
import { Bucket } from 'aws-cdk-lib/aws-s3';
import { CfnWebACL } from 'aws-cdk-lib/aws-wafv2';
import { Aspects, Stack } from 'aws-cdk-lib/core';
Expand Down Expand Up @@ -108,6 +114,45 @@ describe('Amazon CloudFront', () => {
});
validateStack(stack, ruleId, TestType.COMPLIANCE);
});
test.only('Compliance 2', () => {
const distribution = new Distribution(stack, 'Distribution', {
defaultBehavior: {
origin: new S3Origin(new Bucket(stack, 'OriginBucket')),
},
});

const distributionDeliverySource = new CfnDeliverySource(
stack,
'DistributionDeliverySource',
{
name: 'distribution-logs-source',
logType: 'ACCESS_LOGS',
resourceArn: Stack.of(stack).formatArn({
service: 'cloudfront',
region: '',
resource: 'distribution',
resourceName: distribution.distributionId,
}),
}
);

const distributionDeliveryDestination = new CfnDeliveryDestination(
stack,
'DistributionDeliveryDestination',
{
name: 'distribution-logs-destination',
destinationResourceArn: new LogGroup(stack, 'DistributionLogGroup')
.logGroupArn,
outputFormat: 'json',
}
);

new CfnDelivery(stack, 'DistributionDelivery', {
deliverySourceName: distributionDeliverySource.name,
deliveryDestinationArn: distributionDeliveryDestination.attrArn,
}).node.addDependency(distributionDeliverySource);
validateStack(stack, ruleId, TestType.COMPLIANCE);
});
});

describe('CloudFrontDistributionGeoRestrictions: CloudFront distributions may require Geo restrictions', () => {
Expand Down
48 changes: 24 additions & 24 deletions yarn.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading