New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix dns01 authority check #1398

Merged
merged 4 commits into from Jan 21, 2016

Conversation

Projects
None yet
5 participants
@r0ro
Copy link
Contributor

r0ro commented Jan 21, 2016

Don't expect all txt dns replies to contain an authority section.
Server MAY return an authority section, especially on NXDOMAIN the server will return an SOA authority response in order to provide the nxdomain ttl value.
Otherwise there is no need for such section.
Dns client should be checking the header aa flags to check if the response is authoritative and not check the presence of authority section.

Fix #1391

r0ro added some commits Jan 21, 2016

don't expect all txt dns replies to contain an authority section
Server *MAY* return an authority section, especially on NXDOMAIN
the server will return an SOA authority response in order to
provide the nxdomain ttl value.
Otherwise there is no need for such section.
Dns client should be checking the header aa flags to check if the
response is authoritative and not check the presence of authority
section.
only add soa authority response for nxdomain or not entry
This is more what we expect from a dns server.

dig A nx.google.com @ns2.google.com

; <<>> DiG 9.9.5-3ubuntu0.7-Ubuntu <<>> A nx.google.com @ns2.google.com
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 28643
;; flags: qr aa rd; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 0
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;nx.google.com.                 IN      A

;; AUTHORITY SECTION:
google.com.             60      IN      SOA     ns4.google.com. dns-admin.google.com. 112672771 900 900 1800 60

;; Query time: 13 msec
;; SERVER: 216.239.34.10#53(216.239.34.10)
;; WHEN: Thu Jan 21 14:44:06 CET 2016
;; MSG SIZE  rcvd: 81

VS

dig A www.google.com @ns2.google.com

; <<>> DiG 9.9.5-3ubuntu0.7-Ubuntu <<>> A www.google.com @ns2.google.com
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 18684
;; flags: qr aa rd; QUERY: 1, ANSWER: 6, AUTHORITY: 0, ADDITIONAL: 0
;; WARNING: recursion requested but not available

;; QUESTION SECTION:
;www.google.com.                        IN      A

;; ANSWER SECTION:
www.google.com.         300     IN      A       64.233.184.99
www.google.com.         300     IN      A       64.233.184.105
www.google.com.         300     IN      A       64.233.184.106
www.google.com.         300     IN      A       64.233.184.104
www.google.com.         300     IN      A       64.233.184.147
www.google.com.         300     IN      A       64.233.184.103

;; Query time: 13 msec
;; SERVER: 216.239.34.10#53(216.239.34.10)
;; WHEN: Thu Jan 21 14:44:32 CET 2016
;; MSG SIZE  rcvd: 128
@danp

This comment has been minimized.

Copy link

danp commented Jan 21, 2016

To go along with my comment, the dig output you include is from talking to authoritative servers, not caching resolvers.

This output below is more what I think things should be based on, from talking to 8.8.8.8, a caching resolver. Note the AA bit is not set for any responses and no authority records are included when a positive answer is returned.

I do think the changes this PR makes will fix issues with DNS validation, based on my current understanding.

NXDOMAIN:

% dig nx.google.com @8.8.8.8

; <<>> DiG 9.8.3-P1 <<>> nx.google.com @8.8.8.8
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 28282
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 0

;; QUESTION SECTION:
;nx.google.com.         IN  A

;; AUTHORITY SECTION:
google.com.     59  IN  SOA ns2.google.com. dns-admin.google.com. 112683471 900 900 1800 60

;; Query time: 54 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Thu Jan 21 12:05:24 2016
;; MSG SIZE  rcvd: 81

NODATA (name exists but there are no records of the requested type):

% dig google.com srv @8.8.8.8

; <<>> DiG 9.8.3-P1 <<>> google.com srv @8.8.8.8
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 54715
;; flags: qr rd ra; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 0

;; QUESTION SECTION:
;google.com.            IN  SRV

;; AUTHORITY SECTION:
google.com.     59  IN  SOA ns4.google.com. dns-admin.google.com. 112683471 900 900 1800 60

;; Query time: 67 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Thu Jan 21 12:06:15 2016
;; MSG SIZE  rcvd: 78

an answer:

% dig google.com txt @8.8.8.8

; <<>> DiG 9.8.3-P1 <<>> google.com txt @8.8.8.8
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NOERROR, id: 45800
;; flags: qr rd ra; QUERY: 1, ANSWER: 1, AUTHORITY: 0, ADDITIONAL: 0

;; QUESTION SECTION:
;google.com.            IN  TXT

;; ANSWER SECTION:
google.com.     3599    IN  TXT "v=spf1 include:_spf.google.com ~all"

;; Query time: 51 msec
;; SERVER: 8.8.8.8#53(8.8.8.8)
;; WHEN: Thu Jan 21 12:06:33 2016
;; MSG SIZE  rcvd: 76
@r0ro

This comment has been minimized.

Copy link
Contributor Author

r0ro commented Jan 21, 2016

This PR removes the check of authority section presence. It does not introduce any requirement on AA flag. I only mentioned it in case the authority presence check was introduced for the purpose of validating authoritative responses.

@danp

This comment has been minimized.

Copy link

danp commented Jan 21, 2016

Understood, thought it might be worth tweaking the commit message that includes dig output to show what a caching resolver returns instead of an authoritative server as it does now.

Either way, thanks for putting this PR together! Ran into what it fixes this morning.

@rolandshoemaker

This comment has been minimized.

Copy link
Member

rolandshoemaker commented Jan 21, 2016

Boulder expects to be talking authoritative caching DNS server. The RFCs for the protocol indicate that returning a authority section (containing either SOA or NS records) is a SHOULD on either positive or negative responses.

Unfortunately a bunch of implementors have chosen to only return this section on negative responses, something we didn't see in staging due to far less traffic. I would much rather we had some way of identifying the remote DNS server we were talking to (or at least have a possible list of them) but I think this is currently the best short term solution until we can figure something else out.

@jmhodges

This comment has been minimized.

Copy link
Contributor

jmhodges commented Jan 21, 2016

While you're here, mind dropping an "json:",omitempty" on ValidationRecord's Authorities? That would close #1390

That or always making sure Authorities is an actual empty array instead of nil

@r0ro

This comment has been minimized.

Copy link
Contributor Author

r0ro commented Jan 21, 2016

The RFCs for the protocol indicate that returning a authority section (containing either SOA or NS records) is a SHOULD on either positive or negative responses.

Can you point me to the RFC you are referring ? I wasn't able to find a relevant section earlier

While you're here, mind dropping an "json:",omitempty" on ValidationRecord's Authorities? That would close #1390

done

@jmhodges jmhodges added the r=jmhodges label Jan 21, 2016

jmhodges added a commit that referenced this pull request Jan 21, 2016

@jmhodges jmhodges merged commit 31fe1a9 into letsencrypt:master Jan 21, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rolandshoemaker

This comment has been minimized.

Copy link
Member

rolandshoemaker commented Jan 21, 2016

@r0ro I did most of my research last night, currently pretty deep in other patch work but will try to dig up the relevant sections later if I get time. It could also just me incorrectly interpreting some of the rather vague/contradicting language in those DNS RFCs.

@r0ro

This comment has been minimized.

Copy link
Contributor Author

r0ro commented Jan 21, 2016

any idea when it will make it to the production server ?
I'm sorry I missed this on the staging saver. I did my testing before this change was made :(

@danp

This comment has been minimized.

Copy link

danp commented Jan 21, 2016

Can you share what staging and production use for cmd.Config.Common.DNSResolver?

@jmhodges

This comment has been minimized.

Copy link
Contributor

jmhodges commented Jan 21, 2016

@r0ro likely sometime late next week

@dpiddy it points to an internal unbound resolver

@danp

This comment has been minimized.

Copy link

danp commented Jan 21, 2016

@jmhodges thanks. Curious about the behavior described here. Will do some digging.

@danp

This comment has been minimized.

Copy link

danp commented Jan 28, 2016

Was able to use the DNS challenge in production after the deploy with https://github.com/dpiddy/letsencrypt-dnsimple, thanks all!

@NotSqrt

This comment has been minimized.

Copy link

NotSqrt commented Jan 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment