Notice a pinned Private DNS when it happens, and warn in the app too

app/src/main/java/eu/faircode/netguard/ActivityMain.java

@@ -467,6 +467,21 @@ public void onClick(View v) {
             }
         });
 
+        // Private DNS pinned to a hostname while we block DoT: name resolution
+        // fails outright, so the user may never see the notification that says
+        // so — nothing they tap can load anything either.
+        TextView tvPrivateDns = findViewById(R.id.tvPrivateDns);
+        tvPrivateDns.setVisibility(View.GONE);
+        tvPrivateDns.setOnClickListener(new View.OnClickListener() {
+            @Override
+            public void onClick(View v) {
+                Intent settings = new Intent(Settings.ACTION_WIRELESS_SETTINGS);
+                if (settings.resolveActivity(getPackageManager()) == null)
+                    settings = new Intent(Settings.ACTION_WIFI_SETTINGS);
+                startActivity(settings);
+            }
+        });
+
         // Application list
         RecyclerView rvApplication = findViewById(R.id.rvApplication);
         rvApplication.setHasFixedSize(false);
@@ -567,6 +582,16 @@ protected void onResume() {
             tvLocalNetwork.setVisibility(
                     LocalNetworkAccess.isMissing(this) ? View.VISIBLE : View.GONE);
 
+        // Only while we are actually filtering: with the VPN off, port 853 is
+        // not blocked and a pinned resolver works fine.
+        TextView tvPrivateDns = findViewById(R.id.tvPrivateDns);
+        if (tvPrivateDns != null) {
+            boolean vpnEnabled = PreferenceManager.getDefaultSharedPreferences(this)
+                    .getBoolean("enabled", false);
+            tvPrivateDns.setVisibility(
+                    vpnEnabled && Util.isPrivateDnsBlocked(this) ? View.VISIBLE : View.GONE);
+        }
+
         super.onResume();
     }
 

app/src/main/java/eu/faircode/netguard/NetworkReloadPolicy.java

@@ -9,6 +9,7 @@ final class NetworkReloadPolicy {
     static final String REASON_NETWORK_CHANGED = "Network changed";
     static final String REASON_CONNECTED_CHANGED = "Connected state changed";
     static final String REASON_LINK_PROPERTIES_CHANGED = "link properties changed";
+    static final String REASON_PRIVATE_DNS_CHANGED = "private DNS changed";
     static final String REASON_METERED_CHANGED = "Metered state changed";
     static final String REASON_CONNECTIVITY_CHANGED = "connectivity changed";
 
@@ -30,10 +31,17 @@ static String onConnectivityChanged() {
     }
 
     static String onLinkPropertiesChanged(List<?> lastDns, List<?> currentDns,
-                                          boolean compareDns, boolean reloadOnConnectivity) {
+                                          boolean compareDns, boolean reloadOnConnectivity,
+                                          String lastPrivateDns, String currentPrivateDns) {
         if (compareDns ? !same(lastDns, currentDns) : reloadOnConnectivity)
             return REASON_LINK_PROPERTIES_CHANGED;
 
+        // Pinning Private DNS to a hostname leaves the resolver list alone, so
+        // the comparison above never sees it — yet it decides whether blocking
+        // DoT stops name resolution outright, which the user has to be told.
+        if (!Objects.equals(lastPrivateDns, currentPrivateDns))
+            return REASON_PRIVATE_DNS_CHANGED;
+
         return null;
     }
 
@@ -62,6 +70,15 @@ static boolean shouldRestartWireGuard(String reason) {
                 REASON_CONNECTIVITY_CHANGED.equals(reason);
     }
 
+    /**
+     * The same decision across a coalesced burst of callbacks, which keeps only
+     * the last reason. The need for a rebind is sticky: once any reason in the
+     * burst required one, a later reason that does not must not cancel it.
+     */
+    static boolean shouldRestartWireGuard(boolean pendingRestart, String reason) {
+        return pendingRestart || shouldRestartWireGuard(reason);
+    }
+
     static boolean same(List<?> last, List<?> current) {
         if (last == null || current == null || last.size() != current.size())
             return false;

app/src/main/java/eu/faircode/netguard/ServiceSinkhole.java

@@ -119,6 +119,7 @@
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
+import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.locks.ReentrantReadWriteLock;
 import java.util.zip.GZIPInputStream;
 
@@ -1693,7 +1694,7 @@ private Builder getBuilder(List<Rule> listAllowed, List<Rule> listRule) {
         // In "hostname" mode it does not fall back — the user picked that
         // resolver explicitly, so DNS simply fails and nothing resolves. That
         // looks like TC broke the connection, with no hint of why, so say it.
-        if (prefs.getBoolean("block_dot", true) && Util.isPrivateDnsHostnameMode(this)) {
+        if (Util.isPrivateDnsBlocked(this)) {
             Log.w(TAG, "Private DNS set to a hostname: DoT is blocked and Android will not" +
                     " fall back to plaintext DNS, so name resolution fails");
             showPrivateDnsNotification(Util.getPrivateDnsSpecifier(this));
@@ -3146,6 +3147,7 @@ private void listenNetworkChanges() {
             private Boolean last_connected = null;
             private Boolean last_metered = null;
             private List<InetAddress> last_dns = null;
+            private String last_private_dns = null;
 
             @Override
             public void onAvailable(Network network) {
@@ -3168,17 +3170,26 @@ public void onLinkPropertiesChanged(Network network, LinkProperties linkProperti
 
                 // Make sure the right DNS servers are being used
                 List<InetAddress> dns = linkProperties.getDnsServers();
+                // Non-null only when Private DNS is pinned to a hostname, which
+                // leaves the resolver list untouched — so this is the only part
+                // of the properties that reveals the change.
+                String private_dns = (Build.VERSION.SDK_INT < Build.VERSION_CODES.P
+                        ? null : linkProperties.getPrivateDnsServerName());
                 SharedPreferences prefs = PreferenceManager.getDefaultSharedPreferences(ServiceSinkhole.this);
                 String reason = NetworkReloadPolicy.onLinkPropertiesChanged(
                         last_dns,
                         dns,
                         Build.VERSION.SDK_INT >= Build.VERSION_CODES.O,
-                        prefs.getBoolean("reload_onconnectivity", false));
+                        prefs.getBoolean("reload_onconnectivity", false),
+                        last_private_dns,
+                        private_dns);
                 if (reason != null) {
                     Log.i(TAG, "Changed link properties=" + linkProperties +
                             "DNS cur=" + TextUtils.join(",", dns) +
-                            "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns)));
+                            "DNS prv=" + (last_dns == null ? null : TextUtils.join(",", last_dns)) +
+                            " private DNS cur=" + private_dns + " prv=" + last_private_dns);
                     last_dns = dns;
+                    last_private_dns = private_dns;
                     reloadAfterNetworkChange(reason);
                 }
             }
@@ -3228,19 +3239,24 @@ public void onLost(Network network) {
     // ConnectivityManager callbacks within milliseconds of each other. Each
     // reload is a foreground-service update + wakelock + native VPN restart +
     // WireGuard rebind, so bursts are coalesced into a single reload using the
-    // last reason once the burst settles. Every reason string currently in use
-    // maps to the same shouldRestartWireGuard()==true branch, so collapsing to
-    // the latest reason changes no behaviour beyond the log line.
+    // last reason once the burst settles. Not every reason needs the rebind, so
+    // the need for one is accumulated across the burst rather than read off the
+    // surviving reason: a reason that does not need it must not cancel one that
+    // did, or the tunnel keeps a socket bound to a network that is gone.
     private static final long NETWORK_RELOAD_DEBOUNCE_MS = 1500L;
     private static final Object NETWORK_RELOAD_TOKEN = new Object();
     private final Handler networkReloadDebounceHandler = new Handler(Looper.getMainLooper());
+    private final AtomicBoolean pendingWireGuardRestart = new AtomicBoolean(false);
 
     private void reloadAfterNetworkChange(final String reason) {
+        // Callbacks arrive off the main thread; the reload runs on it.
+        if (NetworkReloadPolicy.shouldRestartWireGuard(reason))
+            pendingWireGuardRestart.set(true);
         networkReloadDebounceHandler.removeCallbacksAndMessages(NETWORK_RELOAD_TOKEN);
         networkReloadDebounceHandler.postAtTime(new Runnable() {
             @Override
             public void run() {
-                if (NetworkReloadPolicy.shouldRestartWireGuard(reason))
+                if (pendingWireGuardRestart.getAndSet(false))
                     net.kollnig.missioncontrol.wg.WgEgress.INSTANCE.onUnderlyingNetworkChanged();
                 reload(reason, ServiceSinkhole.this, false);
             }

app/src/main/java/eu/faircode/netguard/Util.java

@@ -57,6 +57,7 @@
 import androidx.core.app.ActivityCompat;
 import androidx.core.app.NotificationManagerCompat;
 import androidx.core.net.ConnectivityManagerCompat;
+import androidx.preference.PreferenceManager;
 
 import net.kollnig.missioncontrol.BuildConfig;
 import net.kollnig.missioncontrol.R;
@@ -572,6 +573,18 @@ public static String getPrivateDnsSpecifier(Context context) {
         return Settings.Global.getString(context.getContentResolver(), "private_dns_specifier");
     }
 
+    /**
+     * Whether the device is left with no working resolver: Private DNS pinned
+     * to a hostname while we block DoT. Deliberately keyed on the mode rather
+     * than on the resolver name, which is only decoration for the warning —
+     * a name that reads back empty must not turn the warning off.
+     */
+    public static boolean isPrivateDnsBlocked(Context context) {
+        return PreferenceManager.getDefaultSharedPreferences(context)
+                .getBoolean("block_dot", true)
+                && isPrivateDnsHostnameMode(context);
+    }
+
     public interface DoubtListener {
         void onSure();
     }

app/src/main/res/layout/main.xml

@@ -64,6 +64,26 @@
         android:textColor="?attr/colorOff"
         android:visibility="gone" />
 
+    <!--
+        Same reasoning as the row above, and the one place this reaches a user
+        who declined notification permissions: with Private DNS pinned nothing
+        resolves at all, so the notification alone may never be seen.
+    -->
+    <TextView
+        android:id="@+id/tvPrivateDns"
+        android:layout_width="match_parent"
+        android:layout_height="wrap_content"
+        android:drawableStart="@drawable/ic_warning_amber"
+        android:drawablePadding="8dp"
+        android:drawableTint="?attr/colorError"
+        android:padding="8dp"
+        android:paddingStart="@dimen/activity_horizontal_margin"
+        android:paddingEnd="@dimen/activity_horizontal_margin"
+        android:text="@string/msg_private_dns"
+        android:textAppearance="@style/TextSmall"
+        android:textColor="?attr/colorOff"
+        android:visibility="gone" />
+
     <FrameLayout
         android:layout_width="match_parent"
         android:layout_height="0dp"

app/src/test/java/eu/faircode/netguard/NetworkReloadPolicyTest.java

@@ -1,6 +1,7 @@
 package eu.faircode.netguard;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 
@@ -76,7 +77,8 @@ public void dnsChangeReloadsOnModernAndroid() {
                         Collections.singletonList("9.9.9.9"),
                         Collections.singletonList("1.1.1.1"),
                         true,
-                        false));
+                        false,
+                        null, null));
     }
 
     @Test
@@ -85,7 +87,8 @@ public void sameDnsDoesNotReloadOnModernAndroid() {
                 Arrays.asList("9.9.9.9", "149.112.112.112"),
                 Arrays.asList("9.9.9.9", "149.112.112.112"),
                 true,
-                false));
+                false,
+                null, null));
     }
 
     @Test
@@ -95,13 +98,80 @@ public void preOConnectivityPreferenceControlsLinkPropertyReload() {
                         Collections.singletonList("9.9.9.9"),
                         Collections.singletonList("9.9.9.9"),
                         false,
-                        true));
+                        true,
+                        null, null));
 
         assertNull(NetworkReloadPolicy.onLinkPropertiesChanged(
                 Collections.singletonList("9.9.9.9"),
                 Collections.singletonList("1.1.1.1"),
                 false,
-                false));
+                false,
+                null, null));
+    }
+
+    /**
+     * Pinning Private DNS to a hostname leaves the resolver list untouched, so
+     * comparing DNS servers alone never notices it and the warning that DoT is
+     * blocked would not appear until some unrelated network change.
+     */
+    @Test
+    public void privateDnsPinnedReloads() {
+        assertEquals("private DNS changed",
+                NetworkReloadPolicy.onLinkPropertiesChanged(
+                        Collections.singletonList("9.9.9.9"),
+                        Collections.singletonList("9.9.9.9"),
+                        true,
+                        false,
+                        null, "dns.google"));
+    }
+
+    @Test
+    public void privateDnsClearedReloads() {
+        assertEquals("private DNS changed",
+                NetworkReloadPolicy.onLinkPropertiesChanged(
+                        Collections.singletonList("9.9.9.9"),
+                        Collections.singletonList("9.9.9.9"),
+                        true,
+                        false,
+                        "dns.google", null));
+    }
+
+    @Test
+    public void samePrivateDnsDoesNotReload() {
+        assertNull(NetworkReloadPolicy.onLinkPropertiesChanged(
+                Collections.singletonList("9.9.9.9"),
+                Collections.singletonList("9.9.9.9"),
+                true,
+                false,
+                "dns.google", "dns.google"));
+    }
+
+    /**
+     * The tunnel is unaffected by a resolver being pinned, so this reload must
+     * not cost a WireGuard rebind and re-handshake.
+     */
+    @Test
+    public void privateDnsChangeDoesNotRestartWireGuard() {
+        assertFalse(NetworkReloadPolicy.shouldRestartWireGuard("private DNS changed"));
+    }
+
+    /**
+     * A burst of callbacks is collapsed to its last reason, but the rebind it
+     * needs is not a property of that reason alone: a private DNS change
+     * landing right after a genuine network change must not cancel the rebind
+     * that change required, or the tunnel keeps a socket bound to a gone
+     * network until some later event.
+     */
+    @Test
+    public void privateDnsChangeDoesNotCancelAPendingRestart() {
+        boolean pending = NetworkReloadPolicy.shouldRestartWireGuard(false, "Network changed");
+        assertTrue(pending);
+        assertTrue(NetworkReloadPolicy.shouldRestartWireGuard(pending, "private DNS changed"));
+    }
+
+    @Test
+    public void privateDnsChangeAloneStillDoesNotRestartWireGuard() {
+        assertFalse(NetworkReloadPolicy.shouldRestartWireGuard(false, "private DNS changed"));
     }
 
     @Test