From a9e2d6d06ebd6b50705695a923d297e038114a8c Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 14:37:21 -0400 Subject: [PATCH 1/7] libnetwork/d/overlay: filter local peers explicitly The overlay driver's checkEncryption function configures the IPSec parameters for the VXLAN tunnels to peer nodes. When called with isLocal=true, it configures encryption for all peer nodes with at least one peerDB entry. Since the local peers are also included in the peerDB, it needs to filter those entries out. It does so by filtering out any peer entries whose VTEP address is equal to the current local advertise address. Trouble is, the local advertise address is not necessarily constant. The driver tries to handle this case by calling peerDBUpdateSelf() when the advertise address changes. This function iterates through the peerDB and tries to update the VTEP address for all local peer entries, but it does not actually do anything: it mutates a temporary copy of the entry which is not persisted back into the peerDB. (It used to be functional, but was broken when the peerDB was extended to use SetMatrix.) So there may be cases where local peer entries are not filtered out properly, resulting in spurious encryption parameters being programmed into the kernel. Filter out local peers when walking the peerDB by filtering on whether the entry has the isLocal flag set. Remove the no-op code which attempts to update local entries in the peerDB. No other code takes any interest in the VTEP value for isLocal peer entries. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 2 +- libnetwork/drivers/overlay/overlay.go | 21 +++++++------------- libnetwork/drivers/overlay/peerdb.go | 25 ------------------------ 3 files changed, 8 insertions(+), 40 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index e81623da73..8bbe6adbdf 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -132,7 +132,7 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, isLocal, add bool) switch { case isLocal: if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { - if aIP != pEntry.vtep { + if !pEntry.isLocal { nodes[pEntry.vtep] = struct{}{} } return false diff --git a/libnetwork/drivers/overlay/overlay.go b/libnetwork/drivers/overlay/overlay.go index 75b65e6fe0..e0063d76ac 100644 --- a/libnetwork/drivers/overlay/overlay.go +++ b/libnetwork/drivers/overlay/overlay.go @@ -30,14 +30,13 @@ var _ discoverapi.Discover = (*driver)(nil) type driver struct { bindAddress, advertiseAddress netip.Addr - config map[string]interface{} - peerDb peerNetworkMap - secMap *encrMap - networks networkTable - initOS sync.Once - localJoinOnce sync.Once - keys []*key - peerOpMu sync.Mutex + config map[string]interface{} + peerDb peerNetworkMap + secMap *encrMap + networks networkTable + initOS sync.Once + keys []*key + peerOpMu sync.Mutex sync.Mutex } @@ -95,12 +94,6 @@ func (d *driver) nodeJoin(data discoverapi.NodeDiscoveryData) error { d.advertiseAddress = advAddr d.bindAddress = bindAddr d.Unlock() - - // If containers are already running on this network update the - // advertise address in the peerDB - d.localJoinOnce.Do(func() { - d.peerDBUpdateSelf() - }) } return nil } diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index ec1b57bfa9..d2d631c19f 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/libnetwork/drivers/overlay/peerdb.go @@ -37,22 +37,6 @@ type peerNetworkMap struct { sync.Mutex } -func (d *driver) peerDbWalk(f func(string, netip.Addr, net.HardwareAddr, *peerEntry) bool) error { - d.peerDb.Lock() - nids := []string{} - for nid := range d.peerDb.mp { - nids = append(nids, nid) - } - d.peerDb.Unlock() - - for _, nid := range nids { - d.peerDbNetworkWalk(nid, func(peerIP netip.Addr, peerMac net.HardwareAddr, pEntry *peerEntry) bool { - return f(nid, peerIP, peerMac, pEntry) - }) - } - return nil -} - func (d *driver) peerDbNetworkWalk(nid string, f func(netip.Addr, net.HardwareAddr, *peerEntry) bool) error { d.peerDb.Lock() pMap, ok := d.peerDb.mp[nid] @@ -359,12 +343,3 @@ func (d *driver) peerFlushOp(nid string) error { delete(d.peerDb.mp, nid) return nil } - -func (d *driver) peerDBUpdateSelf() { - d.peerDbWalk(func(nid string, _ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { - if pEntry.isLocal { - pEntry.vtep = d.advertiseAddress - } - return false - }) -} From 48e0b24ff7f9fedfda03c8d31d2d9e6b34f5576c Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 15:30:56 -0400 Subject: [PATCH 2/7] libnetwork/d/overlay: elide vtep for local peers The VTEP value for a peer in peerDB is only accurate for a remote peer. The VTEP for a local peer would be the driver's advertise address, which is not necessarily constant for the lifetime of the driver instance. The VTEP values persisted in the peerDB entries for local peers could be stale or missing if not kept in sync with the advertise address. And the peerDB could get polluted with duplicate entries for local peers if the advertise address was to change, as entries which differ only by VTEP are considered distinct by SetMatrix. Persisting the advertise address as the VTEP for local peers creates lots of problems that are not easy to solve. Stop persisting the VTEP for local peers in peerDB. Any code that needs to know the VTEP for local peers can look that up from the source of truth: the driver's advertise address. Use the lack of a VTEP in peerDB entries to signify local peers, making the isLocal flag redundant. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 2 +- libnetwork/drivers/overlay/peerdb.go | 23 +++++++++++++++-------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index 8bbe6adbdf..ed744214b8 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -132,7 +132,7 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, isLocal, add bool) switch { case isLocal: if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { - if !pEntry.isLocal { + if !pEntry.isLocal() { nodes[pEntry.vtep] = struct{}{} } return false diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index d2d631c19f..07dda3f8e8 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/libnetwork/drivers/overlay/peerdb.go @@ -20,9 +20,12 @@ const ovPeerTable = "overlay_peer_table" type peerEntry struct { eid string - vtep netip.Addr - prefixBits int // number of 1-bits in network mask of peerIP - isLocal bool + vtep netip.Addr // Virtual Tunnel End Point for non-local peers + prefixBits int // number of 1-bits in network mask of peerIP +} + +func (p *peerEntry) isLocal() bool { + return !p.vtep.IsValid() } type peerMap struct { @@ -105,7 +108,9 @@ func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.Har eid: eid, vtep: vtep, prefixBits: peerIP.Bits(), - isLocal: isLocal, + } + if isLocal { + pEntry.vtep = netip.Addr{} } pMap.Lock() @@ -134,7 +139,9 @@ func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net. eid: eid, vtep: vtep, prefixBits: peerIP.Bits(), - isLocal: isLocal, + } + if isLocal { + pEntry.vtep = netip.Addr{} } pMap.Lock() @@ -170,11 +177,11 @@ func (d *driver) initSandboxPeerDB(nid string) { func (d *driver) peerInitOp(nid string) error { return d.peerDbNetworkWalk(nid, func(peerIP netip.Addr, peerMac net.HardwareAddr, pEntry *peerEntry) bool { // Local entries do not need to be added - if pEntry.isLocal { + if pEntry.isLocal() { return false } - d.peerAddOp(nid, pEntry.eid, netip.PrefixFrom(peerIP, pEntry.prefixBits), peerMac, pEntry.vtep, false, pEntry.isLocal) + d.peerAddOp(nid, pEntry.eid, netip.PrefixFrom(peerIP, pEntry.prefixBits), peerMac, pEntry.vtep, false, pEntry.isLocal()) // return false to loop on all entries return false }) @@ -322,7 +329,7 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net. log.G(context.TODO()).Errorf("peerDeleteOp unable to restore a configuration for nid:%s ip:%v mac:%v err:%s", nid, peerIP, peerMac, err) return err } - return d.peerAddOp(nid, peerEntry.eid, netip.PrefixFrom(peerIPAddr, peerEntry.prefixBits), peerMac, peerEntry.vtep, false, peerEntry.isLocal) + return d.peerAddOp(nid, peerEntry.eid, netip.PrefixFrom(peerIPAddr, peerEntry.prefixBits), peerMac, peerEntry.vtep, false, peerEntry.isLocal()) } func (d *driver) peerFlush(nid string) { From 4b1c1236b9df1ad7458831b06429d071dbaca194 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 16:04:43 -0400 Subject: [PATCH 3/7] libnetwork/d/overlay: peerdb: drop isLocal param Drop the isLocal boolean parameters from the peerDB functions. Local peers have vtep == netip.Addr{}. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/joinleave.go | 8 ++-- libnetwork/drivers/overlay/peerdb.go | 50 ++++++++++++------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/libnetwork/drivers/overlay/joinleave.go b/libnetwork/drivers/overlay/joinleave.go index 7f81b51d2b..ffb2b8db90 100644 --- a/libnetwork/drivers/overlay/joinleave.go +++ b/libnetwork/drivers/overlay/joinleave.go @@ -119,7 +119,7 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf } } - d.peerAdd(nid, eid, ep.addr, ep.mac, d.advertiseAddress, true) + d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{}) if err = d.checkEncryption(nid, netip.Addr{}, true, true); err != nil { log.G(ctx).Warn(err) @@ -197,11 +197,11 @@ func (d *driver) EventNotify(etype driverapi.EventType, nid, tableName, key stri } if etype == driverapi.Delete { - d.peerDelete(nid, eid, addr, mac, vtep, false) + d.peerDelete(nid, eid, addr, mac, vtep) return } - d.peerAdd(nid, eid, addr, mac, vtep, false) + d.peerAdd(nid, eid, addr, mac, vtep) } // Leave method is invoked when a Sandbox detaches from an endpoint. @@ -221,7 +221,7 @@ func (d *driver) Leave(nid, eid string) error { return types.InternalMaskableErrorf("could not find endpoint with id %s", eid) } - d.peerDelete(nid, eid, ep.addr, ep.mac, d.advertiseAddress, true) + d.peerDelete(nid, eid, ep.addr, ep.mac, netip.Addr{}) n.leaveSandbox() diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index 07dda3f8e8..358879170c 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/libnetwork/drivers/overlay/peerdb.go @@ -93,7 +93,7 @@ func (d *driver) peerDbSearch(nid string, peerIP netip.Addr) (netip.Addr, net.Ha return peerIPMatched, peerMacMatched, pEntryMatched, nil } -func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, isLocal bool) (bool, int) { +func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) (bool, int) { d.peerDb.Lock() pMap, ok := d.peerDb.mp[nid] if !ok { @@ -109,9 +109,6 @@ func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.Har vtep: vtep, prefixBits: peerIP.Bits(), } - if isLocal { - pEntry.vtep = netip.Addr{} - } pMap.Lock() defer pMap.Unlock() @@ -124,7 +121,7 @@ func (d *driver) peerDbAdd(nid, eid string, peerIP netip.Prefix, peerMac net.Har return b, i } -func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, isLocal bool) (bool, int) { +func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) (bool, int) { d.peerDb.Lock() pMap, ok := d.peerDb.mp[nid] if !ok { @@ -140,9 +137,6 @@ func (d *driver) peerDbDelete(nid, eid string, peerIP netip.Prefix, peerMac net. vtep: vtep, prefixBits: peerIP.Bits(), } - if isLocal { - pEntry.vtep = netip.Addr{} - } pMap.Lock() defer pMap.Unlock() @@ -181,22 +175,25 @@ func (d *driver) peerInitOp(nid string) error { return false } - d.peerAddOp(nid, pEntry.eid, netip.PrefixFrom(peerIP, pEntry.prefixBits), peerMac, pEntry.vtep, false, pEntry.isLocal()) + d.peerAddOp(nid, pEntry.eid, netip.PrefixFrom(peerIP, pEntry.prefixBits), peerMac, pEntry.vtep, false) // return false to loop on all entries return false }) } -func (d *driver) peerAdd(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, localPeer bool) { +// peerAdd adds a new entry to the peer database. +// +// Local peers are signified by an invalid vtep (i.e. netip.Addr{}). +func (d *driver) peerAdd(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) { d.peerOpMu.Lock() defer d.peerOpMu.Unlock() - err := d.peerAddOp(nid, eid, peerIP, peerMac, vtep, true, localPeer) + err := d.peerAddOp(nid, eid, peerIP, peerMac, vtep, true) if err != nil { log.G(context.TODO()).WithError(err).Warn("Peer add operation failed") } } -func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, updateDB, localPeer bool) error { +func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, updateDB bool) error { if err := validateID(nid, eid); err != nil { return err } @@ -204,15 +201,15 @@ func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.Har var dbEntries int var inserted bool if updateDB { - inserted, dbEntries = d.peerDbAdd(nid, eid, peerIP, peerMac, vtep, localPeer) + inserted, dbEntries = d.peerDbAdd(nid, eid, peerIP, peerMac, vtep) if !inserted { - log.G(context.TODO()).Warnf("Entry already present in db: nid:%s eid:%s peerIP:%v peerMac:%v isLocal:%t vtep:%v", - nid, eid, peerIP, peerMac, localPeer, vtep) + log.G(context.TODO()).Warnf("Entry already present in db: nid:%s eid:%s peerIP:%v peerMac:%v vtep:%v", + nid, eid, peerIP, peerMac, vtep) } } // Local peers do not need any further configuration - if localPeer { + if !vtep.IsValid() { return nil } @@ -261,24 +258,27 @@ func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.Har return nil } -func (d *driver) peerDelete(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, localPeer bool) { +// peerDelete removes an entry from the peer database. +// +// Local peers are signified by an invalid vtep (i.e. netip.Addr{}). +func (d *driver) peerDelete(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) { d.peerOpMu.Lock() defer d.peerOpMu.Unlock() - err := d.peerDeleteOp(nid, eid, peerIP, peerMac, vtep, localPeer) + err := d.peerDeleteOp(nid, eid, peerIP, peerMac, vtep) if err != nil { log.G(context.TODO()).WithError(err).Warn("Peer delete operation failed") } } -func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr, localPeer bool) error { +func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net.HardwareAddr, vtep netip.Addr) error { if err := validateID(nid, eid); err != nil { return err } - deleted, dbEntries := d.peerDbDelete(nid, eid, peerIP, peerMac, vtep, localPeer) + deleted, dbEntries := d.peerDbDelete(nid, eid, peerIP, peerMac, vtep) if !deleted { - log.G(context.TODO()).Warnf("Entry was not in db: nid:%s eid:%s peerIP:%v peerMac:%v isLocal:%t vtep:%v", - nid, eid, peerIP, peerMac, localPeer, vtep) + log.G(context.TODO()).Warnf("Entry was not in db: nid:%s eid:%s peerIP:%v peerMac:%v vtep:%v", + nid, eid, peerIP, peerMac, vtep) } n := d.network(nid) @@ -291,12 +291,12 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net. return nil } - if err := d.checkEncryption(nid, vtep, localPeer, false); err != nil { + if err := d.checkEncryption(nid, vtep, !vtep.IsValid(), false); err != nil { log.G(context.TODO()).Warn(err) } // Local peers do not have any local configuration to delete - if !localPeer { + if vtep.IsValid() { s := n.getSubnetforIP(peerIP) if s == nil { return fmt.Errorf("could not find the subnet %q in network %q", peerIP.String(), n.id) @@ -329,7 +329,7 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net. log.G(context.TODO()).Errorf("peerDeleteOp unable to restore a configuration for nid:%s ip:%v mac:%v err:%s", nid, peerIP, peerMac, err) return err } - return d.peerAddOp(nid, peerEntry.eid, netip.PrefixFrom(peerIPAddr, peerEntry.prefixBits), peerMac, peerEntry.vtep, false, peerEntry.isLocal()) + return d.peerAddOp(nid, peerEntry.eid, netip.PrefixFrom(peerIPAddr, peerEntry.prefixBits), peerMac, peerEntry.vtep, false) } func (d *driver) peerFlush(nid string) { From 0d893252ac723588bfade224eff24adfacfce798 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 16:29:24 -0400 Subject: [PATCH 4/7] libnetwork/d/overlay: checkEncryption: drop isLocal param Since it is not meaningful to add or remove encryption between the local node and itself, the isLocal parameter is redundant. Setting up encryption for all network peers is now invoked by calling checkEncryption(nid, netip.Addr{}, true) Calling checkEncryption with isLocal=true, add=false is now more explicitly a no-op. It always was effectively a no-op, but that was not easy to spot by inspection. In the world with the isLocal flag, calls to checkEncryption where isLocal=true and add=false would have rIP set to d.advertiseAddr. In other words, it was a request to remove encryption parameters between the local peer and itself if peerDB had no remote-peer entries for the network. So either the call would do nothing, or it would remove encryption parameters that aren't used for anything. Now the equivalent call always does nothing. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 12 ++++++++---- libnetwork/drivers/overlay/joinleave.go | 2 +- libnetwork/drivers/overlay/peerdb.go | 4 ++-- 3 files changed, 11 insertions(+), 7 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index ed744214b8..d276f6344d 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -113,8 +113,12 @@ func (e *encrMap) String() string { return b.String() } -func (d *driver) checkEncryption(nid string, rIP netip.Addr, isLocal, add bool) error { - log.G(context.TODO()).Debugf("checkEncryption(%.7s, %v, %t)", nid, rIP, isLocal) +// checkEncryption sets up or removes IPsec encryption parameters for peers on a network. +// +// When given an rIP, encryption paremeters will be set up for the VXLAN tunnel to that peer. +// When !rIP.IsValid(), encryption parameters will be set up for all network peers. +func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { + log.G(context.TODO()).Debugf("checkEncryption(%.7s, %v)", nid, rIP) n := d.network(nid) if n == nil || !n.secure { @@ -130,7 +134,7 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, isLocal, add bool) nodes := map[netip.Addr]struct{}{} switch { - case isLocal: + case !rIP.IsValid(): if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { if !pEntry.isLocal() { nodes[pEntry.vtep] = struct{}{} @@ -154,7 +158,7 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, isLocal, add bool) } } } else { - if len(nodes) == 0 { + if rIP.IsValid() && len(nodes) == 0 { if err := removeEncryption(lIP, rIP, d.secMap); err != nil { log.G(context.TODO()).Warnf("Failed to remove network encryption between %s and %s: %v", lIP, rIP, err) } diff --git a/libnetwork/drivers/overlay/joinleave.go b/libnetwork/drivers/overlay/joinleave.go index ffb2b8db90..65004c23c0 100644 --- a/libnetwork/drivers/overlay/joinleave.go +++ b/libnetwork/drivers/overlay/joinleave.go @@ -121,7 +121,7 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{}) - if err = d.checkEncryption(nid, netip.Addr{}, true, true); err != nil { + if err = d.checkEncryption(nid, netip.Addr{}, true); err != nil { log.G(ctx).Warn(err) } diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index 358879170c..1e6118e4fb 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/libnetwork/drivers/overlay/peerdb.go @@ -235,7 +235,7 @@ func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.Har return fmt.Errorf("subnet sandbox join failed for %q: %v", s.subnetIP.String(), err) } - if err := d.checkEncryption(nid, vtep, false, true); err != nil { + if err := d.checkEncryption(nid, vtep, true); err != nil { log.G(context.TODO()).Warn(err) } @@ -291,7 +291,7 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net. return nil } - if err := d.checkEncryption(nid, vtep, !vtep.IsValid(), false); err != nil { + if err := d.checkEncryption(nid, vtep, false); err != nil { log.G(context.TODO()).Warn(err) } From cb4e7b2f0392ad3092f9ee1e2cdfb4ae0ed0fba8 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 16:54:22 -0400 Subject: [PATCH 5/7] libnetwork/d/overlay: make setupEncryption a method The setupEncryption and removeEncryption functions take several parameters, but all call sites pass the same values for all the parameters aside from remoteIP: values taken from fields of the driver struct. Refactor these functions to be methods of the driver struct and drop the redundant parameters. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 30 ++++++++++++------------ 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index d276f6344d..21a415a488 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -129,8 +129,6 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { return types.ForbiddenErrorf("encryption key is not present") } - lIP := d.bindAddress - aIP := d.advertiseAddress nodes := map[netip.Addr]struct{}{} switch { @@ -153,14 +151,14 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { if add { for rIP := range nodes { - if err := setupEncryption(lIP, aIP, rIP, d.secMap, d.keys); err != nil { - log.G(context.TODO()).Warnf("Failed to program network encryption between %s and %s: %v", lIP, rIP, err) + if err := d.setupEncryption(rIP); err != nil { + log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err) } } } else { if rIP.IsValid() && len(nodes) == 0 { - if err := removeEncryption(lIP, rIP, d.secMap); err != nil { - log.G(context.TODO()).Warnf("Failed to remove network encryption between %s and %s: %v", lIP, rIP, err) + if err := d.removeEncryption(rIP); err != nil { + log.G(context.TODO()).Warnf("Failed to remove network encryption to remote peer %s: %v", rIP, err) } } } @@ -170,7 +168,9 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { // setupEncryption programs the encryption parameters for secure communication // between the local node and a remote node. -func setupEncryption(localIP, advIP, remoteIP netip.Addr, em *encrMap, keys []*key) error { +func (d *driver) setupEncryption(remoteIP netip.Addr) error { + localIP, advIP := d.bindAddress, d.advertiseAddress + keys := d.keys // FIXME: data race log.G(context.TODO()).Debugf("Programming encryption between %s and %s", localIP, remoteIP) indices := make([]*spi, 0, len(keys)) @@ -195,17 +195,17 @@ func setupEncryption(localIP, advIP, remoteIP netip.Addr, em *encrMap, keys []*k } } - em.Lock() - em.nodes[remoteIP] = indices - em.Unlock() + d.secMap.Lock() + d.secMap.nodes[remoteIP] = indices + d.secMap.Unlock() return nil } -func removeEncryption(localIP, remoteIP netip.Addr, em *encrMap) error { - em.Lock() - indices, ok := em.nodes[remoteIP] - em.Unlock() +func (d *driver) removeEncryption(remoteIP netip.Addr) error { + d.secMap.Lock() + indices, ok := d.secMap.nodes[remoteIP] + d.secMap.Unlock() if !ok { return nil } @@ -214,7 +214,7 @@ func removeEncryption(localIP, remoteIP netip.Addr, em *encrMap) error { if i == 0 { dir = bidir } - fSA, rSA, err := programSA(localIP.AsSlice(), remoteIP.AsSlice(), idxs, nil, dir, false) + fSA, rSA, err := programSA(d.bindAddress.AsSlice(), remoteIP.AsSlice(), idxs, nil, dir, false) if err != nil { log.G(context.TODO()).Warn(err) } From 713f8876985ee6accddab339bffa500e3d228fcb Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 16 May 2025 17:18:37 -0400 Subject: [PATCH 6/7] libnetwork/d/overlay: drop checkEncryption function In addition to being three functions in a trenchcoat, the checkEncryption function has a very subtle implementation which is difficult to reason about. That is not a good property for security relevant code to have. Replace two of the three calls to checkEncryption with conditional calls to setupEncryption and removeEncryption, lifting the conditional logic which was hidden away in checkEncryption into the call sites to make it easier to reason about the code. Replace the third call with a call to a new initEncryption function. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 49 +++++++++--------------- libnetwork/drivers/overlay/joinleave.go | 2 +- libnetwork/drivers/overlay/peerdb.go | 12 ++++-- 3 files changed, 28 insertions(+), 35 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index 21a415a488..8f1b2db419 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -113,12 +113,9 @@ func (e *encrMap) String() string { return b.String() } -// checkEncryption sets up or removes IPsec encryption parameters for peers on a network. -// -// When given an rIP, encryption paremeters will be set up for the VXLAN tunnel to that peer. -// When !rIP.IsValid(), encryption parameters will be set up for all network peers. -func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { - log.G(context.TODO()).Debugf("checkEncryption(%.7s, %v)", nid, rIP) +// initEncryption sets up IPsec encryption parameters for all known peers on a network. +func (d *driver) initEncryption(nid string) error { + log.G(context.TODO()).Debugf("initEncryption(%.7s)", nid) n := d.network(nid) if n == nil || !n.secure { @@ -131,35 +128,20 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { nodes := map[netip.Addr]struct{}{} - switch { - case !rIP.IsValid(): - if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { - if !pEntry.isLocal() { - nodes[pEntry.vtep] = struct{}{} - } - return false - }); err != nil { - log.G(context.TODO()).Warnf("Failed to retrieve list of participating nodes in overlay network %.5s: %v", nid, err) - } - default: - if len(d.network(nid).endpoints) > 0 { - nodes[rIP] = struct{}{} + if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { + if !pEntry.isLocal() { + nodes[pEntry.vtep] = struct{}{} } + return false + }); err != nil { + log.G(context.TODO()).Warnf("Failed to retrieve list of participating nodes in overlay network %.5s: %v", nid, err) } log.G(context.TODO()).Debugf("List of nodes: %s", nodes) - if add { - for rIP := range nodes { - if err := d.setupEncryption(rIP); err != nil { - log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err) - } - } - } else { - if rIP.IsValid() && len(nodes) == 0 { - if err := d.removeEncryption(rIP); err != nil { - log.G(context.TODO()).Warnf("Failed to remove network encryption to remote peer %s: %v", rIP, err) - } + for rIP := range nodes { + if err := d.setupEncryption(rIP); err != nil { + log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err) } } @@ -169,8 +151,13 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error { // setupEncryption programs the encryption parameters for secure communication // between the local node and a remote node. func (d *driver) setupEncryption(remoteIP netip.Addr) error { + log.G(context.TODO()).Debugf("setupEncryption(%s)", remoteIP) + localIP, advIP := d.bindAddress, d.advertiseAddress keys := d.keys // FIXME: data race + if len(keys) == 0 { + return types.ForbiddenErrorf("encryption key is not present") + } log.G(context.TODO()).Debugf("Programming encryption between %s and %s", localIP, remoteIP) indices := make([]*spi, 0, len(keys)) @@ -203,6 +190,8 @@ func (d *driver) setupEncryption(remoteIP netip.Addr) error { } func (d *driver) removeEncryption(remoteIP netip.Addr) error { + log.G(context.TODO()).Debugf("removeEncryption(%s)", remoteIP) + d.secMap.Lock() indices, ok := d.secMap.nodes[remoteIP] d.secMap.Unlock() diff --git a/libnetwork/drivers/overlay/joinleave.go b/libnetwork/drivers/overlay/joinleave.go index 65004c23c0..dfda0401f4 100644 --- a/libnetwork/drivers/overlay/joinleave.go +++ b/libnetwork/drivers/overlay/joinleave.go @@ -121,7 +121,7 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{}) - if err = d.checkEncryption(nid, netip.Addr{}, true); err != nil { + if err = d.initEncryption(nid); err != nil { log.G(ctx).Warn(err) } diff --git a/libnetwork/drivers/overlay/peerdb.go b/libnetwork/drivers/overlay/peerdb.go index 1e6118e4fb..926b7f86a3 100644 --- a/libnetwork/drivers/overlay/peerdb.go +++ b/libnetwork/drivers/overlay/peerdb.go @@ -235,8 +235,10 @@ func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.Har return fmt.Errorf("subnet sandbox join failed for %q: %v", s.subnetIP.String(), err) } - if err := d.checkEncryption(nid, vtep, true); err != nil { - log.G(context.TODO()).Warn(err) + if n.secure && len(n.endpoints) > 0 { + if err := d.setupEncryption(vtep); err != nil { + log.G(context.TODO()).Warn(err) + } } // Add neighbor entry for the peer IP @@ -291,8 +293,10 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net. return nil } - if err := d.checkEncryption(nid, vtep, false); err != nil { - log.G(context.TODO()).Warn(err) + if n.secure && len(n.endpoints) == 0 { + if err := d.removeEncryption(vtep); err != nil { + log.G(context.TODO()).Warn(err) + } } // Local peers do not have any local configuration to delete From df6b4057967ef7c37b6f8610dd3e6fd6e1c5b994 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 27 May 2025 14:09:23 -0400 Subject: [PATCH 7/7] libnetwork/d/overlay: drop initEncryption function The (*driver).Join function does many things to set up overlay networking. One of the first things it does is call (*network).joinSandbox, which in turn calls (*driver).initSandboxPeerDB. The initSandboxPeerDB function iterates through the peer db to add entries to the VXLAN FDB, neighbor table and IPsec security association database in the kernel for all known peers on the overlay network. One of the last things the (*driver).Join function does is call (*driver).initEncryption. The initEncryption function iterates through the peer db to add entries to the IPsec security association database in the kernel for all known peers on the overlay network. But the preceding initSandboxPeerDB call already did that! The initEncryption function is redundant and can safely be removed. Signed-off-by: Cory Snider --- libnetwork/drivers/overlay/encryption.go | 35 ------------------------ libnetwork/drivers/overlay/joinleave.go | 4 --- 2 files changed, 39 deletions(-) diff --git a/libnetwork/drivers/overlay/encryption.go b/libnetwork/drivers/overlay/encryption.go index 8f1b2db419..ecb37e250d 100644 --- a/libnetwork/drivers/overlay/encryption.go +++ b/libnetwork/drivers/overlay/encryption.go @@ -113,41 +113,6 @@ func (e *encrMap) String() string { return b.String() } -// initEncryption sets up IPsec encryption parameters for all known peers on a network. -func (d *driver) initEncryption(nid string) error { - log.G(context.TODO()).Debugf("initEncryption(%.7s)", nid) - - n := d.network(nid) - if n == nil || !n.secure { - return nil - } - - if len(d.keys) == 0 { - return types.ForbiddenErrorf("encryption key is not present") - } - - nodes := map[netip.Addr]struct{}{} - - if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool { - if !pEntry.isLocal() { - nodes[pEntry.vtep] = struct{}{} - } - return false - }); err != nil { - log.G(context.TODO()).Warnf("Failed to retrieve list of participating nodes in overlay network %.5s: %v", nid, err) - } - - log.G(context.TODO()).Debugf("List of nodes: %s", nodes) - - for rIP := range nodes { - if err := d.setupEncryption(rIP); err != nil { - log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err) - } - } - - return nil -} - // setupEncryption programs the encryption parameters for secure communication // between the local node and a remote node. func (d *driver) setupEncryption(remoteIP netip.Addr) error { diff --git a/libnetwork/drivers/overlay/joinleave.go b/libnetwork/drivers/overlay/joinleave.go index dfda0401f4..79f2a5d025 100644 --- a/libnetwork/drivers/overlay/joinleave.go +++ b/libnetwork/drivers/overlay/joinleave.go @@ -121,10 +121,6 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{}) - if err = d.initEncryption(nid); err != nil { - log.G(ctx).Warn(err) - } - buf, err := proto.Marshal(&PeerRecord{ EndpointIP: ep.addr.String(), EndpointMAC: ep.mac.String(),