Enforce minimum salt length for PBKDF2 key derivation#15
Open
Enforce minimum salt length for PBKDF2 key derivation#15
Conversation
Short or predictable salts weaken PBKDF2 by enabling precomputation attacks. This adds a validation check (minimum 16 bytes per NIST SP 800-132) to both New and NewWithPasswordNonce constructors, and updates all tests to use compliant salts.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR strengthens the encryption package’s PBKDF2 usage by enforcing a minimum salt length (16 bytes) in constructors, and updates downstream tests/call sites to comply with the new requirement.
Changes:
- Added
MinSaltLengthand avalidateSalt()helper, and invoked validation fromNewandNewWithPasswordNonce. - Updated encryption tests to use 16+ byte salts and added new tests asserting short salts are rejected.
- Updated encrypted cache tests to use a compliant salt.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
encryption/encryptor_decryptor.go |
Enforces minimum salt length via shared validation in both constructors. |
encryption/encryptor_decryptor_test.go |
Updates salts to comply and adds coverage for short-salt rejection. |
cache/encrypted_cache_test.go |
Updates test salt constant to meet the new minimum. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add ErrSaltTooShort sentinel error so callers can use errors.Is - Replace string-matching assertions in tests with errors.Is checks - Change test salt from "salt-sixteen-byte" (17 bytes) to "1234567890abcdef" (exactly 16 bytes) to avoid confusion
rbayerl
approved these changes
Mar 23, 2026
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
NewandNewWithPasswordNonceconstructors in theencryptionpackagenil, empty, or trivially short salts (the existing tests used[]byte("salt")— only 4 bytes)MinSaltLengthconstant so consumers can reference the requirement programmaticallyChanges
encryption/encryptor_decryptor.goMinSaltLengthconst,validateSalt()helper, and validation calls in both constructorsencryption/encryptor_decryptor_test.goTestNew_RejectsShortSaltandTestNewWithPasswordNonce_RejectsShortSaltcache/encrypted_cache_test.goBreaking change
Callers passing salts shorter than 16 bytes will now receive an error. This is intentional — those salts were insecure.
Test plan
go build ./...compiles cleanly