Skip to content

Commit 713f887

Browse files
corherecorhere
committed
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 <[email protected]>
1 parent cb4e7b2 commit 713f887

3 files changed

Lines changed: 28 additions & 35 deletions

File tree

‎libnetwork/drivers/overlay/encryption.go‎

Lines changed: 19 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -113,12 +113,9 @@ func (e *encrMap) String() string {
113113
return b.String()
114114
}
115115

116-
// checkEncryption sets up or removes IPsec encryption parameters for peers on a network.
117-
//
118-
// When given an rIP, encryption paremeters will be set up for the VXLAN tunnel to that peer.
119-
// When !rIP.IsValid(), encryption parameters will be set up for all network peers.
120-
func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error {
121-
log.G(context.TODO()).Debugf("checkEncryption(%.7s, %v)", nid, rIP)
116+
// initEncryption sets up IPsec encryption parameters for all known peers on a network.
117+
func (d *driver) initEncryption(nid string) error {
118+
log.G(context.TODO()).Debugf("initEncryption(%.7s)", nid)
122119

123120
n := d.network(nid)
124121
if n == nil || !n.secure {
@@ -131,35 +128,20 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error {
131128

132129
nodes := map[netip.Addr]struct{}{}
133130

134-
switch {
135-
case !rIP.IsValid():
136-
if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool {
137-
if !pEntry.isLocal() {
138-
nodes[pEntry.vtep] = struct{}{}
139-
}
140-
return false
141-
}); err != nil {
142-
log.G(context.TODO()).Warnf("Failed to retrieve list of participating nodes in overlay network %.5s: %v", nid, err)
143-
}
144-
default:
145-
if len(d.network(nid).endpoints) > 0 {
146-
nodes[rIP] = struct{}{}
131+
if err := d.peerDbNetworkWalk(nid, func(_ netip.Addr, _ net.HardwareAddr, pEntry *peerEntry) bool {
132+
if !pEntry.isLocal() {
133+
nodes[pEntry.vtep] = struct{}{}
147134
}
135+
return false
136+
}); err != nil {
137+
log.G(context.TODO()).Warnf("Failed to retrieve list of participating nodes in overlay network %.5s: %v", nid, err)
148138
}
149139

150140
log.G(context.TODO()).Debugf("List of nodes: %s", nodes)
151141

152-
if add {
153-
for rIP := range nodes {
154-
if err := d.setupEncryption(rIP); err != nil {
155-
log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err)
156-
}
157-
}
158-
} else {
159-
if rIP.IsValid() && len(nodes) == 0 {
160-
if err := d.removeEncryption(rIP); err != nil {
161-
log.G(context.TODO()).Warnf("Failed to remove network encryption to remote peer %s: %v", rIP, err)
162-
}
142+
for rIP := range nodes {
143+
if err := d.setupEncryption(rIP); err != nil {
144+
log.G(context.TODO()).Warnf("Failed to program network encryption to remote peer %s: %v", rIP, err)
163145
}
164146
}
165147

@@ -169,8 +151,13 @@ func (d *driver) checkEncryption(nid string, rIP netip.Addr, add bool) error {
169151
// setupEncryption programs the encryption parameters for secure communication
170152
// between the local node and a remote node.
171153
func (d *driver) setupEncryption(remoteIP netip.Addr) error {
154+
log.G(context.TODO()).Debugf("setupEncryption(%s)", remoteIP)
155+
172156
localIP, advIP := d.bindAddress, d.advertiseAddress
173157
keys := d.keys // FIXME: data race
158+
if len(keys) == 0 {
159+
return types.ForbiddenErrorf("encryption key is not present")
160+
}
174161
log.G(context.TODO()).Debugf("Programming encryption between %s and %s", localIP, remoteIP)
175162

176163
indices := make([]*spi, 0, len(keys))
@@ -203,6 +190,8 @@ func (d *driver) setupEncryption(remoteIP netip.Addr) error {
203190
}
204191

205192
func (d *driver) removeEncryption(remoteIP netip.Addr) error {
193+
log.G(context.TODO()).Debugf("removeEncryption(%s)", remoteIP)
194+
206195
d.secMap.Lock()
207196
indices, ok := d.secMap.nodes[remoteIP]
208197
d.secMap.Unlock()

‎libnetwork/drivers/overlay/joinleave.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ func (d *driver) Join(ctx context.Context, nid, eid string, sboxKey string, jinf
121121

122122
d.peerAdd(nid, eid, ep.addr, ep.mac, netip.Addr{})
123123

124-
if err = d.checkEncryption(nid, netip.Addr{}, true); err != nil {
124+
if err = d.initEncryption(nid); err != nil {
125125
log.G(ctx).Warn(err)
126126
}
127127

‎libnetwork/drivers/overlay/peerdb.go‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -235,8 +235,10 @@ func (d *driver) peerAddOp(nid, eid string, peerIP netip.Prefix, peerMac net.Har
235235
return fmt.Errorf("subnet sandbox join failed for %q: %v", s.subnetIP.String(), err)
236236
}
237237

238-
if err := d.checkEncryption(nid, vtep, true); err != nil {
239-
log.G(context.TODO()).Warn(err)
238+
if n.secure && len(n.endpoints) > 0 {
239+
if err := d.setupEncryption(vtep); err != nil {
240+
log.G(context.TODO()).Warn(err)
241+
}
240242
}
241243

242244
// Add neighbor entry for the peer IP
@@ -291,8 +293,10 @@ func (d *driver) peerDeleteOp(nid, eid string, peerIP netip.Prefix, peerMac net.
291293
return nil
292294
}
293295

294-
if err := d.checkEncryption(nid, vtep, false); err != nil {
295-
log.G(context.TODO()).Warn(err)
296+
if n.secure && len(n.endpoints) == 0 {
297+
if err := d.removeEncryption(vtep); err != nil {
298+
log.G(context.TODO()).Warn(err)
299+
}
296300
}
297301

298302
// Local peers do not have any local configuration to delete

0 commit comments

Comments
 (0)