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.

Sign up for GitHub

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(cloudtrail): accept existing S3 bucket #3680

Merged
merged 18 commits into from
Aug 22, 2019
Merged

feat(cloudtrail): accept existing S3 bucket #3680

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
be94575
Merge pull request #1 from aws/master
slipdexic Aug 12, 2019
2dc143d
Merge pull request #2 from aws/master
slipdexic Aug 15, 2019
edbf83a
Allow external/existing Bucket to be supplied (#3651)
slipdexic Aug 15, 2019
afef742
added tests, fixed bug (#3651)
slipdexic Aug 15, 2019
f0754ce
Merge branch 'master' into slipdexic/cloudtrail-s3-bucket
slipdexic Aug 16, 2019
59b74f8
Simplied test, in this case we do not care about the actual bucket po…
slipdexic Aug 16, 2019
ee475da
Merge branch 'slipdexic/cloudtrail-s3-bucket' of https://github.com/s…
slipdexic Aug 16, 2019
a59139a
Merge pull request #3 from aws/master
slipdexic Aug 16, 2019
37ee9fd
Merge branch 'master' into slipdexic/cloudtrail-s3-bucket
slipdexic Aug 16, 2019
71724bc
Merge pull request #4 from aws/master
slipdexic Aug 18, 2019
eeb778d
Merge pull request #6 from aws/master
slipdexic Aug 19, 2019
f743f1c
Merge pull request #7 from slipdexic/master
slipdexic Aug 19, 2019
a3e9da7
Merge pull request #9 from aws/master
slipdexic Aug 20, 2019
48ac3fb
Merge pull request #10 from slipdexic/master
slipdexic Aug 20, 2019
06d4876
[chore] updated propery as ruqested in code review,
slipdexic Aug 20, 2019
b3158af
Merge pull request #11 from aws/master
slipdexic Aug 22, 2019
54c5ee9
Merge pull request #12 from slipdexic/master
slipdexic Aug 22, 2019
1ee3d6d
(chore) applied change sugested in code review
slipdexic Aug 22, 2019
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
46 changes: 29 additions & 17 deletions packages/@aws-cdk/aws-cloudtrail/lib/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,12 @@ export interface TrailProps {
* @default - No prefix.
*/
readonly s3KeyPrefix?: string;

/** The Amazon S3 bucket
*
* @default - if not supplied a bucket will be created with all the correct permisions
*/
readonly s3Bucket?: s3.IBucket
slipdexic marked this conversation as resolved.
Show resolved Hide resolved
}

export enum ReadWriteType {
Expand Down Expand Up @@ -122,30 +128,36 @@ export class Trail extends Resource {
*/
public readonly trailSnsTopicArn: string;

private s3bucket: s3.IBucket;
private eventSelectors: EventSelector[] = [];

constructor(scope: Construct, id: string, props: TrailProps = {}) {
super(scope, id, {
physicalName: props.trailName,
});

const s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED});
const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com");

s3bucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [s3bucket.bucketArn],
actions: ['s3:GetBucketAcl'],
principals: [cloudTrailPrincipal],
}));

s3bucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)],
actions: ["s3:PutObject"],
principals: [cloudTrailPrincipal],
conditions: {
StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"}
}
}));
if (props.s3Bucket === undefined) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this can be made simpler by writing something like:

this.s3bucket = props.bucket || new s3.Bucket(...);

Same as we do for roles all over the place.

Copy link
Contributor Author

@slipdexic slipdexic Aug 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This would then cause the policy to be applied to the bucket , the intent was to not apply the policy . As the bucket is pre-defined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rix0rrr I have applied the change as suggested.


this.s3bucket = new s3.Bucket(this, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED});

this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
Copy link
Contributor

Choose a reason for hiding this comment

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

To match the behavior of roles, I think we should add to the resource policy, even if a literal bucket is given.

If that is undesired, an adapter for IBucket which drops calls to addToResourcePolicy() can always be written.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The purpose of this PR is to allow a bucket with pre-defined policies to be passed in.
I work in sectors that only a the security team can create Policies.

One current use cases for this , is a company may use a central logging bucket or send CT logs to a SOC bucket owned by another company , in these two cases, CDK would not have permissions to apply any policies.

If that is undesired, an adapter for IBucket which drops calls to addToResourcePolicy() can always be written.

I would not know where to start

Copy link
Contributor

Choose a reason for hiding this comment

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

I work in sectors that only a the security team can create Policies.

Yep, and we need a way to address that use case across the entire CDK. You'll have noticed this is not the only situation in which the CDK tries to do "too much" for users in locked-down IAM environments.

In the mean time, I don't think we should have inconsistent behavior across the CDK because the feature author happens to work or not work in a locked-down environment.

I would not know where to start

A rough sketch of it would be something like:

class BucketWrapper implements IBucket {
  constructor(private readonly inner: IBucket) { 
  }

  public get bucketArn()  {
    return this.inner.bucketArn;
  } 

  public urlForObject(key?: string) {
    return this.inner.urlForObject(key);
  }
  
  // ...

  public addToResourcePolicy(statement: iam.PolicyStatement) {
    // Intentionally do nothing
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I will implement your suggestion, I was not aware of using the above suggest pattern to override addToResourcePolicy. We need to make everyone aware in document about this way of handling this type of issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree, and we should probably provide a set of these classes out of the box.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wrote up an issue here: #3753

resources: [this.s3bucket.bucketArn],
actions: ['s3:GetBucketAcl'],
principals: [cloudTrailPrincipal],
}));

this.s3bucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [this.s3bucket.arnForObjects(`AWSLogs/${Stack.of(this).account}/*`)],
actions: ["s3:PutObject"],
principals: [cloudTrailPrincipal],
conditions: {
StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"}
}
}));
} else {
this.s3bucket = props.s3Bucket; }

let logGroup: logs.CfnLogGroup | undefined;
let logsRole: iam.IRole | undefined;
Expand Down Expand Up @@ -177,7 +189,7 @@ export class Trail extends Resource {
includeGlobalServiceEvents: props.includeGlobalServiceEvents == null ? true : props.includeGlobalServiceEvents,
trailName: this.physicalName,
kmsKeyId: props.kmsKey && props.kmsKey.keyArn,
s3BucketName: s3bucket.bucketName,
s3BucketName: this.s3bucket.bucketName,
s3KeyPrefix: props.s3KeyPrefix,
cloudWatchLogsLogGroupArn: logGroup && logGroup.attrArn,
cloudWatchLogsRoleArn: logsRole && logsRole.roleArn,
Expand All @@ -192,7 +204,7 @@ export class Trail extends Resource {
});
this.trailSnsTopicArn = trail.attrSnsTopicArn;

const s3BucketPolicy = s3bucket.node.findChild("Policy").node.findChild("Resource") as s3.CfnBucketPolicy;
const s3BucketPolicy = this.s3bucket.node.findChild("Policy").node.findChild("Resource") as s3.CfnBucketPolicy;
trail.node.addDependency(s3BucketPolicy);

// If props.sendToCloudWatchLogs is set to true then the trail needs to depend on the created logsRole
Expand Down
110 changes: 110 additions & 0 deletions packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,110 @@
{
"Resources": {
"Bucket83908E77": {
"Type": "AWS::S3::Bucket",
"UpdateReplacePolicy": "Delete",
"DeletionPolicy": "Delete"
},
"S3486F821D": {
"Type": "AWS::S3::Bucket",
"UpdateReplacePolicy": "Retain",
"DeletionPolicy": "Retain"
},
"S3Policy2E4AA1D6": {
"Type": "AWS::S3::BucketPolicy",
"Properties": {
"Bucket": {
"Ref": "S3486F821D"
},
"PolicyDocument": {
"Statement": [
{
"Action": "s3:GetBucketAcl",
"Effect": "Allow",
"Principal": {
"Service": "cloudtrail.amazonaws.com"
},
"Resource": {
"Fn::GetAtt": [
"S3486F821D",
"Arn"
]
}
},
{
"Action": "s3:PutObject",
"Condition": {
"StringEquals": {
"s3:x-amz-acl": "bucket-owner-full-control"
}
},
"Effect": "Allow",
"Principal": {
"Service": "cloudtrail.amazonaws.com"
},
"Resource": {
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"S3486F821D",
"Arn"
]
},
"/AWSLogs/",
{
"Ref": "AWS::AccountId"
},
"/*"
]
]
}
}
],
"Version": "2012-10-17"
}
}
},
"Trail022F0CF2": {
"Type": "AWS::CloudTrail::Trail",
"Properties": {
"IsLogging": true,
"S3BucketName": {
"Ref": "S3486F821D"
},
"EnableLogFileValidation": true,
"EventSelectors": [
{
"DataResources": [
{
"Type": "AWS::S3::Object",
"Values": [
{
"Fn::Join": [
"",
[
{
"Fn::GetAtt": [
"Bucket83908E77",
"Arn"
]
},
"/"
]
]
}
]
}
]
}
],
"IncludeGlobalServiceEvents": true,
"IsMultiRegionTrail": true
},
"DependsOn": [
"S3Policy2E4AA1D6"
]
}
}
}
37 changes: 37 additions & 0 deletions packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail-supplied-bucket.lit.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import iam = require('@aws-cdk/aws-iam');
import s3 = require('@aws-cdk/aws-s3');
import cdk = require('@aws-cdk/core');
import { Stack } from '@aws-cdk/core';

import cloudtrail = require('../lib');

const app = new cdk.App();
const stack = new cdk.Stack(app, 'integ-cloudtrail');

const bucket = new s3.Bucket(stack, 'Bucket', { removalPolicy: cdk.RemovalPolicy.DESTROY });

// using exctecy the same code as inside the cloudtrail class to produce the supplied bucket and policy
const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com");

const Trailbucket = new s3.Bucket(stack, 'S3', {encryption: s3.BucketEncryption.UNENCRYPTED});

Trailbucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [Trailbucket.bucketArn],
actions: ['s3:GetBucketAcl'],
principals: [cloudTrailPrincipal],
}));

Trailbucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [Trailbucket.arnForObjects(`AWSLogs/${Stack.of(stack).account}/*`)],
actions: ["s3:PutObject"],
principals: [cloudTrailPrincipal],
conditions: {
StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"}
}
}));

const trail = new cloudtrail.Trail(stack, 'Trail', {s3Bucket: Trailbucket});

trail.addS3EventSelector([bucket.arnForObjects('')]);

app.synth();
29 changes: 29 additions & 0 deletions packages/@aws-cdk/aws-cloudtrail/test/test.cloudtrail.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { expect, haveResource, not, SynthUtils } from '@aws-cdk/assert';
import iam = require('@aws-cdk/aws-iam');
import { RetentionDays } from '@aws-cdk/aws-logs';
import s3 = require('@aws-cdk/aws-s3');
import { Stack } from '@aws-cdk/core';
import { Test } from 'nodeunit';
import { ReadWriteType, Trail } from '../lib';
Expand Down Expand Up @@ -67,6 +69,33 @@ export = {
test.deepEqual(trail.DependsOn, ['MyAmazingCloudTrailS3Policy39C120B0']);
test.done();
},
'with s3bucket'(test: Test) {
const stack = getTestStack();
const Trailbucket = new s3.Bucket(stack, 'S3');
const cloudTrailPrincipal = new iam.ServicePrincipal("cloudtrail.amazonaws.com");
Trailbucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [Trailbucket.bucketArn],
actions: ['s3:GetBucketAcl'],
principals: [cloudTrailPrincipal],
}));

Trailbucket.addToResourcePolicy(new iam.PolicyStatement({
resources: [Trailbucket.arnForObjects(`AWSLogs/${Stack.of(stack).account}/*`)],
actions: ["s3:PutObject"],
principals: [cloudTrailPrincipal],
conditions: {
StringEquals: {'s3:x-amz-acl': "bucket-owner-full-control"}
}
}));

new Trail(stack, 'Trail', {s3Bucket: Trailbucket});

expect(stack).to(haveResource("AWS::CloudTrail::Trail"));
expect(stack).to(haveResource("AWS::S3::Bucket"));
expect(stack).to(haveResource("AWS::S3::BucketPolicy"));
expect(stack).to(not(haveResource("AWS::Logs::LogGroup")));
test.done();
},
'with cloud watch logs': {
'enabled'(test: Test) {
const stack = getTestStack();
Expand Down