api/admin: migrate password hashing from MD5 to bcrypt - #1566
Draft
ojasshelke46 wants to merge 1 commit into
Draft
api/admin: migrate password hashing from MD5 to bcrypt#1566ojasshelke46 wants to merge 1 commit into
ojasshelke46 wants to merge 1 commit into
Conversation
GenerateUserSecret stored a user's password as a raw md5(username:realm:password) digest. MD5 is fast to compute, which is the opposite of what a password hash needs: commodity hardware tries billions of guesses per second, so common passwords fall in seconds if the users table is ever exposed. There was also no per-user random salt, so two users who picked the same password produced byte-identical stored values, and the digest was directly replayable because it is exactly what the authentication check compares against. Passwords are now hashed with bcrypt at bcrypt.DefaultCost, which is deliberately slow and salts every hash individually. The username is no longer mixed into the input: bcrypt supplies its own salt, and including the username would silently eat into bcrypt's 72-byte input limit. The parameter is kept in the signature so existing callers don't have to change. Stored secrets are migrated without forcing a password reset. VerifyUserPassword accepts both formats and reports which one it found, and Service.Authenticate re-hashes with bcrypt and persists the result on the first successful login against a legacy secret, so an account is only ever checked against md5 once. No scheme-marker column is needed: bcrypt output always starts with "$2" and a legacy secret is always 32 hex characters, so the stored value's own shape distinguishes them. A failed upgrade is logged but doesn't fail the login, which had already succeeded. Migration 0024 widens users.secret from varchar(50) to text, since a bcrypt hash is 60 characters and would not fit. Tests cover the bcrypt round trip, that two users sharing a password now get different secrets, that a legacy md5 secret still verifies, and that a successful legacy login rewrites the stored secret as bcrypt while a subsequent login leaves it alone. Signed-off-by: Ojas Shelke <ojasshelke733@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
GenerateUserSecret hashed passwords with MD5 — a fast, general-purpose
hash with no per-user randomness, unsuited for password storage. Modern
hardware can attempt billions of guesses per second against it, and two
users with the same password produce identical stored values.
If stored secrets are ever exposed by any means — a backup leak, an
overly broad database credential, misconfigured access — MD5 offers
essentially no protection: common passwords fall in seconds, and
matching hashes reveal which users share a password. The stored value
is also directly reusable, since it's the exact string the
authentication check compares against.
What changed
GenerateUserSecretnow hashes with bcrypt atDefaultCost. Usernamedropped from the hash input (bcrypt salts itself; including it would
eat into bcrypt's 72-byte input limit) but kept in the function
signature so callers don't change.
VerifyUserPassword(username, password, storedSecret) (ok, isLegacy, err)— verifies against either format and reports which.Service.Authenticatere-hashes and persists on the first successfullegacy login; a later login on an already-upgraded secret is a no-op.
Format is detected by shape — bcrypt output starts
$2, legacy MD5 is32 hex characters — no separate scheme-marker column needed.
0024_widen_user_secret_for_bcrypt.sqlwidensusers.secretfromvarchar(50)totext(bcrypt output is 60 characters).types.ErrInvalidCredentials; corrected theUser.Secretdoccomment.
golang.org/x/cryptopromoted from indirect to direct ingo.mod.Note for reviewers
Authenticateis new API surface — nothing calls it yet, since today'sauth modes are noop/github/oidc with no local-password login path. It's
the piece a future basic-auth path would use, and it's what makes the
upgrade-on-login behavior meaningful rather than dead code. Happy to
drop it and keep this PR scoped to hashing + migration only if you'd
rather land that separately.
Rebased on top of the
pkg/api→pkg/api/runtimerefactor andre-verified against the new tree; no conflicts (no upstream
0024migration,
admin/users.goandusers_test.gounmoved).Test plan
produce different secrets; legacy MD5 still verifies; legacy
login upgrades the secret, a second login leaves it alone; wrong
password returns
ErrInvalidCredentialsgo build ./...,go vet ./...,golangci-lint run ./pkg/api/...→ 0 issues,gofmtclean./pkg/api/...suite: 2 pre-existing failures(
TestUpdateInstanceStats,TestGetUpdatePackage_MaxUpdatesLimitsReached), confirmedidentical on a clean stash of
main— unrelated to this changeusers.secretistextwith the checkconstraint intact;
TestMigrateDownpasses