Closed Bug 1440563 Opened 2 years ago Closed 2 years ago

TRR::DohEncode should set RD bit

Categories

(Core :: Networking: DNS, defect, P1)

60 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: chantr4, Assigned: bagder)

Details

(Whiteboard: [necko-triaged][trr])

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_13_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/64.0.3282.167 Safari/537.36

Steps to reproduce:

When setting FF nightly 60.0a1 (2018-02-22) (64-bit) (MacOSX) to use a DOH server:
* update to point to network.trr.uri to https://dns.dnsoverhttps.net/dns-query running doh-proxy (https://github.com/facebookexperimental/doh-proxy)
* set network.trr.mode to 2




Actual results:

The queries getting to the DOH server do not have the RD bit set.

When FF performs the network.trr.confirmationNS query, it sends a NS query for example.com without the RD bit set. Recursive resolvers like `unbound` will respond with a REFUSED response (and not even the question set) (see https://github.com/NLnetLabs/unbound/blob/c834b5eecdde3587ad5d2ac757bd90ef0be5a63b/daemon/worker.c#L1282 )

```
$ dig @::1  example.com +nord

; <<>> DiG 9.12.0 <<>> @::1 example.com +nord
; (1 server found)
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: REFUSED, id: 41394
;; flags: qr ad; QUERY: 0, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 0

;; Query time: 0 msec
;; SERVER: ::1#53(::1)
;; WHEN: Fri Feb 23 01:24:13 UTC 2018
;; MSG SIZE  rcvd: 12
```

This can be worked around in unbound by using the following local-data:

local-data: "example.com. 10800 IN NS localhost."

Doing so, indeed help passing the confirmationNS stage, but then the few requests that will follow will also have the RD bit unset, which will cause FF to eventually stop using DOH.

In order to get FF to work with doh-proxy, I can apply https://gist.github.com/chantra/d29bf407fa731ab1078de4c56a54a05c but essentially, no such workaround should be put in place.




Expected results:

The queries getting to the DOH server should have the RD bit set in order to ensure that the recursive DOH server will perform recursion.


https://hg.mozilla.org/mozilla-central/file/2ce4b946d3ac/netwerk/dns/TRR.cpp#l68
should set RD in order for the recursive resolvers to perform recursion.
Component: Untriaged → Networking
Product: Firefox → Core
CCing @bagder given that this is related to bug 1434852
This is an oversight by me in the DNS code. The available servers I've tested with have not objected to these requests and I must've missed the subtlety of the RD bit. I'm back from PTO next week and can fix this then!
Assignee: nobody → daniel
Component: Networking → Networking: DNS
Priority: -- → P1
Whiteboard: [necko-triaged][trr]
@chantr4, can you please check and see if this patch fixes the issue for you?
Flags: needinfo?(chantr4)
Comment on attachment 8955980 [details]
bug 1440563 - set the RD bit in DOH requests

https://reviewboard.mozilla.org/r/224924/#review230924

r+ with tests fixed.
Attachment #8955980 - Flags: review?(valentin.gosu) → review+
I updated FF nightly and I am now running `60.0a1 (2018-03-05)` but the issue is still there. I assume this did not make it to nightly yet?
Flags: needinfo?(chantr4)
Right, I'm interested in your feedback *before* I try to land the change!
(In reply to chantr4 from comment #7)
> I updated FF nightly and I am now running `60.0a1 (2018-03-05)` but the
> issue is still there. I assume this did not make it to nightly yet?

You can try the test build from here: https://queue.taskcluster.net/v1/task/Ymmma9PAT-Gdn3gRdFKuPQ/runs/0/artifacts/public/build/target.dmg
@bagder yes, the patch LGTM. Also, I tested against the build provided by @valentin and it is all good!

Thanks
Thank you!
Pushed by daniel@haxx.se:
https://hg.mozilla.org/integration/autoland/rev/83df6cf24cf8
set the RD bit in DOH requests r=valentin
https://hg.mozilla.org/mozilla-central/rev/83df6cf24cf8
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.