Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 5daa40db authored by Mike Yu's avatar Mike Yu
Browse files

Revert "Support evaluating private DNS by latency" 

This reverts commit 1aede813.

This is no longer needed since we will implement a simpler mechanics
for DoT validation.

Bug: 188153519
Test: cd packages/modules/DnsResolver && atest
Change-Id: I4a2f8b926f27fd38c58aea3a993a311a74fc4950
parent 7135fbfc

DnsTlsDispatcher.cpp

+2 −19
Original line number Diff line number Diff line
@@ -182,18 +182,9 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un 
    // stuck, this function also gets blocked.

    const int connectCounter = xport->transport.getConnectCounter();



    Stopwatch stopwatch;

    const auto& result = queryInternal(*xport, query);

    const int64_t timeTaken = saturate_cast<int64_t>(stopwatch.timeTakenUs() / 1000);

    *connectTriggered = (xport->transport.getConnectCounter() > connectCounter);



    const int64_t targetTime = server.latencyThreshold().value_or(INT64_MAX);

    const bool latencyTooHigh = timeTaken > targetTime;

    if (latencyTooHigh) {

        LOG(WARNING) << "DoT query took too long: " << timeTaken << " ms (threshold: " << targetTime

                     << "ms)";

    }



    DnsTlsTransport::Response code = result.code;

    if (code == DnsTlsTransport::Response::success) {

        if (result.response.size() > ans.size()) {
@@ -215,7 +206,7 @@ DnsTlsTransport::Response DnsTlsDispatcher::query(const DnsTlsServer& server, un 
        xport->lastUsed = now;



        // DoT revalidation specific feature.

        if (xport->checkRevalidationNecessary(code, latencyTooHigh)) {

        if (xport->checkRevalidationNecessary(code)) {

            // Even if the revalidation passes, it doesn't guarantee that DoT queries

            // to the xport can stop failing because revalidation creates a new connection

            // to probe while the xport still uses an existing connection. So far, there isn't
@@ -342,21 +333,13 @@ DnsTlsDispatcher::Transport* DnsTlsDispatcher::getTransport(const Key& key) { 
    return (it == mStore.end() ? nullptr : it->second.get());

}



bool DnsTlsDispatcher::Transport::checkRevalidationNecessary(DnsTlsTransport::Response code,

                                                             bool latencyTooHigh) {

bool DnsTlsDispatcher::Transport::checkRevalidationNecessary(DnsTlsTransport::Response code) {

    if (!revalidationEnabled) return false;



    if (code == DnsTlsTransport::Response::network_error) {

        continuousfailureCount++;

        LOG(WARNING) << "continuousfailureCount incremented: network_error, count = "

                     << continuousfailureCount;

    } else if (latencyTooHigh) {

        continuousfailureCount++;

        LOG(WARNING) << "continuousfailureCount incremented: latency too High, count = "

                     << continuousfailureCount;

    } else {

        continuousfailureCount = 0;

        LOG(WARNING) << "continuousfailureCount reset";

    }



    // triggerThreshold must be greater than 0 because the value of revalidationEnabled is true.

DnsTlsDispatcher.h

+1 −2
Original line number Diff line number Diff line
@@ -107,8 +107,7 @@ class DnsTlsDispatcher : public PrivateDnsValidationObserver { 
        // whether or not this Transport is usable.

        bool usable() const REQUIRES(sLock);



        bool checkRevalidationNecessary(DnsTlsTransport::Response code, bool latencyTooHigh)

                REQUIRES(sLock);

        bool checkRevalidationNecessary(DnsTlsTransport::Response code) REQUIRES(sLock);



        std::chrono::milliseconds timeout() const { return mTimeout; }

DnsTlsServer.h

+0 −9
Original line number Diff line number Diff line
@@ -86,19 +86,10 @@ struct DnsTlsServer : public IPrivateDnsServer { 
    bool active() const override { return mActive; }

    void setActive(bool val) override { mActive = val; }



    // Setter and getter for the latency constraint.

    void setLatencyThreshold(std::optional<int64_t> val) { mLatencyThresholdMs = val; }

    std::optional<int64_t> latencyThreshold() const { return mLatencyThresholdMs; }



  private:

    // State, unrelated to the comparison of DnsTlsServer objects.

    Validation mValidation = Validation::unknown_server;

    bool mActive = false;



    // The DNS response time threshold. If it is set, the latency of this server will be

    // considered when evaluating the server. It is used for the DoT engine to evaluate whether

    // this server, compared to cleartext DNS servers, has relatively high latency or not.

    std::optional<int64_t> mLatencyThresholdMs = std::nullopt;

};



// This comparison only checks the IP address.  It ignores ports, names, and fingerprints.

Experiments.h

+4 −12
Original line number Diff line number Diff line
@@ -49,19 +49,11 @@ class Experiments { 
    // TODO: Migrate other experiment flags to here.

    // (retry_count, retransmission_time_interval)

    static constexpr const char* const kExperimentFlagKeyList[] = {

            "keep_listening_udp",

            "parallel_lookup_release",

            "parallel_lookup_sleep_time",

            "sort_nameservers",

            "dot_async_handshake",

            "dot_connect_timeout_ms",

            "dot_maxtries",

            "dot_revalidation_threshold",

            "dot_xport_unusable_threshold",

            "keep_listening_udp",   "parallel_lookup_release",    "parallel_lookup_sleep_time",

            "sort_nameservers",     "dot_async_handshake",        "dot_connect_timeout_ms",

            "dot_maxtries",         "dot_revalidation_threshold", "dot_xport_unusable_threshold",

            "dot_query_timeout_ms",

            "avoid_bad_private_dns",

            "min_private_dns_latency_threshold_ms",

            "max_private_dns_latency_threshold_ms"};

    };

    // This value is used in updateInternal as the default value if any flags can't be found.

    static constexpr int kFlagIntDefault = INT_MIN;

    // For testing.

PrivateDnsConfiguration.cpp

+12 −71
Original line number Diff line number Diff line
@@ -21,23 +21,18 @@ 
#include <android-base/format.h>

#include <android-base/logging.h>

#include <android-base/stringprintf.h>

#include <netdutils/Stopwatch.h>

#include <netdutils/ThreadUtil.h>

#include <sys/socket.h>



#include "DnsTlsTransport.h"

#include "Experiments.h"

#include "ResolverEventReporter.h"

#include "netd_resolv/resolv.h"

#include "resolv_cache.h"

#include "resolv_private.h"

#include "util.h"



using aidl::android::net::resolv::aidl::IDnsResolverUnsolicitedEventListener;

using aidl::android::net::resolv::aidl::PrivateDnsValidationEventParcel;

using android::base::StringPrintf;

using android::netdutils::setThreadName;

using android::netdutils::Stopwatch;

using std::chrono::milliseconds;



namespace android {
@@ -200,23 +195,6 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un 
    std::thread validate_thread([this, identity, server, netId, isRevalidation] {

        setThreadName(StringPrintf("TlsVerify_%u", netId).c_str());



        const bool avoidBadPrivateDns =

                Experiments::getInstance()->getFlag("avoid_bad_private_dns", 0);

        std::optional<int64_t> latencyThreshold;

        if (avoidBadPrivateDns) {

            const int maxLatency = Experiments::getInstance()->getFlag(

                    "max_private_dns_latency_threshold_ms", kMaxPrivateDnsLatencyThresholdMs);

            const int minLatency = Experiments::getInstance()->getFlag(

                    "min_private_dns_latency_threshold_ms", kMinPrivateDnsLatencyThresholdMs);

            const auto do53Latency = resolv_stats_get_average_response_time(netId, PROTO_UDP);

            const int target =

                    do53Latency.has_value() ? (3 * do53Latency.value().count() / 1000) : 0;



            // The time is limited to the range [minLatency, maxLatency].

            latencyThreshold = std::clamp(target, minLatency, maxLatency);

        }

        const bool isOpportunisticMode = server.name.empty();



        // cat /proc/sys/net/ipv4/tcp_syn_retries yields "6".

        //

        // Start with a 1 minute delay and backoff to once per hour.
@@ -237,24 +215,12 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un 
            // It can take milliseconds to minutes, up to the SYN retry limit.

            LOG(WARNING) << "Validating DnsTlsServer " << server.toIpString() << " with mark 0x"

                         << std::hex << server.validationMark();

            const bool success = DnsTlsTransport::validate(server, server.validationMark());

            LOG(WARNING) << "validateDnsTlsServer returned " << success << " for "

                         << server.toIpString();



            Stopwatch stopwatch;

            const bool gotAnswer = DnsTlsTransport::validate(server, server.validationMark());

            const int32_t timeTaken = saturate_cast<int32_t>(stopwatch.timeTakenUs() / 1000);

            LOG(WARNING) << fmt::format("validateDnsTlsServer returned {} for {}, took {}ms",

                                        gotAnswer, server.toIpString(), timeTaken);



            const int64_t targetTime = latencyThreshold.value_or(INT64_MAX);

            bool latencyTooHigh = false;

            if (isOpportunisticMode && timeTaken > targetTime) {

                latencyTooHigh = true;

                LOG(WARNING) << "validateDnsTlsServer took too long: threshold is " << targetTime

                             << "ms";

            }



            // TODO: combine these boolean variables into a bitwise variable.

            const bool needs_reeval = this->recordPrivateDnsValidation(

                    identity, netId, gotAnswer, isRevalidation, latencyTooHigh);

            const bool needs_reeval =

                    this->recordPrivateDnsValidation(identity, netId, success, isRevalidation);



            if (!needs_reeval) {

                break;
@@ -267,8 +233,6 @@ void PrivateDnsConfiguration::startValidation(const ServerIdentity& identity, un 
                break;

            }

        }



        this->updateServerLatencyThreshold(identity, latencyThreshold, netId);

    });

    validate_thread.detach();

}
@@ -304,8 +268,8 @@ void PrivateDnsConfiguration::sendPrivateDnsValidationEvent(const ServerIdentity 
}



bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& identity,

                                                         unsigned netId, bool gotAnswer,

                                                         bool isRevalidation, bool latencyTooHigh) {

                                                         unsigned netId, bool success,

                                                         bool isRevalidation) {

    constexpr bool NEEDS_REEVALUATION = true;

    constexpr bool DONT_REEVALUATE = false;
@@ -326,17 +290,14 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i 
    }



    bool reevaluationStatus = NEEDS_REEVALUATION;

    if (gotAnswer) {

        if (!latencyTooHigh) {

    if (success) {

        reevaluationStatus = DONT_REEVALUATE;

        }

    } else if (mode->second == PrivateDnsMode::OFF) {

        reevaluationStatus = DONT_REEVALUATE;

    } else if (mode->second == PrivateDnsMode::OPPORTUNISTIC && !isRevalidation) {

        reevaluationStatus = DONT_REEVALUATE;

    }



    bool success = gotAnswer;

    auto& tracker = netPair->second;

    auto serverPair = tracker.find(identity);

    if (serverPair == tracker.end()) {
@@ -351,12 +312,10 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i 
        reevaluationStatus = DONT_REEVALUATE;

    }



    const bool succeededQuickly = success && !latencyTooHigh;



    // Send private dns validation result to listeners.

    sendPrivateDnsValidationEvent(identity, netId, succeededQuickly);

    sendPrivateDnsValidationEvent(identity, netId, success);



    if (succeededQuickly) {

    if (success) {

        updateServerState(identity, Validation::success, netId);

    } else {

        // Validation failure is expected if a user is on a captive portal.
@@ -366,7 +325,7 @@ bool PrivateDnsConfiguration::recordPrivateDnsValidation(const ServerIdentity& i 
                                                                       : Validation::fail;

        updateServerState(identity, result, netId);

    }

    LOG(WARNING) << "Validation " << (succeededQuickly ? "success" : "failed");

    LOG(WARNING) << "Validation " << (success ? "success" : "failed");



    return reevaluationStatus;

}
@@ -426,24 +385,6 @@ base::Result<IPrivateDnsServer*> PrivateDnsConfiguration::getPrivateDnsLocked( 
    return iter->second.get();

}



void PrivateDnsConfiguration::updateServerLatencyThreshold(const ServerIdentity& identity,

                                                           std::optional<int64_t> latencyThreshold,

                                                           uint32_t netId) {

    std::lock_guard guard(mPrivateDnsLock);



    const auto result = getPrivateDnsLocked(identity, netId);

    if (!result.ok()) return;



    if (result.value()->isDot()) {

        DnsTlsServer& server = *static_cast<DnsTlsServer*>(result.value());

        server.setLatencyThreshold(latencyThreshold);

        LOG(INFO) << "Set latencyThreshold "

                  << (latencyThreshold ? std::to_string(latencyThreshold.value()) + "ms"

                                       : "nullopt")

                  << " to " << server.toIpString();

    }

}



void PrivateDnsConfiguration::setObserver(PrivateDnsValidationObserver* observer) {

    std::lock_guard guard(mPrivateDnsLock);

    mObserver = observer;