Skip to content

Why are secrets in plain text? #729

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
esseti opened this issue Aug 2, 2019 · 7 comments · Fixed by #1020 or #1093
Closed

Why are secrets in plain text? #729

esseti opened this issue Aug 2, 2019 · 7 comments · Fixed by #1020 or #1093
Assignees
Milestone

Comments

@esseti
Copy link

esseti commented Aug 2, 2019

Hello all,
I was digging in the data and I found out that all the secrets are stored in plain text in the DB.

Why so?

reading this #276 it seems that the client_secret is used to generate/calculate some value, which one precisely? so for that, there's no way to hash it.

However, what about access_token and refresh tokens? why are not they hashed? in the end, they should be, somehow, as a password, so the real value is not needed (although you will have access to the real value each request if needed).

@esseti esseti changed the title Why secrets are in plain text Why secrets are in plain text? Aug 2, 2019
@esseti esseti changed the title Why secrets are in plain text? Why are secrets in plain text? Aug 2, 2019
@IvanAnishchuk
Copy link
Member

I think the main reason is that most of the time it's not required. With the multiple factors involved (you usually need client secret + password to get an access token) and them being random, often temporary, and not reused anywhere, stealing a bunch of client secrets doesn't give intruder much more than they already have.

But I can see some value in hashing and salting not just client secrets but also all the tokens and grants. Hashing access tokens by default is usually pointless (would make requests too slow) but in case of extreme paranoia (e.g. auth servers used by important resource servers) could be possible as well (even introspection should work, I think, token is always presented plain when something needs to be done), probably with some clever optimization to speed up verifying requests or perhaps just lighter hash algo as those tokens are short-lived and random enough.

I think it could be implemented as an extension over DOT or even as an optional model mixin or something here. Feel free to PR and, you know, occasionally ask questions about how to override one method or other.

@pkarman
Copy link
Contributor

pkarman commented Oct 13, 2021

The client credentials (2-legged) flow does not require an additional password, so in that sense the client_id and client_secret act like a username/password pair. Not hashing the secrets does feel like a gap, since a compromise of the database effectively gives you plain text passwords for all client credentials applications.

I'd be happy to work up a patch to address this. I did not see any mention of it elsewhere.

@pkarman
Copy link
Contributor

pkarman commented Oct 13, 2021

@IvanAnishchuk I took a first pass in #1020 -- let me know what you think.

@auvipy
Copy link
Contributor

auvipy commented Oct 15, 2021

will need more input for this, i personally dont think thats an big isseue. but lets wait

@pkarman
Copy link
Contributor

pkarman commented Oct 15, 2021

For each registered application, you’ll need to store the public client_id and the private client_secret. Because these are essentially equivalent to a username and password, you should not store the secret in plain text, instead only store an encrypted or hashed version, to help reduce the likelihood of the secret leaking.

from https://www.oauth.com/oauth2-servers/client-registration/client-id-secret/

@n2ygk
Copy link
Member

n2ygk commented Jan 9, 2022

Since #1020 has been reverted, reopening this.

Still would be a great feature to have.

@n2ygk n2ygk reopened this Jan 9, 2022
@n2ygk n2ygk added this to the 1.7.0 milestone Jan 9, 2022
@n2ygk n2ygk modified the milestones: 1.7.0, 2.0.0 Jan 19, 2022
@n2ygk n2ygk self-assigned this Jan 19, 2022
@n2ygk
Copy link
Member

n2ygk commented Jan 19, 2022

I'll start with @pkarman's work and turn this into a breaking change that hashes all secrets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment