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

Bitwarden does not react on SIGTERM (v1.13.1 sqlite) #905

Closed
mkilchhofer opened this issue Mar 14, 2020 · 11 comments
Closed

Bitwarden does not react on SIGTERM (v1.13.1 sqlite) #905

mkilchhofer opened this issue Mar 14, 2020 · 11 comments
Labels
enhancement New feature or request low priority Won't fix anytime soon, but will accept PR if provided Third party Needs an update from the third party library

Comments

@mkilchhofer
Copy link
Contributor

mkilchhofer commented Mar 14, 2020

Subject of the issue

At least the sqlite flavor of bitwarden_rs does not react on SIGTERM. I already wondered why evicting kubernetes nodes takes longer when bitwarden_rs runs on this node.
I now determined that the binary /bitwarden_rs inside the sqlite flavored docker image does not react on a SIGTERM signal.

Your environment

  • Bitwarden_rs version: 1.13.1
  • Install method: Docker image (sqlite flavor)
  • Clients used: n/a
  • Reverse proxy and version: n/a
  • Version of mysql/postgresql: n/a
  • Other relevant information: n/a

Steps to reproduce / Actual behaviour

Terminate the docker Image takes long and the exist status it not 0:

[root@localhost ~]# time podman stop bitwardenrs 
153e82e8be39928e69f61500a895a26d166041351f9d9076f00bbadc10ac41f4

real	0m10.252s
user	0m0.079s
sys	0m0.075s

[root@localhost ~]# podman ps -a 
CONTAINER ID  IMAGE                                COMMAND        CREATED         STATUS                       PORTS                   NAMES
153e82e8be39  docker.io/bitwardenrs/server:1.13.1  /bitwarden_rs  16 minutes ago  Exited (137) 50 seconds ago                          bitwardenrs

Another attempt to check if the binary handles SIGTERM:

  1. Start the docker image again (podman start bitwardenrs)
  2. Start a bash shell inside the container context (podman exec -ti bitwardenrs bash)
  3. Update apt and install procps package
root@153e82e8be39:/# apt update && apt install -y procps
  1. Send SIGTERM to the running binary:
root@153e82e8be39:/# ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 21:33 ?        00:00:00 /bitwarden_rs
root          24       0  0 21:34 pts/0    00:00:00 bash
root         521      24  0 21:35 pts/0    00:00:00 ps -ef

root@153e82e8be39:/# kill -SIGTERM 1
root@153e82e8be39:/# 

Nothing happens, I expect, that bitwarden starts to teardown.

Expected behaviour

Bitwarden_rs binary implements a signal handler and handles SIGTERM correctly

Relevant logs

[root@localhost ~]# podman logs -f -t bitwardenrs
2020-03-14T22:13:24.288732634+01:00 /--------------------------------------------------------------------\
2020-03-14T22:13:24.288732634+01:00 |                       Starting Bitwarden_RS                        |
2020-03-14T22:13:24.288732634+01:00 |                           Version 1.13.1                           |
2020-03-14T22:13:24.288732634+01:00 |--------------------------------------------------------------------|
2020-03-14T22:13:24.289065999+01:00 | This is an *unofficial* Bitwarden implementation, DO NOT use the   |
2020-03-14T22:13:24.289065999+01:00 | official channels to report bugs/features, regardless of client.   |
2020-03-14T22:13:24.289065999+01:00 | Report URL: https://github.com/dani-garcia/bitwarden_rs/issues/new |
2020-03-14T22:13:24.289065999+01:00 \--------------------------------------------------------------------/
2020-03-14T22:13:24.289065999+01:00 
2020-03-14T22:13:24.311394678+01:00 [2020-03-14 21:13:24][bitwarden_rs][INFO] JWT keys don't exist, checking if OpenSSL is available...
2020-03-14T22:13:24.317867576+01:00 OpenSSL 1.1.1d  10 Sep 2019
2020-03-14T22:13:24.318256898+01:00 [2020-03-14 21:13:24][bitwarden_rs][INFO] OpenSSL detected, creating keys...
2020-03-14T22:13:24.325024560+01:00 Generating RSA private key, 2048 bit long modulus (2 primes)
2020-03-14T22:13:24.404630301+01:00 .........................................+++++
2020-03-14T22:13:24.430492846+01:00 ...........+++++
2020-03-14T22:13:24.431083510+01:00 e is 65537 (0x010001)
2020-03-14T22:13:24.439687964+01:00 writing RSA key
2020-03-14T22:13:24.445086436+01:00 writing RSA key
2020-03-14T22:13:24.445596102+01:00 [2020-03-14 21:13:24][bitwarden_rs][INFO] Keys created correctly.
2020-03-14T22:13:24.451183193+01:00 Running migration 20180114171611
2020-03-14T22:13:24.455337694+01:00 Running migration 20180217205753
2020-03-14T22:13:24.459399492+01:00 Running migration 20180427155151
2020-03-14T22:13:24.464446097+01:00 Running migration 20180508161616
2020-03-14T22:13:24.467767809+01:00 Running migration 20180525232323
2020-03-14T22:13:24.471871810+01:00 Running migration 20180601112529
2020-03-14T22:13:24.474783802+01:00 Running migration 20180711181453
2020-03-14T22:13:24.478268008+01:00 Running migration 20180827172114
2020-03-14T22:13:24.481535714+01:00 Running migration 20180910111213
2020-03-14T22:13:24.484098757+01:00 Running migration 20180919144557
2020-03-14T22:13:24.486938657+01:00 Running migration 20181127152651
2020-03-14T22:13:24.490226826+01:00 Running migration 20190526216651
2020-03-14T22:13:24.512066536+01:00 Running migration 20191010083032
2020-03-14T22:13:24.516511527+01:00 Running migration 20191117011009
2020-03-14T22:13:24.536860468+01:00 [2020-03-14 21:13:24][start][INFO] Rocket has launched from http://0.0.0.0:80

// here is the second start

2020-03-14T22:33:08.693269989+01:00 /--------------------------------------------------------------------\
2020-03-14T22:33:08.693269989+01:00 |                       Starting Bitwarden_RS                        |
2020-03-14T22:33:08.693269989+01:00 |                           Version 1.13.1                           |
2020-03-14T22:33:08.693269989+01:00 |--------------------------------------------------------------------|
2020-03-14T22:33:08.693269989+01:00 | This is an *unofficial* Bitwarden implementation, DO NOT use the   |
2020-03-14T22:33:08.693269989+01:00 | official channels to report bugs/features, regardless of client.   |
2020-03-14T22:33:08.693269989+01:00 | Report URL: https://github.com/dani-garcia/bitwarden_rs/issues/new |
2020-03-14T22:33:08.693269989+01:00 \--------------------------------------------------------------------/
2020-03-14T22:33:08.693269989+01:00 
2020-03-14T22:33:08.766893584+01:00 [2020-03-14 21:33:08][start][INFO] Rocket has launched from http://0.0.0.0:80

// here I sent a TERM signal with "kill -SIGTERM 1"
// bitwarden_rs continues to run and does not log something that it captured any signal
@mkilchhofer
Copy link
Contributor Author

@dani-garcia
Copy link
Owner

Indeed the current version of the rocket web server we use doesn't support any kind of graceful shutdow.

The new async version in development does have the capabilities to do it and I have a separate branch using it: https://github.com/dani-garcia/bitwarden_rs/tree/async

@BlackDex BlackDex added the enhancement New feature or request label Oct 9, 2020
@BlackDex BlackDex added low priority Won't fix anytime soon, but will accept PR if provided Third party Needs an update from the third party library labels Nov 18, 2020
@mfriedenhagen
Copy link

Another solution could be to use tiny-init or dumb-init. these are meant as small init replacements and handle signals correctly. See https://github.com/Yelp/dumb-init for a complete explanation.

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Jan 2, 2021

@mfriedenhagen no, that would not help. An init "system" like you mentioned would help when a main process forks child process(es) and does not control the lifecycle of the childs.

The problem is that the whole /bitwarden_rs process does not catch any signal. There are no childs as I observe in normal operation. Maybe there are childs while creating an sqlite backup.

@mfriedenhagen
Copy link

Hello, when reading https://github.com/Yelp/dumb-init's README it is my understanding that the Linux kernel does not send SIGTERM to PID 1 at all if no explicit handler is implemented. If dumb-init would start bitwarden_rs the standard SIGTERM handler would terminate bitwarden_rs as expected.

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Jan 2, 2021

Okay, we can simulate this without building a new container image with podman and docker when starting the container with the option --init.
Indeed this helps stopping the container almost with no "lag":

[mkilchhofer@localhost ~]$ docker run --name bw_rs-init --init --rm docker.io/bitwardenrs/server:1.13.1
/--------------------------------------------------------------------\
|                       Starting Bitwarden_RS                        |
|                           Version 1.13.1                           |
|--------------------------------------------------------------------|
| This is an *unofficial* Bitwarden implementation, DO NOT use the   |
| official channels to report bugs/features, regardless of client.   |
| Report URL: https://github.com/dani-garcia/bitwarden_rs/issues/new |
\--------------------------------------------------------------------/

[2021-01-02 11:02:04][bitwarden_rs][INFO] JWT keys don't exist, checking if OpenSSL is available...
OpenSSL 1.1.1d  10 Sep 2019
[2021-01-02 11:02:04][bitwarden_rs][INFO] OpenSSL detected, creating keys...
Generating RSA private key, 2048 bit long modulus (2 primes)
...........................................................................+++++
..............................+++++
e is 65537 (0x010001)
writing RSA key
writing RSA key
[2021-01-02 11:02:05][bitwarden_rs][INFO] Keys created correctly.
Running migration 20180114171611
(...)
Running migration 20191117011009
[2021-01-02 11:02:05][start][INFO] Rocket has launched from http://0.0.0.0:80
^C[mkilchhofer@localhost ~]$ 
[mkilchhofer@localhost ~]$ 

As you can see, when we Press CTRL+C the container stops.
To prove this we can see the output of ps -ef inside the container while it's running:

[mkilchhofer@localhost ~]$ docker exec -ti bw_rs-init bash
root@a05b79e3eeb0:/# apt update && apt install -y procps

root@a05b79e3eeb0:/# ps -ef
UID          PID    PPID  C STIME TTY          TIME CMD
root           1       0  0 11:10 ?        00:00:00 /sbin/docker-init -- /bitwarden_rs
root           6       1  0 11:10 ?        00:00:00 /bitwarden_rs
root          25       0  0 11:11 pts/0    00:00:00 bash
root         357      25  0 11:11 pts/0    00:00:00 ps -ef

And without the option, we are unable to stop the container with CTRL+C:

[mkilchhofer@localhost ~]$ docker run --name bw_rs --rm docker.io/bitwardenrs/server:1.13.1
/--------------------------------------------------------------------\
|                       Starting Bitwarden_RS                        |
|                           Version 1.13.1                           |
|--------------------------------------------------------------------|
| This is an *unofficial* Bitwarden implementation, DO NOT use the   |
| official channels to report bugs/features, regardless of client.   |
| Report URL: https://github.com/dani-garcia/bitwarden_rs/issues/new |
\--------------------------------------------------------------------/

[2021-01-02 11:02:21][bitwarden_rs][INFO] JWT keys don't exist, checking if OpenSSL is available...
OpenSSL 1.1.1d  10 Sep 2019
[2021-01-02 11:02:21][bitwarden_rs][INFO] OpenSSL detected, creating keys...
Generating RSA private key, 2048 bit long modulus (2 primes)
..................+++++
............................................+++++
e is 65537 (0x010001)
writing RSA key
writing RSA key
[2021-01-02 11:02:21][bitwarden_rs][INFO] Keys created correctly.
Running migration 20180114171611
(...)
Running migration 20191117011009
[2021-01-02 11:02:22][start][INFO] Rocket has launched from http://0.0.0.0:80
^C^C^C^C

What we do not know is what's going on inside the bitwarden_rs process. It seems that the framework rocket still does not support graceful shutdown in the mainline version.

@BlackDex
Copy link
Collaborator

BlackDex commented Jan 2, 2021

As stated in the comment from @dani-garcia, this is not yet in the master branch. It is only in the async version, which we will probably release shortly once rocket releases there new version as a stable release so that we know there will be no to minimal breaking changes made. Untill then, run bitwarden_rs using docker run -d which will detach itself, and you can stop it via docker stop.

@BlackDex
Copy link
Collaborator

BlackDex commented Jan 2, 2021

Also, i see that you are still using a very old version.
We currently are on v1.18 and not v1.13

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Jan 2, 2021

There are some more things I observed, when I terminate a detached bitwarden conainer which docker stop:

[mkilchhofer@localhost ~]$ docker run --name bw_rs-init --init --detach docker.io/bitwardenrs/server:1.13.1
e750910edeb190bb9e5a672d4563b4812bc36fd8fdb7bdd4a5e18c61932b6878

[mkilchhofer@localhost ~]$ docker ps -a
CONTAINER ID        IMAGE                       COMMAND             CREATED             STATUS                             PORTS               NAMES
e750910edeb1        bitwardenrs/server:1.13.1   "/bitwarden_rs"     12 seconds ago      Up 12 seconds (health: starting)   80/tcp, 3012/tcp    bw_rs-init

[mkilchhofer@localhost ~]$ docker stop bw_rs-init
bw_rs-init
[mkilchhofer@localhost ~]$ docker ps -a
CONTAINER ID        IMAGE                       COMMAND             CREATED             STATUS                      PORTS               NAMES
e750910edeb1        bitwardenrs/server:1.13.1   "/bitwarden_rs"     24 seconds ago      Exited (143) 1 second ago                       bw_rs-init

We see here, that the return code is 143. From my java experience it looks like we also need to substract 128 and then we get the real 15 return code which is SIGTERM.

@BlackDex yes I know I used an old version in the comment above. I would like to reproduce this with the same version as the issue was created.
I think we indeed could enhance the shutdown experience by adding dumb-init or tiny-init to the Dockerfile. WDYT @BlackDex and @dani-garcia ?

@mkilchhofer
Copy link
Contributor Author

mkilchhofer commented Jan 2, 2021

The enhancement would be that simple:

diff --git a/docker/amd64/Dockerfile b/docker/amd64/Dockerfile
index 75ee4dd..7edb32b 100644
--- a/docker/amd64/Dockerfile
+++ b/docker/amd64/Dockerfile
@@ -78,6 +78,7 @@ RUN apt-get update && apt-get install -y \
     openssl \
     ca-certificates \
     curl \
+    dumb-init \
     sqlite3 \
     libmariadb-dev-compat \
     libpq5 \
@@ -101,5 +102,6 @@ HEALTHCHECK --interval=60s --timeout=10s CMD ["/healthcheck.sh"]
 
 # Configures the startup!
 WORKDIR /
+ENTRYPOINT ["/usr/bin/dumb-init", "--"]
 CMD ["/start.sh"]
 
diff --git a/docker/amd64/Dockerfile.alpine b/docker/amd64/Dockerfile.alpine
index 3ede25b..675c28b 100644
--- a/docker/amd64/Dockerfile.alpine
+++ b/docker/amd64/Dockerfile.alpine
@@ -74,6 +74,7 @@ ENV SSL_CERT_DIR=/etc/ssl/certs
 RUN apk add --no-cache \
         openssl \
         curl \
+        dumb-init \
         sqlite \
         postgresql-libs \
         ca-certificates
@@ -96,5 +97,6 @@ HEALTHCHECK --interval=60s --timeout=10s CMD ["/healthcheck.sh"]
 
 # Configures the startup!
 WORKDIR /
+ENTRYPOINT ["/usr/bin/dumb-init", "--"]
 CMD ["/start.sh"]
 

@BlackDex
Copy link
Collaborator

This issue is solved now for all images.
@mkilchhofer added dumb-init which catches all the kill signals and handles then correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low priority Won't fix anytime soon, but will accept PR if provided Third party Needs an update from the third party library
Projects
None yet
Development

No branches or pull requests

4 participants