NotificationsYou must be signed in to change notification settings
Fork1.1k
Star11.4k

chore!: ensure consistent secret token generation and hashing#20388

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

Emyrk merged 23 commits intomainfromstevenmasley/refresh_hashing

Oct 23, 2025

Merged

chore!: ensure consistent secret token generation and hashing#20388

Emyrk merged 23 commits intomainfromstevenmasley/refresh_hashing

Oct 23, 2025

Conversation

Copy link

Member

Emyrk commentedOct 20, 2025•
edited
Loading

I noticed our oauth2 tokens useduserpassword.Hash which 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.

github-actionsbot assignedEmyrk

Oct 20, 2025

Emyrk changed the title~~chore: oauth2 refresh tokens to use sha256 hashed secrets~~chore: oauth2 tokens to use sha256 hashed secrets

Oct 20, 2025

Emyrk marked this pull request as ready for review

October 21, 2025 15:37

Emyrk requested review fromThomasK33 andcode-asher

October 21, 2025 15:37

Emyrk added5 commits

October 21, 2025 10:38

chore: oauth2 refresh tokens to use sha256 hashed secrets

d1cf114

Salts are not required for random secrets. Salts are only useful toprotect against rainbow table style attacks. Since the input it 40random characters, salts are overkill.

chore: refactor RegistrationAccessToken from string to []byte

55ef6ed

make gen

41e1c4a

reuse apikey generate for more places

feb3df7

bump migration

2cfc267

Emyrk force-pushed thestevenmasley/refresh_hashing branch fromc3eb94d to2cfc267Compare

October 21, 2025 15:46

fmt

44744f7

Emyrk removed request forThomasK33 andcode-asher

October 21, 2025 16:30

Emyrk marked this pull request as draft

October 21, 2025 16:30

Emyrk added4 commits

October 21, 2025 11:31

fmt

8e853f8

fix test compile

914de0f

fixup! fix test compile

168c0a8

Linting

7d9b034

Emyrk changed the title~~chore: oauth2 tokens to use sha256 hashed secrets~~chore: consistent secret token generation and hashing

Oct 21, 2025

test fixes

b954782

Emyrk changed the title~~chore: consistent secret token generation and hashing~~chore: ensure consistent secret token generation and hashing

Oct 21, 2025

Emyrk added4 commits

October 21, 2025 15:22

linting

91f6f3e

linting

d2c1d28

linting

9a5c796

linting

96dc3e1

Emyrk marked this pull request as ready for review

October 21, 2025 20:58

Emyrk requested review fromThomasK33 andcode-asher

October 21, 2025 20:58

code-asher approved these changes

Oct 21, 2025

View reviewed changes

Copy link

Member

code-asher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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).

Copy link

Contributor

Parkreiner commentedOct 22, 2025•
edited
Loading

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 approved these changes

Oct 22, 2025

View reviewed changes

Copy link

Member

ThomasK33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

LGTM, some style nits nothing to get blocked on.

coderd/database/migrations/000387_oauth_app_byte_reg_access_token.up.sql OutdatedShow resolvedHide resolved

coderd/oauth2provider/registration.go OutdatedShow resolvedHide resolved

Copy link

Member

ThomasK33 commentedOct 22, 2025

there might be some people using it with Backstage

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.

but could I get an explanation for the ways that it becomes a breaking change

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.

Copy link

MemberAuthor

Emyrk commentedOct 22, 2025•
edited
Loading

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

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.

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.

Yes there are some type changes there too.@Parkreiner I'll dm you on slack