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

Dockerfile: Explictly create volume paths and set permissions #909

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

mbrase
Copy link
Contributor

@mbrase mbrase commented Jun 22, 2022

Checklist

  • All new and existing tests are passing
  • (If adding features:) I have added tests to cover my changes
  • (If docs changes needed:) I have updated the documentation accordingly.
  • I have added an entry to CHANGES.rst because this is a user-facing change or an important bugfix
  • I have written proper commit message(s)

What changes does this Pull Request introduce?

This change explicitly creates the volume mount directories in the docker image and gives them global write permissions (1777).

Why is this necessary?

This allows the docker images to be use named/anonymous volumes (e.g -v /db or -v isso-db:/db) with non-root users (e.g. --user=2000:2000).

For example:

$ mkdir config
$ sed 's,comments.db,/db/\0,g' contrib/isso.sample.cfg > config/isso.cfg 
$ docker run --user 2000:2000 --rm --name isso -p 127.0.0.1:8080:8080 \ 
    -v $PWD/config:/config -v /db ghcr.io/isso-comments/isso:0.13.0
Traceback (most recent call last):
  File "/isso/bin/gunicorn", line 8, in <module>
    sys.exit(run())
  File "/isso/lib/python3.10/site-packages/gunicorn/app/wsgiapp.py", line 67, in run
    WSGIApplication("%(prog)s [OPTIONS] [APP_MODULE]").run()
  File "/isso/lib/python3.10/site-packages/gunicorn/app/base.py", line 231, in run
    super().run()
  File "/isso/lib/python3.10/site-packages/gunicorn/app/base.py", line 72, in run
    Arbiter(self).run()
  File "/isso/lib/python3.10/site-packages/gunicorn/arbiter.py", line 58, in __init__
    self.setup(app)
  File "/isso/lib/python3.10/site-packages/gunicorn/arbiter.py", line 118, in setup
    self.app.wsgi()
  File "/isso/lib/python3.10/site-packages/gunicorn/app/base.py", line 67, in wsgi
    self.callable = self.load()
  File "/isso/lib/python3.10/site-packages/gunicorn/app/wsgiapp.py", line 58, in load
    return self.load_wsgiapp()
  File "/isso/lib/python3.10/site-packages/gunicorn/app/wsgiapp.py", line 48, in load_wsgiapp
    return util.import_app(self.app_uri)
  File "/isso/lib/python3.10/site-packages/gunicorn/util.py", line 359, in import_app
    mod = importlib.import_module(module)
  File "/usr/local/lib/python3.10/importlib/__init__.py", line 126, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
  File "<frozen importlib._bootstrap>", line 1050, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1027, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1006, in _find_and_load_unlocked
  File "<frozen importlib._bootstrap>", line 688, in _load_unlocked
  File "<frozen importlib._bootstrap_external>", line 883, in exec_module
  File "<frozen importlib._bootstrap>", line 241, in _call_with_frames_removed
  File "/isso/isso/run.py", line 16, in <module>
    application = make_app(
  File "/isso/isso/__init__.py", line 185, in make_app
    isso = App(conf)
  File "/isso/isso/__init__.py", line 101, in __init__
    self.db = db.SQLite3(conf.get('general', 'dbpath'), conf)
  File "/isso/isso/db/__init__.py", line 32, in __init__
    rv = self.execute([
  File "/isso/isso/db/__init__.py", line 59, in execute
    with sqlite3.connect(self.path) as con:
sqlite3.OperationalError: unable to open database file

The reason that this change fixes the issue is that empty docker named/anonymous volumes are prepopulated with the container contents at the mount path, including owners and permissions. Since /db does not exist a volume mounted there will use default owner of root:root and permissions 0755, which is not writeable by a non-root user.

Note that this does not change the behavior of bind mounts, which always retain the permissions of the host filesystem.

This allows running the docker image with a non-root user (--user 2000)
with named/anonymous volumes (-v isso-db:/db).

When an empty volume is mounted into a container (non-bind mount), it is
prepopulated with the files at the mount path, including owners and
permissions. The issue is that since /db does not exist at docker build
time, the volume defaults to root ownership with 0755 permissions, which
prevents non-root users from creating the sqlite database. This change
explicitly creates the /config and /db directories with 1777 permissions
to allow any user to write to them.

Note that this has no impact on bind mounts, since these always use
permissions of the host filesystem.
@BBaoVanC
Copy link
Contributor

I think setting the permissions to 777 is a bit much. I know there's other images I've used that work fine when using a non-root container, maybe we'll have to figure out how they work

@mbrase
Copy link
Contributor Author

mbrase commented Jun 22, 2022

Thanks for the feedback @BBaoVanC! I chose the 1777 permissions based on some other docker images I use which allow using --user with docker volumes, but I'm open to alternatives if you are aware of other solutions. For example:

I'll also note that the sticky bit (the 1 in 1777) is significant here and makes this more secure. The directory sticky bit prevents other users from deleting or renaming files that they do not own (which is normally allowed by 0777), but still allows them to create files. On most Linux distros, they use 1777 for the /tmp directory permissions.

@BBaoVanC
Copy link
Contributor

Thanks for the feedback @BBaoVanC! I chose the 1777 permissions based on some other docker images I use which allow using --user with docker volumes, but I'm open to alternatives if you are aware of other solutions. For example:

* [MYSQL](https://github.com/docker-library/mysql/blob/32aecb725b28afa043a5e879977a7947dfb64c14/8.0/Dockerfile.debian#L85)

* [Ghost](https://github.com/docker-library/ghost/blob/792a7dede4645f80fa0f3022303d12d156b5db80/5/debian/Dockerfile#L77)

I'll also note that the sticky bit (the 1 in 1777) is significant here and makes this more secure. The directory sticky bit prevents other users from deleting or renaming files that they do not own (which is normally allowed by 0777), but still allows them to create files. On most Linux distros, they use 1777 for the /tmp directory permissions.

Ah that makes more sense then. I guess this is fine, since the 1777 permissions will only apply to the top level directory, not any of the data that's inside.

@ix5
Copy link
Member

ix5 commented Jun 27, 2022

Hey @mbrase, thanks for the PR! I'm no Docker expert, but the justifications for this change look convincing.

Would you mind solving a few of the issues raised in #858 while you're at it?

@ix5
Copy link
Member

ix5 commented Jul 6, 2022

@mbrase if you don't currently have enough free time to overhaul the whole Dockerfile, I'm fine with merging this as-is.

@mbrase
Copy link
Contributor Author

mbrase commented Jul 11, 2022

I apologize for the delayed response, I've been traveling lately. I am not much of a docker expert myself, and am a bit intimidated with overhauling the entire Dockerfile. I'd prefer to just merge this before it gets stale.

@ix5 ix5 merged commit 8d0f08a into isso-comments:master Jul 11, 2022
@ix5
Copy link
Member

ix5 commented Jul 11, 2022

Thanks, no worries.

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