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

dnsdist: Add support for setting Extended DNS Error statuses #13473

Merged
merged 5 commits into from
Nov 10, 2023

Conversation

rgacogne
Copy link
Member

@rgacogne rgacogne commented Nov 9, 2023

Short description

This PR adds support for adding EDNS Extended DNS Error statuses from DNSDist, via the following mechanisms:

  • SetExtendedDNSErrorAction
  • SetExtendedDNSErrorResponseAction
  • DNSQuestion:setExtendedDNSError(infoCode [, extraText])
  • DNSResponse:setExtendedDNSError(infoCode [, extraText])
  • dnsdist_ffi_dnsquestion_set_extended_dns_error(...)

Closes #12572

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)

This PR adds support for adding EDNS Extended DNS Error statuses
from DNSDist, via the following mechanisms:
- `SetExtendedDNSErrorAction`
- `SetExtendedDNSErrorResponseAction`
- `DNSQuestion:setExtendedDNSError(infoCode [, extraText])`
- `DNSResponse:setExtendedDNSError(infoCode [, extraText])`
- `dnsdist_ffi_dnsquestion_set_extended_dns_error(...)`
@coveralls
Copy link

coveralls commented Nov 9, 2023

Pull Request Test Coverage Report for Build 6823512332

Warning: This coverage report may be inaccurate.

We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
To ensure accuracy in future PRs, please see these guidelines.
A quick fix for this PR: rebase it; your next report should be accurate.

  • 54 of 70 (77.14%) changed or added relevant lines in 5 files are covered.
  • 506 unchanged lines in 12 files lost coverage.
  • Overall coverage increased (+1.3%) to 57.667%

Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/dnsdistdist/dnsdist-edns.cc 20 24 83.33%
pdns/dnsdist-lua-actions.cc 22 28 78.57%
pdns/dnsdist.cc 2 8 25.0%
Files with Coverage Reduction New Missed Lines %
pdns/backends/gsql/gsqlbackend.hh 1 97.14%
pdns/iputils.hh 1 74.84%
pdns/packethandler.cc 1 72.58%
pdns/dnsdistdist/dnsdist-healthchecks.cc 3 55.64%
pdns/rcpgenerator.cc 3 90.09%
pdns/iputils.cc 4 44.17%
pdns/tcpiohandler.cc 5 66.52%
pdns/recursordist/test-syncres_cc1.cc 7 88.95%
pdns/dnsdistdist/dnsdist-metrics.cc 11 73.39%
pdns/recursordist/rpzloader.cc 36 8.06%
Totals Coverage Status
Change from base Build 6800541130: 1.3%
Covered Lines: 106002
Relevant Lines: 153099

💛 - Coveralls

Copy link
Member

@chbruyand chbruyand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@rgacogne rgacogne merged commit e5ca975 into PowerDNS:master Nov 10, 2023
74 checks passed
@rgacogne rgacogne deleted the ddist-ede branch November 10, 2023 14:51
@mdavids
Copy link

mdavids commented Aug 6, 2024

[UPDATED the content upon request]

I noticed that the MBZ flag is set to 0x0080 when doing this:

addAction(AndRule({QNameRule('example.nl.'), QClassRule(DNSClass.IN), QTypeRule(DNSQType.TXT)}), SetExtendedDNSErrorAction(4, "We fiddled around with this"))

Then dig +do TXT example.nl @dnsdistresolver.example.nl responds with:

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; MBZ: 0x0080, udp: 1232
; EDE: 4 (Forged Answer): (We fiddled around with this)
;; QUESTION SECTION:

A 'dig +nodo TXT example.nl @dnsdistresolver.example.nl' works okay.

We use:

dnsdist 1.10.0-alpha0.1183.master.g5508d1039 (Lua 5.1.4 [LuaJIT 2.1.0-beta3])
Enabled features: AF_XDP cdb dns-over-quic dns-over-http3 dns-over-tls(gnutls openssl) dns-over-https(h2o nghttp2) dnscrypt ebpf fstrm ipcipher libedit libsodium lmdb protobuf re2 recvmmsg/sendmmsg systemd

And dig v 9.20.0 .

An anonymized pcap file is attached below:
mbz-servfail-nombz-anon.pcap.tar.gz

It has a query for example.nl with EDE addAction enabled as above, a query for servfail.nl with and EDE response from the backend server and a query for example.nl with the EDE addAction disabled. The do-bit was enabled for all queries.

For addResponseAction it's the same thing.

@micheloe
Copy link

micheloe commented Aug 6, 2024

Please share a complete minimal example and / or what you are trying to achieve. Since you omitted how you are actually using this luarule, nobody knows. I cannot reproduce this with the following minimal test configuration:

function luarule(dr)
    dr:setExtendedDNSError(4, "We fiddled around with this")
    return DNSAction.Allow, ""
end

function luaresponserule(dr)
    dr:setExtendedDNSError(4, "We fiddled around with this")
    return DNSResponseAction.Allow, ""
end

setLocal('0.0.0.0:5300')

newServer({ address="1.1.1.1:53" })
addAction(QNameRule("tst.example.com."), LuaAction(luarule))
addResponseAction(QNameRule("tstres.example.com."), LuaResponseAction(luaresponserule))

The test results:

dig -p 5300 tstres.example.com txt @127.0.0.1
; <<>> DiG 9.16.23-RH <<>> -p 5300 tstres.example.com txt @127.0.0.1
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 47747
;; flags: qr rd ra ad; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; EDE: 4 (Forged Answer): (We fiddled around with this)
;; QUESTION SECTION:
;tstres.example.com.		IN	TXT

;; AUTHORITY SECTION:
example.com.		3600	IN	SOA	ns.icann.org. noc.dns.icann.org. 2024041854 7200 3600 1209600 3600

;; Query time: 93 msec
;; SERVER: 127.0.0.1#5300(127.0.0.1)
;; WHEN: Tue Aug 06 14:03:16 CEST 2024
;; MSG SIZE  rcvd: 136
dig -p 5300 tst.example.com txt @127.0.0.1
; <<>> DiG 9.16.23-RH <<>> -p 5300 tst.example.com txt @127.0.0.1
;; global options: +cmd
;; Got answer:
;; ->>HEADER<<- opcode: QUERY, status: NXDOMAIN, id: 5780
;; flags: qr rd ra ad; QUERY: 1, ANSWER: 0, AUTHORITY: 1, ADDITIONAL: 1

;; OPT PSEUDOSECTION:
; EDNS: version: 0, flags:; udp: 1232
; EDE: 4 (Forged Answer): (We fiddled around with this)
;; QUESTION SECTION:
;tst.example.com.		IN	TXT

;; AUTHORITY SECTION:
example.com.		3600	IN	SOA	ns.icann.org. noc.dns.icann.org. 2024041854 7200 3600 1209600 3600

;; Query time: 99 msec
;; SERVER: 127.0.0.1#5300(127.0.0.1)
;; WHEN: Tue Aug 06 14:03:33 CEST 2024
;; MSG SIZE  rcvd: 133

Note that you can also use SetExtendedDNSErrorAction() in an addAction() and a SetExtendedDNSErrorResponseAction() in an addResponseAction() directly.

@Habbie
Copy link
Member

Habbie commented Aug 6, 2024

Also please note that comments on closed tickets/PRs tend to get forgotten! When in doubt, a new Q&A discussion is always a good place.

@mdavids
Copy link

mdavids commented Aug 6, 2024

I updated completely rewrote my initial text with some additional information.

SetExtendedDNSErrorResponseAction() was a good suggestion. Thanks.

@Habbie
Copy link
Member

Habbie commented Aug 8, 2024

Using dnsdist 5508d10, and dig 9.20.0, I cannot reproduce the MBZ issue with the old or the new config that @mdavids posted. As backend I tried 9.9.9.9 and dns4all.

@Habbie
Copy link
Member

Habbie commented Aug 8, 2024

(continued debugging should move to a Discussion or IRC)

@Habbie
Copy link
Member

Habbie commented Aug 8, 2024

@mdavids kindly posted #14547 after which I immediately -did- manage to reproduce.

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

Successfully merging this pull request may close these issues.

dnsdist: Having the ability to add EDNS Extended Errors to responses
6 participants