Improve logout security

[ashokdelphia]

The overall aim here is to try to improve logout security in two ways:

• allow the 'blacklist' to cover any token (transitively) refreshed from the same original token; this relies on using the token_id of the original token (from its jti claim, or the orig__jti claim of a refreshed token), so is only possible when JWT_TOKEN_ID is either include or require
• stop storing the whole token values when the config allows us to rely on token ids being present (that is when JWT_TOKEN_ID='require'). The potential danger is that if someone can read and delete from the blacklist, then they could revivify a token that is yet to expire and make it a valid token again.

My general preference would be to phase out support for JWT_TOKEN_ID='off', and start making require the default for new installations. That could substantially simplify matters here. It would take some more thought about how to do that and ensure that people upgrading had a graceful upgrade path, but I think it's something we could consider for a major version bump. In the meantime, I'd still like to stop storing whole token values (or emitting them in query logs) for a system that's using the require setting; this PR should allow that.

Apologies for making a relatively large PR; hopefully the individual commits are each relatively simple to review in turn.

---

• Stop checking non-existent tokens against the token blacklist. (163e167)

  Since the field is currently non-nullable this is relatively safe, although the query is pointless in that case.

  I'm about to start using the contents of the token, and allow the token to be absent in blacklist entries, so would like to abort early for these requests.

• Permit storing either the token or its original id (or both) for blacklist entries. (9a1a51a)

  For Django 2.2 and above, also add constraint that at least one of the values is non-null. Since we allow configuring whether token ids are included, the token_id column needs to be optional. Where we can rely on token ids, I am going to storing the whole token value, so that also needs to be nullable.

• Fill in the token_id value for existing blacklist entries. (381a7d8)

• Store a token id for invalidated tokens, if we have them. (2a69faf)

  For refreshed tokens, I'm storing the original token's id, so that the blacklist lookup can match for any token from the same chain of refreshes. (The orig_jti claim of a refreshed token matches the jti claim of the original token, and is copied down the chain of refreshes. For original tokens, the orig_jti claim is absent, and we use the jti claim.)

• Stop storing the whole token value for systems that are configured appropriately. (2003eb4)

  The admin interface shows these values and allows deleting them, which allows someone with privileged access to impersonate someone who logs out (by copying a token and removing it from the blacklist).

• Add tests for when the token and id fields are filled in the blacklist entry. (86b810a)

• Add a central point for checking tokens that are blocked. (a043949)

  I'm about to make the logic here more elaborate, and prefer it in one spot in any case.

• Move checking for blocked tokens to after other checks for validity. (e0b0ed5)

  When the blacklist was a straightforward - and presumably cheap - literal lookup by token value, I can see the merit for checking it first. If we care about using the contents of the token, then it's preferable to know that we can reliably decode the token.

  Note that this would change the response for invalid tokens. Assuming that only valid tokens are added to the blacklist, then the only visible sign of this would be for tokens that were added to the blacklist and then later expire. An external observer would now see the expiration error first, instead of the blacklist message. If an installation is clearing expired tokens, this is essentially the same behaviour that you would get in the old code, after the token expires and is purged from the blacklist.

• Check for blocked tokens depending on the setting for token ids. (b63dc86)

  If all valid tokens should have a token id ('require'), then all still-valid blacklisted tokens should have been recorded with a token_id, and we can purely look up using the id. Since the migration adding this column backfills the token_id values, this should be safe to rely on immediately after the migration completes.

  If tokens are being generated without ids ('off'), then we look up the blacklist record by the whole token value as before. This means that when altering the config from 'require' to 'off' existing blacklisted tokens would no longer match. I doubt that is going to be a common transition in practice. If someone needed to do that carefully, they could shift from 'require' to 'include' for one token expiration horizon, and then to 'off', but I think this is sufficiently fringe to ignore in practice.

  For the default setting ('include'), we check for a blacklist entry with either the token id or the exact token value. This should ensure that for people who have kept the defaults, things work in as straightforward manner as possible. Refreshed tokens are invalidated if any token from the same chain of refreshes is invalidated, which still improves logout security for the default case, even though we still storing the full token values, which has some downsides.

• Promote the refresh endpoint fixture to the top level. (436ccc3)

  I'd like to test something with refreshed tokens outside of the views, and this seems like a reasonably simple way to get one.

• Add some basic tests for when a token is blocked. (29f66c1)

  I hope this is a reasonable place to put the tests, but happy to take any suggestions for a better place for them to live.

• Add cautionary notes about using blacklisted tokens with the default setting for JWT_TOKEN_ID. (3920395)

  It would be a breaking change, but I think we should consider making 'require' the default value at some point in the future, and perhaps drop support for 'off'.

[ashokdelphia]

I'm reasonably sure that the broken build is the same as on master, but haven't got all of the versions running locally in order to reproduce and try and fix it (in a separate PR).

[ashokdelphia]

@fitodic Sorry this is a fairly hefty change, so I appreciate it'll take longer to get to for review.

I'm pretty itchy about having almost-valid tokens hanging about in the database as it opens up a handful of potential security problems that would be best solved by never storing whole tokens.

If you'd like to discuss whether we should do that a different way, please let me know.

[fitodic]

@ashokdelphia Thanks for the hard work you've put into this and sorry for taking so long to answer.

I went through it change-by-change as you've described it and I have nothing to add or object. It's outstanding 👏🏼

As to setting the JWT_TOKEN_ID's default value to 'require', I agree that this would be a breaking change that could be shipped as a major version.

Regarding the failing test, that's my fault. I merged one PR too late, but I'll fix it as soon as I find the time.

I'll merge this and release a new minor version shortly.

[ashokdelphia]

I'll merge this and release a new minor version shortly.

@fitodic That's great. Thank you!