Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Docker image and Tomcat configuration hardening #39

Merged
merged 8 commits into from
Feb 9, 2024

Conversation

ahennr
Copy link
Contributor

@ahennr ahennr commented Dec 12, 2023

In this PR some of recommendations listed in the CIS Apache Tomcat (9) and CIS Docker Benchmarks (see also https://www.cisecurity.org/cis-benchmarks) have been applied.
These recommendations contain best practices for configuring Tomcat and Docker in a secure way.
However, there exist much more benchmarks for other software as well.

Attackers often use sensitive information provided by the webserver (e.g. versions, headers, stack traces…) in their attacks. The Recommendations regarding Tomcat should increase the complexity of determination of this information.

In particular, I've integrated the following configuration changes:

  • Remove the host-manager and the manager from webapps folder (since it is not required for GeoServer AFAIK)
  • In (a new) server.xml which is copied to $CATALINA_HOME/conf/:
    • Set allowTrace="false" in HTTP-Connector (since TRACE requests often contain sensitive information)
    • Set xpoweredBy="false" explicitly (will remove X-Powered-By HTTP header)
    • In startup.sh script, the SHUTDOWN command in server.xml is replaced to a non deterministic value (here: sequence of ten random chars). Usually Tomcat listens on TCP port 8005 to accept shutdown requests (If this port needs is exposed by any application). Herewith, its prevented that malicious local users shutting down Tomcat.
  • Alter the advertised server.info String in order to hide version number and build date for attackers (e.g. on 404 not found). If the default shutdown command should be as is, ENABLE_DEFAULT_SHUTDOWN can be set to true an an environment variable.

Beside, based on the work in @alapierre 's fork of GeoServer docker (see https://github.com/alapierre/geoserver-docker/blob/master/Dockerfile#L108), I've added a user geoserver (in group geoserver) which is used to run GeoServer in the docker image. Usually containers should run as a non-root user (see also https://docs.docker.com/develop/develop-images/instructions/#user and CIS Docker Benchmark).

Furthermore, the CIS Docker benchmark recommends to remove setuid and setgid permissions in the docker image to prevent privilege escalation attacks within containers.

I'm not sure if these are useful settings / improvements for the GeoServer base image itself, or rather for another variant as proposed in #27 (multi variant build), what do you think about it?

@buehner
Copy link
Member

buehner commented Dec 20, 2023

I really like this. Thx a lot!

One thing that comes to my mind: #34 also adds a custom server.xml.

We should have this in mind, once one of these PRs should be merged.

@fleimgruber
Copy link

I just found this on a search to change the default Tomcat port 8080 as we would like to use this in a docker network where port 8080 is already taken. @buehner do all config options that this server.xml specifies have their default values currently or is there another config source file?

@buehner
Copy link
Member

buehner commented Feb 7, 2024

I think it makes sense to use an explicit user ID when adding/creating the geoserver user. Something like sudo useradd -u 1500 geoserver

This might help in case someone has problems with user permission (after merging this PR) that could be solved by something like chown -R 1500:1500 ...

@buehner
Copy link
Member

buehner commented Feb 7, 2024

Generally it would be nice to also add some lines to the README to document this (use of a custom server.xml)

@ahennr ahennr marked this pull request as ready for review February 7, 2024 11:59
@buehner
Copy link
Member

buehner commented Feb 7, 2024

Many thanks for the adjustments. There is one more point that came to my mind:

There is a CONFIG_OVERRIDES_DIR feature that is already used elsewhere:

ENV CONFIG_OVERRIDES_DIR=/opt/config_overrides

docker/startup.sh

Lines 104 to 107 in 0966018

if [ -d "${CONFIG_OVERRIDES_DIR}" ] && [ -f "${CONFIG_OVERRIDES_DIR}/context.xml" ]; then
echo "Installing configuration override for context.xml with substituted environment variables"
envsubst < "${CONFIG_OVERRIDES_DIR}"/context.xml > "${CATALINA_HOME}/conf/context.xml"
else

This is also used in #44

I think it makes sense to handle aspects like this as uniformly as possible, so I would suggest to adapt this accordingly.

For this PR this would probably mean that we remove COPY config/server.xml $CATALINA_HOME/conf/server.xml and instead check in startup.sh whether the desired configuration has been mounted like it is done in the snippet above.

What do you think?

@buehner
Copy link
Member

buehner commented Feb 7, 2024

Maybe I am also mixing things up here. I guess we always want the hardened server.xml, so maybe my previous post is not fully correct.

But maybe it still makes sense to add such an (optional) possibility to allow the use of custom xmls in a uniform way

@buehner
Copy link
Member

buehner commented Feb 7, 2024

@ahennr Thx for letting me push some more commits to your branch :-)

Please review my latest changes. If you are happy, I think we can merge this

@ahennr
Copy link
Contributor Author

ahennr commented Feb 7, 2024

Looks good @buehner , I've no further comments. Please merge at will

@buehner
Copy link
Member

buehner commented Feb 7, 2024

do all config options that this server.xml specifies have their default values currently or is there another config source file?

@fleimgruber Not sure what exactly changed in the server.xml that is used here (compared to the original one). I think @ahennr just added some security hardenings as explained above. But if you want to know the defaults of an original server.xml, just download an official tomcat installation zip and you will find the original server.xml in the conf dir there.

@fleimgruber
Copy link

fleimgruber commented Feb 8, 2024

@buehner thanks for checking back!

Not sure what exactly changed in the server.xml that is used here (compared to the original one).

Part of my confusion was that there was no original one in this repo - this very PR adds it.

In the meantime I created a fresh server.xml based on upstream as you suggested and mounted it much like it is done in this PR here, but instead in a Docker compose file.

So all questions answered and sorry for hijacking this PR.

@buehner buehner merged commit 3727938 into geoserver:master Feb 9, 2024
This was referenced Feb 9, 2024
@ahennr ahennr deleted the tomcat-hardening-cis branch February 9, 2024 10:58
@petersmythe
Copy link
Contributor

petersmythe commented Feb 20, 2024

Is it possible that this PR broke the Jenkins Docker build job, last working on 9 Feb?

see: https://build.geoserver.org/job/geoserver-release-docker/buildTimeTrend

The error in https://build.geoserver.org/job/geoserver-release-docker/lastBuild/console:

Step 67/74 : RUN cd $CATALINA_HOME/lib     && jar xf catalina.jar org/apache/catalina/util/ServerInfo.properties     && sed -i 's/Apache Tomcat\/'"${TOMCAT_VERSION}"'/i_am_a_teapot/g' org/apache/catalina/util/ServerInfo.properties     && sed -i 's/'"${TOMCAT_VERSION}"'/x.y.z/g' org/apache/catalina/util/ServerInfo.properties     && sed -i 's/^server.built=.*/server.built=/g' org/apache/catalina/util/ServerInfo.properties     && jar uf catalina.jar org/apache/catalina/util/ServerInfo.properties     && rm -rf org/apache/catalina/util/ServerInfo.properties
 ---> Running in 9ec746c415e5
�[91msed: -e expression #1, char 0: no previous regular expression
�[0mThe command '/bin/sh -c cd $CATALINA_HOME/lib     && jar xf catalina.jar org/apache/catalina/util/ServerInfo.properties     && sed -i 's/Apache Tomcat\/'"${TOMCAT_VERSION}"'/i_am_a_teapot/g' org/apache/catalina/util/ServerInfo.properties     && sed -i 's/'"${TOMCAT_VERSION}"'/x.y.z/g' org/apache/catalina/util/ServerInfo.properties     && sed -i 's/^server.built=.*/server.built=/g' org/apache/catalina/util/ServerInfo.properties     && jar uf catalina.jar org/apache/catalina/util/ServerInfo.properties     && rm -rf org/apache/catalina/util/ServerInfo.properties' returned a non-zero code: 1
Build step 'Execute shell' marked build as failure
Finished: FAILURE

matches the newly added code.

@buehner
Copy link
Member

buehner commented Feb 20, 2024

@petersmythe I think you are right. I just had a very quick look and I don't know much about jenkins, but could this be some kind of jenkins specific error? This seems to run fine locally, so I guess locally a zero code is returned from the line. But i think @ahennr and I will try to find some time soon to have a deeper look here. Thx for reporting

@aaime
Copy link
Member

aaime commented Feb 20, 2024

This issue is also affecting the 2.23.5 release (or, as an alternative, we'll have to release without the Docker image)

@buehner
Copy link
Member

buehner commented Feb 20, 2024

@petersmythe @aaime We could easily fix this in #51

Successful build: https://build.geoserver.org/job/geoserver-release-docker/759/

The reason was a missing TOMCAT_VERSION env variable. For some reason this seems to be no problem when using the latest docker locally (as the tomcat build arg seems to be passed to the second build step), but probably older/other versions of docker are missing this feature.

Thx for the help @ahennr

@petersmythe
Copy link
Contributor

Wonderful, thank you for the quick fix @buehner and @ahennr

@petersmythe
Copy link
Contributor

@petersmythe @aaime We could easily fix this in #51

Successful build: https://build.geoserver.org/job/geoserver-release-docker/759/

The reason was a missing TOMCAT_VERSION env variable. For some reason this seems to be no problem when using the latest docker locally (as the tomcat build arg seems to be passed to the second build step), but probably older/other versions of docker are missing this feature.

Thx for the help @ahennr

Could someone please double check docker run docker.osgeo.org/geoserver:2.24.2

I get a permission denied error:

...
Digest: sha256:2e56eeb73f1dad0d76216a3e978d930bfe3b362cf2af0336c67fe6463a7288bc
Status: Downloaded newer image for docker.osgeo.org/geoserver:2.24.2
Welcome to GeoServer 2.24.2
Starting download of extensions
mkdir: cannot create directory '/opt/additional_libs/': Permission denied
...
Error: A fatal exception has occurred. Program will exit.

@buehner
Copy link
Member

buehner commented Feb 20, 2024

I just ran docker run -p 80:8080 docker.osgeo.org/geoserver:2.24.2 successfully without any problems. LGTM 🤷

@buehner
Copy link
Member

buehner commented Feb 20, 2024

But docker run -p 80:8080 --env INSTALL_EXTENSIONS=true --env STABLE_EXTENSIONS="wps,ysld" docker.osgeo.org/geoserver:2.24.2 does not work for me (same error, thx @ahennr )

We will have a look again

@buehner
Copy link
Member

buehner commented Feb 20, 2024

@petersmythe We added this: #52
Feel free to merge (or let me know)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants