-
-
Notifications
You must be signed in to change notification settings - Fork 100
Support rootless (backwards compatible) #110
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
base: master
Are you sure you want to change the base?
Conversation
the fake terminal is not compatible with rootless, and isn't necessary anymore - i tested it. |
the containers are available here https://github.com/simonfelding/protonmail-bridge-docker/pkgs/container/protonmail |
Thanks for working on this. Will you also update the README (and maybe docker-compose.yml) to show how to run it as non-root? |
Hi there, this PR is quite important as currently the auto update mechanism breaks existing containers. This PR has been open for 3 months now, is there anything that must be done for this to be merged? Testing? Documentation? I'd love to help out. |
I'll take a look this weekend, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Built with:
$ docker buildx build --build-arg version=$(cat ../VERSION) -t protonmail-bridge:$(cat ../VERSION)_pr110
Then I ran with a previously initialized volume, which had its content owned by user UID/GID 8535:
$ docker stop protonmail-bridge ; docker rm protonmail-bridge ; docker run --user 8535:8535 -d --name=protonmail-bridge -v protonmail:/home/protonmail -e HOME=/home/protonmail -p 127.0.0.1:1025:2025/tcp -p 127.0.0.1:1143:2143/tcp --restart=unless-stopped protonmail-bridge:v3.19.0_pr110
That worked fine.
I did see this warning in the container logs:
WARN[May 3 18:37:31.911] Failed to add test credentials to keychain error="failed to open dbus connection: exec: \"dbus-launch\": executable file not found in $PATH" helper="*keychain.SecretServiceDBusHelper"
but that doesn't seem to prevent the bridge from working.
# give friendly error if the user doesn't own the data | ||
if [[ $(id -u) != 0 ]]; then | ||
if [[ `find $HOME/.* -not -user $(id -u) | wc -l` != 0 ]]; then | ||
echo "You do not own the data in $HOME. Please chown it to $(id -u), run the container as the owner of the data or run the container as root." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: "You do not own some or all of the data in $HOME" and "recursively chown it"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that's better.
fi | ||
|
||
# delete lock files if they exist - this can happen if the container is restarted forcefully | ||
find $HOME -name "*.lock" -delete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there might be a chance of two containers mounting the same filesystems, and the other one still running?
It might make sense to ask the user to confirm before deleting, or print a message telling them what to do instead of silently deleting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. Can you help with a contribution for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure:
find $HOME -name "*.lock" -delete | |
mapfile -t lockfiles < <(find $HOME -name "*.lock" ) | |
if [ ${#lockfiles[@]} -gt 0 ]; then | |
echo "WARNING: found ${#lockfiles[@]} lock file(s): ${lockfiles[@]}" | |
echo " Make sure no other container proton-bridge are running (e.g. in another container)." | |
read -p " Press Y to delete the lockfiles and continue, or anything else to abort: " REPLY | |
if [ "$REPLY" = Y ]; then | |
# Quote all filenames for safety, e.g. in case of spaces in filenames: | |
for LOCKFILE in "${lockfiles[@]}" ; do | |
rm "$LOCKFILE" | |
done | |
else | |
echo "user aborted." | |
exit 1 | |
fi | |
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm quite interested in a rootless setup for that container, so @simonfelding if you could review the changes it would make it easier for users to just checkout that PR instead of having to do the diffs :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the "Y" should be case insensitive during that prompt, would be a bit better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I can't use docker compose up
because it prompts me for a Y
so I believe we should add an env var to enable automatically deleting lockfiles
@@ -12,9 +12,13 @@ RUN bash /install.sh | |||
FROM debian:sid-slim | |||
LABEL maintainer="Simon Felding <[email protected]>" | |||
|
|||
# These are only exported if running as root |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure "exported" is the correct word. Maybe you meant "used"?
|
||
# give friendly error if you don't have protonmail data | ||
if [[ `find $HOME | wc -l` == 1 ]]; then # 1 because find $HOME will always return $HOME | ||
echo 'Protonmail does not seem to have been initialized yet. Enter the container with something like `docker exec -it <container_name>` and type "help" for instructions on how to set up the ProtonMail Bridge' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not clear. I suggest this:
echo 'Protonmail does not seem to have been initialized yet. For instructions on how to set up the ProtonMail Bridge, enter the container with something like `docker exec -it protonmail-bridge help` or `docker compose run protonmail-bridge help`'
Edit: oh you meant to type help
in the console right? For some reason the --cli help
just showed the man page in the docker logs so maybe the bridge's cli changed with updates or something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to use expect
to automatically add the add
inside the console? We could even use docker secrets
to store the protonmail account info just during the init.
echo "Starting ProtonMail Bridge. Connect to the CLI with `docker exec -it <container_name>` and type 'help' for instructions." | ||
/protonmail/proton-bridge --cli $@ | ||
echo "ProtonMail bridge stopped. waiting 30 seconds before exiting in order to preserve the logs." | ||
sleep 30 # so we have time to read the logs in case of a crash loop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wouldn't it be better to delete any remaining lock files at the end of that script too? For some reason the bridge seems to not clean those sometimes for me. But in any case at the last line of the entrypoint any remaining lock file has to have been forgotten and can be deleted safely no?
Wouldn't it be better to use a read only filesystem and drop all capabilities? That would increase security |
| .Settings.Autostart = false | ||
| .Settings.SMTPPort = 1025 | ||
| .Settings.IMAPPort = 1143 ' \ | ||
| /protonmail/vault-editor write |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is that future proof? I believe using expect
to specify the ports, disabling telemetry etc would be way better as the user facing shell has less chance of suffering breaking changes than the internal proton bridge json.
Btw there is |
Running as root is now optional.