Merge "Move the logic of (re)evaluation of Private DNS"

am: 04233ef175

Change-Id: Ic510dabd6a2d40cd6d33a21e0128d85a0126127f

services/core/java/com/android/server/ConnectivityService.java

@@ -902,7 +902,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
 
     // Used only for testing.
     // TODO: Delete this and either:
-    // 1. Give Fake SettingsProvider the ability to send settings change notifications (requires
+    // 1. Give FakeSettingsProvider the ability to send settings change notifications (requires
     //    changing ContentResolver to make registerContentObserver non-final).
     // 2. Give FakeSettingsProvider an alternative notification mechanism and have the test use it
     //    by subclassing SettingsObserver.
@@ -911,6 +911,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
         mHandler.sendEmptyMessage(EVENT_CONFIGURE_MOBILE_DATA_ALWAYS_ON);
     }
 
+    // See FakeSettingsProvider comment above.
+    @VisibleForTesting
+    void updatePrivateDnsSettings() {
+        mHandler.sendEmptyMessage(EVENT_PRIVATE_DNS_SETTINGS_CHANGED);
+    }
+
     private void handleMobileDataAlwaysOn() {
         final boolean enable = toBool(Settings.Global.getInt(
                 mContext.getContentResolver(), Settings.Global.MOBILE_DATA_ALWAYS_ON, 1));
@@ -940,8 +946,8 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     private void registerPrivateDnsSettingsCallbacks() {
-        for (Uri u : DnsManager.getPrivateDnsSettingsUris()) {
+        for (Uri uri : DnsManager.getPrivateDnsSettingsUris()) {
-            mSettingsObserver.observe(u, EVENT_PRIVATE_DNS_SETTINGS_CHANGED);
+            mSettingsObserver.observe(uri, EVENT_PRIVATE_DNS_SETTINGS_CHANGED);
         }
     }
 
@@ -994,8 +1000,12 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (network == null) {
             return null;
         }
+        return getNetworkAgentInfoForNetId(network.netId);
+    }
+
+    private NetworkAgentInfo getNetworkAgentInfoForNetId(int netId) {
         synchronized (mNetworkForNetId) {
-            return mNetworkForNetId.get(network.netId);
+            return mNetworkForNetId.get(netId);
         }
     }
 
@@ -1135,9 +1145,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
         NetworkAgentInfo nai;
         if (vpnNetId != NETID_UNSET) {
-            synchronized (mNetworkForNetId) {
+            nai = getNetworkAgentInfoForNetId(vpnNetId);
-                nai = mNetworkForNetId.get(vpnNetId);
-            }
             if (nai != null) return nai.network;
         }
         nai = getDefaultNetwork();
@@ -2113,41 +2121,21 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 default:
                     return false;
                 case NetworkMonitor.EVENT_NETWORK_TESTED: {
-                    final NetworkAgentInfo nai;
+                    final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(msg.arg2);
-                    synchronized (mNetworkForNetId) {
-                        nai = mNetworkForNetId.get(msg.arg2);
-                    }
                     if (nai == null) break;
 
                     final boolean valid = (msg.arg1 == NetworkMonitor.NETWORK_TEST_RESULT_VALID);
                     final boolean wasValidated = nai.lastValidated;
                     final boolean wasDefault = isDefaultNetwork(nai);
 
-                    final PrivateDnsConfig privateDnsCfg = (msg.obj instanceof PrivateDnsConfig)
-                            ? (PrivateDnsConfig) msg.obj : null;
                     final String redirectUrl = (msg.obj instanceof String) ? (String) msg.obj : "";
 
-                    final boolean reevaluationRequired;
-                    final String logMsg;
-                    if (valid) {
-                        reevaluationRequired = updatePrivateDns(nai, privateDnsCfg);
-                        logMsg = (DBG && (privateDnsCfg != null))
-                                 ? " with " + privateDnsCfg.toString() : "";
-                    } else {
-                        reevaluationRequired = false;
-                        logMsg = (DBG && !TextUtils.isEmpty(redirectUrl))
-                                 ? " with redirect to " + redirectUrl : "";
-                    }
                     if (DBG) {
+                        final String logMsg = !TextUtils.isEmpty(redirectUrl)
+                                 ? " with redirect to " + redirectUrl
+                                 : "";
                         log(nai.name() + " validation " + (valid ? "passed" : "failed") + logMsg);
                     }
-                    // If there is a change in Private DNS configuration,
-                    // trigger reevaluation of the network to test it.
-                    if (reevaluationRequired) {
-                        nai.networkMonitor.sendMessage(
-                                NetworkMonitor.CMD_FORCE_REEVALUATION, Process.SYSTEM_UID);
-                        break;
-                    }
                     if (valid != nai.lastValidated) {
                         if (wasDefault) {
                             metricsLogger().defaultNetworkMetrics().logDefaultNetworkValidity(
@@ -2176,10 +2164,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 case NetworkMonitor.EVENT_PROVISIONING_NOTIFICATION: {
                     final int netId = msg.arg2;
                     final boolean visible = toBool(msg.arg1);
-                    final NetworkAgentInfo nai;
+                    final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(netId);
-                    synchronized (mNetworkForNetId) {
-                        nai = mNetworkForNetId.get(netId);
-                    }
                     // If captive portal status has changed, update capabilities or disconnect.
                     if (nai != null && (visible != nai.lastCaptivePortalDetected)) {
                         final int oldScore = nai.getCurrentScore();
@@ -2210,18 +2195,10 @@ public class ConnectivityService extends IConnectivityManager.Stub
                     break;
                 }
                 case NetworkMonitor.EVENT_PRIVATE_DNS_CONFIG_RESOLVED: {
-                    final NetworkAgentInfo nai;
+                    final NetworkAgentInfo nai = getNetworkAgentInfoForNetId(msg.arg2);
-                    synchronized (mNetworkForNetId) {
-                        nai = mNetworkForNetId.get(msg.arg2);
-                    }
                     if (nai == null) break;
 
-                    final PrivateDnsConfig cfg = (PrivateDnsConfig) msg.obj;
+                    updatePrivateDns(nai, (PrivateDnsConfig) msg.obj);
-                    final boolean reevaluationRequired = updatePrivateDns(nai, cfg);
-                    if (nai.lastValidated && reevaluationRequired) {
-                        nai.networkMonitor.sendMessage(
-                                NetworkMonitor.CMD_FORCE_REEVALUATION, Process.SYSTEM_UID);
-                    }
                     break;
                 }
             }
@@ -2259,61 +2236,38 @@ public class ConnectivityService extends IConnectivityManager.Stub
         }
     }
 
+    private boolean networkRequiresValidation(NetworkAgentInfo nai) {
+        return NetworkMonitor.isValidationRequired(
+                mDefaultRequest.networkCapabilities, nai.networkCapabilities);
+    }
+
     private void handlePrivateDnsSettingsChanged() {
         final PrivateDnsConfig cfg = mDnsManager.getPrivateDnsConfig();
 
         for (NetworkAgentInfo nai : mNetworkAgentInfos.values()) {
-            // Private DNS only ever applies to networks that might provide
+            handlePerNetworkPrivateDnsConfig(nai, cfg);
-            // Internet access and therefore also require validation.
-            if (!NetworkMonitor.isValidationRequired(
-                    mDefaultRequest.networkCapabilities, nai.networkCapabilities)) {
-                continue;
-            }
-
-            // Notify the NetworkMonitor thread in case it needs to cancel or
-            // schedule DNS resolutions. If a DNS resolution is required the
-            // result will be sent back to us.
-            nai.networkMonitor.notifyPrivateDnsSettingsChanged(cfg);
-
-            if (!cfg.inStrictMode()) {
-                // No strict mode hostname DNS resolution needed, so just update
-                // DNS settings directly. In opportunistic and "off" modes this
-                // just reprograms netd with the network-supplied DNS servers
-                // (and of course the boolean of whether or not to attempt TLS).
-                //
-                // TODO: Consider code flow parity with strict mode, i.e. having
-                // NetworkMonitor relay the PrivateDnsConfig back to us and then
-                // performing this call at that time.
-                updatePrivateDns(nai, cfg);
-            }
         }
     }
 
-    private boolean updatePrivateDns(NetworkAgentInfo nai, PrivateDnsConfig newCfg) {
+    private void handlePerNetworkPrivateDnsConfig(NetworkAgentInfo nai, PrivateDnsConfig cfg) {
-        final boolean reevaluationRequired = true;
+        // Private DNS only ever applies to networks that might provide
-        final boolean dontReevaluate = false;
+        // Internet access and therefore also require validation.
+        if (!networkRequiresValidation(nai)) return;
 
-        final PrivateDnsConfig oldCfg = mDnsManager.updatePrivateDns(nai.network, newCfg);
+        // Notify the NetworkMonitor thread in case it needs to cancel or
+        // schedule DNS resolutions. If a DNS resolution is required the
+        // result will be sent back to us.
+        nai.networkMonitor.notifyPrivateDnsSettingsChanged(cfg);
+
+        // With Private DNS bypass support, we can proceed to update the
+        // Private DNS config immediately, even if we're in strict mode
+        // and have not yet resolved the provider name into a set of IPs.
+        updatePrivateDns(nai, cfg);
+    }
+
+    private void updatePrivateDns(NetworkAgentInfo nai, PrivateDnsConfig newCfg) {
+        mDnsManager.updatePrivateDns(nai.network, newCfg);
         updateDnses(nai.linkProperties, null, nai.network.netId);
-
-        if (newCfg == null) {
-            if (oldCfg == null) return dontReevaluate;
-            return oldCfg.useTls ? reevaluationRequired : dontReevaluate;
-        }
-
-        if (oldCfg == null) {
-            return newCfg.useTls ? reevaluationRequired : dontReevaluate;
-        }
-
-        if (oldCfg.useTls != newCfg.useTls) {
-            return reevaluationRequired;
-        }
-
-        if (newCfg.inStrictMode() && !Objects.equals(oldCfg.hostname, newCfg.hostname)) {
-            return reevaluationRequired;
-        }
-
-        return dontReevaluate;
     }
 
     private void updateLingerState(NetworkAgentInfo nai, long now) {
@@ -3252,7 +3206,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
         if (isNetworkWithLinkPropertiesBlocked(lp, uid, false)) {
             return;
         }
-        nai.networkMonitor.sendMessage(NetworkMonitor.CMD_FORCE_REEVALUATION, uid);
+        nai.networkMonitor.forceReevaluation(uid);
     }
 
     private ProxyInfo getDefaultProxy() {
@@ -4871,7 +4825,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
     }
 
     public void handleUpdateLinkProperties(NetworkAgentInfo nai, LinkProperties newLp) {
-        if (mNetworkForNetId.get(nai.network.netId) != nai) {
+        if (getNetworkAgentInfoForNetId(nai.network.netId) != nai) {
             // Ignore updates for disconnected networks
             return;
         }
@@ -5451,6 +5405,7 @@ public class ConnectivityService extends IConnectivityManager.Stub
                 Slog.wtf(TAG, networkAgent.name() + " connected with null LinkProperties");
             }
 
+            handlePerNetworkPrivateDnsConfig(networkAgent, mDnsManager.getPrivateDnsConfig());
             updateLinkProperties(networkAgent, null);
 
             networkAgent.networkMonitor.sendMessage(NetworkMonitor.CMD_NETWORK_CONNECTED);

services/core/java/com/android/server/connectivity/DnsManager.java

@@ -61,6 +61,51 @@ import java.util.StringJoiner;
  * This class it NOT designed for concurrent access. Furthermore, all non-static
  * methods MUST be called from ConnectivityService's thread.
  *
+ * [ Private DNS ]
+ * The code handling Private DNS is spread across several components, but this
+ * seems like the least bad place to collect all the observations.
+ *
+ * Private DNS handling and updating occurs in response to several different
+ * events. Each is described here with its corresponding intended handling.
+ *
+ * [A] Event: A new network comes up.
+ * Mechanics:
+ *     [1] ConnectivityService gets notifications from NetworkAgents.
+ *     [2] in updateNetworkInfo(), the first time the NetworkAgent goes into
+ *         into CONNECTED state, the Private DNS configuration is retrieved,
+ *         programmed, and strict mode hostname resolution (if applicable) is
+ *         enqueued in NetworkAgent's NetworkMonitor, via a call to
+ *         handlePerNetworkPrivateDnsConfig().
+ *     [3] Re-resolution of strict mode hostnames that fail to return any
+ *         IP addresses happens inside NetworkMonitor; it sends itself a
+ *         delayed CMD_EVALUATE_PRIVATE_DNS message in a simple backoff
+ *         schedule.
+ *     [4] Successfully resolved hostnames are sent to ConnectivityService
+ *         inside an EVENT_PRIVATE_DNS_CONFIG_RESOLVED message. The resolved
+ *         IP addresses are programmed into netd via:
+ *
+ *             updatePrivateDns() -> updateDnses()
+ *
+ *         both of which make calls into DnsManager.
+ *     [5] Upon a successful hostname resolution NetworkMonitor initiates a
+ *         validation attempt in the form of a lookup for a one-time hostname
+ *         that uses Private DNS.
+ *
+ * [B] Event: Private DNS settings are changed.
+ * Mechanics:
+ *     [1] ConnectivityService gets notifications from its SettingsObserver.
+ *     [2] handlePrivateDnsSettingsChanged() is called, which calls
+ *         handlePerNetworkPrivateDnsConfig() and the process proceeds
+ *         as if from A.3 above.
+ *
+ * [C] Event: An application calls ConnectivityManager#reportBadNetwork().
+ * Mechanics:
+ *     [1] NetworkMonitor is notified and initiates a reevaluation, which
+ *         always bypasses Private DNS.
+ *     [2] Once completed, NetworkMonitor checks if strict mode is in operation
+ *         and if so enqueues another evaluation of Private DNS, as if from
+ *         step A.5 above.
+ *
  * @hide
  */
 public class DnsManager {

services/core/java/com/android/server/connectivity/NetworkMonitor.java

@@ -34,6 +34,7 @@ import android.net.NetworkRequest;
 import android.net.ProxyInfo;
 import android.net.TrafficStats;
 import android.net.Uri;
+import android.net.dns.ResolvUtil;
 import android.net.metrics.IpConnectivityLog;
 import android.net.metrics.NetworkEvent;
 import android.net.metrics.ValidationProbeEvent;
@@ -64,6 +65,7 @@ import com.android.internal.annotations.VisibleForTesting;
 import com.android.internal.util.Protocol;
 import com.android.internal.util.State;
 import com.android.internal.util.StateMachine;
+import com.android.server.connectivity.DnsManager.PrivateDnsConfig;
 
 import java.io.IOException;
 import java.net.HttpURLConnection;
@@ -77,6 +79,7 @@ import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.List;
 import java.util.Random;
+import java.util.UUID;
 import java.util.concurrent.CountDownLatch;
 import java.util.concurrent.TimeUnit;
 
@@ -165,7 +168,7 @@ public class NetworkMonitor extends StateMachine {
      * Force evaluation even if it has succeeded in the past.
      * arg1 = UID responsible for requesting this reeval.  Will be billed for data.
      */
-    public static final int CMD_FORCE_REEVALUATION = BASE + 8;
+    private static final int CMD_FORCE_REEVALUATION = BASE + 8;
 
     /**
      * Message to self indicating captive portal app finished.
@@ -205,9 +208,15 @@ public class NetworkMonitor extends StateMachine {
      * Private DNS. If a DNS resolution is required, e.g. for DNS-over-TLS in
      * strict mode, then an event is sent back to ConnectivityService with the
      * result of the resolution attempt.
+     *
+     * A separate message is used to trigger (re)evaluation of the Private DNS
+     * configuration, so that the message can be handled as needed in different
+     * states, including being ignored until after an ongoing captive portal
+     * validation phase is completed.
      */
     private static final int CMD_PRIVATE_DNS_SETTINGS_CHANGED = BASE + 13;
     public static final int EVENT_PRIVATE_DNS_CONFIG_RESOLVED = BASE + 14;
+    private static final int CMD_EVALUATE_PRIVATE_DNS = BASE + 15;
 
     // Start mReevaluateDelayMs at this value and double.
     private static final int INITIAL_REEVALUATE_DELAY_MS = 1000;
@@ -215,6 +224,7 @@ public class NetworkMonitor extends StateMachine {
     // Before network has been evaluated this many times, ignore repeated reevaluate requests.
     private static final int IGNORE_REEVALUATE_ATTEMPTS = 5;
     private int mReevaluateToken = 0;
+    private static final int NO_UID = 0;
     private static final int INVALID_UID = -1;
     private int mUidResponsibleForReeval = INVALID_UID;
     // Stop blaming UID that requested re-evaluation after this many attempts.
@@ -224,6 +234,8 @@ public class NetworkMonitor extends StateMachine {
 
     private static final int NUM_VALIDATION_LOG_LINES = 20;
 
+    private String mPrivateDnsProviderHostname = "";
+
     public static boolean isValidationRequired(
             NetworkCapabilities dfltNetCap, NetworkCapabilities nc) {
         // TODO: Consider requiring validation for DUN networks.
@@ -261,13 +273,12 @@ public class NetworkMonitor extends StateMachine {
 
     public boolean systemReady = false;
 
-    private DnsManager.PrivateDnsConfig mPrivateDnsCfg = null;
-
     private final State mDefaultState = new DefaultState();
     private final State mValidatedState = new ValidatedState();
     private final State mMaybeNotifyState = new MaybeNotifyState();
     private final State mEvaluatingState = new EvaluatingState();
     private final State mCaptivePortalState = new CaptivePortalState();
+    private final State mEvaluatingPrivateDnsState = new EvaluatingPrivateDnsState();
 
     private CustomIntentReceiver mLaunchCaptivePortalAppBroadcastReceiver = null;
 
@@ -293,6 +304,10 @@ public class NetworkMonitor extends StateMachine {
         // Add suffix indicating which NetworkMonitor we're talking about.
         super(TAG + networkAgentInfo.name());
 
+        // Logs with a tag of the form given just above, e.g.
+        //     <timestamp>   862  2402 D NetworkMonitor/NetworkAgentInfo [WIFI () - 100]: ...
+        setDbg(VDBG);
+
         mContext = context;
         mMetricsLog = logger;
         mConnectivityServiceHandler = handler;
@@ -305,10 +320,11 @@ public class NetworkMonitor extends StateMachine {
         mDefaultRequest = defaultRequest;
 
         addState(mDefaultState);
-        addState(mValidatedState, mDefaultState);
         addState(mMaybeNotifyState, mDefaultState);
             addState(mEvaluatingState, mMaybeNotifyState);
             addState(mCaptivePortalState, mMaybeNotifyState);
+        addState(mEvaluatingPrivateDnsState, mDefaultState);
+        addState(mValidatedState, mDefaultState);
         setInitialState(mDefaultState);
 
         mIsCaptivePortalCheckEnabled = getIsCaptivePortalCheckEnabled();
@@ -321,6 +337,17 @@ public class NetworkMonitor extends StateMachine {
         start();
     }
 
+    public void forceReevaluation(int responsibleUid) {
+        sendMessage(CMD_FORCE_REEVALUATION, responsibleUid, 0);
+    }
+
+    public void notifyPrivateDnsSettingsChanged(PrivateDnsConfig newCfg) {
+        // Cancel any outstanding resolutions.
+        removeMessages(CMD_PRIVATE_DNS_SETTINGS_CHANGED);
+        // Send the update to the proper thread.
+        sendMessage(CMD_PRIVATE_DNS_SETTINGS_CHANGED, newCfg);
+    }
+
     @Override
     protected void log(String s) {
         if (DBG) Log.d(TAG + "/" + mNetworkAgentInfo.name(), s);
@@ -349,6 +376,12 @@ public class NetworkMonitor extends StateMachine {
                 mDefaultRequest.networkCapabilities, mNetworkAgentInfo.networkCapabilities);
     }
 
+
+    private void notifyNetworkTestResultInvalid(Object obj) {
+        mConnectivityServiceHandler.sendMessage(obtainMessage(
+                EVENT_NETWORK_TESTED, NETWORK_TEST_RESULT_INVALID, mNetId, obj));
+    }
+
     // DefaultState is the parent of all States.  It exists only to handle CMD_* messages but
     // does not entail any real state (hence no enter() or exit() routines).
     private class DefaultState extends State {
@@ -392,41 +425,66 @@ public class NetworkMonitor extends StateMachine {
 
                     switch (message.arg1) {
                         case APP_RETURN_DISMISSED:
-                            sendMessage(CMD_FORCE_REEVALUATION, 0 /* no UID */, 0);
+                            sendMessage(CMD_FORCE_REEVALUATION, NO_UID, 0);
                             break;
                         case APP_RETURN_WANTED_AS_IS:
                             mDontDisplaySigninNotification = true;
                             // TODO: Distinguish this from a network that actually validates.
-                            // Displaying the "!" on the system UI icon may still be a good idea.
+                            // Displaying the "x" on the system UI icon may still be a good idea.
-                            transitionTo(mValidatedState);
+                            transitionTo(mEvaluatingPrivateDnsState);
                             break;
                         case APP_RETURN_UNWANTED:
                             mDontDisplaySigninNotification = true;
                             mUserDoesNotWant = true;
-                            mConnectivityServiceHandler.sendMessage(obtainMessage(
+                            notifyNetworkTestResultInvalid(null);
-                                    EVENT_NETWORK_TESTED, NETWORK_TEST_RESULT_INVALID,
-                                    mNetId, null));
                             // TODO: Should teardown network.
                             mUidResponsibleForReeval = 0;
                             transitionTo(mEvaluatingState);
                             break;
                     }
                     return HANDLED;
-                case CMD_PRIVATE_DNS_SETTINGS_CHANGED:
+                case CMD_PRIVATE_DNS_SETTINGS_CHANGED: {
-                    if (isValidationRequired()) {
+                    final PrivateDnsConfig cfg = (PrivateDnsConfig) message.obj;
-                        // This performs a blocking DNS resolution of the
+                    if (!isValidationRequired() || cfg == null || !cfg.inStrictMode()) {
-                        // strict mode hostname, if required.
+                        // No DNS resolution required.
-                        resolvePrivateDnsConfig((DnsManager.PrivateDnsConfig) message.obj);
+                        //
-                        if ((mPrivateDnsCfg != null) && mPrivateDnsCfg.inStrictMode()) {
+                        // We don't force any validation in opportunistic mode
-                            mConnectivityServiceHandler.sendMessage(obtainMessage(
+                        // here. Opportunistic mode nameservers are validated
-                                    EVENT_PRIVATE_DNS_CONFIG_RESOLVED, 0, mNetId,
+                        // separately within netd.
-                                    new DnsManager.PrivateDnsConfig(mPrivateDnsCfg)));
+                        //
-                        }
+                        // Reset Private DNS settings state.
+                        mPrivateDnsProviderHostname = "";
+                        break;
                     }
-                    return HANDLED;
+
+                    mPrivateDnsProviderHostname = cfg.hostname;
+
+                    // DNS resolutions via Private DNS strict mode block for a
+                    // few seconds (~4.2) checking for any IP addresses to
+                    // arrive and validate. Initiating a (re)evaluation now
+                    // should not significantly alter the validation outcome.
+                    //
+                    // No matter what: enqueue a validation request; one of
+                    // three things can happen with this request:
+                    //     [1] ignored (EvaluatingState or CaptivePortalState)
+                    //     [2] transition to EvaluatingPrivateDnsState
+                    //         (DefaultState and ValidatedState)
+                    //     [3] handled (EvaluatingPrivateDnsState)
+                    //
+                    // The Private DNS configuration to be evaluated will:
+                    //     [1] be skipped (not in strict mode), or
+                    //     [2] validate (huzzah), or
+                    //     [3] encounter some problem (invalid hostname,
+                    //         no resolved IP addresses, IPs unreachable,
+                    //         port 853 unreachable, port 853 is not running a
+                    //         DNS-over-TLS server, et cetera).
+                    sendMessage(CMD_EVALUATE_PRIVATE_DNS);
+                    break;
+                }
                 default:
-                    return HANDLED;
+                    break;
             }
+            return HANDLED;
         }
     }
 
@@ -440,7 +498,7 @@ public class NetworkMonitor extends StateMachine {
             maybeLogEvaluationResult(
                     networkEventType(validationStage(), EvaluationResult.VALIDATED));
             mConnectivityServiceHandler.sendMessage(obtainMessage(EVENT_NETWORK_TESTED,
-                    NETWORK_TEST_RESULT_VALID, mNetId, mPrivateDnsCfg));
+                    NETWORK_TEST_RESULT_VALID, mNetId, null));
             mValidations++;
         }
 
@@ -449,10 +507,14 @@ public class NetworkMonitor extends StateMachine {
             switch (message.what) {
                 case CMD_NETWORK_CONNECTED:
                     transitionTo(mValidatedState);
-                    return HANDLED;
+                    break;
+                case CMD_EVALUATE_PRIVATE_DNS:
+                    transitionTo(mEvaluatingPrivateDnsState);
+                    break;
                 default:
                     return NOT_HANDLED;
             }
+            return HANDLED;
         }
     }
 
@@ -569,11 +631,11 @@ public class NetworkMonitor extends StateMachine {
                 case CMD_REEVALUATE:
                     if (message.arg1 != mReevaluateToken || mUserDoesNotWant)
                         return HANDLED;
-                    // Don't bother validating networks that don't satisify the default request.
+                    // Don't bother validating networks that don't satisfy the default request.
                     // This includes:
                     //  - VPNs which can be considered explicitly desired by the user and the
                     //    user's desire trumps whether the network validates.
-                    //  - Networks that don't provide internet access.  It's unclear how to
+                    //  - Networks that don't provide Internet access.  It's unclear how to
                     //    validate such networks.
                     //  - Untrusted networks.  It's unsafe to prompt the user to sign-in to
                     //    such networks and the user didn't express interest in connecting to
@@ -588,7 +650,6 @@ public class NetworkMonitor extends StateMachine {
                     //    expensive metered network, or unwanted leaking of the User Agent string.
                     if (!isValidationRequired()) {
                         validationLog("Network would not satisfy default request, not validating");
-                        mPrivateDnsCfg = null;
                         transitionTo(mValidatedState);
                         return HANDLED;
                     }
@@ -601,20 +662,18 @@ public class NetworkMonitor extends StateMachine {
                     // if this is found to cause problems.
                     CaptivePortalProbeResult probeResult = isCaptivePortal();
                     if (probeResult.isSuccessful()) {
-                        resolvePrivateDnsConfig();
+                        // Transit EvaluatingPrivateDnsState to get to Validated
-                        transitionTo(mValidatedState);
+                        // state (even if no Private DNS validation required).
+                        transitionTo(mEvaluatingPrivateDnsState);
                     } else if (probeResult.isPortal()) {
-                        mConnectivityServiceHandler.sendMessage(obtainMessage(EVENT_NETWORK_TESTED,
+                        notifyNetworkTestResultInvalid(probeResult.redirectUrl);
-                                NETWORK_TEST_RESULT_INVALID, mNetId, probeResult.redirectUrl));
                         mLastPortalProbeResult = probeResult;
                         transitionTo(mCaptivePortalState);
                     } else {
                         final Message msg = obtainMessage(CMD_REEVALUATE, ++mReevaluateToken, 0);
                         sendMessageDelayed(msg, mReevaluateDelayMs);
                         logNetworkEvent(NetworkEvent.NETWORK_VALIDATION_FAILED);
-                        mConnectivityServiceHandler.sendMessage(obtainMessage(
+                        notifyNetworkTestResultInvalid(probeResult.redirectUrl);
-                                EVENT_NETWORK_TESTED, NETWORK_TEST_RESULT_INVALID, mNetId,
-                                probeResult.redirectUrl));
                         if (mAttempts >= BLAME_FOR_EVALUATION_ATTEMPTS) {
                             // Don't continue to blame UID forever.
                             TrafficStats.clearThreadStatsUid();
@@ -700,6 +759,110 @@ public class NetworkMonitor extends StateMachine {
         }
     }
 
+    private class EvaluatingPrivateDnsState extends State {
+        private int mPrivateDnsReevalDelayMs;
+        private PrivateDnsConfig mPrivateDnsConfig;
+
+        @Override
+        public void enter() {
+            mPrivateDnsReevalDelayMs = INITIAL_REEVALUATE_DELAY_MS;
+            mPrivateDnsConfig = null;
+            sendMessage(CMD_EVALUATE_PRIVATE_DNS);
+        }
+
+        @Override
+        public boolean processMessage(Message msg) {
+            switch (msg.what) {
+                case CMD_EVALUATE_PRIVATE_DNS:
+                    if (inStrictMode()) {
+                        if (!isStrictModeHostnameResolved()) {
+                            resolveStrictModeHostname();
+
+                            if (isStrictModeHostnameResolved()) {
+                                notifyPrivateDnsConfigResolved();
+                            } else {
+                                handlePrivateDnsEvaluationFailure();
+                                break;
+                            }
+                        }
+
+                        // Look up a one-time hostname, to bypass caching.
+                        //
+                        // Note that this will race with ConnectivityService
+                        // code programming the DNS-over-TLS server IP addresses
+                        // into netd (if invoked, above). If netd doesn't know
+                        // the IP addresses yet, or if the connections to the IP
+                        // addresses haven't yet been validated, netd will block
+                        // for up to a few seconds before failing the lookup.
+                        if (!sendPrivateDnsProbe()) {
+                            handlePrivateDnsEvaluationFailure();
+                            break;
+                        }
+                    }
+
+                    // All good!
+                    transitionTo(mValidatedState);
+                    break;
+                default:
+                    return NOT_HANDLED;
+            }
+            return HANDLED;
+        }
+
+        private boolean inStrictMode() {
+            return !TextUtils.isEmpty(mPrivateDnsProviderHostname);
+        }
+
+        private boolean isStrictModeHostnameResolved() {
+            return (mPrivateDnsConfig != null) &&
+                   mPrivateDnsConfig.hostname.equals(mPrivateDnsProviderHostname) &&
+                   (mPrivateDnsConfig.ips.length > 0);
+        }
+
+        private void resolveStrictModeHostname() {
+            try {
+                // Do a blocking DNS resolution using the network-assigned nameservers.
+                mPrivateDnsConfig = new PrivateDnsConfig(
+                        mPrivateDnsProviderHostname,
+                        mNetwork.getAllByName(mPrivateDnsProviderHostname));
+            } catch (UnknownHostException uhe) {
+                mPrivateDnsConfig = null;
+            }
+        }
+
+        private void notifyPrivateDnsConfigResolved() {
+            mConnectivityServiceHandler.sendMessage(obtainMessage(
+                    EVENT_PRIVATE_DNS_CONFIG_RESOLVED, 0, mNetId, mPrivateDnsConfig));
+        }
+
+        private void handlePrivateDnsEvaluationFailure() {
+            notifyNetworkTestResultInvalid(null);
+
+            // Queue up a re-evaluation with backoff.
+            //
+            // TODO: Consider abandoning this state after a few attempts and
+            // transitioning back to EvaluatingState, to perhaps give ourselves
+            // the opportunity to (re)detect a captive portal or something.
+            sendMessageDelayed(CMD_EVALUATE_PRIVATE_DNS, mPrivateDnsReevalDelayMs);
+            mPrivateDnsReevalDelayMs *= 2;
+            if (mPrivateDnsReevalDelayMs > MAX_REEVALUATE_DELAY_MS) {
+                mPrivateDnsReevalDelayMs = MAX_REEVALUATE_DELAY_MS;
+            }
+        }
+
+        private boolean sendPrivateDnsProbe() {
+            // q.v. system/netd/server/dns/DnsTlsTransport.cpp
+            final String ONE_TIME_HOSTNAME_SUFFIX = "-dnsotls-ds.metric.gstatic.com";
+            final String host = UUID.randomUUID().toString().substring(0, 8) +
+                    ONE_TIME_HOSTNAME_SUFFIX;
+            try {
+                final InetAddress[] ips = mNetworkAgentInfo.network().getAllByName(host);
+                return (ips != null && ips.length > 0);
+            } catch (UnknownHostException uhe) {}
+            return false;
+        }
+    }
+
     // Limits the list of IP addresses returned by getAllByName or tried by openConnection to at
     // most one per address family. This ensures we only wait up to 20 seconds for TCP connections
     // to complete, regardless of how many IP addresses a host has.
@@ -710,7 +873,9 @@ public class NetworkMonitor extends StateMachine {
 
         @Override
         public InetAddress[] getAllByName(String host) throws UnknownHostException {
-            List<InetAddress> addrs = Arrays.asList(super.getAllByName(host));
+            // Always bypass Private DNS.
+            final List<InetAddress> addrs = Arrays.asList(
+                    ResolvUtil.blockingResolveAllLocally(this, host));
 
             // Ensure the address family of the first address is tried first.
             LinkedHashMap<Class, InetAddress> addressByFamily = new LinkedHashMap<>();
@@ -1065,44 +1230,6 @@ public class NetworkMonitor extends StateMachine {
         return null;
     }
 
-    public void notifyPrivateDnsSettingsChanged(DnsManager.PrivateDnsConfig newCfg) {
-        // Cancel any outstanding resolutions.
-        removeMessages(CMD_PRIVATE_DNS_SETTINGS_CHANGED);
-        // Send the update to the proper thread.
-        sendMessage(CMD_PRIVATE_DNS_SETTINGS_CHANGED, newCfg);
-    }
-
-    private void resolvePrivateDnsConfig() {
-        resolvePrivateDnsConfig(DnsManager.getPrivateDnsConfig(mContext.getContentResolver()));
-    }
-
-    private void resolvePrivateDnsConfig(DnsManager.PrivateDnsConfig cfg) {
-        // Nothing to do.
-        if (cfg == null) {
-            mPrivateDnsCfg = null;
-            return;
-        }
-
-        // No DNS resolution required.
-        if (!cfg.inStrictMode()) {
-            mPrivateDnsCfg = cfg;
-            return;
-        }
-
-        if ((mPrivateDnsCfg != null) && mPrivateDnsCfg.inStrictMode() &&
-                (mPrivateDnsCfg.ips.length > 0) && mPrivateDnsCfg.hostname.equals(cfg.hostname)) {
-            // We have already resolved this strict mode hostname. Assume that
-            // Private DNS services won't be changing serving IP addresses very
-            // frequently and save ourselves one re-resolve.
-            return;
-        }
-
-        mPrivateDnsCfg = cfg;
-        final DnsManager.PrivateDnsConfig resolvedCfg = DnsManager.tryBlockingResolveOf(
-                mNetwork, mPrivateDnsCfg.hostname);
-        if (resolvedCfg != null) mPrivateDnsCfg = resolvedCfg;
-    }
-
     /**
      * @param responseReceived - whether or not we received a valid HTTP response to our request.
      * If false, isCaptivePortal and responseTimestampMs are ignored

tests/net/java/com/android/server/ConnectivityServiceTest.java

@@ -17,6 +17,9 @@
 package com.android.server;
 
 import static android.net.ConnectivityManager.CONNECTIVITY_ACTION;
+import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OFF;
+import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_OPPORTUNISTIC;
+import static android.net.ConnectivityManager.PRIVATE_DNS_MODE_PROVIDER_HOSTNAME;
 import static android.net.ConnectivityManager.TYPE_ETHERNET;
 import static android.net.ConnectivityManager.TYPE_MOBILE;
 import static android.net.ConnectivityManager.TYPE_MOBILE_FOTA;
@@ -70,6 +73,7 @@ import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.spy;
 import static org.mockito.Mockito.times;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 
@@ -181,6 +185,9 @@ public class ConnectivityServiceTest {
     private static final int TIMEOUT_MS = 500;
     private static final int TEST_LINGER_DELAY_MS = 120;
 
+    private static final String MOBILE_IFNAME = "test_rmnet_data0";
+    private static final String WIFI_IFNAME = "test_wlan0";
+
     private MockContext mServiceContext;
     private WrappedConnectivityService mService;
     private WrappedConnectivityManager mCm;
@@ -751,7 +758,7 @@ public class ConnectivityServiceTest {
 
     // NetworkMonitor implementation allowing overriding of Internet connectivity probe result.
     private class WrappedNetworkMonitor extends NetworkMonitor {
-        public Handler connectivityHandler;
+        public final Handler connectivityHandler;
         // HTTP response code fed back to NetworkMonitor for Internet connectivity probe.
         public int gen204ProbeResult = 500;
         public String gen204ProbeRedirectUrl = null;
@@ -926,6 +933,7 @@ public class ConnectivityServiceTest {
         // Ensure that the default setting for Captive Portals is used for most tests
         setCaptivePortalMode(Settings.Global.CAPTIVE_PORTAL_MODE_PROMPT);
         setMobileDataAlwaysOn(false);
+        setPrivateDnsSettings(PRIVATE_DNS_MODE_OFF, "ignored.example.com");
     }
 
     @After
@@ -2580,6 +2588,14 @@ public class ConnectivityServiceTest {
         waitForIdle();
     }
 
+    private void setPrivateDnsSettings(String mode, String specifier) {
+        final ContentResolver cr = mServiceContext.getContentResolver();
+        Settings.Global.putString(cr, Settings.Global.PRIVATE_DNS_MODE, mode);
+        Settings.Global.putString(cr, Settings.Global.PRIVATE_DNS_SPECIFIER, specifier);
+        mService.updatePrivateDnsSettings();
+        waitForIdle();
+    }
+
     private boolean isForegroundNetwork(MockNetworkAgent network) {
         NetworkCapabilities nc = mCm.getNetworkCapabilities(network.getNetwork());
         assertNotNull(nc);
@@ -3579,7 +3595,7 @@ public class ConnectivityServiceTest {
         mCm.registerNetworkCallback(networkRequest, networkCallback);
 
         LinkProperties lp = new LinkProperties();
-        lp.setInterfaceName("wlan0");
+        lp.setInterfaceName(WIFI_IFNAME);
         LinkAddress myIpv4Address = new LinkAddress("192.168.12.3/24");
         RouteInfo myIpv4DefaultRoute = new RouteInfo((IpPrefix) null,
                 NetworkUtils.numericToInetAddress("192.168.12.1"), lp.getInterfaceName());
@@ -3668,52 +3684,63 @@ public class ConnectivityServiceTest {
 
     @Test
     public void testBasicDnsConfigurationPushed() throws Exception {
-        final String IFNAME = "test_rmnet_data0";
+        setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com");
-        final String[] EMPTY_TLS_SERVERS = new String[0];
+        ArgumentCaptor<String[]> tlsServers = ArgumentCaptor.forClass(String[].class);
+
+        // Clear any interactions that occur as a result of CS starting up.
+        reset(mNetworkManagementService);
+
+        final String[] EMPTY_STRING_ARRAY = new String[0];
         mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
         waitForIdle();
         verify(mNetworkManagementService, never()).setDnsConfigurationForNetwork(
-                anyInt(), any(), any(), any(), anyString(), eq(EMPTY_TLS_SERVERS));
+                anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY));
+        verifyNoMoreInteractions(mNetworkManagementService);
 
         final LinkProperties cellLp = new LinkProperties();
-        cellLp.setInterfaceName(IFNAME);
+        cellLp.setInterfaceName(MOBILE_IFNAME);
         // Add IPv4 and IPv6 default routes, because DNS-over-TLS code does
         // "is-reachable" testing in order to not program netd with unreachable
         // nameservers that it might try repeated to validate.
         cellLp.addLinkAddress(new LinkAddress("192.0.2.4/24"));
-        cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("192.0.2.4"), IFNAME));
+        cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("192.0.2.4"),
+                MOBILE_IFNAME));
         cellLp.addLinkAddress(new LinkAddress("2001:db8:1::1/64"));
-        cellLp.addRoute(
+        cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("2001:db8:1::1"),
-                new RouteInfo((IpPrefix) null, InetAddress.getByName("2001:db8:1::1"), IFNAME));
+                MOBILE_IFNAME));
         mCellNetworkAgent.sendLinkProperties(cellLp);
         mCellNetworkAgent.connect(false);
         waitForIdle();
-        verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork(
-                anyInt(), mStringArrayCaptor.capture(), any(), any(),
-                anyString(), eq(EMPTY_TLS_SERVERS));
         // CS tells netd about the empty DNS config for this network.
-        assertEmpty(mStringArrayCaptor.getValue());
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
+                anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY));
         reset(mNetworkManagementService);
 
         cellLp.addDnsServer(InetAddress.getByName("2001:db8::1"));
         mCellNetworkAgent.sendLinkProperties(cellLp);
         waitForIdle();
-        verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork(
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
                 anyInt(), mStringArrayCaptor.capture(), any(), any(),
-                anyString(), eq(EMPTY_TLS_SERVERS));
+                eq(""), tlsServers.capture());
         assertEquals(1, mStringArrayCaptor.getValue().length);
         assertTrue(ArrayUtils.contains(mStringArrayCaptor.getValue(), "2001:db8::1"));
+        // Opportunistic mode.
+        assertTrue(ArrayUtils.contains(tlsServers.getValue(), "2001:db8::1"));
         reset(mNetworkManagementService);
 
         cellLp.addDnsServer(InetAddress.getByName("192.0.2.1"));
         mCellNetworkAgent.sendLinkProperties(cellLp);
         waitForIdle();
-        verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork(
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
                 anyInt(), mStringArrayCaptor.capture(), any(), any(),
-                anyString(), eq(EMPTY_TLS_SERVERS));
+                eq(""), tlsServers.capture());
         assertEquals(2, mStringArrayCaptor.getValue().length);
         assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(),
                 new String[]{"2001:db8::1", "192.0.2.1"}));
+        // Opportunistic mode.
+        assertEquals(2, tlsServers.getValue().length);
+        assertTrue(ArrayUtils.containsAll(tlsServers.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
         reset(mNetworkManagementService);
 
         final String TLS_SPECIFIER = "tls.example.com";
@@ -3726,7 +3753,7 @@ public class ConnectivityServiceTest {
                 mCellNetworkAgent.getNetwork().netId,
                 new DnsManager.PrivateDnsConfig(TLS_SPECIFIER, TLS_IPS)));
         waitForIdle();
-        verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork(
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
                 anyInt(), mStringArrayCaptor.capture(), any(), any(),
                 eq(TLS_SPECIFIER), eq(TLS_SERVERS));
         assertEquals(2, mStringArrayCaptor.getValue().length);
@@ -3735,6 +3762,77 @@ public class ConnectivityServiceTest {
         reset(mNetworkManagementService);
     }
 
+    @Test
+    public void testPrivateDnsSettingsChange() throws Exception {
+        final String[] EMPTY_STRING_ARRAY = new String[0];
+        ArgumentCaptor<String[]> tlsServers = ArgumentCaptor.forClass(String[].class);
+
+        // Clear any interactions that occur as a result of CS starting up.
+        reset(mNetworkManagementService);
+
+        // The default on Android is opportunistic mode ("Automatic").
+        setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com");
+
+        mCellNetworkAgent = new MockNetworkAgent(TRANSPORT_CELLULAR);
+        waitForIdle();
+        // CS tells netd about the empty DNS config for this network.
+        verify(mNetworkManagementService, never()).setDnsConfigurationForNetwork(
+                anyInt(), eq(EMPTY_STRING_ARRAY), any(), any(), eq(""), eq(EMPTY_STRING_ARRAY));
+        verifyNoMoreInteractions(mNetworkManagementService);
+
+        final LinkProperties cellLp = new LinkProperties();
+        cellLp.setInterfaceName(MOBILE_IFNAME);
+        // Add IPv4 and IPv6 default routes, because DNS-over-TLS code does
+        // "is-reachable" testing in order to not program netd with unreachable
+        // nameservers that it might try repeated to validate.
+        cellLp.addLinkAddress(new LinkAddress("192.0.2.4/24"));
+        cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("192.0.2.4"),
+                MOBILE_IFNAME));
+        cellLp.addLinkAddress(new LinkAddress("2001:db8:1::1/64"));
+        cellLp.addRoute(new RouteInfo((IpPrefix) null, InetAddress.getByName("2001:db8:1::1"),
+                MOBILE_IFNAME));
+        cellLp.addDnsServer(InetAddress.getByName("2001:db8::1"));
+        cellLp.addDnsServer(InetAddress.getByName("192.0.2.1"));
+
+        mCellNetworkAgent.sendLinkProperties(cellLp);
+        mCellNetworkAgent.connect(false);
+        waitForIdle();
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
+                anyInt(), mStringArrayCaptor.capture(), any(), any(),
+                eq(""), tlsServers.capture());
+        assertEquals(2, mStringArrayCaptor.getValue().length);
+        assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
+        // Opportunistic mode.
+        assertEquals(2, tlsServers.getValue().length);
+        assertTrue(ArrayUtils.containsAll(tlsServers.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
+        reset(mNetworkManagementService);
+
+        setPrivateDnsSettings(PRIVATE_DNS_MODE_OFF, "ignored.example.com");
+        verify(mNetworkManagementService, times(1)).setDnsConfigurationForNetwork(
+                anyInt(), mStringArrayCaptor.capture(), any(), any(),
+                eq(""), eq(EMPTY_STRING_ARRAY));
+        assertEquals(2, mStringArrayCaptor.getValue().length);
+        assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
+        reset(mNetworkManagementService);
+
+        setPrivateDnsSettings(PRIVATE_DNS_MODE_OPPORTUNISTIC, "ignored.example.com");
+        verify(mNetworkManagementService, atLeastOnce()).setDnsConfigurationForNetwork(
+                anyInt(), mStringArrayCaptor.capture(), any(), any(),
+                eq(""), tlsServers.capture());
+        assertEquals(2, mStringArrayCaptor.getValue().length);
+        assertTrue(ArrayUtils.containsAll(mStringArrayCaptor.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
+        assertEquals(2, tlsServers.getValue().length);
+        assertTrue(ArrayUtils.containsAll(tlsServers.getValue(),
+                new String[]{"2001:db8::1", "192.0.2.1"}));
+        reset(mNetworkManagementService);
+
+        // Can't test strict mode without properly mocking out the DNS lookups.
+    }
+
     private void checkDirectlyConnectedRoutes(Object callbackObj,
             Collection<LinkAddress> linkAddresses, Collection<RouteInfo> otherRoutes) {
         assertTrue(callbackObj instanceof LinkProperties);