Enable versioning and server access logging for ArtifactBucket

[knassef]

Description of changes: For security reasons;

• Created new bucket AccessLogsBucket to store s3 server access logs of ArtifactBucket
• Enabled versioning in both buckets (ArtifactBucket and AccessLogsBucket)
• Set LifecycleConfiguration to maximum value

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

[rjlohan]

Thanks for this contribution, this is a nice improvement. 👍

[benbridts]

@knassef I promise I didn't set out to nitpick every line of this. There's nothing wrong with the template in general, but I have a tendency to keep asking questions

[knassef]

@ikben ah no worries at all, we are having a reasonable conversation here, trying to understand pros and cons of different approaches. so all good :)

[knassef]

@jotompki yes, my team is using this approach with all S3 bucket as we have a hard security requirement to log all s3 buckets (with no exceptions). AFAIK CloudTrail also dumps logs into an s3 bucket so we will face the same issue with that approach as well. I have addressed the other comments and now DestinationBucketName is an optional parameter and logging will only be enabled if it was passed. Do you think we can come to an agreement on this?

[knassef]

would appreciate any response regarding my last comment :)

[johnttompkins]

so if we create the bucket, we set a bucket name of "" that logs to ""? That will throw some validation errors on the CloudFormation end.

I'm all for adding an access logs bucket just don't think it is necessary for the logs bucket to log to itself.

[PatMyron]

AccessLogsBucket | CREATE_FAILED | Property BucketName cannot be empty if specified.

[knassef]

please check #396 (comment)

[PatMyron]

This fails to successfully create any CloudFormation stack regardless if the DestinationBucketName parameter is specified or not. Even after negating the CreateLogBucket condition, this template still fails to successfully create a CloudFormation stack if the DestinationBucketName parameter is not specified

@knassef please test not only that these CloudFormation stacks successfully deploy but also the desired behavior of both specifying DestinationBucketName and not specifying DestinationBucketName

This bucket should also enforce secure transport: #389

[PatMyron]

Suggested change

	CreateLogBucket: !Equals [!Ref DestinationBucketName, ""]
	CreateLogBucket: !Not [!Equals [!Ref DestinationBucketName, ""]]

[knassef]

please check #396 (comment)

[PatMyron]

ArtifactBucket | CREATE_FAILED | The target bucket for logging does not exist (Service: Amazon S3; Status Code: 400; Error Code: InvalidTargetBucketForLogging)

[knassef]

please check #396 (comment)

[PatMyron]

AccessLogsBucket | CREATE_FAILED | Property BucketName cannot be empty if specified.

[PatMyron]

Suggested change

	DestinationBucketName:
	Description: Set this if you want to write access logs to a bucket you manage
	Type: String
	Default: ""
	DestinationBucketName:
	Description: Set this if you want to write access logs to a bucket you manage
	Type: String
	Default: ""
	AllowedPattern: "^[a-z0-9.-]*$"

We can't be as strict as the AWS::S3::Bucket.BucketName AllowedPattern because of the empty default, but we can at least constrain with some AllowedPattern to prevent invalid S3 bucket name characters

[benbridts]

Quick AllowedPattern note:^[a-z0-9][a-z0-9.-]*[a-z0-9]$|^$ should catch both cases

[knassef]

please check #396 (comment)

[knassef]

To unblock this, I have removed the self logging of the logs bucket to keep things simple for this PR as I see its not the general use case and I will try to think of another way to keep it optional without complicating the template.

[rjlohan]

Thanks for taking the effort on this, especially with all the rework you did!

Stale

[knassef]

Thank you as well everyone! 🙏