Skip to content

Commit 5682acb

Browse files
champtarchamptar
committed
portmap: ensure nftables backend only intercept local traffic
portmap iptables backend uses `-m addrtype --dst-type LOCAL` and a common chain (CNI-HOSTPORT-DNAT) for both hostPort and hostIP/hostPort. Before this commit, nftables backend was using 2 separate chains, `hostip_hostports` and `hostports`. The goal was to avoid using `fib daddr type local` before we jump to `hostip_hostports`, but this is a behavior change compared to iptables backend, and a security issue (hostIP: 1.1.1.1 / hostPort: 53). Also while switching from input to prerouting hook, we forgot to add the fib lookup for `hostports`, rendering the nftables backend half broken. To allow transparent upgrades and avoid running the fib lookup twice, we use an intermediate chain (`hostports_all`) ``` chain hostports_all { jump hostip_hostports jump hostports } ``` Long-term we want to remove `hostip_hostports`, so all new rules are created in the `hostports` chain. We can't use implicit chains (`jump { jump hostip_hostports; jump hostports }`) as it's not supported by knftables.Fake yet. Fixes 9296c5f Fixes 01a94e1 Signed-off-by: Etienne Champetier <[email protected]>
1 parent f12dcbc commit 5682acb

File tree

2 files changed

+86
-44
lines changed

2 files changed

+86
-44
lines changed

‎plugins/meta/portmap/portmap_nftables.go‎

Lines changed: 40 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,14 @@ import (
2626
const (
2727
tableName = "cni_hostport"
2828

29+
// Intermediate chain to jump to both 'hostip_hostports' and 'hostports'
30+
hostPortsAllChain = "hostports_all"
31+
// This chain was used for rules with hostIP, we do not use it anymore,
32+
// but we keep it to make upgrade transparent
2933
hostIPHostPortsChain = "hostip_hostports"
30-
hostPortsChain = "hostports"
31-
masqueradingChain = "masquerading"
34+
// Chain containing all the rules
35+
hostPortsChain = "hostports"
36+
masqueradingChain = "masquerading"
3237
)
3338

3439
// The nftables portmap implementation is fairly similar to the iptables implementation:
@@ -104,58 +109,69 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet
104109
})
105110

106111
tx.Add(&knftables.Chain{
107-
Name: "hostports",
112+
Name: hostPortsChain,
108113
})
114+
109115
tx.Add(&knftables.Chain{
110-
Name: "hostip_hostports",
116+
Name: hostIPHostPortsChain,
111117
})
112118

119+
// setup intermediate chain
113120
tx.Add(&knftables.Chain{
114-
Name: "prerouting",
115-
Type: knftables.PtrTo(knftables.NATType),
116-
Hook: knftables.PtrTo(knftables.PreroutingHook),
117-
Priority: knftables.PtrTo(knftables.DNATPriority),
121+
Name: hostPortsAllChain,
118122
})
123+
119124
tx.Flush(&knftables.Chain{
120-
Name: "prerouting",
125+
Name: hostPortsAllChain,
121126
})
127+
122128
tx.Add(&knftables.Rule{
123-
Chain: "prerouting",
129+
Chain: hostPortsAllChain,
124130
Rule: knftables.Concat(
125-
conditions,
126131
"jump", hostIPHostPortsChain,
127132
),
128133
})
134+
129135
tx.Add(&knftables.Rule{
130-
Chain: "prerouting",
136+
Chain: hostPortsAllChain,
131137
Rule: knftables.Concat(
132-
conditions,
133138
"jump", hostPortsChain,
134139
),
135140
})
136141

137142
tx.Add(&knftables.Chain{
138-
Name: "output",
143+
Name: "prerouting",
139144
Type: knftables.PtrTo(knftables.NATType),
140-
Hook: knftables.PtrTo(knftables.OutputHook),
145+
Hook: knftables.PtrTo(knftables.PreroutingHook),
141146
Priority: knftables.PtrTo(knftables.DNATPriority),
142147
})
143148
tx.Flush(&knftables.Chain{
144-
Name: "output",
149+
Name: "prerouting",
145150
})
146151
tx.Add(&knftables.Rule{
147-
Chain: "output",
152+
Chain: "prerouting",
148153
Rule: knftables.Concat(
149154
conditions,
150-
"jump", hostIPHostPortsChain,
155+
"fib daddr type local",
156+
"jump", hostPortsAllChain,
151157
),
152158
})
159+
160+
tx.Add(&knftables.Chain{
161+
Name: "output",
162+
Type: knftables.PtrTo(knftables.NATType),
163+
Hook: knftables.PtrTo(knftables.OutputHook),
164+
Priority: knftables.PtrTo(knftables.DNATPriority),
165+
})
166+
tx.Flush(&knftables.Chain{
167+
Name: "output",
168+
})
153169
tx.Add(&knftables.Rule{
154170
Chain: "output",
155171
Rule: knftables.Concat(
156172
conditions,
157173
"fib daddr type local",
158-
"jump", hostPortsChain,
174+
"jump", hostPortsAllChain,
159175
),
160176
})
161177

@@ -184,8 +200,10 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet
184200
}
185201

186202
if useHostIP {
203+
// we add the rule to 'hostports' instead of 'hostip_hostports'
204+
// as we want to remove 'hostip_hostports' long-term
187205
tx.Add(&knftables.Rule{
188-
Chain: hostIPHostPortsChain,
206+
Chain: hostPortsChain,
189207
Rule: knftables.Concat(
190208
ipX, "daddr", e.HostIP,
191209
e.Protocol, "dport", e.HostPort,
@@ -243,7 +261,7 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet
243261
func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet net.IPNet) error {
244262
isV6 := (containerNet.IP.To4() == nil)
245263

246-
var hostPorts, hostIPHostPorts, masqueradings int
264+
var hostPorts, masqueradings int
247265
for _, e := range config.RuntimeConfig.PortMaps {
248266
if e.HostIP != "" {
249267
hostIP := net.ParseIP(e.HostIP)
@@ -252,14 +270,8 @@ func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet ne
252270
if isV6 != isHostV6 {
253271
continue
254272
}
255-
if hostIP.IsUnspecified() {
256-
hostPorts++
257-
} else {
258-
hostIPHostPorts++
259-
}
260-
} else {
261-
hostPorts++
262273
}
274+
hostPorts++
263275
}
264276
if *config.SNAT {
265277
masqueradings = 1
@@ -278,12 +290,6 @@ func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet ne
278290
return err
279291
}
280292
}
281-
if hostIPHostPorts > 0 {
282-
err := checkPortsAgainstRules(nft, hostIPHostPortsChain, config.ContainerID, hostIPHostPorts)
283-
if err != nil {
284-
return err
285-
}
286-
}
287293
if masqueradings > 0 {
288294
err := checkPortsAgainstRules(nft, masqueradingChain, config.ContainerID, masqueradings)
289295
if err != nil {

‎plugins/meta/portmap/portmap_nftables_test.go‎

Lines changed: 46 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -85,21 +85,22 @@ var _ = Describe("portmapping configuration (nftables)", func() {
8585
add table ip cni_hostport { comment "CNI portmap plugin" ; }
8686
add chain ip cni_hostport hostip_hostports
8787
add chain ip cni_hostport hostports
88+
add chain ip cni_hostport hostports_all
8889
add chain ip cni_hostport masquerading { type nat hook postrouting priority 100 ; }
8990
add chain ip cni_hostport output { type nat hook output priority -100 ; }
9091
add chain ip cni_hostport prerouting { type nat hook prerouting priority -100 ; }
91-
add rule ip cni_hostport hostip_hostports ip daddr 192.168.0.2 tcp dport 8083 dnat to 10.0.0.2:83 comment "icee6giejonei6so"
9292
add rule ip cni_hostport hostports tcp dport 8080 dnat to 10.0.0.2:80 comment "icee6giejonei6so"
9393
add rule ip cni_hostport hostports tcp dport 8081 dnat to 10.0.0.2:80 comment "icee6giejonei6so"
9494
add rule ip cni_hostport hostports udp dport 8080 dnat to 10.0.0.2:81 comment "icee6giejonei6so"
9595
add rule ip cni_hostport hostports udp dport 8082 dnat to 10.0.0.2:82 comment "icee6giejonei6so"
96+
add rule ip cni_hostport hostports ip daddr 192.168.0.2 tcp dport 8083 dnat to 10.0.0.2:83 comment "icee6giejonei6so"
9697
add rule ip cni_hostport hostports tcp dport 8084 dnat to 10.0.0.2:84 comment "icee6giejonei6so"
98+
add rule ip cni_hostport hostports_all jump hostip_hostports
99+
add rule ip cni_hostport hostports_all jump hostports
97100
add rule ip cni_hostport masquerading ip saddr 10.0.0.2 ip daddr 10.0.0.2 masquerade comment "icee6giejonei6so"
98101
add rule ip cni_hostport masquerading ip saddr 127.0.0.1 ip daddr 10.0.0.2 masquerade comment "icee6giejonei6so"
99-
add rule ip cni_hostport output a b jump hostip_hostports
100-
add rule ip cni_hostport output a b fib daddr type local jump hostports
101-
add rule ip cni_hostport prerouting a b jump hostip_hostports
102-
add rule ip cni_hostport prerouting a b jump hostports
102+
add rule ip cni_hostport output a b fib daddr type local jump hostports_all
103+
add rule ip cni_hostport prerouting a b fib daddr type local jump hostports_all
103104
`)
104105
actualRules := strings.TrimSpace(ipv4Fake.Dump())
105106
Expect(actualRules).To(Equal(expectedRules))
@@ -119,18 +120,53 @@ add rule ip cni_hostport prerouting a b jump hostports
119120
add table ip6 cni_hostport { comment "CNI portmap plugin" ; }
120121
add chain ip6 cni_hostport hostip_hostports
121122
add chain ip6 cni_hostport hostports
123+
add chain ip6 cni_hostport hostports_all
122124
add chain ip6 cni_hostport output { type nat hook output priority -100 ; }
123125
add chain ip6 cni_hostport prerouting { type nat hook prerouting priority -100 ; }
124-
add rule ip6 cni_hostport hostip_hostports ip6 daddr 2001:db8:a::1 tcp dport 8085 dnat to [2001:db8::2]:85 comment "icee6giejonei6so"
125126
add rule ip6 cni_hostport hostports tcp dport 8080 dnat to [2001:db8::2]:80 comment "icee6giejonei6so"
126127
add rule ip6 cni_hostport hostports tcp dport 8081 dnat to [2001:db8::2]:80 comment "icee6giejonei6so"
127128
add rule ip6 cni_hostport hostports udp dport 8080 dnat to [2001:db8::2]:81 comment "icee6giejonei6so"
128129
add rule ip6 cni_hostport hostports udp dport 8082 dnat to [2001:db8::2]:82 comment "icee6giejonei6so"
130+
add rule ip6 cni_hostport hostports ip6 daddr 2001:db8:a::1 tcp dport 8085 dnat to [2001:db8::2]:85 comment "icee6giejonei6so"
129131
add rule ip6 cni_hostport hostports tcp dport 8086 dnat to [2001:db8::2]:86 comment "icee6giejonei6so"
130-
add rule ip6 cni_hostport output c d jump hostip_hostports
131-
add rule ip6 cni_hostport output c d fib daddr type local jump hostports
132-
add rule ip6 cni_hostport prerouting c d jump hostip_hostports
133-
add rule ip6 cni_hostport prerouting c d jump hostports
132+
add rule ip6 cni_hostport hostports_all jump hostip_hostports
133+
add rule ip6 cni_hostport hostports_all jump hostports
134+
add rule ip6 cni_hostport output c d fib daddr type local jump hostports_all
135+
add rule ip6 cni_hostport prerouting c d fib daddr type local jump hostports_all
136+
`)
137+
actualRules = strings.TrimSpace(ipv6Fake.Dump())
138+
Expect(actualRules).To(Equal(expectedRules))
139+
140+
err = pmNFT.unforwardPorts(conf)
141+
Expect(err).NotTo(HaveOccurred())
142+
143+
expectedRules = strings.TrimSpace(`
144+
add table ip cni_hostport { comment "CNI portmap plugin" ; }
145+
add chain ip cni_hostport hostip_hostports
146+
add chain ip cni_hostport hostports
147+
add chain ip cni_hostport hostports_all
148+
add chain ip cni_hostport masquerading { type nat hook postrouting priority 100 ; }
149+
add chain ip cni_hostport output { type nat hook output priority -100 ; }
150+
add chain ip cni_hostport prerouting { type nat hook prerouting priority -100 ; }
151+
add rule ip cni_hostport hostports_all jump hostip_hostports
152+
add rule ip cni_hostport hostports_all jump hostports
153+
add rule ip cni_hostport output a b fib daddr type local jump hostports_all
154+
add rule ip cni_hostport prerouting a b fib daddr type local jump hostports_all
155+
`)
156+
actualRules = strings.TrimSpace(ipv4Fake.Dump())
157+
Expect(actualRules).To(Equal(expectedRules))
158+
159+
expectedRules = strings.TrimSpace(`
160+
add table ip6 cni_hostport { comment "CNI portmap plugin" ; }
161+
add chain ip6 cni_hostport hostip_hostports
162+
add chain ip6 cni_hostport hostports
163+
add chain ip6 cni_hostport hostports_all
164+
add chain ip6 cni_hostport output { type nat hook output priority -100 ; }
165+
add chain ip6 cni_hostport prerouting { type nat hook prerouting priority -100 ; }
166+
add rule ip6 cni_hostport hostports_all jump hostip_hostports
167+
add rule ip6 cni_hostport hostports_all jump hostports
168+
add rule ip6 cni_hostport output c d fib daddr type local jump hostports_all
169+
add rule ip6 cni_hostport prerouting c d fib daddr type local jump hostports_all
134170
`)
135171
actualRules = strings.TrimSpace(ipv6Fake.Dump())
136172
Expect(actualRules).To(Equal(expectedRules))

0 commit comments

Comments
 (0)