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

[lkubb]

Currently, the container process runs with root privileges. This cannot be changed, even by specifying PUID/PGID (as suggested in docker-compose.yml).

This PR migrates to running as an unprivileged user and makes it possible to specify PUID/PGID environment variables to choose UID/GID. Migration of existing data owned by root is handled transparently.

A side-effect is that the /app directory is read-only to the python process, I'm not sure if that is a problem since I'm not familiar with the architecture of changedetection.

I also took the liberty to clean up the Dockerfile (and apt cache) a bit, hope that's not a problem. :) Please feel free to suggest changes or deny the PR, if it does not fit your vision.

Fixes https://github.com/dgtlmoon/changedetection.io/issues/565.

[dgtlmoon]

master now tests the application inside the fully built docker container

also need to test this on an existing install which used root.root

[dgtlmoon]

@lkubb biggest one so far, gosu not found

[dgtlmoon]

hmm when it's running as test, it needs to use /datastore but only when run in github/automated, because often people test locally when developing too

[dgtlmoon]

https://github.com/dgtlmoon/changedetection.io/blob/e99f07a51d84b351f6dbeda0e04d62596a66fd96/changedetectionio/tests/conftest.py#L33

somehow needs a var passed to pytest like pytest -d /datastore or.. unsure

[lkubb]

a) Introduce a new env var DATASTORE_TESTING, which defaults to test-datastore, but override it in the github workflow b) Just mkdir the test directory in the docker build script. Not very clean but it shouldn't hurt.

[dgtlmoon]

b) Just mkdir the test directory in the docker build script. Not very clean but it shouldn't hurt.

This is fine if theres a comment above like # creating test-dir for pytest to run in

[lkubb]

I added mkdir earlier, but you will need to approve the workflow.

[dgtlmoon]

needs /app/changedetectionio/.pytest_cache

but why not just chown all the /app to the same UID/GUID that we run the container as?

[lkubb]

Because usually, there is no good reason for an application to be able to modify itself. We could of course disable all that with a build arg specifically for the workflow, but I would be very hesitant to chown the app folder for production.

[dgtlmoon]

haha I love tests, what looked like a simple PR always shows up something

FAILED tests/test_backup.py::test_backup - PermissionError: [Errno 13] Permis...

so /datastore directory isnt writable (it creates the backup ZIP there)

[lkubb]

so /datastore directory isnt writable (it creates the backup ZIP there)

From what I can tell, it creates it as /app/changedetectionio/download.zip, right?

============================= test session starts ==============================
platform linux -- Python 3.10.9, pytest-6.2.5, py-1.11.0, pluggy-1.0.0
rootdir: /app/changedetectionio, configfile: pytest.ini
plugins: flask-1.2.0
collected 1 item

tests/test_backup.py::test_backup 
[...]
>       with open("download.zip", 'wb') as f:
E       PermissionError: [Errno 13] Permission denied: 'download.zip'

[dgtlmoon]

Oops right you are, that test needs fixing so its using the test-datastore directory

[dgtlmoon]

Or better is that it doesnt even write the file to disk and just stores it memory

[dgtlmoon]

please change

    # ZipFile from buffer seems non-obvious, just save it instead
    with open("download.zip", 'wb') as f:
        f.write(res.data)

    zip = ZipFile('download.zip')

to

    from io import BytesIO, StringIO
    zip = ZipFile(BytesIO(res.data))

[lkubb]

Already did, try running the workflow again.

[dgtlmoon]

cool, and what about people who have /datastore mounted as root already (like everyone :) ) ? i guess that will continue to work, needs testing somehow

[lkubb]

That should definitely work since it's handled by the entrypoint script with chown -R. Honestly, imho a manual test should be sufficient since it's a simple and one-time operation inside a very specific environment. I'm not too eager to create an automated test for that inside the current testing infrastructure.

On a side-note regarding the tests in general:

For test-datastore, you might consider using something like this instead:

import shutil


@pytest.fixture(scope="session")
def datastore_path(tmp_path_factory):
    datastore = tmp_path_factory.mktemp("test-datastore")

    try:
        yield datastore
    finally:
        shutil.rmtree(str(datastore)), ignore_errors=True)

This will make sure the directory is writable and clean up after itself. This would also allow to remove the mkdir -p /app/changedetectionio/test-datastore && chown -R changedetection:changedetection /app/changedetectionio/test-datastore from the Dockerfile. Also, you could/would need to replace stuff like

with open("test-datastore/notification-url.txt", "w") as f:
    f.write(request.url)

with

(datastore_path / "notification-url.txt").write_text(request.url)

Is there a reason each test/test_*.py is run separately? Pytest autodiscovers those files by itself, so instead of

find tests/test_*py -type f|while read test_name
do
  echo "TEST RUNNING $test_name"
  pytest $test_name
done

pytest tests/

should be enough (and would speed up testing).

[dgtlmoon]

Is there a reason each test/test_*.py is run separately? Pytest autodiscovers those files by itself, so instead of

It seems to fail, can you provide a PR for this @lkubb ?

[dgtlmoon]

I'de love to pick this one up again in the next few months!

[dgtlmoon]

Also good to add https://docs.kernel.org/userspace-api/no_new_privs.html to docker-compose.yml

    security_opt:
      - no-new-privileges:true

[dgtlmoon]

https://github.com/docker-library/python/issues/20