Skip to content

Commit 5f7eead

Browse files
KershawChangkjang@mozilla.com
KershawChang
authored and
kjang@mozilla.com
committed
Bug 1980812 - Avoid setting explicit h2 ALPN for connections, r=necko-reviewers,valentin
Differential Revision: https://phabricator.services.mozilla.com/D262000
1 parent 2a179a0 commit 5f7eead

File tree

4 files changed

+99
-20
lines changed

4 files changed

+99
-20
lines changed

‎netwerk/dns/HTTPSSVC.cpp‎

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,7 @@ static nsTArray<SVCBWrapper> FlattenRecords(const nsACString& aHost,
359359
if (alpnList.IsEmpty()) {
360360
result.AppendElement(SVCBWrapper(record));
361361
} else {
362+
bool h1AlpnAdded = false;
362363
if (!hasNoDefaultAlpn) {
363364
// Consider two scenarios when "no-default-alpn" is not found:
364365
// 1. If echConfig is present in the record:
@@ -374,15 +375,32 @@ static nsTArray<SVCBWrapper> FlattenRecords(const nsACString& aHost,
374375
if (!aHost.Equals(record.mSvcDomainName) || record.mHasEchConfig) {
375376
alpnList.AppendElement(
376377
std::make_tuple(""_ns, SupportedAlpnRank::HTTP_1_1));
378+
h1AlpnAdded = true;
377379
}
378380
}
379381
for (const auto& alpn : alpnList) {
380-
SVCBWrapper wrapper(record);
381-
wrapper.mAlpn = Some(alpn);
382-
if (IsHttp3(std::get<1>(alpn))) {
382+
const auto alpnRank = std::get<1>(alpn);
383+
if (IsHttp3(alpnRank)) {
383384
aH3RecordCount++;
384385
}
385-
result.AppendElement(wrapper);
386+
387+
// Skip explicit h2 if h1 is already present.
388+
if (alpnRank == SupportedAlpnRank::HTTP_2 && h1AlpnAdded) {
389+
continue;
390+
}
391+
392+
// For h2, normalize the tuple to use an empty ALPN. If both h1 and h2
393+
// are available, the ALPN negotiation will automatically select h2 when
394+
// the server supports it, otherwise it will fall back to h1.
395+
// However, if `no-default-alpn` is present, fallback to h1 is not
396+
// possible, so we must explicitly set h2.
397+
auto chosen =
398+
(alpnRank == SupportedAlpnRank::HTTP_2 && !hasNoDefaultAlpn)
399+
? std::make_tuple(""_ns, alpnRank)
400+
: alpn;
401+
SVCBWrapper wrapper(record);
402+
wrapper.mAlpn = Some(chosen);
403+
result.AppendElement(std::move(wrapper));
386404
}
387405
}
388406
}

‎netwerk/test/unit/test_http3_fast_fallback.js‎

Lines changed: 61 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -913,3 +913,64 @@ add_task(async function testFastfallbackToTheSameRecord() {
913913

914914
await trrServer.stop();
915915
});
916+
917+
// Similar to the previous test, but no ech.
918+
add_task(async function testFastfallbackToTheSameRecord1() {
919+
trrServer = new TRRServer();
920+
await trrServer.start();
921+
Services.prefs.setBoolPref("network.dns.upgrade_with_https_rr", true);
922+
Services.prefs.setBoolPref("network.dns.use_https_rr_as_altsvc", true);
923+
Services.prefs.setBoolPref("network.dns.echconfig.enabled", true);
924+
Services.prefs.setBoolPref("network.dns.http3_echconfig.enabled", true);
925+
926+
Services.prefs.setIntPref("network.trr.mode", 3);
927+
Services.prefs.setCharPref(
928+
"network.trr.uri",
929+
`https://foo.example.com:${trrServer.port()}/dns-query`
930+
);
931+
Services.prefs.setBoolPref("network.http.http3.enable", true);
932+
933+
Services.prefs.setIntPref(
934+
"network.dns.httpssvc.http3_fast_fallback_timeout",
935+
1000
936+
);
937+
938+
await trrServer.registerDoHAnswers("test.no_ech.org", "HTTPS", {
939+
answers: [
940+
{
941+
name: "test.no_ech.org",
942+
ttl: 55,
943+
type: "HTTPS",
944+
flush: false,
945+
data: {
946+
priority: 1,
947+
name: "test.no_ech.org",
948+
values: [
949+
{ key: "alpn", value: ["h3", "h2"] },
950+
{ key: "port", value: h2Port },
951+
],
952+
},
953+
},
954+
],
955+
});
956+
957+
await trrServer.registerDoHAnswers("test.no_ech.org", "A", {
958+
answers: [
959+
{
960+
name: "test.no_ech.org",
961+
ttl: 55,
962+
type: "A",
963+
flush: false,
964+
data: "127.0.0.1",
965+
},
966+
],
967+
});
968+
969+
let chan = makeChan(`https://test.no_ech.org/server-timing`);
970+
let [req] = await channelOpenPromise(chan);
971+
Assert.equal(req.protocolVersion, "h2");
972+
let internal = req.QueryInterface(Ci.nsIHttpChannelInternal);
973+
Assert.equal(internal.remotePort, h2Port);
974+
975+
await trrServer.stop();
976+
});

‎netwerk/test/unit/test_https_rr_ech_prefs.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ add_task(async function testEchConfigEnabled() {
106106
checkResult(inRecord, false, true, {
107107
expectedPriority: 2,
108108
expectedName: "test.bar_2.com",
109-
expectedAlpn: "h2",
109+
expectedAlpn: "",
110110
});
111111
checkResult(inRecord, true, false, {
112112
expectedPriority: 1,
@@ -129,12 +129,12 @@ add_task(async function testEchConfigEnabled() {
129129
checkResult(inRecord, false, false, {
130130
expectedPriority: 2,
131131
expectedName: "test.bar_2.com",
132-
expectedAlpn: "h2",
132+
expectedAlpn: "",
133133
});
134134
checkResult(inRecord, false, true, {
135135
expectedPriority: 2,
136136
expectedName: "test.bar_2.com",
137-
expectedAlpn: "h2",
137+
expectedAlpn: "",
138138
});
139139
checkResult(inRecord, true, false, {
140140
expectedPriority: 2,
@@ -212,12 +212,12 @@ add_task(async function testTwoRecordsHaveEchConfig() {
212212
checkResult(inRecord, false, false, {
213213
expectedPriority: 2,
214214
expectedName: "test.foo_h2.com",
215-
expectedAlpn: "h2",
215+
expectedAlpn: "",
216216
});
217217
checkResult(inRecord, false, true, {
218218
expectedPriority: 2,
219219
expectedName: "test.foo_h2.com",
220-
expectedAlpn: "h2",
220+
expectedAlpn: "",
221221
});
222222
checkResult(inRecord, true, false, {
223223
expectedPriority: 2,
@@ -244,7 +244,7 @@ add_task(async function testTwoRecordsHaveEchConfig() {
244244
checkResult(inRecord, false, true, {
245245
expectedPriority: 2,
246246
expectedName: "test.foo_h2.com",
247-
expectedAlpn: "h2",
247+
expectedAlpn: "",
248248
});
249249
checkResult(inRecord, true, false, {
250250
expectedPriority: 1,
@@ -429,7 +429,7 @@ add_task(async function testOneRecordsHasEchConfig() {
429429
checkResult(inRecord, false, true, {
430430
expectedPriority: 2,
431431
expectedName: "test.foo_h2.com",
432-
expectedAlpn: "h2",
432+
expectedAlpn: "",
433433
});
434434
checkResult(inRecord, true, false, {
435435
expectedPriority: 1,
@@ -456,7 +456,7 @@ add_task(async function testOneRecordsHasEchConfig() {
456456
checkResult(inRecord, false, true, {
457457
expectedPriority: 2,
458458
expectedName: "test.foo_h2.com",
459-
expectedAlpn: "h2",
459+
expectedAlpn: "",
460460
});
461461
checkResult(inRecord, true, false, {
462462
expectedPriority: 1,

‎netwerk/test/unit/test_https_rr_sorted_alpn.js‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ add_task(async function testSortedAlpnH3() {
8989
checkResult(inRecord, false, true, {
9090
expectedPriority: 1,
9191
expectedName: "test.alpn.com",
92-
expectedAlpn: "h2",
92+
expectedAlpn: "",
9393
});
9494
checkResult(inRecord, true, false, {
9595
expectedPriority: 1,
@@ -106,12 +106,12 @@ add_task(async function testSortedAlpnH3() {
106106
checkResult(inRecord, false, false, {
107107
expectedPriority: 1,
108108
expectedName: "test.alpn.com",
109-
expectedAlpn: "h2",
109+
expectedAlpn: "",
110110
});
111111
checkResult(inRecord, false, true, {
112112
expectedPriority: 1,
113113
expectedName: "test.alpn.com",
114-
expectedAlpn: "h2",
114+
expectedAlpn: "",
115115
});
116116
checkResult(inRecord, true, false, {
117117
expectedPriority: 1,
@@ -130,12 +130,12 @@ add_task(async function testSortedAlpnH3() {
130130
checkResult(inRecord, false, false, {
131131
expectedPriority: 1,
132132
expectedName: "test.alpn.com",
133-
expectedAlpn: "h2",
133+
expectedAlpn: "",
134134
});
135135
checkResult(inRecord, false, true, {
136136
expectedPriority: 1,
137137
expectedName: "test.alpn.com",
138-
expectedAlpn: "h2",
138+
expectedAlpn: "",
139139
});
140140
checkResult(inRecord, true, false, {
141141
expectedPriority: 1,
@@ -158,7 +158,7 @@ add_task(async function testSortedAlpnH3() {
158158
checkResult(inRecord, false, true, {
159159
expectedPriority: 1,
160160
expectedName: "test.alpn.com",
161-
expectedAlpn: "h2",
161+
expectedAlpn: "",
162162
});
163163
checkResult(inRecord, true, false, {
164164
expectedPriority: 1,
@@ -203,12 +203,12 @@ add_task(async function testSortedAlpnH2() {
203203
checkResult(inRecord, false, false, {
204204
expectedPriority: 1,
205205
expectedName: "test.alpn_2.com",
206-
expectedAlpn: "h2",
206+
expectedAlpn: "",
207207
});
208208
checkResult(inRecord, false, true, {
209209
expectedPriority: 1,
210210
expectedName: "test.alpn_2.com",
211-
expectedAlpn: "h2",
211+
expectedAlpn: "",
212212
});
213213
checkResult(inRecord, true, false, {
214214
expectedPriority: 1,

0 commit comments

Comments
 (0)