Logout to revoke tokens

[heartsucker]

Status

Ready for review

Description of Changes

Fixes #3933

Added a /logout endpoint to the API. Added a revoked_tokens DB table.

Test Plan

(edited by @redshiftzero)

Preconditions

1. set TOKEN_EXPIRATION_MINS in securedrop/journalist_app/api.py to a smaller value, like 5 minutes or so.
2. replace ./manage.py run with bash in securedrop/bin/run (this is so we can re-run the dev server manually to check the token cleanup works as expected with all the dev env setup in securedrop/bin/run)

Testing

1. Run dev env with make -C securedrop dev
2. Start dev server with ./manage.py run
3. Get a token with:

curl -X POST -H "Content-Type: application/json" --data '{"username":"journalist","passphrase":"correct horse battery staple profanity oil chewy","one_time_code":"875674"}' 127.0.0.1:8081/api/v1/token

4. Confirm you can revoke the token

$ curl -X POST -H "Content-Type: application/json" -H "Authorization: Token yourtokengoeshere" 127.0.0.1:8081/api/v1/logout

{
  "message": "Your token has been revoked."
}

5. Attempt to revoke the token once more to confirm that your token no longer works on the logout endpoint:

curl -X POST -H "Content-Type: application/json" -H "Authorization: Token yourtokengoeshere" 127.0.0.1:8081/api/v1/logout

{
  "error": "Forbidden",
  "message": "API token is invalid or expired."
}

6. Stop the server, and inspect the database:

$ sqlite3 /var/lib/securedrop/db.sqlite
select * from revoked_tokens;

Confirm that you see a single row in this database and that it contains your token expired.

7. Provided it has been less than 5 minutes since you created that token restart the server and run once more with ./manage.py run.

8. Confirm that you can still not use this token (this confirms it was not removed from the database incorrectly):

curl -X POST -H "Content-Type: application/json" -H "Authorization: Token yourtokengoeshere" 127.0.0.1:8081/api/v1/logout

{
  "error": "Forbidden",
  "message": "API token is invalid or expired."
}

9. Wait until 5 minutes have passed from when you first created this token (since we set 5 minutes as the expiration time) and restart the server one last time.

10. Confirm that the token is now gone from the revoked_tokens table in the database following the same steps as above:

$ sqlite3 /var/lib/securedrop/db.sqlite
select * from revoked_tokens;

(and for good measure if you like you can also verify that you still cannot use the token because it’s expired).

Deployment

Nothing special. Backwards compatible API change.

Checklist

If you made changes to the server application code:

• Linting (make ci-lint) and tests (make -C securedrop test) pass in the development container

[redshiftzero]

Regarding the ever-increasing size of the blacklisted token table (cc @emkll @heartsucker), what about the following:

1. We store the both the token and the expiration timestamp (recall that there is an 8 hour expiration on the tokens, beyond which we don't need to store them in the blacklist) in the database.
2. We write a small function that will check for expired tokens and delete them from the blacklist table. We could use @app.before_first_request to execute this on application start such that every day we clean out old tokens.

Thoughts?

[redshiftzero]

Actually we can skip step 1 and just try to validate the tokens as part of the function described in step 2, since expired tokens won't validate.

[redshiftzero]

I've implemented my suggestions above (since we are so close to the 0.13.0 cutoff, I didn't want to merge this without the cleanup of expired and revoked API tokens implemented). To facilitate testing from other people who are less familiar with the app code, I added more detailed steps to test in the PR description above, which I have followed to verify this functionality works as expected.

If someone could inspect/approve my last four commits and comment here if they look good, then we can merge - I 👍 @heartsucker's changes (not explicitly approving this PR to prevent accidental merge of unreviewed code in my commits).

[rmol]

I ran through the test plan, and it worked as expected. Took me a minute to realize I needed to make a request after restarting the server (steps 9 and 10) to get the expired revoked token purged, because before_first_request, but that got me wondering if Flask has a better place for housekeeping like this, like maybe record_once? Seems like it would be better to get this done without delaying any user request, though it's unlikely to take too long.

Also, it's not clear to me from the Flask docs what happens when a function registered either way throws an exception -- do we need to be sure that doesn't happen?

[redshiftzero]

Seems like it would be better to get this done without delaying any user request, though it's unlikely to take too long.

Agreed - and good idea re: record_once. So I tried this out in this branch as an experiment (there's a generic exception raised to test the unexpected exception behavior), and here's the issue: since that method is called at blueprint registration time, if there is an unexpected exception, then the application initialization will fail - whereas if we use before_first_request, then if there is an unexpected exception, the application will otherwise continue on merrily. Given that behavior, I think before_first_request seems like the most robust approach, even though the first request processing time will be a bit delayed. Let me know what you think!

[rmol]

I was afraid of something like that with record_once. Looks good to me as it is then. 👍

[heartsucker]

@redshiftzero's changes look good.

One follow up we may want to note is that an NTP based attack right at server/application boot could change the server time so that the token no longer validates so it is cleared from the table, but a second NTP update could reset the server to the actual time allowing an attacker to use the token. However, if the attacker managed to capture a token, we likely have much larger problems than this. I still think this should be documented for completeness.

[emkll]

Looks good to me, both functionally and diff review.

Regarding @heartsucker 's point on ntp-based attacks, definitely worth tracking, but the risk is even lesser due to Tor bootstrapping and authenticated tor hidden services being sensitive to clock skew