fix(NPC,LBC): harden network policy and load balancer controllers

pkg/controllers/lballoc/lballoc.go

@@ -3,8 +3,10 @@ package lballoc
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"os"
+	"strings"
 	"sync"
 	"time"
 
@@ -391,16 +393,25 @@ func (lbc *LoadBalancerController) allocateService(svc *v1core.Service) error {
 	if want6 && !have6 {
 		ipv6, err6 = lbc.ipv6Ranges.getNextFreeIP(allocated6)
 	}
-	err := err6
+
+	// If no allocation was needed, the service already has all requested IPs
+	if ipv4 == nil && ipv6 == nil && err4 == nil && err6 == nil {
+		return nil
+	}
+
+	var errMsgs []string
 	if err4 != nil {
-		err = err4
+		errMsgs = append(errMsgs, fmt.Sprintf("IPv4: %v", err4))
+	}
+	if err6 != nil {
+		errMsgs = append(errMsgs, fmt.Sprintf("IPv6: %v", err6))
 	}
 
 	if ipv4 == nil && ipv6 == nil {
-		return errors.New("unable to allocate address: " + err.Error())
+		return fmt.Errorf("unable to allocate address: %s", strings.Join(errMsgs, "; "))
 	}
 	if (ipv4 == nil || ipv6 == nil) && requireDual {
-		return errors.New("unable to allocate dual-stack addresses: " + err.Error())
+		return fmt.Errorf("unable to allocate dual-stack addresses: %s", strings.Join(errMsgs, "; "))
 	}
 
 	// This is only non-nil during certain unit tests that need to understand when this goroutine is finished to remove

pkg/controllers/lballoc/lballoc_test.go

@@ -596,14 +596,15 @@ func TestAllocateService(t *testing.T) {
 	fp := v1core.IPFamilyPolicyRequireDualStack
 	svc.Spec.IPFamilyPolicy = &fp
 	err = mlbc.allocateService(&svc)
-	errExp := "unable to allocate dual-stack addresses: no IPs left to allocate"
+	errExp := "unable to allocate dual-stack addresses: IPv4: no IPs left to allocate"
 	if errExp != err.Error() {
 		t.Fatalf("expected %s, got %s", errExp, err)
 	}
 
 	mlbc.ipv4Ranges = ir4
 	mlbc.ipv6Ranges = newipRanges(nil)
 	err = mlbc.allocateService(&svc)
+	errExp = "unable to allocate dual-stack addresses: IPv6: no IPs left to allocate"
 	if errExp != err.Error() {
 		t.Fatalf("expected %s, got %s", errExp, err)
 	}
@@ -612,7 +613,7 @@ func TestAllocateService(t *testing.T) {
 	fp = v1core.IPFamilyPolicyPreferDualStack
 	svc.Spec.IPFamilyPolicy = &fp
 	err = mlbc.allocateService(&svc)
-	errExp = "unable to allocate address: no IPs left to allocate"
+	errExp = "unable to allocate address: IPv4: no IPs left to allocate; IPv6: no IPs left to allocate"
 	if errExp != err.Error() {
 		t.Fatalf("expected %s, got %s", errExp, err)
 	}

pkg/controllers/netpol/pod.go

@@ -4,12 +4,27 @@ import (
 	"crypto/sha256"
 	"encoding/base32"
 	"strings"
+	"unicode"
 
 	api "k8s.io/api/core/v1"
 	"k8s.io/client-go/tools/cache"
 	"k8s.io/klog/v2"
 )
 
+// sanitizeForComment removes control characters (newlines, tabs, etc.) from a
+// string before it is embedded into an iptables-restore comment. This is a
+// defense-in-depth measure: Kubernetes API server validates pod/namespace names
+// to DNS label format, but we should not rely solely on external validation when
+// constructing input for iptables-restore.
+func sanitizeForComment(s string) string {
+	return strings.Map(func(r rune) rune {
+		if unicode.IsControl(r) {
+			return -1
+		}
+		return r
+	}, s)
+}
+
 func (npc *NetworkPolicyController) newPodEventHandler() cache.ResourceEventHandler {
 	return cache.ResourceEventHandlerFuncs{
 		AddFunc: func(obj interface{}) {
@@ -88,7 +103,7 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 			}
 
 			// add rule to log the packets that will be dropped due to network policy enforcement
-			comment := "\"rule to log dropped traffic POD name:" + pod.name + " namespace: " + pod.namespace + "\""
+			comment := "\"rule to log dropped traffic POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) + "\""
 			args := []string{"-A", podFwChainName, "-m", "comment", "--comment", comment,
 				"-m", "mark", "!", "--mark", "0x10000/0x10000", "-j", "NFLOG",
 				"--nflog-group", "100", "-m", "limit", "--limit", "10/minute", "--limit-burst", "10", "\n"}
@@ -100,8 +115,8 @@ func (npc *NetworkPolicyController) syncPodFirewallChains(networkPoliciesInfo []
 			filterTableRules.WriteString(strings.Join(args, " "))
 
 			// add rule to DROP if no applicable network policy permits the traffic
-			comment = "\"rule to REJECT traffic destined for POD name:" + pod.name + " namespace: " +
-				pod.namespace + "\""
+			comment = "\"rule to REJECT traffic destined for POD name:" + sanitizeForComment(pod.name) + " namespace: " +
+				sanitizeForComment(pod.namespace) + "\""
 			args = []string{"-A", podFwChainName, "-m", "comment", "--comment", comment,
 				"-m", "mark", "!", "--mark", "0x10000/0x10000", "-j", "REJECT", "\n"}
 			filterTableRules.WriteString(strings.Join(args, " "))
@@ -188,7 +203,7 @@ func (npc *NetworkPolicyController) setupPodNetpolRules(pod podInfo, podFwChainN
 			if _, ok := policy.targetPods[pod.ip]; !ok {
 				continue
 			}
-			comment := "\"run through nw policy " + policy.name + "\""
+			comment := "\"run through nw policy " + sanitizeForComment(policy.name) + "\""
 			policyChainName := networkPolicyChainName(policy.namespace, policy.name, version, ipFamily)
 			var args []string
 			switch policy.policyType {
@@ -255,7 +270,7 @@ func (npc *NetworkPolicyController) interceptPodInboundTraffic(pod podInfo, podF
 
 		// ensure there is rule in filter table and FORWARD chain to jump to pod specific firewall chain
 		// this rule applies to the traffic getting routed (coming for other node pods)
-		comment := "\"rule to jump traffic destined to POD name:" + pod.name + " namespace: " + pod.namespace +
+		comment := "\"rule to jump traffic destined to POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) +
 			" to chain " + podFwChainName + "\""
 		args := []string{"-A", kubeForwardChainName, "-m", "comment", "--comment", comment, "-d", ip,
 			"-j", podFwChainName + "\n"}
@@ -269,7 +284,7 @@ func (npc *NetworkPolicyController) interceptPodInboundTraffic(pod podInfo, podF
 
 		// ensure there is rule in filter table and forward chain to jump to pod specific firewall chain
 		// this rule applies to the traffic getting switched (coming for same node pods)
-		comment = "\"rule to jump traffic destined to POD name:" + pod.name + " namespace: " + pod.namespace +
+		comment = "\"rule to jump traffic destined to POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) +
 			" to chain " + podFwChainName + "\""
 		args = []string{"-A", kubeForwardChainName, "-m", "physdev", "--physdev-is-bridged",
 			"-m", "comment", "--comment", comment,
@@ -294,15 +309,15 @@ func (npc *NetworkPolicyController) interceptPodOutboundTraffic(pod podInfo, pod
 			// ensure there is rule in filter table and FORWARD chain to jump to pod specific firewall chain
 			// this rule applies to the traffic getting forwarded/routed (traffic from the pod destined
 			// to pod on a different node)
-			comment := "\"rule to jump traffic from POD name:" + pod.name + " namespace: " + pod.namespace +
+			comment := "\"rule to jump traffic from POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) +
 				" to chain " + podFwChainName + "\""
 			args := []string{"-A", chain, "-m", "comment", "--comment", comment, "-s", ip, "-j", podFwChainName, "\n"}
 			filterTableRules.WriteString(strings.Join(args, " "))
 		}
 
 		// ensure there is rule in filter table and forward chain to jump to pod specific firewall chain
 		// this rule applies to the traffic getting switched (coming for same node pods)
-		comment := "\"rule to jump traffic from POD name:" + pod.name + " namespace: " + pod.namespace +
+		comment := "\"rule to jump traffic from POD name:" + sanitizeForComment(pod.name) + " namespace: " + sanitizeForComment(pod.namespace) +
 			" to chain " + podFwChainName + "\""
 		args := []string{"-A", kubeForwardChainName, "-m", "physdev", "--physdev-is-bridged",
 			"-m", "comment", "--comment", comment,

pkg/controllers/netpol/sanitize_test.go

@@ -0,0 +1,58 @@
+package netpol
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSanitizeForComment(t *testing.T) {
+	tests := []struct {
+		name  string
+		input string
+		want  string
+	}{
+		{
+			name:  "clean string passes through",
+			input: "my-pod-name",
+			want:  "my-pod-name",
+		},
+		{
+			name:  "newlines stripped",
+			input: "my-pod\n-A INJECT -j DROP\n",
+			want:  "my-pod-A INJECT -j DROP",
+		},
+		{
+			name:  "tabs stripped",
+			input: "my-pod\tinjected",
+			want:  "my-podinjected",
+		},
+		{
+			name:  "carriage return stripped",
+			input: "my-pod\rinjected",
+			want:  "my-podinjected",
+		},
+		{
+			name:  "null bytes stripped",
+			input: "my-pod\x00injected",
+			want:  "my-podinjected",
+		},
+		{
+			name:  "normal characters preserved",
+			input: "nginx-deployment-7c79f4c9b8-abc12",
+			want:  "nginx-deployment-7c79f4c9b8-abc12",
+		},
+		{
+			name:  "empty string",
+			input: "",
+			want:  "",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := sanitizeForComment(tt.input)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}