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

feat: set GEOSERVER_REQUIRE_FILE to currently used $GEOSERVER_DATA_DIR in startup.sh #62

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

simonseyock
Copy link
Contributor

@simonseyock simonseyock commented Jul 17, 2024

There exists a problem where the default settings are copied over the existing geoserver data dir if a custom data dir is used.

If only GEOSERVER_DATA_DIR and neither SKIP_DEMO_DATA nor GEOSERVER_REQUIRE_FILE are set to any values. The GEOSERVER_REQUIRE_FILE will still be set to the default location because it is set in the Dockerfile and not in the startup script.

I assume that GEOSERVER_REQUIRE_FILE is not used outside the startup script is that correct?

The proposed solution here is one way to solve the issue without dropping the environment variable, but honestly as it is only used for checking if the demo data should be copied, I would suggest to drop the variable all together and rather do something like this:

## install release data directory if needed before starting tomcat
if [ "${SKIP_DEMO_DATA}" = "false" ] && [ ! -f "$GEOSERVER_DATA_DIR/global.xml" ]; then
  echo "Initialize $GEOSERVER_DATA_DIR from data directory included in geoserver.war"
  cp -r $CATALINA_HOME/webapps/geoserver/data/* $GEOSERVER_DATA_DIR
fi

instead of

## Skip demo data
if [ "${SKIP_DEMO_DATA}" = "true" ]; then
  unset GEOSERVER_REQUIRE_FILE
elif [ -z "$GEOSERVER_REQUIRE_FILE" ]; then
  export GEOSERVER_REQUIRE_FILE="$GEOSERVER_DATA_DIR/global.xml"
fi

## install release data directory if needed before starting tomcat
if [ ! -z "$GEOSERVER_REQUIRE_FILE" ] && [ ! -f "$GEOSERVER_REQUIRE_FILE" ]; then
  echo "Initialize $GEOSERVER_DATA_DIR from data directory included in geoserver.war"
  cp -r $CATALINA_HOME/webapps/geoserver/data/* $GEOSERVER_DATA_DIR
fi

@simonseyock
Copy link
Contributor Author

Hint: I do not see GEOSERVER_REQUIRE_FILE in here: https://docs.geoserver.org/stable/en/user/configuration/properties/index.html

@buehner
Copy link
Member

buehner commented Jul 18, 2024

I assume that GEOSERVER_REQUIRE_FILE is not used outside the startup script is that correct?

Yes. I am pretty sure that this env only exists in this docker project, but not in main project or somewhere else. -> wrong

edit: I found this: https://docs.geoserver.org/stable/en/user/datadirectory/setting.html#require-files-to-exist

@buehner
Copy link
Member

buehner commented Jul 18, 2024

GEOSERVER_REQUIRE_FILE was first introduced in #6 and later SKIP_DEMO_DATA in #9

Maybe this helps to understand the scenario.

@buehner
Copy link
Member

buehner commented Jul 18, 2024

The proposed solution here is one way to solve the issue without dropping the environment variable, but honestly as it is only used for checking if the demo data should be copied, I would suggest to drop the variable all together and rather do something like this:

## install release data directory if needed before starting tomcat
if [ "${SKIP_DEMO_DATA}" = "false" ] && [ ! -f "$GEOSERVER_DATA_DIR/global.xml" ]; then
  echo "Initialize $GEOSERVER_DATA_DIR from data directory included in geoserver.war"
  cp -r $CATALINA_HOME/webapps/geoserver/data/* $GEOSERVER_DATA_DIR
fi

I agree. Seems like SKIP_DEMO_DATA is the only flag we should use to control whether the demo data should be installed or not.

For now, I'd like to merge this MR as it fixes the problem. We could target the proposed solution to fully drop the GEOSERVER_REQUIRE_FILE in a follow up PR. Fine for you @simonseyock ?

@buehner
Copy link
Member

buehner commented Jul 18, 2024

Maybe @jodygarnett knows more about GEOSERVER_REQUIRE_FILE and why it was introduced in #6 ?

Main question: Do we need GEOSERVER_REQUIRE_FILE in this docker project?

Current understanding on my side is: We don't need it here as we have SKIP_DEMO_DATA and when starting a docker container with the gs image, one could still set the GEOSERVER_REQUIRE_FILE env as it is a feature of the main project according to https://docs.geoserver.org/stable/en/user/datadirectory/setting.html#require-files-to-exist ?

@jodygarnett
Copy link
Member

jodygarnett commented Jul 18, 2024

GEOSERVER_REQUIRE_FILE was specifically introduced to mitigate some kind of security vulnerability with Docker images being started up against an empty GEOSERVER_DATA_DIR. It was reported before we did CVEs and I did not use Docker at the time so I do not remember the exact attack.

But this is used to confirm the location has a global.xml or some other file and thus we are sure it contains a data directory. The use is optional because some people start up GeoServer with an empty data directory and then use the REST API to configure.

The workflow we have, where the startup script unpacks the default data directory, so we are sure there is a data directory in place by the time GeoServer runs, should be compatible with and safe when combined with GEOSERVER_REQUIRE_FILE.

I guess it would be smarter if the default value for GEOSERVER_REQUIRE_FILE was “global.xml”, and one had to go out of there way to say provide an empty string when starting up with an empty data directory on purpose.

@jodygarnett
Copy link
Member

@simonseyock for this statement:

I assume that GEOSERVER_REQUIRE_FILE is not used outside the startup script is that correct?

Is my answer sufficient? The setting should always be provided and indicates a file GeoServer checks when it starts up. If the file is not present GeoServer will refuse to start up to avoid writing files into "the wrong" location.

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

Successfully merging this pull request may close these issues.

3 participants