Repository navigation
refactor: untangle the USM decode and key code - #52
Merged
Merged
Conversation
In FIPS 140-only mode crypto/hmac panics on a key shorter than 14 octets. The signer recovers the panic, writes the stack to stdout and returns no message and no error, so MarshalMsg yields an empty message. A 14-octet key signs.
…eld apart USM unmarshal and setSecurityParameters each wrote the engine-ID adoption (take the ID, drop both keys, derive them for it); both call adoptEngineID now, whose caller holds the lock or owns a private copy, as every decoder does. readAuthParams owns what unmarshal spread over 20 lines: the alias of msgAuthenticationParameters in the packet, its read, the rejection of authenticated flags without an authentication protocol, and the zeroing before the digest check, now marked as the known bug it is. The fuzz known-panic table names the new function. The privacy-parameters check is one condition.
- The authentication and privacy protocol descriptions are arrays indexed by protocol value instead of switches; values outside the table still describe no protocol. - init seeds the salt counter without an error result: crypto/rand.Read never returns one since Go 1.24, so the branches that forwarded it, and validateParameters' check of it, were dead. - The key cache creates the password-to-key hasher on a miss only; a hit allocated one and never used it. Its test checks that a hit creates none. - validate states each passphrase requirement as one condition.
- SnmpPacket.authenticate and UsmSecurityParameters.authenticate sign a message; exchange.authenticate verifies one. The signers are named sign now, and their docs mark their known bugs: the recovered panic (stack on stdout, no message, no error) and the digest written over the first placeholder anywhere in the message. The stdout text is unchanged. - discoveryRequired becomes discoveryPacket, which says what it returns; the discovery packet test builds its packet with it instead of a copy. - testAuthentication chooses the flags and the verifying parameters in one place; the verifier is still dereferenced only when the flags ask for authentication.
Each case passes on the code before the USM decode refactor: - Decoding a message whose flags ask for authentication or privacy fails, with its error text, when the decoder has no protocol for it, NoAuth and NoPriv included: the decoding counterpart of the encoding security-level test. - A keys-only AES user in the trap table rejects a noAuthNoPriv trap from another engine: adopting the engine ID drops the keys, and no AES key derives from the empty passphrase (known bug). - validate reports the missing privacy passphrase first when both are missing.
ilyam8
added this pull request to stack #53
October 13, 2026 06:17
ilyam8
changed the title
ilyam8
marked this pull request as ready for review
October 13, 2026 06:17
stelfrag
approved these changes
Oct 13, 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
2 participants
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
Phase 6 (guardrails), USM decode and keys. No behavior change, no exported API change.
unmarshalandsetSecurityParameterseach wrote the adoption (take the ID, dropboth keys, derive them for it); both call
adoptEngineIDnow. Its caller holds the lock or owns a private copy, asevery decoder does.
readAuthParams. It owns whatunmarshalspread over 20 lines: the alias of msgAuthenticationParameters in thepacket, its read, the rejection of authenticated flags without a protocol, and the zeroing before the digest check,
now marked as the known bug it is. The privacy-parameters check is one condition.
instead of switches.
inithas no error result:crypto/rand.Readnever returns one since Go 1.24. The keycache creates the password-to-key hasher on a miss only (a hit allocated one and never used it).
validatestateseach passphrase requirement as one condition.
SnmpPacket,UsmSecurityParameters) are namedsign, apartfrom
exchange.authenticate, which verifies; their docs mark their known bugs (the recovered panic, the firstplaceholder).
discoveryRequiredbecomesdiscoveryPacket.testAuthenticationpicks the flags and the verifyingparameters in one place.
Validation
(a recovered panic: stack on stdout, no message, no error); decode errors for flags without a protocol (NoAuth and
NoPriv included); a keys-only AES table user rejecting a trap from another engine;
validate's order when bothpassphrases are missing.
apart by any reachable outcome (a NoAuth table entry nothing reads; a privacy check that differs only in an error
text, as in refactor: name the msgFlags authFlag and privFlag tests #51), and one drops the USM parameters' logger, which the engine log transcripts catch.
decode unchanged.
go test -race, FIPS 140-only, golangci-lint, cross-builds, govulncheck: pass locally.