From 750d093300599c5f79a2e343f9998310b64b9de5 Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 22 Sep 2026 14:53:37 +0200 Subject: [PATCH] Restrict rule authorization to accept rules, the destination side for SSH, and the marker protocol's own port --- management/server/types/aliases.go | 10 +- .../networkmap_components_correctness_test.go | 35 ++++ .../networkmap/networkmapcompute.go | 41 ++-- .../networkmap/networkmapcompute_test.go | 15 +- .../types/policy_authorized_users.go | 50 ++++- .../policy_authorized_users_security_test.go | 198 ++++++++++++++++-- 6 files changed, 297 insertions(+), 52 deletions(-) diff --git a/management/server/types/aliases.go b/management/server/types/aliases.go index c82a0363d..01ba7e740 100644 --- a/management/server/types/aliases.go +++ b/management/server/types/aliases.go @@ -81,14 +81,18 @@ func MergeWildcardUsers(dst map[string]map[string]struct{}, users map[string]str // NormalizePolicyRuleProtocol is the real-typed sibling of the twin helper in // shared types: it maps the NetBird virtual protocols to the wire protocol and -// scopes a portless netbird-vnc rule to the embedded VNC port. +// scopes a portless marker-protocol rule to the port that protocol implies. func NormalizePolicyRuleProtocol(rule *PolicyRule) (*PolicyRule, PolicyRuleProtocolType) { protocol := sharedtypes.WirePolicyRuleProtocol(rule.Protocol) - if rule.Protocol != PolicyRuleProtocolNetbirdVNC || len(rule.Ports) > 0 || len(rule.PortRanges) > 0 { + if len(rule.Ports) > 0 || len(rule.PortRanges) > 0 { + return rule, protocol + } + ports, ok := sharedtypes.MarkerScopedPorts(rule.Protocol) + if !ok { return rule, protocol } scoped := *rule - scoped.Ports = sharedtypes.VNCScopedPorts() + scoped.Ports = ports return &scoped, protocol } diff --git a/management/server/types/networkmap_components_correctness_test.go b/management/server/types/networkmap_components_correctness_test.go index 35b5f7149..098bcb3cb 100644 --- a/management/server/types/networkmap_components_correctness_test.go +++ b/management/server/types/networkmap_components_correctness_test.go @@ -1412,3 +1412,38 @@ func TestPeerSSHEnabledFromPolicies_MatchesMap_Sweep(t *testing.T) { assertSSHEquivalence(t, account, peerID, validatedPeers) } } + +// The bidirectional shape is the one the three SSH deciders can disagree on: +// Calculate walks ApplyResolvedRuleToState, the components sync path walks +// ruleEnablesSSHForPeer, and the login/register path walks +// PeerSSHEnabledFromPolicies. Every other case in this file sets +// Bidirectional:false or omits it, so none of them exercise the source side. +func TestPeerSSHEnabledFromPolicies_MatchesMap_BidirectionalNetbirdSSH(t *testing.T) { + account, validatedPeers := scalableTestAccountWithoutDefaultPolicy(20, 2) + account.Groups["ssh-users"] = &types.Group{ID: "ssh-users", Name: "SSH Users", Peers: []string{}} + account.Policies = append(account.Policies, &types.Policy{ + ID: "policy-ssh-bidi", Name: "SSH Access", Enabled: true, AccountID: "test-account", + Rules: []*types.PolicyRule{{ + ID: "rule-ssh-bidi", Name: "Allow SSH", Enabled: true, + Action: types.PolicyTrafficActionAccept, Protocol: types.PolicyRuleProtocolNetbirdSSH, + Bidirectional: true, + Sources: []string{"group-0"}, Destinations: []string{"group-1"}, + AuthorizedGroups: map[string][]string{"ssh-users": {"root"}}, + }}, + }) + + // peer-0 is source-only, peer-10 is destination-only. + assertSSHEquivalence(t, account, "peer-0", validatedPeers) + assertSSHEquivalence(t, account, "peer-10", validatedPeers) + + sourceOnly := componentsNetworkMap(account, "peer-0", validatedPeers) + require.NotNil(t, sourceOnly) + assert.False(t, sourceOnly.EnableSSH, + "a bidirectional netbird-ssh rule must not enable SSH on its source-side peer") + assert.Empty(t, sourceOnly.AuthorizedUsers, + "a source-side peer must carry no authorized users") + + destination := componentsNetworkMap(account, "peer-10", validatedPeers) + require.NotNil(t, destination) + assert.True(t, destination.EnableSSH, "the destination-side peer must have SSH enabled") +} diff --git a/shared/management/networkmap/networkmapcompute.go b/shared/management/networkmap/networkmapcompute.go index 11c6f3d70..e9d2d8448 100644 --- a/shared/management/networkmap/networkmapcompute.go +++ b/shared/management/networkmap/networkmapcompute.go @@ -16,18 +16,36 @@ type authRequirements struct { needAllowedUserIDs bool } -// collectFor records what rule needs to resolve its authorized users, for a -// peer the resolver will authorize under it. +// collectFor records what rule needs to resolve its authorized users, for the +// peer named by peerInSources/peerInDestinations. It collects nothing for a +// peer the resolver will not authorize under the rule, so the components do not +// carry the account's user list to a peer that has no use for it. +// +// The sides mirror ApplyResolvedRuleToState: SSH authorization follows the +// destination alone, while a bidirectional VNC rule also authorizes its source +// side, because the Noise_IK handshake needs the session key on both ends. A +// rule that is not an accept authorizes nobody on either side. // // Both marker protocols resolve users the same way, so a VNC rule needs exactly // what an SSH rule needs: the group-to-user mapping when the rule names groups, // the account's allowed-user set when it names nobody, and nothing at all when // it carries its own user. -func (a *authRequirements) collectFor(rule *nmdata.PolicyRule, peerSSHEnabled bool) { - isMarkerRule := rule.Protocol == string(types.PolicyRuleProtocolNetbirdSSH) || - rule.Protocol == string(types.PolicyRuleProtocolNetbirdVNC) - if !isMarkerRule { - if nmdata.PolicyRuleImpliesLegacySSH(rule) && peerSSHEnabled { +func (a *authRequirements) collectFor(rule *nmdata.PolicyRule, peerSSHEnabled, peerInSources, peerInDestinations bool) { + if rule.Action != string(types.PolicyTrafficActionAccept) { + return + } + + switch rule.Protocol { + case string(types.PolicyRuleProtocolNetbirdVNC): + if !peerInDestinations && !(rule.Bidirectional && peerInSources) { + return + } + case string(types.PolicyRuleProtocolNetbirdSSH): + if !peerInDestinations { + return + } + default: + if peerInDestinations && nmdata.PolicyRuleImpliesLegacySSH(rule) && peerSSHEnabled { a.needAllowedUserIDs = true } return @@ -393,14 +411,7 @@ func (nmd *NetworkMapData) getPeersGroupsPoliciesRoutes( } - // Collected for whichever side the resolver will actually authorize: - // a bidirectional rule grants access in both directions, so a peer - // that appears only in Sources is authorized too. Gating this on - // peerInDestinations alone leaves that peer's rule reaching the - // resolver with none of the inputs it needs to name a user. - if peerInDestinations || (rule.Bidirectional && peerInSources) { - authReqs.collectFor(rule, peerSSHEnabled) - } + authReqs.collectFor(rule, peerSSHEnabled, peerInSources, peerInDestinations) } if policyRelevant { relevantPolicies = append(relevantPolicies, policy) diff --git a/shared/management/networkmap/networkmapcompute_test.go b/shared/management/networkmap/networkmapcompute_test.go index 85eeaa8d2..51cb20200 100644 --- a/shared/management/networkmap/networkmapcompute_test.go +++ b/shared/management/networkmap/networkmapcompute_test.go @@ -961,15 +961,14 @@ func TestGetPeerNetworkMapComponents_SSHRequirements(t *testing.T) { mutateRule: func(r *nmdata.PolicyRule) { r.Ports = []string{"443"} }, sshEnabled: true, }, - // A bidirectional rule grants access both ways, so the peer is - // authorized from the sources side too and needs the same inputs. + // SSH authorization follows the destination side, so a source-side peer + // needs no lookup inputs either way round. { - name: "netbird-ssh on the source side of a bidirectional rule", + name: "netbird-ssh only counts on the destination side", mutateRule: func(r *nmdata.PolicyRule) { r.Protocol = string(nbtypes.PolicyRuleProtocolNetbirdSSH) }, targetInSrc: true, - wantAllowed: true, }, { name: "netbird-ssh on the source side of a one-way rule", @@ -979,6 +978,14 @@ func TestGetPeerNetworkMapComponents_SSHRequirements(t *testing.T) { }, targetInSrc: true, }, + // A drop rule authorizes nobody, so it needs no lookup inputs. + { + name: "netbird-ssh drop rule needs no inputs", + mutateRule: func(r *nmdata.PolicyRule) { + r.Protocol = string(nbtypes.PolicyRuleProtocolNetbirdSSH) + r.Action = string(nbtypes.PolicyTrafficActionDrop) + }, + }, // VNC resolves authorized users exactly the way SSH does, so it needs // the same inputs carried into the components. Leaving it out strips diff --git a/shared/management/types/policy_authorized_users.go b/shared/management/types/policy_authorized_users.go index 423287aee..7a43a55d8 100644 --- a/shared/management/types/policy_authorized_users.go +++ b/shared/management/types/policy_authorized_users.go @@ -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 } diff --git a/shared/management/types/policy_authorized_users_security_test.go b/shared/management/types/policy_authorized_users_security_test.go index 0a8944c6a..166567547 100644 --- a/shared/management/types/policy_authorized_users_security_test.go +++ b/shared/management/types/policy_authorized_users_security_test.go @@ -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) } }