feat(cloudtrail): accept existing S3 bucket

[slipdexic]

Make CloudTrail accept existing S3 bucket to write to.

Fixes #3651.

---

Please read the contribution guidelines and follow the pull-request checklist.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

[mergify]

Pull Request Checklist

• Testing

• Unit test added (prefer to add a new test rather than modify existing tests)
• CLI change? Re-run/add CLI integration tests

• Documentation

• Inline docs: make sure all public APIs are documented (copy & paste from official AWS docs)
• README: update module README
• Design: for significant features, follow the design process

• Title uses the format type(scope): text

• Type: fix, feat, refactor go into CHANGELOG, chore is hidden
• Scope: name of the module without the aws- or cdk- prefix or postfix (e.g. s3 instead of aws-s3-deployment)
• Style: use all lower-case, do not end with a period

• Description

• Rationale: describe rationale of change and approach taken
• Issues: Indicate issues fixed via: fixes #xxx or closes #xxx
• Breaking?: last paragraph: BREAKING CHANGE: <describe what changed + link for details>

• Sensitive Modules (requires 2 PR approvers)

• IAM document library (in @aws-cdk/aws-iam)
• EC2 security groups and ACLs (in @aws-cdk/aws-ec2)
• Grant APIs (if not based on official documentation with a reference)

[mergify]

Codebuild (Continuous Integration) build failed for current commits. Please check log and resolve before PR is merged.

[rix0rrr]

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.

[slipdexic]

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

[slipdexic]

@rix0rrr I have applied the change as suggested.

[rix0rrr]

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.

[slipdexic]

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

[rix0rrr]

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
  }
}

[slipdexic]

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.

[rix0rrr]

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

[rix0rrr]

Wrote up an issue here: #3753

Pull request has been modified.

[slipdexic]

@rix0rrr please review again.