ansible / django-ansible-base

Apache License 2.0
18 stars 49 forks source link

[oauth2_provider] Hash access and refresh tokens #641

Closed relrod closed 1 week ago

relrod commented 2 weeks ago

~Missing pieces that still need to be done:~

EDIT: The above has been done.

Previously, OAuth2 Access tokens including PATs were not hashed or encrypted in any way, and were stored in plaintext in the database. Seeing them required direct database access, but it is still better to hash them, since they are long-lived.

This commit implements hashing (using sha256) of access tokens. Hashing is unsalted, as we need to be able to key on a stable input - but also because the tokens are already random strings and a salt adds nothing more of security.

The input bearer token (used to auth a user) is hashed in LoggedOAuth2Authentication and stuffed back into the request. This might seem like a weird place to inject the hash, but it avoids having to override any DOT internals.

The serializer has been updated to account for the new functionality and still works the same way as in the past: On POST (new token creation), the token will be displayed -- after that it will not.

Test fixtures and tests have been updated as well.

BrennanPaciorek commented 2 weeks ago

The problem with salting just clicked for me. I think we can use a application-wide salt for OAuth2 tokens if we want to get salts working, or we can use the application SECRET_KEY, or something derived from it (this would probably require some documentation, since we don't want users cycling the key, then getting irritated when their oauth2 sessions get invalidated).

Maybe we can add a table to the database for the purpose of storing a single salt, then set database constraints to make sure one is always present?

Definitely understand why implementing salts may be more trouble than its worth, so I am perfectly fine without the implementation of salts.

relrod commented 2 weeks ago

@BrennanPaciorek Yeah I think the unspoken thing here is: salts become hard because the naïve approach of just storing the salt in the database with the token doesn't work, because you have no idea what to key on to get that (hash, token) pair. For username/password login, this is avoided because you can key on the username you're given, so you can salt the passwords and easily retrieve that salt and apply it to the incoming password too. But for token auth you're only given a token with nothing else to key on.

I thought about using something like SECRET_KEY or any kind of hardcoded/setting string (creatively called a "pepper" in crypto parlance - like a salt but it never changes), but as you said if it changes, it will invalidate all the keys, so I wasn't sure that it was worth it.

the addition of a salt is useful for the purpose of hindering this pre-computation by introducing additional input to the hashing function that is not controllable by the user.

But that is kind of what I meant with "because the tokens are already random strings and a salt adds nothing more of security" -- the input to the hashing function when the token is generated, isn't controllable by the user, it's a random string. So someone would have to have a table of hashes of completely random (number/letter) strings, but if they did that, they could also have hashes of completely random strings with a few more characters (i.e. a salt) added to them. In other words, I think it doesn't really add anything except making the input slightly longer. But the tokens are all 30 random characters.

For funsies, I generated an oauth token and dropped it into https://www.passwordmonster.com/ (the first Google result for "password crack time calculator") and it told me this:

image

sonarcloud[bot] commented 1 week ago

Quality Gate Passed Quality Gate passed

Issues
4 New issues
0 Accepted issues

Measures
0 Security Hotspots
96.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud