fix(core): use crypto/rand for packet preambles - #1642
Draft
justin-layerv wants to merge 1 commit into
Draft
Conversation
There was a problem hiding this comment.
✅ Looks Good - Code looks good
Clean, well-scoped port that swaps the per-call math/rand seed for crypto/rand. I reviewed correctness, the import change, the callers, and the test.
Verified
- Import cleanup is safe —
math/randwas only referenced by the replaced line;timeis still used elsewhere (utils.go:59), so keeping it is correct. - Contract preserved — public signature stays
GetRandomUint32() uint32, and the rejection loop keeps the non-zero guarantee. Both callers (nhp/core/scheme/curve/header.go:40,nhp/core/scheme/gmsm/header.go:40) use the value as an XOR mask, so behavior is unchanged. - Correct rejection sampling — uniform over
[1, 2³²-1];binary.BigEndian.Uint32reading of 4 fresh bytes each iteration is right, and the retry-on-zero avoids reusing a partially consumed buffer. - Test is not flaky — 1000 draws from a 2³² space give ~1.16e-4 probability of any collision; the test tolerates one collision (fails only on ≥2, P ≈ 7e-9). Good balance between flake-safety and detecting a stuck/low-entropy source. The zero-check also guards the contract.
Minor suggestions (non-blocking)
- Panic on
rand.Readerror: previously this function could never fail; it now panics if the system CSPRNG errors. This is standard Go practice and such failures are essentially catastrophic anyway, so it's acceptable — just noting the behavior change. Callers running underutils.CatchPanic()will recover; a call outside a recovered goroutine would crash the daemon. - Per-packet cost:
crypto/rand.Readis now invoked once per header write. On Linux this is the bufferedgetrandompath and is fast, so no practical concern — flagging only for awareness on high-throughput paths.
Nice improvement: removes the closely-timed-seed collision risk and makes the exported helper safe for future security-sensitive callers.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## main #1642 +/- ##
==========================================
- Coverage 12.54% 12.52% -0.02%
==========================================
Files 96 96
Lines 14526 14526
==========================================
- Hits 1822 1820 -2
Misses 12526 12526
- Partials 178 180 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
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.
What changed
GetRandomUint32's per-callmath/randseed withcrypto/rand.Why
GetRandomUint32is an exported helper used for packet header preambles. Its previous implementation seeded a new pseudo-random generator fromtime.Now().UnixNanoon every call, which could collide under closely timed calls and left a weak-random API available for future nonce or identifier use.The header preamble itself is transmitted in cleartext, so this is not claiming new confidentiality. It removes the per-call seed collision and makes the exported default safe for security-sensitive future callers. This ports the generally applicable core of layervai/nhp#2333 to current OpenNHP.
Validation
go test -race -count=1 ./...innhpgo test -race -count=1 ./ac ./agent ./dbinendpointsgo vet ./...in both Go modules