Add "public read" policy to Asset Manager S3 bucket #130
Conversation
|
@surminus: We hope you don't mind, but we've added you as a reviewer, because you have some context for the changes we're making. |
|
If this is going to be a public bucket, it should have logging enabled, and maybe consider the option of enabling the S3 website service (if https is not required) too. Also, should this go via Fastly? Currently we serve the assets service via Fastly, which helps with caching and certificate management. |
|
@afda16: Thanks. Those are all good suggestions, however, this is just a first step so we can validate some assumptions. Once we've confirmed that it's working as expected, we'd be happy to add logging and serve objects via Fastly. Before we setup Fastly, we're imagining setting up a DNS CNAME. And at that point we'll need to create a bucket with the fully-qualified domain. Bearing all this in mind, would you be happy for us to merge this? |
|
I think the problem is that, depending on the assumptions, it might work differently when you refactor things later. I'm not sure how you are going to test this, but using the S3 API you might get rate limit errors. We also need to make clear somewhere that this shouldn't be deployed like this on the production AWS account, but we don't use Fastly on Integration so we need a good idea of how to do this on each environment. |
|
For now, maybe move the |
|
Thanks for reviewing, @afda16. I'm going to make the change you've suggested in the hope that we can get this deployed to integration today. |
We want to experiment with having Asset Manager redirect asset URLs to objects on S3 - see this pull request [1] for details. Note that this experimental Asset Manager behaviour is disabled by default, but can be enabled using environment variables or on a per-request basis using a query string parameter [2]. Previously the default bucket policy did not allow public read access, but now that we want to redirect requests and have S3 serve the asset to the end-user, we need to add an explicit policy to the bucket to allow public read access. We considered setting the policy on individual objects, but decided it would be simpler to apply it to the bucket as a whole and then create a separate bucket if we need some objects to have a more restrictive policy. Note that we will want to create a DNS CNAME and rename the S3 bucket at some point to get the full effect, but this feels like a useful first step. [1]: alphagov/asset-manager#112 [2]: https://github.com/alphagov/asset-manager/blob/master/README.md#assets-on-s3
I've tested this against my own AWS account and can confirm that the bucket policy is only applied in the integration environment (i.e. when `DEPLOY_ENV=integration`). This hopefully addresses the infrastructure team's concern about making this change in staging/production without us also configuring logging and a CDN. While we plan to do both of those things, we don't want them to prevent us making progress with testing the effect of this change in integration.
chrisroos
force-pushed
the
add-asset-manager-s3-bucket-policy-to-allow-public-read-access
branch
from
5b74231
to
41cf8ac
Compare
|
Hi @afda16. I've added a second commit that ensures we only apply the public-read bucket policy in integration. I've left the bucket in all environments because it's already been created in staging and production and I'm not sure that it makes sense to remove it. I've also rebased this branch on master and force pushed. Can you take another look, please? We'd really like to get this deployed/applied in integration so that we can test the effect of this change on the Asset Manager. cc @floehopper. |
|
@afda16: Thanks. Merging. |
floehopper
deleted the
add-asset-manager-s3-bucket-policy-to-allow-public-read-access
branch
We added this bucket policy in govuk-terraform-provisioning PR 130[1] so that we could test the effect of redirecting to assets served on S3. We've decided to pursue an alternative approach in the short term (proxying requests via nginx) so we don't need this public policy set on the bucket. I've tested this using my local AWS account and confirmed that it removed the policy as expected. [1]: #130
We want to experiment with having Asset Manager redirect asset URLs to objects on S3 - see this pull request for details. Note that this experimental Asset Manager behaviour is disabled by default, but can be enabled using environment variables or on a per-request basis using a query string parameter.
Previously the default bucket policy did not allow public read access, but now that we want to redirect requests and have S3 serve the asset to the end-user, we need to add an explicit policy to the bucket to allow public read access.
We considered setting the policy on individual objects, but decided it would be simpler to apply it to the bucket as a whole and then create a separate bucket if we need some objects to have a more restrictive
policy.
Note that we will want to create a DNS CNAME and rename the S3 bucket at some point to get the full effect, but this feels like a useful first step.