fix: accept an acknowledgement carrying a VarBindList - #13
Conversation
The subagent never registers against net-snmp. It connects, sends Open, and rejects the reply, logging "AgentX session ended: unexpected AgentX acknowledgement" before backing off 1, 2, 4, 8, 16 and 30 seconds forever. acknowledge() required the Response payload to be exactly 8 bytes. A Response PDU is sysUpTime, error and index followed by a VarBindList (RFC 2741 6.2.11), and the master may populate it. net-snmp does. Probing a live master directly over its Unix socket: OPEN -> type=18 payload_length=40 error=0 session=21 REGISTER -> type=18 payload_length=32 error=0 session=21 Type, transactionID and packetID all matched and the error was noAgentXError, so both operations had in fact succeeded and the session was granted. Only the length check refused them, so the region was registered on the master while the subagent tore the connection down. Comparing against 8 rather than requiring equality fixes it. The function only reads the fixed 8 byte block at bytes[24..28], so a longer payload needs no other handling. Reproduced on two unrelated hosts: Ubuntu 22.04 with net-snmp 5.9.1 and Debian 13 with a healthy, idle snmpd. The existing coverage misses it because the test master answers with Response::from_header, whose VarBindList is empty and whose payload is therefore exactly 8, which no real master sends. The added test drives acknowledge() over a socket pair with a net-snmp shaped reply for both Open and Register, and fails with the production error message when the comparison is restored.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe acknowledgement parser now accepts AgentX Response PDUs with payloads of at least 8 bytes. A test verifies Open and Register acknowledgements that include VarBindList data. ChangesAcknowledgement payload handling
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The acknowledgement parser now accepts valid longer responses without weakening validation of the required fixed fields. No actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit checks the response byte, Comment |
The problem
The subagent never registers against net-snmp. It connects, sends Open, rejects the reply, and loops:
Walking
1.3.6.1.2.1.31.1.2returns nothing, because the session is torn down before it serves anything.Cause
acknowledge()required the Response payload to be exactly 8 bytes. A Response PDU issysUpTime,errorandindexfollowed by a VarBindList (RFC 2741 §6.2.11), and the master is free to populate it. net-snmp does.Probing a live master directly over its Unix socket with a hand-rolled Open and Register:
Type,
transactionIDandpacketIDall match and the error isnoAgentXError, so both operations actually succeeded and the session was granted. Only the length check refused them, which means the region was registered on the master while the subagent tore the connection down.The fix
Compare against 8 instead of requiring equality. The function only reads the fixed 8-byte block at
bytes[24..28], so a longer payload needs no other handling.Why existing tests missed it
tests/support/agentx.rsanswers withResponse::from_header(...), whose VarBindList is empty and whose payload is therefore exactly 8 — a length no real master sends. The suite has never exercised a response shaped like net-snmp's.The added unit test drives
acknowledge()over a socket pair with a net-snmp-shaped reply for both Open and Register. Restoring the old comparison makes it fail with the production message:Reproduced on
Two unrelated hosts, so this is not load or version specific:
Both installed from the v0.2.0 release
.deband run in the foreground.Checks
cargo fmt --check,cargo clippy --all-targets -- -D warnings,cargo test(47 + 6 + 1, 7 root-only ignored) andscripts/opengrep-scan.shall pass.One unrelated observation
acknowledge()collapses type, payload length, packet id, transaction id and session id into a single message, so every cause reads the same. Naming the mismatched field and its value would turn a multi-run investigation into one. Happy to follow up separately if useful.Summary by CodeRabbit