aws / copilot-cli

The AWS Copilot CLI is a tool for developers to build, release and operate production ready containerized applications on AWS App Runner or Amazon ECS on AWS Fargate.
https://aws.github.io/copilot-cli/
Apache License 2.0
3.52k stars 414 forks source link

`PipelineBuiltArtifactBucket` does not enforce KMS key and is not customisable #5328

Closed FlorianSW closed 1 year ago

FlorianSW commented 1 year ago

Hi 👋

I'm currently evaluating AWS Copilot and run into an issue: The PipelineBuiltArtifactBucket S3 bucket is created by the initial StackSet by copilot and is therefore not customisable as far as I can see. Unfortunately, the bucket also uses SSE-S3 as the default server-side encryption. This encryption method violates one of the security regulations of the context I evaluate copilot in. This regulation requires S3 objects to be encrypted by a customer-owned KMS key.

Fortunately, such a KMS key is already created with appropriate permissions in the same stack set. It's intended use is, as far as I know, to be used by CodePipeline when it puts objects into the bucket. My assumption therefore is, that it would be perfectly fine to tweak the S3 bucket in a way that the usage of KMS to encrypt objects is enforced? I assume that this would also be in the interest of the original idea of that bucket's use case, as the KMS key is already used for pipeline generated objects (the primary use case of this bucket).

Would it be possible to change the PipelineBuiltArtifactBucket to enforce KMS key encryption or at least make this behaviour configurable if it is not possible to change the default?

Lou1415926 commented 1 year ago

Hi @FlorianSW ! Thank you very much for bringing this up, for the deep dive you've done into the codebase, AND for opening the PR ❤️ !

Fortunately, such a KMS key is already created with appropriate permissions in the same stack set. It's intended use is, as far as I know, to be used by CodePipeline when it puts objects into the bucket.

This is correct. The KMS key is to encrypt the artifacts generated by the pipeline stages when they are uploaded to the s3 bucket.

Based on this, I agree that we can change the default server-side encryption from s3-managed-key to the dedicated KMS key. This would correspond to the ⬇️ changes in your PR:

      BucketEncryption:
        ServerSideEncryptionConfiguration:
          - ServerSideEncryptionByDefault:
              SSEAlgorithm: 'aws:kms'
              KMSMasterKeyID: !GetAtt KMSKey.Arn
            BucketKeyEnabled: true

However I do have a question regarding the permission changes in the PR ⬇️

          - Action: s3:PutObject
            Effect: Deny
            Resource: !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}/*
            Principal: '*'
            Condition:
              StringNotEquals:
                s3:x-amz-server-side-encryption: 'aws:kms'
          - Action: s3:PutObject
            Effect: Deny
            Resource: !Sub arn:${AWS::Partition}:s3:::${PipelineBuiltArtifactBucket}/*
            Principal: '*'
            Condition:
              Null:
                s3:x-amz-server-side-encryption: true

These statements deny any uploads that are not explicitly requesting for server-side encryption by the said KMS key.

To elaborate, with the "deny" statements, ⬇️ fails because of "AccessDenied"

    resp, err := s.s3Manager.Upload( &s3manager.UploadInput{
        Body:        buf,
        Bucket:      aws.String(bucket),
        Key:         aws.String(key),
        ACL:         aws.String(s3.ObjectCannedACLBucketOwnerFullControl),
        ContentType: defaultContentTypeFromExt(key),
                // NOTE: There is no server side encryption option in this request. The object will be encrypted using the bucket's default server-side encryption.
    })

and ⬇️ should succeed:

    resp, err := s.s3Manager.Upload( &s3manager.UploadInput{
        Body:        buf,
        Bucket:      aws.String(bucket),
        Key:         aws.String(key),
        ACL:         aws.String(s3.ObjectCannedACLBucketOwnerFullControl),
        ContentType: defaultContentTypeFromExt(key),
        SSEKMSKeyId:          aws.String("arn:aws:kms:...."), // NOTE: the key ARN.
        ServerSideEncryption: aws.String("aws:kms"),  // NOTE: Explicitly for server-side encryption using the said KMS key.
    })

As far as I understand to this point, any uploads (e.g. via. PutObject calls), if not specified with a server-side encryption option, will use the default encryption method specified on the bucket - in this case, the KMS key. This ensures that any uploads are indeed encrypted.

Therefore, it seems like adding the deny statements only prevents any uploads that are explicitly asking for server-side encryption that is not by the said KMS key. Is this the intention?

It is entirely possible that I am missing something here, or from the doc. Feel free to point it out, if there is any!

FlorianSW commented 1 year ago

Hi @Lou1415926,

thanks for your response and for pointing out the permission issue. Indeed you're right: I have overreached the goal with these policy statements 🙈

I'll revisit the changed policies tomorrow and upload a new commit to the PR :) From looking at it, it should be as simple as removing the deny statement, when s3:x-amz-server-side-encryption is null, but I will go ahead and verify this tomorrow to make sure :)

FlorianSW commented 1 year ago

I updated the PR with a revised bucket policy to enforce using KMS encryption or the default (kms) encryption for the bucket :) https://github.com/aws/copilot-cli/pull/5329/commits/0763b3fa1336318d86180147fbf61e32e0ef5199

Lou1415926 commented 12 months ago

Hi @FlorianSW, AWS Copilot v1.32.0 is now released: https://github.com/aws/copilot-cli/releases/tag/v1.32.0 🎉🚀 Your contribution that enforce non-default KMS key is included in this release - thank you very much! Also check out our blog post for other very cool features ❤️ !