Fix EDNS0 compliance #2357

miekg · 2018-12-01T17:56:49Z

Do SizeAndDo in the server and remove all uses of this from the plugins
and do it in the server in the ScrubWiriter. Also always do it. This
is to get into compliance for https://dnsflagday.net/.

The pkg/edns0 now exports the EDNS0 options we understand; this is
exported to allow plugins add things here. This should be documented
still. We don't echo back options we don't understand.

Note I can't test it because of: miekg/dns#857

Running a test instance and pointing the https://ednscomp.isc.org/ednscomp
to it shows the tests are now fixed:

EDNS Compliance Tester
Checking: 'miek.nl' as at 2018-12-01T17:53:15Z

miek.nl. @147.75.204.203 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok
miek.nl. @2604:1380:2002:a000::1 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok

All Ok
Codes
ok - test passed.

corbot · 2018-12-01T17:56:53Z

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked rajansandeep (via /OWNERS) for a review.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

miekg · 2018-12-01T18:41:42Z

needs more tests and the edns.Supported is (of course) introducing a datarace.

miekg · 2018-12-01T19:29:56Z

this breaks rewrite tests because we allow EDNS option rewriting there (/cc @greenpau )

rdrozhdzh · 2018-12-03T11:36:41Z

from https://tools.ietf.org/html/rfc6891#section-6.2.6

Middleboxes that simply forward requests to a recursive resolver MUST
NOT modify and MUST NOT delete the OPT record contents in either
direction.

Middleboxes that have additional functionality, such as answering
queries or acting as intelligent forwarders, SHOULD be able to
process the OPT record and act based on its contents. These
middleboxes MUST consider the incoming request and any outgoing
requests as separate transactions if the characteristics of the
messages are different.

A more in-depth discussion of this type of equipment and other
considerations regarding their interaction with DNS traffic is found
in [RFC5625].

So, if we have simple config like

.:53 {
  forward 10.20.30.40
}

we "MUST NOT modify and MUST NOT delete the OPT record contents in either direction".

Here we don't know in advance which EDNS options are supported by upstream resolver or by authoritative servers responsible for some zones. So we cannot create a list of supported options.

Perhaps, it is responsibility of authoritative server (or plugins which behave like authoritative servser) to drop all unsupported options from response.

Please, let me know if my understanding of the standard is incorrect.

BTW, which config did you use when testing coredns for EDNS compliance?

miekg · 2018-12-04T08:02:24Z

[ Quoting <notifications@github.com> in "Re: [coredns/coredns] Fix EDNS0 com..." ]

from https://tools.ietf.org/html/rfc6891#section-6.2.6 > Middleboxes that simply forward requests to a recursive resolver MUST NOT modify and MUST NOT delete the OPT record contents in either direction. > Middleboxes that have additional functionality, such as answering queries or acting as intelligent forwarders, SHOULD be able to process the OPT record and act based on its contents. These middleboxes MUST consider the incoming request and any outgoing requests as separate transactions if the characteristics of the messages are different. > A more in-depth discussion of this type of equipment and other considerations regarding their interaction with DNS traffic is found in [RFC5625]. So, if we have simple config like ``` .:53 { forward 10.20.30.40 } ``` we "MUST NOT modify and MUST NOT delete the OPT record contents in either direction".

I'm not sure what the forward does right now here.

Here we don't know in advance which EDNS options are supported by upstream resolver or by authoritative servers responsible for some zones. So we cannot create a list of supported options.

yes, this clashes with the standard and other plugins. Not sure what the solution should look like here.

Perhaps, it is responsibility of authoritative server (or plugins which behave like authoritative servser) to drop all unsupported options from response.

Technically we *can* query the upstream and see what it supports? BIND9 does this as well.

Please, let me know if my understanding of the standard is incorrect.

Think so, but I might need to read 6891 in its entirety, because OPT used to be hop by hop

BTW, which config did you use when testing coredns for EDNS compliance?

~~~ corefile . { file /tmp/miek.nl.signed miek.nl log errors } ~~~ /Miek

…

-- Miek Gieben

rdrozhdzh · 2018-12-04T10:11:56Z

. {
   file /tmp/miek.nl.signed miek.nl
   log
   errors
}

I think in such config the file plugin should drop all unsupported ENDS options, since this plugin creates (not just relays or transmits) DNS response.

Other plugins and coredns itself should not touch any EDNS options in response

miekg · 2018-12-04T12:00:32Z

the file plugin doesn't support edns0 option by itself, so I don't understand we you mean with drop. edns0 must be handled server side, the only things plugins can do is add options.

rdrozhdzh · 2018-12-04T15:53:19Z

request/request.go

@@ -208,6 +208,10 @@ func (r *Request) SizeAndDo(m *dns.Msg) bool {
 		mo.SetUDPSize(o.UDPSize())
 		mo.Hdr.Ttl &= 0xff00 // clear flags

+		if len(o.Option) > 0 {
+			o.Option = supportedOptions(o.Option)


Perhaps it should be smth like

mo.Option = append(mo.Option, supportedOptions(o.Option)...)

i.e. we should not remove (overwrite) existing options from response which might come from upstream server.
Also, we likely need to add check for duplicates in the code above, i.e. don't add option again if it already presents in response OPT record.

no, that will not work, because we whip the entire opt record anyway in forward and proxy right now

@miekg, please check at least if o.Option = should be mo.Option =

Seems, in your solution you remove unknown options from query message, shouldn't you remove them from response?

See also line 23

m.Extra = append(m.Extra, o)

which is called only if response doesn't have OPT record

we've never removed OPT from the response, this PR doesn't change that behavior.

I'm talking not about removing entire OPT RR, but about filtering individual options which you do in supportedOptions().
You take the options list from query OPT record, filter this list and assign back to query OPT. This doesn't affect response OPT record.

... uh yes?

I don't understand what you're getting at:

is this something we need to do?

did we do this before and not anymore?

Is there an actual bug in the PR?

I meant to say that the code

coredns/request/request.go

Lines 211 to 213 in a2ad651

if len(o.Option) > 0 {

o.Option = supportedOptions(o.Option)

}

doesn't make any sense here in the block

if mo := m.IsEdns0(); mo != nil {

because we have return at the end of this block, and the modified object o is not used before return

For comparison, the similar code at

coredns/request/request.go

Lines 226 to 228 in a2ad651

if len(o.Option) > 0 {

o.Option = supportedOptions(o.Option)

}

may have sense, because the modified o object is used in line

coredns/request/request.go

Line 233 in a2ad651

m.Extra = append(m.Extra, o)

before return statement

So, I just asked you to check if this code at lines 211-213 is really needed, or should be modified to make sense

you have a point

@2604

Do SizeAndDo in the server (ScrubWriter) and remove all uses of this from the plugins. Also *always* do it. This is to get into compliance for https://dnsflagday.net/. The pkg/edns0 now exports the EDNS0 options we understand; this is exported to allow plugins add things there. The *rewrite* plugin used this to add custom EDNS0 option codes that the server needs to understand. This also needs a new release of miekg/dns because it triggered a race-condition that was basicly there forever. See: * miekg/dns#857 * miekg/dns#859 Running a test instance and pointing the https://ednscomp.isc.org/ednscomp to it shows the tests are now fixed: ~~~ EDNS Compliance Tester Checking: 'miek.nl' as at 2018-12-01T17:53:15Z miek.nl. @147.75.204.203 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok miek.nl. @2604:1380:2002:a000::1 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok All Ok Codes ok - test passed. ~~~ Signed-off-by: Miek Gieben <miek@miek.nl> Signed-off-by: Miek Gieben <miek@miek.nl>

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg · 2018-12-07T10:30:29Z

I'll also fix the variable naming in that function o and mo are not helping at all

Per comment thread in #2357 which spotted a bug; updated the code and added some comments. This function should probably be redone as some point or made obsolete. Signed-off-by: Miek Gieben <miek@miek.nl>

* core: edns0 tweaks Per comment thread in #2357 which spotted a bug; updated the code and added some comments. This function should probably be redone as some point or made obsolete. Signed-off-by: Miek Gieben <miek@miek.nl> * Remove setting options when m is EDNS0 record Assume upstream set them correctly or a plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

@2604

* Fix EDNS0 compliance Do SizeAndDo in the server (ScrubWriter) and remove all uses of this from the plugins. Also *always* do it. This is to get into compliance for https://dnsflagday.net/. The pkg/edns0 now exports the EDNS0 options we understand; this is exported to allow plugins add things there. The *rewrite* plugin used this to add custom EDNS0 option codes that the server needs to understand. This also needs a new release of miekg/dns because it triggered a race-condition that was basicly there forever. See: * miekg/dns#857 * miekg/dns#859 Running a test instance and pointing the https://ednscomp.isc.org/ednscomp to it shows the tests are now fixed: ~~~ EDNS Compliance Tester Checking: 'miek.nl' as at 2018-12-01T17:53:15Z miek.nl. @147.75.204.203 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok miek.nl. @2604:1380:2002:a000::1 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok All Ok Codes ok - test passed. ~~~ Signed-off-by: Miek Gieben <miek@miek.nl> Signed-off-by: Miek Gieben <miek@miek.nl> * typos in comments Signed-off-by: Miek Gieben <miek@miek.nl>

* core: edns0 tweaks Per comment thread in coredns#2357 which spotted a bug; updated the code and added some comments. This function should probably be redone as some point or made obsolete. Signed-off-by: Miek Gieben <miek@miek.nl> * Remove setting options when m is EDNS0 record Assume upstream set them correctly or a plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

@2604

* Fix EDNS0 compliance Do SizeAndDo in the server (ScrubWriter) and remove all uses of this from the plugins. Also *always* do it. This is to get into compliance for https://dnsflagday.net/. The pkg/edns0 now exports the EDNS0 options we understand; this is exported to allow plugins add things there. The *rewrite* plugin used this to add custom EDNS0 option codes that the server needs to understand. This also needs a new release of miekg/dns because it triggered a race-condition that was basicly there forever. See: * miekg/dns#857 * miekg/dns#859 Running a test instance and pointing the https://ednscomp.isc.org/ednscomp to it shows the tests are now fixed: ~~~ EDNS Compliance Tester Checking: 'miek.nl' as at 2018-12-01T17:53:15Z miek.nl. @147.75.204.203 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok miek.nl. @2604:1380:2002:a000::1 (drone.coredns.io.): dns=ok edns=ok edns1=ok edns@512=ok ednsopt=ok edns1opt=ok do=ok ednsflags=ok docookie=ok edns512tcp=ok optlist=ok All Ok Codes ok - test passed. ~~~ Signed-off-by: Miek Gieben <miek@miek.nl> Signed-off-by: Miek Gieben <miek@miek.nl> * typos in comments Signed-off-by: Miek Gieben <miek@miek.nl>

* core: edns0 tweaks Per comment thread in coredns#2357 which spotted a bug; updated the code and added some comments. This function should probably be redone as some point or made obsolete. Signed-off-by: Miek Gieben <miek@miek.nl> * Remove setting options when m is EDNS0 record Assume upstream set them correctly or a plugin. Signed-off-by: Miek Gieben <miek@miek.nl>

corbot bot requested a review from rajansandeep December 1, 2018 17:56

miekg changed the title ~~Fix EDNS0 compliance~~ WIP: Fix EDNS0 compliance Dec 1, 2018

miekg force-pushed the EDNS0 branch from a5bed2a to 923471c Compare December 1, 2018 19:19

miekg changed the title ~~WIP: Fix EDNS0 compliance~~ Fix EDNS0 compliance Dec 1, 2018

miekg mentioned this pull request Dec 1, 2018

coredns test failures with master and not 1.1.0 miekg/dns#858

Closed

miekg force-pushed the EDNS0 branch from 32049e9 to a266013 Compare December 2, 2018 09:09

miekg requested a review from yongtang December 2, 2018 09:13

rdrozhdzh reviewed Dec 4, 2018

View reviewed changes

miekg added 2 commits December 5, 2018 22:15

typos in comments

a2ad651

Signed-off-by: Miek Gieben <miek@miek.nl>

miekg force-pushed the EDNS0 branch from b46ec53 to a2ad651 Compare December 5, 2018 22:15

miekg merged commit fc667b9 into master Dec 6, 2018

corbot bot deleted the EDNS0 branch December 6, 2018 21:18

miekg mentioned this pull request Dec 9, 2018

core: edns0 tweaks #2385

Merged

gcs278 mentioned this pull request Oct 11, 2022

plugin/cache: cache now uses source query DNSSEC option for upstream #5671

Merged

gcs278 mentioned this pull request Nov 21, 2023

EDNS0 header should not be copied to response as it arrived #6188

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix EDNS0 compliance #2357

Fix EDNS0 compliance #2357

miekg commented Dec 1, 2018

corbot bot commented Dec 1, 2018

miekg commented Dec 1, 2018

miekg commented Dec 1, 2018

rdrozhdzh commented Dec 3, 2018

miekg commented Dec 4, 2018 via email

rdrozhdzh commented Dec 4, 2018

miekg commented Dec 4, 2018

rdrozhdzh Dec 4, 2018

miekg Dec 4, 2018

rdrozhdzh Dec 7, 2018

miekg Dec 7, 2018

rdrozhdzh Dec 7, 2018 •

edited

Loading

miekg Dec 7, 2018

rdrozhdzh Dec 7, 2018

miekg Dec 9, 2018

miekg commented Dec 7, 2018

	if len(o.Option) > 0 {
	o.Option = supportedOptions(o.Option)
	}

Fix EDNS0 compliance #2357

Fix EDNS0 compliance #2357

Conversation

miekg commented Dec 1, 2018

corbot bot commented Dec 1, 2018

miekg commented Dec 1, 2018

miekg commented Dec 1, 2018

rdrozhdzh commented Dec 3, 2018

miekg commented Dec 4, 2018 via email

rdrozhdzh commented Dec 4, 2018

miekg commented Dec 4, 2018

rdrozhdzh Dec 4, 2018

Choose a reason for hiding this comment

miekg Dec 4, 2018

Choose a reason for hiding this comment

rdrozhdzh Dec 7, 2018

Choose a reason for hiding this comment

miekg Dec 7, 2018

Choose a reason for hiding this comment

rdrozhdzh Dec 7, 2018 • edited Loading

Choose a reason for hiding this comment

miekg Dec 7, 2018

Choose a reason for hiding this comment

rdrozhdzh Dec 7, 2018

Choose a reason for hiding this comment

miekg Dec 9, 2018

Choose a reason for hiding this comment

miekg commented Dec 7, 2018

rdrozhdzh Dec 7, 2018 •

edited

Loading