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

Include SYNC_USER1 in syncserver Dockerfile as documentation #3715

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

omarkohl
Copy link
Contributor

@omarkohl omarkohl commented Jan 11, 2025

Note: I recommend reviewing only the second commit of this PR since the first one is a separate PR #3714 . I will rebase things as needed. (a.k.a. stacked diffs).

Graphical Docker tools frequently display the exposed ENV variables in the UI so it's user-friendly to display this to show the user it can (and should) be set.

PUID and PGID are optional env variables to specify the user and group id of
the user that the anki-sync-server process should run with.

This gives more flexibility for solving permission problems with volumes and is
a common pattern for Docker images (e.g. see here:
https://docs.linuxserver.io/general/understanding-puid-and-pgid/)

The anki-sync-server process will write any files with the permissions of the
user it's running with, which can be a problem when you need to access those
files from outside the container or when they are being written into a bind
mount that is owned by a particular user on the host system.

To be able to implement this the entrypoint.sh needs to run as root (since it
needs to create a user and change file permissions). anki-sync-server then
needs to be started with the user 'anki', which is why the new dependency
'su-exec' is required. The user 'anki' and group 'anki-group' can no longer be
created at image build time because then their ids would be fixed.

Also update the build instructions to require building the Docker image inside
the directory where the Dockerfile resides since the build now needs to copy
the entrypoint.sh and it seems wrong the specify the path
docs/syncserver/entrypoint.sh inside the Dockerfile.
Graphical Docker tools frequently display the exposed ENV variables in the UI
so it's user-friendly to display this to show the user it can (and should) be
set.
@dae
Copy link
Member

dae commented Jan 12, 2025

I'm not really keen on the idea of having a default password set, as it's not great security practice. If you must include something in the dockerfile, perhaps we should have code on the server end that refuses to run when the default password hasn't been changed.

@jeankhawand heads up

@omarkohl
Copy link
Contributor Author

I agree that there is risk associated with setting a default password.

Preventing the server from starting when the default password is used would be worse than discarding this PR in my opinion, since it would require users for whom the container is not starting to be able to read and correctly interpret the log output of the container, which is even more of a hassle than reading the documentation to figure out how to set a user and password.

The reason why I tend to think that in this case the default password is fine:

  • The main use case for this container is private / at home usage. It seems unlikely (though not impossible!) that someone would run it completely unprotected in a public network.
  • A user has to type in the password into their Anki devices before being able to sync. The password is "veryinsecure!" for a reason, namely to make them reconsider changing that password.

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.

2 participants