[v3.31] app-policy: match named ports against the IP+port set key

app-policy/checker/match.go

@@ -29,10 +29,16 @@ import (
 	"github.com/projectcalico/calico/libcalico-go/lib/selector"
 )
 
+// protocolMapL4 maps an L4 protocol number to the name Felix uses in IP+port IP set
+// members. A named port can be declared on tcp, udp or sctp, and Felix emits members
+// for all three. Envoy's ext_authz adapter only ever reports tcp or udp, but the key is
+// built from whatever the Flow reports, so a Flow carrying 132 gets "sctp" here rather
+// than an empty protocol field that no member can equal.
 var protocolMapL4 = map[int32]string{
-	1:  "icmp",
-	6:  "tcp",
-	17: "udp",
+	1:   "icmp",
+	6:   "tcp",
+	17:  "udp",
+	132: "sctp",
 }
 
 type namespaceMatch struct {
@@ -498,10 +504,8 @@ func matchDstIPPortSetIds(r *proto.Rule, req *requestCache) bool {
 			"DstIpPortSetIds": r.GetDstIpPortSetIds(),
 		}).Debug("matching destination IP port sets")
 	}
-	protocolStr := protocolMapL4[int32(req.GetProtocol())]
-	// The values compared against are of the for "ip,protocol:port".
-	ipProtoPort := fmt.Sprintf("%s,%s:%d", req.GetDestIP(), protocolStr, req.GetDestPort())
-	return matchIPSetsAll(r.GetDstIpPortSetIds(), req.getIPSet, ipProtoPort)
+	// The values compared against are of the form "ip,protocol:port".
+	return matchIPSetsAll(r.GetDstIpPortSetIds(), req.getIPSet, req.getDstIPProtoPortStr())
 }
 
 // matchDstIPSets checks if the destination IP is within the IP sets and not in the not IP sets. It
@@ -543,20 +547,24 @@ func matchIPSetsNotAny(ids []string, ipsSetFunc func(string) policystore.IPSet,
 // matchDstPort checks if the destination port is within the port ranges and named port sets. It
 // also checks if the destination port is not within the not port ranges and named port sets.
 func matchDstPort(r *proto.Rule, req *requestCache) bool {
-	return matchPort("dst", r.GetDstPorts(), r.GetDstNamedPortIpSetIds(), req.getIPSet, req.GetDestPort()) &&
-		matchNotPort("dst", r.GetNotDstPorts(), r.GetNotDstNamedPortIpSetIds(), req.getIPSet, req.GetDestPort())
+	return matchPort("dst", r.GetDstPorts(), r.GetDstNamedPortIpSetIds(), req.getIPSet, req.GetDestPort(), req.getDstIPProtoPortStr) &&
+		matchNotPort("dst", r.GetNotDstPorts(), r.GetNotDstNamedPortIpSetIds(), req.getIPSet, req.GetDestPort(), req.getDstIPProtoPortStr)
 }
 
 // matchSrcPort checks if the source port is within the port ranges and named port sets. It also
 // checks if the source port is not within the not port ranges and named port sets.
 func matchSrcPort(r *proto.Rule, req *requestCache) bool {
-	return matchPort("src", r.GetSrcPorts(), r.GetSrcNamedPortIpSetIds(), req.getIPSet, req.GetSourcePort()) &&
-		matchNotPort("src", r.GetNotSrcPorts(), r.GetNotSrcNamedPortIpSetIds(), req.getIPSet, req.GetSourcePort())
+	return matchPort("src", r.GetSrcPorts(), r.GetSrcNamedPortIpSetIds(), req.getIPSet, req.GetSourcePort(), req.getSrcIPProtoPortStr) &&
+		matchNotPort("src", r.GetNotSrcPorts(), r.GetNotSrcNamedPortIpSetIds(), req.getIPSet, req.GetSourcePort(), req.getSrcIPProtoPortStr)
 }
 
 // matchPort checks if the port is within the port ranges and named port sets. It returns true if
 // the port matches, false otherwise.
-func matchPort(dir string, ranges []*proto.PortRange, namedPortSets []string, ipsSetFunc func(string) policystore.IPSet, port int) bool {
+//
+// A named port set holds "<IP>,<protocol>:<port>" members, not bare port numbers: it names a
+// port on a specific set of endpoints. namedPortKey supplies that key for the leg being tested,
+// which is what the other dataplanes match such a set on.
+func matchPort(dir string, ranges []*proto.PortRange, namedPortSets []string, ipsSetFunc func(string) policystore.IPSet, port int, namedPortKey func() string) bool {
 	if log.IsLevelEnabled(log.DebugLevel) {
 		log.WithFields(log.Fields{
 			"ranges":        ranges,
@@ -574,18 +582,21 @@ func matchPort(dir string, ranges []*proto.PortRange, namedPortSets []string, ip
 			return true
 		}
 	}
+	if len(namedPortSets) == 0 {
+		return false
+	}
+	key := namedPortKey()
 	for _, id := range namedPortSets {
-		portStr := fmt.Sprintf("%d", port)
-		if s := ipsSetFunc(id); s != nil && s.Contains(portStr) {
+		if s := ipsSetFunc(id); s != nil && s.Contains(key) {
 			return true
 		}
 	}
 	return false
 }
 
 // matchNotPort checks if the port is not within the port ranges and named port sets. It returns
-// true if the port matches, false otherwise.
-func matchNotPort(dir string, ranges []*proto.PortRange, namedPortSets []string, ipsSetFunc func(string) policystore.IPSet, port int) bool {
+// true if the port matches, false otherwise. See matchPort for how named port sets are keyed.
+func matchNotPort(dir string, ranges []*proto.PortRange, namedPortSets []string, ipsSetFunc func(string) policystore.IPSet, port int, namedPortKey func() string) bool {
 	if log.IsLevelEnabled(log.DebugLevel) {
 		log.WithFields(log.Fields{
 			"ranges":        ranges,
@@ -603,9 +614,12 @@ func matchNotPort(dir string, ranges []*proto.PortRange, namedPortSets []string,
 			return false
 		}
 	}
+	if len(namedPortSets) == 0 {
+		return true
+	}
+	key := namedPortKey()
 	for _, id := range namedPortSets {
-		portStr := fmt.Sprintf("%d", port)
-		if s := ipsSetFunc(id); s != nil && s.Contains(portStr) {
+		if s := ipsSetFunc(id); s != nil && s.Contains(key) {
 			return false
 		}
 	}

app-policy/checker/match_test.go

@@ -1106,6 +1106,161 @@ func TestMatchNets(t *testing.T) {
 	}
 }
 
+// TestMatchNamedPorts covers a rule that references a named port. Felix resolves the
+// name against the endpoints that declare it and sends an IP+port set whose members are
+// "<IP>,<protocol>:<port>", so the checker has to look that key up rather than the bare
+// port number. See https://github.com/projectcalico/calico/issues/13174.
+func TestMatchNamedPorts(t *testing.T) {
+	// A workload endpoint at 10.0.0.1 declaring a named port "http" on tcp/8080, as Felix
+	// resolves it into an IP+port set for a rule that says ports: [http].
+	store := policystore.NewPolicyStore()
+	httpSet := policystore.NewIPSet(proto.IPSetUpdate_IP_AND_PORT)
+	httpSet.AddString("10.0.0.1,tcp:8080")
+	store.IPSetByID["http"] = httpSet
+	// The same endpoint declaring a named port "diameter" on sctp/3868.
+	diameterSet := policystore.NewIPSet(proto.IPSetUpdate_IP_AND_PORT)
+	diameterSet.AddString("10.0.0.1,sctp:3868")
+	store.IPSetByID["diameter"] = diameterSet
+
+	testCases := []struct {
+		title    string
+		rule     *proto.Rule
+		srcIP    string
+		srcPort  int
+		dstIP    string
+		dstPort  int
+		protocol int
+		match    bool
+	}{
+		{
+			title:    "dst named port matches the endpoint that declares it",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  8080,
+			protocol: 6,
+			match:    true,
+		},
+		{
+			title:    "dst named port does not match another endpoint on the same port",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.2",
+			dstPort:  8080,
+			protocol: 6,
+			match:    false,
+		},
+		{
+			title:    "dst named port does not match a different port on the endpoint",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  9090,
+			protocol: 6,
+			match:    false,
+		},
+		{
+			title:    "dst named port does not match a different protocol",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  8080,
+			protocol: 17,
+			match:    false,
+		},
+		{
+			title:    "negated dst named port excludes the endpoint that declares it",
+			rule:     &proto.Rule{NotDstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  8080,
+			protocol: 6,
+			match:    false,
+		},
+		{
+			title:    "negated dst named port admits another endpoint on the same port",
+			rule:     &proto.Rule{NotDstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.2",
+			dstPort:  8080,
+			protocol: 6,
+			match:    true,
+		},
+		{
+			title:    "src named port matches on the source leg",
+			rule:     &proto.Rule{SrcNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.1",
+			srcPort:  8080,
+			dstIP:    "10.0.0.9",
+			dstPort:  33333,
+			protocol: 6,
+			match:    true,
+		},
+		{
+			title:    "src named port does not match the destination leg",
+			rule:     &proto.Rule{SrcNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  8080,
+			protocol: 6,
+			match:    false,
+		},
+		{
+			title:    "numeric port is ORed with the named port set",
+			rule:     &proto.Rule{DstPorts: []*proto.PortRange{{First: 9090, Last: 9090}}, DstNamedPortIpSetIds: []string{"http"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.2",
+			dstPort:  9090,
+			protocol: 6,
+			match:    true,
+		},
+		{
+			title:    "dst named port on sctp matches an sctp flow",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"diameter"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  3868,
+			protocol: 132,
+			match:    true,
+		},
+		{
+			title:    "dst named port on sctp does not match tcp to the same port",
+			rule:     &proto.Rule{DstNamedPortIpSetIds: []string{"diameter"}},
+			srcIP:    "10.0.0.9",
+			srcPort:  33333,
+			dstIP:    "10.0.0.1",
+			dstPort:  3868,
+			protocol: 6,
+			match:    false,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.title, func(t *testing.T) {
+			RegisterTestingT(t)
+
+			fl := &mocks.Flow{}
+			fl.On("GetSourceIP").Return(libnet.ParseIP(tc.srcIP).IP)
+			fl.On("GetDestIP").Return(libnet.ParseIP(tc.dstIP).IP)
+			fl.On("GetSourcePort").Return(tc.srcPort)
+			fl.On("GetDestPort").Return(tc.dstPort)
+			fl.On("GetProtocol").Return(tc.protocol)
+			req := &requestCache{Flow: fl, store: store}
+
+			Expect(matchSrcPort(tc.rule, req) && matchDstPort(tc.rule, req)).To(Equal(tc.match))
+		})
+	}
+}
+
 func TestMatchDstIPPortSetIds(t *testing.T) {
 	RegisterTestingT(t)
 
@@ -1197,6 +1352,26 @@ func TestMatchDstIPPortSetIds(t *testing.T) {
 			proto:    6,
 			expected: false,
 		},
+		{
+			title: "match IP in sctp set",
+			rule: &proto.Rule{
+				DstIpPortSetIds: []string{"setSCTP"},
+			},
+			destIP:   "192.168.1.8",
+			destPort: 3868,
+			proto:    132,
+			expected: true,
+		},
+		{
+			title: "no match IP in sctp set with tcp",
+			rule: &proto.Rule{
+				DstIpPortSetIds: []string{"setSCTP"},
+			},
+			destIP:   "192.168.1.8",
+			destPort: 3868,
+			proto:    6,
+			expected: false,
+		},
 	}
 
 	store := policystore.NewPolicyStore()
@@ -1212,7 +1387,10 @@ func TestMatchDstIPPortSetIds(t *testing.T) {
 	store.IPSetByID["set80"] = set80
 	store.IPSetByID["set443"] = set443
 	store.IPSetByID["setMulti"] = setMulti
+	setSCTP := policystore.NewIPSet(proto.IPSetUpdate_IP)
+	setSCTP.AddString("192.168.1.8,sctp:3868")
 	store.IPSetByID["setProto"] = setProto
+	store.IPSetByID["setSCTP"] = setSCTP
 
 	for _, tc := range testCases {
 		t.Run(tc.title, func(t *testing.T) {

app-policy/checker/requestcache.go

@@ -99,6 +99,25 @@ func (r *requestCache) getDstNamespace() *namespace {
 	return nil
 }
 
+// getSrcIPProtoPortStr returns the source "<IP>,<protocol>:<port>" key used for
+// IP+port set matching.
+func (r *requestCache) getSrcIPProtoPortStr() string {
+	return ipProtoPortKey(r.GetSourceIP().String(), r.GetProtocol(), r.GetSourcePort())
+}
+
+// getDstIPProtoPortStr returns the destination "<IP>,<protocol>:<port>" key used for
+// IP+port set matching.
+func (r *requestCache) getDstIPProtoPortStr() string {
+	return ipProtoPortKey(r.GetDestIP().String(), r.GetProtocol(), r.GetDestPort())
+}
+
+// ipProtoPortKey builds the member format Felix uses for IP+port IP sets, e.g.
+// "10.0.0.1,tcp:8080". An unnamed protocol leaves that field empty, which no
+// member can equal, so the lookup just misses.
+func ipProtoPortKey(ipStr string, protocol, port int) string {
+	return fmt.Sprintf("%s,%s:%d", ipStr, protocolMapL4[int32(protocol)], port)
+}
+
 // getIPSet returns the IPSet with the given ID.
 func (r *requestCache) getIPSet(id string) policystore.IPSet {
 	s, ok := r.store.IPSetByID[id]

app-policy/checker/requestcache_test.go

@@ -216,3 +216,16 @@ func TestNamespaceLabels(t *testing.T) {
 	Expect(uut.getDstNamespace().Name).To(Equal("sub"))
 	Expect(uut.getDstNamespace().Labels).To(Equal(map[string]string{"k7": "v7", "k8": "v8"}))
 }
+
+// ipProtoPortKey has to produce exactly the member format Felix emits for an IP+port
+// set, for each protocol a named port can be declared on. A protocol with no name
+// yields an empty field, which no member can equal, so the lookup misses.
+func TestIPProtoPortKey(t *testing.T) {
+	RegisterTestingT(t)
+
+	Expect(ipProtoPortKey("10.0.0.1", 6, 8080)).To(Equal("10.0.0.1,tcp:8080"))
+	Expect(ipProtoPortKey("10.0.0.1", 17, 53)).To(Equal("10.0.0.1,udp:53"))
+	Expect(ipProtoPortKey("10.0.0.1", 132, 3868)).To(Equal("10.0.0.1,sctp:3868"))
+	Expect(ipProtoPortKey("fd00::1", 6, 8080)).To(Equal("fd00::1,tcp:8080"))
+	Expect(ipProtoPortKey("10.0.0.1", 47, 8080)).To(Equal("10.0.0.1,:8080"))
+}