nginxinc / nginx-s3-gateway

NGINX S3 Caching Gateway
Apache License 2.0
496 stars 126 forks source link

feat: Support AWS S3 Express One Zone buckets #229

Closed 4141done closed 4 months ago

4141done commented 4 months ago

What

This change adds the S3_SERVICE configuration variable which will default to s3 and may be one of s3express or s3.

It also introduces the virtual-v2 S3_STYLE argument option in support of the connectivity requirement of the S3 Express One Zone (directory) buckets. We are using this as a successor to virtual and believe it should work well in all AWS usages but want to be cautious as we make this change.

Many thanks for @hveiga for driving the implementation of this feature in their original pull request.

Setting this variable to s3express will change the "service" used to sign the requests with the V4 header to s3express. Currently the gateway works without this step, but it's advised in the documentation here.

Other Changes

We are moving the determination of the hostname used to query S3 into the docker entrypoint (or bootstrap script for non-docker installs). If S3_STYLE is set to virtual (this is the default and aws recommended scheme) then the hostname will be:

${S3_BUCKET_NAME}.${S3_SERVER}:${S3_SERVER_PORT}

which will be used in these locations:

Based on my reading here: https://docs.aws.amazon.com/AmazonS3/latest/userguide/VirtualHosting.html It looks like AWS recommends that the bucket be always prepended and other schemes exist only for backwards compatibility reasons. However, please comment on this discussion if you have concerns https://github.com/nginxinc/nginx-s3-gateway/discussions/231

4141done commented 4 months ago

One argument for making a code change is that the security guidance advises us to use the service name s3express in the v4 signing header.

You use your S3 Express One Zone credentials to sign Zonal endpoint (object level) API requests with AWS Signature Version 4, with s3express as the service name.

It's hard to tell from the doc, but that might just be required if you're attempting to create a persistent session. It seems to work without the service name change but we might want to consider sending the correct name in case it's a bug on their end.

I'm on the fence about whether it's enough to warrant another config var, but maybe if enabling the variable also enabled the session keeping it would be worth it.

4141done commented 4 months ago

Now that we've verified that this change works, I'm going to follow @hveiga 's original direction and add the S3_SERVICE configuration variable which will default to s3 and may be one of s3express or s3.

Setting this variable to s3express will have the following effects:

  1. Change the "service" used to sign the requests with the V4 header to s3express. Currently the gateway works without this step, but it's advised in the documentation here.
  2. Set the upstreams for both open source and plus to prepend the bucket name to the server address as seen in this PR

I may do a bit of a refactor to accomplish this since we currently have a good number of places where the host is being assembled:

  1. https://github.com/nginxinc/nginx-s3-gateway/blob/7f3064b2c5cf08871164609de8d7e3f972e0a6a5/common/etc/nginx/templates/default.conf.template#L22 for the Host header
  2. https://github.com/nginxinc/nginx-s3-gateway/blob/7f3064b2c5cf08871164609de8d7e3f972e0a6a5/common/etc/nginx/include/s3gateway.js#L230 for the host used to sign the v4 headers
  3. https://github.com/nginxinc/nginx-s3-gateway/blob/7f3064b2c5cf08871164609de8d7e3f972e0a6a5/oss/etc/nginx/templates/upstreams.conf.template#L10 for the actual proxying of the server.

Based on my reading, for a virtual style request signed with the V4 signature we want:

This is what is happening at the moment but for forward maintainability I'd like to see if we can colocate the logic as much as possible. It may not be possible to avoid duplication due to the fact that the upstream blocks need to have the server address templated in before the nginx server is started.

4141done commented 4 months ago

Thank you for the approval @hveiga . I am going to have some other NGINX folks look over this before I merge. I've also opened a discussion here https://github.com/nginxinc/nginx-s3-gateway/discussions/231 on the chance that this change could cause issues for others. I will wait for a while to give people a chance to speak up before merging. I will make sure that you are credited for this contribution when the PR is merged

4141done commented 4 months ago

Thanks again for all your help on this change @hveiga - please note that in the end I wound up adding the S3_STYLE config option virtual-v2 to cut down on risk so you'll need to use that config. Please let me know if you experience any issues and we can fix forward.

hveiga commented 4 months ago

Thanks again for all your help on this change @hveiga - please note that in the end I wound up adding the S3_STYLE config option virtual-v2 to cut down on risk so you'll need to use that config. Please let me know if you experience any issues and we can fix forward.

Thanks to you for the final push to get this in. I'll be testing today and I can report back if I find any issues. šŸ™Œ

hveiga commented 4 months ago

I just tested both S3 General and S3 Express buckets with ghcr.io/nginxinc/nginx-s3-gateway/nginx-oss-s3-gateway:latest-20240425 and it works as expected šŸŽ‰ šŸ™Œ

4141done commented 4 months ago

I just tested both S3 General and S3 Express buckets with ghcr.io/nginxinc/nginx-s3-gateway/nginx-oss-s3-gateway:latest-20240425 and it works as expected šŸŽ‰ šŸ™Œ

Woohoo! I'm going to close out the issue and the discussion point then.