From 38dd45cdd73b169f36afca8bc4d1a20913b6881a Mon Sep 17 00:00:00 2001 From: Kershaw Chang Date: Fri, 4 Feb 2022 12:11:44 +0000 Subject: [PATCH] Bug 1750413 - Give http3 and http2 more priority when selecting alpn, r=necko-reviewers,dragana Differential Revision: https://phabricator.services.mozilla.com/D137139 --- modules/libpref/init/StaticPrefList.yaml | 6 - netwerk/dns/HTTPSSVC.cpp | 40 ++-- netwerk/dns/HTTPSSVC.h | 8 +- netwerk/dns/nsIDNSByTypeRecord.idl | 2 +- netwerk/protocol/http/nsHttp.cpp | 21 ++- netwerk/protocol/http/nsHttp.h | 21 ++- netwerk/protocol/http/nsHttpChannel.cpp | 4 +- .../protocol/http/nsHttpConnectionInfo.cpp | 4 +- .../test/unit/test_https_rr_sorted_alpn.js | 177 ++++++++++++++++++ netwerk/test/unit/xpcshell.ini | 3 + 10 files changed, 248 insertions(+), 38 deletions(-) create mode 100644 netwerk/test/unit/test_https_rr_sorted_alpn.js diff --git a/modules/libpref/init/StaticPrefList.yaml b/modules/libpref/init/StaticPrefList.yaml index ebb9259e61d5..26628df58a8d 100644 --- a/modules/libpref/init/StaticPrefList.yaml +++ b/modules/libpref/init/StaticPrefList.yaml @@ -10644,12 +10644,6 @@ value: 50 mirror: always -# Whether to use https rr for speculative connections. -- name: network.dns.use_https_rr_for_speculative_connection - type: RelaxedAtomicBool - value: false - mirror: always - # Whether to force a transaction to wait https rr. - name: network.dns.force_waiting_https_rr type: RelaxedAtomicBool diff --git a/netwerk/dns/HTTPSSVC.cpp b/netwerk/dns/HTTPSSVC.cpp index 51ef394432bc..efaf19bbf405 100644 --- a/netwerk/dns/HTTPSSVC.cpp +++ b/netwerk/dns/HTTPSSVC.cpp @@ -200,18 +200,33 @@ void SVCB::GetIPHints(CopyableTArray& aAddresses) const { } } -nsTArray SVCB::GetAllAlpn() const { - nsTArray alpnList; +class AlpnComparator { + public: + bool Equals(const Tuple& aA, + const Tuple& aB) const { + return Get<1>(aA) == Get<1>(aB); + } + bool LessThan(const Tuple& aA, + const Tuple& aB) const { + return Get<1>(aA) > Get<1>(aB); + } +}; + +nsTArray> SVCB::GetAllAlpn() const { + nsTArray> alpnList; for (const auto& value : mSvcFieldValue) { if (value.mValue.is()) { - alpnList.AppendElements(value.mValue.as().mValue); + for (const auto& alpn : value.mValue.as().mValue) { + alpnList.AppendElement(MakeTuple(alpn, IsAlpnSupported(alpn))); + } } } + alpnList.Sort(AlpnComparator()); return alpnList; } SVCBRecord::SVCBRecord(const SVCB& data, - Maybe> aAlpn) + Maybe> aAlpn) : mData(data), mAlpn(aAlpn) { mPort = mData.GetPort(); } @@ -228,7 +243,7 @@ NS_IMETHODIMP SVCBRecord::GetName(nsACString& aName) { Maybe SVCBRecord::GetPort() { return mPort; } -Maybe> SVCBRecord::GetAlpn() { +Maybe> SVCBRecord::GetAlpn() { return mAlpn; } @@ -287,21 +302,21 @@ static bool CheckRecordIsUsable(const SVCB& aRecord, nsIDNSService* aDNSService, return true; } -static bool CheckAlpnIsUsable(SupportedAlpnType aAlpnType, bool aNoHttp2, +static bool CheckAlpnIsUsable(SupportedAlpnRank aAlpnType, bool aNoHttp2, bool aNoHttp3, bool aCheckHttp3ExcludedList, const nsACString& aTargetName, uint32_t& aExcludedCount) { // Skip if this alpn is not supported. - if (aAlpnType == SupportedAlpnType::NOT_SUPPORTED) { + if (aAlpnType == SupportedAlpnRank::NOT_SUPPORTED) { return false; } // Skip if we don't want to use http2. - if (aNoHttp2 && aAlpnType == SupportedAlpnType::HTTP_2) { + if (aNoHttp2 && aAlpnType == SupportedAlpnRank::HTTP_2) { return false; } - if (aAlpnType == SupportedAlpnType::HTTP_3) { + if (IsHttp3(aAlpnType)) { if (aCheckHttp3ExcludedList && gHttpHandler->IsHttp3Excluded(aTargetName)) { aExcludedCount++; return false; @@ -318,13 +333,14 @@ static bool CheckAlpnIsUsable(SupportedAlpnType aAlpnType, bool aNoHttp2, static nsTArray FlattenRecords(const nsTArray& aRecords) { nsTArray result; for (const auto& record : aRecords) { - nsTArray alpnList = record.GetAllAlpn(); + nsTArray> alpnList = + record.GetAllAlpn(); if (alpnList.IsEmpty()) { result.AppendElement(SVCBWrapper(record)); } else { for (const auto& alpn : alpnList) { SVCBWrapper wrapper(record); - wrapper.mAlpn.emplace(MakeTuple(alpn, IsAlpnSupported(alpn))); + wrapper.mAlpn = Some(alpn); result.AppendElement(wrapper); } } @@ -367,7 +383,7 @@ DNSHTTPSSVCRecordBase::GetServiceModeRecordInternal( continue; } - if (Get<1>(*(record.mAlpn)) == SupportedAlpnType::HTTP_3) { + if (IsHttp3(Get<1>(*(record.mAlpn)))) { // If the selected alpn is h3 and ech for h3 is disabled, we want // to find out if there is another non-h3 record that has // echConfig. If yes, we'll use the non-h3 record with echConfig diff --git a/netwerk/dns/HTTPSSVC.h b/netwerk/dns/HTTPSSVC.h index 0cf83dfd51b6..760917b5eb12 100644 --- a/netwerk/dns/HTTPSSVC.h +++ b/netwerk/dns/HTTPSSVC.h @@ -100,7 +100,7 @@ struct SVCB { Maybe GetPort() const; bool NoDefaultAlpn() const; void GetIPHints(CopyableTArray& aAddresses) const; - nsTArray GetAllAlpn() const; + nsTArray> GetAllAlpn() const; uint16_t mSvcFieldPriority = 0; nsCString mSvcDomainName; nsCString mEchConfig; @@ -112,7 +112,7 @@ struct SVCB { struct SVCBWrapper { explicit SVCBWrapper(const SVCB& aRecord) : mRecord(aRecord) {} - Maybe> mAlpn; + Maybe> mAlpn; const SVCB& mRecord; }; @@ -123,7 +123,7 @@ class SVCBRecord : public nsISVCBRecord { explicit SVCBRecord(const SVCB& data) : mData(data), mPort(Nothing()), mAlpn(Nothing()) {} explicit SVCBRecord(const SVCB& data, - Maybe> aAlpn); + Maybe> aAlpn); private: friend class DNSHTTPSSVCRecordBase; @@ -132,7 +132,7 @@ class SVCBRecord : public nsISVCBRecord { SVCB mData; Maybe mPort; - Maybe> mAlpn; + Maybe> mAlpn; }; class DNSHTTPSSVCRecordBase { diff --git a/netwerk/dns/nsIDNSByTypeRecord.idl b/netwerk/dns/nsIDNSByTypeRecord.idl index 8249908198d5..270b6772ce22 100644 --- a/netwerk/dns/nsIDNSByTypeRecord.idl +++ b/netwerk/dns/nsIDNSByTypeRecord.idl @@ -30,7 +30,7 @@ namespace net { native TypeResult(mozilla::net::TypeRecordResultType); native MaybePort(mozilla::Maybe); -native MaybeAlpnTuple(mozilla::Maybe>); +native MaybeAlpnTuple(mozilla::Maybe>); [scriptable, uuid(5d13241b-9d46-448a-90d8-77c418491026)] interface nsIDNSByTypeRecord : nsIDNSRecord diff --git a/netwerk/protocol/http/nsHttp.cpp b/netwerk/protocol/http/nsHttp.cpp index eab55b112a1f..24a4389b474a 100644 --- a/netwerk/protocol/http/nsHttp.cpp +++ b/netwerk/protocol/http/nsHttp.cpp @@ -970,10 +970,21 @@ nsresult HttpProxyResponseToErrorCode(uint32_t aStatusCode) { return rv; } -SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn) { +SupportedAlpnRank H3VersionToRank(const nsACString& aVersion) { + for (uint32_t i = 0; i < kHttp3VersionCount; i++) { + if (aVersion.Equals(kHttp3Versions[i])) { + return static_cast( + static_cast(SupportedAlpnRank::HTTP_3_DRAFT_29) + i); + } + } + + return SupportedAlpnRank::NOT_SUPPORTED; +} + +SupportedAlpnRank IsAlpnSupported(const nsACString& aAlpn) { if (StaticPrefs::network_http_http3_enable() && gHttpHandler->IsHttp3VersionSupported(aAlpn)) { - return SupportedAlpnType::HTTP_3; + return H3VersionToRank(aAlpn); } if (gHttpHandler->IsSpdyEnabled()) { @@ -981,15 +992,15 @@ SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn) { SpdyInformation* spdyInfo = gHttpHandler->SpdyInfo(); if (NS_SUCCEEDED(spdyInfo->GetNPNIndex(aAlpn, &spdyIndex)) && spdyInfo->ProtocolEnabled(spdyIndex)) { - return SupportedAlpnType::HTTP_2; + return SupportedAlpnRank::HTTP_2; } } if (aAlpn.LowerCaseEqualsASCII("http/1.1")) { - return SupportedAlpnType::HTTP_1_1; + return SupportedAlpnRank::HTTP_1_1; } - return SupportedAlpnType::NOT_SUPPORTED; + return SupportedAlpnRank::NOT_SUPPORTED; } bool SecurityErrorToBeHandledByTransaction(nsresult aReason) { diff --git a/netwerk/protocol/http/nsHttp.h b/netwerk/protocol/http/nsHttp.h index cd838474ac3e..ac5ee85e599f 100644 --- a/netwerk/protocol/http/nsHttp.h +++ b/netwerk/protocol/http/nsHttp.h @@ -37,13 +37,22 @@ enum class HttpVersion { enum class SpdyVersion { NONE = 0, HTTP_2 = 5 }; -enum class SupportedAlpnType : uint8_t { - HTTP_3 = 0, - HTTP_2, - HTTP_1_1, - NOT_SUPPORTED +enum class SupportedAlpnRank : uint8_t { + NOT_SUPPORTED = 0, + HTTP_1_1 = 1, + HTTP_2 = 2, + // Note that the order here MUST be the same as the order in kHttp3Versions. + HTTP_3_DRAFT_29 = 3, + HTTP_3_DRAFT_30 = 4, + HTTP_3_DRAFT_31 = 5, + HTTP_3_DRAFT_32 = 6, + HTTP_3_VER_1 = 7, }; +inline bool IsHttp3(SupportedAlpnRank aRank) { + return aRank >= SupportedAlpnRank::HTTP_3_DRAFT_29; +} + extern const uint32_t kHttp3VersionCount; extern const nsCString kHttp3Versions[]; @@ -389,7 +398,7 @@ void LogHeaders(const char* lineStart); nsresult HttpProxyResponseToErrorCode(uint32_t aStatusCode); // Convert an alpn string to SupportedAlpnType. -SupportedAlpnType IsAlpnSupported(const nsACString& aAlpn); +SupportedAlpnRank IsAlpnSupported(const nsACString& aAlpn); static inline bool AllowedErrorForHTTPSRRFallback(nsresult aError) { return psm::IsNSSErrorCode(-1 * NS_ERROR_GET_CODE(aError)) || diff --git a/netwerk/protocol/http/nsHttpChannel.cpp b/netwerk/protocol/http/nsHttpChannel.cpp index ce80777e59a9..bb4914dcc392 100644 --- a/netwerk/protocol/http/nsHttpChannel.cpp +++ b/netwerk/protocol/http/nsHttpChannel.cpp @@ -7020,8 +7020,8 @@ static void ReportHTTPSRRTelemetry( getter_AddRefs(svcbRecord)))) { MOZ_ASSERT(svcbRecord); - Maybe> alpn = svcbRecord->GetAlpn(); - bool isHttp3 = alpn ? Get<1>(*alpn) == SupportedAlpnType::HTTP_3 : false; + Maybe> alpn = svcbRecord->GetAlpn(); + bool isHttp3 = alpn ? IsHttp3(Get<1>(*alpn)) : false; Telemetry::Accumulate(Telemetry::HTTPS_RR_WITH_HTTP3_PRESENTED, isHttp3); } } diff --git a/netwerk/protocol/http/nsHttpConnectionInfo.cpp b/netwerk/protocol/http/nsHttpConnectionInfo.cpp index 1854dc0780b0..8f0ef0f679de 100644 --- a/netwerk/protocol/http/nsHttpConnectionInfo.cpp +++ b/netwerk/protocol/http/nsHttpConnectionInfo.cpp @@ -334,10 +334,10 @@ nsHttpConnectionInfo::CloneAndAdoptHTTPSSVCRecord( // Try to get the port and Alpn. If this record has SvcParamKeyPort defined, // the new port will be used as mRoutedPort. Maybe port = aRecord->GetPort(); - Maybe> alpn = aRecord->GetAlpn(); + Maybe> alpn = aRecord->GetAlpn(); // Let the new conn info learn h3 will be used. - bool isHttp3 = alpn ? Get<1>(*alpn) == SupportedAlpnType::HTTP_3 : false; + bool isHttp3 = alpn ? mozilla::net::IsHttp3(Get<1>(*alpn)) : false; LOG(("HTTPSSVC: use new routed host (%s) and new npnToken (%s)", name.get(), alpn ? Get<0>(*alpn).get() : "None")); diff --git a/netwerk/test/unit/test_https_rr_sorted_alpn.js b/netwerk/test/unit/test_https_rr_sorted_alpn.js new file mode 100644 index 000000000000..6d5afd4546ba --- /dev/null +++ b/netwerk/test/unit/test_https_rr_sorted_alpn.js @@ -0,0 +1,177 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +"use strict"; + +ChromeUtils.import("resource://gre/modules/NetUtil.jsm"); + +let trrServer; + +const dns = Cc["@mozilla.org/network/dns-service;1"].getService( + Ci.nsIDNSService +); + +function setup() { + trr_test_setup(); +} + +setup(); +registerCleanupFunction(async () => { + trr_clear_prefs(); + Services.prefs.clearUserPref("network.http.http3.support_version1"); + if (trrServer) { + await trrServer.stop(); + } +}); + +function checkResult(inRecord, noHttp2, noHttp3, result) { + if (!result) { + Assert.throws( + () => { + inRecord + .QueryInterface(Ci.nsIDNSHTTPSSVCRecord) + .GetServiceModeRecord(noHttp2, noHttp3); + }, + /NS_ERROR_NOT_AVAILABLE/, + "Should get an error" + ); + return; + } + + let record = inRecord + .QueryInterface(Ci.nsIDNSHTTPSSVCRecord) + .GetServiceModeRecord(noHttp2, noHttp3); + Assert.equal(record.priority, result.expectedPriority); + Assert.equal(record.name, result.expectedName); + Assert.equal(record.selectedAlpn, result.expectedAlpn); +} + +add_task(async function testSortedAlpnH3() { + dns.clearCache(true); + + trrServer = new TRRServer(); + await trrServer.start(); + + Services.prefs.setIntPref("network.trr.mode", 3); + Services.prefs.setCharPref( + "network.trr.uri", + `https://foo.example.com:${trrServer.port}/dns-query` + ); + Services.prefs.setBoolPref("network.http.http3.support_version1", true); + await trrServer.registerDoHAnswers("test.alpn.com", "HTTPS", { + answers: [ + { + name: "test.alpn.com", + ttl: 55, + type: "HTTPS", + flush: false, + data: { + priority: 1, + name: "test.alpn.com", + values: [{ key: "alpn", value: ["h2", "http/1.1", "h3-30", "h3"] }], + }, + }, + ], + }); + + let { inRecord } = await new TRRDNSListener("test.alpn.com", { + type: dns.RESOLVE_TYPE_HTTPSSVC, + }); + + checkResult(inRecord, false, false, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h3", + }); + checkResult(inRecord, false, true, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h2", + }); + checkResult(inRecord, true, false, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h3", + }); + checkResult(inRecord, true, true, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "http/1.1", + }); + + Services.prefs.setBoolPref("network.http.http3.support_version1", false); + checkResult(inRecord, false, false, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h3-30", + }); + checkResult(inRecord, false, true, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h2", + }); + checkResult(inRecord, true, false, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "h3-30", + }); + checkResult(inRecord, true, true, { + expectedPriority: 1, + expectedName: "test.alpn.com", + expectedAlpn: "http/1.1", + }); +}); + +add_task(async function testSortedAlpnH2() { + dns.clearCache(true); + + Services.prefs.setIntPref("network.trr.mode", 3); + Services.prefs.setCharPref( + "network.trr.uri", + `https://foo.example.com:${trrServer.port}/dns-query` + ); + await trrServer.registerDoHAnswers("test.alpn_2.com", "HTTPS", { + answers: [ + { + name: "test.alpn_2.com", + ttl: 55, + type: "HTTPS", + flush: false, + data: { + priority: 1, + name: "test.alpn_2.com", + values: [{ key: "alpn", value: ["http/1.1", "h2"] }], + }, + }, + ], + }); + + let { inRecord } = await new TRRDNSListener("test.alpn_2.com", { + type: dns.RESOLVE_TYPE_HTTPSSVC, + }); + + checkResult(inRecord, false, false, { + expectedPriority: 1, + expectedName: "test.alpn_2.com", + expectedAlpn: "h2", + }); + checkResult(inRecord, false, true, { + expectedPriority: 1, + expectedName: "test.alpn_2.com", + expectedAlpn: "h2", + }); + checkResult(inRecord, true, false, { + expectedPriority: 1, + expectedName: "test.alpn_2.com", + expectedAlpn: "http/1.1", + }); + checkResult(inRecord, true, true, { + expectedPriority: 1, + expectedName: "test.alpn_2.com", + expectedAlpn: "http/1.1", + }); + + await trrServer.stop(); + trrServer = null; +}); diff --git a/netwerk/test/unit/xpcshell.ini b/netwerk/test/unit/xpcshell.ini index be1acef0589c..83e4d818095d 100644 --- a/netwerk/test/unit/xpcshell.ini +++ b/netwerk/test/unit/xpcshell.ini @@ -590,3 +590,6 @@ head = head_channels.js head_cache.js head_cookies.js head_trr.js trr_common.js skip-if = os == "android" run-sequentially = node server exceptions dont replay well [test_trr_blocklist.js] +[test_https_rr_sorted_alpn.js] +skip-if = os == "android" +run-sequentially = node server exceptions dont replay well