From d192c55ad4af52727c92de4890c845eac35421c2 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Wed, 5 Nov 2025 11:13:39 -0500 Subject: [PATCH 1/2] portmap: fix CHECK for nftables backend Fixes 01a94e17c77e6ff8e5019e15c42d8d92cf87194f Signed-off-by: Etienne Champetier --- plugins/meta/portmap/portmap_nftables.go | 16 ++- plugins/meta/portmap/portmap_nftables_test.go | 116 +++++++++++++----- 2 files changed, 100 insertions(+), 32 deletions(-) diff --git a/plugins/meta/portmap/portmap_nftables.go b/plugins/meta/portmap/portmap_nftables.go index 75ad72063..d3e7049dc 100644 --- a/plugins/meta/portmap/portmap_nftables.go +++ b/plugins/meta/portmap/portmap_nftables.go @@ -246,14 +246,24 @@ func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet ne var hostPorts, hostIPHostPorts, masqueradings int for _, e := range config.RuntimeConfig.PortMaps { if e.HostIP != "" { - hostIPHostPorts++ + hostIP := net.ParseIP(e.HostIP) + isHostV6 := (hostIP.To4() == nil) + // Ignore wrong-IP-family HostIPs + if isV6 != isHostV6 { + continue + } + if hostIP.IsUnspecified() { + hostPorts++ + } else { + hostIPHostPorts++ + } } else { hostPorts++ } } if *config.SNAT { - masqueradings = len(config.RuntimeConfig.PortMaps) - if isV6 { + masqueradings = 1 + if !isV6 { masqueradings *= 2 } } diff --git a/plugins/meta/portmap/portmap_nftables_test.go b/plugins/meta/portmap/portmap_nftables_test.go index 0b11f20e8..c367f7ad7 100644 --- a/plugins/meta/portmap/portmap_nftables_test.go +++ b/plugins/meta/portmap/portmap_nftables_test.go @@ -27,6 +27,31 @@ import ( var _ = Describe("portmapping configuration (nftables)", func() { containerID := "icee6giejonei6so" + configTmpl := `{ + "name": "test", + "type": "portmap", + "cniVersion": "%s", + "backend": "nftables", + "runtimeConfig": { + "portMappings": [ + { "hostPort": 8080, "containerPort": 80, "protocol": "tcp"}, + { "hostPort": 8081, "containerPort": 80, "protocol": "tcp"}, + { "hostPort": 8080, "containerPort": 81, "protocol": "udp"}, + { "hostPort": 8082, "containerPort": 82, "protocol": "udp"}, + { "hostPort": 8083, "containerPort": 83, "protocol": "tcp", "hostIP": "192.168.0.2"}, + { "hostPort": 8084, "containerPort": 84, "protocol": "tcp", "hostIP": "0.0.0.0"}, + { "hostPort": 8085, "containerPort": 85, "protocol": "tcp", "hostIP": "2001:db8:a::1"}, + { "hostPort": 8086, "containerPort": 86, "protocol": "tcp", "hostIP": "::"} + ] + }, + "snat": true, + "conditionsV4": ["a", "b"], + "conditionsV6": ["c", "d"] + }` + containerNet4, err := types.ParseCIDR("10.0.0.2/24") + Expect(err).NotTo(HaveOccurred()) + containerNet6, err := types.ParseCIDR("2001:db8::2/64") + Expect(err).NotTo(HaveOccurred()) for _, ver := range []string{"0.3.0", "0.3.1", "0.4.0", "1.0.0"} { // Redefine ver inside for scope so real value is picked up by each dynamically defined It() @@ -45,37 +70,14 @@ var _ = Describe("portmapping configuration (nftables)", func() { } }) - It(fmt.Sprintf("[%s] generates correct rules on ADD", ver), func() { - configBytes := []byte(fmt.Sprintf(`{ - "name": "test", - "type": "portmap", - "cniVersion": "%s", - "backend": "nftables", - "runtimeConfig": { - "portMappings": [ - { "hostPort": 8080, "containerPort": 80, "protocol": "tcp"}, - { "hostPort": 8081, "containerPort": 80, "protocol": "tcp"}, - { "hostPort": 8080, "containerPort": 81, "protocol": "udp"}, - { "hostPort": 8082, "containerPort": 82, "protocol": "udp"}, - { "hostPort": 8083, "containerPort": 83, "protocol": "tcp", "hostIP": "192.168.0.2"}, - { "hostPort": 8084, "containerPort": 84, "protocol": "tcp", "hostIP": "0.0.0.0"}, - { "hostPort": 8085, "containerPort": 85, "protocol": "tcp", "hostIP": "2001:db8:a::1"}, - { "hostPort": 8086, "containerPort": 86, "protocol": "tcp", "hostIP": "::"} - ] - }, - "snat": true, - "conditionsV4": ["a", "b"], - "conditionsV6": ["c", "d"] - }`, ver)) + It(fmt.Sprintf("[%s] generates correct rules", ver), func() { + configBytes := []byte(fmt.Sprintf(configTmpl, ver)) conf, _, err := parseConfig(configBytes, "foo") Expect(err).NotTo(HaveOccurred()) conf.ContainerID = containerID - containerNet, err := types.ParseCIDR("10.0.0.2/24") - Expect(err).NotTo(HaveOccurred()) - - err = pmNFT.forwardPorts(conf, *containerNet) + err = pmNFT.forwardPorts(conf, *containerNet4) Expect(err).NotTo(HaveOccurred()) expectedRules := strings.TrimSpace(` @@ -103,10 +105,8 @@ add rule ip cni_hostport prerouting a b jump hostports // Disable snat, generate IPv6 rules *conf.SNAT = false - containerNet, err = types.ParseCIDR("2001:db8::2/64") - Expect(err).NotTo(HaveOccurred()) - err = pmNFT.forwardPorts(conf, *containerNet) + err = pmNFT.forwardPorts(conf, *containerNet6) Expect(err).NotTo(HaveOccurred()) expectedRules = strings.TrimSpace(` @@ -129,6 +129,64 @@ add rule ip6 cni_hostport prerouting c d jump hostports actualRules = strings.TrimSpace(ipv6Fake.Dump()) Expect(actualRules).To(Equal(expectedRules)) }) + + It(fmt.Sprintf("[%s] has working CHECK", ver), func() { + configBytes := []byte(fmt.Sprintf(configTmpl, ver)) + + conf, _, err := parseConfig(configBytes, "foo") + Expect(err).NotTo(HaveOccurred()) + conf.ContainerID = containerID + + // CHECK KO: Missing all rules + err = pmNFT.checkPorts(conf, *containerNet4) + Expect(err).To(HaveOccurred()) + err = pmNFT.checkPorts(conf, *containerNet6) + Expect(err).To(HaveOccurred()) + + *conf.SNAT = false + + // ADD with SNAT=false + err = pmNFT.forwardPorts(conf, *containerNet4) + Expect(err).NotTo(HaveOccurred()) + err = pmNFT.forwardPorts(conf, *containerNet6) + Expect(err).NotTo(HaveOccurred()) + + // CHECK OK: no SNAT rules + err = pmNFT.checkPorts(conf, *containerNet4) + Expect(err).NotTo(HaveOccurred()) + err = pmNFT.checkPorts(conf, *containerNet6) + Expect(err).NotTo(HaveOccurred()) + + *conf.SNAT = true + + // CHECK KO: missing SNAT rules + err = pmNFT.checkPorts(conf, *containerNet4) + Expect(err).To(HaveOccurred()) + err = pmNFT.checkPorts(conf, *containerNet6) + Expect(err).To(HaveOccurred()) + + // ADD with SNAT=true + err = pmNFT.forwardPorts(conf, *containerNet4) + Expect(err).NotTo(HaveOccurred()) + err = pmNFT.forwardPorts(conf, *containerNet6) + Expect(err).NotTo(HaveOccurred()) + + // CHECK OK: All rules present + err = pmNFT.checkPorts(conf, *containerNet4) + Expect(err).NotTo(HaveOccurred()) + err = pmNFT.checkPorts(conf, *containerNet6) + Expect(err).NotTo(HaveOccurred()) + + // DELETE + err = pmNFT.unforwardPorts(conf) + Expect(err).NotTo(HaveOccurred()) + + // CHECK KO: Missing all rules + err = pmNFT.checkPorts(conf, *containerNet4) + Expect(err).To(HaveOccurred()) + err = pmNFT.checkPorts(conf, *containerNet6) + Expect(err).To(HaveOccurred()) + }) }) } }) From 853b7e858ea226b5a877ae2f08740c28b89e7ad9 Mon Sep 17 00:00:00 2001 From: Etienne Champetier Date: Mon, 3 Nov 2025 20:27:19 -0500 Subject: [PATCH 2/2] 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 9296c5f80a319b63f25ab0708d022d94eebc00d8 Fixes 01a94e17c77e6ff8e5019e15c42d8d92cf87194f Signed-off-by: Etienne Champetier --- plugins/meta/portmap/portmap_nftables.go | 74 ++++++++++--------- plugins/meta/portmap/portmap_nftables_test.go | 56 +++++++++++--- 2 files changed, 86 insertions(+), 44 deletions(-) diff --git a/plugins/meta/portmap/portmap_nftables.go b/plugins/meta/portmap/portmap_nftables.go index d3e7049dc..bd58e4ae0 100644 --- a/plugins/meta/portmap/portmap_nftables.go +++ b/plugins/meta/portmap/portmap_nftables.go @@ -26,9 +26,14 @@ import ( const ( tableName = "cni_hostport" + // Intermediate chain to jump to both 'hostip_hostports' and 'hostports' + hostPortsAllChain = "hostports_all" + // This chain was used for rules with hostIP, we do not use it anymore, + // but we keep it to make upgrade transparent hostIPHostPortsChain = "hostip_hostports" - hostPortsChain = "hostports" - masqueradingChain = "masquerading" + // Chain containing all the rules + hostPortsChain = "hostports" + masqueradingChain = "masquerading" ) // The nftables portmap implementation is fairly similar to the iptables implementation: @@ -104,58 +109,69 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet }) tx.Add(&knftables.Chain{ - Name: "hostports", + Name: hostPortsChain, }) + tx.Add(&knftables.Chain{ - Name: "hostip_hostports", + Name: hostIPHostPortsChain, }) + // setup intermediate chain tx.Add(&knftables.Chain{ - Name: "prerouting", - Type: knftables.PtrTo(knftables.NATType), - Hook: knftables.PtrTo(knftables.PreroutingHook), - Priority: knftables.PtrTo(knftables.DNATPriority), + Name: hostPortsAllChain, }) + tx.Flush(&knftables.Chain{ - Name: "prerouting", + Name: hostPortsAllChain, }) + tx.Add(&knftables.Rule{ - Chain: "prerouting", + Chain: hostPortsAllChain, Rule: knftables.Concat( - conditions, "jump", hostIPHostPortsChain, ), }) + tx.Add(&knftables.Rule{ - Chain: "prerouting", + Chain: hostPortsAllChain, Rule: knftables.Concat( - conditions, "jump", hostPortsChain, ), }) tx.Add(&knftables.Chain{ - Name: "output", + Name: "prerouting", Type: knftables.PtrTo(knftables.NATType), - Hook: knftables.PtrTo(knftables.OutputHook), + Hook: knftables.PtrTo(knftables.PreroutingHook), Priority: knftables.PtrTo(knftables.DNATPriority), }) tx.Flush(&knftables.Chain{ - Name: "output", + Name: "prerouting", }) tx.Add(&knftables.Rule{ - Chain: "output", + Chain: "prerouting", Rule: knftables.Concat( conditions, - "jump", hostIPHostPortsChain, + "fib daddr type local", + "jump", hostPortsAllChain, ), }) + + tx.Add(&knftables.Chain{ + Name: "output", + Type: knftables.PtrTo(knftables.NATType), + Hook: knftables.PtrTo(knftables.OutputHook), + Priority: knftables.PtrTo(knftables.DNATPriority), + }) + tx.Flush(&knftables.Chain{ + Name: "output", + }) tx.Add(&knftables.Rule{ Chain: "output", Rule: knftables.Concat( conditions, "fib daddr type local", - "jump", hostPortsChain, + "jump", hostPortsAllChain, ), }) @@ -184,8 +200,10 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet } if useHostIP { + // we add the rule to 'hostports' instead of 'hostip_hostports' + // as we want to remove 'hostip_hostports' long-term tx.Add(&knftables.Rule{ - Chain: hostIPHostPortsChain, + Chain: hostPortsChain, Rule: knftables.Concat( ipX, "daddr", e.HostIP, e.Protocol, "dport", e.HostPort, @@ -243,7 +261,7 @@ func (pmNFT *portMapperNFTables) forwardPorts(config *PortMapConf, containerNet func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet net.IPNet) error { isV6 := (containerNet.IP.To4() == nil) - var hostPorts, hostIPHostPorts, masqueradings int + var hostPorts, masqueradings int for _, e := range config.RuntimeConfig.PortMaps { if e.HostIP != "" { hostIP := net.ParseIP(e.HostIP) @@ -252,14 +270,8 @@ func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet ne if isV6 != isHostV6 { continue } - if hostIP.IsUnspecified() { - hostPorts++ - } else { - hostIPHostPorts++ - } - } else { - hostPorts++ } + hostPorts++ } if *config.SNAT { masqueradings = 1 @@ -278,12 +290,6 @@ func (pmNFT *portMapperNFTables) checkPorts(config *PortMapConf, containerNet ne return err } } - if hostIPHostPorts > 0 { - err := checkPortsAgainstRules(nft, hostIPHostPortsChain, config.ContainerID, hostIPHostPorts) - if err != nil { - return err - } - } if masqueradings > 0 { err := checkPortsAgainstRules(nft, masqueradingChain, config.ContainerID, masqueradings) if err != nil { diff --git a/plugins/meta/portmap/portmap_nftables_test.go b/plugins/meta/portmap/portmap_nftables_test.go index c367f7ad7..340c88a43 100644 --- a/plugins/meta/portmap/portmap_nftables_test.go +++ b/plugins/meta/portmap/portmap_nftables_test.go @@ -84,21 +84,22 @@ var _ = Describe("portmapping configuration (nftables)", func() { add table ip cni_hostport { comment "CNI portmap plugin" ; } add chain ip cni_hostport hostip_hostports add chain ip cni_hostport hostports +add chain ip cni_hostport hostports_all add chain ip cni_hostport masquerading { type nat hook postrouting priority 100 ; } add chain ip cni_hostport output { type nat hook output priority -100 ; } add chain ip cni_hostport prerouting { type nat hook prerouting priority -100 ; } -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" add rule ip cni_hostport hostports tcp dport 8080 dnat to 10.0.0.2:80 comment "icee6giejonei6so" add rule ip cni_hostport hostports tcp dport 8081 dnat to 10.0.0.2:80 comment "icee6giejonei6so" add rule ip cni_hostport hostports udp dport 8080 dnat to 10.0.0.2:81 comment "icee6giejonei6so" add rule ip cni_hostport hostports udp dport 8082 dnat to 10.0.0.2:82 comment "icee6giejonei6so" +add rule ip cni_hostport hostports ip daddr 192.168.0.2 tcp dport 8083 dnat to 10.0.0.2:83 comment "icee6giejonei6so" add rule ip cni_hostport hostports tcp dport 8084 dnat to 10.0.0.2:84 comment "icee6giejonei6so" +add rule ip cni_hostport hostports_all jump hostip_hostports +add rule ip cni_hostport hostports_all jump hostports add rule ip cni_hostport masquerading ip saddr 10.0.0.2 ip daddr 10.0.0.2 masquerade comment "icee6giejonei6so" add rule ip cni_hostport masquerading ip saddr 127.0.0.1 ip daddr 10.0.0.2 masquerade comment "icee6giejonei6so" -add rule ip cni_hostport output a b jump hostip_hostports -add rule ip cni_hostport output a b fib daddr type local jump hostports -add rule ip cni_hostport prerouting a b jump hostip_hostports -add rule ip cni_hostport prerouting a b jump hostports +add rule ip cni_hostport output a b fib daddr type local jump hostports_all +add rule ip cni_hostport prerouting a b fib daddr type local jump hostports_all `) actualRules := strings.TrimSpace(ipv4Fake.Dump()) Expect(actualRules).To(Equal(expectedRules)) @@ -113,18 +114,53 @@ add rule ip cni_hostport prerouting a b jump hostports add table ip6 cni_hostport { comment "CNI portmap plugin" ; } add chain ip6 cni_hostport hostip_hostports add chain ip6 cni_hostport hostports +add chain ip6 cni_hostport hostports_all add chain ip6 cni_hostport output { type nat hook output priority -100 ; } add chain ip6 cni_hostport prerouting { type nat hook prerouting priority -100 ; } -add rule ip6 cni_hostport hostip_hostports ip6 daddr 2001:db8:a::1 tcp dport 8085 dnat to [2001:db8::2]:85 comment "icee6giejonei6so" add rule ip6 cni_hostport hostports tcp dport 8080 dnat to [2001:db8::2]:80 comment "icee6giejonei6so" add rule ip6 cni_hostport hostports tcp dport 8081 dnat to [2001:db8::2]:80 comment "icee6giejonei6so" add rule ip6 cni_hostport hostports udp dport 8080 dnat to [2001:db8::2]:81 comment "icee6giejonei6so" add rule ip6 cni_hostport hostports udp dport 8082 dnat to [2001:db8::2]:82 comment "icee6giejonei6so" +add rule ip6 cni_hostport hostports ip6 daddr 2001:db8:a::1 tcp dport 8085 dnat to [2001:db8::2]:85 comment "icee6giejonei6so" add rule ip6 cni_hostport hostports tcp dport 8086 dnat to [2001:db8::2]:86 comment "icee6giejonei6so" -add rule ip6 cni_hostport output c d jump hostip_hostports -add rule ip6 cni_hostport output c d fib daddr type local jump hostports -add rule ip6 cni_hostport prerouting c d jump hostip_hostports -add rule ip6 cni_hostport prerouting c d jump hostports +add rule ip6 cni_hostport hostports_all jump hostip_hostports +add rule ip6 cni_hostport hostports_all jump hostports +add rule ip6 cni_hostport output c d fib daddr type local jump hostports_all +add rule ip6 cni_hostport prerouting c d fib daddr type local jump hostports_all +`) + actualRules = strings.TrimSpace(ipv6Fake.Dump()) + Expect(actualRules).To(Equal(expectedRules)) + + err = pmNFT.unforwardPorts(conf) + Expect(err).NotTo(HaveOccurred()) + + expectedRules = strings.TrimSpace(` +add table ip cni_hostport { comment "CNI portmap plugin" ; } +add chain ip cni_hostport hostip_hostports +add chain ip cni_hostport hostports +add chain ip cni_hostport hostports_all +add chain ip cni_hostport masquerading { type nat hook postrouting priority 100 ; } +add chain ip cni_hostport output { type nat hook output priority -100 ; } +add chain ip cni_hostport prerouting { type nat hook prerouting priority -100 ; } +add rule ip cni_hostport hostports_all jump hostip_hostports +add rule ip cni_hostport hostports_all jump hostports +add rule ip cni_hostport output a b fib daddr type local jump hostports_all +add rule ip cni_hostport prerouting a b fib daddr type local jump hostports_all +`) + actualRules = strings.TrimSpace(ipv4Fake.Dump()) + Expect(actualRules).To(Equal(expectedRules)) + + expectedRules = strings.TrimSpace(` +add table ip6 cni_hostport { comment "CNI portmap plugin" ; } +add chain ip6 cni_hostport hostip_hostports +add chain ip6 cni_hostport hostports +add chain ip6 cni_hostport hostports_all +add chain ip6 cni_hostport output { type nat hook output priority -100 ; } +add chain ip6 cni_hostport prerouting { type nat hook prerouting priority -100 ; } +add rule ip6 cni_hostport hostports_all jump hostip_hostports +add rule ip6 cni_hostport hostports_all jump hostports +add rule ip6 cni_hostport output c d fib daddr type local jump hostports_all +add rule ip6 cni_hostport prerouting c d fib daddr type local jump hostports_all `) actualRules = strings.TrimSpace(ipv6Fake.Dump()) Expect(actualRules).To(Equal(expectedRules))