From 776a6f4ed61ecb3a9d3f5a4d2a3e994ef7633542 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Sat, 22 Aug 2026 12:11:15 +0200 Subject: [PATCH] Fix WireGuard rotation edge cases (#766) 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 --- .../eu/faircode/netguard/ApplicationEx.java | 13 ++ .../wg/IvpnProfileGenerator.java | 28 ++- .../wg/MullvadProfileGenerator.java | 32 ++- .../wg/VpnKeyRotationManager.java | 28 +-- .../PreviousKeyPreferenceMigrationTest.java | 54 +++++ .../wg/ProfileGeneratorTest.java | 207 ++++++++++++++++++ 6 files changed, 314 insertions(+), 48 deletions(-) create mode 100644 app/src/test/java/eu/faircode/netguard/PreviousKeyPreferenceMigrationTest.java create mode 100644 app/src/test/java/net/kollnig/missioncontrol/wg/ProfileGeneratorTest.java diff --git a/app/src/main/java/eu/faircode/netguard/ApplicationEx.java b/app/src/main/java/eu/faircode/netguard/ApplicationEx.java index 701d8ae7..3b423210 100644 --- a/app/src/main/java/eu/faircode/netguard/ApplicationEx.java +++ b/app/src/main/java/eu/faircode/netguard/ApplicationEx.java @@ -199,6 +199,19 @@ public void onActivityDestroyed(@NonNull Activity activity) { } static void migratePreferences(SharedPreferences prefs) { + if (prefs.contains("mullvad_previous_privkey") || + prefs.contains("mullvad_previous_address") || + prefs.contains("ivpn_previous_privkey") || + prefs.contains("ivpn_previous_address")) { + prefs.edit() + .remove("mullvad_previous_privkey") + .remove("mullvad_previous_address") + .remove("ivpn_previous_privkey") + .remove("ivpn_previous_address") + .apply(); + Log.i(TAG, "Removed obsolete WireGuard previous-key preferences"); + } + if (prefs.contains("onboarding_complete") && !prefs.contains("onboarding_version")) { boolean completed = prefs.getBoolean("onboarding_complete", false); prefs.edit() diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java b/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java index 6dfec972..74705f76 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/IvpnProfileGenerator.java @@ -82,7 +82,8 @@ public static class ApiRejectedException extends IOException { } } - private static class Relay { + // Package-private so tests can supply relay data without making HTTP calls. + static class Relay { String hostname; String countryCode; String countryName; @@ -132,14 +133,14 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo if (account.isEmpty()) throw new IllegalArgumentException("IVPN account number is required"); + Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); WgProfileManager.IvpnSession session = reusableSession; if (session == null || !session.isUsable()) { - String privateKey = Wgbridge.generatePrivateKey(); - String publicKey = Wgbridge.publicKey(privateKey); + String privateKey = newPrivateKey(); + String publicKey = derivePublicKey(privateKey); session = createSession(account, privateKey, publicKey, captchaId, captchaValue); } - Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); String config = buildConfig(session.privateKey, session.address, relay); return new GeneratedProfile("IVPN - " + relay.countryName, config, account, relay.countryCode, relay.countryName, relay.hostname, session); @@ -171,9 +172,18 @@ public WgProfileManager.IvpnSession rotateSessionKey(WgProfileManager.IvpnSessio return new WgProfileManager.IvpnSession(session.token, newPrivateKey, newPublicKey, address); } - private WgProfileManager.IvpnSession createSession(String account, String privateKey, - String publicKey, String captchaId, - String captchaValue) + // Package-private seams keep generator tests independent of the native library and HTTP. + String newPrivateKey() { + return Wgbridge.generatePrivateKey(); + } + + String derivePublicKey(String privateKey) { + return Wgbridge.publicKey(privateKey); + } + + WgProfileManager.IvpnSession createSession(String account, String privateKey, + String publicKey, String captchaId, + String captchaValue) throws Exception { JSONObject body = new JSONObject(); body.put("username", account); @@ -209,7 +219,7 @@ private WgProfileManager.IvpnSession createSession(String account, String privat return new WgProfileManager.IvpnSession(token, privateKey, publicKey, address); } - private List fetchRelays() throws Exception { + List fetchRelays() throws Exception { Request request = new Request.Builder() .url(API + "/v5/servers.json") .build(); @@ -318,7 +328,7 @@ private String addressWithCidr(String address) { String trimmed = address == null ? "" : address.trim(); if (trimmed.contains("/")) return trimmed; - return trimmed + "/32"; + return trimmed + (trimmed.contains(":") ? "/128" : "/32"); } private String dnsFromRelay(Relay relay) { diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java b/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java index 48341f04..4a36a2ab 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/MullvadProfileGenerator.java @@ -81,7 +81,8 @@ public boolean isPublicKeyInUse() { } } - private static class Relay { + // Package-private so tests can supply relay data without making HTTP calls. + static class Relay { String hostname; String countryCode; String countryName; @@ -125,19 +126,18 @@ public GeneratedProfile generate(String accountNumber, String requestedCountryCo throw new IllegalArgumentException("Mullvad account number is required"); WgConfig reusable = parseReusableConfig(reusableConfig); + Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); String privateKey; JSONObject device; if (reusable == null) { - privateKey = Wgbridge.generatePrivateKey(); - String publicKey = Wgbridge.publicKey(privateKey); + privateKey = newPrivateKey(); + String publicKey = derivePublicKey(privateKey); String token = fetchWebToken(account); device = createDevice(token, publicKey); } else { privateKey = reusable.getPrivateKey(); device = deviceFromConfig(reusable); } - Relay relay = chooseRelay(fetchRelays(), requestedCountryCode, excludeHostname); - String config = buildConfig(privateKey, device, relay); return new GeneratedProfile("Mullvad - " + relay.countryName, config, account, relay.countryCode, relay.countryName, relay.hostname, device.optString("id", "")); @@ -209,7 +209,16 @@ private JSONObject deviceFromConfig(WgConfig config) throws Exception { return device; } - private String fetchWebToken(String accountNumber) throws Exception { + // Package-private seams keep generator tests independent of the native library and HTTP. + String newPrivateKey() { + return Wgbridge.generatePrivateKey(); + } + + String derivePublicKey(String privateKey) { + return Wgbridge.publicKey(privateKey); + } + + String fetchWebToken(String accountNumber) throws Exception { JSONObject body = new JSONObject(); body.put("account_number", accountNumber); @@ -220,7 +229,7 @@ private String fetchWebToken(String accountNumber) throws Exception { return token; } - private JSONObject createDevice(String token, String publicKey) throws Exception { + JSONObject createDevice(String token, String publicKey) throws Exception { JSONObject body = new JSONObject(); body.put("pubkey", publicKey); body.put("hijack_dns", false); @@ -248,7 +257,7 @@ private List listDevices(String token) throws Exception { } } - private List fetchRelays() throws Exception { + List fetchRelays() throws Exception { Request request = new Request.Builder() .url(API + "/www/relays/all") .build(); @@ -351,10 +360,9 @@ private String buildConfig(String privateKey, JSONObject device, Relay relay) { if (!TextUtils.isEmpty(deviceName)) sb.append("# Mullvad device = ").append(deviceName).append('\n'); sb.append("PrivateKey = ").append(privateKey).append('\n'); - sb.append("Address = ").append(ipv4); - if (!TextUtils.isEmpty(ipv6)) - sb.append(", ").append(ipv6); - sb.append('\n'); + String address = TextUtils.isEmpty(ipv4) ? ipv6 : + TextUtils.isEmpty(ipv6) ? ipv4 : ipv4 + ", " + ipv6; + sb.append("Address = ").append(address).append('\n'); sb.append("DNS = ").append(DEFAULT_DNS).append("\n\n"); sb.append("[Peer]\n"); sb.append("# Mullvad relay = ").append(relay.hostname).append('\n'); diff --git a/app/src/main/java/net/kollnig/missioncontrol/wg/VpnKeyRotationManager.java b/app/src/main/java/net/kollnig/missioncontrol/wg/VpnKeyRotationManager.java index 25603c71..d22c9925 100644 --- a/app/src/main/java/net/kollnig/missioncontrol/wg/VpnKeyRotationManager.java +++ b/app/src/main/java/net/kollnig/missioncontrol/wg/VpnKeyRotationManager.java @@ -347,16 +347,9 @@ private static void commitProviderKey(Context context, WgProfileManager manager, String newPublic, String mullvadDeviceId) throws Exception { SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); - prefs.edit() - .putString(key(provider, "previous_privkey"), previousPrivate) - .putString(key(provider, "previous_address"), - currentAddress(manager.getProviderConfig(provider, account))) - .apply(); - long before = dependencies.runtime.now(); boolean activeChanged = manager.rewriteProviderInterface(provider, account, newPrivate, newAddress); if (!activeChanged || !prefs.getBoolean("wg_enabled", false)) { - clearPrevious(prefs, provider); clearPending(prefs, provider); return; } @@ -365,7 +358,6 @@ private static void commitProviderKey(Context context, WgProfileManager manager, dependencies.runtime.sleep(HANDSHAKE_TIMEOUT_MS); Long latest = dependencies.runtime.latestHandshakeMillisOrNull(); if (latest != null && latest >= before) { - clearPrevious(prefs, provider); clearPending(prefs, provider); return; } @@ -379,7 +371,6 @@ private static void rollbackProvider(Context context, WgProfileManager manager, String account, String previousPrivate, String previousPublic, String connectedPublic, String mullvadDeviceId) throws Exception { - SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(context); if (PROVIDER_MULLVAD.equals(provider)) { dependencies.mullvad.rotateDevicePubkey(account, mullvadDeviceId, previousPublic); manager.rewriteProviderInterface(provider, account, previousPrivate, null); @@ -393,19 +384,9 @@ private static void rollbackProvider(Context context, WgProfileManager manager, addressWithCidr(rollback.address)); } dependencies.runtime.reload("vpn provider key rotation rollback", context); - clearPrevious(prefs, provider); throw new RollbackException(label(provider) + " rolled back: missing handshake"); } - private static String currentAddress(String config) { - try { - WgConfig parsed = WgConfigParser.INSTANCE.parse(config); - return TextUtils.join(", ", parsed.getAddress()); - } catch (Throwable ignored) { - return ""; - } - } - private static void storePending(SharedPreferences prefs, String provider, String privateKey, String publicKey) { prefs.edit() @@ -426,18 +407,11 @@ private static void clearPending(SharedPreferences prefs, String provider) { .apply(); } - private static void clearPrevious(SharedPreferences prefs, String provider) { - prefs.edit() - .remove(key(provider, "previous_privkey")) - .remove(key(provider, "previous_address")) - .apply(); - } - private static String addressWithCidr(String address) { String trimmed = address == null ? "" : address.trim(); if (TextUtils.isEmpty(trimmed) || trimmed.contains("/")) return trimmed; - return trimmed + "/32"; + return trimmed + (trimmed.contains(":") ? "/128" : "/32"); } private static String key(String provider, String suffix) { diff --git a/app/src/test/java/eu/faircode/netguard/PreviousKeyPreferenceMigrationTest.java b/app/src/test/java/eu/faircode/netguard/PreviousKeyPreferenceMigrationTest.java new file mode 100644 index 00000000..46574bab --- /dev/null +++ b/app/src/test/java/eu/faircode/netguard/PreviousKeyPreferenceMigrationTest.java @@ -0,0 +1,54 @@ +package eu.faircode.netguard; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +import android.content.SharedPreferences; + +import androidx.preference.PreferenceManager; + +import net.kollnig.missioncontrol.data.BlockingMode; + +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.RuntimeEnvironment; +import org.robolectric.annotation.Config; + +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 36, qualifiers = "en") +public class PreviousKeyPreferenceMigrationTest { + private SharedPreferences prefs; + + @Before + public void setUp() { + prefs = PreferenceManager.getDefaultSharedPreferences(RuntimeEnvironment.getApplication()); + prefs.edit().clear().commit(); + } + + @Test + public void obsoletePreviousKeyPreferencesAreRemoved() { + prefs.edit() + .putString("mullvad_previous_privkey", "old-private") + .putString("mullvad_previous_address", "10.64.0.2/32") + .putString("ivpn_previous_privkey", "old-private") + .putString("ivpn_previous_address", "10.64.0.3/32") + .putString("unrelated_pref", "keep") + .putBoolean("wg_enabled", true) + .putString(BlockingMode.PREF_BLOCKING_MODE, BlockingMode.MODE_STRICT) + .commit(); + + ApplicationEx.migratePreferences(prefs); + + assertFalse(prefs.contains("mullvad_previous_privkey")); + assertFalse(prefs.contains("mullvad_previous_address")); + assertFalse(prefs.contains("ivpn_previous_privkey")); + assertFalse(prefs.contains("ivpn_previous_address")); + assertEquals("keep", prefs.getString("unrelated_pref", "")); + assertTrue(prefs.getBoolean("wg_enabled", false)); + assertEquals(BlockingMode.MODE_STRICT, + prefs.getString(BlockingMode.PREF_BLOCKING_MODE, "")); + } +} diff --git a/app/src/test/java/net/kollnig/missioncontrol/wg/ProfileGeneratorTest.java b/app/src/test/java/net/kollnig/missioncontrol/wg/ProfileGeneratorTest.java new file mode 100644 index 00000000..832338e2 --- /dev/null +++ b/app/src/test/java/net/kollnig/missioncontrol/wg/ProfileGeneratorTest.java @@ -0,0 +1,207 @@ +package net.kollnig.missioncontrol.wg; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +import org.json.JSONObject; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.robolectric.RobolectricTestRunner; +import org.robolectric.annotation.Config; + +import java.io.IOException; +import java.util.Base64; +import java.util.Collections; +import java.util.List; + +@RunWith(RobolectricTestRunner.class) +@Config(sdk = 36, qualifiers = "en") +public class ProfileGeneratorTest { + private static final String ACCOUNT = "test-account"; + private static final String PRIVATE_KEY = key(0); + private static final String PUBLIC_KEY = key(1); + private static final String PEER_KEY = key(9); + private static final String IPV6 = "fc00:bbbb:bbbb:bb01::2/128"; + private static final String IPV4 = "10.64.0.2/32"; + + @Test + public void mullvadRelayFailureDoesNotCreateDevice() throws Exception { + IOException failure = new IOException("relay fetch failed"); + TestMullvadGenerator generator = new TestMullvadGenerator(failure); + + try { + generator.generate(ACCOUNT, "de"); + fail("Expected relay fetch failure"); + } catch (IOException ex) { + assertSame(failure, ex); + } + + assertEquals(0, generator.fetchWebTokenCalls); + assertEquals(0, generator.createDeviceCalls); + } + + @Test + public void ivpnRelayFailureDoesNotCreateSession() throws Exception { + IOException failure = new IOException("relay fetch failed"); + TestIvpnGenerator generator = new TestIvpnGenerator(failure); + + try { + generator.generate(ACCOUNT, "de", null); + fail("Expected relay fetch failure"); + } catch (IOException ex) { + assertSame(failure, ex); + } + + assertEquals(0, generator.createSessionCalls); + } + + @Test + public void mullvadIpv6OnlyReusableConfigHasNoLeadingAddressComma() throws Exception { + TestMullvadGenerator generator = new TestMullvadGenerator(null); + + String config = generator.generate(ACCOUNT, "de", reusableConfig(IPV6)).config; + + assertTrue(config.contains("Address = " + IPV6)); + assertFalse(config.contains("Address = ,")); + } + + @Test + public void mullvadReusableConfigKeepsBothAddresses() throws Exception { + TestMullvadGenerator generator = new TestMullvadGenerator(null); + + String config = generator.generate(ACCOUNT, "de", + reusableConfig(IPV4 + ", " + IPV6)).config; + + assertTrue(config.contains("Address = " + IPV4 + ", " + IPV6)); + } + + @Test + public void ivpnIpv6AddressGetsIpv6Cidr() throws Exception { + TestIvpnGenerator generator = new TestIvpnGenerator(null); + WgProfileManager.IvpnSession session = new WgProfileManager.IvpnSession( + "session", PRIVATE_KEY, PUBLIC_KEY, "fc00:bbbb:bbbb:bb01::2"); + + String config = generator.generate(ACCOUNT, "de", session).config; + + assertTrue(config.contains("Address = fc00:bbbb:bbbb:bb01::2/128")); + } + + private static String reusableConfig(String address) { + return "[Interface]\n" + + "PrivateKey = " + PRIVATE_KEY + "\n" + + "Address = " + address + "\n" + + "DNS = 10.64.0.1\n\n" + + "[Peer]\n" + + "PublicKey = " + PEER_KEY + "\n" + + "AllowedIPs = 0.0.0.0/0, ::/0\n" + + "Endpoint = 198.51.100.1:51820\n"; + } + + private static MullvadProfileGenerator.Relay mullvadRelay() { + MullvadProfileGenerator.Relay relay = new MullvadProfileGenerator.Relay(); + relay.hostname = "de-test-wireguard"; + relay.countryCode = "de"; + relay.countryName = "Germany"; + relay.ipv4 = "198.51.100.1"; + relay.publicKey = PEER_KEY; + relay.speed = 1; + return relay; + } + + private static IvpnProfileGenerator.Relay ivpnRelay() { + IvpnProfileGenerator.Relay relay = new IvpnProfileGenerator.Relay(); + relay.hostname = "de-test-wireguard"; + relay.countryCode = "de"; + relay.countryName = "Germany"; + relay.host = "198.51.100.1"; + relay.publicKey = PEER_KEY; + return relay; + } + + private static String key(int value) { + byte[] bytes = new byte[32]; + java.util.Arrays.fill(bytes, (byte) value); + return Base64.getEncoder().encodeToString(bytes); + } + + private static class TestMullvadGenerator extends MullvadProfileGenerator { + private final IOException relayFailure; + int fetchWebTokenCalls; + int createDeviceCalls; + + TestMullvadGenerator(IOException relayFailure) { + this.relayFailure = relayFailure; + } + + @Override + List fetchRelays() throws Exception { + if (relayFailure != null) + throw relayFailure; + return Collections.singletonList(mullvadRelay()); + } + + @Override + String fetchWebToken(String accountNumber) { + fetchWebTokenCalls++; + return "token"; + } + + @Override + JSONObject createDevice(String token, String publicKey) throws Exception { + createDeviceCalls++; + return new JSONObject() + .put("id", "device") + .put("name", "test") + .put("ipv4_address", IPV4) + .put("ipv6_address", IPV6); + } + + @Override + String newPrivateKey() { + return PRIVATE_KEY; + } + + @Override + String derivePublicKey(String privateKey) { + return PUBLIC_KEY; + } + } + + private static class TestIvpnGenerator extends IvpnProfileGenerator { + private final IOException relayFailure; + int createSessionCalls; + + TestIvpnGenerator(IOException relayFailure) { + this.relayFailure = relayFailure; + } + + @Override + List fetchRelays() throws Exception { + if (relayFailure != null) + throw relayFailure; + return Collections.singletonList(ivpnRelay()); + } + + @Override + WgProfileManager.IvpnSession createSession(String account, String privateKey, + String publicKey, String captchaId, + String captchaValue) { + createSessionCalls++; + return new WgProfileManager.IvpnSession("session", privateKey, publicKey, + "10.64.0.2"); + } + + @Override + String newPrivateKey() { + return PRIVATE_KEY; + } + + @Override + String derivePublicKey(String privateKey) { + return PUBLIC_KEY; + } + } +}