[client] Add IPv6 support to SSH server, client config, and netflow logger#5687
[client] Add IPv6 support to SSH server, client config, and netflow logger#5687lixmal merged 5 commits intoclient-ipv6-dnsfrom
Conversation
📝 WalkthroughWalkthroughThis PR adds comprehensive IPv6 support across SSH and network flow handling. Changes include IPv6 address extraction from peer configurations, dual IPv4/IPv6 SSH server listeners, enhanced network flow tracking with IPv6 ICMP support, updated connection validation for both address families, and migrations from string to netip.Addr types for type safety. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 review |
✅ Actions performedReview triggered.
|
ec971ef to
4350665
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
client/internal/netflow/types/types.go (1)
22-22: UpdategetProtocolFromPacket()to distinguish ICMPv6 from ICMP.The new
ICMPv6 = Protocol(58)constant is correct, butclient/firewall/uspfilter/filter.go:1124currently returnsnftypes.ICMPfor bothLayerTypeICMPv4andLayerTypeICMPv6. This causes ICMPv6 traffic to be recorded asProtocol(1)when originating from the userspace filter, while the conntrack path returnsProtocol(58), creating inconsistent protocol identification across the two paths.Update
getProtocolFromPacket()to returnnftypes.ICMPv6forLayerTypeICMPv6(aligning with the TODO comment at line 1200).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/internal/netflow/types/types.go` at line 22, getProtocolFromPacket currently maps both LayerTypeICMPv4 and LayerTypeICMPv6 to nftypes.ICMP, causing ICMPv6 packets to be misclassified; update the mapping in getProtocolFromPacket (in client/firewall/uspfilter/filter.go) so that when pkt.LayerType() == gopacket.LayerTypeICMPv6 it returns nftypes.ICMPv6 (the new Protocol(58)) while keeping LayerTypeICMPv4 mapped to nftypes.ICMP, and ensure the change aligns with the existing TODO near the function.client/ssh/config/manager_test.go (1)
29-60: Please cover the new IPv6 host alias in this test.This fixture still only proves the IPv4/FQDN paths. The new
PeerSSHInfo.IPv6branch inManager.buildHostPatterns()can regress without failing any test, so I'd add a dual-stack peer and assert the rendered config contains the IPv6 literal too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/ssh/config/manager_test.go` around lines 29 - 60, Test currently only asserts IPv4/FQDN output so the new IPv6 branch in Manager.buildHostPatterns() is untested; add a dual-stack PeerSSHInfo (set both IP and IPv6 fields) to the peers slice passed to SetupSSHClientConfig and assert the generated config (read from manager.sshConfigDir + manager.sshConfigFile) contains the IPv6 literal string as well; ensure the added peer references the PeerSSHInfo.IPv6 field and that SetupSSHClientConfig/Manager.buildHostPatterns() will render that address so the test fails if IPv6 support regresses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/internal/engine_ssh.go`:
- Around line 44-50: Make the IPv6 setup atomic: when
e.wgInterface.Address().IPv6 is valid, first attempt to create the IPv6 listener
(the existing AddListener call) and only if that succeeds call
e.firewall.AddInboundDNAT(v6, ...). If AddInboundDNAT fails, immediately tear
down the previously created listener (call the listener cleanup/remove method
used where AddListener returns a handle, e.g., RemoveListener/Close) and log the
failure; conversely, only log the "SSH port redirection enabled" success message
after both the listener creation and AddInboundDNAT have succeeded. Also guard
the DNAT step behind whatever firewall capability check exists (e.g., a
SupportsIPv6 or similar) and skip/rollback when IPv6 DNAT is unsupported.
- Around line 163-176: extractPeerIPs currently accepts any parsed AllowedIps
prefix and picks the first per family, which can be a subnet (e.g. 10.0.0.0/8)
instead of a single-overlay host address; change it to only accept
single-address prefixes: when iterating peerConfig.GetAllowedIps(), parse the
prefix and then for IPv4 require prefix.Bits() == 32 and for IPv6 require
prefix.Bits() == 128 before using prefix.Addr().Unmap() to set v4/v6; update the
logic in extractPeerIPs (and the code that populates PeerSSHInfo) to ignore
non-/32 and non-/128 prefixes so only single-address overlay entries become
PeerSSHInfo addresses.
---
Nitpick comments:
In `@client/internal/netflow/types/types.go`:
- Line 22: getProtocolFromPacket currently maps both LayerTypeICMPv4 and
LayerTypeICMPv6 to nftypes.ICMP, causing ICMPv6 packets to be misclassified;
update the mapping in getProtocolFromPacket (in
client/firewall/uspfilter/filter.go) so that when pkt.LayerType() ==
gopacket.LayerTypeICMPv6 it returns nftypes.ICMPv6 (the new Protocol(58)) while
keeping LayerTypeICMPv4 mapped to nftypes.ICMP, and ensure the change aligns
with the existing TODO near the function.
In `@client/ssh/config/manager_test.go`:
- Around line 29-60: Test currently only asserts IPv4/FQDN output so the new
IPv6 branch in Manager.buildHostPatterns() is untested; add a dual-stack
PeerSSHInfo (set both IP and IPv6 fields) to the peers slice passed to
SetupSSHClientConfig and assert the generated config (read from
manager.sshConfigDir + manager.sshConfigFile) contains the IPv6 literal string
as well; ensure the added peer references the PeerSSHInfo.IPv6 field and that
SetupSSHClientConfig/Manager.buildHostPatterns() will render that address so the
test fails if IPv6 support regresses.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 87691edb-06d9-462d-9439-faecaf9c5285
📒 Files selected for processing (10)
client/internal/engine_ssh.goclient/internal/netflow/conntrack/conntrack.goclient/internal/netflow/logger/logger.goclient/internal/netflow/logger/logger_test.goclient/internal/netflow/manager.goclient/internal/netflow/types/types.goclient/internal/rosenpass/manager.goclient/ssh/config/manager.goclient/ssh/config/manager_test.goclient/ssh/server/server.go
4350665 to
e5e177e
Compare
e5e177e to
1a7e835
Compare
|



Describe your changes
GetPeerSSHKeymatches by v6 addressnet.JoinHostPortfor correct v6 bracket formattingIssue 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