getlift / lift

Expanding Serverless Framework beyond functions using the AWS CDK
MIT License
913 stars 113 forks source link

Expose bucketName variable in server-side-website #241

Closed acrobat closed 2 years ago

acrobat commented 2 years ago

I needed the created assets bucket name to reference it in a BucketPolicy. So I've exposed the bucketname as a variable (like in the storage construct)

acrobat commented 2 years ago

@fredericbarthelet I my case due to a security requirement I need to add the s3-bucket-ssl-requests-only rule

resources:
    Resources:
        BucketPolicy:
            Type: AWS::S3::BucketPolicy
            Properties:
                Bucket: ${construct:website.assetsBucketName}
                PolicyDocument:
                    Statement:
                        - Effect: Deny
                          Action: '*'
                          Principal: '*'
                          Resource:
                              - !Join ['', ['arn:aws:s3:::', '${construct:website.assetsBucketName}']]
                              - !Join ['', ['arn:aws:s3:::', '${construct:website.assetsBucketName}', '/*']]
                          Condition:
                              Bool:
                                  "aws:SecureTransport": false
fredericbarthelet commented 2 years ago

Thanks for updating your PR @acrobat. I believe this could be part of Lift as is in order to ensure all buckets deployed by Lift are compliant regarding this rule from AWS Config (not only server side website). Do you know any situation where an HTTP access to the bucket is relevant and might therefor prevent implementing this by default ?

@mnapoli what do you think as well of implementing this by default on Lift's buckets ?

acrobat commented 2 years ago

That could be a good idea to expand the default bucket setup. For example in the storage construct this is already done https://github.com/getlift/lift/blob/c0f260ad59bb1d4c08a15083caf0803e3baf027d/src/constructs/aws/Storage.ts#L52

It might make sense to default enable enforceSSL and set encryption to the default S3 mode in the server-side-website construct too.

So if you all agree I can add this to the assets bucket setup but I think exposing the bucket name would still be usefull if you want to do something outside of the default lift bucket config.

mnapoli commented 2 years ago

Enforcing SSL in Lift sounds like a good idea 👍

However I don't think encryption makes sense here: these are public assets. No idea if this could impact latency serving assets, but it would be a waste of computing resources anyway.

As discussed with @fredericbarthelet today, it may also be worth looking into creating a CDK construct (private, in Lift's codebase) for a S3 bucket serving assets? Because we have the same kind of S3 bucket in server-side-website, single-page-app and static-website. I don't remember if they are strictly identical though 🤔 so it may, or may not be, a good idea. Just food for thought.

fredericbarthelet commented 2 years ago

@acrobat would you be up to change your PR to implement such Asset construct with sslEnforced by default and use it in server-side-website and other constructs relying on asset ?

acrobat commented 2 years ago

@fredericbarthelet sounds good! I will try to come up with a PR! Are you going to merge this PR also? Or do I just add that change in this PR first?

fredericbarthelet commented 2 years ago

@acrobat merging this one for now, it still might be useful to export asset bucket name. If you'd like to have the AWS Config rule compliant by default in Lift in the future, I'd accept another PR with pleasure for you to implement this Lift Bucket as discussed above. Thanks :)