From e96c71a0a0e77c73cbbf95298cdd68ce914ce7cd Mon Sep 17 00:00:00 2001 From: Andrea Panattoni Date: Thu, 18 Jul 2024 16:39:59 +0200 Subject: [PATCH 1/2] Make sure that policies with no valid peers are enforced If a policy rule has a `from` (or `to`) selector that matches no pods, the subject pod has to not be reached by (or has to not reach) any pods. The following example helps clarify the reasons behind these: Given a scenario with 3 pods (A, B, C) and a rule like: ``` podSelector: matchLabels: name: A ingress: - from: - podSelector: matchLabels: name: B policyTypes: - Ingress ``` Pod A can be reached only by pod B. Pod C can't reach A, and this has to be ensured even if pod B is deleted. Add an end-to-end test case to validate this scenario and adjust unit tests accordingly. Signed-off-by: Andrea Panattoni --- e2e/tests/ingress-ns-selector-no-pods.bats | 63 ++++++++++++ e2e/tests/ingress-ns-selector-no-pods.yml | 109 +++++++++++++++++++++ pkg/server/policyrules.go | 12 +-- pkg/server/policyrules_test.go | 98 ++++++++++++++++-- 4 files changed, 263 insertions(+), 19 deletions(-) create mode 100755 e2e/tests/ingress-ns-selector-no-pods.bats create mode 100644 e2e/tests/ingress-ns-selector-no-pods.yml diff --git a/e2e/tests/ingress-ns-selector-no-pods.bats b/e2e/tests/ingress-ns-selector-no-pods.bats new file mode 100755 index 00000000..466e37b1 --- /dev/null +++ b/e2e/tests/ingress-ns-selector-no-pods.bats @@ -0,0 +1,63 @@ +#!/usr/bin/env bats + + +# Note: +# These test cases verify that an ingress rule on the server that matches no pods +# does not allow any pods to reach the server. + +setup() { + cd $BATS_TEST_DIRNAME + load "common" + server_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods" "pod-server") + client_a_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods" "pod-client-a") + client_b_net1=$(get_net1_ip "test-ingress-ns-selector-no-pods-blue" "pod-client-b") +} + +@test "setup ingress-ns-selector-no-pods test environments" { + # create test manifests + kubectl create -f ingress-ns-selector-no-pods.yml + + # verify all pods are available + run kubectl -n test-ingress-ns-selector-no-pods wait --for=condition=ready -l app=test-ingress-ns-selector-no-pods pod --timeout=${kubewait_timeout} + [ "$status" -eq "0" ] + + sleep 5 +} + +@test "test-ingress-ns-selector-no-pods check client-a -> server" { + # nc should NOT succeed from client-a to server by policy + run kubectl -n test-ingress-ns-selector-no-pods exec pod-client-a -- sh -c "echo x | nc -w 1 ${server_net1} 5555" + [ "$status" -eq "1" ] +} + +@test "test-ingress-ns-selector-no-pods check client-b -> server" { + # nc should NOT succeed from client-b to server by policy + run kubectl -n test-ingress-ns-selector-no-pods-blue exec pod-client-b -- sh -c "echo x | nc -w 1 ${server_net1} 5555" + [ "$status" -eq "1" ] +} + +@test "test-ingress-ns-selector-no-pods check server -> client-a" { + # nc should succeed from server to client-a by no policy definition for direction (egress for pod-server) + run kubectl -n test-ingress-ns-selector-no-pods exec pod-server -- sh -c "echo x | nc -w 1 ${client_a_net1} 5555" + [ "$status" -eq "0" ] +} + +@test "test-ingress-ns-selector-no-pods check server -> client-b" { + # nc should succeed from server to client-b by no policy definition for direction (egress for pod-server) + run kubectl -n test-ingress-ns-selector-no-pods exec pod-server -- sh -c "echo x | nc -w 1 ${client_b_net1} 5555" + [ "$status" -eq "0" ] +} + +@test "cleanup environments" { + # remove test manifests + kubectl delete -f ingress-ns-selector-no-pods.yml + run kubectl -n test-ingress-ns-selector-no-pods wait --for=delete -l app=test-ingress-ns-selector-no-pods pod --timeout=${kubewait_timeout} + [ "$status" -eq "0" ] + + sleep 3 + # check that no iptables files in pod-iptables + pod_name=$(kubectl -n kube-system get pod -o wide | grep 'kind-worker' | grep multi-net | cut -f 1 -d ' ') + run kubectl -n kube-system exec ${pod_name} -- \ + sh -c "find /var/lib/multi-networkpolicy/iptables/ -name '*.iptables' | wc -l" + [ "$output" = "0" ] +} diff --git a/e2e/tests/ingress-ns-selector-no-pods.yml b/e2e/tests/ingress-ns-selector-no-pods.yml new file mode 100644 index 00000000..9cd041fe --- /dev/null +++ b/e2e/tests/ingress-ns-selector-no-pods.yml @@ -0,0 +1,109 @@ +--- +apiVersion: "k8s.cni.cncf.io/v1" +kind: NetworkAttachmentDefinition +metadata: + namespace: default + name: macvlan1-simple +spec: + config: '{ + "cniVersion": "0.3.1", + "name": "macvlan1-simple", + "plugins": [ + { + "type": "macvlan", + "mode": "bridge", + "ipam":{ + "type":"host-local", + "subnet":"2.2.6.0/24", + "rangeStart":"2.2.6.8", + "rangeEnd":"2.2.6.67" + } + }] + }' +--- +# namespace for MultiNetworkPolicy +apiVersion: v1 +kind: Namespace +metadata: + name: test-ingress-ns-selector-no-pods +--- +apiVersion: v1 +kind: Namespace +metadata: + name: test-ingress-ns-selector-no-pods-blue +--- +# Pods +apiVersion: v1 +kind: Pod +metadata: + name: pod-server + namespace: test-ingress-ns-selector-no-pods + annotations: + k8s.v1.cni.cncf.io/networks: default/macvlan1-simple + labels: + app: test-ingress-ns-selector-no-pods + name: pod-server +spec: + containers: + - name: macvlan-worker1 + image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test + command: ["nc", "-klp", "5555"] + securityContext: + privileged: true +--- +apiVersion: v1 +kind: Pod +metadata: + name: pod-client-a + namespace: test-ingress-ns-selector-no-pods + annotations: + k8s.v1.cni.cncf.io/networks: default/macvlan1-simple + labels: + app: test-ingress-ns-selector-no-pods + name: pod-client-a +spec: + containers: + - name: macvlan-worker1 + image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test + command: ["nc", "-klp", "5555"] + securityContext: + privileged: true +--- +apiVersion: v1 +kind: Pod +metadata: + name: pod-client-b + namespace: test-ingress-ns-selector-no-pods-blue + annotations: + k8s.v1.cni.cncf.io/networks: default/macvlan1-simple + labels: + app: test-ingress-ns-selector-no-pods + name: pod-client-b +spec: + containers: + - name: macvlan-worker1 + image: ghcr.io/k8snetworkplumbingwg/multi-networkpolicy-iptables:e2e-test + command: ["nc", "-klp", "5555"] + securityContext: + privileged: true +--- +# MultiNetworkPolicies +# this policy accepts ingress trafic from pod-client-a to pod-server +apiVersion: k8s.cni.cncf.io/v1beta1 +kind: MultiNetworkPolicy +metadata: + name: policy-test-ingress-ns-selector-no-pods + namespace: test-ingress-ns-selector-no-pods + annotations: + k8s.v1.cni.cncf.io/policy-for: default/macvlan1-simple +spec: + podSelector: + matchLabels: + name: pod-server + ingress: + - from: + - namespaceSelector: + matchLabels: + foo: bar # No namespace with this label exists + policyTypes: + - Ingress diff --git a/pkg/server/policyrules.go b/pkg/server/policyrules.go index ad34378b..46a28776 100644 --- a/pkg/server/policyrules.go +++ b/pkg/server/policyrules.go @@ -280,7 +280,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI writeLine(ipt.policyIndex, "-A", fmt.Sprintf("MULTI-%d-INGRESS", pIndex), "-j", chainName) s.podMap.Update(s.podChanges) - validPeers := 0 for _, peer := range from { if peer.PodSelector != nil || peer.NamespaceSelector != nil { podSelectorMap, err := metav1.LabelSelectorAsMap(peer.PodSelector) @@ -334,7 +333,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI writeLine(ipt.ingressFrom, "-A", chainName, "-i", podIntf.InterfaceName, "-s", ip, "-j", "MARK", "--set-xmark", "0x20000/0x20000") - validPeers++ } } // ingress should accept reverse path @@ -357,7 +355,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI if ipt.isIPFamilyCompatible(except) { writeLine(ipt.ingressFrom, "-A", chainName, "-i", podIntf.InterfaceName, "-s", except, "-j", "DROP") - validPeers++ } } } @@ -369,7 +366,6 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI writeLine(ipt.ingressFrom, "-A", chainName, "-i", podIntf.InterfaceName, "-s", peer.IPBlock.CIDR, "-j", "MARK", "--set-xmark", "0x20000/0x20000") - validPeers++ } } for _, podIntf := range podInfo.Interfaces { @@ -390,7 +386,7 @@ func (ipt *iptableBuffer) renderIngressFrom(s *Server, podInfo *controllers.PodI } // Add skip rule if no froms - if len(from) == 0 || validPeers == 0 { + if len(from) == 0 { writeLine(ipt.ingressFrom, "-A", chainName, "-m", "comment", "--comment", "\"no ingress from, skipped\"", "-j", "MARK", "--set-xmark", "0x20000/0x20000") @@ -509,7 +505,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo writeLine(ipt.policyIndex, "-A", fmt.Sprintf("MULTI-%d-EGRESS", pIndex), "-j", chainName) s.podMap.Update(s.podChanges) - validPeers := 0 for _, peer := range to { if peer.PodSelector != nil || peer.NamespaceSelector != nil { podSelectorMap, err := metav1.LabelSelectorAsMap(peer.PodSelector) @@ -564,7 +559,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo writeLine(ipt.egressTo, "-A", chainName, "-o", podIntf.InterfaceName, "-d", ip, "-j", "MARK", "--set-xmark", "0x20000/0x20000") - validPeers++ } } // egress should accept reverse path @@ -587,7 +581,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo if ipt.isIPFamilyCompatible(except) { writeLine(ipt.egressTo, "-A", chainName, "-o", multi.InterfaceName, "-d", except, "-j", "DROP") - validPeers++ } } } @@ -599,7 +592,6 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo writeLine(ipt.egressTo, "-A", chainName, "-o", podIntf.InterfaceName, "-d", peer.IPBlock.CIDR, "-j", "MARK", "--set-xmark", "0x20000/0x20000") - validPeers++ } } // egress should accept reverse path @@ -621,7 +613,7 @@ func (ipt *iptableBuffer) renderEgressTo(s *Server, podInfo *controllers.PodInfo } // Add skip rules if no to - if len(to) == 0 || validPeers == 0 { + if len(to) == 0 { writeLine(ipt.egressTo, "-A", chainName, "-m", "comment", "--comment", "\"no egress to, skipped\"", "-j", "MARK", "--set-xmark", "0x20000/0x20000") diff --git a/pkg/server/policyrules_test.go b/pkg/server/policyrules_test.go index 8cbf83ac..66870a70 100644 --- a/pkg/server/policyrules_test.go +++ b/pkg/server/policyrules_test.go @@ -1257,6 +1257,92 @@ COMMIT Expect(buf.filterRules.String()).To(Equal(string(finalizedRules))) }) + It("ingress rules namespaceSeelctor with non existent labels", func() { + ingressPolicies1 := &multiv1beta1.MultiNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{ + Name: "ingressPolicies1", + Namespace: "testns1", + }, + Spec: multiv1beta1.MultiNetworkPolicySpec{ + Ingress: []multiv1beta1.MultiNetworkPolicyIngressRule{ + { + From: []multiv1beta1.MultiNetworkPolicyPeer{ + { + NamespaceSelector: &metav1.LabelSelector{ + MatchLabels: map[string]string{ + "foo": "bar", // No namespace exists with this label + }, + }, + }, + }, + }, + }, + }, + } + + ipt := fakeiptables.NewFake() + Expect(ipt).NotTo(BeNil()) + buf := newIptableBuffer() + Expect(buf).NotTo(BeNil()) + + // verify buf initialized at init + buf.Init(ipt) + s := NewFakeServer("samplehost") + Expect(s).NotTo(BeNil()) + + AddNamespace(s, "testns1") + AddNamespace(s, "testns2") + + Expect(s.netdefChanges.Update( + nil, + NewNetDef("testns1", "net-attach1", NewCNIConfig("testCNI", "multi")))).To(BeTrue()) + Expect(s.netdefChanges.GetPluginType(types.NamespacedName{Namespace: "testns1", Name: "net-attach1"})).To(Equal("multi")) + Expect(s.netdefChanges.Update( + nil, + NewNetDef("testns2", "net-attach1", NewCNIConfig("testCNI", "multi")))).To(BeTrue()) + Expect(s.netdefChanges.GetPluginType(types.NamespacedName{Namespace: "testns2", Name: "net-attach1"})).To(Equal("multi")) + + pod1 := NewFakePodWithNetAnnotation( + "testns1", + "testpod1", + "net-attach1", + NewFakeNetworkStatus("testns1", "net-attach1", "192.168.1.1", "10.1.1.1"), + nil) + AddPod(s, pod1) + podInfo1, err := s.podMap.GetPodInfo(pod1) + Expect(err).NotTo(HaveOccurred()) + + pod2 := NewFakePodWithNetAnnotation( + "testns2", + "testpod2", + "net-attach1", + NewFakeNetworkStatus("testns2", "net-attach1", "192.168.1.2", "10.1.1.2"), + nil) + AddPod(s, pod2) + buf.renderIngress(s, podInfo1, 0, ingressPolicies1, []string{"testns1/net-attach1", "testns2/net-attach1"}) + + buf.FinalizeRules() + finalizedRules := + `*filter +:MULTI-INGRESS - [0:0] +:MULTI-INGRESS-COMMON - [0:0] +:MULTI-EGRESS - [0:0] +:MULTI-EGRESS-COMMON - [0:0] +:MULTI-0-INGRESS - [0:0] +:MULTI-0-INGRESS-0-PORTS - [0:0] +:MULTI-0-INGRESS-0-FROM - [0:0] +-A MULTI-INGRESS -m comment --comment "policy:ingressPolicies1 net-attach-def:testns1/net-attach1" -i net1 -j MULTI-0-INGRESS +-A MULTI-INGRESS -m mark --mark 0x30000/0x30000 -j RETURN +-A MULTI-0-INGRESS -j MARK --set-xmark 0x0/0x30000 +-A MULTI-0-INGRESS -j MULTI-0-INGRESS-0-PORTS +-A MULTI-0-INGRESS -j MULTI-0-INGRESS-0-FROM +-A MULTI-0-INGRESS -m mark --mark 0x30000/0x30000 -j RETURN +-A MULTI-0-INGRESS-0-PORTS -m comment --comment "no ingress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 +COMMIT +` + Expect(buf.filterRules.String()).To(Equal(string(finalizedRules))) + }) + It("enforce policy with net-attach-def in a different namespace than pods", func() { ingressPolicies1 := &multiv1beta1.MultiNetworkPolicy{ ObjectMeta: metav1.ObjectMeta{ @@ -1811,13 +1897,11 @@ COMMIT -A MULTI-0-EGRESS -j MULTI-0-EGRESS-0-TO -A MULTI-0-EGRESS -m mark --mark 0x30000/0x30000 -j RETURN -A MULTI-0-INGRESS-0-PORTS -m comment --comment "no ingress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-INGRESS-0-FROM -m comment --comment "no ingress from, skipped" -j MARK --set-xmark 0x20000/0x20000 -A MULTI-0-EGRESS-0-PORTS -m comment --comment "no egress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-EGRESS-0-TO -m comment --comment "no egress to, skipped" -j MARK --set-xmark 0x20000/0x20000 COMMIT ` - Expect(buf.filterRules.String()).To(Equal(expectedRules), buf.filterRules.String()) + Expect(buf.filterRules.String()).To(Equal(expectedRules), buf.filterRules.String) }) It("shoud manage dual stack networks", func() { @@ -2059,10 +2143,9 @@ COMMIT -A MULTI-0-INGRESS -j MULTI-0-INGRESS-0-FROM -A MULTI-0-INGRESS -m mark --mark 0x30000/0x30000 -j RETURN -A MULTI-0-INGRESS-0-PORTS -m comment --comment "no ingress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-INGRESS-0-FROM -m comment --comment "no ingress from, skipped" -j MARK --set-xmark 0x20000/0x20000 COMMIT ` - Expect(buf.filterRules.String()).To(Equal(finalizedRules)) + Expect(buf.filterRules.String()).To(Equal(finalizedRules), buf.filterRules.String()) }) It("ingress rules podselector/matchlabels", func() { @@ -2150,10 +2233,9 @@ COMMIT -A MULTI-0-INGRESS -j MULTI-0-INGRESS-0-FROM -A MULTI-0-INGRESS -m mark --mark 0x30000/0x30000 -j RETURN -A MULTI-0-INGRESS-0-PORTS -m comment --comment "no ingress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-INGRESS-0-FROM -m comment --comment "no ingress from, skipped" -j MARK --set-xmark 0x20000/0x20000 COMMIT ` - Expect(buf.filterRules.String()).To(Equal(finalizedRules)) + Expect(buf.filterRules.String()).To(Equal(finalizedRules), buf.filterRules.String()) }) It("egress rules ipblock", func() { @@ -2228,7 +2310,6 @@ COMMIT -A MULTI-0-EGRESS -j MULTI-0-EGRESS-0-TO -A MULTI-0-EGRESS -m mark --mark 0x30000/0x30000 -j RETURN -A MULTI-0-EGRESS-0-PORTS -m comment --comment "no egress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-EGRESS-0-TO -m comment --comment "no egress to, skipped" -j MARK --set-xmark 0x20000/0x20000 COMMIT ` Expect(buf.filterRules.String()).To(Equal(finalizedRules)) @@ -2319,7 +2400,6 @@ COMMIT -A MULTI-0-EGRESS -j MULTI-0-EGRESS-0-TO -A MULTI-0-EGRESS -m mark --mark 0x30000/0x30000 -j RETURN -A MULTI-0-EGRESS-0-PORTS -m comment --comment "no egress ports, skipped" -j MARK --set-xmark 0x10000/0x10000 --A MULTI-0-EGRESS-0-TO -m comment --comment "no egress to, skipped" -j MARK --set-xmark 0x20000/0x20000 COMMIT ` Expect(buf.filterRules.String()).To(Equal(finalizedRules)) From 26d979ac0bb860dd16b7f3b978be0daa2470172d Mon Sep 17 00:00:00 2001 From: Andrea Panattoni Date: Thu, 18 Jul 2024 16:49:12 +0200 Subject: [PATCH 2/2] Run all e2e tests in `./e2e/tests/*.bats` Signed-off-by: Andrea Panattoni --- .github/workflows/kind-e2e.yml | 13 ++----------- 1 file changed, 2 insertions(+), 11 deletions(-) diff --git a/.github/workflows/kind-e2e.yml b/.github/workflows/kind-e2e.yml index 96311d71..1724baca 100644 --- a/.github/workflows/kind-e2e.yml +++ b/.github/workflows/kind-e2e.yml @@ -30,17 +30,8 @@ jobs: export TERM=dumb # enable ip6_tables sudo modprobe ip6_tables - bats ./tests/simple-v4-ingress.bats - bats ./tests/simple-v4-egress.bats - bats ./tests/simple-v6-ingress.bats - bats ./tests/ipblock.bats - # stacked case - bats ./tests/stacked.bats - bats ./tests/ipblock-stacked.bats - # multiple items in egress/ingress - bats ./tests/ipblock-list.bats - bats ./tests/simple-v4-ingress-list.bats - bats ./tests/simple-v4-egress-list.bats + + bats ./tests/*.bats - name: Export kind logs if: ${{ failure() }}