Hash customer passwords with PBKDF2 and upgrade legacy hashes transparently#727
Merged
Merged
Conversation
…rently Replaces the fast SHA1/SHA-x password hashing with PBKDF2 (HMAC-SHA256, 210k iterations, per-password 16-byte salt) and an optional server-side pepper from configuration. No database schema change. - New/changed passwords are stored as a self-describing value in the existing Customer.Password field: PBKDF2$1$<iter>$<salt>$<hash>, so the algorithm/parameters are known per record and never depend on a global setting. PasswordSalt is left empty (salt is embedded). - Verification is format-aware and constant-time (VerifyPassword): legacy SHA hashes still verify via the existing HashedPasswordFormat; PBKDF2 is detected by prefix. - Legacy hashes are transparently re-hashed to PBKDF2 after a successful login (LoginCustomer), so no password reset or data migration is needed. - Centralizes the previously duplicated hash-compare logic across the customer manager and the login/change-password/delete-account validators. - Adds Security:PasswordHashKey (pepper) and Security:PasswordHashIterations to configuration; both optional (empty pepper = no pepper). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses weak customer password hashing by introducing a PBKDF2 (HMAC-SHA256) self-describing hash format with optional server-side pepper and iteration configuration, and by centralizing password verification through a single format-aware API that can transparently upgrade legacy credentials on login.
Changes:
- Added PBKDF2 hashing + format-aware verification/upgrade helpers to
EncryptionService/IEncryptionService. - Updated customer login/account validators and customer manager flows to use
VerifyPassword(...)and upgrade legacy hashes after successful authentication. - Added configuration surface (
PasswordHashKey,PasswordHashIterations) and expanded tests for PBKDF2 + legacy compatibility.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Web/Grand.Web/Validators/Customer/LoginValidator.cs | Uses centralized VerifyPassword instead of duplicating per-format comparisons. |
| src/Web/Grand.Web/Validators/Customer/DeleteAccountValidator.cs | Uses centralized VerifyPassword for password confirmation during account deletion. |
| src/Web/Grand.Web/Validators/Customer/ChangePasswordValidator.cs | Uses centralized VerifyPassword when validating the old password. |
| src/Web/Grand.Web/App_Data/appsettings.json | Adds Security.PasswordHashKey and Security.PasswordHashIterations settings. |
| src/Web/Grand.Web.AdminShared/Validators/Common/LoginValidator.cs | Uses centralized VerifyPassword for admin login validation. |
| src/Tests/Grand.Business.Customers.Tests/Services/CustomerManagerServiceTests.cs | Updates tests/mocks to accommodate VerifyPassword usage. |
| src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs | Adds PBKDF2 round-trip, legacy verify, upgrade detection, and pepper sensitivity tests. |
| src/Core/Grand.Infrastructure/Configuration/SecurityConfig.cs | Extends SecurityConfig with pepper and iteration settings. |
| src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs | Verifies via VerifyPassword and upgrades legacy hashes on successful login; uses PBKDF2 for new hashed passwords. |
| src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs | Introduces HashPassword, VerifyPassword, and PasswordHashNeedsUpgrade. |
| src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs | Implements PBKDF2 hashing/verification and upgrade detection logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…on weaker cost Addresses review feedback on the password hashing PR: - VerifyPbkdf2 now fails closed (returns false) for corrupted stored hashes - non-positive iteration counts, invalid Base64, empty hash, wrong segment count - instead of letting Rfc2898DeriveBytes throw on the auth path. - PasswordHashNeedsUpgrade now compares the embedded iteration count to the configured cost, so raising PasswordHashIterations re-hashes existing PBKDF2 credentials on next login (matches the documented behavior). - Adds tests for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The only reachable throw inside the block is Convert.FromBase64String (FormatException); iteration count and output length are already guarded above and the remaining Pbkdf2 arguments are constant/non-null, so the broader ArgumentException branch was dead. Narrowing makes the intent (reject a corrupted stored value) explicit without masking argument errors. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
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
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.
Summary
Addresses audit finding #1 (weak password hashing): customer passwords were hashed with fast, general-purpose SHA1/SHA-x (a 5-byte salt for the
Hashedformat), which is unsuitable for password storage. This switches new/changed passwords to PBKDF2 (HMAC-SHA256, 210k iterations, 16-byte CSPRNG salt) with an optional server-side pepper from configuration.No database schema change. No forced password reset.
How it works
Customer.Passwordfield:PBKDF2$1$<iterations>$<saltB64>$<hashB64>. The algorithm and cost are known per record, so verification never depends on a global setting and different accounts can use different formats side by side.PasswordSaltis left empty for PBKDF2 (the salt is embedded).IEncryptionService.VerifyPassword(...): PBKDF2 is detected by prefix; anything else falls back to the legacy SHA path using the existingCustomerSettings.HashedPasswordFormat, so existing accounts keep working.CustomerManagerService.LoginCustomerre-hashes to PBKDF2 and persists it. Storefront, admin and API web login all route throughLoginCustomer.CreatePasswordHash(...) == Passwordcompare (customer manager + 4 validators) now goes through the singleVerifyPasswordentry point.Configuration (new, both optional)
Under
Securityinappsettings.json:PasswordHashKey— server-side pepper mixed into the hash. Keep it out of source control (env varSecurity__PasswordHashKey/ secret store). Empty = no pepper. Changing it invalidates existing PBKDF2 hashes (legacy SHA unaffected), so set it once before go-live.PasswordHashIterations— default210000(OWASP). Embedded per hash, so it can be raised safely later.Tests
EncryptionServiceTests— 11/11 (added: PBKDF2 round-trip, legacy SHA verify, upgrade detection, pepper sensitivity).CustomerManagerServiceTests— 5/5.Notes / follow-ups (not in this PR)
HashedPasswordFormatsetting now only governs verification of pre-existing SHA hashes; new hashes are always PBKDF2.Clear/Encryptedpasswords can be fully converted to clean PBKDF2 (plaintext is recoverable), and legacyHashedcould be onion-wrapped. Can follow in a separate PR if desired.🤖 Generated with Claude Code