diff --git a/shared/management/networkmap/decode.go b/shared/management/networkmap/decode.go index d4b6649f0..1d03e2aba 100644 --- a/shared/management/networkmap/decode.go +++ b/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 } } diff --git a/shared/management/networkmap/decode_test.go b/shared/management/networkmap/decode_test.go index 7e2f17c60..e2f8775d7 100644 --- a/shared/management/networkmap/decode_test.go +++ b/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))) +}