Skip to content

Commit

Permalink
feat: S3BucketSSLRequestsOnly additionally runs on bucket policies (#731
Browse files Browse the repository at this point in the history 
)
  • Loading branch information
@dontirun
dontirun authored Mar 24, 2022
1 parent c4c37c7 commit 5ba19fa
Show file tree
Hide file tree
Showing 8 changed files with 131 additions and 18 deletions.
10 changes: 5 additions & 5 deletions RULES.md
View file

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion src/packs/aws-solutions.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ export class AwsSolutionsChecks extends NagPack {
});
this.applyRule({
ruleSuffixOverride: 'S10',
info: 'The S3 Bucket does not require requests to use SSL.',
info: 'The S3 Bucket or bucket policy does not require requests to use SSL.',
explanation:
'You can use HTTPS (TLS) to help prevent potential attackers from eavesdropping on or manipulating network traffic using person-in-the-middle or similar attacks. You should allow only encrypted connections over HTTPS (TLS) using the aws:SecureTransport condition on Amazon S3 bucket policies.',
level: NagMessageLevel.ERROR,
Expand Down
2 changes: 1 addition & 1 deletion src/packs/hipaa-security.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -926,7 +926,7 @@ export class HIPAASecurityChecks extends NagPack {
node: node,
});
this.applyRule({
info: 'The S3 Bucket does not require requests to use SSL - (Control IDs: 164.312(a)(2)(iv), 164.312(c)(2), 164.312(e)(1), 164.312(e)(2)(i), 164.312(e)(2)(ii)).',
info: 'The S3 Bucket or bucket policy does not require requests to use SSL - (Control IDs: 164.312(a)(2)(iv), 164.312(c)(2), 164.312(e)(1), 164.312(e)(2)(i), 164.312(e)(2)(ii)).',
explanation:
'To help protect data in transit, ensure that your Amazon Simple Storage Service (Amazon S3) buckets require requests to use Secure Socket Layer (SSL). Because sensitive data can exist, enable encryption in transit to help protect that data.',
level: NagMessageLevel.ERROR,
Expand Down
2 changes: 1 addition & 1 deletion src/packs/nist-800-53-r4.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -775,7 +775,7 @@ export class NIST80053R4Checks extends NagPack {
node: node,
});
this.applyRule({
info: 'The S3 Bucket does not require requests to use SSL - (Control IDs: AC-17(2), SC-7, SC-8, SC-8(1), SC-13).',
info: 'The S3 Bucket or bucket policy does not require requests to use SSL - (Control IDs: AC-17(2), SC-7, SC-8, SC-8(1), SC-13).',
explanation:
'To help protect data in transit, ensure that your Amazon Simple Storage Service (Amazon S3) buckets require requests to use Secure Socket Layer (SSL). Because sensitive data can exist, enable encryption in transit to help protect that data.',
level: NagMessageLevel.ERROR,
Expand Down
2 changes: 1 addition & 1 deletion src/packs/nist-800-53-r5.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ export class NIST80053R5Checks extends NagPack {
node: node,
});
this.applyRule({
info: 'The S3 Bucket does not require requests to use SSL - (Control IDs: AC-4, AC-4(22), AC-17(2), AC-24(1), AU-9(3), CA-9b, CM-6a, CM-9b, IA-5(1)(c), PM-11b, PM-17b, SC-7(4)(b), SC-7(4)(g), SC-8, SC-8(1), SC-8(2), SC-8(3), SC-8(4), SC-8(5), SC-13a, SC-16(1), SC-23, SI-1a.2, SI-1a.2, SI-1c.2).',
info: 'The S3 Bucket or bucket policy does not require requests to use SSL - (Control IDs: AC-4, AC-4(22), AC-17(2), AC-24(1), AU-9(3), CA-9b, CM-6a, CM-9b, IA-5(1)(c), PM-11b, PM-17b, SC-7(4)(b), SC-7(4)(g), SC-8, SC-8(1), SC-8(2), SC-8(3), SC-8(4), SC-8(5), SC-13a, SC-16(1), SC-23, SI-1a.2, SI-1a.2, SI-1c.2).',
explanation:
'To help protect data in transit, ensure that your Amazon Simple Storage Service (Amazon S3) buckets require requests to use Secure Socket Layer (SSL). Because sensitive data can exist, enable encryption in transit to help protect that data.',
level: NagMessageLevel.ERROR,
Expand Down
2 changes: 1 addition & 1 deletion src/packs/pci-dss-321.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,7 @@ export class PCIDSS321Checks extends NagPack {
node: node,
});
this.applyRule({
info: 'The S3 Bucket does not require requests to use SSL - (Control IDs: 2.2, 4.1, 8.2.1).',
info: 'The S3 Bucket or bucket policy does not require requests to use SSL - (Control IDs: 2.2, 4.1, 8.2.1).',
explanation:
'To help protect data in transit, ensure that your Amazon Simple Storage Service (Amazon S3) buckets require requests to use Secure Socket Layer (SSL). Because sensitive data can exist, enable encryption in transit to help protect that data.',
level: NagMessageLevel.ERROR,
Expand Down
37 changes: 30 additions & 7 deletions src/rules/s3/S3BucketSSLRequestsOnly.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ import { CfnResource, Stack } from '@aws-cdk/core';
import { NagRuleCompliance, NagRules } from '../../nag-rules';

/**
* S3 Buckets require requests to use SSL
* S3 Buckets and bucket policies require requests to use SSL
* @param node the CfnResource to check
*/

Expand All @@ -23,7 +23,10 @@ export default Object.defineProperty(
let found = false;
for (const child of Stack.of(node).node.findAll()) {
if (child instanceof CfnBucketPolicy) {
if (isMatchingCompliantPolicy(child, bucketLogicalId, bucketName)) {
if (
isMatchingPolicy(child, bucketLogicalId, bucketName) &&
isCompliantPolicy(child, bucketLogicalId, bucketName)
) {
found = true;
break;
}
Expand All @@ -33,6 +36,14 @@ export default Object.defineProperty(
return NagRuleCompliance.NON_COMPLIANT;
}
return NagRuleCompliance.COMPLIANT;
} else if (node instanceof CfnBucketPolicy) {
const bucketLogicalId = NagRules.resolveResourceFromInstrinsic(
node,
node.bucket
);
return isCompliantPolicy(node, bucketLogicalId, node.bucket)
? NagRuleCompliance.COMPLIANT
: NagRuleCompliance.NON_COMPLIANT;
} else {
return NagRuleCompliance.NOT_APPLICABLE;
}
Expand All @@ -41,22 +52,34 @@ export default Object.defineProperty(
{ value: parse(__filename).name }
);

/**
* Helper function to check whether the Bucket Policy belongs to the given bucket
* @param node The CfnBucketPolicy to check.
* @param bucketLogicalId The Cfn Logical ID of the bucket.
* @param bucketName The name of the bucket.
* @returns Whether the CfnBucketPolicy belongs to th egiven bucket.
*/
function isMatchingPolicy(
node: CfnBucketPolicy,
bucketLogicalId: string,
bucketName: string | undefined
): boolean {
const bucket = NagRules.resolveResourceFromInstrinsic(node, node.bucket);
return bucket === bucketLogicalId || bucket === bucketName;
}

/**
* Helper function to check whether the Bucket Policy requires SSL on the given bucket.
* @param node The CfnBucketPolicy to check.
* @param bucketLogicalId The Cfn Logical ID of the bucket.
* @param bucketName The name of the bucket.
* @returns Whether the CfnBucketPolicy requires SSL on the given bucket.
*/
function isMatchingCompliantPolicy(
function isCompliantPolicy(
node: CfnBucketPolicy,
bucketLogicalId: string,
bucketName: string | undefined
): boolean {
const bucket = NagRules.resolveResourceFromInstrinsic(node, node.bucket);
if (bucket !== bucketLogicalId && bucket !== bucketName) {
return false;
}
const resolvedPolicyDocument = Stack.of(node).resolve(node.policyDocument);
for (const statement of resolvedPolicyDocument.Statement) {
const resolvedStatement = Stack.of(node).resolve(statement);
Expand Down
92 changes: 91 additions & 1 deletion test/rules/S3.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import {
Bucket,
BucketAccessControl,
BucketEncryption,
BucketPolicy,
CfnBucket,
CfnBucketPolicy,
} from '@aws-cdk/aws-s3';
Expand Down Expand Up @@ -277,7 +278,7 @@ describe('Amazon Simple Storage Service (S3)', () => {
});
});

describe('S3BucketSSLRequestsOnly: S3 Buckets require requests to use SSL', () => {
describe('S3BucketSSLRequestsOnly: S3 Buckets and bucket policies require requests to use SSL', () => {
const ruleId = 'S3BucketSSLRequestsOnly';
test('Noncompliance 1', () => {
new CfnBucket(stack, 'rBucket');
Expand Down Expand Up @@ -377,6 +378,73 @@ describe('Amazon Simple Storage Service (S3)', () => {
});
validateStack(stack, ruleId, TestType.NON_COMPLIANCE);
});
test('Noncompliance 7', () => {
new CfnBucketPolicy(stack, 'rPolicy', {
bucket: 'foo',
policyDocument: new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['nots3:*'],
effect: Effect.DENY,
principals: [new AnyPrincipal()],
conditions: { Bool: { 'aws:SecureTransport': 'false' } },
resources: ['arn:aws:s3:::foo', 'arn:aws:s3:::foo/*'],
}),
],
}),
});
validateStack(stack, ruleId, TestType.NON_COMPLIANCE);
});
test('Noncompliance 8', () => {
new CfnBucketPolicy(stack, 'rPolicy', {
bucket: 'foo',
policyDocument: new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['s3:*'],
effect: Effect.DENY,
principals: [new AnyPrincipal()],
conditions: { Bool: { 'aws:SecureTransport': 'false' } },
resources: ['arn:aws:s3:::foo', 'arn:aws:s3:::foobar/*'],
}),
],
}),
});
validateStack(stack, ruleId, TestType.NON_COMPLIANCE);
});
test('Noncompliance 9', () => {
const bucket = new Bucket(stack, 'rBucket', {
enforceSSL: true,
});
const newPolicy = new BucketPolicy(stack, 'rBucketPolicy2', {
bucket: bucket,
});
newPolicy.document.addStatements(
new PolicyStatement({
effect: Effect.DENY,
principals: [new AnyPrincipal()],
actions: ['s3:PutObject'],
resources: [bucket.arnForObjects('*')],
conditions: {
StringNotLikeIfExists: {
's3:x-amz-server-side-encryption-aws-kms-key-id': 'foo',
},
},
}),
new PolicyStatement({
effect: Effect.DENY,
principals: [new AnyPrincipal()],
actions: ['s3:PutObject'],
resources: [bucket.arnForObjects('*')],
conditions: {
StringEquals: {
's3:x-amz-server-side-encryption': 'AES256',
},
},
})
);
validateStack(stack, ruleId, TestType.NON_COMPLIANCE);
});
test('Compliance', () => {
const compliantBucket = new Bucket(stack, 'rBucket');
new CfnBucketPolicy(stack, 'rPolicy', {
Expand Down Expand Up @@ -430,6 +498,28 @@ describe('Amazon Simple Storage Service (S3)', () => {
],
}),
});
const importedBucket = Bucket.fromBucketName(
stack,
'rImportedBucket',
'baz'
);
new CfnBucketPolicy(stack, 'rPolicy4', {
bucket: importedBucket.bucketName,
policyDocument: new PolicyDocument({
statements: [
new PolicyStatement({
actions: ['s3:*'],
effect: Effect.DENY,
principals: [new AnyPrincipal()],
conditions: { Bool: { 'aws:SecureTransport': 'false' } },
resources: [
importedBucket.bucketArn,
importedBucket.arnForObjects('*'),
],
}),
],
}),
});
validateStack(stack, ruleId, TestType.COMPLIANCE);
});
});
Expand Down

0 comments on commit 5ba19fa

Please sign in to comment.