aws-cloudtrail: can't create a bucket only trail

jonny-rimek commented 3 years ago

I want to create a trail that doesn't log management events, but only s3 events. Because the management events are logged inside my main account.

Reproduction Steps

        const trail = new cloudtrail.Trail(this, 't', {
            managementEvents: ReadWriteType.NONE,
            sendToCloudWatchLogs: true,
        });

        trail.addS3EventSelector([{
            bucket: props.uploadBucket,
            objectPrefix: "upload",
        }], {
            includeManagementEvents: false,
            readWriteType: ReadWriteType.WRITE_ONLY,
        });

error message:

Invalid combination of parameters in EventSelectors. When includeManagementEvents is false, DataResources must not be empty (Service: AWSCloudTrail; Status Code: 400; Error Code: InvalidEve
ntSelectorsException; Request ID: 8d454eeb-5e33-43d5-be7a-8bbc63279541; Proxy: null)

What did you expect to happen?

Be able to create a trail that only logs bucket events, I created one via the console

What actually happened?

See error message

Environment

**CDK CLI Version : 1.95.1 (build ed2bbe6)

This is :bug: Bug Report

varnenec commented 3 years ago

I'm affected by that as well. I thought the alternative would be to use CfnTrail, despite the drawback of extra maintenance costs due to there being more code to write. But even that fails with the same error as the one reported by the OP.

varnenec commented 3 years ago

Correction: I was able to us CfnTrail to create a trail that only logs data events, and no management events. It would be ideal if Trail can be fixed, so that it's as simple as using the managementEvents property in TrailProps.

The mistake I was making was in essentially setting includeManagementEvents to false in its own EventSelector, and that's also what the implementation of Trail in CDK is doing, as far as I can tell from reading the code. Once I made sure that the same EventSelector had both the management events flag, and the DataResources (for S3 and Lambda in my case), then it worked.

jonny-rimek commented 3 years ago

@varnenec can you share you implementation please? For now I activated s3 access log feature, but I didn't check yet what exactly the difference is to cloudtrail and would still implement this.

NetaNir commented 3 years ago

The error you pasted seem to be coming from CloudFormation? If so Im not sure why would using the L1 would help in this case. What does the CDK generated template looks like? (the one created by the L2)

In the meantime, if it does work on the L1, you can use escape hatch with the L2

varnenec commented 3 years ago

I thought I had posted my reply, but apparently I didn't :( Reposting it again. My example is a trail that only records S3 and Lambda events, and no management events.

Caveats:

Trail sets some CloudTrail properties to true by default, whereas CfnTrail does not. You have to set them yourself.
Trail modifies the resource policy of your chosen CloudTrail logging S3 bucket. CfnTrail does not, so you have to do it yourself.
Trail has a logical resource name in the CloudFormation template that contains a hash at the end, like MyDataEventOnlyTrail33B07A. CfnTrail produces a name without the hash. Where this matters is that you can't, unfortunately, treat a change from Trail to CfnTrail as an update operation - your old trail is deleted, and a new one is created.
The EventSelector code to include all S3 events and all Lambda events was taken verbatim from CDK's own implementation of Trail. I'm sure it could've been arrived it by reading the AWS docs, but it's not what I did.

Code:

        // CfnTrail vs Trail: explicitly give CloudTrail permissions to write to the logging bucket
        cloudTrailBucket.addToResourcePolicy(
                new PolicyStatement(PolicyStatementProps.builder()
                        .effect(Effect.ALLOW)
                        .principals(ImmutableList.of(new ServicePrincipal("cloudtrail.amazonaws.com")))
                        .actions(ImmutableList.of("s3:PutObject"))
                        .resources(ImmutableList.of(String.format("%s/AWSLogs/%s/*", cloudTrailBucket.getBucketArn(), this.getAccount())))
                        .conditions(ImmutableMap.of("StringEquals", ImmutableMap.of("s3:x-amz-acl", "bucket-owner-full-control")))
                        .build())
        );

        cloudTrailBucket.addToResourcePolicy(
                new PolicyStatement(PolicyStatementProps.builder()
                        .effect(Effect.ALLOW)
                        .principals(ImmutableList.of(new ServicePrincipal("cloudtrail.amazonaws.com")))
                        .actions(ImmutableList.of("s3:GetBucketAcl"))
                        .resources(ImmutableList.of(cloudTrailBucket.getBucketArn()))
                        .build())
        );

        CfnTrailProps props = CfnTrailProps.builder()
                .trailName("my-data-event-cloud-trail")
                .s3BucketName(cloudTrailBucket.getBucketName())
                .isLogging(true)
                .isMultiRegionTrail(true)
                .includeGlobalServiceEvents(true)
                .enableLogFileValidation(true)
                .eventSelectors(ImmutableList.of(
                        new CfnTrail.EventSelectorProperty.Builder()
                                .includeManagementEvents(false)
                                .dataResources(ImmutableList.of(
                                        CfnTrail.DataResourceProperty.builder()
                                                .type("AWS::S3::Object")
                                                .values(ImmutableList.of(String.format("arn:%s:s3:::", this.getPartition())))
                                                .build(),

                                        CfnTrail.DataResourceProperty.builder()
                                                .type("AWS::Lambda::Function")
                                                .values(ImmutableList.of(String.format("arn:%s:lambda", this.getPartition())))
                                                .build()
                        )).build()
                ))
                .build();

        new CfnTrail(this, "MyDataEventCloudTrail", props);

varnenec commented 3 years ago

The resulting trail looks like:

    "MyDataEventCloudTrail": {
      "Type": "AWS::CloudTrail::Trail",
      "Properties": {
        "IsLogging": true,
        "S3BucketName": {
          "Ref": "CloudtrailLoggingBucket8722A167"
        },
        "EnableLogFileValidation": true,
        "EventSelectors": [
          {
            "DataResources": [
              {
                "Type": "AWS::S3::Object",
                "Values": [
                  {
                    "Fn::Join": [
                      "",
                      [
                        "arn:",
                        {
                          "Ref": "AWS::Partition"
                        },
                        ":s3:::"
                      ]
                    ]
                  }
                ]
              },
              {
                "Type": "AWS::Lambda::Function",
                "Values": [
                  {
                    "Fn::Join": [
                      "",
                      [
                        "arn:",
                        {
                          "Ref": "AWS::Partition"
                        },
                        ":lambda"
                      ]
                    ]
                  }
                ]
              }
            ],
            "IncludeManagementEvents": false
          }
        ],
        "IncludeGlobalServiceEvents": true,
        "IsMultiRegionTrail": true,
        "TrailName": "my-data-event-cloud-trail"
      }
    }

NetaNir commented 3 years ago

And you want IsLogging to be false? If so use escape hatch

varnenec commented 3 years ago

No, IsLogging is not the problem. The problem is how IncludeManagementEvents is set to false by CDK.

The equivalent code using TrailProps would look like:

TrailProps props = TrailProps.builder()
   .trailName("my-data-event-cloud-trail")
   .bucket(cloudTrailBucket)
   .managementEvents(ReadWriteType.NONE) // <--- problematic piece
   .isMultiRegionTrail(true)
   .build();

Trail trail = new Trail(this, "MyDataEventCloudTrail", props);
trail.logAllS3DataEvents();
trail.logAllLambdaEvents();

The problem is that the CDK logic in Trail creates a separate EventSelector for just the IncludeManagementEvents flag, which is incorrect per CloudFormation - an event selector with that flag must also have DataSelectors. My code works because both the data selectors, and the flag, and part of the same EventSelector.

NetaNir commented 3 years ago

Unless Im missing something (sorry about that), you can use escape hatch to set IncludeManagementEvents to true?

varnenec commented 3 years ago

The flag is part of an EventSelector, and Trail generates an array of those objects. One of the elements in the array is an EventSelector which only contains the IncludeManagementEvents flag by itself. This is what CloudFormation rejects as invalid input - an EventSelector with this flag set must also contain some DataSelectors. Fixing that via the escape hatch involves essentially overwriting the EventSelector array by using the laborious logic I showed above with CfnTrailProps. This is problematic for two reasons:

There is, in fact, a bug in Trail. The code snippet I provided with Trail and TrailProps should work as written, but it produces un-deployable CloudFormation.
The workaround for the bug requires creating the EventSelector by hand, which is verbose and error-prone.

NetaNir commented 3 years ago

got it. Thanks

ueda00 commented 3 years ago

@varnenec

This works, try.

const trail = new Trail(this, "trail");
trail.addS3EventSelector([{
      bucket: myBucket,
      objectPrefix: myPrefix,
}], {
      includeManagementEvents: false,
      readWriteType: ReadWriteType.WRITE_ONLY,
});

github-actions[bot] commented 2 years ago

This issue has not received any attention in 1 year. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

varnenec commented 2 years ago

Has this been addressed in CDK V2 by chance? I haven't yet had the opportunity to try it out.

aws / aws-cdk