Stop storing passwords reversibly and fail fast on weak JWT secrets (audit #2 & #3)#728
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>
Audit #2 (crypto) - eliminate reversible/deterministic 3DES "encryption" used as a password verifier; hash instead: - Installer now creates the default administrator with a PBKDF2 hash (via IEncryptionService) instead of the legacy TripleDES "Encrypted" format. - API users (UserApi) are stored as PBKDF2 hashes on create/update; login verifies PBKDF2 for new records and falls back to the legacy TripleDES check for existing ones (re-saving the user migrates it). Removes the reversible password + per-user key stored side by side. - EncryptText/DecryptText are documented as legacy verification-only; the algorithm is left unchanged so existing values still verify. Audit #3 (JWT secrets) - add ApiSecurityStartup that validates BackendAPI and FrontendAPI signing keys at startup: outside Development it aborts startup when an enabled API has a missing, placeholder or <32-char secret (a weak key allows forging valid tokens); in Development it only warns. appsettings comments updated to require setting the secrets via env/secret store. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…crets-hardening # Conflicts: # src/Business/Grand.Business.Common/Services/Security/EncryptionService.cs # src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs # src/Business/Grand.Business.Customers/Services/CustomerManagerService.cs # src/Tests/Grand.Business.Common.Tests/Services/Security/EncryptionServiceTests.cs
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses security audit findings by moving API user/admin credential storage away from reversible encryption and by enforcing stronger JWT signing secrets at application startup, reducing the risk of credential disclosure and token forgery.
Changes:
- Update API user creation/update and installer default admin creation to store passwords as PBKDF2 hashes via
IEncryptionService.HashPassword(...)instead of reversible TripleDES encryption. - Add
ApiSecurityStartupto fail fast (outside Development) when Backend/Frontend API JWT secrets are missing/placeholder/too short. - Document
EncryptText/DecryptTextas legacy verification-only inIEncryptionService.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Web/Grand.Web/App_Data/appsettings.json | Adds guidance/comments about required strong JWT secrets and startup fail-fast behavior. |
| src/Web/Grand.Web.Admin/Controllers/ApiUserController.cs | Switches API user password storage to PBKDF2; stops writing per-user encryption keys for new records. |
| src/Modules/Grand.Module.Installer/Services/InstallationService.cs | Hashes the installer-created default admin password using PBKDF2 and clears salt. |
| src/Modules/Grand.Module.Installer/Grand.Module.Installer.csproj | Adds Grand.Business.Core reference to access IEncryptionService for hashing. |
| src/Modules/Grand.Module.Api/Validators/Common/LoginValidator.cs | Adds PBKDF2 verification path for API-user login with fallback to legacy encrypted comparison. |
| src/Modules/Grand.Module.Api/Infrastructure/ApiSecurityStartup.cs | New startup validation to block weak/placeholder API JWT secrets outside Development. |
| src/Business/Grand.Business.Core/Interfaces/Common/Security/IEncryptionService.cs | Clarifies EncryptText/DecryptText are legacy-only and recommends modern alternatives. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Review feedback: choosing the PBKDF2 vs legacy path from an empty PrivateKey is fragile - a PBKDF2 record that still carries a PrivateKey (imported data or a future update that only rewrites Password) would take the legacy path and always fail to authenticate. Add IEncryptionService.IsHashedPassword to detect the modern self-describing hash from the stored value, and use it as the discriminator. The legacy path now also verifies in constant time and is guarded against a missing key. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.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
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.
Stacked on #727 (PBKDF2 password hashing) — review/merge that first. Base will be retargeted to
developonce #727 lands.Audit #2 — reversible password storage (reframed from the original "3DES → AES-GCM")
Investigation showed
DecryptTextis unused in production;EncryptText(TripleDES) is used only as a deterministic, keyed password verifier compared with==— there is no encryption-at-rest that gets decrypted. So swapping the cipher to AES-GCM would break the equality checks and fix nothing. The real issue is passwords stored reversibly, so this hashes them instead:IEncryptionService.HashPassword) instead of the TripleDESEncryptedformat. (Adds a lightweightGrand.Business.Corereference to the installer so the hash format has a single source of truth.)UserApi): create/update now store a PBKDF2 hash. Login verifies PBKDF2 for new records and falls back to the legacy TripleDES check for existing ones (re-saving the user migrates it). This removes the previous design where the reversible ciphertext and its per-user key were stored in the same record — effectively plaintext.EncryptText/DecryptTextare documented as legacy verification-only; the algorithm is intentionally left unchanged so existing stored values still verify. The path phases out via the on-login upgrade added in Hash customer passwords with PBKDF2 and upgrade legacy hashes transparently #727.Audit #3 — weak/placeholder JWT signing keys
ApiSecurityStartup(IStartupApplicationin the API module) validatesBackendAPIandFrontendAPIsigning keys at startup. For an enabled API with a missing / placeholder /<32-char secret it aborts startup outside Development (fail-fast — a weak key lets anyone forge valid tokens) and only warns in Development so local setups keep working.appsettings.jsoncomments now state the secrets are required and should be provided viaBackendAPI__SecretKey/FrontendAPI__SecretKey(env var) or a secret store.Not changed (deliberately)
ValidateIssuer/ValidateAudiencedefaults — left configurable (enabling them requires configured issuer/audience); documented rather than forced to avoid breaking existing deployments.Verification
Grand.Module.Api,Grand.Module.Installer,Grand.Web.Adminsucceed.EncryptionServiceTests11/11 (PBKDF2 primitives unchanged from Hash customer passwords with PBKDF2 and upgrade legacy hashes transparently #727).IEncryptionServicefrom the request-scoped provider (install runs underHttpContext.RequestServices).🤖 Generated with Claude Code