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

Run Docker container as unprivileged user, allow PUID/PGID selection #722

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open
50 changes: 32 additions & 18 deletions Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -39,41 +39,55 @@
FROM python:${PYTHON_VERSION}-slim-bookworm
LABEL org.opencontainers.image.source="https://github.com/dgtlmoon/changedetection.io"

RUN apt-get update && apt-get install -y --no-install-recommends \
libxslt1.1 \
# For presenting price amounts correctly in the restock/price detection overview
locales \
# For pdftohtml
poppler-utils \
zlib1g \
&& apt-get clean && rm -rf /var/lib/apt/lists/*


# https://stackoverflow.com/questions/58701233/docker-logs-erroneously-appears-empty-until-container-stops
ENV PYTHONUNBUFFERED=1

RUN [ ! -d "/datastore" ] && mkdir /datastore
lkubb marked this conversation as resolved.
Show resolved Hide resolved
RUN set -ex; \
apt-get update && apt-get install -y --no-install-recommends \
gosu \
libxslt1.1 \
# For presenting price amounts correctly in the restock/price detection overview
locales \
# For pdftohtml
poppler-utils \
zlib1g && \
apt-get clean && \
rm -rf /var/lib/apt/lists/*; \
useradd -u 911 -U -m -s /bin/false changedetection && \
usermod -G users changedetection; \
mkdir -p /datastore
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkdir if not exists? will it work if it exists?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mkdir -p prevents the command to fail upon existing directory, so the exit code will be 0.


# Re #80, sets SECLEVEL=1 in openssl.conf to allow monitoring sites with weak/old cipher suites
RUN sed -i 's/^CipherString = .*/CipherString = DEFAULT@SECLEVEL=1/' /etc/ssl/openssl.cnf

# Copy modules over to the final image and add their dir to PYTHONPATH
COPY --from=builder /dependencies /usr/local
ENV PYTHONPATH=/usr/local
ENV PYTHONPATH=/usr/local \
# https://stackoverflow.com/questions/58701233/docker-logs-erroneously-appears-empty-until-container-stops
PYTHONUNBUFFERED=1 \
# https://stackoverflow.com/questions/64808915/should-pycache-folders-be-included-in-production-containers
# This avoids permission denied errors because the app directory is root-owned.
PYTHONDONTWRITEBYTECODE=1 \
dgtlmoon marked this conversation as resolved.
Show resolved Hide resolved
DATASTORE_PATH="/datastore" \
# Disable creation of Pytest cache dir when running tests inside the container by default
PYTEST_ADDOPTS="-p no:cacheprovider"

EXPOSE 5000

# The entrypoint script handling PUID/PGID and permissions
COPY --chmod=755 docker-entrypoint.sh /app/docker-entrypoint.sh

# The actual flask app module
COPY changedetectionio /app/changedetectionio
# Starting wrapper
COPY changedetection.py /app/changedetection.py

# create test directory for pytest to run in
RUN mkdir -p /app/changedetectionio/test-datastore && \
chown changedetection:changedetection /app/changedetectionio/test-datastore

# Github Action test purpose(test-only.yml).
# On production, it is effectively LOGGER_LEVEL=''.
ARG LOGGER_LEVEL=''
ENV LOGGER_LEVEL "$LOGGER_LEVEL"

Check warning on line 89 in Dockerfile

View workflow job for this annotation

GitHub Actions / test-container-build

Legacy key/value format with whitespace separator should not be used

LegacyKeyValueFormat: "ENV key=value" should be used instead of legacy "ENV key value" format More info: https://docs.docker.com/go/dockerfile/rule/legacy-key-value-format/

WORKDIR /app
CMD ["python", "./changedetection.py", "-d", "/datastore"]


ENTRYPOINT ["/app/docker-entrypoint.sh"]
CMD ["python", "/app/changedetection.py"]
6 changes: 4 additions & 2 deletions changedetectionio/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,15 +42,17 @@ def main():
global datastore
global app

datastore_path = None
datastore_path = os.environ.get('DATASTORE_PATH')
do_cleanup = False
host = ''
ipv6_enabled = False
port = os.environ.get('PORT') or 5000
ssl_mode = False

if datastore_path is not None:
pass
# On Windows, create and use a default path.
if os.name == 'nt':
elif os.name == 'nt':
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt this stay as if ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it would then not respect the DATASTORE_PATH defined by the user (or the default one set during container build time).

datastore_path = os.path.expandvars(r'%APPDATA%\changedetection.io')
os.makedirs(datastore_path, exist_ok=True)
else:
Expand Down
5 changes: 1 addition & 4 deletions changedetectionio/tests/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,6 @@ def measure_memory_usage(request):
s = f"Peak memory used by the test {request.node.fspath} - '{request.node.name}': {max_memory_used:.2f} MB"
logger.debug(s)

with open("test-memory.log", 'a') as f:
f.write(f"{s}\n")

# Assert that the memory usage is less than 200MB
# assert max_memory_used < 150, f"Memory usage exceeded 200MB: {max_memory_used:.2f} MB"

Expand Down Expand Up @@ -109,6 +106,6 @@ def teardown():
app.config.exit.set()
cleanup(app_config['datastore_path'])


request.addfinalizer(teardown)
yield app
2 changes: 1 addition & 1 deletion changedetectionio/tests/test_backup.py
Original file line number Diff line number Diff line change
Expand Up @@ -67,4 +67,4 @@ def test_backup(client, live_server, measure_memory_usage):
follow_redirects=True
)

assert b'No backups found.' in res.data
assert b'No backups found.' in res.data
41 changes: 41 additions & 0 deletions docker-entrypoint.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
#!/bin/bash

set -eu

DATASTORE_PATH="${DATASTORE_PATH:-/datastore}"

# If the first argument looks like a flag, assume we want to run changedetection
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm what is the problem you're trying to solve here?

Copy link
Author

@lkubb lkubb Jul 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming you're referring to the line below: If the container is e.g. run with docker-compose run changedetection -c, this would run python ./changedetection.py -d /datastore -c inside the container. It's mostly a convention, nothing that's necessary, just nice to have. If you prefer it removed, no worries. :)

if [ "${1:0:1}" = '-' ]; then
set -- python /app/changedetection.py "$@"
fi

# If we're running as root, by default make sure process uid/gid
# and datadir permissions are correct. This can be skipped by setting
# KEEP_PRIVILEGES to something non-empty.
if [ "$(id -u)" = '0' -a -z "${KEEP_PRIVILEGES:-}" ]; then
dgtlmoon marked this conversation as resolved.
Show resolved Hide resolved
PUID=${PUID:-911}
PGID=${PGID:-911}

groupmod -o -g "$PGID" changedetection
usermod -o -u "$PUID" changedetection

# Check if the supplied uid/gid grants write permissions on the datastore
# root directory. Only if it does not, chown it recursively.
# In my testing, `test -w "$DATASTORE_PATH"` did not work reliably.
tempfile="$DATASTORE_PATH/.check-writable"
gosu changedetection:changedetection bash -c ">> '$tempfile'" &&
rm -f "$tempfile" ||
chown -R changedetection:changedetection "$DATASTORE_PATH" ||
(
echo "Failed to change permissions on $DATASTORE_PATH. Ensure it is writable by $PUID:$PGID" >&2
exit 1
)

# Ensure the home directory's permissions are adjusted as well.
chown -R changedetection:changedetection ~changedetection

# Restart this script as an unprivileged user
exec gosu changedetection:changedetection "$BASH_SOURCE" "$@"
lkubb marked this conversation as resolved.
Show resolved Hide resolved
fi

exec "$@"