fix Calibrate edge cases, improve test coverage#31
Merged
Conversation
Address findings from code review: - validate params before generating salt in GenerateFromPassword - guard division-by-zero in Calibrate when durPerP truncates to 0 - prevent Calibrate from decrementing P to 0 when constraints are unsatisfiable (return P=1 instead of erroring) - reject negative memMiBytes in Calibrate - document that Calibrate always sets R=8 Test improvements: - fix TestGenerateFromPassword: assert invalid params produce errors (9 of 13 test cases were previously dead code) - add TestHashFormat: validate N$R$P$hexsalt$hexdk structure - add TestKnownHash: RFC 7914 Section 12 vector 3 as regression pin - add TestCheck: direct Params.Check() test with boundary values - expand TestCost to cover non-default params - add nil/empty input coverage to TestCompareHashAndPassword - widen TestCalibrate timing tolerance to reduce CI flakiness Also bump go.mod to go 1.21 (matches CI matrix) and gitignore coverage.out.
elithrar
force-pushed
the
code-review-fixes
branch
from
c198adb to
e15d8dc
Compare
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
1 participant
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.
Code review surfaced several bugs in Calibrate and gaps in test coverage.
Calibrate fixes:
int64(dur) / int64(p.P)truncates to 0 in the optimal-P calculationErrInvalidParamsmemMiBytesnot rejectedGenerateFromPassword: validate params before consuming entropy from
crypto/randTest fixes:
TestGenerateFromPasswordnow asserts that invalid params produce errors — 9 of 13pass: falsecases were dead codeTestKnownHashpins RFC 7914 Section 12 vector 3 (pleaseletmein/SodiumChloride/N=16384/r=8/p=1/dkLen=64) as a regression testTestHashFormatvalidates theN$R$P$hexsalt$hexdkoutput structureTestCheckdirectly exercisesParams.Check()with boundary values (N=2, N=3, SaltLen=7/8, DKLen=15/16)TestCostexpanded to cover non-default paramsTestCompareHashAndPasswordcovers nil/empty inputsTestCalibratetiming tolerance widened (timeout/4to3*timeout) to reduce CI flakinessAlso bumps
go.modtogo 1.21to match the CI matrix, and gitignorescoverage.out.