Skip to content

refactor: one resync path in send and the SNMPv3 negotiation in its steps - #45

Merged
ilyam8 merged 7 commits into
engine-exchangefrom
engine-send
Oct 11, 2026
Merged

ilyam8 merged 7 commits into
engine-exchangefrom
engine-send

Conversation

@ilyam8

@ilyam8 ilyam8 commented Oct 9, 2026 •

Copy link
Copy Markdown
Member

Summary

Restructures send, the request boundary, and the SNMPv3 negotiation. No behavior or exported API change.

send handled the two Reports that resynchronize the client (notInTimeWindow, unknownEngineID) in two copies of the same branch, and recognized them with its own switch beside the table answers used.

  • reportKinds (replacing reportErrors): each Report counter gives the request's error, whether it resynchronizes the client, and its name in log lines; reportKindOf serves both answers and send.
  • send keeps the boundary (recover, the Conn and Retries checks, the negotiation) and resynchronizes in one paragraph: update the packet, send it again, return the Report's error if that fails. The retransmission stays in send on purpose: every result passes through its named results, which the recover returns when a panic (user code's included, such as a Logger) ends the request.
  • negotiateInitialSecurityParameters either discovers the engine (discoverEngine, RFC 3414 section 4) or derives the keys; engineFoundDespiteUnknownUser names the tolerance for devices that answer discovery with usmStatsUnknownUserNames. Its guards stay: the client's Logger runs inside send before the negotiation and can change the client.

Each known bug of these functions keeps one site with a known-bug comment (setSecurityParameters, storeSecurityParameters and the discovery exception of testAuthentication included); the stale RFC 2574 comment is replaced.

The test commits pin what mutation probes and two reviews found unobserved, each passing on the previous code: a panic during the retransmission (from a hook or the Logger), hooks and a Logger that change the client, Reports answering the retransmission, a resync Report for another request ID, a discovery answer without an engine ID (AES, DES, MD5), and the engine time of the retransmission's answer (several of these are known bugs). BenchmarkGet and BenchmarkGetV3 run through send (BenchmarkSendOneRequest stops below it).

Testing

  • go test ./... on darwin/arm64 and GOARCH=386; -race; the FIPS 140-only tests; golangci-lint v2.14.0 (incl. end2end): 0 issues; GOOS=windows go vet.
  • Goldens unchanged by the refactor commits. Logger output unchanged: the engine suites rerun with a recording Logger (method, format, arguments) on the old and new code give the same 3081 calls.
  • Mutation probes on the new code (56, plus 16 from review): all caught except 2 equivalent, 3 reachable only through user code changing the client mid-request, and 2 that change only the Logger call (caught by the Logger comparison).
  • An independent equivalence review compared the old and new code side by side: every pinned scenario with and without a Logger, 800k random scenarios (v2c and v3, with Reports in the first and the retransmission's answers), a matrix of user-hook and Logger actions at every call point, FIPS 140-only, and loopback TCP. Its findings are fixed in this PR; no network-driven input differs.
  • Interleaved A/B against the previous code: BenchmarkSendOneRequest, BenchmarkGet and BenchmarkGetV3 within noise, B/op and allocs/op identical.

@ilyam8
ilyam8 added this pull request to stack #38 October 9, 2026 21:08
@ilyam8 ilyam8 changed the title engine send refactor: one resync path in send and the SNMPv3 negotiation in its steps Oct 9, 2026
@ilyam8
ilyam8 force-pushed the engine-send branch 2 times, most recently from 053f493 to 924c431 Compare October 11, 2026 09:46
Mutation probes on send's restructuring found its resynchronization path
unpinned in four places:

- resync/panic-in-retransmission: a panic during the retransmission is
  returned as the recover error together with the Report, the result the
  request had when it panicked (send's named results).
- resync/security-model-changed-by-hook: a hook that changes the client's
  security model before the retransmission makes the packet update fail,
  and the request returns its error.
- resync/retransmission-answered-by-report: a Report answering the
  retransmission is returned with ErrNotInTimeWindow (known bug).
- resync/retransmission-answer-time: the engine time of the answer to the
  retransmission is not stored (known bug).

All four pass on the current code.
…teps

send handled the two Reports that resynchronize the client, notInTimeWindow
and unknownEngineID, in two copies of the same branch, and recognized them
with its own switch beside the table answers used.

- reportKinds replaces reportErrors: each Report counter gives the
  request's error, whether it resynchronizes the client and its name in log
  lines; reportKindOf serves answers and send.
- send keeps the boundary (recover with its named results, the Conn and
  Retries checks, the negotiation) and hands a resync Report to resync,
  which updates the packet, sends it again and returns the Report's error
  if that fails.
- negotiateInitialSecurityParameters discovers the engine (discoverEngine)
  or derives the keys; engineFoundDespiteUnknownUser names the tolerance
  for devices that answer discovery with usmStatsUnknownUserNames. Its two
  guards are gone: mkSnmpPacket copies the client's version and security
  model, send negotiates only for an SNMPv3 packet, and no hook runs in
  between.

Each known bug of these functions keeps one site with a known-bug comment,
setSecurityParameters' included; the stale RFC 2574 comment is replaced.

No behavior change: goldens and logger output unchanged.
BenchmarkSendOneRequest stops below send, so the negotiation, the store
and the resync dispatch had no benchmark. BenchmarkGet runs an SNMPv2c Get
on the in-memory transport; BenchmarkGetV3 an authNoPriv Get after
discovery against the fake agent.
…hanges the client

Review of send's restructuring found two paths no scenario observed; both
pins pass on the code before it.

- discovery/report-without-engine-id/{md5,sha-aes}: a discovery Report
  without an engine ID is accepted and the request goes out with an empty
  engine ID; at authPriv it fails in the encoder for want of a privacy key
  (known bug).
- hooks/logger-changes-{security-model,version}: the client's Logger is user
  code that runs inside send before the negotiation; when it changes the
  client's security model or version, the negotiation's guards end the
  request with an empty packet.
…wn bugs

Review of send's restructuring:

- The two guards of negotiateInitialSecurityParameters come back: the
  client's Logger runs inside send between mkSnmpPacket and the
  negotiation, so a Logger that changes the client reaches them.
- The retransmission failure's log line passes the same format and
  argument to the Logger as before (the text was the same, the call was
  not).
- Known-bug comments: setSecurityParameters adopts boots and time from any
  accepted message and keeps the time without the moment it arrived (the
  150 s notInTimeWindow round trip, moved here from resync);
  storeSecurityParameters takes the context engine ID only while it is
  empty; discovery accepts an answer without an engine ID; the discovery
  exception of testAuthentication skips the digest for any message with an
  empty user name and engine ID; resync returns a nil error for a resync
  Report only.
- reportKind.resync and reportKindOf say what they decide more precisely.

No behavior change: goldens and logger output (method, format, arguments)
unchanged.
All pass on the code before send's restructuring:

- hooks/logger-panics-after-failed-retransmission: a Logger that panics
  on the retransmission's failure line ends the request with the Report
  that answered the retransmission, not the first one.
- hooks/onfinish-changes-version: a hook that changes the client's
  version during the request makes the store fail, and the request returns
  its error with the reply.
- resync/report-for-other-request-id: a notInTimeWindow Report for
  another request ID still resynchronizes the request (known bug).
- resync/retransmission-answered-by-authenticated-report: the engine time
  of an authenticated Report answering the retransmission is not stored
  (known bug).
- discovery/report-without-engine-id/sha256-des: with DES, a discovery
  answer without an engine ID makes the encoder panic (known bug).

onLogging replaces the Logger helper of the previous pins and records the
line it acts on.
send's recover returns the result held by its named results when a panic,
user code's included, ends the request. With the retransmission in a
helper, a Logger panicking on the retransmission's failure line returned
the first Report instead of the retransmission's answer. The
resynchronization is one paragraph of send again, with its known-bug
comments; resync is gone.

No behavior change: goldens and logger output unchanged.
@ilyam8
ilyam8 merged commit 138938b into master Oct 11, 2026
21 checks passed
@ilyam8
ilyam8 deleted the engine-send branch October 11, 2026 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants