libn/d/overlay: don't remove in-use IPsec tunnels - #53420
Conversation
There was a problem hiding this comment.
Pull request overview
This PR hardens encrypted overlay networking by making VXLAN peer neighbor programming transactional and ensuring IPsec tunnel teardown only happens when the corresponding setup actually succeeded, preventing in-use tunnels from being removed and potentially exposing traffic.
Changes:
- Make
addNeighbor()transactional with rollback on post-encryption failures, and adjustdeleteNeighbor()teardown ordering to avoid a cleartext window. - Add
Namespace.SetNeighbor()(netlinkNeighSet) to replace existing FDB entries instead of failing withEEXIST. - Export
NeighborSearchErrorfields so callers can construct/inspect “missing vs present” neighbor/FDB errors in a consistent form.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| daemon/libnetwork/osl/neigh_linux.go | Export NeighborSearchError fields and add SetNeighbor() wrapper using netlink NeighSet to support replace semantics. |
| daemon/libnetwork/drivers/overlay/peerdb.go | Make peer neighbor programming rollback-safe, adjust FDB programming to use replace semantics, and reorder teardown to avoid dropping IPsec while entries are still being removed. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // Delete neighbor entry for the peer IP | ||
| if err := n.sbox.DeleteNeighbor(peerIP.Addr().AsSlice(), peerMac.AsSlice(), osl.WithLinkName(s.vxlanName)); err != nil { | ||
| return fmt.Errorf("could not delete neighbor entry in the sandbox: %w", err) | ||
| } |
Encrypted overlay-network traffic flows to endpoints hosted on a peer node are sent through a single shared IPsec tunnel per peer. A tunnel to a peer is established the first time the local node learns of an encrypted-overlay endpoint hosted on that peer, and is torn down when no more encrypted-overlay endpoints exist on that peer. The overlay network driver implements this behaviour by maintaining reference counts: removeEncryption() only tears down the tunnel when called the same number of times for a peer as setupEncryption(). addNeighbor() and deleteNeighbor() were not a balanced pair. addNeighbor() could return without ever calling setupEncryption() on failure, while deleteNeighbor() called removeEncryption() unconditionally. Since addNeighbor() and deleteNeighbor() are called whenever an entry is added to and removed from overlay_peer_table, respectively, the removal of a peer entry whose setup failed would release a reference it never took. Consequently, the reference count for the encryption parameters could drop to zero while an endpoint on the same node was still using the tunnel. The security associations, policy and packet-marking firewall rules would be removed out from under it, allowing VXLAN datagrams encapsulating traffic from the local endpoint to go out on the wire in cleartext. Make addNeighbor() transactional: any failure after setupEncryption() rolls it back, along with the neighbor entry and the FDB entry's reference count, so a reference to the tunnel is held iff addNeighbor() returned successfully. deleteNeighbor() proceeds with teardown iff the peer's neighbor entry is present in the kernel, which is the case only if the matching addNeighbor() had succeeded. Make deleteNeighbor() release the encryption reference last so nothing can be transmitted between the FDB entry being removed and the tunnel being torn down. Program the FDB entry with NeighSet() so an entry left behind by a failed deletion is replaced instead of wedging every subsequent addNeighbor() with EEXIST. Export the fields of NeighborSearchError so the overlay driver can report a missing FDB entry in the form its callers already expect. Signed-off-by: Cory Snider <csnider@mirantis.com>
b7c7323 to
81813ce
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (1)
daemon/libnetwork/drivers/overlay/peerdb.go:161
- The new transactional behavior in addNeighbor (setupEncryption + deferred rollback, plus later neighbor/FDB programming) is security-sensitive and has multiple failure paths. There don’t appear to be unit tests covering the rollback semantics (e.g., setupEncryption succeeds, then neighbor/FDB programming fails, and removeEncryption + neighbor cleanup happen exactly once; and that deleteNeighbor tears down in the intended order). Adding targeted tests would help prevent regressions in the reference-counting logic that can lead to premature tunnel teardown or leaked encryption state.
if n.secure {
if err := n.driver.setupEncryption(vtep); err != nil {
return fmt.Errorf("could not setup encryption for peer %v: %w", vtep, err)
}
defer func() {
robmry
left a comment
There was a problem hiding this comment.
Reviewed this in some depth. The core reasoning looks right to me, and both orderings seem correct: encryption established before the neighbour entries on setup and released after them on teardown, with the rollback defers unwinding LIFO. Registering each defer after its operation succeeds is also what keeps the EEXIST path safe, since an already-exists failure from AddNeighbor then can't trigger a spurious neighbour delete. And SetNeighbor is the right call for the FDB, whose delete does match on the MAC.
One finding I think needs attention, on the teardown precondition (details inline at peerdb.go:252 and :257). The short version: the kernel's IP-neighbour delete looks the entry up by (dst, dev) and ignores the supplied lladdr, so getting past DeleteNeighbor(peerIP, ...) doesn't prove that the matching addNeighbor succeeded. I checked the kernel behaviour in a netns:
# ip neigh add 10.9.9.9 lladdr 00:11:22:33:44:55 dev veth0
# ip neigh show dev veth0
10.9.9.9 lladdr 00:11:22:33:44:55 PERMANENT
# ip neigh del 10.9.9.9 lladdr 00:11:22:33:44:66 dev veth0 # note: different MAC
# echo $?
0
# ip neigh show dev veth0
# (entry is gone)In the "transient state" the surrounding comments describe — two peerdb entries for one peerIP, only one configuration active at a time — that means deleteNeighbor can delete the incumbent entry's programming and then return an error which peerDelete maps to the transient case, skipping the restore that would have reprogrammed it.
Three smaller notes inline, plus one general one: there's no test coverage for the rollback semantics or the teardown ordering. This is now a three-function protocol coordinated through error shape with two rollback defers, so a table test over the failure paths would be worth having.
On CI, the three red checks all look unrelated to me. integration-cli (DockerSwarmSuite) is TestAPISwarmServicesMultipleAgents, matching #53076 — the log shows timeout hit after 30s and then the daemon socket disappearing during teardown, rather than any assertion about overlay state. The other two lanes (docker-py on snapshotter, and integration/container|build|system on oraclelinux-8) don't exercise overlay peer programming.
Reviewed by Claude Code on behalf of @robmry.
| } | ||
|
|
||
| // Delete neighbor entry for the peer IP | ||
| if err := n.sbox.DeleteNeighbor(peerIP.Addr().AsSlice(), peerMac.AsSlice(), osl.WithLinkName(s.vxlanName)); err != nil { |
There was a problem hiding this comment.
DeleteNeighbor here can delete an entry that this call never programmed. The kernel's IP-neighbour delete looks the entry up by (dst, dev) only — the lladdr is not part of the match — so this succeeds and removes whatever entry exists for peerIP on the link, even one programmed by a different peerdb entry with a different MAC. (Verified in a netns; transcript in the review body. The FDB delete below is fine: for AF_BRIDGE the MAC is the lookup key.)
That undercuts the invariant asserted in the comment at line 272 — reaching the teardown doesn't imply that the matching addNeighbor succeeded.
Separately, changing this wrap from %v to %w changes caller behaviour. peerDelete does:
if dbEntries > 0 && errors.As(err, &osl.NeighborSearchError{}) {
return nil
}With %v the error chain was broken, so a not-found from this call fell through to the dbEntries > 0 restore block. With %w it now takes that early return instead, and the surviving peerdb entry is never reprogrammed. The %w is the correct wrap — it's the interaction with that early return that I think needs a second look.
Reviewed by Claude Code on behalf of @robmry.
| // Remove fdb entry to the bridge for the peer mac | ||
| if n.fdbCnt.Add(hashable.IPMACFrom(vtep, peerMac), -1) == 0 { | ||
| if err := n.sbox.DeleteNeighbor(vtep.AsSlice(), peerMac.AsSlice(), osl.WithLinkName(s.vxlanName), osl.WithFamily(syscall.AF_BRIDGE)); err != nil { | ||
| if v := hashable.IPMACFrom(vtep, peerMac); n.fdbCnt[v] == 0 { |
There was a problem hiding this comment.
This precondition is evaluated after line 252 has already mutated kernel state, so when it trips we've deleted a neighbour entry and then reported failure.
Concretely, with two peerdb entries for the same peerIP but different MACs: entry B's addNeighbor failed at AddNeighbor with EEXIST, so it never incremented fdbCnt, and peerAdd swallowed that as the transient case. On peerDelete(B), line 252 deletes entry A's live neighbour entry (the MAC isn't part of the lookup), then this branch trips and returns a NeighborSearchError, which peerDelete maps to return nil — skipping the restore. Entry A's neighbour entry is gone and nothing puts it back.
Hoisting this check above line 252 would fix both halves: a failed precondition would leave all state untouched, and the incumbent entry — which belongs to a different peerdb entry — would be left alone.
fdbCnt does look like a sound proxy for "a matching addNeighbor succeeded", since addNeighbor increments it only after AddNeighbor succeeds and its rollback decrements it again.
For completeness, the same-MAC case works out. Because overlay derives the MAC from the IP (GenerateMACFromIP), two entries for one IP normally share a MAC, so this guard passes and removeEncryption releases entry A's reference — but then the restore immediately reprograms everything, and the new teardown ordering means there's no cleartext window. That one is churn rather than a leak. It's the differing-MAC case that loses the entry.
Reviewed by Claude Code on behalf of @robmry.
| // that control flow can only reach here if we succeeded in deleting the | ||
| // neighbor entry (and fdb entry, if applicable) since the entries are | ||
| // only present in the kernel if the matching addNeighbor call we are | ||
| // reversing was successful. |
There was a problem hiding this comment.
Worth qualifying this claim: the reference-to-entry correspondence also doesn't survive a sandbox rebuild, independently of any error path.
leaveSandbox calls destroySandbox, which nils n.sbox and resets sboxInit, and initSandbox resets n.fdbCnt to an empty map — but d.secMap is driver-scoped and nothing releases the references held for the peers. On the next join, the peer database is replayed through addNeighbor, so setupEncryption runs again for every peer with no matching removeEncryption, and the counts drift upward once per leave/rejoin cycle.
That's pre-existing and in the safe direction (the tunnel stays up rather than being torn down early), so not something this PR needs to fix. But it does mean the invariant holds only within a single sandbox lifetime, which seems worth stating explicitly in a comment that future readers will lean on.
Reviewed by Claude Code on behalf of @robmry.
| defer func() { | ||
| if retErr != nil { | ||
| if err := n.driver.removeEncryption(vtep); err != nil { | ||
| retErr = errors.Join(retErr, fmt.Errorf("could not roll back encryption for peer %v: %w", vtep, err)) |
There was a problem hiding this comment.
Joining the rollback error into retErr widens what the callers' error-shape checks can match. peerAdd has:
if dbEntries > 1 && errors.As(err, &osl.NeighborSearchError{}) {
return nil
}errors.As walks errors.Join trees, so a NeighborSearchError originating in a rollback rather than in the primary failure can now satisfy that branch, causing a genuine failure to be reported as the benign transient case. It's a narrow path — the rollback would have to hit a not-found — but now that the fields are exported, that branch could test Present instead of type-matching both variants, which would make it robust either way.
Reviewed by Claude Code on behalf of @robmry.
| mac net.HardwareAddr | ||
| linkName string | ||
| present bool | ||
| IP net.IP |
There was a problem hiding this comment.
Two small things now that these fields are exported.
The two in-package construction sites (lines 60 and 102) are still positional literals — NeighborSearchError{dstIP, dstMac, linkName, false} — so adding a field to this struct would break them silently. go vet's composites check only flags unkeyed literals for types from other packages, so it won't catch these. Might be worth switching all four sites to field names while you're here.
Also, exporting the fields makes it possible for callers to synthesise an error describing kernel state that was never queried, which is what peerdb.go:258 does. That's really a workaround for peerDelete's error-shape-based control flow; a constructor, or an error type local to the overlay driver, would keep the fabrication out of osl's API surface. Your call — just noting that the API widening is load-bearing for exactly one caller.
Reviewed by Claude Code on behalf of @robmry.
Summary
Encrypted overlay-network traffic flows to endpoints hosted on a peer node are sent through a single shared IPsec tunnel per peer. A tunnel to a peer is established the first time the local node learns of an encrypted-overlay endpoint hosted on that peer, and is torn down when no more encrypted-overlay endpoints exist on that peer. The overlay network driver implements this behaviour by maintaining reference counts:
removeEncryption()only tears down the tunnel when called the same number of times for a peer assetupEncryption().addNeighbor()anddeleteNeighbor()were not a balanced pair.addNeighbor()could return without ever callingsetupEncryption()on failure, whiledeleteNeighbor()calledremoveEncryption()unconditionally. SinceaddNeighbor()anddeleteNeighbor()are called whenever an entry is added to and removed fromoverlay_peer_table, respectively, the removal of a peer entry whose setup failed would release a reference it never took. Consequently, the reference count for the encryption parameters could drop to zero while an endpoint on the same node was still using the tunnel. The security associations, policy and packet-marking firewall rules would be removed out from under it, allowing VXLAN datagrams encapsulating traffic from the local endpoint to go out on the wire in cleartext.Make
addNeighbor()transactional: any failure aftersetupEncryption()rolls it back, along with the neighbor entry and the FDB entry's reference count, so a reference to the tunnel is held iffaddNeighbor()returned successfully.deleteNeighbor()proceeds with teardown iff the peer's neighbor entry is present in the kernel, which is the case only if the matchingaddNeighbor()had succeeded.Make
deleteNeighbor()release the encryption reference last so nothing can be transmitted between the FDB entry being removed and the tunnel being torn down.Program the FDB entry with
NeighSet()so an entry left behind by a failed deletion is replaced instead of wedging every subsequentaddNeighbor()withEEXIST. Export the fields ofNeighborSearchErrorso the overlay driver can report a missing FDB entry in the form its callers already expect.Release notes (optional)
- Fix an issue where errors programming the kernel to encrypt the overlay network data-plane could in some circumstances lead to encrypted-overlay-network traffic to some nodes being transmitted in cleartext. As the receiving peer would drop cleartext packets for encrypted overlay networks as spoofed, the loss of confidentiality is limited to unidirectional flows (e.g. UDP DNS queries) and handshake attempts that never proceed (e.g. TCP SYN).A picture of a cute animal (not mandatory but encouraged)