add proposal: Deprecate and Remove the Legacy SHA-1 Password#286
add proposal: Deprecate and Remove the Legacy SHA-1 Password#286wy65701436 wants to merge 1 commit into
Conversation
…tion Path Signed-off-by: wang yan <yan-yw.wang@broadcom.com>
| - Add an internal metric/log (and optionally an admin-facing count) of how | ||
| many local users still have a legacy `password_version`. This lets | ||
| operators gauge readiness for removal. | ||
| - Publish a deprecation notice in the release notes and documentation: |
There was a problem hiding this comment.
From experience that we have made in the past, this won't work; many users will skip that and later complain. Only solution is to bump the major version from 2 to 3.
What I would rather do instead is make the announcement in the APP with a "Banner Message"
What we can do is create a Banner Message (sql script) this way the user/admin will notice about the upcoming changes. It would make sense to create the banner in sql conditionally scan the DB for SHA1 if there are any create a banner. (this way users who are not using SHA1 are not impacted) Admin can disable the banner message.
The only negative imact I can think of is that we will override any existing messges. Hower since this is important and only impacts a few legacy users I would argue in favor of this.
|
I want to surface one alternative. This proposal wants to:
Upgrade-on-login only ever migrates accounts that log in during the window. Dormant-but-valid local accounts (break-glass admins, service-style users, seasonal accounts) keep their sha1@4096 hash indefinitely and get silently locked out at removed. So for the dormant population, goals (a) and (b) are partially in conflict. The alternative concept (hash nesting, no plaintext required): Re-hash the existing stored hash under the strong KDF, expressed entirely with the current Encrypt so there's no new crypto: // One-time batch migration over each legacy row (must be Go, not SQL — pgcrypto has no PBKDF2):
nested := utils.Encrypt(stored, salt, utils.PBKDF2SHA256) // outer 600000 over the existing hash
// store: password=nested, password_version="pbkdf2_sha256+sha1", salt unchanged
// Verification for a nested row:
inner := utils.Encrypt(password, salt, utils.SHA1) // reproduces the old stored hash exactly
candidate := utils.Encrypt(inner, salt, utils.PBKDF2SHA256) // same transform the migration applied
match := candidate == entry.PasswordThis lifts the work factor to 600000 iterations for every legacy row immediately, dormant or not, with zero user action and no plaintext. Upgrade-on-login can still run on top, opportunistically replacing nested hashes with clean pbkdf2_sha256 when a user does log in. Trade-off: nesting does not let us drop the crypto/sha1 import, because verifying a nested hash still calls SHA-1 for the inner layer. If the primary driver is satisfying gosec G401/G505 by deleting the primitive, upgrade-on-login is the right call and nesting works against it. If the primary driver is hardening the weak work factor across the whole user base without a lockout event, nesting gets there for everyone in a single migration. Question: |
There was a problem hiding this comment.
Pull request overview
Adds a new proposal document describing a phased plan to migrate legacy PBKDF2-HMAC-SHA1 local-password hashes to the current PBKDF2-HMAC-SHA256 scheme (upgrade-on-login), announce a deprecation window, and remove the SHA-1 verification path in a future major release.
Changes:
- Introduces a 3-phase approach: upgrade-on-login, visibility/deprecation notice, and eventual removal.
- Documents current password hashing/versioning behavior and why SHA-1 support persists today.
- Provides a conceptual implementation sketch and security considerations.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Harbor still carries a legacy local-database password verification path based | ||
| on PBKDF2-HMAC-**SHA1**. It exists only to authenticate user accounts whose | ||
| password hashes were created before the SHA-256 migration in Harbor 1.9.1. |
| In a future major release (e.g. the next Harbor major / "2.0"-style | ||
| lifecycle boundary), after the deprecation window: |
chlins
left a comment
There was a problem hiding this comment.
The upgrade condition in the sketch is inverted — it should whitelist legacy versions, not exclude the current one.
The pseudocode gates the rehash on "not equal to the current scheme":
if entry.PasswordVersion != utils.PBKDF2SHA256 {
m.UpdatePassword(ctx, entry.UserID, password)
}This is a forward-compatibility trap. The moment a stronger scheme is introduced (say pbkdf2_sha512), this branch will treat those stronger credentials as "legacy" and downgrade them by re-hashing back to pbkdf2_sha256 on every login.
The condition should explicitly enumerate the legacy versions we actually want to migrate off of:
if entry.PasswordVersion == utils.SHA1 || entry.PasswordVersion == utils.SHA256 {
m.UpdatePassword(ctx, entry.UserID, password)
}This is especially worth pinning down given that pbkdf2Params already falls back to sha256 + 600000 for unknown versions — an !=-based check compounds that fragility. I'd suggest the proposal define the upgrade trigger as an explicit legacy-version allowlist rather than "anything that isn't the current scheme."
add proposal: Deprecate and Remove the Legacy SHA-1 Password