search

nginxinc / nginx-s3-gateway

NGINX S3 Caching Gateway
Apache License 2.0
488 stars 127 forks source link

Add new ENV config S3_SERVICE_REMOVE_X_AMZ_HEADERS #241

Closed gawsoftpl closed 3 months ago

gawsoftpl commented 4 months ago

Add S3_SERVICE_REMOVE_X_AMZ_HEADERS header flag for use case where you want to have x-amz headers in nginx proxy

Proposed changes

In my use case I need x-amz headers from s3 in proxy response. I know that this is a security issue. But my project save some metadata in s3 files and do not share it with public.

Checklist

Before creating a PR, run through this checklist and mark each as complete:

github-actions[bot] commented 4 months ago

CLA Assistant Lite bot ✅ All required contributors have signed the F5 CLA for this PR. Thank you!

gawsoftpl commented 4 months ago

I have hereby read the F5 CLA and agree to its terms

gawsoftpl commented 4 months ago

I see that CI not passed because S3_SERVICE_REMOVE_X_AMZ_HEADERS was required env I have changed it. Now env S3_SERVICE_REMOVE_X_AMZ_HEADERS is not required

4141done commented 4 months ago

Hi @gawsoftpl thank you for your contribution. Instead of adding a new configuration variable specific to AWS response headers, I think it would be best of all users of the gateway if we could introduce a configuration option that is the opposite of HEADER_PREFIXES_TO_STRIP.

It could follow the same format (list of prefixes separated by ;) and be called something like HEADER_PREFIXES_ALLOWED.

We then have two implementation options:

  1. Change the default of HEADER_PREFIXES_TO_STRIP to x-amz-. If it's specified, we'd have to add x-amz- to the list if x-amz- is not specified in HEADER_PREFIXES_ALLOWED to avoid accidentally causing people with the gateway configured with HEADER_PREFIXES_TO_STRIP to start returning aws headers.
  2. Don't change the default of HEADER_PREFIXES_TO_STRIP and just remove the x-amz- stripping in the JS code if it's specified in HEADER_PREFIXES_ALLOWED

Both achieve the same thing but (1) is little harder to reason about. I think I prefer (2) but open to discuss.

That way we have a generic way to control both headers stripped and headers preserved. It also helps us take another step towards being more general purpose for non-AWS object stores in the JS code which is desirable.

Would this approach also satisfy your needs?

gawsoftpl commented 4 months ago

Hi @gawsoftpl thank you for your contribution. Instead of adding a new configuration variable specific to AWS response headers, I think it would be best of all users of the gateway if we could introduce a configuration option that is the opposite of HEADER_PREFIXES_TO_STRIP.

It could follow the same format (list of prefixes separated by ;) and be called something like HEADER_PREFIXES_ALLOWED.

We then have two implementation options:

  1. Change the default of HEADER_PREFIXES_TO_STRIP to x-amz-. If it's specified, we'd have to add x-amz- to the list if x-amz- is not specified in HEADER_PREFIXES_ALLOWED to avoid accidentally causing people with the gateway configured with HEADER_PREFIXES_TO_STRIP to start returning aws headers.
  2. Don't change the default of HEADER_PREFIXES_TO_STRIP and just remove the x-amz- stripping in the JS code if it's specified in HEADER_PREFIXES_ALLOWED

Both achieve the same thing but (1) is little harder to reason about. I think I prefer (2) but open to discuss.

That way we have a generic way to control both headers stripped and headers preserved. It also helps us take another step towards being more general purpose for non-AWS object stores in the JS code which is desirable.

Would this approach also satisfy your needs?

Ok, so the best solution will be number 2. add Header HEADER_PREFIXES_ALLOWED and remove x-amz- if is allowed. I have upload new commit with this update. Please review.

gawsoftpl commented 3 months ago

Thank you so much for all of your work on this. I tested this against a real S3 bucket and was able to verify that the config variables work together as expected.

I did discover one area where we could improve the messaging:

diff --git a/common/docker-entrypoint.d/00-check-for-required-env.sh b/common/docker-entrypoint.d/00-check-for-required-env.sh
index a09a76a..31b1947 100755
--- a/common/docker-entrypoint.d/00-check-for-required-env.sh
+++ b/common/docker-entrypoint.d/00-check-for-required-env.sh
@@ -134,4 +134,5 @@ echo "Directory Listing Path Prefix: ${DIRECTORY_LISTING_PATH_PREFIX}"
 echo "Provide Index Pages Enabled: ${PROVIDE_INDEX_PAGE}"
 echo "Append slash for directory enabled: ${APPEND_SLASH_FOR_POSSIBLE_DIRECTORY}"
 echo "Stripping the following headers from responses: x-amz-;${HEADER_PREFIXES_TO_STRIP}"
+echo "Allow the following headers from responses (these take precendence over the above): ${HEADER_PREFIXES_ALLOWED}"
 echo "CORS Enabled: ${CORS_ENABLED}"

Can you make that small change then I will go ahead and merge it!

I added line: echo "Allow the following headers from responses (these take precendence over the above): ${HEADER_PREFIXES_ALLOWED}"

to docker entrypoint

4141done commented 3 months ago

Thanks again for your contribution, @gawsoftpl - the image with your changes is here https://github.com/nginxinc/nginx-s3-gateway/pkgs/container/nginx-s3-gateway%2Fnginx-oss-s3-gateway/211366123?tag=latest-20240502