bgp: parse NOTIFICATIONs, enforce hold-timer, support TCP-MD5 auth#6
Merged
Conversation
Previously the drive loop read from the TCP stream into a 64-byte
buffer and silently discarded whatever arrived — comment at the
discard site admitted "A strict parser is out of scope". In
production that meant an operator had no way to tell *why* a peer
closed the session: route policy mismatch, hold-timer expiry, admin
shutdown, all looked identical (transient disconnect, retry, repeat).
Now: the drive loop accumulates inbound bytes into a `Vec<u8>`,
`take_message` extracts whole BGP messages from it (self-healing on
marker desync, bounded by RFC 4271 §4.1 max length), and any
NOTIFICATION yields a `BgpEvent::PeerNotification { peer_ip,
error_code, error_subcode }`. The event gets logged at `warn!` with
the human-readable name from RFC 4271 §4.5, and `lb-node` increments
a new labelled counter.
Other peer-initiated messages (KEEPALIVE, UPDATE) are still ignored
— this commit is scoped to NOTIFICATION; a strict UPDATE parser
isn't on the roadmap.
New encoding helpers in `lb-bgp::messages`:
* `encode_notification(code, subcode)` — the RFC 4271 §4.5 layout
with the body's variable data omitted (every widespread BGP
speaker treats it as opaque). Used by the upcoming hold-timer
expiry commit too.
* `parse_notification(data) -> Option<(u8, u8)>`
* `notification_error_name(code) -> &'static str`
* `mod error_code` with the two codes we emit ourselves
(`HOLD_TIMER_EXPIRED=4`, `CEASE=6`).
New metric:
* `lb_bgp_peer_notifications_total{peer_ip, code, subcode}` —
labelled by the specific error so operators can alert on sudden
spikes of Cease (admin shutdown) vs UPDATE errors.
One new integration test
(`peer_notification_tears_down_session_and_emits_event`) uses the
newly-extensible `MockRouter::inject_bytes` to push a NOTIFICATION
at the speaker mid-session and asserts the event fires with the
right code/subcode.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
RFC 4271 §6.5 requires the speaker to tear a session down when no
BGP message arrives within the negotiated hold time. Previously our
session only *generated* KEEPALIVEs at `hold / 3` and never checked
for inbound silence — a peer that went away without closing TCP
(black hole, routing fault, kernel freeze) left us in `Established`
indefinitely, with BGP announcements still nominally active but no
one listening.
Two changes plumbed through:
1. Hold-time negotiation. `BgpSpeaker::connect` now pulls the peer's
hold_time out of their OPEN message and returns the negotiated
value (`min(our_hold, peer_hold)`) alongside the stream. Helper
`messages::parse_open_hold_time` is new, with its own round-trip
test. The keepalive cadence is derived from the *negotiated*
value, not our advertised one — matches RFC behaviour when the
peer advertises a shorter hold.
2. Hold-timer enforcement. The drive loop now tracks `last_inbound:
Instant` (bumped on every successful `read`) and races a
`sleep_until(last_inbound + negotiated_hold)` against the other
select arms. If the sleep wins, we emit a NOTIFICATION (error
code 4 = Hold Timer Expired) best-effort on the dying stream,
log at `warn!`, and return a transient disconnect so the
backoff/reconnect path takes over.
`negotiated_hold == 0` (peer disabled keepalives per RFC 4271 §4.2)
is handled too: both the keepalive ticker and the hold timer are
gated off, `select!` ignores those arms entirely.
Testing:
* `hold_timer_expires_and_fires_notification` — integration test
with `hold_time_secs = 2`, asserts the mock router observes a
HOLD_TIMER_EXPIRED NOTIFICATION on the wire within 5s of the
handshake completing.
* MockRouter grew a `RouterEvent::Notification { code, subcode }`
so tests can assert on the exact bytes the speaker sent.
* `parse_open_hold_time` round-trip tests in `messages`.
All 210 workspace tests pass; clippy + fmt clean.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously the BGP speaker opened sessions in plain TCP. For peering
across administrative domains (the typical production setup — this LB
sits between two ASes worth of routing trust) this is unsafe: an
attacker on the path can inject UPDATEs and redirect traffic.
New per-peer config field `md5_password: Option<String>`. When set,
the speaker installs `TCP_MD5SIG` on the socket before `connect()`
(via `tokio::net::TcpSocket` so the sockopt lands on the socket
*before* the SYN goes out — a missing signature on the handshake
would be rejected by the peer). The helper:
* lives in `speaker.rs` because MD5 is a per-session concern, not
protocol framing;
* is gated behind `#[cfg(target_os = "linux")]` — `TCP_MD5SIG` is
a Linux-specific kernel option. On other OSes (macOS for local
dev) the stub logs a `warn!` and returns `Ok(())` so peering
still proceeds in plain TCP, which fails cleanly at handshake
rather than mysteriously;
* enforces the Linux `TCP_MD5SIG_MAXKEYLEN=80` byte cap, returning
`InvalidInput` on oversize keys;
* defines the `tcp_md5sig` layout locally (the `libc` crate
doesn't expose it uniformly across versions);
* only handles IPv4 peers today, consistent with H5 being out of
scope.
Config, test, and docs updates:
* `lb-types` gets the new field + a round-trip test.
* `lb.example.toml` shows the commented-out line with the warning.
* `.docs/operations.md` BGP peer table lists the knob with the
"treat the config file as a secret" note.
Not tested end-to-end: MD5 needs a peer configured with the same
key. Testing the `setsockopt` call would require either mocking
`libc` or running against a real MD5-speaking peer in CI; both are
out of scope for this PR. The code path is exercised by
`cargo build` on Linux CI.
Workspace tests 210 → 211.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Clippy 1.95 (`collapsible_match`) flags a nested `match { Some(...) =>
{ if ... } }` as reducible to a match guard. We can't use the
guard form here because the inner check calls `.await`, which isn't
allowed in a match guard; let-chains would work but the workspace is
still on edition 2021. Easiest fix: collapse via a small binding
rather than nested scrutinees.
No behaviour change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
3 tasks
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.
Summary
PR 2 of the production-readiness remediation plan. Closes the three Tier-3 BGP findings from the audit: H1 (hold-timer expiry), H2 (NOTIFICATION parsing), H3 (MD5 auth). Three atomic commits. Workspace tests 203 → 211;
cargo clippy --workspace --all-targets -- -D warningsclean; fmt clean.Commits
1.
Parse inbound BGP NOTIFICATION messages(H2)The drive loop used to read into a 64-byte buffer and silently discard whatever arrived — the comment at the discard site admitted "A strict parser is out of scope". Operators had no visibility into why peers closed sessions.
Now: inbound bytes accumulate,
take_messageextracts whole BGP messages (self-healing on marker desync, bounded by RFC 4271 §4.1 max length), and any NOTIFICATION yields a newBgpEvent::PeerNotification+ awarn!log + alb_bgp_peer_notifications_total{peer_ip, code, subcode}counter increment. Other peer-initiated messages (KEEPALIVE, UPDATE) are still ignored — a strict UPDATE parser isn't on the roadmap.New helpers in
lb-bgp::messages:encode_notification(code, subcode)parse_notification(data) -> Option<(u8, u8)>notification_error_name(code) -> &'static strmod error_code { HOLD_TIMER_EXPIRED, CEASE }for the codes we emit ourselvesNew integration test
peer_notification_tears_down_session_and_emits_eventuses the newly-addedMockRouter::inject_bytesto push a Cease NOTIFICATION mid-session and asserts the event fires with the right code/subcode.2.
Enforce BGP hold-timer expiry(H1)BgpSpeaker::connectnow parses the peer's OPEN hold-time and returns the RFC-4271-negotiated value (min(our_hold, peer_hold)) alongside the stream. The drive loop trackslast_inbound: Instant(bumped on every successfulread) and races asleep_until(last_inbound + negotiated_hold)against the other select arms; if the sleep wins, we send a HOLD_TIMER_EXPIRED NOTIFICATION and return a transient disconnect.negotiated_hold == 0(peer disabled keepalives per RFC §4.2) disables both the keepalive ticker and the hold timer.Integration test
hold_timer_expires_and_fires_notificationconfigures the peer withhold_time_secs = 2, asserts the mock router observes a HOLD_TIMER_EXPIRED NOTIFICATION on the wire within 5s of handshake completion.3.
Support RFC 2385 TCP-MD5 authentication per peer(H3)New optional
md5_password: Option<String>per peer config. When set, the speaker installsTCP_MD5SIGon the socket beforeconnect()(viatokio::net::TcpSocketso the sockopt lands before the SYN). The helper:speaker.rsas a per-session concern#[cfg(target_os = "linux")]— TCP_MD5SIG is Linux-specificwarn!and returns Ok so peering attempts plain TCP and fails cleanly at handshake rather than mysteriouslyTCP_MD5SIG_MAXKEYLENcap, returningInvalidInputon oversize keysstruct tcp_md5siglocally (thelibccrate doesn't expose it uniformly)Not tested end-to-end: runtime MD5 verification needs a real peer configured with the same key, or a full libc mock — out of scope for this PR. The code path is exercised by
cargo buildon Linux CI.Test plan
cargo test --workspace→ 211 passingcargo clippy --workspace --all-targets -- -D warningscleancargo fmt --all --checkcleanlb-bgpbuilds with thecfg(target_os = "linux")libcdependency resolvedhold_time_secsand reconnectsWhat's next
Per the plan:
feat/fragment-reassembly(H4) — wire the existingFragmentTableinto the hot path.feat/io-backend-selection(S1/S2/S3) — delete DPDK stub, config-driven backend, complete AF_XDP viaxdpiloneor similar.feat/lb-trace(H6).chore/hardening(M2–M5, M7).🤖 Generated with Claude Code