Repository navigation
Conversation
… replace Two related bugs found together after a production tunnel silently died and stayed down for 3 days with nothing noticing: 1. restctl's monitorConnections goroutine already polls swanctl every 5s and correctly detects a missing connection/child via checkStatus(), but downgradeConnectionStatus only used that to mark the IPSecConnection's status Down/Degraded - nothing ever called the existing RestartConnection/ RestartConnectionChild swanctl-initiate logic to actually recover it. The self-healing plumbing was half-built: detection worked, recovery never fired automatically. Add attemptSelfHeal, invoked from downgradeConnectionStatus for every detected failure, with a 30s per-connection/child cooldown so a genuinely unreachable peer doesn't get hammered with a re-initiate every 5s tick. 2. translate() set no default for the IKE-level `unique` option, so it was left unset and charon's own default applies. Reproduced live: a re-initiate racing the connection's own start_action/trap policy (e.g. during a reauth, or a self-heal initiate racing an in-flight acquire) succeeds on both paths, leaving two IKE_SA/CHILD_SA pairs sharing one reqid. One of the pair goes untracked by future rekey/reauth scheduling and can be silently lost without ever logging a delete - this is almost certainly what actually happened to the tunnel that started this investigation. Default `unique` to "replace" so charon tears down the older SA to the same identity instead of letting both live, which is what the auto re-initiate in (1) now depends on being safe to call. Verified: go build/vet/gofmt clean. (1) is inferred from reading the existing (unused) restart endpoints and the monitor/detect loop; (2) was reproduced live - manually re-initiating a connection that still had its start_action trap routed produced exactly this duplicate-SA state, confirmed via swanctl --list-sas showing two ESTABLISHED IKE_SAs to the same peer, one carrying zero traffic.
This branch has not been deployed
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
2 participants
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.
Two related bugs found together after a production tunnel silently died
restctl's monitorConnections goroutine already polls swanctl every 5s
and correctly detects a missing connection/child via checkStatus(), but
downgradeConnectionStatus only used that to mark the IPSecConnection's
status Down/Degraded - nothing ever called the existing RestartConnection/
RestartConnectionChild swanctl-initiate logic to actually recover it.
Add attemptSelfHeal, invoked from downgradeConnectionStatus for every
detected failure, with a 30s per-connection/child cooldown so a genuinely
unreachable peer doesn't get hammered with a re-initiate every 5s tick.
translate() set no default for the IKE-level
uniqueoption, so itwas left unset and charon's own default applies. Reproduced live: a
re-initiate racing the connection's own start_action/trap policy (e.g.
during a reauth, or a self-heal initiate racing an in-flight
acquire) succeeds on both paths, leaving two IKE_SA/CHILD_SA pairs
sharing one reqid. One of the pair goes untracked by future
rekey/reauth scheduling and can be silently lost without ever logging
a delete - this is almost certainly what actually happened to the
tunnel that started this investigation. Default
uniqueto "replace"so charon tears down the older SA to the same identity instead of
letting both live, which is what the auto re-initiate in (1) now
depends on being safe to call.