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

EDNS0 interface needs to be exported #857

Closed
miekg opened this issue Dec 1, 2018 · 8 comments · Fixed by #1041
Closed

EDNS0 interface needs to be exported #857

miekg opened this issue Dec 1, 2018 · 8 comments · Fixed by #1041

Comments

@miekg
Copy link
Owner

miekg commented Dec 1, 2018

The EDNS0 interface is

type EDNS0 interface {
	// Option returns the option code for the option.
	Option() uint16
	// pack returns the bytes of the option data.
	pack() ([]byte, error)
	// unpack sets the data as found in the buffer. Is also sets
	// the length of the slice as the length of the option data.
	unpack([]byte) error
	// String returns the string representation of the option.
	String() string
}

This means I can't externally define a new option; i.e. a bogus 100 one to do some EDNS0 compliance testing.

miekg added a commit to coredns/coredns that referenced this issue Dec 1, 2018
Note I can't test it because of:
miekg/dns#857

But I'm running a test instance and pointing the https://ednscomp.isc.org/ednscomp
to it

Signed-off-by: Miek Gieben <miek@miek.nl>
miekg added a commit to coredns/coredns that referenced this issue Dec 1, 2018
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.
~~~

Signed-off-by: Miek Gieben <miek@miek.nl>
miekg added a commit to coredns/coredns that referenced this issue Dec 1, 2018
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.

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.
~~~

Signed-off-by: Miek Gieben <miek@miek.nl>
@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 1, 2018

This seems sensible to me. Should pack take a []byte to allow it to avoid allocation and be more like PrivateRR?

@miekg
Copy link
Owner Author

miekg commented Dec 1, 2018 via email

@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 1, 2018

It would need a Len method then. Right now (*OPT).len calls pack just for the length.

PrivateRdata has one:

// PrivateRdata is an interface used for implementing "Private Use" RR types, see
// RFC 6895. This allows one to experiment with new RR types, without requesting an
// official type code. Also see dns.PrivateHandle and dns.PrivateHandleRemove.
type PrivateRdata interface {
	// String returns the text presentaton of the Rdata of the Private RR.
	String() string
	// Parse parses the Rdata of the private RR.
	Parse([]string) error
	// Pack is used when packing a private RR into a buffer.
	Pack([]byte) (int, error)
	// Unpack is used when unpacking a private RR from a buffer.
	// TODO(miek): diff. signature than Pack, see edns0.go for instance.
	Unpack([]byte) (int, error)
	// Copy copies the Rdata.
	Copy(PrivateRdata) error
	// Len returns the length in octets of the Rdata.
	Len() int
}

@miekg
Copy link
Owner Author

miekg commented Dec 1, 2018 via email

miekg added a commit to coredns/coredns that referenced this issue Dec 2, 2018
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>
miekg added a commit to coredns/coredns that referenced this issue Dec 5, 2018
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>
miekg added a commit to coredns/coredns that referenced this issue Dec 6, 2018
* 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>
Jason-ZW pushed a commit to rancher/coredns that referenced this issue Apr 17, 2019
* 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>
dna2github pushed a commit to dna2fork/coredns that referenced this issue Jul 19, 2019
* 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>
omeranson added a commit to omeranson/dns that referenced this issue Dec 3, 2019
Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue miekg#857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes miekg#857
omeranson added a commit to omeranson/dns that referenced this issue Dec 4, 2019
Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue miekg#857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes miekg#857
omeranson added a commit to omeranson/dns that referenced this issue Dec 4, 2019
Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue miekg#857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes miekg#857
miekg pushed a commit that referenced this issue Dec 6, 2019
Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue #857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

Closes #857
@tmthrgd
Copy link
Collaborator

tmthrgd commented Dec 8, 2019

#1041 was reverted, see #1045 and #1046. Reopening for decision.

@tmthrgd tmthrgd reopened this Dec 8, 2019
@micahhausler
Copy link

I'm developing a CoreDNS plugin and have a need to set a non-spec'd EDSN0 option, I see that the interface was made exportable and then reverted, is there a plan to make this breaking change tied to a future release? Is there a workaround for this besides forking this repository?

@miekg
Copy link
Owner Author

miekg commented Mar 24, 2020 via email

@miekg
Copy link
Owner Author

miekg commented Mar 21, 2021

We've tried, maybe too hard, maybe something for a v2....

@miekg miekg closed this as completed Mar 21, 2021
aanm pushed a commit to cilium/dns that referenced this issue Jul 29, 2022
Replace all the private methods in the EDNS0 with public methods.
Additionally, as suggested in issue miekg#857, made Pack receive a
pre-allocated byte array, introduce a Len method, and have Pack
and Unpack return the number of octets written and read (respectively)
if there was no error.

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

Successfully merging a pull request may close this issue.

3 participants