Add new ENV config S3_SERVICE_REMOVE_X_AMZ_HEADERS

[gawsoftpl]

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:

• I have read the contributing guidelines.
• I have signed the F5 Contributor License Agreement (CLA).
• If applicable, I have added tests that prove my fix is effective or that my feature works.
• If applicable, I have checked that any relevant tests pass after adding my changes.
• I have updated any relevant documentation (e.g. README.md).

[github-actions]

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

[gawsoftpl]

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

[gawsoftpl]

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]

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]

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.

[4141done]

This looks good! I just have two items I'd like to address before we merge:

1. Adding some unit tests to confirm the new behaviour
2. A few tweaks to the docs that I've noted in a comment

Please let me know if you need my assistance with either of these. In the meantime I'm going to make some time to test in the next day or two. I think we may be missing some areas where the new config var needs to be explicitly allowed (like in the nginx.conf file)

[4141done]

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!

[gawsoftpl]

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]

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