[client] Add unicast UPnP discovery for port forwarding.#6650
Conversation
Add a UPnP IGD discovery fallback that sends unicast SSDP M-SEARCH requests directly to the default gateway when PCP and multicast discovery do not find a gateway.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds unicast UPnP/IGD discovery and NAT port mapping, wires it into gateway discovery as a PCP fallback, and adds tests plus a direct ChangesUnicast UPnP Discovery
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/portforward/upnp/nat.go`:
- Around line 124-140: DeletePortMapping currently relies on NAT.ports cache
state, so a freshly rediscovered gateway in Cleanup() will no-op and leave the
router mapping behind. Update the cleanup flow so the external port is preserved
outside the ephemeral NAT instance, likely on State alongside InternalPort and
Protocol, or make Cleanup() reuse the same gateway/client used by
AddPortMapping. Then adjust NAT.DeletePortMapping to use the persisted external
port instead of returning nil when n.ports[internalPort] is empty.
- Around line 44-51: GetDeviceAddress and GetInternalAddress should not resolve
n.root.URLBase.Host through DNS, since that host comes from untrusted IGD XML
and can block indefinitely. Update these paths to avoid name resolution entirely
by parsing only literal IPs from URLBase.Host (for example, extracting the host
component and validating it as an IP) and reject or handle non-literal hosts
explicitly. Keep the same safety approach in both NAT methods so they behave
consistently with GetExternalAddress’s bounded network behavior.
In `@client/internal/portforward/upnp/upnp_test.go`:
- Around line 162-165: The SOAP request body handling in fakeIGD.handleSOAP can
truncate or panic because it assumes r.ContentLength is reliable and uses a
single Body.Read call. Replace this with a full read of the request body in
handleSOAP so the test always captures the complete SOAP payload, and avoid
allocating based directly on ContentLength. This will keep the later
mappingBody/deleteBody assertions accurate even when the request is split across
reads or uses chunked transfer.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44f7b234-dd1a-4b7c-9532-5a160edc1b86
📒 Files selected for processing (5)
client/internal/portforward/state.goclient/internal/portforward/upnp/nat.goclient/internal/portforward/upnp/upnp.goclient/internal/portforward/upnp/upnp_test.gogo.mod
| // GetDeviceAddress returns the internal address of the gateway device. | ||
| func (n *NAT) GetDeviceAddress() (net.IP, error) { | ||
| addr, err := net.ResolveUDPAddr("udp4", n.root.URLBase.Host) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| return addr.IP, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unbounded host resolution on untrusted IGD data.
GetDeviceAddress/GetInternalAddress resolve n.root.URLBase.Host via net.ResolveUDPAddr/net.Dial with no timeout, unlike GetExternalAddress which wraps its call in a 10s context. URLBase.Host comes from the untrusted device-description XML returned by whatever answered the SSDP search, so if it's a hostname (not a literal IP) this can trigger an unbounded DNS lookup.
🛡️ Suggested fix avoiding DNS resolution entirely
func (n *NAT) GetDeviceAddress() (net.IP, error) {
- addr, err := net.ResolveUDPAddr("udp4", n.root.URLBase.Host)
+ host, _, err := net.SplitHostPort(n.root.URLBase.Host)
if err != nil {
return nil, err
}
- return addr.IP, nil
+ ip := net.ParseIP(host)
+ if ip == nil {
+ return nil, fmt.Errorf("device URLBase host is not an IP: %s", host)
+ }
+ return ip, nil
}Also applies to: 71-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/internal/portforward/upnp/nat.go` around lines 44 - 51,
GetDeviceAddress and GetInternalAddress should not resolve n.root.URLBase.Host
through DNS, since that host comes from untrusted IGD XML and can block
indefinitely. Update these paths to avoid name resolution entirely by parsing
only literal IPs from URLBase.Host (for example, extracting the host component
and validating it as an IP) and reject or handle non-literal hosts explicitly.
Keep the same safety approach in both NAT methods so they behave consistently
with GetExternalAddress’s bounded network behavior.
| // DeletePortMapping removes a port mapping. | ||
| func (n *NAT) DeletePortMapping(ctx context.Context, protocol string, internalPort int) error { | ||
| proto, err := mapProtocol(protocol) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| n.mu.Lock() | ||
| externalPort := n.ports[internalPort] | ||
| delete(n.ports, internalPort) | ||
| n.mu.Unlock() | ||
|
|
||
| if externalPort == 0 { | ||
| return nil | ||
| } | ||
| return n.client.DeletePortMappingCtx(ctx, "", uint16(externalPort), proto) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
DeletePortMapping silently no-ops when the external port isn't cached — breaks cleanup on freshly-discovered gateways.
DeletePortMapping only calls DeletePortMappingCtx when n.ports[internalPort] > 0; otherwise it returns nil (success) without touching the gateway. Since state.go's Cleanup() calls discoverGateway(ctx) fresh, and natFromService always constructs a brand-new &NAT{ports: make(map[int]int)} (line 215), the ports map at cleanup time is always empty for the unicast UPnP path — meaning the actual router-side port mapping is never deleted, and the caller has no way to know cleanup silently failed. The stale mapping only disappears once its own UPnP lease expires.
Consider persisting the external port outside the ephemeral NAT instance (e.g. on the State struct alongside InternalPort/Protocol) so it survives rediscovery, or have Cleanup() reuse the same gateway instance used for the original AddPortMapping.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 138-138: Narrowing a non-constant integer to a smaller fixed-width type (int8/int16/int32, uint8/uint16/uint32) can silently overflow or wrap, yielding negative or truncated values that are dangerous in size, length, or index logic. Validate the source value is within the target type's range before converting (e.g. bounds-check, or use a checked helper), and avoid narrowing untrusted or len()/parsed values.
Context: uint16(externalPort)
Note: [CWE-190] Integer Overflow or Wraparound.
(integer-overflow-narrowing-conversion-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/internal/portforward/upnp/nat.go` around lines 124 - 140,
DeletePortMapping currently relies on NAT.ports cache state, so a freshly
rediscovered gateway in Cleanup() will no-op and leave the router mapping
behind. Update the cleanup flow so the external port is preserved outside the
ephemeral NAT instance, likely on State alongside InternalPort and Protocol, or
make Cleanup() reuse the same gateway/client used by AddPortMapping. Then adjust
NAT.DeletePortMapping to use the persisted external port instead of returning
nil when n.ports[internalPort] is empty.
| func (f *fakeIGD) handleSOAP(w http.ResponseWriter, r *http.Request) { | ||
| body := make([]byte, r.ContentLength) | ||
| _, _ = r.Body.Read(body) | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Body read can silently truncate the SOAP payload.
r.Body.Read(body) performs a single Read call, but io.Reader may return fewer bytes than requested even when more data is available (this is explicitly allowed by the interface contract). Combined with make([]byte, r.ContentLength), this can leave body incomplete (padded with zero bytes) if the request arrives in multiple TCP reads, which would silently corrupt the assert.Contains checks on mappingBody/deleteBody later in the tests. It would also panic with a negative-length slice if ContentLength is ever -1 (e.g., chunked transfer).
🐛 Proposed fix using io.ReadAll
+ "io"
+
func (f *fakeIGD) handleSOAP(w http.ResponseWriter, r *http.Request) {
- body := make([]byte, r.ContentLength)
- _, _ = r.Body.Read(body)
+ body, _ := io.ReadAll(r.Body)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (f *fakeIGD) handleSOAP(w http.ResponseWriter, r *http.Request) { | |
| body := make([]byte, r.ContentLength) | |
| _, _ = r.Body.Read(body) | |
| func (f *fakeIGD) handleSOAP(w http.ResponseWriter, r *http.Request) { | |
| body, _ := io.ReadAll(r.Body) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/internal/portforward/upnp/upnp_test.go` around lines 162 - 165, The
SOAP request body handling in fakeIGD.handleSOAP can truncate or panic because
it assumes r.ContentLength is reliable and uses a single Body.Read call. Replace
this with a full read of the request body in handleSOAP so the test always
captures the complete SOAP payload, and avoid allocating based directly on
ContentLength. This will keep the later mappingBody/deleteBody assertions
accurate even when the request is split across reads or uses chunked transfer.
…create-upnp-port-mapping-on-opnsense-firewall
|
1 participant
Describe your changes
Add a UPnP IGD discovery fallback that sends unicast SSDP M-SEARCH requests directly to the default gateway when PCP and multicast discovery do not find a gateway.
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