chore!: ensure consistent secret token generation and hashing#20388
Conversation
Salts are not required for random secrets. Salts are only useful to protect against rainbow table style attacks. Since the input it 40 random characters, salts are overkill.
c3eb94d to
2cfc267
Compare
code-asher
left a comment
There was a problem hiding this comment.
Nice!
This is a breaking change for the oauth tokens. Since oauth is only allowed for dev builds and experimental, this is ok.
To add to this, I think we use it for MCP and there might be some people using it with Backstage (@Parkreiner just a heads up).
|
Yeah, we just rolled out oauth for Backstage, since we have a pretty high-profile lead who really wanted it I want to say changing the tokens should be okay, but could I get an explanation for the ways that it becomes a breaking change? That'd be really helpful for me, and I don't know if it goes beyond requiring the user to reauthorize |
ThomasK33
left a comment
There was a problem hiding this comment.
LGTM, some style nits nothing to get blocked on.
It's unlikely. The OAuth components were implemented before I added the experiment and were gated exclusively by the 'dev' version check. So, anyone using Backstage with the OAuth provider must have been running a custom development build.
As far as I know, it's a breaking change because the codersdk now exposes a different data type for those fields. As a result, any code that explicitly types or relies on it being a nullable string in SQL will no longer work. However, I could be wrong. |
TIL a customer is using it 🤦 I will comment later today specifically what breaks. Iirc all apps need to be remade and all oauth refresh tokens become invalid.
Yes there are some type changes there too. @Parkreiner I'll dm you on slack |
|
@Parkreiner
So basically all of oauth will be broken and have to be redone 😢 |
I noticed our oauth2 tokens used
userpassword.Hashwhich salts the hash. Since our tokens are 40 random characters, salting is redundant and provides no increased security.This PR uses the same sha256 hashing technique as we use for APIKeys. So now all randomly generated secrets will be hashed with sha256 for consistency.
This is a breaking change for the oauth tokens. Since oauth is only allowed for dev builds and experimental, this is ok.