Fix WireGuard rotation edge cases (#766)

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()

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<Relay> fetchRelays() throws Exception {
+    List<Relay> 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) {

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<JSONObject> listDevices(String token) throws Exception {
         }
     }
 
-    private List<Relay> fetchRelays() throws Exception {
+    List<Relay> 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');

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) {

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, ""));
+    }
+}