[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_r599878298



##########
File path: guacamole-docker/bin/start.sh
##########
@@ -708,6 +708,58 @@ associate_json() {
     # Add required .jar files to GUACAMOLE_EXT
     ln -s /opt/guacamole/json/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() {
+    # Add <Valve> element
+    xmlstarlet edit --inplace \
+        --insert '/Server/Service/Engine/Host/*' --type elem -n Valve \
+        --insert '/Server/Service/Engine/Host/Valve[not(@className)]' --type attr -n className -v org.apache.catalina.valves.RemoteIpValve \
+        $CATALINA_BASE/conf/server.xml
+
+    # Allowed IPs
+    if [ -z "$GUACAMOLE_PROXY_ALLOWED_IPS_REGEX" ]; then
+    	echo "Using default Tomcat allowed IPs regex"

Review comment:
       Please use 4-space indents per level.

##########
File path: guacamole-docker/bin/start.sh
##########
@@ -708,6 +708,58 @@ associate_json() {
     # Add required .jar files to GUACAMOLE_EXT
     ln -s /opt/guacamole/json/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() {
+    # Add <Valve> element
+    xmlstarlet edit --inplace \
+        --insert '/Server/Service/Engine/Host/*' --type elem -n Valve \
+        --insert '/Server/Service/Engine/Host/Valve[not(@className)]' --type attr -n className -v org.apache.catalina.valves.RemoteIpValve \
+        $CATALINA_BASE/conf/server.xml
+
+    # Allowed IPs
+    if [ -z "$GUACAMOLE_PROXY_ALLOWED_IPS_REGEX" ]; then

Review comment:
       `GUACAMOLE_PROXY_ALLOWED_IPS_REGEX`, `GUACAMOLE_PROXY_IP_HEADER`, etc. feel redundant with their `GUACAMOLE_` prefix, which is not used by any of the other Docker-specific environment variables for the `guacamole/guacamole` image. It might also result in some confusion, as guacd is sometimes referred to as the Guacamole proxy daemon.

##########
File path: Dockerfile
##########
@@ -50,6 +50,9 @@ RUN /opt/guacamole/bin/build-guacamole.sh "$BUILD_DIR" /opt/guacamole "$BUILD_PR
 # For the runtime image, we start with the official Tomcat distribution
 FROM tomcat:${TOMCAT_VERSION}-${TOMCAT_JRE}
 
+# Install XMLStarlet for server.conf alterations

Review comment:
       server.conf?

##########
File path: Dockerfile
##########
@@ -50,6 +50,9 @@ RUN /opt/guacamole/bin/build-guacamole.sh "$BUILD_DIR" /opt/guacamole "$BUILD_PR
 # For the runtime image, we start with the official Tomcat distribution
 FROM tomcat:${TOMCAT_VERSION}-${TOMCAT_JRE}
 
+# Install XMLStarlet for server.conf alterations
+RUN apt-get update -qq && apt-get install -y xmlstarlet

Review comment:
       For Docker images, the package cache should be cleaned after `apt-get` is finished, to avoid unnecessarily increasing the size of the image.
   
   For example, in the guacamole-server `Dockerfile`:
   
   https://github.com/apache/guacamole-server/blob/b2ae2fdf003a6854ac42877ce0fce8e88ceb038a/Dockerfile#L143-L147




-- 
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