[management] Add native ssh port rule on 22 #4810
Conversation
we check for ssh enabled on management and client before adding an additional rule
Refactored `addNativeSSHRule` to return the expanded slice for better testability. Added comprehensive unit tests to validate SSH rule expansion behavior under various conditions.
WalkthroughImplements feature-aware firewall rule expansion: derives peer-supported features (native SSH, portRanges) from peer version, prefers explicit Ports over PortRanges when expanding, conditionally appends a native SSH (22022) rule when policy and peer support allow, and adds helpers plus tests for SSH expansion behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Expander as expandPortsAndRanges
participant FeatureDetector as peerSupportedFirewallFeatures
participant PortExpander as Port/PortRange Expansion
participant SSHHelper as addNativeSSHRule
Caller->>Expander: expandPortsAndRanges(rule, peer)
Expander->>FeatureDetector: peerSupportedFirewallFeatures(peer.Version)
FeatureDetector-->>Expander: supportedFeatures{nativeSSH, portRanges}
alt rule has explicit Ports
Expander->>PortExpander: expand explicit Ports (always)
else Ports empty and portRanges supported
Expander->>PortExpander: expand PortRanges (use ranges)
else Ports empty and portRanges unsupported
Expander->>PortExpander: convert PortRanges to single-port rules when possible
end
PortExpander-->>Expander: expanded rules list
alt nativeSSH supported AND shouldCheckRulesForNativeSSH == true
Expander->>SSHHelper: addNativeSSHRule(baseRule, expanded)
SSHHelper-->>Expander: expanded (+22022 if not duplicate)
end
Expander-->>Caller: final expanded rules
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
mlsmaycon
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/server/types/account_test.go (1)
842-1021: SSH native port expansion tests are comprehensive and aligned with behaviorThe table covers all key branches: version gating (old vs 0.60.0 vs
-dev), SSHEnabled andServerSSHAllowedflags, TCP vs UDP, presence/absence of port 22 (including within ranges), and duplication avoidance for 22022. The way you deriveportsfromresultmatches howexpandPortsAndRangesconstructs rules, so this should be robust.If you want to reduce duplication later, you could factor tiny helpers for constructing the common
peer/baseFirewallRule, but that’s purely cosmetic.management/server/types/account.go (1)
41-50: Use named fields insupportedFeaturesstruct literal for clarity and maintainabilityThe positional literal
supportedFeatures{true, true}at line 1726 should use named fields to be more self-documenting and future-proof:if strings.Contains(peerVer, "dev") { return supportedFeatures{ nativeSSH: true, portRanges: true, } }Verified that the single call site at line 1660 correctly expects the
supportedFeaturesreturn type with no issues.Regarding the function name:
peerSupportsPortRangesdoes return a broader struct now (port ranges + native SSH), so consider renaming to something likepeerFirewallFeatureson your next refactor—optional.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/types/account.go(3 hunks)management/server/types/account_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
management/server/types/account_test.go (1)
management/server/types/policy.go (2)
PolicyRuleProtocolTCP(21-21)PolicyRuleProtocolUDP(23-23)
management/server/types/account.go (5)
shared/management/proto/management.pb.go (6)
FirewallRule(2958-2971)FirewallRule(2986-2986)FirewallRule(3001-3003)Flags(796-816)Flags(831-831)Flags(846-848)management/server/types/firewall_rule.go (1)
FirewallRule(22-43)management/server/types/policyrule.go (1)
PolicyRule(41-83)management/server/peer/peer.go (2)
Peer(16-58)Flags(99-112)management/server/posture/nb_version.go (1)
MeetsMinVersion(57-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: Client / Unit
- GitHub Check: Build Cache
- GitHub Check: release_ui_darwin
- GitHub Check: Android / Build
- GitHub Check: release
- GitHub Check: iOS / Build
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: JS / Lint
🔇 Additional comments (1)
management/server/types/account.go (1)
1659-1721: Native SSH expansion logic is coherent and well‑scopedUsing
peerSupportsPortRanges(peer.Meta.WtVersion)plusshouldCheckRulesForNativeSSHto gate native 22022 rules keeps the behavior constrained to:
- Peers that actually support the feature (version ≥ 0.60.0 or
*-dev).- Peers with SSH enabled and
Meta.Flags.ServerSSHAllowed.- TCP rules only.
- Rules that already include port 22, either explicitly or via ranges.
addNativeSSHRuleandisPortInRulecorrectly handle both single ports and ranges, skip adding 22022 if it’s already present, and avoid affecting non‑SSH or non‑TCP rules. The “preferPortsoverPortRanges” behavior is preserved via the early break whenlen(rule.Ports) > 0.No functional issues stand out here. Nice incremental extension of the existing expansion behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
management/server/types/account.go (2)
1715-1717: Consider clarifying zero-value PortRange handling.The function correctly checks both exact port matches and port ranges. However, when
PortRangeis unset (zero values), the range checkStart <= port && port <= Endevaluates as0 <= port && port <= 0, which only matches port 0.While this is technically correct, consider adding a guard to make the intent explicit:
func isPortInRule(portString string, portInt uint16, rule *FirewallRule) bool { - return rule.Port == portString || (rule.PortRange.Start <= portInt && portInt <= rule.PortRange.End) + return rule.Port == portString || + (rule.PortRange.End > 0 && rule.PortRange.Start <= portInt && portInt <= rule.PortRange.End) }
1723-1742: Consider explicit version checks for long-term maintainability.Lines 1734-1736 assume that native SSH support (>= 0.60.0) implies port ranges support without checking, relying on the fact that 0.60.0 > 0.48.0. While correct today, this creates a fragile dependency between version constants.
Consider checking both versions independently for clarity:
func peerSupportedFirewallFeatures(peerVer string) supportedFeatures { if strings.Contains(peerVer, "dev") { return supportedFeatures{true, true} } var features supportedFeatures meetMinVer, err := posture.MeetsMinVersion(firewallRuleMinNativeSSHVer, peerVer) features.nativeSSH = err == nil && meetMinVer - if features.nativeSSH { - features.portRanges = true - } else { - meetMinVer, err = posture.MeetsMinVersion(firewallRuleMinPortRangesVer, peerVer) - features.portRanges = err == nil && meetMinVer - } + meetMinVer, err = posture.MeetsMinVersion(firewallRuleMinPortRangesVer, peerVer) + features.portRanges = err == nil && meetMinVer return features }This removes the implicit assumption while maintaining correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/types/account.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/types/account.go (4)
management/server/types/firewall_rule.go (1)
FirewallRule(22-43)shared/management/http/api/types.gen.go (2)
PolicyRule(1345-1380)Peer(1021-1105)management/server/peer/peer.go (2)
Peer(16-58)Flags(99-112)management/server/posture/nb_version.go (1)
MeetsMinVersion(57-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (26)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Windows
🔇 Additional comments (5)
management/server/types/account.go (5)
43-44: LGTM! Version constant follows existing conventions.The new version constant is well-documented and follows the same pattern as the existing
firewallRuleMinPortRangesVer.
47-50: LGTM! Clean feature tracking abstraction.The
supportedFeaturesstruct provides a clean way to track multiple peer capabilities together.
1696-1713: LGTM! Native SSH rule addition logic is sound.The function correctly:
- Avoids duplicating port 22022 if already present
- Only adds 22022 when port 22 exists
- Preserves the base firewall rule configuration
1719-1721: LGTM! Comprehensive condition checking for native SSH.The function properly validates all necessary conditions before allowing native SSH rules to be added: feature support, peer SSH enabled, server permission, and TCP protocol.
1664-1687: Verify the breaking change: Ports now completely skip PortRanges.Lines 1671-1674 confirm a breaking change where
rule.PortRangesis completely ignored whenrule.Portsis non-empty (viabreak). The inline comment "prefer PolicyRule.Ports" indicates this is intentional.However, test coverage only validates scenarios with either Ports OR PortRanges, never both. Confirm this behavior is correct and consider adding a test case where both fields are populated to document the intended precedence.
Enhanced coverage by adding tests for native SSH rule expansion under various scenarios, including handling duplicate ports, port ranges, and version-specific behaviors.
Replaced hardcoded port values with constants for better maintainability and readability. Added documentation for native SSH logic and validation.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
management/server/types/account.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
management/server/types/account.go (6)
shared/management/proto/management.pb.go (6)
FirewallRule(2958-2971)FirewallRule(2986-2986)FirewallRule(3001-3003)Flags(796-816)Flags(831-831)Flags(846-848)management/server/types/firewall_rule.go (1)
FirewallRule(22-43)management/server/types/policyrule.go (1)
PolicyRule(41-83)management/server/peer/peer.go (2)
Peer(16-58)Flags(99-112)management/server/types/policy.go (1)
PolicyRuleProtocolTCP(21-21)management/server/posture/nb_version.go (1)
MeetsMinVersion(57-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Management / Integration (amd64, postgres)
- GitHub Check: Management / Benchmark (API) (amd64, postgres)
- GitHub Check: Management / Integration (amd64, sqlite)
- GitHub Check: Management / Benchmark (API) (amd64, sqlite)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Management / Benchmark (amd64, postgres)
- GitHub Check: Management / Benchmark (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Linux
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: release
- GitHub Check: Client / Unit
🔇 Additional comments (6)
management/server/types/account.go (6)
43-50: LGTM!The constants are well-defined and documented. The native SSH port 22022 is appropriate for SSH connection hijacking by clients.
52-55: LGTM!Good encapsulation of peer feature capabilities in an internal struct.
1701-1719: LGTM!The logic correctly adds port 22022 only when port 22 is present and avoids duplicates. The implementation is clear and defensive.
1721-1723: LGTM!The function correctly checks both Port (string) and PortRange (numeric range) fields. Edge cases with zero-valued PortRange are handled appropriately.
1732-1751: LGTM with efficient optimization!The feature detection logic is well-structured:
- Dev versions receive all features for development flexibility
- Native SSH support (0.60.0+) implies port ranges support since 0.60.0 > 0.48.0, avoiding redundant version checks
- Falls back to explicit port ranges check for older versions (0.48.0-0.59.x)
1676-1679: API validation enforces mutual exclusivity; the defensive break is safe but could be clearer.The concern is mitigated by validation at line 229 in
management/server/http/handlers/policies/policies_handler.go, which rejects requests with both Ports and PortRanges populated. All PolicyRules reachingexpandPortsAndRangeshave already passed this validation, so the break at lines 1676-1679 is defensive programming that shouldn't trigger in normal operation.Test cases confirm this design—all test cases populate either Ports or PortRanges, never both. However, the break comment "prefer PolicyRule.Ports" is misleading since the code silently skips PortRanges rather than preferring one over the other. Consider clarifying the comment or adding an assertion to document the mutual exclusivity guarantee.
| // shouldCheckRulesForNativeSSH determines whether specific policy rules should be checked for native SSH support. | ||
| // While users can add the nativeSSHPortString, we look for cases when they used port 22 and based on SSH enabled | ||
| // in both management and client, we indicate to add the native port. | ||
| func shouldCheckRulesForNativeSSH(supportsNative bool, rule *PolicyRule, peer *nbpeer.Peer) bool { | ||
| return supportsNative && peer.SSHEnabled && peer.Meta.Flags.ServerSSHAllowed && rule.Protocol == PolicyRuleProtocolTCP | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Consider including PolicyRuleProtocolALL in protocol check.
The function currently only checks for TCP protocol, but SSH operates over TCP and policy rules with Protocol=ALL should also support native SSH since ALL encompasses TCP.
Should the condition be:
rule.Protocol == PolicyRuleProtocolTCP || rule.Protocol == PolicyRuleProtocolALLThis would ensure native SSH rules are added for ALL-protocol policies.
🏁 Script executed:
#!/bin/bash
# Search for how PolicyRuleProtocolALL is defined and used
ast-grep --pattern 'PolicyRuleProtocolALL = PolicyRuleProtocolType($_)'
# Search for tests covering native SSH with ALL protocol
rg -n -C3 'PolicyRuleProtocolALL.*SSH|SSH.*PolicyRuleProtocolALL' management/server/types/Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find protocol constant definitions
rg -n "PolicyRuleProtocol" management/server/types/account.go | head -20
# Find shouldCheckRulesForNativeSSH usage/calls
rg -n "shouldCheckRulesForNativeSSH" management/server/
# Look for protocol handling patterns in the same file
rg -n "rule\.Protocol ==" management/server/types/account.go | head -10Length of output: 750
🏁 Script executed:
#!/bin/bash
# Find where PolicyRuleProtocol constants are defined
rg -n "const.*PolicyRuleProtocol|type.*PolicyRuleProtocol" management/server/types/
# Find protocol check patterns to see how ALL is typically handled
rg -n "Protocol.*ALL|ALL.*Protocol" management/server/types/account.go -B2 -A2
# Get context around line 1694 to see how shouldCheckRulesForNativeSSH is used
sed -n '1690,1700p' management/server/types/account.goLength of output: 1176
🏁 Script executed:
#!/bin/bash
# Find constant definitions for PolicyRuleProtocol types
cat management/server/types/policyrule.go | grep -A 20 "type PolicyRuleProtocolType"
# Search for protocol matching logic to understand how ALL is handled
rg -n "Protocol.*==" management/server/types/ -A 1 -B 1 | head -40
# Check if there are utility functions for protocol matching
rg -n "func.*Protocol|Protocol.*Match" management/server/types/account.goLength of output: 1506
🏁 Script executed:
#!/bin/bash
# Find const block with protocol values
rg -n "const.*=" management/server/types/policyrule.go | head -20
# Look for ALL protocol definition
rg -n "PolicyRuleProtocolALL|PolicyRuleProtocolTCP" management/server/types/policyrule.go
# Check test for native SSH to understand expected behavior
rg -n -B5 -A5 "addNativeSSHRule|nativeSSH" management/server/types/account.go | head -50Length of output: 1718
🏁 Script executed:
#!/bin/bash
# Get full policyrule.go file to find const blocks
cat management/server/types/policyrule.go | head -100
# Look for protocol constant definitions throughout types package
rg -n "PolicyRuleProtocol.*=" management/server/types/ | grep -v "//"Length of output: 3148
🏁 Script executed:
#!/bin/bash
# Search for tests covering native SSH scenarios
rg -n "nativeSSH|addNativeSSHRule" management/server/types/ --type go -g "*test*"
# Check if there are any tests with PolicyRuleProtocolALL in context of SSH
rg -n "PolicyRuleProtocolALL" management/server/types/ --type go -g "*test*" -B3 -A3Length of output: 1452
Include PolicyRuleProtocolALL in protocol check for native SSH support.
The function currently only checks rule.Protocol == PolicyRuleProtocolTCP, but policies with Protocol = PolicyRuleProtocolALL should also support native SSH since "all" encompasses TCP. This causes native SSH rules to be silently skipped for all-protocol policies.
The fix should be:
rule.Protocol == PolicyRuleProtocolTCP || rule.Protocol == PolicyRuleProtocolALLThis ensures native SSH port expansion (22 → 22022) works for policies with protocol="all", which is used in production scenarios (as seen in test fixtures).
🤖 Prompt for AI Agents
In management/server/types/account.go around lines 1725 to 1730, the protocol
check in shouldCheckRulesForNativeSSH only allows PolicyRuleProtocolTCP so rules
with Protocol set to PolicyRuleProtocolALL are skipped; update the boolean
condition to accept both TCP and ALL (i.e., treat ALL as encompassing TCP) so
native SSH port expansion runs for all-protocol policies, ensuring the function
returns true when supportsNative is true, peer SSH is enabled, ServerSSHAllowed
flag is set, and the rule protocol is either TCP or ALL.
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit