1860038 - Enable network.dns.port_prefixed_qname_https_rr

Status: RESOLVED FIXED

(Core :: Networking: DNS, task, P2)

Description

[Valentin Gosu [:valentin] (he/him)]

When resolving port prefixed HTTPS records we use the port in the additional info, but that is not part of the record key.

https://searchfox.org/mozilla-central/rev/e37662380b7b5507732a4f37a17da1a2f7802c04/netwerk/protocol/http/HTTPSRecordResolver.cpp#52-53

dns->NewAdditionalInfo(""_ns, mConnInfo->OriginPort(),
                       getter_AddRefs(info));

Ideally, the caller would add the port to the name here and here instead.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1755902

:kershaw, since you are the author of the regressor, bug 1755902, could you take a look?

For more information, please visit BugBot documentation.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1755902

Comment 3

[Valentin Gosu [:valentin] (he/him)]

I'll try to post a fix soon.

Comment 4

[Valentin Gosu [:valentin] (he/him)]

I was looking at writing a test for this and realized that this is actually preffed off.
We'll turn it on in this bug instead.

Comment 5

[Valentin Gosu [:valentin] (he/him)]

I later realized that we change the host to include the port, and that is being used for keying, so there's no issue with

https://searchfox.org/mozilla-central/rev/d6576544301cacc0e393fbc919c53e4e6b0d46ec/netwerk/dns/nsHostResolver.cpp#468-474

if (StaticPrefs::network_dns_port_prefixed_qname_https_rr() &&
    type == nsIDNSService::RESOLVE_TYPE_HTTPSSVC && aPort != -1 &&
    aPort != 443) {
  originHost = Some(host);
  host = nsPrintfCString("_%d._https.%s", aPort, host.get());
  LOG(("  Using port prefixed host name [%s]", host.get()));
}
[...]
    nsHostKey key(host, aTrrServer, type, flags, af,
                  (aOriginAttributes.mPrivateBrowsingId > 0), originSuffix);

Where I think there might be issues is when cancelling the request for example here we should also pass the port.

I'll keep this bug open to flip the pref and enable the feature.

Comment 6

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1860038 - Enable network.dns.port_prefixed_qname_https_rr r=kershaw

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

There is an r+ patch which didn't land and no activity in this bug for 2 weeks.
:valentin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit BugBot documentation.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Fixed the test failures.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fea89f53d2c9
Enable network.dns.port_prefixed_qname_https_rr r=kershaw,necko-reviewers

Comment 10

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/fea89f53d2c9