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

Two remarks about the Docker container: Drop root privileges #66

Closed
lablans opened this issue Jul 9, 2018 · 9 comments
Closed

Two remarks about the Docker container: Drop root privileges #66

lablans opened this issue Jul 9, 2018 · 9 comments
Labels
documentation Documentation is missing enhancement New feature or request

Comments

@lablans
Copy link

lablans commented Jul 9, 2018

I've got two suggestions about @mprasil's Docker container:

  • It's currently running as root, which is undesirable from a security standpoint. Could the container drop root privileges?
  • When issuing a docker stop command, the container won't stop gracefully but wait to be killed after the default timeout of 10 secs and then quit with a non-zero exit code. Could the container react properly to stop commands?

Many thanks to @mprasil for the container and, of course, @dani-garcia for the Rust implementation!

@mprasil
Copy link
Contributor

mprasil commented Jul 10, 2018

Hi, thanks for suggestions.

Dropping the root privileges

This is probably a good idea, however there are two things that you need to make sure:

  1. That you can write to your data folder. For this you can just run the container and provide some user ID that can write to the mapped data folder:
  --user=99 # or whatever used id you have as an owner of the data folder
  1. That you can listen on the specified port. By default this is port 80, which requires root. You can specify different port and then map that port to the outside:
  -e ROCKET_PORT=8080 -p 80:8080

This way you can run your container with non-root user. Now doing this as a default is a bit complicated as it complicates the usage for not so big benefit in many cases. (remember a lot of people run this only internally plus the root user inside the container is already quite limited in what it can do) Also this would break the setup for people already using the image. So I'd probably document the above to explain how to do this if you really want, but leave the root as default. Hope that makes sense.

Graceful shutdown

I believe this is depending on this issue in the rocket to be resolved. We can change the STOPSIGNAL in the Dockerfile to maybe kill straight away, but it won't be graceful anyways so I'm not sure if that makes sense? I'd say it might be worth doing it now, but we need to monitor the upstream issue to make the shutdown graceful once it's resolved.

@mprasil mprasil added documentation Documentation is missing enhancement New feature or request labels Jul 10, 2018
@lablans
Copy link
Author

lablans commented Jul 10, 2018

Many thanks, @mprasil! Running with lower privileges works fine:

# /usr/bin/docker run -ti --rm --name bitwarden -e ROCKET_PORT=8080 --user 1002 -v /srv/bitwarden/:/data/ -p 127.0.0.1:32080:8080 mprasil/bitwarden:latest

@mprasil
Copy link
Contributor

mprasil commented Jul 11, 2018

I've created the PR to add the info to documentation. As for the graceful shutdown, we can revisit that once the upstream bug is fixed.

dani-garcia added a commit that referenced this issue Jul 11, 2018
Document running container with lower privileges (fixes #66)
@lablans
Copy link
Author

lablans commented Jul 11, 2018

Many thanks!

@cybis
Copy link

cybis commented Apr 14, 2019

It seems that there is still no gracefull shutdown, docker ps -a still shows exit code 137 for my bitwarden container after trying to stop it. I was wondering what (potential) impact this has on the database? What if the SIGKILL signal is received while a database operation is still being performed? Can the data get corrupted?

@dani-garcia
Copy link
Owner

If there is any transaction in progress, then that info might be lost, but the database shouldn't be corrupted by the process getting killed. It's not the best solution, but it'll have to work for now.

Rocket's plan for the next release was to update to the next hyper release to enable asynchronous functionality, the blocker for the graceful shutdown is precisely that rocket is using an older hyper version, so hopefully this won't be a problem soon.

@cybis
Copy link

cybis commented Apr 14, 2019

Thanks a lot for the update. Currently rocket 0.4.0 seems to be used, let's see if the next release fixes it.

@lowne
Copy link

lowne commented Jan 19, 2020

Graceful shutdown

Given #246 I didn't want to open another issue, however I'd like to have an (open) issue to refer to. I had a look and the upstream dependency tree grows to infinity and beyond (even config files!), so I don't think this will be solved (in rocket) anytime soon. By the way,

We can change the STOPSIGNAL in the Dockerfile

seems like a good idea, no reason to block who-knows-what downstream for 10 seconds for no benefit whatsoever.

So, it'd be swell if this issue could be reopened for tracking purposes, and the title edited to "graceful shutdown and root privileges" (the latter half, while trivial, kept for search).

EDIT: Hm, I only now noticed the close via doc-commit (about root), which is too bad as there's subsequent useful discussion here about the lack of graceful shutdown. Oh well; I suppose my request should then be disregarded.

@dani-garcia
Copy link
Owner

As you mention, the non-root setup is documented in the wiki: https://github.com/dani-garcia/bitwarden_rs/wiki/Running-the-server-with-non-root-user.

About graceful shutdown, that's coming in rockets new version, there's a preview available here:
https://github.com/dani-garcia/bitwarden_rs/tree/async and the docker image bitwardenrs/server:test_async (only x86 for now)

thelittlefireman pushed a commit to thelittlefireman/bitwarden_rs that referenced this issue Mar 19, 2021
thelittlefireman pushed a commit to thelittlefireman/bitwarden_rs that referenced this issue Mar 19, 2021
Document running container with lower privileges (fixes dani-garcia#66)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Documentation is missing enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants