Add support for Ed25519 DNSSEC signing from RFC 8080 #458

tmthrgd · 2017-02-16T08:43:03Z

This pull request adds support for the Ed25519 algorithm in DNSSEC from RFC 8080. RFC 8080 also specifies the use of Ed448 but there is no standard library implementation of Ed448 so it is not included, this should not be an issue as Ed25519 will always see more use than Ed448.

I'm not sure if this pull request meets the 'Depends only on the standard library.' feature listed in the README. Ed25519 support comes from a Golang sub-repositories, in particular golang.org/x/crypto/ed25519. The sub-repositories are described with:

These packages are part of the Go Project but outside the main Go tree. They are developed under looser compatibility requirements than the Go core. Install them with "go get".

Note: the examples included in the RFC are invalid and were corrected in RFC Errata Report (4935).

miekg · 2017-02-16T17:26:36Z

Thank you. This is a useful addition. I'm however torn about the golang.org/x dep that it adds; yes we could vendor, use the new dep, but it has been something we managed to avoid. I think for that reason alone merging this would be impossible. Is there any chance this code will move into the std lib?

Code looks good btw.

tmthrgd · 2017-02-16T22:19:10Z

Unfortunately I don't have an easy solution for the golang.org/x dep. Some things to note though:

ed25519 was added to golang.org/x/crypto in https://go-review.googlesource.com/22030 on 25th/26th April 2016,
golang.org/x/crypto/ed25519 hasn't had a single commit or change since it was included (see https://go.googlesource.com/crypto/+log/master/ed25519),
the distinct but related package, golang.org/x/crypto/curve25519, was vendored into the standard library in https://go-review.googlesource.com/30822,
golang.org/x/crypto/curve25519 will be used internally by crypto/tls from go1.8 (see https://go-review.googlesource.com/30824),
golang.org/x/crypto/chacha20poly1305 has also been vendored into the standard library to be used internally by crypto/tls (see https://go-review.googlesource.com/30957), and
golang.org/x/crypto/ed25519 is used by golang.org/x/crypto/ssh, which is also a sub-repository, (see https://godoc.org/golang.org/x/crypto/ed25519?importers).

It's quite unfortunate that ed25519 isn't yet part of the standard library. I'm happy for this not to merged and just left open for the indefinite future until either ed25519 becomes crypto/ed25519 or some other solution is arrived at. I've really got no idea when ed25519 might become part of the standard library.

I guess other possible solutions are to simply import golang.org/x/crypto/ed25519 and consider the sub-repositories to be loosely part of the standard library or to vendor it. Neither is ideal of course.

miekg · 2017-02-17T09:54:03Z

I have no problem leaving this PR open until enough stuff sits in the standard lib.

andrewtj · 2017-05-17T07:06:59Z

The example RRSIG records have unbalanced parenthesis. I guess the parsing machinery doesn't explicitly check that it has found the end of an entry?

Thanks for doing this. I'm using it for testing another (non-Go) implementation 👍

miekg · 2017-05-17T08:49:45Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Add support for Ed2..." ]

The example RRSIG records have unbalanced parenthesis. I guess the parsing machinery doesn't explicitly check that it has found the end of an entry?

Geh, interesting bug. It does know when the RR ends, so by virtue of that we don't check the parenthesis. /Miek

…

-- Miek Gieben

tmthrgd · 2017-05-17T11:39:51Z

@andrewtj You're right, and it is an interesting bug. I didn't even notice it, principally because there was no parsing error. It's a mistake in RFC 8080 though, and I missed it when I filed the errata. I'm not familiar enough with the DNS record format to know where it should go. You're really more than welcome! I was very much hoping that getting an implementation out there would help quicken adoption of Ed25519!

@miekg Could that be handled by simply keeping a counter that is incremented with opening parenthesis and decremented with closing parenthesis, and then checking it's equal to zero at the end of the record?

miekg · 2017-05-17T11:47:52Z

[ Quoting <notifications@github.com> in "Re: [miekg/dns] Add support for Ed2..." ]

@andrewtj You're right, and it is an interesting bug. I didn't even notice it, principally because there was no parsing error. It's a mistake in [RFC 8080](https://tools.ietf.org/html/rfc8080) though, and I missed it when I filed the [errata](https://www.rfc-editor.org/errata_search.php?rfc=8080). I'm not familiar enough with the DNS record format to know where it should go. You're really more than welcome! I was very much hoping that getting an implementation out there would help quicken adoption of Ed25519!

oh, nice. Should file an errata ! :)

@miekg Could that be handled by simply keeping a counter that is incremented with opening parenthesis and decremented with closing parenthesis, and then checking it's equal to zero at the end of the record?

(haven't look at the code). Yes, but I believe I'm already doing that.

miekg · 2017-05-19T17:25:03Z

The code check closing braces and errors on that, not superfluous opening braces

miekg · 2017-05-19T17:27:50Z

named-checkzones barf: dns_rdata_fromtext: example.org:8: unbalanced parentheses.
#488

miekg · 2017-11-15T07:29:47Z

The popularity of the algo's and the fact that we don't support them becomes problematic; we should revisit this and probably start vendoring things.

andrewtj · 2017-11-15T07:55:36Z

My only concern with vendoring is the possibility of missing upstream updates. A Travis job should be able to warn of that happening though.

tmthrgd · 2017-11-15T08:04:19Z

@miekg I just resolved the merge conflicts associate with this pull request.

I still think you can get away without vendoring in this case as the golang.org/x/crypto/ed25519 package has proved very stable. So far there have been no changes to the API in it's history. Even if it must be vendored, I think that's a very approachable problem.

Note: The test case from RFC 8080 has been modified to correct the missing final brace, but is otherwise present as-is.

tmthrgd · 2017-11-15T08:45:55Z

The changes have been rebased onto the master branch and the test failure noted in #458 (comment) has been fixed. I'm not sure if this matches the current coding style as I haven't otherwise changed the code since the 16th of February.

codecov-io · 2017-11-15T08:48:31Z

Codecov Report

Merging #458 into master will increase coverage by 0.06%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master     #458      +/-   ##
==========================================
+ Coverage    58.3%   58.37%   +0.06%     
==========================================
  Files          37       37              
  Lines        9865     9924      +59     
==========================================
+ Hits         5752     5793      +41     
- Misses       3064     3072       +8     
- Partials     1049     1059      +10

Impacted Files	Coverage Δ
dnssec_privkey.go	`96.77% <100%> (+0.28%)`	⬆️
dnssec_keygen.go	`66.96% <57.14%> (-1.41%)`	⬇️
dnssec_keyscan.go	`69.35% <58.82%> (-1.06%)`	⬇️
dnssec.go	`61.42% <63.33%> (+0.48%)`	⬆️
ztypes.go	`41.92% <0%> (+0.32%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fc4eb2...538facf. Read the comment docs.

miekg · 2017-11-15T08:58:47Z

My only concern with vendoring is the possibility of missing upstream updates. A Travis job should be able to warn of that happening though.
Some kind of signal we be good, yes.

I'll review this, get this merged; then setup godep to vendor this.

cc @fastest963 we might then also start vendoring the x/net/ stuff and pull out our internal "fork" of it

miekg · 2017-11-15T09:02:32Z

dnssec.go

-	h.Write(wire)
+	switch rr.Algorithm {
+	case ED25519:
+		signature, err := sign(k, append(signdata, wire...), crypto.Hash(0), rr.Algorithm)


I haven't read the rfc/draft. Why does this need to be done like this? (some comments maybe?).
What's crypto.Hash(0)? And why can't we do the double h.Write?

@miekg That's not directly related to the draft but to how ed25519 operates. Most signature schemes operate on hashed data while ed25519 performs the hashing internally and thus requires the original message. While there is a variant of ed25519 that uses pre-hashing, RFC 8080 does not use it – very few things do. Basically ed25519 needs to be passed the raw message without any hashing so it's special cased here.

The crypto.Hash(0) is basically used as a placeholder to mean 'not hashed', it's checked in (ed25519.PrivateKey).Sign.

~~It might be clearer if I switch this to calling ed25519.Sign directly.~~

@miekg Scratch calling ed25519.Sign directly, that would prevent the use of opaque crypto.Signers. I'll improve the documentation here instead.

miekg · 2017-11-15T09:06:04Z

dnssec_keyscan.go

+			if len(p1) != 32 {
+				return nil, ErrPrivKey
+			}
+			_, p, err = ed25519.GenerateKey(bytes.NewReader(p1))


The interface in the ed25519 pkgs uses GenerateKey to reconstruct a key? There is no e.g. setBytes equiv?

@miekg Unfortunately the representation of ed25519 private keys in Golang and RFC 8080 differ. RFC 8080 uses only the seed (or the random private half), while Golang uses seed || public-key – in ed25519 the public key is derived from the seed. ed25519.GenerateKey uses the first 32-bytes read from rand (bytes.NewReader(p1)) as the seed/private-half and then derives the public half from this. I'm using ed25519.GenerateKey in the same fashion as ED25519_keypair_from_seed from BoringSSL.

I'll completely agree that this isn't particularly obvious and I can add documentation to this. I'm not sure exactly what that documentation should be though.

Something like your comment, maybe somewhat condensed?

@miekg I've got something worked out now, but I'm not sure whether to draw the parallel to ED25519_keypair_from_seed.

// ... // // This is used in the same manner as |ED25519_keypair_from_seed| // from BoringSSL.

Thoughts?

OK. Not sure about referencing a C impl. in the comments makes sense for a Go library though? Did you push your changes?

@miekg That was my thinking too. I've pushed them now, I was just waiting to hear what you thought first.

miekg · 2017-11-15T11:37:08Z

dnssec_keyscan.go

@@ -166,6 +171,39 @@ func readPrivateKeyECDSA(m map[string]string) (*ecdsa.PrivateKey, error) {
 	return p, nil
 }

+func readPrivateKeyED25519(m map[string]string) (ed25519.PrivateKey, error) {
+	var p ed25519.PrivateKey
+	// TODO: validate that the required flags are present


Should we do this TODO here now as well? Or just garbage in garbage out?

@miekg I think there is a compelling argument to be strict from the get go, but at the same time this is consistent with all the other key types so I'd be inclined to leave it. It could be worth making them all stricter in a separate pull request though.

tmthrgd · 2017-11-16T04:59:46Z

Does anyone know of any other implementations that could be used for interop testing? I would hate for this pull request to land and later find out that it entrenches some mistaken or invalid behaviour.

andrewtj · 2017-11-16T05:57:21Z

It's in the BIND 9.12 RC and I think Knot and PowerDNS have some support too.

miekg · 2017-11-27T08:17:17Z

planning to merge, add the dep and using dep today-ish

This is PR #458 with the dependency added into it.

* Add support for Ed25519 DNSSEC signing from RFC 8080 Note: The test case from RFC 8080 has been modified to correct the missing final brace, but is otherwise present as-is. * Explain why ed25519 is special cased in (*RRSIG).Sign * Explain use of ed25519.GenerateKey in readPrivateKeyED25519 * Add dep This is PR #458 with the dependency added into it.

miekg · 2017-11-27T10:50:46Z

Merged as #591.

Closing this one. Thanks everybody!

This was added in golang/crypto@5ba7f63082 and can replace the workaround from miekg#458.

This was added in golang/crypto@5ba7f63082 and can replace the workaround from #458.

Add support for Ed25519 DNSSEC signing from RFC 8080

385e4fc

Note: The test case from RFC 8080 has been modified to correct the missing final brace, but is otherwise present as-is.

miekg requested changes Nov 15, 2017

View reviewed changes

tmthrgd added 2 commits November 15, 2017 20:53

Explain why ed25519 is special cased in (*RRSIG).Sign

1854b78

Explain use of ed25519.GenerateKey in readPrivateKeyED25519

538facf

miekg reviewed Nov 15, 2017

View reviewed changes

miekg approved these changes Nov 15, 2017

View reviewed changes

miekg added a commit that referenced this pull request Nov 27, 2017

Add dep

70eab72

This is PR #458 with the dependency added into it.

miekg mentioned this pull request Nov 27, 2017

458+dep #591

Merged

miekg closed this Nov 27, 2017

tmthrgd deleted the eddsa_dnssec branch November 27, 2017 11:07

tmthrgd added a commit to tmthrgd/miekgdns that referenced this pull request Jul 24, 2018

Use RFC 8032 functions added to x/crypto/ed25519

3b493dd

This was added in golang/crypto@5ba7f63082 and can replace the workaround from miekg#458.

tmthrgd mentioned this pull request Jul 24, 2018

Use RFC 8032 functions added to x/crypto/ed25519 #715

Merged

miekg pushed a commit that referenced this pull request Jul 25, 2018

Use RFC 8032 functions added to x/crypto/ed25519 (#715)

1e845a5

This was added in golang/crypto@5ba7f63082 and can replace the workaround from #458.

adam-burns mentioned this pull request Feb 8, 2022

SIG(0) with ED25519 panic: crypto: requested hash function #0 is unavailable #1339

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Ed25519 DNSSEC signing from RFC 8080 #458

Add support for Ed25519 DNSSEC signing from RFC 8080 #458

tmthrgd commented Feb 16, 2017 •

edited

Loading

miekg commented Feb 16, 2017

tmthrgd commented Feb 16, 2017

miekg commented Feb 17, 2017

andrewtj commented May 17, 2017

miekg commented May 17, 2017 via email

tmthrgd commented May 17, 2017 •

edited

Loading

miekg commented May 17, 2017 via email

miekg commented May 19, 2017

miekg commented May 19, 2017

miekg commented Nov 15, 2017

andrewtj commented Nov 15, 2017

tmthrgd commented Nov 15, 2017

tmthrgd commented Nov 15, 2017

codecov-io commented Nov 15, 2017 •

edited

Loading

miekg commented Nov 15, 2017

miekg Nov 15, 2017

tmthrgd Nov 15, 2017 •

edited

Loading

tmthrgd Nov 15, 2017

miekg Nov 15, 2017

tmthrgd Nov 15, 2017

miekg Nov 15, 2017

tmthrgd Nov 15, 2017

miekg Nov 15, 2017

tmthrgd Nov 15, 2017

miekg Nov 15, 2017

tmthrgd Nov 16, 2017

tmthrgd commented Nov 16, 2017

andrewtj commented Nov 16, 2017

miekg commented Nov 27, 2017

miekg commented Nov 27, 2017

Add support for Ed25519 DNSSEC signing from RFC 8080 #458

Add support for Ed25519 DNSSEC signing from RFC 8080 #458

Conversation

tmthrgd commented Feb 16, 2017 • edited Loading

miekg commented Feb 16, 2017

tmthrgd commented Feb 16, 2017

miekg commented Feb 17, 2017

andrewtj commented May 17, 2017

miekg commented May 17, 2017 via email

tmthrgd commented May 17, 2017 • edited Loading

miekg commented May 17, 2017 via email

miekg commented May 19, 2017

miekg commented May 19, 2017

miekg commented Nov 15, 2017

andrewtj commented Nov 15, 2017

tmthrgd commented Nov 15, 2017

tmthrgd commented Nov 15, 2017

codecov-io commented Nov 15, 2017 • edited Loading

Codecov Report

miekg commented Nov 15, 2017

Choose a reason for hiding this comment

tmthrgd Nov 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmthrgd commented Nov 16, 2017

andrewtj commented Nov 16, 2017

miekg commented Nov 27, 2017

miekg commented Nov 27, 2017

tmthrgd commented Feb 16, 2017 •

edited

Loading

tmthrgd commented May 17, 2017 •

edited

Loading

codecov-io commented Nov 15, 2017 •

edited

Loading

tmthrgd Nov 15, 2017 •

edited

Loading