Refactor scrypt hashing and source app session mgmt pt 2/3 #5694

nabla-c0d3 · 2021-01-11T06:04:37Z

Status

Ready.

Description of Changes

This PR follows #5692 .

This PR adds logic to cache the source's information as their "session" in the newly-implemented SessionManager, so the user's filesystem_id and gpg_secret are only generated once, on login. In the current implementation, the filesystem_id is generated on every authenticated request hitting the source app, ie. an scrypt call (=slow) is done on each authenticated request.
More generally, the SessionManager centralizes documents session management in the source app, and hence replaces the usage of the Flask's "thread locals" flask.g and flask.session. These globals are opaque (it's hard to know what data/keys are in them, and when), untyped, and overall a bit of an antipattern; even the Flask documentation mentions that they "make large applications harder to maintain" (https://flask.palletsprojects.com/en/1.1.x/design/#micro-with-dependencies).
This PR removes the dependency to scrypt library and switches to cryptography's implementation (for Investigate replacement of pyotp with equivalent functionality in cryptography #5613).
The new centralized session-management logic should also fix the following issues:
- Session expiry banner on the source index page #5716
- Logged-out user is shown "You were logged out due to inactivity" and redirected to index page #5741

nabla-c0d3 · 2021-01-12T05:46:15Z

securedrop/source_app/session_manager.py

+    @classmethod
+    def log_user_in(cls, user: SourceUser, user_passphrase: "DicewarePassphrase") -> None:
+        # Save the passphrase in the user's session cookie
+        session[cls._KEY_IN_SESSION_COOKIE] = user_passphrase


This follows the same logic as the current code base, which is to put the user's passphrase in their session cookie (which is encrypted). I don't know if this is useful or not, but we could replace the passphrase in the cookie with a randomly-generated session ID.

nabla-c0d3 · 2021-08-19T19:59:06Z

securedrop/source_app/__init__.py

-        # ignore_static here because `crypto_util.hash_codename` is scrypt
-        # (very time consuming), and we don't need to waste time running if
-        # we're just serving a static resource that won't need to access
-        # these common values.


This is no longer needed since I removed the "time consuming" scrypt call from the code that handles incoming requests.

nabla-c0d3 · 2021-08-19T20:01:01Z

securedrop/source_app/__init__.py

-                del session['logged_in']
-                del session['codename']
-                return redirect(url_for('main.index'))
-            g.loc = app.storage.path(g.filesystem_id)


All the authentication and session logic in this file is now handled by the SessionManager, which is called when the @login_required decorator is used in the Flask routes. This ensures that all authentication / session management logic happens in a single code location, thereby reducing complexity.

nabla-c0d3 · 2021-08-19T20:05:24Z

securedrop/source_app/decorators.py

+        except UserNotLoggedIn:
+            return redirect(url_for("main.login"))
+
+        return f(*args, **kwargs, logged_in_source=logged_in_source)


The decorator directly passes the source user to the Flask route/code (to make the "data flow" more obvious/simple), so using flask.g or session (and checking that they are in the right "state") is no longer needed.

nabla-c0d3 · 2021-08-25T20:59:36Z

securedrop/source_app/decorators.py

+            logged_in_source = SessionManager.get_logged_in_user()
+
+        except (UserSessionExpired, UserHasBeenDeleted):
+            return clear_session_and_redirect_to_logged_out_page(session)


This way of handling expiring sessions should fix the following issues: #5741, #5716

Add compatibility layer for session management to reduce size of changes Make passphrase as the session cookie more obvious Cleanup test_source fixture and explicitely pass a db_session Clarify name Consolidate logout behavior Run black formatting Add tests for SessionManager Fix tests securedrop/requirements/python3/securedrop-app-code-requirements.in securedrop/requirements/python3/securedrop-app-code-requirements.txt

kushaldas

Running it on staging for a few days, no issues shown.

verified Session expiry banner on the source index page #5716 is fixed
verified Logged-out user is shown "You were logged out due to inactivity" and redirected to index page #5741 is fixed
On g https://flask.palletsprojects.com/en/1.1.x/design/#thread-locals

Include tests, and also updates the apparmor entries. Merging this now and then moving to the last PR in this set.

nabla-c0d3 · 2021-09-08T02:35:54Z

Awesome!

kushaldas · 2021-09-09T08:08:38Z

We may have to revert this. While reviewing #5695 I noticed that sometimes using a second or third source at the same time is causing logout due to inactivity. And a a few times after updating to this session manager code, I can see that the source interface is not showing the source passphrase on the top.

For example:

nabla-c0d3 · 2021-09-09T15:49:35Z

I can look into this.

zenmonkeykstop · 2021-09-09T16:31:19Z

Thanks @nabla-c0d3 - if you wanna chat about it (even just for rubber-duck mode) folks are around on SD Gitter: https://gitter.im/freedomofpress/securedrop

nabla-c0d3 · 2021-09-10T04:41:39Z

Thanks @nabla-c0d3 - if you wanna chat about it (even just for rubber-duck mode) folks are around on SD Gitter: https://gitter.im/freedomofpress/securedrop

Thanks! It was the morning for me so I only got to this just now. Some thoughts/questions:

Using a second or third source at the same time is causing logout due to inactivity

I tried on the dev server and wasn't able to reproduce this. I was logged in as 4 different sources, uploading documents and such, but never got logged out on any of them.
This brings the following question: @kushaldas were you using the dev server or a production setup? If it was production, which web server is used to serve the source interface, and does the web server use multiple processes to process requests? If that's the case the cache at SessionManager._CACHE would not be shared across processes, so users would see the "logged out" message if their request hit a different process.
I could revert the logic to not cache anything and only use the session cookie, but this means that every request will trigger one call to scrypt to derive the filesystem_id from the passphrase in the cookie, as it was previously.

source interface is not showing the source passphrase on the top

I tried on the dev server and the passphrase is displayed right after user creation. It is not displayed on subsequent logins, which, to me, is expected; this seemed to be the intent of the code when I refactored it. It would be easy tho to update the logic to always show the passphrase after logging in (instead of just after user creation).
Or is it that right after you created a source user, the passphrase still wasn't displayed? This would be unexpected to me and a bug.

That's all I have for now... let me know your thoughts @kushaldas @zenmonkeykstop . Thanks!

zenmonkeykstop · 2021-09-10T15:18:18Z

For the first issue, I managed to get the "logged out due to inactivity" error intermittently (about half the time) in a staging env by:

logging in as a new source (/generate -> /lookup)
logging out and then going back to the home page (without clicking the Tor Browser New Identity button)
clicking through to /generate and getting a new codename
clicking thru to /lookup - instead of getting the /lookup page I get redirected to home with the "logged out" error.

This isn't precisely what Kushal saw, though, and I haven't found a way to consistently replicate that yet. (Also as STRs go, it is as the kids say, kinda sus)

Production (and staging) instances use Apache and mod_wsgi, and 2 processes are being spawned, so it's possible different requests will be served by different child processes. It might be better to back that change out as you mentioned. We could look at alternatives for subsequent releases. (The application uses Redis to cache GPG fingerprints already, but I think more work/research would be required before using it for full-on session management.)

For the second issue (no passphrase on /lookup) - my understanding is that the scenario Kushal is describing is the passphrase not showing up on /lookup on first login. I haven't managed to replicate this yet though, and it could just be a misunderstanding, so focusing on the first issue seems like the best idea for now.

nabla-c0d3 · 2021-09-12T05:58:27Z

I went through the steps you described a few times but I haven't been able to reproduce the logout errors/bugs. My environment might be different. Is your staging environment the same as running make dev ?

Regardless, I've updated the next PR at #5695 to remove the local cache, so that the whole session information is stored in the session cookie, like it was previously. This should get rid of these errors; I've tried it in my dev environment and it seemed to work fine.

zenmonkeykstop · 2021-09-13T14:57:49Z

Hey @nabla-c0d3 - we have 3 types of environments, each with their own foibles:

dev env (make dev): Docker-based, single-server environment, that just runs the two wsgi apps directly, exposing the services via open Docker ports. This is the simplest to run and most useful for web development, downside is that architecture-wise it's not a good match for production.
staging env (make staging): libvirt-based, multiserver environment, this is closer to production in that it includes both separate app and mon servers, proxies the apps via Apache, and makes the services available over Tor. It's more hassle to set up and requires that you build deb packages to push changes, rather than live-editing code, but it does catch the kind of issues that will only appear once the app is running against the prod-like server config.
prod VM env (molecule create -s libvirt-prod-focal): this is mostly used for upgrade testing (ie. 2.0.1 to 2.0.2). It is a virtualized production environment that requires a (usually also virtualized) Tails admin workstation to set up. It will catch any issues that you'll see in staging, but can also be used to test the effects of new packaging scripts on an existing system.

Thanks for the updates (and for the knock-on updates to pr number 3 :) )! We'll take a pass through ASAP.

nabla-c0d3 changed the title ~~[WIP] Refactor scrypt hashing pt 2/3~~ Refactor scrypt hashing and source app session mgmt pt 2/3 Jan 11, 2021

This was referenced Jan 12, 2021

Refactor scrypt hashing and source app session mgmt pt 1/3 #5692

Merged

Refactor scrypt hashing and source app session mgmt pt 3/3 #5695

Merged

nabla-c0d3 force-pushed the refactor-scrypt-hashing-pt-2 branch from acbf43f to 3083589 Compare January 12, 2021 05:39

nabla-c0d3 commented Jan 12, 2021

View reviewed changes

nabla-c0d3 force-pushed the refactor-scrypt-hashing-pt-2 branch from 3083589 to e320129 Compare March 26, 2021 02:14

nabla-c0d3 force-pushed the refactor-scrypt-hashing-pt-2 branch 3 times, most recently from d87ae38 to ce9cbbe Compare August 17, 2021 20:46

eloquence added this to the 2.1.0 milestone Aug 18, 2021

nabla-c0d3 commented Aug 19, 2021

View reviewed changes

kushaldas self-assigned this Aug 25, 2021

nabla-c0d3 force-pushed the refactor-scrypt-hashing-pt-2 branch from ce9cbbe to 01675a4 Compare August 25, 2021 20:18

nabla-c0d3 marked this pull request as ready for review August 25, 2021 20:20

nabla-c0d3 requested a review from a team as a code owner August 25, 2021 20:20

nabla-c0d3 commented Aug 25, 2021

View reviewed changes

nabla-c0d3 force-pushed the refactor-scrypt-hashing-pt-2 branch from 01675a4 to 7762ec2 Compare September 1, 2021 02:40

kushaldas approved these changes Sep 7, 2021

View reviewed changes

kushaldas merged commit 6020958 into freedomofpress:develop Sep 7, 2021

nabla-c0d3 deleted the refactor-scrypt-hashing-pt-2 branch September 25, 2021 04:43

nabla-c0d3 mentioned this pull request Oct 30, 2021

Update Flask version to 2.0.*, along with associated requirements #6154

Closed

5 tasks

cfm mentioned this pull request Nov 14, 2021

Rebase securedrop@signal-proto-focal against latest develop freedomofpress/securedrop-signal-poc#27

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor scrypt hashing and source app session mgmt pt 2/3 #5694

Refactor scrypt hashing and source app session mgmt pt 2/3 #5694

nabla-c0d3 commented Jan 11, 2021 •

edited

Loading

nabla-c0d3 Jan 12, 2021

nabla-c0d3 Aug 19, 2021

nabla-c0d3 Aug 19, 2021 •

edited

Loading

nabla-c0d3 Aug 19, 2021 •

edited

Loading

nabla-c0d3 Aug 25, 2021

kushaldas left a comment

nabla-c0d3 commented Sep 8, 2021

kushaldas commented Sep 9, 2021

nabla-c0d3 commented Sep 9, 2021

zenmonkeykstop commented Sep 9, 2021

nabla-c0d3 commented Sep 10, 2021

zenmonkeykstop commented Sep 10, 2021

nabla-c0d3 commented Sep 12, 2021

zenmonkeykstop commented Sep 13, 2021

Refactor scrypt hashing and source app session mgmt pt 2/3 #5694

Refactor scrypt hashing and source app session mgmt pt 2/3 #5694

Conversation

nabla-c0d3 commented Jan 11, 2021 • edited Loading

Status

Description of Changes

nabla-c0d3 Jan 12, 2021

Choose a reason for hiding this comment

nabla-c0d3 Aug 19, 2021

Choose a reason for hiding this comment

nabla-c0d3 Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

nabla-c0d3 Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

nabla-c0d3 Aug 25, 2021

Choose a reason for hiding this comment

kushaldas left a comment

Choose a reason for hiding this comment

nabla-c0d3 commented Sep 8, 2021

kushaldas commented Sep 9, 2021

nabla-c0d3 commented Sep 9, 2021

zenmonkeykstop commented Sep 9, 2021

nabla-c0d3 commented Sep 10, 2021

zenmonkeykstop commented Sep 10, 2021

nabla-c0d3 commented Sep 12, 2021

zenmonkeykstop commented Sep 13, 2021

nabla-c0d3 commented Jan 11, 2021 •

edited

Loading

nabla-c0d3 Aug 19, 2021 •

edited

Loading

nabla-c0d3 Aug 19, 2021 •

edited

Loading