feature: add CORS_ALLOW_PRIVATE_NETWORK_ACCESS env var

[danielcompton]

Fixes #180.

[4141done]

Hi @danielcompton thank you for you contribution and the thorough description in the issue. I agree that this is a good feature for us to provide and the env var name you chose pairs well with our existing CORS_ options.

Give me a moment to review and test and I think we can get this merged 🤘

[4141done]

So after looking through this more closely in the review it seems that implementing this using the logic in this PR will be a little more complicated than I thought.

To enumerate some of the options:

1. You could maintain your own modification to the common/etc/nginx/templates/gateway/cors.conf.template file including the header. We could document how to do this in the README.
2. We could explore adding the ability to always include certain response headers. That would result in Access-Control-Allow-Private-Network: true being sent always but might be a good general purpose improvement.
3. We could implement your proposed CORS_ALLOW_PRIVATE_NETWORK_ACCESS as I suggested in my comment (always returning true if enabled). But without any awareness of the actual headers received.

These are all options that I am fine going with but you know the needs of someone with this use case better than I do. I don't feel comfortable putting in a lot of logic to set the headers conditionally but I could certainly supply that code if you wanted to implement that in your deployment.

[4141done]

Thank you for documenting this so well.

[danielcompton]

@4141done sorry for the delay on this. I've updated the code, tested that it works locally, and also updated some other parts of the Dockerfiles and scripts that I think also needed to be updated.

[alessfg]

Hi there @danielcompton!

We have changed the base branch of the repo since your original commits -- I edited the PR to target the new base branch (main) but it looks like there are some merge conflict. Can you update the PR when you get a chance?

Thanks!

[github-actions]

✅ All required contributors have signed the F5 CLA for this PR. Thank you!
_{Posted by the CLA Assistant Lite bot.}

[danielcompton]

Hi @alessfg sorry for the delay, I've rebased this now and resolved the merge conflicts.

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

[danielcompton]

recheck

[alessfg]

Thanks! I think you might need to agree to the CLA on a separate message (sorry about this, we are still testing out the workflow!)

[4141done]

Thank you for picking this up again @danielcompton 🎉

Let me rebuild my context on this issue and review this week.

[danielcompton]

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

[4141done]

Thanks for your patience @danielcompton - this change looks good to me!
I want to set aside some time to do my own testing on Monday but I don't expect we'll need any changes to merge.

[4141done]

Ok so I spoke to some of my more knowledgeable NGINX colleagues and there was some concern about introducing a set in an if since there are known issues with if in a location. We need to fix the existing usages in this file but probably should not introduce a new one.

Luckily we already have an area in the bootstrap script for this project where we message the cors-related variables so my suggestion is that we put this there for now. Here's a diff that illustrates this:

diff --git a/common/docker-entrypoint.sh b/common/docker-entrypoint.sh
index 8688602..939c57b 100644
--- a/common/docker-entrypoint.sh
+++ b/common/docker-entrypoint.sh
@@ -68,6 +68,12 @@ if [ -z "${CORS_ALLOWED_ORIGIN+x}" ]; then
   export CORS_ALLOWED_ORIGIN="*"
 fi
 
+# See documentation for this feature. We do not parse this as a boolean
+# since "true" and "false" are the required values of the header this populates
+if [ "${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}" != "true" ] && [ "${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}" != "false" ]; then
+  export CORS_ALLOW_PRIVATE_NETWORK_ACCESS=""  
+fi
+
 # This is the primary logic to determine the s3 host used for the
 # upstream (the actual proxying action) as well as the `Host` header
 #
diff --git a/standalone_ubuntu_oss_install.sh b/standalone_ubuntu_oss_install.sh
index dbeb3f1..ea74e7f 100644
--- a/standalone_ubuntu_oss_install.sh
+++ b/standalone_ubuntu_oss_install.sh
@@ -30,6 +30,17 @@ fi
 
 failed=0
 
+parseBoolean() {
+  case "$1" in
+    TRUE | true | True | YES | Yes | 1)
+      echo 1
+      ;;
+    *)
+      echo 0
+      ;;
+  esac
+}
+
 required=("S3_BUCKET_NAME" "S3_SERVER" "S3_SERVER_PORT" "S3_SERVER_PROTO"
 "S3_REGION" "S3_STYLE" "ALLOW_DIRECTORY_LIST" "AWS_SIGS_VERSION")
 
@@ -142,6 +153,8 @@ if [ "${to_install}" != "" ]; then
   systemctl stop nginx
 fi
 
+cors_enabled="$(parseBoolean "${CORS_ENABLED}")"
+
 echo "▶ Adding environment variables to NGINX configuration file: /etc/nginx/environment"
 cat > "/etc/nginx/environment" << EOF
 # Enables or disables directory listing for the S3 Gateway (true=enabled, false=disabled)
@@ -179,18 +192,24 @@ PROXY_CACHE_VALID_NOTFOUND=${PROXY_CACHE_VALID_NOTFOUND:-'1m'}
 # Proxy caching time for response code 403
 PROXY_CACHE_VALID_FORBIDDEN=${PROXY_CACHE_VALID_FORBIDDEN:-'30s'}
 # Enables or disables CORS for the S3 Gateway (true=enabled, false=disabled)
-CORS_ENABLED=${CORS_ENABLED:-'false'}
+CORS_ENABLED=${cors_enabled:-'false'}
 # Configure portion of URL to be removed (optional)
 STRIP_LEADING_DIRECTORY_PATH=${STRIP_LEADING_DIRECTORY_PATH:-''}
 # Configure portion of URL to be added to the beginning of the requested path (optional)
 PREFIX_LEADING_DIRECTORY_PATH=${PREFIX_LEADING_DIRECTORY_PATH:-''}
 EOF
 
+# Normalize the CORS_ENABLED environment variable to a numeric value
+# so that it can be easily parsed in the nginx configuration.
+cat >> "/etc/nginx/environment" << EOF
+CORS_ENABLED="${cors_enabled}"
+EOF
+
 # By enabling CORS, we also need to enable the OPTIONS method which
 # is not normally used as part of the gateway. The following variable
 # defines the set of acceptable headers.
 set +o nounset   # don't abort on unbound variable
-if [ "${CORS_ENABLED}" == "1" ]; then
+if [ "${cors_enabled}" == "1" ]; then
     cat >> "/etc/nginx/environment" << EOF
 LIMIT_METHODS_TO="GET HEAD OPTIONS"
 LIMIT_METHODS_TO_CSV="GET, HEAD, OPTIONS"
@@ -202,6 +221,18 @@ LIMIT_METHODS_TO_CSV="GET, HEAD"
 EOF
 fi
 
+# See documentation for this feature. We do not parse this as a boolean
+# since "true" and "false" are the required values of the header this populates
+if [ "${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}" != "true" ] && [ "${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}" != "false" ]; then
+    cat >> "/etc/nginx/environment" << EOF
+CORS_ALLOW_PRIVATE_NETWORK_ACCESS=""
+EOF
+else
+    cat >> "/etc/nginx/environment" << EOF
+CORS_ALLOW_PRIVATE_NETWORK_ACCESS="${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}"
+EOF
+fi
+
 # This is the primary logic to determine the s3 host used for the
 # upstream (the actual proxying action) as well as the `Host` header
 #

(the above diff also contains a small fix for a bug I discovered in the VM bootstrap script regarding the CORS_ENABLED variable handling)

This would mean the changes to cors.conf.template would then just be this:

add_header 'Access-Control-Allow-Private-Network' '${CORS_ALLOW_PRIVATE_NETWORK_ACCESS}';

Sorry about the churn on this PR but I'm also learning as I touch various parts of this project.

[4141done]

Oh- and don't worry about any changes in the Ubuntu standalone script. That one is a bit more tricky to verify so I'll handle.adding that after this pr merges

[danielcompton]

Thanks for the guidance @4141done. I've updated the branch with your suggestions. I'm also happy for you to make any other changes directly if that's easier for you.

[4141done]

Hey apologies for my slowness - just wanted to let you know that I've received this and it looks good - I'm just adding this to the VM startup script and running a quick test then I think we can merge soon.

[4141done]

Thank you for your work and patience with us on this!

[danielcompton]

Amazing, thanks so much! I appreciate your help on this.