fix(protonvpn): adopt reassigned forwarded ports instead of restarting port forwarding - #3409
Open
quadseven wants to merge 1 commit into
Open
fix(protonvpn): adopt reassigned forwarded ports instead of restarting port forwarding#3409quadseven wants to merge 1 commit into
quadseven wants to merge 1 commit into
Conversation
The ProtonVPN gateway may hand back a different external port when an existing NAT-PMP mapping is refreshed. addPortMappingTCPUDP treated that as an error, which propagated out of KeepPortForward and tore down the whole port forwarding service: the firewall rules were removed, the port file was cleared and the service was rebuilt from scratch. The up command only ran for the port obtained before the reassignment, so a downstream client was left listening on a port that was no longer forwarded. Add an optional OnPortsChanged callback to PortForwardObjects so a provider can hand newly assigned ports back to the service. The service closes the firewall for the previous ports before opening the new ones, rewrites the port file and re-runs the up command, reusing the same path as SetPortsForwarded. Providers whose service does not supply the callback keep returning an error rather than silently forwarding a port the client is not listening on.
quadseven
added a commit
to quadseven/grug
that referenced
this pull request
Jul 26, 2026
The enforcement-gap monitor paged on quadseven/gluetun an hour after the repo was forked on 2026-07-26. It was red on four repos; three of them - gluetun, flood, vibetunnel - are forks of third-party OSS with ZERO PRs each. We fork upstream projects only to carry a patch branch that goes back upstream. The merge target for that work is the PARENT repo, so there is no first-party PR for a DoR check to gate, and requiring a DoR body on an upstream-sync merge would be meaningless. gluetun exists purely so infra's production/docker/gluetun-portfix can build from a patch branch (upstream passteque/gluetun#3409). Excludes forks from the denominator, mirroring the archived-repo skip directly above it. This differs from that case in one way worth stating: grug COULD write a ruleset to a fork, so this is a deliberate policy call that forks are out of scope, not a capability limit. It narrows the denominator without hiding anything, and the fourth repo in that same alert proves it: `yuzu-yard-sale` is NOT a fork, is genuinely unenforced, and stays counted. A test asserts exactly that. Tests: 3 added (fork excluded, real gap survives the exclusion, missing `fork` key counts as first-party). 49/49 pass. Claude-Session: https://claude.ai/code/session_01CMFMgGqrxJ1a1gy7nUqzpq Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #3408.
Why
The ProtonVPN gateway may hand back a different external port when an existing NAT-PMP mapping is refreshed.
addPortMappingTCPUDPtreated that as an error:That error propagated out of
KeepPortForward, soStart's goroutine rancleanup()and pushed the error to the loop, which tore down and rebuilt the whole port forwarding service. Observed effect on a live setup:VPN_PORT_FORWARDING_UP_COMMANDhad already run for the previous port and did not run again for the replacement, so the downstream client stayed on a port that was no longer forwardedGET /v1/portforwardended up reporting0while the port file and the client still held the stale portWorth noting that the external-port check could only ever fire on the refresh path.
PortForwardcallsaddPortMappingTCPUDPwithexternalPortof1(first, symmetric mapping) or0(extra mappings), and both are excluded by the condition. So the check existed solely to reject the case this PR now handles.Size: S
What changed
utils.PortForwardObjectsgains an optionalOnPortsChangedcallback so a provider can hand newly assigned ports back to the port forwarding service.Service.onPortsChangedapplies them in place: it calls the existingcleanup()to close the firewall for the previous ports, then the existingonNewPorts()to allow the new ones, rewrite the port file and re-run the up command. This is the same sequenceSetPortsForwardedalready uses, so no new firewall logic is introduced.protonvpn.KeepPortForwardcompares each refreshed mapping against the previous one, logs any reassignment, and callsOnPortsChangedonce per refresh if anything moved.addPortMappingTCPUDPno longer rejects a changed external port; the caller now decides. The internal-port check and the TCP/UDP consistency check are unchanged.OnPortsChangedis nil,KeepPortForwardreturns the newErrPortsReassignedinstead, preserving today's restart behaviour rather than silently forwarding a port the client is not listening on.Ordering and the firewall
onPortsChangeddeliberately removes the old allowed port before adding the new one, so a reassignment cannot leave a hole open on a port that is no longer forwarded.Test_Service_onPortsChangedasserts that ordering withgomock.InOrder. Nothing outside the forwarded-port rules is touched, so the kill switch and outbound-subnet handling are unaffected.onPortsChangedtakes onlyportMutex, neverstartStopMutex. It is invoked from theKeepPortForwardgoroutine, andStopholdsstartStopMutexwhile waiting for that goroutine to return, so taking it here would deadlock. This is noted in a comment on the method.Acceptance criteria
GET /v1/portforwardreport the newly assigned port.Test plan
go build ./...clean.go vet ./internal/portforward/... ./internal/provider/...clean.Test_Service_onPortsChangedandTest_Service_onPortsChanged_keepsPreviousPortsOnFailure, covering the adopt path, the firewall ordering, the port file contents, and the failure path where the new port cannot be allowed.Test_portsMapToStringcovering no-change, single reassignment, multiple ports, partial change and unknown internal port.mockgendirective extended fromLoggertoLogger,PortAllower;mocks_test.goregenerated with the same command.go test ./internal/portforward/... ./internal/provider/...all pass.Two notes on the local run, for transparency:
internal/netlink,internal/tunandinternal/natpmpfail for me, but they fail identically on an unmodified checkout ofmaster(verified 5/5 runs forTest_Client_AddPortMapping/rpc_error, which expects an i/o timeout and gets a connection refused). These look macOS-specific and unrelated.KeepPortForwarditself, because it constructsnatpmp.New()internally and the client is not injectable. Happy to add one if you would like that refactored as part of this PR.Out of scope
KeepPortForwardimplementations, which do not renew a port mapping this way.