Commit 7749bccbd5 for frr
commit 7749bccbd52cba8c0fe84dabcf72eda00caa2a63
Author: Abdul Wasey <w453y.me@gmail.com>
Date: Mon Sep 14 07:21:56 2026 +0000
bfdd: offload the keys a rollover needs, and name the ones left out
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>
diff --git a/bfdd/dplane.c b/bfdd/dplane.c
index 3663c787cf..1a9609b5f5 100644
--- a/bfdd/dplane.c
+++ b/bfdd/dplane.c
@@ -1314,6 +1314,7 @@ static int bfd_dplane_send_session_auth(const struct bfd_session *bs)
struct key *key;
uint16_t count = 0;
uint16_t msglen;
+ time_t now = time(NULL);
for (ALL_LIST_ELEMENTS_RO(bs->kc->key, node, key)) {
enum bfd_auth_type type;
@@ -1324,12 +1325,18 @@ static int bfd_dplane_send_session_auth(const struct bfd_session *bs)
/* A key whose algorithm has no BFD equivalent is unusable. */
type = map_keychain_algo_to_bfd_auth_type(key->hash_algo, bs->auth_meticulous);
- if (type == BFD_AUTH_TYPE_RESERVED)
+ if (type == BFD_AUTH_TYPE_RESERVED) {
+ zlog_warn("%s: %s: key id %u has no BFD authentication type, not offloaded",
+ __func__, bs->kc->name, key->index);
continue;
+ }
keylen = strlen(key->string);
- if (keylen == 0 || keylen > BFDDP_AUTH_KEY_MAX)
+ if (keylen == 0 || keylen > BFDDP_AUTH_KEY_MAX) {
+ zlog_warn("%s: %s: key id %u is %zu bytes, outside 1..%u, not offloaded",
+ __func__, bs->kc->name, key->index, keylen, BFDDP_AUTH_KEY_MAX);
continue;
+ }
/*
* RFC 5880 gives the Auth Key ID eight bits, so a key chain
@@ -1337,11 +1344,28 @@ static int bfd_dplane_send_session_auth(const struct bfd_session *bs)
* keeps the data plane's view of the key chain honest;
* truncating would give two keys the same identifier.
*/
- if (key->index > UINT8_MAX)
+ if (key->index > UINT8_MAX) {
+ zlog_warn("%s: %s: key id %u does not fit the eight bit Auth Key ID, not offloaded",
+ __func__, bs->kc->name, key->index);
+ continue;
+ }
+
+ /*
+ * A key whose accept period has closed can never be used
+ * again by either side. Spending one of this message's few
+ * slots on it costs a key the chain has yet to roll on to,
+ * which is the one case the slots exist for.
+ *
+ * Read the period the way `key_valid` does: 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.
+ */
+ if (key->accept.start != 0 && key->accept.end != -1 && key->accept.end < now)
continue;
if (count == BFDDP_AUTH_KEY_COUNT_MAX) {
- zlog_warn("%s: key chain %s has more than %u usable keys, the rest are not offloaded",
+ zlog_warn("%s: %s: more than %u keys are still live, the rest are not offloaded",
__func__, bs->kc->name, BFDDP_AUTH_KEY_COUNT_MAX);
break;
}
diff --git a/tests/topotests/bfd_dplane_auth_topo1/r1/frr.conf b/tests/topotests/bfd_dplane_auth_topo1/r1/frr.conf
index 41abaa2a6c..2b54fbf4f8 100644
--- a/tests/topotests/bfd_dplane_auth_topo1/r1/frr.conf
+++ b/tests/topotests/bfd_dplane_auth_topo1/r1/frr.conf
@@ -7,14 +7,14 @@ key chain rollover
key 1
key-string firstkey00000001
cryptographic-algorithm hmac-sha-1
- send-lifetime 00:00:00 1 January 2026 23:59:59 30 June 2026
- accept-lifetime 00:00:00 1 January 2026 23:59:59 31 July 2026
+ send-lifetime 00:00:00 1 January 2040 23:59:59 30 June 2040
+ accept-lifetime 00:00:00 1 January 2040 23:59:59 31 July 2040
exit
key 2
key-string secondkey0000002
cryptographic-algorithm hmac-sha-1
- send-lifetime 00:00:00 1 July 2026 23:59:59 31 December 2026
- accept-lifetime 00:00:00 1 June 2026 23:59:59 31 December 2026
+ send-lifetime 00:00:00 1 July 2040 23:59:59 31 December 2040
+ accept-lifetime 00:00:00 1 June 2040 23:59:59 31 December 2040
exit
key 3
key-string thirdkey00000003
diff --git a/tests/topotests/bfd_dplane_auth_topo1/test_bfd_dplane_auth_topo1.py b/tests/topotests/bfd_dplane_auth_topo1/test_bfd_dplane_auth_topo1.py
index 3e7978153a..aa29902e7a 100644
--- a/tests/topotests/bfd_dplane_auth_topo1/test_bfd_dplane_auth_topo1.py
+++ b/tests/topotests/bfd_dplane_auth_topo1/test_bfd_dplane_auth_topo1.py
@@ -126,6 +126,11 @@ def _keys(dump):
The lifetimes are seconds since the epoch, and the daemon reads them
from a local time, so nothing here compares them against a fixed
value. What matters is how they sit relative to one another.
+
+ The chain itself is dated well into the future on purpose: a key whose
+ accept period has closed is not offloaded, so a fixture written around
+ the date it was authored would quietly lose a key and take the overlap
+ it demonstrates with it.
"""
out = []
pattern = re.compile(