[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #489: GUACAMOLE-1005: Docker, configure RemoteIPValve

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #489:
URL: https://github.com/apache/guacamole-client/pull/489#discussion_r581293682



##########
File path: guacamole-docker/bin/start.sh
##########
@@ -683,6 +683,43 @@ END
     ln -s /opt/guacamole/cas/guacamole-auth-*.jar   "$GUACAMOLE_EXT"
 }
 
+##
+## Sets up Tomcat's remote IP valve that allows gathering the remote IP
+## from headers set by a remote proxy
+## Upstream documentation: https://tomcat.apache.org/tomcat-8.5-doc/api/org/apache/catalina/valves/RemoteIpValve.html
+##
+enable_remote_ip_valve() {
+    # Use Tomcat defaults if optional variables have not been provided
+    if [ -z "$GUACAMOLE_PROXY_ALLOWED_IPS_REGEX" ]; then
+    	echo "Using default Tomcat allowed IPs regex"
+    fi
+    if [ -z "$GUACAMOLE_PROXY_IP_HEADER" ]; then
+        echo "Using default Tomcat proxy IP header"
+    fi
+    if [ -z "$GUACAMOLE_PROXY_PROTOCOL_HEADER" ]; then
+        echo "Using default Tomcat proxy protocol header"
+    fi
+    if [ -z "$GUACAMOLE_PROXY_BY_HEADER" ]; then
+        echo "Using default Tomcat proxy forwarded by header"
+    fi
+
+    # Build the new Tomcat configuration inplace
+    ## Explaination:
+    ## The initial regex ((\s)+)</Host>
+    ## Matches the spaces before </Host> as \1 and individual spaces as \2, ...
+    ## The replacement will be located at \1\2\2 (original + 2 spaces)
+    ## ${VAR:+expr} expressions yield either empty (thus using Tomcat's default) or our setting
+    ## The last line restores the configuration file original tag at its original indentation
+    sed -i "s|^\(\(\s\)\+\)</Host>|\1\2\2<Valve \
+	className=\"org.apache.catalina.valves.RemoteIpValve\" \
+	${GUACAMOLE_PROXY_ALLOWED_IPS_REGEX:+internalProxies=\"$GUACAMOLE_PROXY_ALLOWED_IPS_REGEX\"} \
+	${GUACAMOLE_PROXY_IP_HEADER:+remoteIpHeader=\"$GUACAMOLE_PROXY_IP_HEADER\"} \
+	${GUACAMOLE_PROXY_BY_HEADER:+remoteIpProxiesHeader=\"$GUACAMOLE_PROXY_BY_HEADER\"} \
+	${GUACAMOLE_PROXY_PROTOCOL_HEADER:+protocolHeader=\"$GUACAMOLE_PROXY_PROTOCOL_HEADER\"} \
+	/>\n\1</Host>|" \
+	/usr/local/tomcat/conf/server.xml
+}

Review comment:
       I find two main issues with this:
   
   1. The values supplied via environment variables are not properly escaped for inclusion within XML.
   2. It's too complex.
   
   There must be a simpler, more readable way to achieve this. Using `envsubst` on our own template `server.xml` with pre-escaped variables, for example.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org