Conversation
The data plane protocol had nowhere to put an authentication key, so a session that authenticates could be offloaded and would then be transmitted in the clear while `show bfd peer` reported it as authenticated. Sending the key that is active right now is not enough. A key chain rolls over on a clock: `bfd_keychain_key_find_active()` is consulted from ptm_bfd_snd() and bfd_check_auth() on every packet, so the software path follows a rollover without being told. Nothing re-reads a lifetime for an offloaded session, and there is no expiry hook in the key chain, so a data plane handed the active key would keep signing with it long after it expired. Pushing a new key at every boundary would put the daemon back in a path that offloading exists to keep it out of. So send the keys with the periods in which each may be used, and let the data plane decide. It has the packets, so it is the only side that can tell which key applies to one. The two periods overlap on purpose during a rollover, a key ceasing to be used for transmit before it ceases to be accepted, and a data plane holding a single key cannot honour that. DP_SESSION_AUTH carries the keys and is variable length, since a key chain may hold several. The array is declared at its full size inside the message so a receiver may copy a message in and read it in place, which is how every other message here is handled; only the keys that are present go on the wire. SESSION_AUTH says whether a session authenticates at all, so a data plane that cannot use keys can refuse it rather than run it unprotected, and so that removing a key chain is signalled by the flag clearing rather than by an empty message. There is no capability exchange in this protocol, so nothing can be enforced from this end. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
The listener stands in for a data plane in the topology tests, so it has to be able to say what it was sent. Count the sessions that arrived announcing authentication, and keep the keys from the most recent message carrying any, with the lifetimes as they arrived rather than as they were configured. Copying a message into `struct bfddp_message` is safe again now that the key array is declared at its full size there, but it is worth noting that it was not while the keys merely followed the header: a message carrying three keys is larger than that structure used to be. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Check that the session announces it authenticates, that every key in the chain is sent rather than only the one in use, that a key added later is sent too, and that the lifetimes survive the trip. The lifetimes are compared against each other rather than against fixed timestamps, because the daemon reads them from a local time and the test would otherwise depend on the zone it runs in. What matters is the shape: a key stops being used to transmit before it stops being accepted, and the next key is accepted before it starts being sent. That overlap is the whole reason the periods travel with the keys. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Greptile SummaryThis PR extends the BFD data-plane protocol to carry complete authentication key chains and key lifetimes, allowing offloaded sessions to handle authentication rollover without returning to the daemon’s per-packet path.
Confidence Score: 5/5The PR appears safe to merge, with no accepted new findings or outstanding previous findings. All previous findings are resolved, and the changes since the previous review introduce no distinct actionable issue eligible for a new comment. Important Files Changed
Sequence DiagramsequenceDiagram
participant CFG as Keychain configuration
participant BFD as bfdd
participant DP as BFD data plane
CFG->>BFD: Key or lifetime update
BFD->>BFD: keychain_updated hook
BFD->>DP: "DP_ADD_SESSION / session update<br/>SESSION_AUTH set"
alt Confined transport and enqueue succeeds
BFD->>DP: "DP_SESSION_AUTH<br/>keys + send/accept lifetimes"
DP->>DP: Select keys as lifetimes roll over
else Transport refused or enqueue fails
BFD->>DP: DP_DELETE_SESSION
BFD->>BFD: Run session in software
end
Reviews (8): Last reviewed commit: "tests: do not require openssl to test th..." | Re-trigger Greptile |
The `keychain_updated` hook fires when a key is created or removed, when its string changes and when its algorithm changes. It does not fire when a send or accept lifetime changes, so a user holding the chain is never told that the period a key may be used in has moved. That went unnoticed because the users in tree read the chain on the path that needs it: `key_lookup_for_send` and `key_lookup_for_accept` are consulted per packet, so a changed period takes effect on the next one without anyone being notified. A user that was handed the periods ahead of time instead has nothing to re-read, and there is no expiry hook in the key chain to fire on its behalf either. bfdd offloading a session to a data plane is such a user. Fire the hook from the five lifetime callbacks the way the other leaves under a key already do. The two dnode helpers hand the keychain back so each callback has the name to notify with. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
A key chain may hold more keys than one message can carry, and the loop stopped at the first `BFDDP_AUTH_KEY_COUNT_MAX` it found in list order. List order is key id order, which has nothing to do with which keys are live, so a chain long enough could spend every slot on keys whose accept period had already closed and send none of the key it is about to roll on to. The data plane would then refuse the packets that arrive after the handover, which is the one moment this message exists for. Skip a key whose accept period has ended. It can never verify a packet again on either side, so it is only history, and the slots are few. The period is read the way `key_valid` reads it: a key is stored zeroed, so a zero start means no lifetime was configured and the key is always acceptable. Only a key that was given a period can fall out of one. The other four reasons a key is not offloaded were silent. An operator whose chain reached the data plane with fewer keys than were configured had nothing to look at, and the four are all things they can act on: an algorithm with no BFD equivalent, an empty or over-long key, a key id past the eight bits the wire has for it, and now a chain with more live keys than fit. Say which key and why. The topotest chain moves to 2040 because of the first change. Dating a fixture around the day it was written means a key silently drops out of it later, and it would have taken the overlap the test demonstrates with it. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
A session and its keys are two messages, and the output queue can take the first and refuse the second. On registration that is already handled: `_bfd_dplane_add_session` drops the association, `bfd_dplane_add_session` returns non-zero, and the session runs in the daemon instead. A later update has no such recovery. The session is offloaded already, and four of the five callers of `bfd_dplane_update_session` discard what it returns, so a data plane could be left holding `SESSION_AUTH` with keys that no longer match the configuration and nothing anywhere would say so. A conforming data plane fails closed on that - keys it cannot use mean a session it must not run - so the visible symptom is a session that will not come up, with no reason given. Say so. Recovering is a larger question than this message: the protocol has no transaction, and a retry needs somewhere to hold the state until the queue drains. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
…ect them Every other message in this protocol describes a session. This one carries the key that protects it, and the protocol runs over whatever `--dplaneaddr` names: a UNIX socket, or a plain TCP connection with no transport security and no peer authentication. On such a connection bfdd cannot tell who accepted it and cannot stop anyone on the path from reading it, so a key put on it is a key given away, and an attacker holding it can impersonate the peer on a session that was configured precisely to stop that. The documentation already asks an operator to keep this interface to trusted parties, but that is advice about who connects, and it does nothing about who reads. Offload a session that authenticates only over a UNIX socket, whose permissions the documentation already covers, or a loopback address, which does not leave the host. Anything else refuses, which propagates as a registration failure, and `bfd_session_apply` then runs the session in the daemon where it authenticates as usual. A session is never downgraded to running unprotected, and the refusal is logged. The test is on the connection rather than on a knob because there is no answer an operator could give that would make the key safe to send. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
The suite checked that a key reaches the data plane when it is added. It did not check that editing one reaches it, and a lifetime is the edit that matters here: it is the field this message exists to carry, and the one the key chain was not notifying about. Change an accept lifetime on a key the listener already holds and require the value it holds to move. Reading the new value back rather than a count is what separates this from the case above it, which a resend of the old set would also satisfy. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
|
@greptileai review |
The confinement test compared against INADDR_LOOPBACK, which is the single address 127.0.0.1. The whole of 127.0.0.0/8 is loopback and --dplaneaddr takes any of it, so a data plane reached at 127.0.0.2 was refused its keys and the session fell back to the daemon for no reason. Use IPV4_NET127, which lib already has, and apply it to the v4 mapped form as well, since a v4 client on a dual stack listener arrives mapped and the whole range maps. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
Skipping keys whose accept period had closed stopped the message being spent on history, but what was left was still taken in list order, which is key id order and says nothing about when a key matters. A chain with more simultaneously usable keys than one message holds could therefore fill every slot with rollovers years out and send none of the key the peer is signing with right now. Take two passes. The first offloads every key that is acceptable at this moment, because those are the ones the data plane needs to verify the packets arriving at it, and nothing may displace them. The second fills whatever room is left with the keys the chain has yet to roll on to, in chain order, so the next handover is covered before a distant one. The per-key checks move into `bfd_dplane_classify_key` so the two passes cannot disagree about what is usable, and only the first pass warns, so a key is complained about once rather than twice. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
A session and its keys are two messages on one queue, and the queue can take the first and refuse the second. Logging that was not enough: the session stays offloaded, so the data plane goes on running it while believing it is protected by keys that are no longer the configured ones. There is no transaction in this protocol to roll the pair back with. Take the session back instead. `bfd_dplane_delete_session` asks the data plane to drop it, best effort on the same queue, and clears the association whether or not that message fits; `bfd_session_enable` then starts the session in the daemon, where it authenticates as usual. Losing the fast path is the smaller harm, and it is the same fallback a data plane disconnect already performs in `_bfd_session_unregister_dplane`. Registration must not do this. `bfd_session_enable` is what calls `bfd_dplane_add_session` to begin with, so falling back from underneath it would re-enter it with the association already cleared and recurse. That path already drops the association and returns non-zero, and the caller opens a socket instead, so it is left alone: the detach is behind a flag that only `bfd_dplane_update_session` sets. `bfd_dplane_update_session` loses its const, since taking the session back writes to it. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
|
@greptileai review |
Registration queues `DP_ADD_SESSION` before the keys are built, so by the time they are refused the data plane has already been told to run the session. Clearing only the local association left it holding one bfdd believes it never offloaded: bfdd starts the session in software, the data plane goes on answering for it, and a data plane that ignores `SESSION_AUTH` answers without authentication. That is the downgrade the confinement check exists to prevent, and the check made it certain rather than unlikely. Before it, the only way to fail here was an enqueue that did not fit, which the session message itself would usually have hit first. A connection the check rejects fails every time, with the session message already on the queue. So send `DP_DELETE_SESSION` on this path too, rather than only when a later update fails. The withdrawal is now unconditional and only the software fallback stays behind the flag, since registration reaches here from inside `bfd_session_enable` and cannot call it again. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
|
@greptileai review |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
@greptileai review |
`getpeername` on a socket that is still connecting fails, and bfdd enqueues to a data plane in exactly that state: `bfd_dplane_enqueue` has a case for the client still connecting, so a session registered in that window reaches the confinement check before the connection completes. The check read the error as "cannot tell", refused the keys, and withdrew the session. In client mode there is no need to ask the socket. bfdd was told which address to connect to, `bdc->addr` holds it from context creation onwards, and that is the address whose reachability the check is about. Server mode still asks the socket, because there `bdc->addr` is what bfdd bound and says nothing about who reached it. An accepted socket is connected by definition, so the failure above cannot arise there. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
|
@greptileai review |
Authentication was the last section of RFC 5880 this engine did not implement, and the only one that could not be done the way the others were: the kernel offers an XDP program no way to compute a keyed digest, bpf_crypto_* being skcipher only, no hashing and no HMAC anywhere in the BTF. Keeping authenticated sessions in userspace would have withdrawn the engine's whole property from exactly the sessions an operator asked to protect, so the digest is carried in tree, in one header both planes include. Two implementations of one digest disagree silently. All three types bfdd can produce run in the program. Keyed SHA1 went first, because its packet is a constant 52 bytes; simple password was the harder one, since 24 + 3 + the key length is a runtime length and the verifier wants a bound on every access. bpf_xdp_load_bytes and bpf_xdp_store_bytes carry that, and the fixed scratch block they read into also fixed an IPv6 UDP checksum fold that had presented as an authentication failure across 58 flaps with both auth counters flat. Acceptance is two sided, as s6.8.6 requires: an authenticated packet on a session with no key is discarded, and so is a bare packet on a session that has one. The second half is what stops a peer stripping authentication simply by not offering it. The replay window follows s6.7.4 rather than approximating it, with an upper edge, a circular comparison, and a window forgotten after twice the detection time so a restarted peer can resynchronise. That last rule lives in the sweep, because the program validates authentication whether or not it is answering for the session, so a resync living in userspace could never fire. The same reasoning covers demand mode, where the window has to age ahead of the detect hold or a peer restarting inside silence we requested is refused forever. Keys travel as a chain with the periods each may be used in, because a rollover runs on a clock and an offloaded session is not on the path that re-reads lifetimes. The data plane holds the packets, so it decides which key applies to one. Not implemented: keyed MD5, which no keychain algorithm maps onto. The digest follows bfdd rather than the letter of s6.7.4, reported upstream as FRRouting/frr#23274 without a patch. Running authenticated sessions under bfdd needs the data plane key extension, FRRouting/frr#23331.
|
@greptileai review |
a032fb0 to
212f624
Compare
|
ci:rerun |
`send-lifetime` and `accept-lifetime` take `(1993-2035)`, so the 2040 dates the fixture moved to are rejected outright with `% Unknown command`. A rejected line fails the candidate configuration as a whole, which takes the `bfd peer` block with it, so the router came up with no session at all and every case failed for a reason unrelated to what it was testing. The dates were moved into the future in the first place so that no key in the fixture expires while the test is still in use. 2035 does not keep that property, it postpones the loss of it: a key whose accept period has closed is not offloaded, so once those windows close the test requires three keys and receives one, and starts failing on the wall clock and nothing else. The accept periods therefore end with a duration rather than a date. An end given as a duration is computed from the start, so it is never written down and the year ceiling does not reach it. The send periods keep their dates: they have to close before the next key opens, which is the overlap the test demonstrates, so they are bounded by each other rather than by the calendar. Every assertion on the lifetimes compares them against one another, so none of them moves. The case added alongside it had the same problem twice over, being both out of range and in the wrong field order. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
`hmac-sha-1` needs `--with-crypto=openssl`, which is not the default: `keychain_get_algo_id_by_name` does not recognise the name under `CRYPTO_INTERNAL` and returns `KEYCHAIN_ALGO_NULL`, which the northbound validation then refuses. Having it in the startup configuration is worse than having it in a test. A refused leaf fails the candidate configuration as a whole, so the `bfd peer` block never applies either and the router comes up with no session: every case in the file fails, for a reason none of them is about. `bfd_authentication_topo1` guards its SHA-1 cases with `has_crypto_openssl`, but that cannot help here, because the damage is done at startup before any test body runs. Use cleartext. Nothing here asserts on the authentication type, a key carries its lifetimes the same either way, and `cleartext` is recognised by both crypto backends, so the module runs everywhere rather than skipping half of itself on the builds that matter least. Signed-off-by: Abdul Wasey <w453y.me@gmail.com>
212f624 to
95a1ecf
Compare
|
@greptileai review |
|
I'll leave this here for a couple of days to see if anyone else wants to take a look. |
bfddp_session_msghas nowhere to put an authentication key. Upstream thatspace is a
/* TODO: missing authentication. */, and nothing guards theoffload path, so a session with a key chain is registered like any other
and the data plane transmits it in the clear while
show bfd peerreportsauthentication enabled.
Sending the active key would not fix it.
bfd_keychain_key_find_active()is consulted from
ptm_bfd_snd()andbfd_check_auth()on every packet,so the software path rolls over without being told. An offloaded session is
not on that path and the key chain has no expiry hook, so a data plane
handed one key keeps signing with it long after it expires. Pushing a new
key at each boundary puts the daemon back in the per-packet path that
offloading exists to avoid.
So send the whole chain with the periods each key may be used in, and let
the data plane pick. It has the packets, so it is the only side that can
tell which key applies to one. The send and accept periods overlap during a
rollover, and a single key cannot express that.
What this does:
DP_SESSION_AUTHcarryingstruct bfddp_auth_key: type, key id,length, the key, and send and accept periods as absolute seconds with
-1for no expirySESSION_AUTHto the session flags, so a data plane can refuse asession it cannot protect, and a removed key chain is signalled by the
flag clearing rather than by an empty message
the data plane noticing a lifetime passed
first and cannot be displaced, the room left goes to upcoming ones, and a
key whose accept period has closed is dropped. Chain order is key id
order, which would let low ids spend every slot on rollovers years away
configured: unmappable algorithm, empty or over-long key, key id past the
eight bits the wire has, more live keys than the message holds
socket or a loopback address
DP_DELETE_SESSIONwhen the keys cannot beenqueued
keychain_updatedfrom the lifetime callbacks inlib/keychain_nb.cThe message is variable length and
bfddp_message_header.lengthbounds it.The key array is declared at full size in
struct bfddp_session_authso areceiver can copy into
struct bfddp_messageand read in place, as withevery other message here; only the keys present go on the wire.
BFDDP_AUTH_KEY_COUNT_MAXis 16.Refusing an unprotected transport is a behaviour change and worth flagging.
Every other message here describes a session; this one carries the key
protecting it, over whatever
--dplaneaddrnames. On a plain TCPconnection bfdd cannot tell who accepted it or stop anyone on the path
reading it. So an authenticated session offloads only over a UNIX socket or
loopback, and otherwise runs in the daemon and authenticates there. The
existing
bfd.rstwarning covers who connects, not who reads, so I did nottreat it as covering this, and there is a note beside it now. I put the
test on the connection rather than behind a knob because no answer an
operator could give makes writing the key to that socket safe. Happy to be
wrong if it breaks a real deployment.
Withdrawal is not symmetric between the two paths. Registration has already
queued
DP_ADD_SESSIONby the time keys are refused, so clearing onlybs->bdcwould leave the data plane running a session bfdd thinks it neveroffloaded, unprotected if it ignores
SESSION_AUTH. Both paths sendDP_DELETE_SESSION; only the update path then restarts the session insoftware, because registration is reached from inside
bfd_session_enableand would re-enter it.
The
lib/keychain_nb.cchange reaches every keychain user, not just bfdd.The hook fires on key create, destroy, key-string and algorithm, but not on
a lifetime. That has not mattered because every user in tree reads the
chain per packet through
key_lookup_for_sendorkey_lookup_for_accept.One handed the periods once has nothing to re-read. Worth a look from
someone who knows the other consumers.
What this does not fix is that there is no capability exchange, so bfdd
cannot tell whether a data plane honours
SESSION_AUTHat all. The flaglets a data plane refuse what it cannot protect, which beats today's silent
downgrade, but one that ignores it looks identical to one that does not.
That wants solving separately.
Testing
bfd_dplane_auth_topo1is new: one router with a key chain, offloading tothe test listener. It checks that the session announces it authenticates,
that every key is sent and not just the one in use, that the lifetimes
survive the trip, and that a key added or a lifetime edited afterwards is
pushed. The last covers the
lib/change and reads the value back, since aresend of the old set would satisfy a count.
Lifetimes are compared against each other, not against fixed timestamps,
because the daemon reads them from a local time. What matters is the shape:
a key stops being sent before it stops being accepted, and the next is
accepted before it is sent.
The chain uses
cleartext, nothmac-sha-1. That needs--with-crypto=openssl, and a rejected algorithm fails the candidateconfiguration as a whole and takes the
bfd peerblock with it, so themodule would come up with no session rather than skip. Nothing asserts on
the authentication type and lifetimes travel either way. 5 passed on a
build with openssl and 5 on a build without.
bfd_dplane_counters_topo1,bfd_authentication_topo1andbfd_demand_topo1alongside it, 37 passed.bfd_dplane_update_sessionloses its
consthere and withdrawal reaches every offloaded session, notonly authenticated ones.
There is also a second implementation on the other side of the socket. An
out-of-tree XDP data plane consumes
DP_SESSION_AUTHand verifies keyedSHA1 and simple password in the driver path, which is where the in-place
read constraint came from. A rollover driven entirely by the key chain ran
on a live 64-session mesh, both ends moving key id within one frame pair,
no state change and no gap beyond the session interval.