Fix WireGuard rotation edge cases (#766) - #774
Merged
Merged
Conversation
Create provider resources only after a relay is secured, stop persisting write-only previous-key prefs, pick the CIDR prefix by address family, and omit the empty address component for IPv4-less Mullvad configs. Co-Authored-By: Claude Opus 5 <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 #766 — the four verified WireGuard rotation/profile-generation edge cases, bundled because each alone is tiny.
1. Provider resources orphaned when the relay fetch fails (the one with real user cost)
MullvadProfileGenerator.java:126-142created the Mullvad device (fetchWebToken+createDevice), andIvpnProfileGenerator.java:133-146minted the IVPN session, beforechooseRelay(fetchRelays(), …)ran. A failing relay-list fetch therefore left a device/session attached to no profile.This is the only item with an irreversible external cost: a Mullvad account has a hard cap of 5 device slots and IVPN caps concurrent sessions, so repeated failed setups burn provider quota, and nothing in the app can release the orphan — the user has to clean it up on the provider dashboard.
Fix: fetch relays and choose the relay first; create the device/session only once a relay is secured. Nothing after creation can fail (
buildConfigis pure string building), so no cleanup path is introduced — deliberately, since best-effort cleanup here would risk masking the original error for no gain.2. Write-only previous-key prefs retained old private keys
<provider>_previous_privkey/<provider>_previous_addresswere written inVpnKeyRotationManager.commitProviderKey(:349-353) and cleared on completion (:429-434), but read nowhere —rollbackProvideruses its in-memory parameters. They bought no recovery, while process death between the write and the clear left an old WireGuard private key in plaintext SharedPreferences indefinitely.Fix: drop the writes and the now-dead
clearPrevious/currentAddresshelpers, and clean up any values already persisted by earlier versions inApplicationEx.migratePreferencesso existing installs don't keep a stale key forever.Deliberately out of scope: implementing real prefs-backed rollback recovery (surviving process death mid-rotation). That is a much larger design change — it needs a durable rotation journal and a resume path on startup — and does not belong in a bundle of small edge-case fixes.
3. Blind
/32suffix (latent hardening)VpnKeyRotationManager.java:433-438appended/32regardless of address family. The issue missed a second copy atIvpnProfileGenerator.java:317-321; both are fixed to emit/128for IPv6. Latent today because IVPN currently returns IPv4, so this is correctness hardening rather than a live bug.4. Malformed
Addressline for IPv4-less reusable profiles (latent hardening)MullvadProfileGenerator.buildConfig(:354-357) emittedAddress = , fc00:…whenipv4_addresswas empty — reachable viadeviceFromConfig(:196-208) for an imported IPv6-only reusable config. TC's own parser tolerates it; standardwg-quickdoes not. Now only the non-empty components are joined.Test evidence
New
ProfileGeneratorTest(5 tests) andPreviousKeyPreferenceMigrationTest(1 test). The generators are exercised through small package-private seams (fetchRelays,fetchWebToken,createDevice,createSession, plusnewPrivateKey/derivePublicKeyso Robolectric never loads the nativewgbridgelibrary); production behaviour is unchanged.mullvadRelayFailureDoesNotCreateDevice/ivpnRelayFailureDoesNotCreateSessionassert the relay failure propagates unchanged and thatfetchWebToken/createDevice/createSessionwere never invoked. Verified this fails on the pre-fix ordering: temporarily restoring the old order inMullvadProfileGenerator.generategives5 tests completed, 1 failed.mullvadIpv6OnlyReusableConfigHasNoLeadingAddressCommaandmullvadReusableConfigKeepsBothAddresses.ivpnIpv6AddressGetsIpv6CidrassertsAddress = fc00:bbbb:bbbb:bb01::2/128.obsoletePreviousKeyPreferencesAreRemovedseeds all four legacy keys plus unrelated prefs and asserts the migration removes exactly the four. The pre-existingVpnKeyRotationManagerTestassertion onmullvad_previous_privkeystill passes.Full suite: 262 tests, 0 skipped, 0 failures, 0 errors.