[management] add ssh authorized users in network map cache#5048
Conversation
📝 WalkthroughWalkthroughThe changes add SSH support to the network map builder (per-peer EnableSSH and AuthorizedUsers), wire SSH view construction into build/update flows, and refactor golden tests to serialize and compare deterministic JSON outputs for legacy vs new builders. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Test as Test Setup
participant Builder as NetworkMapBuilder
participant Policy as Policy Engine
participant Groups as Group/User Map
participant SSHCache as PeerSSHView Cache
participant NetworkMap as NetworkMap Assembly
note over Builder,Policy Groups: Build-phase: scan policies, resolve groups/users
Test->>Builder: Initialize builder with peers, policies, groups, users
Builder->>Policy: Request policy rules (including SSH rules)
Policy-->>Builder: Return rules
loop each peer
Builder->>Groups: Resolve group→user IDs
Groups-->>Builder: Return user IDs
Builder->>Policy: Evaluate SSH eligibility for peer
Policy-->>Builder: Return SSH authorization decisions
Builder->>SSHCache: Build/store PeerSSHView (EnableSSH, AuthorizedUsers)
end
Builder->>NetworkMap: Assemble map (incl. EnableSSH, AuthorizedUsers)
NetworkMap-->>Test: Return assembled map
Test->>Test: Convert via toNetworkMapJSON and compare legacy vs new JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 (1)
management/server/types/networkmapbuilder.go (1)
994-1013: Consider logging when authorized group is not found.When
groupIDfromrule.AuthorizedGroupsis not found inb.cache.groupIDToUserIDs(line 997-998), the code silently continues. This could mask configuration issues where an SSH policy references a non-existent or user-less group.🔧 Suggested improvement
for groupID, localUsers := range rule.AuthorizedGroups { userIDs, ok := b.cache.groupIDToUserIDs[groupID] if !ok { + log.Debugf("SSH authorized group %s has no users, skipping", groupID) continue }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.go
🧬 Code graph analysis (2)
management/server/types/networkmap_golden_test.go (2)
management/server/types/networkmapbuilder.go (1)
NewNetworkMapBuilder(93-124)management/server/types/network.go (3)
Network(111-120)ForwardingRule(68-73)NetworkMap(32-43)
management/server/types/networkmapbuilder.go (2)
management/server/types/account.go (1)
Account(74-109)management/server/types/policyrule.go (1)
PolicyRuleProtocolType(14-14)
⏰ 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). (2)
- GitHub Check: Agent
- GitHub Check: CodeQL analysis (go)
🔇 Additional comments (18)
management/server/types/networkmap_golden_test.go (6)
37-58: LGTM - SSH-related constants added.The new constants for SSH users group, SSH policy, and user IDs are well-defined and follow the existing naming conventions.
88-104: Golden file workflow writes before assertion.The test writes both
networkmap_golden.jsonandnetworkmap_golden_new.jsonwhen outputs differ, then asserts equality. This is useful for debugging mismatches, but be aware these files will be written to disk on every test failure. Consider whether these files should be.gitignored or if they're intentionally tracked for baseline comparisons.
681-727: LGTM - Deterministic JSON serialization for network maps.The
toNetworkMapJSONfunction correctly handles theAuthorizedUsersmap by sorting both the outer keys (local users) and inner keys (user IDs) before serialization. This ensures consistent JSON output for golden file comparisons.
805-813: LGTM - SSH policy test data.The SSH policy correctly exercises the new
PolicyRuleProtocolNetbirdSSHprotocol withAuthorizedGroupsmapping, allowing testing of the group-to-local-user SSH authorization flow.
846-854: LGTM - User setup for SSH testing.The users map correctly configures differential group membership:
userDevIDanduserOpsIDare in the SSH users group whileuserAdminIDis not, enabling proper testing of SSH authorization based on group membership.
137-213: LGTM - Golden tests updated consistently.The
TestGetPeerNetworkMap_Golden_WithNewPeerand related tests follow a consistent pattern usingtoNetworkMapJSONfor deterministic comparison between legacy and new builder outputs.management/server/types/networkmapbuilder.go (12)
82-85: LGTM - PeerSSHView type definition.The
PeerSSHViewstruct appropriately captures SSH state per peer withEnableSSHandAuthorizedUsersas a nested map for efficient lookup.
1067-1072: LGTM - Protocol mapping for NetbirdSSH.The
firewallRuleProtocolfunction correctly mapsPolicyRuleProtocolNetbirdSSHto TCP for firewall rules, ensuring proper transport layer handling.
1096-1102: LGTM - SSH view integration in GetPeerNetworkMap.The
sshViewis correctly retrieved from cache and passed toassembleNetworkMap. The nil check at line 1188 properly handles cases where SSH view is unavailable, making SSH data optional in the network map.
1299-1299: LGTM - SSH view built on peer addition.The SSH view is correctly built when a peer is added incrementally.
1960-1960: LGTM - SSH view cleaned up on peer deletion.The SSH cache entry is correctly removed when a peer is deleted.
1035-1043: LGTM - Allowed user IDs builder.The
buildAllowedUserIDsfunction correctly filters out blocked and service users to create the set of users eligible for SSH access.
109-111: LGTM - SSH cache initialization.The new cache fields
peerSSH,groupIDToUserIDs, andallowedUserIDsare correctly initialized inNewNetworkMapBuilder.
163-170: LGTM - SSH cache cleared and rebuilt in buildGlobalIndexes.The SSH-related caches are properly cleared and rebuilt when global indexes are regenerated.
443-443: LGTM - Consistent protocol handling in firewall rules.The
firewallRuleProtocolfunction is used consistently across all firewall rule creation paths, ensuringPolicyRuleProtocolNetbirdSSHis correctly mapped to TCP.
1178-1193: LGTM - NetworkMap assembly with optional SSH data.The SSH fields (
EnableSSH,AuthorizedUsers) are correctly added to theNetworkMaponly whensshViewis available, maintaining backward compatibility.
1045-1065: All SSH port constants are properly defined inmanagement/server/types/account.go(lines 48–52):
defaultSSHPortString = "22"defaultSSHPortNumber = 22nativeSSHPortString = "22022"nativeSSHPortNumber = 22022The methods in
networkmapbuilder.gocorrectly reference these constants.
16-16: Theauthpackage is not a client-specific package, but rather shared authorization logic used by both the management server and client SSH server. Theauth.Wildcardconstant is a simple string literal ("*"), and the auth package itself has no client-side dependencies—it only imports standard libraries and shared code. This import pattern is already established throughout the management server codebase (account.go, grpc/conversion.go, etc.), with no circular dependency concerns.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Pull request overview
This PR adds SSH authorized users to the network map cache as part of an ongoing feature implementation. The changes introduce SSH-related data structures and logic to the cached network map builder, allowing for efficient retrieval of SSH configuration without recalculating it on every network map request.
- Adds
PeerSSHViewstructure to cache SSH enablement and authorized users per peer - Implements
buildPeerSSHViewto calculate SSH access based on policies and legacy SSH settings - Updates firewall rule protocol handling to convert NetbirdSSH protocol to TCP for firewall rules
- Refactors golden tests to compare legacy and new builder implementations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| management/server/types/networkmapbuilder.go | Adds SSH view caching infrastructure including new cache fields (peerSSH, groupIDToUserIDs, allowedUserIDs), SSH view building logic, and firewall protocol conversion for NetbirdSSH |
| management/server/types/networkmap_golden_test.go | Updates tests to compare legacy and new implementations, adds SSH test data (users, groups, policies), and introduces JSON conversion helper for consistent serialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
management/server/types/networkmapbuilder.go (1)
1056-1060: Consider adding sshView to the nil check for consistency.While the code handles
sshView == nilsafely inassembleNetworkMap(Line 1148), the inconsistency between view checks could cause confusion. For clarity, either:
- Add
sshViewto this check, or- Add a comment explaining why
sshViewis optional while other views are requiredBased on past review comments, this consistency issue has been noted before.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
management/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-28T12:20:47.254Z
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 4849
File: management/internals/modules/zones/manager/manager.go:55-86
Timestamp: 2025-11-28T12:20:47.254Z
Learning: In the NetBird management server, DNS zones without records are automatically filtered out in network map generation (filterPeerAppliedZones in management/internals/controllers/network_map/controller/controller.go checks `len(zone.Records) == 0`). Therefore, CreateZone operations don't need to call UpdateAccountPeers since empty zones don't affect the network map.
Applied to files:
management/server/types/networkmap_golden_test.gomanagement/server/types/networkmapbuilder.go
🧬 Code graph analysis (1)
management/server/types/networkmap_golden_test.go (5)
management/server/types/networkmapbuilder.go (1)
NewNetworkMapBuilder(105-141)management/server/types/policyrule.go (1)
PolicyRule(41-89)management/server/types/policy.go (2)
PolicyTrafficActionAccept(12-12)PolicyRuleProtocolNetbirdSSH(27-27)management/server/types/user.go (2)
UserRoleAdmin(14-14)UserRoleUser(15-15)management/server/types/account.go (1)
Account(74-109)
⏰ 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: JS / Lint
- GitHub Check: Darwin
- GitHub Check: Build Cache
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: release
🔇 Additional comments (12)
management/server/types/networkmapbuilder.go (7)
15-15: LGTM - SSH auth import used correctly.The import is properly utilized for the
auth.Wildcardconstant throughout the SSH authorization logic.
51-54: LGTM - SSH cache fields properly integrated.The new SSH-related cache fields follow the same lifecycle patterns as existing cache maps: initialized in the constructor, cleared in
buildGlobalIndexes, and cleaned up inOnPeerDeleted.
84-87: LGTM - PeerSSHView structure is clear.The structure appropriately represents per-peer SSH configuration with a boolean flag and a two-level map for local-user-to-NetBird-user-ID authorization.
393-427: LGTM - SSH authorization logic is sound.The implementation correctly handles both NetBird SSH and legacy SSH protocols:
- NetBird SSH supports group-based authorization with local user mappings
- Legacy SSH defaults to wildcard access for all allowed users
- Proper use of
maps.Cloneprevents unintended mutations of shared cache data
1010-1018: LGTM - User filtering logic is correct.The helper appropriately excludes blocked users and service users from SSH authorization, ensuring only active regular users can be authorized.
1020-1025: LGTM - Protocol mapping is appropriate.Correctly maps the logical NetBird SSH protocol to its underlying TCP transport for firewall rule generation.
1138-1153: LGTM - SSH data properly integrated into NetworkMap.The SSH fields are correctly populated from the per-peer SSH view with appropriate nil-safety checks.
management/server/types/networkmap_golden_test.go (5)
33-33: LGTM - SSH test constants are well-defined.The new constants follow existing naming conventions and provide clear identifiers for SSH-related test entities.
Also applies to: 41-41, 52-54
677-723: LGTM - Deterministic JSON serialization well-implemented.The
networkMapJSONtype and converter properly address the need for stable, comparable JSON output by:
- Converting
map[string]struct{}sets to sorted string slices- Ensuring deterministic ordering for all map keys
- Using
omitemptyto handle empty AuthorizedUsers gracefullyThis is the correct approach for golden file testing.
73-100: LGTM - Enhanced golden file comparison workflow.The new approach of writing both legacy and new builder outputs to separate files when differences occur significantly improves debugging experience. The use of
JSONEqfor final assertion ensures structural comparison rather than string matching.This workflow assumes differences indicate bugs in the new builder that need fixing, which is appropriate for a refactoring validation suite.
Also applies to: 180-209, 320-349, 461-490, 540-569
801-809: LGTM - SSH policy correctly tests group-based authorization.The policy appropriately exercises the SSH authorization logic with:
- NetBird SSH protocol
- Group-to-group connectivity (dev → ops)
- Local user mappings via AuthorizedGroups
This validates the SSH integration added to the network map builder.
842-850: LGTM - User test data properly configured for SSH scenarios.The user setup correctly models the SSH authorization flow:
- Users assigned to SSH group and their respective functional groups
- Group memberships align with the SSH policy requirements
- Mix of admin and regular users tests different authorization levels
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
that's an addition to the changes made during feature implementation
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
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.