Skip to content

fix(rust-plugins): SNMP protocol hardening — validate agent responses, bound walks, timeouts & retries - #6413

Open
julienmathis wants to merge 3 commits into
developfrom
feat/snmp-protocol-hardening
Open

julienmathis wants to merge 3 commits into
developfrom
feat/snmp-protocol-hardening

Conversation

@julienmathis

Copy link
Copy Markdown
Contributor

Summary

The SNMP engine blindly trusted the agent it queried — yet the agent is precisely the untrusted party of the dialogue.

  • Agent errors are now detected: a response with error-status != 0 produces a typed error naming the RFC 3416 status, e.g. UNKNOWN: SNMP agent returned an error: noSuchName (status 2, index 3).
  • Request/response correlation: every request now carries a fresh request-id, and received datagrams are validated (request-id, community echo, PDU type) before being accepted — an unrelated datagram (e.g. a late retransmission of a previous request) is discarded instead of being consumed as the current answer.
  • Bounded walks: OIDs must be strictly increasing during a walk (the classic snmpwalk "OID not increasing" guard) and a single walk may collect at most 100 000 values — both protect against a buggy or malicious agent looping the walk or streaming endless data.
  • Configurable timeouts and retries, three new CLI options:
    Option Default Role
    --timeout <s> 1 Receive timeout per request attempt
    --snmp-retries <n> 2 Retries after a timed-out attempt
    --collect-timeout <s> 50 Global budget for the whole collection (all gets + walks share one deadline) — exits with a clean UNKNOWN before centengine's kill timeout
  • The UDP receive buffer was 1024 bytes, silently truncating large bulk responses; raised to 65535.

Connection parameters (target, community, timeouts, retries) are now threaded through a single SnmpConfig instead of loose string arguments, and the two walk loops (snmp_bulk_walk / snmp_bulk_walk_with_labels) share one implementation.

Test plan

  • cargo build --release succeeds
  • cargo test: 75 passed, 0 failed (10 new tests: RFC 3416 error names, response validation — valid / wrong id / wrong community / agent error —, non-increasing OID ×2, varbind cap, expired deadline)
  • Manual check against an unroutable address (TEST-NET, 203.0.113.1):
    • default settings → UNKNOWN: No valid SNMP response from 203.0.113.1:161 after 2 attempts (timeout 1s per attempt) after ~2s, exit 3
    • --collect-timeout 2 with a longer per-attempt timeout → UNKNOWN: SNMP collection exceeded the global timeout of 2s after ~2s, exit 3 — the global budget correctly preempts the attempt schedule

…, bound walks, timeouts & retries

The engine blindly trusted the agent it queried, which is precisely the
untrusted party of the dialogue:

- Agent errors are now detected: a response with error-status != 0
  produces a typed error naming the RFC 3416 status (e.g. "noSuchName").
- Request/response correlation: every request carries a fresh
  request-id, and received datagrams are validated (request-id,
  community echo, PDU type) before being accepted — unrelated
  datagrams (e.g. a late retransmission of a previous request) are
  discarded instead of being consumed as the current answer.
- Bounded walks: OIDs must be strictly increasing during a walk (the
  classic snmpwalk "OID not increasing" guard) and a single walk may
  collect at most 100 000 values — both protect against a buggy or
  malicious agent looping the walk or streaming endless data.
- Configurable timeouts and retries via three new CLI options:
  --timeout (per-attempt receive timeout, default 1s), --snmp-retries
  (default 2), and --collect-timeout (global budget for the whole
  collection, default 50s, so the plugin exits with a clean UNKNOWN
  before centengine's own kill timeout).
- The UDP receive buffer was 1024 bytes, silently truncating large
  bulk responses; raised to 65535.

Connection parameters (target, community, timeouts, retries) are now
threaded through a single SnmpConfig instead of loose string
arguments, and the two walk loops share one implementation.

Test plan:
- cargo build --release succeeds
- cargo test: 75 passed, 0 failed (10 new: RFC 3416 names, response
  validation, non-increasing OID, varbind cap, expired deadline)
- Manual check against an unroutable address: default settings exit
  UNKNOWN after retries*timeout with the attempt count in the message;
  --collect-timeout correctly preempts a longer per-attempt timeout
…-failure message

The protocol-hardening commit changed the no-connection error message
from "Could not connect to X is the hostname..." to "No valid SNMP
response from X after N attempts (timeout Ts per attempt)" (clearer:
it names the retry budget actually exhausted), but never updated the
Robot fixture that pins it. Both cgs-no-connection cases have been
failing CI since this branch was opened.
@julienmathis
julienmathis requested a review from a team as a code owner September 8, 2026 12:13
@julienmathis
julienmathis requested review from sechkem and removed request for a team September 8, 2026 12:13
snmp_bulk_get built a GetBulkRequest PDU for every "get" query, relying on
a workaround that stripped a trailing .0 and depended on GetNext landing
exactly one leaf ahead. GetBulk (even with max-repetitions=1) can never
perform an exact match, so this silently returned the wrong value for any
non-.0-suffixed OID, such as a specific row of a multi-row table.

Every currently shipped definition only queries .0-suffixed scalars, so
the bug was latent. Switch to a real GetRequest and drop the trailing-zero
workaround, which is no longer needed.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant