feat(auth): replace bcrypt with PBKDF2 for password hashing by nbmaiti · Pull Request #1297 · device-management-toolkit/console

nbmaiti · 2026-10-06T06:59:47Z

Description:

Migrate password hashing from bcrypt to PBKDF2-SHA256 for improved security and standards compliance.

Changes

Create new pkg/secrets/pbkdf2.go with PBKDF2 implementation (SHA256, 100k iterations, 16-byte salt)
Update password generation in cmd/app/secret_store.go
Update password verification in internal/controller/httpapi/v1/login.go
Update all test fixtures to use PBKDF2 functions
Remove all bcrypt dependencies from password handling code

Testing

✅ All existing tests pass
✅ Password generation and verification working correctly
✅ Login endpoint validated with new hashing
✅ Zero regressions

Security

PBKDF2-SHA256 with 100,000 iterations (OWASP recommended)
128-bit random salt per password
Hash format supports future iteration count increases
No plaintext passwords in logs or storage

codecov · 2026-10-06T07:32:33Z

Codecov Report

❌ Patch coverage is 69.85646% with 126 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.19%. Comparing base (0b9dd57) to head (3067e86).

Files with missing lines	Patch %	Lines
cmd/app/secret_store.go	77.13%	59 Missing ⚠️
config/config.go	31.16%	53 Missing ⚠️
internal/controller/httpapi/v1/login.go	82.35%	6 Missing ⚠️
pkg/secrets/pbkdf2.go	89.47%	4 Missing ⚠️
cmd/app/main.go	71.42%	2 Missing ⚠️
internal/app/app.go	33.33%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1297      +/-   ##
==========================================
+ Coverage   61.99%   62.19%   +0.20%     
==========================================
  Files         158      160       +2     
  Lines       13337    13679     +342     
==========================================
+ Hits         8268     8508     +240     
- Misses       5069     5171     +102

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Copilot review overview

🟡 Changes recommended

Security, keyring reliability, cleanup correctness, denial-of-service, and test race issues remain unresolved.

Review effort: Balanced
Findings: 4 · 8 · 2

Open (14)

Add rate limiting before expensive password derivation · New
Use constant-time password-hash comparisons
Do not print bootstrap credentials to stdout
Preserve explicitly configured JWT signing keys
Validate full hash format before classifying passwords
Reject empty config paths in clean mode
Preserve whitespace in configured passwords
Avoid requiring keyring for configured credentials
Avoid rewriting read-only config during credential resolution
Return errors when cleanup is incomplete
Support the documented single-dash clean flag
Serialize tests that mutate global state
Run table-driven subtests in parallel
Parallelize table-driven subtests

Previously missed (3)

In code that hasn't changed since last review

Restore existing keyring values on write failure

cmd/app/secret_store.go:242

Deleting a successfully written entry is not a rollback when that key already existed. Because startup rewrites all three keyring values, a transient failure writing one value causes these branches to delete previously valid credentials and leave the installation partially configured. Snapshot and restore prior values, or avoid rewriting unchanged keyring credentials.

Increase PBKDF2 work factor to 600,000 iterations

pkg/secrets/pbkdf2.go:16

PBKDF2-HMAC-SHA256's current OWASP work factor is 600,000 iterations, not 100,000. Using 100,000 makes newly stored admin credentials substantially cheaper to brute-force and does not meet the standards-compliance claim in this PR.

Split oversized authentication subsystem into focused changes

cmd/app/secret_store.go:1

This new 528-line subsystem combines keyring persistence, first-run bootstrap, .env parsing, JWT lifecycle, and a destructive cleanup CLI with the PBKDF2 migration. The repository requires focused 50–300-line PRs and prerequisite refactors to be split from the feature; separating these concerns is necessary to make the authentication change reviewable and reduce rollout risk.

Copilot

Copilot review overview

🟡 Changes recommended

Credential migration can break keyring-less deployments, alter existing passwords, and expose bootstrap secrets through stdout.

Review effort: Balanced
Findings: 4 · 10 · 2

Open (16)

Add rate limiting before expensive password derivation
Use constant-time password-hash comparisons
Do not print bootstrap credentials to stdout
Preserve explicitly configured JWT signing keys
Retain weak-password warnings before hashing · New
Increase PBKDF2 iterations to meet OWASP guidance · New
Validate full hash format before classifying passwords
Reject empty config paths in clean mode
Preserve whitespace in configured passwords
Avoid requiring keyring for configured credentials
Avoid rewriting read-only config during credential resolution
Return errors when cleanup is incomplete
Support the documented single-dash clean flag
Serialize tests that mutate global state
Run table-driven subtests in parallel
Parallelize table-driven subtests

Store standalone admin credentials and the JWT signing key in the OS keyring. Migrate legacy config values with bcrypt hashing and keep generated JWT keys in process memory when they are not configured. Preserve OAuth2 precedence and provide clean recovery for the complete local credential set.

- Create new pkg/secrets/pbkdf2.go with PBKDF2 implementation - Use SHA256 hash function with 100,000 iterations and 16-byte salt - Replace bcrypt.GenerateFromPassword with GeneratePBKDF2Hash - Replace bcrypt.CompareHashAndPassword with VerifyPBKDF2Hash - Update all password hashing in cmd/app/secret_store.go - Update password verification in internal/controller/httpapi/v1/login.go - Update all test fixtures to use PBKDF2 functions - Remove all bcrypt imports and dependencies from password handling code - All existing tests pass with PBKDF2 implementation

- Run go mod tidy to regenerate go.sum - Add missing golang.org/x/term entry (v0.46.0) - Ensure all transitive dependencies are properly recorded - Fixes CI error: missing go.sum entry for golang.org/x/term

- Add TestGeneratePBKDF2Hash: validates hash generation - Add TestGeneratePBKDF2Hash_DifferentSalts: verifies unique salts - Add TestVerifyPBKDF2Hash_ValidPassword: validates password verification - Add TestVerifyPBKDF2Hash_InvalidPassword: verifies wrong password rejection - Add TestVerifyPBKDF2Hash_NonPBKDF2Hash: validates non-PBKDF2 hash handling - Add TestVerifyPBKDF2Hash_MalformedPBKDF2Hash: edge cases for malformed hashes - Add TestIsPBKDF2Hash: validates hash type detection - Add TestPBKDF2HashFormat: validates hash format compliance - Add TestPBKDF2Consistency: ensures deterministic verification - Achieves 86.7% coverage for pkg/secrets package

- Add 4 new test cases for normalizeAdminPasswordHash in cmd/app/main_test.go: - TestNormalizeAdminPasswordHash_EmptyString: verify empty password handling - TestNormalizeAdminPasswordHash_SpecialCharacters: verify special chars support - TestNormalizeAdminPasswordHash_VeryLongPassword: verify 10k char passwords - TestNormalizeAdminPasswordHash_UniqueHashes: verify random salt generation - Improve pkg/secrets/pbkdf2_test.go with additional edge cases - Achieve 86.7% test coverage for pkg/secrets package - All tests passing with parallel execution

Fixes for: - godot: Add period to format comment - mnd: Extract magic number 3 to pbkdf2PartsCount constant - paralleltest: Add t.Parallel() calls to subtest ranges - tparallel: Add t.Parallel() to subtest definitions - wsl_v5: Add missing blank lines for readability All tests passing with parallel execution enabled

…eview 1. DoS Protection - Rate limiting on login endpoint - Track failed login attempts per client IP - Limit to 5 failures per 15 minute window - Return HTTP 429 (Too Many Requests) when exceeded - Thread-safe implementation using sync.Mutex - Automatically reset after timeout period - Resets on successful authentication 2. Credential Exposure Prevention - Redirect bootstrap credentials to stderr (not stdout) - Prevents accidental capture in stdout logs - Maintains user visibility for initial setup 3. Strict Hash Format Validation - Already implemented: validates complete PBKDF2 format - Checks: iterations (valid int), salt (hex), hash (hex) - Prevents misclassifying malformed strings as hashes 4. Test Serialization - Added //nolint:paralleltest to tests mutating global state - Prevents race conditions from concurrent test execution All tests passing with proper synchronization and validation.

- Add blank line after loginMutex.Lock() before attempts assignment (wsl_v5) - Add blank line before rate-limit return statement (nlreturn) - Add blank line after loginMutex.Lock() before loginAttempts increment (wsl_v5) All golangci-lint issues resolved in login.go

Copilot

Copilot review overview

🟡 Changes recommended

The hashing, rate-limiting, keyring fallback, and cleanup flows contain security and operational defects.

Review effort: Balanced
Findings: 6 · 11 · 2

Open (19)

Destructive cleanup test can erase real user credentials · New
Client IP rate limiting is bypassable via forwarded headers · New
Concurrent requests bypass the per-client attempt limit · New
Use constant-time password-hash comparisons
Do not print bootstrap credentials to stdout
Preserve explicitly configured JWT signing keys
Rate-limit entries with fewer failures never expire · New
Increase PBKDF2 iterations to meet OWASP guidance
Retain weak-password warnings before hashing
Validate full hash format before classifying passwords
Reject empty config paths in clean mode
Preserve whitespace in configured passwords
Avoid requiring keyring for configured credentials
Avoid rewriting read-only config during credential resolution
Return errors when cleanup is incomplete
Support the documented single-dash clean flag
Serialize tests that mutate global state
Run table-driven subtests in parallel
Parallelize table-driven subtests

Resolved since last review (1)

Add rate limiting before expensive password derivation

+	// Rate limiting: PBKDF2 with 100k iterations requires significant CPU.
+	// Unauthenticated clients attempting concurrent failed logins could saturate resources.
+	// Reject requests from clients with repeated auth failures (tracked per-IP).
+	clientIP := c.ClientIP()


Closes out the 16 still-open Copilot comments on the PBKDF2 migration: timing-safe hash comparison and a higher iteration count, a misclassifying IsPBKDF2Hash that could lock out an admin whose password started with the hash prefix, a spoofable login rate-limit key via untrusted proxy headers, a check-then-act race and unbounded growth in the login attempt limiter, several admin-credential CLI bugs (dash handling, swallowed clean errors, discarded JWT keys, unconditional config rewrites, mandatory keyring, password trimming), a missing weak-password warning, and an empty --config=/-config= value silently falling back to the default path. Signed-off-by: Nabendu Maiti <nabendu.bikash.maiti@intel.com>

Copilot

Copilot review overview

🟡 Changes recommended

Plaintext secrets can enter logs, configuration rewrites can break read-only deployments, and proxy-aware throttling and API contracts need correction.

Review effort: Balanced
Findings: 3 · 4 · 5

Open (12)

Secrets are exposed through stderr and retained in logs · New
Client IP rate limiting is bypassable via forwarded headers
Do not print bootstrap credentials to stdout
Proxy handling causes shared rate limits and incorrect client IPs · New
Add boundary, reset, expiry, and concurrency rate-limit tests · New
Avoid rewriting read-only config during credential resolution
Serialize tests that mutate global state
Split unrelated credential, JWT, proxy, and throttling changes · New
Document the new 429 response in API contracts · New
Align documented PBKDF2 iterations with implementation · New
Run table-driven subtests in parallel
Parallelize table-driven subtests

Resolved since last review (13)

Concurrent requests bypass the per-client attempt limit
Destructive cleanup test can erase real user credentials
Use constant-time password-hash comparisons
Preserve explicitly configured JWT signing keys
Rate-limit entries with fewer failures never expire
Increase PBKDF2 iterations to meet OWASP guidance
Retain weak-password warnings before hashing
Validate full hash format before classifying passwords
Reject empty config paths in clean mode
Preserve whitespace in configured passwords
Avoid requiring keyring for configured credentials
Return errors when cleanup is incomplete
Support the documented single-dash clean flag

+	// No reverse proxy is trusted by default, so c.ClientIP() (used for
+	// per-client login rate limiting) reads RemoteAddr directly instead of an
+	// attacker-controlled X-Forwarded-For/X-Real-IP header.
+	_ = handler.SetTrustedProxies(nil)


+	if !reserveLoginAttempt(clientIP) {
+		c.JSON(http.StatusTooManyRequests, gin.H{errorKey: "rate_limited", messageKey: "Too many failed login attempts. Please try again later."})

 		return
 	}


+	handled, err := handleAdminCLI(os.Args[1:], newKeyringStorageFunc(), os.Stdout)
+	if err != nil {
+		log.Fatalf("Admin command error: %v", err)
+	}
+
+	if handled {
+		return
+	}


+	if !reserveLoginAttempt(clientIP) {
+		c.JSON(http.StatusTooManyRequests, gin.H{errorKey: "rate_limited", messageKey: "Too many failed login attempts. Please try again later."})


+
+const (
+	pbkdf2Prefix     = "$pbkdf2$"
+	pbkdf2Iterations = 600000


#1297 Adds configurable http.trusted_proxies so ClientIP() isn't forced to ignore every reverse proxy (previously SetTrustedProxies(nil) always, collapsing all clients behind a real proxy into one rate-limit bucket), covers the login rate limiter with tests for the attempt boundary, 429 response, successful-login reset, window expiry, and concurrent reservations, and documents the new 429 outcome in both the OpenAPI declaration and the Postman collection for /api/v1/authorize.

nbmaiti force-pushed the feat/jwt_auth_keystore branch from 73f0f9a to d0d3944 Compare October 6, 2026 07:08

nbmaiti changed the title ~~Feat/jwt auth keystore~~ feat(auth): replace bcrypt with PBKDF2 for password hashing Oct 6, 2026

nbmaiti requested a balanced review from Copilot October 6, 2026 07:52

Copilot started reviewing on behalf of nbmaiti October 6, 2026 07:52 View session

This comment was marked as outdated.

Sign in to view

nbmaiti requested a balanced review from Copilot October 6, 2026 08:35

Copilot started reviewing on behalf of nbmaiti October 6, 2026 08:36 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread internal/controller/httpapi/v1/login.go

nbmaiti requested a balanced review from Copilot October 6, 2026 08:54

Copilot started reviewing on behalf of nbmaiti October 6, 2026 08:55 View session

Copilot AI reviewed Oct 6, 2026

View reviewed changes

Comment thread cmd/app/secret_store.go Outdated

Comment thread pkg/secrets/pbkdf2.go Outdated

nbmaiti added 9 commits October 7, 2026 11:51

fix: update go.sum with proper dependency entries

8697c22

- Run go mod tidy to regenerate go.sum - Add missing golang.org/x/term entry (v0.46.0) - Ensure all transitive dependencies are properly recorded - Fixes CI error: missing go.sum entry for golang.org/x/term

style: apply gofmt formatting to pbkdf2_test.go

61a9a2b

nbmaiti force-pushed the feat/jwt_auth_keystore branch from 0fea9f0 to b054ac4 Compare October 7, 2026 06:27

nbmaiti requested a balanced review from Copilot October 7, 2026 07:15

Copilot started reviewing on behalf of nbmaiti October 7, 2026 07:15 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

nbmaiti requested a balanced review from Copilot October 7, 2026 08:54

Copilot started reviewing on behalf of nbmaiti October 7, 2026 08:54 View session

Copilot AI reviewed Oct 7, 2026

View reviewed changes

Navigation Menu

Repository navigation

feat(auth): replace bcrypt with PBKDF2 for password hashing - #1297

feat(auth): replace bcrypt with PBKDF2 for password hashing#1297
nbmaiti wants to merge 11 commits into
mainfrom
feat/jwt_auth_keystore

nbmaiti commented Oct 6, 2026

Uh oh!

codecov Bot commented Oct 6, 2026 •

edited

Loading

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		if !reserveLoginAttempt(clientIP) {
		c.JSON(http.StatusTooManyRequests, gin.H{errorKey: "rate_limited", messageKey: "Too many failed login attempts. Please try again later."})

Conversation

nbmaiti commented Oct 6, 2026

Changes

Testing

Security

Uh oh!

codecov Bot commented Oct 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

This comment was marked as outdated.

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🟡 Changes recommended

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Oct 6, 2026 •

edited

Loading