Restrict rule authorization to accept rules, the destination side for SSH, and the marker protocol's own port

2026-10-09 06:59:08 +02:00 · 2026-09-22 14:53:37 +02:00
parent 6c15fa1d84
commit 750d093300
6 changed files with 297 additions and 52 deletions
@@ -71,10 +71,17 @@ func ApplyResolvedRuleToState[P any](
 ) {
 	emitRuleDirections(rule, sourcePeers, destPeers, peerInSources, peerInDestinations, generateResources)

-	receivingPeer := peerInDestinations || (rule.Bidirectional && peerInSources)
+	// The firewall rule above carries the rule's own action, but authorization
+	// is only ever granted by an accept: a rule written to deny SSH or VNC must
+	// not contribute authorized users, and an action this code does not
+	// recognise as an accept grants nothing either.
+	if rule.Action != string(PolicyTrafficActionAccept) {
+		return
+	}
+
 	switch {
 	case rule.Protocol == string(PolicyRuleProtocolNetbirdSSH):
-		if !receivingPeer {
+		if !peerInDestinations {
 			return
 		}
 		state.SSHEnabled = true
@@ -82,7 +89,7 @@ func ApplyResolvedRuleToState[P any](
 	case rule.Protocol == string(PolicyRuleProtocolNetbirdVNC):
 		cb.handleVNCRule(rule, peerInSources, peerInDestinations, state)
 	case nmdata.PolicyRuleImpliesLegacySSH(rule) && targetPeerSSHEnabled:
-		if !receivingPeer {
+		if !peerInDestinations {
 			return
 		}
 		state.SSHEnabled = true
@@ -211,16 +218,43 @@ func VNCScopedPorts() []string {
 	return []string{strconv.Itoa(VNCInternalPort)}
 }

+// SSHScopedPorts returns the ports a netbird-ssh rule is scoped to when it
+// declares none of its own. It matches the port range ParseRuleString gives the
+// bare "netbird-ssh" spelling, so the two ways of writing the same rule produce
+// the same reach.
+func SSHScopedPorts() []string {
+	return []string{strconv.Itoa(nativeSSHPortNumber)}
+}
+
+// MarkerScopedPorts returns the ports the NetBird marker protocols imply, for a
+// rule that declares none of its own. ok is false for every other protocol,
+// which carries no implied port.
+func MarkerScopedPorts(protocol PolicyRuleProtocolType) ([]string, bool) {
+	switch protocol {
+	case PolicyRuleProtocolNetbirdVNC:
+		return VNCScopedPorts(), true
+	case PolicyRuleProtocolNetbirdSSH:
+		return SSHScopedPorts(), true
+	default:
+		return nil, false
+	}
+}
+
 // NormalizePolicyRuleProtocol maps a rule's protocol with
-// WirePolicyRuleProtocol and scopes a portless netbird-vnc rule to the
-// embedded VNC port. It returns the effective rule, which is a shallow copy
-// only when the ports had to be overridden.
+// WirePolicyRuleProtocol and scopes a portless marker-protocol rule to the port
+// that protocol implies, so it doesn't degrade into an unscoped TCP allow. It
+// returns the effective rule, which is a shallow copy only when the ports had
+// to be overridden.
 func NormalizePolicyRuleProtocol(rule *nmdata.PolicyRule) (*nmdata.PolicyRule, PolicyRuleProtocolType) {
 	protocol := WirePolicyRuleProtocol(PolicyRuleProtocolType(rule.Protocol))
-	if rule.Protocol != string(PolicyRuleProtocolNetbirdVNC) || len(rule.Ports) > 0 || len(rule.PortRanges) > 0 {
+	if len(rule.Ports) > 0 || len(rule.PortRanges) > 0 {
+		return rule, protocol
+	}
+	ports, ok := MarkerScopedPorts(PolicyRuleProtocolType(rule.Protocol))
+	if !ok {
 		return rule, protocol
 	}
 	scoped := *rule
-	scoped.Ports = VNCScopedPorts()
+	scoped.Ports = ports
 	return &scoped, protocol
 }
@@ -1,6 +1,9 @@
 package types

 import (
+	"fmt"
+	"slices"
+	"strconv"
 	"testing"

 	"github.com/netbirdio/netbird/shared/management/networkmap/nmdata"
@@ -82,13 +85,42 @@ func TestHandleVNCRule_DestinationAlwaysGetsPubkey(t *testing.T) {
 	}
 }

-// TestApplyResolvedRule_BidirectionalSSHEnablesSourcePeer locks the
-// bidirectional widening for netbird-ssh rules: a peer that appears only
-// in the rule's sources of a bidirectional SSH rule must get SSH enabled
-// and its authorized users collected, because the rule grants access in
-// both directions. A unidirectional rule must not do this for a
-// source-only peer.
-func TestApplyResolvedRule_BidirectionalSSHEnablesSourcePeer(t *testing.T) {
+// TestApplyResolvedRule_SSHSkipsSourcePeer locks SSH authorization to the
+// destination side. Unlike VNC, whose Noise_IK handshake needs the session
+// pubkey on both ends of a bidirectional rule, SSH authorization follows the
+// destination alone: a peer that appears only in a rule's sources must not get
+// SSH enabled or authorized users collected, bidirectional or not.
+func TestApplyResolvedRule_SSHSkipsSourcePeer(t *testing.T) {
+	for _, bidirectional := range []bool{true, false} {
+		t.Run(fmt.Sprintf("bidirectional=%t", bidirectional), func(t *testing.T) {
+			collected := false
+			cb := RuleAuthCallbacks{
+				CollectSSHUsers: func(_ *nmdata.PolicyRule, _ map[string]map[string]struct{}) {
+					collected = true
+				},
+			}
+			rule := &nmdata.PolicyRule{
+				Protocol:      string(PolicyRuleProtocolNetbirdSSH),
+				Bidirectional: bidirectional,
+			}
+			state := NewPeerConnResolveState()
+
+			ApplyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, false, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)
+
+			if state.SSHEnabled {
+				t.Fatal("expected SSH NOT enabled on source-only peer of SSH rule")
+			}
+			if collected {
+				t.Fatal("expected NO authorized users collected on source-only peer of SSH rule")
+			}
+		})
+	}
+}
+
+// TestApplyResolvedRule_SSHEnablesDestinationPeer is the positive counterpart:
+// a peer in the rule's destinations gets SSH enabled and its authorized users
+// collected.
+func TestApplyResolvedRule_SSHEnablesDestinationPeer(t *testing.T) {
 	collected := false
 	cb := RuleAuthCallbacks{
 		CollectSSHUsers: func(_ *nmdata.PolicyRule, target map[string]map[string]struct{}) {
@@ -98,27 +130,103 @@ func TestApplyResolvedRule_BidirectionalSSHEnablesSourcePeer(t *testing.T) {
 	}
 	rule := &nmdata.PolicyRule{
 		Protocol:      string(PolicyRuleProtocolNetbirdSSH),
+		Action:        string(PolicyTrafficActionAccept),
 		Bidirectional: true,
 	}
 	state := NewPeerConnResolveState()

-	ApplyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, false, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)
+	ApplyResolvedRuleToState(rule, nil, nil, false /*peerInSources*/, true /*peerInDestinations*/, false, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)

 	if !state.SSHEnabled {
-		t.Fatal("expected SSH enabled on source-side peer of bidirectional SSH rule")
+		t.Fatal("expected SSH enabled on destination-side peer of SSH rule")
 	}
 	if !collected {
-		t.Fatal("expected authorized users collected on source-side peer of bidirectional SSH rule")
+		t.Fatal("expected authorized users collected on destination-side peer of SSH rule")
 	}
 	if _, ok := state.AuthorizedUsers["local"]; !ok {
-		t.Fatal("expected authorized users map populated for source-side peer")
+		t.Fatal("expected authorized users map populated for destination-side peer")
 	}
 }

-// TestApplyResolvedRule_UnidirectionalSSHSkipsSourcePeer is the negative
-// counterpart: a unidirectional SSH rule must not enable SSH for a peer
-// that appears only in sources.
-func TestApplyResolvedRule_UnidirectionalSSHSkipsSourcePeer(t *testing.T) {
+// TestApplyResolvedRule_LegacySSHSkipsSourcePeer covers the same boundary for
+// the legacy TCP/22 path, which enables SSH off the peer's own flag.
+func TestApplyResolvedRule_LegacySSHSkipsSourcePeer(t *testing.T) {
+	rule := &nmdata.PolicyRule{
+		Protocol:      string(PolicyRuleProtocolTCP),
+		Ports:         []string{"22"},
+		Bidirectional: true,
+	}
+	cb := RuleAuthCallbacks{
+		GetAllowedUserIDs: func() map[string]struct{} {
+			return map[string]struct{}{"user1": {}}
+		},
+	}
+	state := NewPeerConnResolveState()
+
+	ApplyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, true /*targetPeerSSHEnabled*/, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)
+
+	if state.SSHEnabled {
+		t.Fatal("expected SSH NOT enabled on source-only peer of legacy SSH rule")
+	}
+	if len(state.AuthorizedUsers) != 0 {
+		t.Fatalf("expected no authorized users for source-only peer, got %v", state.AuthorizedUsers)
+	}
+}
+
+// TestApplyResolvedRule_DropRuleGrantsNoAuthorization covers a rule written to
+// deny: the firewall rule it emits carries the drop, but the authorization
+// switch must not read it as a grant. Without this, a netbird-ssh rule with
+// action=drop and no authorized groups falls to the default branch and hands
+// the account's whole allowed-user set to the wildcard key.
+func TestApplyResolvedRule_DropRuleGrantsNoAuthorization(t *testing.T) {
+	for _, protocol := range []PolicyRuleProtocolType{
+		PolicyRuleProtocolNetbirdSSH,
+		PolicyRuleProtocolNetbirdVNC,
+	} {
+		t.Run(string(protocol), func(t *testing.T) {
+			collectedSSH, collectedVNC := false, false
+			cb := RuleAuthCallbacks{
+				CollectSSHUsers: func(_ *nmdata.PolicyRule, _ map[string]map[string]struct{}) {
+					collectedSSH = true
+				},
+				CollectVNCUsers: func(_ *nmdata.PolicyRule, _ map[string]map[string]struct{}) {
+					collectedVNC = true
+				},
+				GetAllowedUserIDs: func() map[string]struct{} {
+					return map[string]struct{}{"user1": {}}
+				},
+			}
+			rule := &nmdata.PolicyRule{
+				Protocol:       string(protocol),
+				Action:         string(PolicyTrafficActionDrop),
+				SessionPubKey:  "pubkey",
+				AuthorizedUser: "user1",
+			}
+			state := NewPeerConnResolveState()
+
+			emitted := 0
+			ApplyResolvedRuleToState(rule, nil, nil, false, true /*peerInDestinations*/, true,
+				func(*nmdata.PolicyRule, []*nmdata.Peer, int) { emitted++ }, cb, state)
+
+			if emitted == 0 {
+				t.Fatal("expected the drop rule to still be emitted to the firewall")
+			}
+			if state.SSHEnabled {
+				t.Fatal("expected SSH NOT enabled by a drop rule")
+			}
+			if collectedSSH || collectedVNC {
+				t.Fatal("expected no authorized users collected from a drop rule")
+			}
+			if len(state.VNCSessionPubKeys) != 0 {
+				t.Fatal("expected no VNC session pubkeys distributed by a drop rule")
+			}
+		})
+	}
+}
+
+// TestApplyResolvedRule_UnknownActionGrantsNoAuthorization: an action this code
+// does not recognise as an accept must not grant either.
+func TestApplyResolvedRule_UnknownActionGrantsNoAuthorization(t *testing.T) {
 	collected := false
 	cb := RuleAuthCallbacks{
 		CollectSSHUsers: func(_ *nmdata.PolicyRule, _ map[string]map[string]struct{}) {
@@ -126,17 +234,63 @@ func TestApplyResolvedRule_UnidirectionalSSHSkipsSourcePeer(t *testing.T) {
 		},
 	}
 	rule := &nmdata.PolicyRule{
-		Protocol:      string(PolicyRuleProtocolNetbirdSSH),
-		Bidirectional: false,
+		Protocol: string(PolicyRuleProtocolNetbirdSSH),
+		Action:   "quarantine",
 	}
 	state := NewPeerConnResolveState()

-	ApplyResolvedRuleToState(rule, nil, nil, true /*peerInSources*/, false /*peerInDestinations*/, false, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)
+	ApplyResolvedRuleToState(rule, nil, nil, false, true, false, func(*nmdata.PolicyRule, []*nmdata.Peer, int) {}, cb, state)

-	if state.SSHEnabled {
-		t.Fatal("expected SSH NOT enabled on source-only peer of unidirectional SSH rule")
+	if state.SSHEnabled || collected {
+		t.Fatal("expected an unrecognized action to grant nothing")
 	}
-	if collected {
-		t.Fatal("expected NO authorized users collected on source-only peer of unidirectional SSH rule")
+}
+
+// TestNormalizePolicyRuleProtocol_PortlessMarkerRulesAreScoped: a marker-protocol
+// rule with no ports of its own must be scoped to the port that protocol
+// implies. A portless netbird-ssh rule otherwise generates a bare tcp firewall
+// rule, which opens every TCP port on the destination.
+func TestNormalizePolicyRuleProtocol_PortlessMarkerRulesAreScoped(t *testing.T) {
+	cases := []struct {
+		protocol  PolicyRuleProtocolType
+		wantPorts []string
+	}{
+		{PolicyRuleProtocolNetbirdSSH, []string{"22022"}},
+		{PolicyRuleProtocolNetbirdVNC, []string{strconv.Itoa(VNCInternalPort)}},
+	}
+	for _, tc := range cases {
+		t.Run(string(tc.protocol), func(t *testing.T) {
+			rule := &nmdata.PolicyRule{Protocol: string(tc.protocol)}
+			effective, protocol := NormalizePolicyRuleProtocol(rule)
+
+			if protocol != PolicyRuleProtocolTCP {
+				t.Fatalf("expected wire protocol tcp, got %s", protocol)
+			}
+			if !slices.Equal(effective.Ports, tc.wantPorts) {
+				t.Fatalf("expected ports %v, got %v", tc.wantPorts, effective.Ports)
+			}
+			if len(rule.Ports) != 0 {
+				t.Fatal("expected the caller's rule to be left untouched")
+			}
+		})
+	}
+}
+
+// A marker rule that declares its own ports keeps them, and a plain protocol is
+// never given ports it did not ask for.
+func TestNormalizePolicyRuleProtocol_LeavesOtherRulesAlone(t *testing.T) {
+	explicit := &nmdata.PolicyRule{Protocol: string(PolicyRuleProtocolNetbirdSSH), Ports: []string{"2222"}}
+	effective, _ := NormalizePolicyRuleProtocol(explicit)
+	if effective != explicit {
+		t.Fatal("expected a rule with its own ports to be returned as-is")
+	}
+
+	portless := &nmdata.PolicyRule{Protocol: string(PolicyRuleProtocolTCP)}
+	effective, protocol := NormalizePolicyRuleProtocol(portless)
+	if effective != portless || len(effective.Ports) != 0 {
+		t.Fatal("expected a portless tcp rule to be left unscoped")
+	}
+	if protocol != PolicyRuleProtocolTCP {
+		t.Fatalf("expected tcp, got %s", protocol)
 	}
 }