Drop network map rules with an unrecognized protocol instead of decoding them as all

shared/management/networkmap/decode.go

@@ -134,6 +134,11 @@ func DecodeEnvelope(ctx context.Context, env *proto.NetworkMapEnvelope) (*types.
 			return nil, fmt.Errorf("invalid envelope: policies[%d] is nil", i)
 		}
 		policy := decodePolicyCompact(pc, pc.Id, peerIDByIndex)
+		if policy == nil {
+			log.Warnf("envelope: policies[%d] (%s) uses unsupported protocol %s, skipping; upgrade the client to enforce it",
+				i, pc.Id, pc.Protocol)
+			continue
+		}
 		c.Policies = append(c.Policies, policy)
 		policyByID[pc.Id] = policy
 	}
@@ -375,13 +380,21 @@ func decodePeerCompact(pc *proto.PeerCompact, peerID string) *nmdata.Peer {
 	return peer
 }
 
+// decodePolicyCompact rebuilds a policy from its wire form. It returns nil when
+// the rule names a protocol this build does not know, which a newer management
+// can ship; the policy is then not enforced at all rather than enforced as
+// something wider than it was written to be.
 func decodePolicyCompact(pc *proto.PolicyCompact, policyID string, peerIDByIndex []string) *nmdata.Policy {
+	protocol, ok := protocolFromProto(pc.Protocol)
+	if !ok {
+		return nil
+	}
 	rule := &nmdata.PolicyRule{
 		ID:                  policyID, // 1 rule per policy → reuse synthesized id
 		PolicyID:            policyID,
 		Enabled:             true,
 		Action:              string(actionFromProto(pc.Action)),
-		Protocol:            string(protocolFromProto(pc.Protocol)),
+		Protocol:            string(protocol),
 		Bidirectional:       pc.Bidirectional,
 		Ports:               uint32SliceToStrings(pc.Ports),
 		PortRanges:          portRangesFromProto(pc.PortRanges),
@@ -565,29 +578,36 @@ func portRangesFromProto(ranges []*proto.PortInfo_Range) []nmdata.RulePortRange
 	return out
 }
 
+// actionFromProto maps a wire action. An action this build does not recognise
+// denies: a firewall decision must not default to letting traffic through.
 func actionFromProto(a proto.RuleAction) types.PolicyTrafficActionType {
-	if a == proto.RuleAction_DROP {
-		return types.PolicyTrafficActionDrop
+	if a == proto.RuleAction_ACCEPT {
+		return types.PolicyTrafficActionAccept
 	}
-	return types.PolicyTrafficActionAccept
+	return types.PolicyTrafficActionDrop
 }
 
-func protocolFromProto(p proto.RuleProtocol) types.PolicyRuleProtocolType {
+// protocolFromProto maps a wire protocol. ok is false for a value this build
+// does not recognise, and the caller must then discard the rule rather than
+// substitute a protocol: substituting ALL widens the rule to every IP protocol
+// and, because an ALL match short-circuits the port comparison, past the
+// rule's own port restriction.
+func protocolFromProto(p proto.RuleProtocol) (types.PolicyRuleProtocolType, bool) {
 	switch p {
 	case proto.RuleProtocol_TCP:
-		return types.PolicyRuleProtocolTCP
+		return types.PolicyRuleProtocolTCP, true
 	case proto.RuleProtocol_UDP:
-		return types.PolicyRuleProtocolUDP
+		return types.PolicyRuleProtocolUDP, true
 	case proto.RuleProtocol_ICMP:
-		return types.PolicyRuleProtocolICMP
+		return types.PolicyRuleProtocolICMP, true
 	case proto.RuleProtocol_ALL:
-		return types.PolicyRuleProtocolALL
+		return types.PolicyRuleProtocolALL, true
 	case proto.RuleProtocol_NETBIRD_SSH:
-		return types.PolicyRuleProtocolNetbirdSSH
+		return types.PolicyRuleProtocolNetbirdSSH, true
 	case proto.RuleProtocol_NETBIRD_VNC:
-		return types.PolicyRuleProtocolNetbirdVNC
+		return types.PolicyRuleProtocolNetbirdVNC, true
 	default:
-		return types.PolicyRuleProtocolALL
+		return "", false
 	}
 }
 

shared/management/networkmap/decode_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/netbirdio/netbird/shared/management/networkmap/nmdata"
 	"github.com/netbirdio/netbird/shared/management/proto"
+	"github.com/netbirdio/netbird/shared/management/types"
 )
 
 func TestDecodePolicy(t *testing.T) {
@@ -59,3 +60,53 @@ func TestResourceCompactLegacyWireFormat(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, legacy, encoded)
 }
+
+// A management newer than this client can ship a protocol value this build has
+// no case for. Mapping it to ALL would both widen the rule to every IP protocol
+// and, because an ALL match short-circuits the port comparison, discard the
+// port restriction the rule was written with. The rule must be dropped instead.
+func TestDecodePolicyCompact_UnknownProtocolIsDropped(t *testing.T) {
+	const futureProtocol = proto.RuleProtocol(99)
+
+	pc := &proto.PolicyCompact{
+		Id:       "policy-1",
+		Action:   proto.RuleAction_ACCEPT,
+		Protocol: futureProtocol,
+		PortRanges: []*proto.PortInfo_Range{
+			{Start: 25900, End: 25900},
+		},
+		SourceGroupIds:      []string{"g-src"},
+		DestinationGroupIds: []string{"g-dst"},
+	}
+
+	assert.Nil(t, decodePolicyCompact(pc, pc.Id, nil),
+		"a rule with an unrecognized protocol must not decode into an enforceable rule")
+
+	_, ok := protocolFromProto(futureProtocol)
+	assert.False(t, ok, "an unrecognized protocol must not resolve to a known one")
+}
+
+// Every protocol the encoder can emit must survive a round trip, so the
+// drop-on-unknown rule above cannot quietly start discarding valid policies.
+func TestProtocolFromProto_KnownValuesRoundTrip(t *testing.T) {
+	known := []proto.RuleProtocol{
+		proto.RuleProtocol_ALL,
+		proto.RuleProtocol_TCP,
+		proto.RuleProtocol_UDP,
+		proto.RuleProtocol_ICMP,
+		proto.RuleProtocol_NETBIRD_SSH,
+		proto.RuleProtocol_NETBIRD_VNC,
+	}
+	for _, p := range known {
+		decoded, ok := protocolFromProto(p)
+		require.Truef(t, ok, "protocol %s must decode", p)
+		assert.Equalf(t, p, GetProtoProtocol(string(decoded)), "protocol %s must round trip", p)
+	}
+}
+
+// An action the switch does not recognize must deny, not accept.
+func TestActionFromProto_UnknownDenies(t *testing.T) {
+	assert.Equal(t, types.PolicyTrafficActionAccept, actionFromProto(proto.RuleAction_ACCEPT))
+	assert.Equal(t, types.PolicyTrafficActionDrop, actionFromProto(proto.RuleAction_DROP))
+	assert.Equal(t, types.PolicyTrafficActionDrop, actionFromProto(proto.RuleAction(99)))
+}