Skip to content
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

pass empty response #7431

Merged
merged 8 commits into from Feb 22, 2019
Merged

pass empty response #7431

merged 8 commits into from Feb 22, 2019

Conversation

@alenichev
Copy link
Contributor

@alenichev alenichev commented Jan 29, 2019

Short description

pass empty responses without errors (BADVERS without soa for example).

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Jan 29, 2019

Allowing a qdcount of 0 is not necessary for a response without SOA, what this does allow is a NoError response without the query. I'm not sure we should allow that, do you have an example of a server returning that kind of response?

@alenichev
Copy link
Contributor Author

@alenichev alenichev commented Jan 29, 2019

Sure - it is a nsd, latest version. Here is result of dig against it, which give us no errors in isc's "EDNS Compliance Tester":

$ dig -p 534 +edns=200 soa vk.com @localhost
../../../../lib/isc/unix/net.c:581: sendmsg() failed: Operation not permitted

; <<>> DiG 9.10.3-P4-Debian <<>> -p 534 +edns=200 soa vk.com @localhost
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: BADVERS, id: 44817
;; flags: qr rd; QUERY: 0, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; Query time: 0 msec
;; SERVER: 127.0.0.1#534(127.0.0.1)
;; WHEN: Tue Jan 29 19:41:49 MSK 2019
;; MSG SIZE  rcvd: 23

dnsdist before patch:

$  dig +edns=200 soa vk.com @localhost
../../../../lib/isc/unix/net.c:581: sendmsg() failed: Operation not permitted

; <<>> DiG 9.10.3-P4-Debian <<>> +edns=200 soa vk.com @localhost
;; global options: +cmd
;; connection timed out; no servers could be reached

and after:

$  dig +edns=200 soa vk.com @localhost
../../../../lib/isc/unix/net.c:581: sendmsg() failed: Operation not permitted

; <<>> DiG 9.10.3-P4-Debian <<>> +edns=200 soa vk.com @localhost
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: BADVERS, id: 13180
;; flags: qr rd; QUERY: 0, ANSWER: 0, AUTHORITY: 0, ADDITIONAL: 1
;; WARNING: recursion requested but not available

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 4096
;; Query time: 0 msec
;; SERVER: 127.0.0.1#53(127.0.0.1)
;; WHEN: Tue Jan 29 19:47:11 MSK 2019
;; MSG SIZE  rcvd: 23

@rgacogne rgacogne added this to the dnsdist-1.3.x milestone Jan 30, 2019
@rgacogne
Copy link
Member

@rgacogne rgacogne commented Feb 4, 2019

Alright, I'm not too happy about this but I can guess we need to allow such responses if NSD is sending them. This does increase the risk of collision a lot, though, since we can't check that the qname, qtype and qclass match the ones in the initial query, so perhaps we should turn that into an option and not accept it by default.
We also need to remove a regression test that checks explicitly that we don't let these kind of empty responses go through:

======================================================================
FAIL: Basics: Header-only NoError response should be dropped
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/build/PowerDNS/pdns/regression-tests.dnsdist/test_Basics.py", line 403, in testHeaderOnlyNoErrorResponse
    self.assertEquals(receivedResponse, None)
AssertionError: <DNS message, ID 65339> != None

@rgacogne
Copy link
Member

@rgacogne rgacogne commented Feb 14, 2019

Would you mind making this optional? Then we can worry about fixing the failing test, or I can do it if you prefer.

@alenichev
Copy link
Contributor Author

@alenichev alenichev commented Feb 14, 2019

new patch added; not sure about option name - setAllowEmptyResponse()

pdns/dnsdist.cc Outdated Show resolved Hide resolved
@rgacogne rgacogne merged commit dd8b587 into PowerDNS:master Feb 22, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@aerique aerique mentioned this pull request Mar 21, 2019
8 tasks done
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants