Unified server side sessions for JI and API #6403

lsd-cat · 2022-04-18T08:34:20Z

Status

Design phase ended
Coding pahse ended
Test plan

Summary of behavioral changes

Reminder: the changes apply only to the Journalist Interface and Journalist API, NOT to the Source Interface

Change	Before	After	Comment
GUI Session expiration	120 minutes - Renewable unlimited times upon any request	120 minutes - Renewable up to 5 times upon any request in the last half hour of validity
API Session expiration	8 hours - Nonrenewable	120 minutes - Renewable up to 5 times upon any request in the last half hour of validity	Uses same backend and mechanism as the JI.It is renewed automatically, so the client does not need to issue any 'renew' or 'refresh' request. In the last half an hour of its validity, if any request is made, token life is extended server-side.
Session info storage JI	Flask signed session cookie, the payload is fully client-side, no server state	Signed cookie with a session identifier. The identifier matches a record in Resid with the actual session payload	Sessions are now stored server-side with all the consequences of this.
Session info storage API	The signed JWT token contains the signed payload, namely the authenticated user ID and the expiration.	The token is now the signed session identifier with the same backend as the JI.	API/JI session tokens are not exchangeable as they use different prefixes in Redis and different signing salts.
API token request	The current API request to issue a token is not subject to any change.	N/A	The expiration is the initial 2 hours expiration. The behavior can be easily changed, but it seems not to be used by securedrop-client anyway.
Cookie signing	Cookies are signed to ensure their integrity.	Cookies are signed to prevent usage of non-issued server tokens (weird Redis payloads, leaked ids from Redis, etc).	In general, this is not enforced by most server-side implementations. As it has little overhead, looks like a reasonable hardening.

Additional changes to the `g` object

See #6428

Description of Changes

This commit attempts to completely redesign the session mechanism for the Journalist Interface and the Journalist API in order to unify the two backends and behaviors, drop some dependencies, and have more control overall.

Currently, the JI relies on built-in Flask sessions for user authentication. They rely on using a session cookie with a signed payload using the app SECRET_KEY. These session cookies are renewable and non-revokable unless changing the SECRET_KEY. It is impossible to know how many sessions are there and destroy them server-side. Current logout functionality just destroys the cookie in the browser.
The API uses issues a non-renewable JWT token upon login. Upon logout, the token is placed in a database table for revocation until its expiration. For each authenticated API endpoint, a decorator is used to enforce the check of such token. Upon internal discussions, we have concluded that we are not benefiting from any of the benefits of JWT as a delegation mechanism in the project but we are keeping the burden of maintaining two session backends. Furthermore, itsdangerous has deprecated its JWT signing code.

The changes here move all the sessions from the client-side to the server-side. Sessions are now stored in Redis, and the auth cookie only contains a random string. The session identifier is still signed server-side and verified at each request: this is to prevent that a read primitive in Redis could allow an attacker to steal sessions; with this hardening, the attacker would also need the knowledge of the SECRET_KEY (or of course steal a valid cookie from a logged-in user). No major changes for the JI, while for the API the decorator is dropped, as well as the whole JWT mechanism and now behaves the same as the JI. Authentication on protected endpoints is enforced via the same approach of the JI, which is overall less error-prone than the decorator.
Both logouts now destroy sessions server-side. Before, on passphrase change, a nonce was incremented to invalidate past sessions for the affected user. That mechanism is dropped and sessions are simply deleted from the session store in that event.

Session lifetime is preset to 2 hours. Each session can be renewed a maximum of 5 times by simply using it 30 minutes before its expiration. Both values are configurable and can be tuned depending on the needs that arise from discussion or during testing.

No new dependencies are needed.
The database migration drops the nonce column for journalists and the revoked_tokens table for JWT revocation as well as the corresponding models.

Fixes

Fixes #6428.
Fixes #6224.
Fixes #204 (only for the JI, NOT for the SI).

Testing

For manual testing, a lot of manual logins/perform tasks/logout as well as for the API. We should test compatibility with securedrop-client and check if changes there are needed. The token API endpoint has not changed, as well as the authenticator header so we are not expecting big breakage.

Deployment

The new sessions library has some configuration options that could be added to config.py. However, since it has been written ad hoc for this project, the default settings are already the proper ones.

Source

https://github.com/fengsp/flask-session has been used as a starting point, albeit very little of it remains.

Test Plan (draft)

Here is a quick deaft for the test plan. Will be updated this week as I go through the changes one last time and do some more manual testing.

Test Plan

Migration

Database migration work (deletes the revoked token table)
Token cleanup procedures do not run/exists
* Reboot of the server/restart of redis logs destroy all sessions Persistant by Redis default config
Test new config with non standard values
Config migration works

Security

Session ids are random
Sessions id are signed

Invalid session id (non existant/wrong signature) gets ignored

Login to the JI
Tamper the cookie signature

Confirm that user is logged out

Functional

JI

Journalists can Login

Login to the JI

Confirm that the user is logged

Sessions of multiple users are separate

Login to the JI as user X in browser A
Login to the JI as user Y in browser B

Confirm that the two userss navigation does not interfere

Sessions are renewed in accordance with the new expiration/renewal policy

Login to the JI
Wait 2 hours

Confirm that user is logged out automatically
Confirm that an error message is shown

Login to the JI
Wait between 1.5h and 2h and refresh
Wait 1h

Confirm that user is still logged in

Logout works

Login to the JI as user X in browser A
Login to the JI as user Y in browser B
Logout from user X in browser A

Confirm that user A is logged out
Confirm that user B is still logged in

Deleting a user destroy all sessions of the user

Login to the JI as admin X in browser A
Login to the JI as admin/user Y in browser B
From admin X in browser A, delete user Y

Confirm that user Y in browser B is logged out

Changing a user password destroy all sessions of the user

Login to the JI as admin X in browser A
Login to the JI as admin/user Y in browser B
From admin X in browser A, change password of user Y

Confirm that user Y in browser B is logged out

Login to the JI as admin X
Change admin X password

Confirm that admin X is logged out on password change

API

Token are issues on succesful API login

Login to the /token endpoint

Confirm that the login is succesful

Use the token for an authenticated operation

Verify that the authenticated operation accepts the token

Send a request to the logout endpoint

Confirm that a message fo succesful logout is returned

Retry the operation at point 2.

Cionfirm that the operation at point 2 now requires a valid token

Integration

This changes should not affect the client, as the token format is exactly the same and token renewal will happen in the background transparently, however testing remains ncessary

SD Client

Login and sesison lifetime

Login through the client

Confirm that the client synchronizes succesfully

Session lifetime

We could need a bit of tuning here, what is going to happen is as follow:

The server will issue a token valid for two hours
As the client constantly tries to sync, as soon as the token hit ~1.30h of lifetime the token will be renewed for 2 additional hours.
At the ~3.30h mark, the token will renew again, and again at ~5.30, ~7.30, ~9.30. After that 5 renewal, the token lifetime ends and the session is destroyed. (max duration time would be 12h)
It is possible, that during testing phase we want to adjust this settings (5 renewals, 2h each, triggered at 30 minutes to session end)
Login through the client

Confirm that the token gets renewed and the session last the expected time.

securedrop/tests/test_journalist_utils.py

securedrop/tests/test_journalist_api.py

securedrop/journalist_app/utils.py

securedrop/journalist_app/admin.py

securedrop/journalist_app/api.py

securedrop/journalist_app/main.py

legoktm

As a general comment, you don't need to wrap types with quotes to turn them into strings, 95% just using the plain type is fine. They only need to be turned into strings if there's some recursive loading going on (like in models.py).

Other than that, I don't see any mypy failures, the one in CI is from pylint.

securedrop/journalist_app/__init__.py

securedrop/journalist_app/sessions.py

securedrop/journalist_app/utils.py

securedrop/alembic/versions/c5a02eb52f2d_dropped_session_nonce_from_journalist_.py

legoktm · 2022-04-18T18:47:12Z

re, mypy, the biggest cause of warnings is save_session types session as a SessionMixin rather than RedisSession. Switching that gets rid of most of the errors. There are also (at least) 2 cases where a field is typed as Optional[T] so we need to check it's not null first. Here's my diff:

diff --git a/securedrop/journalist_app/sessions.py b/securedrop/journalist_app/sessions.py
index 3811ec687..a202cbc61 100644
--- a/securedrop/journalist_app/sessions.py
+++ b/securedrop/journalist_app/sessions.py
@@ -18,7 +18,7 @@ class ServerSideSession(CallbackDict, SessionMixin):
             self.modified = True
         CallbackDict.__init__(self, initial, on_update)
         self.sid = sid
-        self.token = None
+        self.token: Optional[str] = None
         self.is_api = False
         self.to_destroy = False
         self.to_regenerate = False
@@ -45,6 +45,8 @@ class SessionInterface(FlaskSessionInterface):
         return token_urlsafe(32)
 
     def _get_signer(self, app: 'Flask') -> 'URLSafeTimedSerializer':
+        if not app.secret_key:
+            raise RuntimeError('No secret key set')
         return URLSafeTimedSerializer(app.secret_key,
                                       salt=self.salt)
 
@@ -102,6 +104,8 @@ class RedisSessionInterface(SessionInterface):
             sid = self._get_signer(app).loads(sid)
         except BadSignature:
             return new_session(is_api)
+        if not sid:
+            return new_session(is_api)
 
         val = self.redis.get(self.key_prefix + sid)
         if val is not None:
@@ -112,7 +116,7 @@ class RedisSessionInterface(SessionInterface):
                 return new_session(is_api)
         return new_session(is_api)
 
-    def save_session(self, app: 'Flask', session: 'SessionMixin', response: 'Response') -> 'None':
+    def save_session(self, app: 'Flask', session: 'RedisSession', response: 'Response') -> 'None':
         '''This is called at the end of each request, just
         before sending the response.
         '''

That leaves me with:

securedrop/journalist_app/sessions.py:51: error: "SessionInterface" has no attribute "salt"
securedrop/journalist_app/sessions.py:81: error: Return type "Optional[SessionMixin]" of "open_session" incompatible with return type "None" in supertype "SessionInterface"
securedrop/journalist_app/sessions.py:154: error: Argument 2 to "set_cookie" of "BaseResponse" has incompatible type "Optional[str]"; expected "str"

The first issue goes away if the interfaces are merged, like I suggested inline. The second is because open_session() is defined in the parent as returning nothing, so we should get rid of our returns too.

The final one has to do with response.set_cookie(app.session_cookie_name, session.token,, where token is defined as Optional[str]. Should token be typed as str instead?

lsd-cat · 2022-04-22T15:54:38Z

Implemented most required changes, tests still in progress, thanks for the help!

The second is because open_session() is defined in the parent as returning nothing, so we should get rid of our returns too.

I am not sure how to resolve this one. Flask docs specifically says that open_session() should return a session object or None if impossible. https://tedboy.github.io/flask/interface_api.session_interface.html

Also here in the parent https://github.com/pallets/flask/blob/a03719b01076a5bfdc2c8f4024eda7b874614bc1/src/flask/sessions.py#L299 it indeed doe not return but it has the Optional return type and it is there just as a placeholder to extend. Do we have any other option?

legoktm · 2022-04-25T17:27:36Z

I am not sure how to resolve this one. Flask docs specifically says that open_session() should return a session object or None if impossible. https://tedboy.github.io/flask/interface_api.session_interface.html

The correct docs link is https://flask.palletsprojects.com/en/2.0.x/api/#flask.sessions.SessionInterface.open_session btw, but the return type seems to still be Optional[SessionMixin]. I'll try to poke at it, otherwise we can use # type: ignore to suppress mypy.

lgtm-com · 2022-05-11T22:14:35Z

This pull request introduces 2 alerts when merging d11545f into f384d7c - view on LGTM.com

new alerts:

2 for Unused import

securedrop/journalist_app/__init__.py

securedrop/journalist_app/sessions.py

securedrop/journalist_app/utils.py

securedrop/journalist_app/__init__.py

lgtm-com · 2022-05-23T20:37:11Z

This pull request introduces 2 alerts when merging ec60813 into 1c133e5 - view on LGTM.com

new alerts:

2 for Unused import

nabla-c0d3

I did a quick review - I don't have all the context (nor the Flask session mgmt knowledge) so it's really just a few thoughts.

securedrop/journalist_app/__init__.py

securedrop/journalist_app/sessions.py

securedrop/tests/test_journalist_session.py

securedrop/journalist_app/sessions.py

ghost · 2022-08-16T11:41:21Z

I'm not seeing any issues, so +1 for me.

legoktm · 2022-08-18T19:28:13Z

Did a quick skim through, I think we're mostly ready to move this forward?

There are still a few unaddressed comments from @nabla-c0d3 that we should get to.
And then we need to squash this down somehow.
I explicitly listed some bugs that will be fixed by this as well.

legoktm · 2022-09-21T18:40:48Z

We should remove the "Rebase:" prefix from the commit messages, otherwise I have no other comments!

zenmonkeykstop · 2022-09-23T16:11:48Z

securedrop/journalist_app/sessions.py

+        else:
+            if expires < (30 * 60) and session["renew_count"] > 0:
+                session["renew_count"] -= 1
+                expires += self.lifetime


Should this be expires = self.lifetime? Otherwise a session lifetime can be set up to 7200+1800 sec which is over the limit in the spec.

I think that the spec is badly worded and we should keep this behavior so that the overall duration is equal for every session regardless of when the renew happens. It would be easier to debug and maintain in the future, especially considering that the client will renew almost immediately when it is possible.

zenmonkeykstop · 2022-09-23T16:23:16Z

Performed upgrade testing as follows:

set up 2.4.2 prod vms
built packages off this branch and installed them on prod using the upgrade test procedure
upgrade was successful and db migration worked.

Tested session expiry using redis-cli {keys|get|ttl|expire}. There are a couple of issues with session lifetimes:

sessions persist across server reboots, as there's nothing stopping the keys being written to disk (our redis.conf is pretty much the default, so that would be tunable there but require provisioning logic). This seems counter to the test plan's stated Reboot of the server/restart of redis logs destroy all sessions
when a session is extended in the last 30 minutes of its lifetime, the extension is for 2hrs+the remainder. The feature description would seem to indicate that renewal should be just for 2hrs.

lsd-cat · 2022-09-23T18:24:03Z

Tested session expiry using redis-cli {keys|get|ttl|expire}. There are a couple of issues with session lifetimes:

* sessions persist across server reboots, as there's nothing stopping the keys being written to disk (our redis.conf is pretty much the default, so that would be tunable there but require provisioning logic). This seems counter to the test plan's stated `Reboot of the server/restart of redis logs destroy all sessions`

The non persistance in my testing environment was due to container themselves being not persistant, even though Redis in the early days used not to store snapshots by default, it indeed does now. Although the persistance of sessions is counterintuitive per se (most systems do not), it is not a major issue since the payload contains only the user id, the csrf token, the renewal count and the ttl. I'll amend the test plan to reflect this.

* when a session is extended in the last 30 minutes of its lifetime, the extension is for 2hrs+the remainder. The feature description would seem to indicate that renewal should be just for 2hrs.

As noted above, I will also amend the description to properly reflect this behavior, as we concluded during the development phase that the currently implemented is the intended on.

zenmonkeykstop · 2022-09-23T21:29:11Z

securedrop/config.py.example

@@ -17,6 +17,10 @@ class SourceInterfaceFlaskConfig(FlaskConfig):
 class JournalistInterfaceFlaskConfig(FlaskConfig):
    SECRET_KEY = '{{ journalist_secret_key.stdout }}'
    SESSION_COOKIE_NAME = "js"
+    SESSION_SIGNER_SALT = "js_session"


New config.py variables should not be expected to be present on existing instances, as config.py is created at install time and rarely touched afterward.

zenmonkeykstop · 2022-09-23T21:31:26Z

securedrop/journalist_app/sessions.py

+
+def logout_user(uid: int) -> None:
+    redis = Redis()
+    for key in redis.keys(app.config["SESSION_KEY_PREFIX"] + "*") + redis.keys(


This config key may not be set on existing instances that were upgraded. Calling this function causes a 500 error with the following example stack trace:

[Fri Sep 23 18:35:04.907262 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] E RROR:journalist_app:Exception on /account/new-password [POST] [Fri Sep 23 18:35:04.907331 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] T raceback (most recent call last): [Fri Sep 23 18:35:04.907354 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask/app.py", line 2073, in wsgi_app [Fri Sep 23 18:35:04.907370 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] response = self.full_dispatch_request() [Fri Sep 23 18:35:04.907384 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask/app.py", line 1518, in full_dis patch_request [Fri Sep 23 18:35:04.907414 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] rv = self.handle_user_exception(e) [Fri Sep 23 18:35:04.907428 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask/app.py", line 1516, in full_dispatch_request [Fri Sep 23 18:35:04.907442 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] rv = self.dispatch_request() [Fri Sep 23 18:35:04.907454 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/opt/venvs/securedrop-app-code/lib/python3.8/site-packages/flask/app.py", line 1502, in dispatch_request [Fri Sep 23 18:35:04.907467 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] return self.ensure_sync(self.view_functions[rule.endpoint])(**req.view_args) [Fri Sep 23 18:35:04.907478 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/var/www/securedrop/journalist_app/account.py", line 46, in new_password [Fri Sep 23 18:35:04.907486 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] logout_user(user.id) [Fri Sep 23 18:35:04.907498 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] File "/var/www/securedrop/journalist_app/sessions.py", line 262, in logout_user [Fri Sep 23 18:35:04.907511 2022] [wsgi:error] [pid 687:tid 118508390467328] [remote 127.0.0.1:60948] KeyError: 'SESSION_KEY_PREFIX'

As logout_user was the only function outside the class it would thus not fallback on the default values. Fix is to move it in the same context as the other functions that require self.key_prefix and self.redis. It is also cleaner since we do not reinstantiate the redis object too. 64d4072

securedrop/journalist_app/sessions.py

- Add Alembic migration; - logout, session duration and test fixes; - add config options

Due to multiple reasons, now that there is a custom session wrapper, keeping the user state also in a flask.g object causes issues iof synchronization and cleaning. This commit completely removes its usage and replaces it with a a few additional session functions.

…ion; minor fixes

…onfig.py.example

Concerns addressed!

zenmonkeykstop

Test plan looking good!

Migration

Database migration work (deletes the revoked token table)
Token cleanup procedures do not run/exists
Sessions persist across reboot (as per default redis config)

Security

Session ids are random
Sessions id are signed not checked

Invalid session id (non existant/wrong signature) gets ignored

Login to the JI
Tamper the cookie signature
Confirm that user is logged out

Functional - JI

Journalists can Login

Login to the JI
Confirm that the user is logged

Sessions of multiple users are separate

Login to the JI as user X in browser A
Login to the JI as user Y in browser B
Confirm that the two users navigation does not interfere

Sessions are renewed in accordance with the new expiration/renewal policy

Login to the JI
Wait 2 hours (or use redis-cli to expire the session key...)
Confirm that user is logged out automatically
Confirm that an error message is shown
Login to the JI
Wait between 1.5h and 2h and refresh (or fake it with expires as above)
Wait 1h (same deal)
Confirm that user is still logged in

Logout works

Login to the JI as user X in browser A
Login to the JI as user Y in browser B
Logout from user X in browser A
Confirm that user X is logged out
Confirm that user Y is still logged in

Deleting a user destroy all sessions of the user

Login to the JI as admin X in browser A
Login to the JI as admin/user Y in browser B
From admin X in browser A, delete user Y
Confirm that user Y in browser B is logged out

Changing a user password destroy all sessions of the user

Login to the JI as admin X in browser A
Login to the JI as admin/user Y in browser B
From admin X in browser A, change password of user Y
-[x] Confirm that user Y in browser B is logged out
Login to the JI as admin X
Change admin X password
Confirm that admin X is logged out on password change

API (tested via client)