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

Commit 09bd567e authored by Mike Yu's avatar Mike Yu Committed by Automerger Merge Worker
Browse files

Merge "Revert "Support evaluating private DNS by latency"" am: d1b3b803 am:... 

Merge "Revert "Support evaluating private DNS by latency"" am: d1b3b803 am: c9ae70a7 am: 728126a1

Original change: https://android-review.googlesource.com/c/platform/packages/modules/DnsResolver/+/1733918

Change-Id: I358ab3be65f67cdf44ed2b25d980948e5e9b6659
parents c0948cf5 728126a1

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;