-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support for generating ed25519 keys and certificates #1097
Conversation
One of the things that this does is remove the go 1.12 as the built target, as the ed25519 package is part of std library... |
Have you considered to use CIRCL for ed signatures? |
Oh, neat idea @armfazh ! I'll start playing with that ;) |
On the same note @armfazh .. I think openssl also supports ed448, so maybe we can support here as well with your implementation? |
So, several tests are missing and some needs to be checked for the ed algorithm:
|
That is a great idea, let me know if the current CIRCL API needs changes. Happy to review concise PRs. |
For sure! I'll finish this PR, and then create another one for ed448 @armfazh . Thanks so much! |
I jumped into some interfaces conversion problems with using circl, as detailed on issue: cloudflare/circl#109 . Let's see if that can be modified and if not, I'll try something else ;) |
This also should be taken into account: golang/go#20544 |
So, @armfazh , I've been playing with different ideas but it does not seem like it will work with CIRCL: the 'CreateCertificate' from the X509 package only supports "The currently supported key types are *rsa.PublicKey, *ecdsa.PublicKey and ed25519.PublicKey" (https://golang.org/pkg/crypto/x509/#CreateCertificate), which forces to use the ed25519 from Golang, or do a "conversion" of some kind (which still will mean that package 25519 is needed, ruling out go 1.12). I'll finish this PR by switching back to Golang's ed25119; but I could maybe start an issue into Golang x/crypto libraries for support of ed448 (now that some applications like Privacy Pass will actually need it) and adding it to x509. cc./ @alxdavids |
One can use circl for signature operations, while being compliant with stdlib types. |
Yes! @armfazh , just did the same! ;) It is sad that we will probably only be able to signing (with ed448, as well).. but well. I'll try to see if in the actual Golang libraries, ed448 can be implemented. I found this issue that mentions that it will be good to have ed448: https://github.com/golang/go/issues?q=is%3Aissue+is%3Aopen+ed448 |
So, @lgarofalo , as part of this PR, I realized that some of the data used for testing is expired, which shouldn't matter while testing as if not we will have to change the data every time that it gets expired. I added a small fix on the previous commit. Let me know if you think is ok ;) |
Codecov Report
@@ Coverage Diff @@
## master #1097 +/- ##
==========================================
+ Coverage 56.27% 56.47% +0.19%
==========================================
Files 77 76 -1
Lines 7309 7354 +45
==========================================
+ Hits 4113 4153 +40
+ Misses 2727 2723 -4
- Partials 469 478 +9
Continue to review full report at Codecov.
|
@armfazh .. maybe you can help me on this (this current PR has lots of debugging, sorry for that): for some reason using the circl ed25519 fails on the s390x machines, and not on the 64 archs. Using the stdlib ed21559 works just fine. Any ideas? |
Let's do baby steps first, so we can make small incremental changes. In the meantime, we can check what makes circl to fail on s390x architecture. |
Perfect @armfazh ! Then, I will:
Thanks so much! |
Ok, this PR is basically done. There is a small thing I found out: the tls package inside the scan folder, that is vendored, has not been update from stdlib in a while. Not sure if it is the best thing to do right now, but the latest version adds there the support for ed25519. |
This should be addressed with #1101 (still a WIP). |
Awesome @ltv511 ! I'll keep and eye on it and wait until that one is merged ;) |
csr/csr.go
Outdated
case ed25519.PrivateKey: | ||
key, err = derhelpers.MarshalEd25519PrivateKey(priv) | ||
if err != nil { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a line missing here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes! I'll add it ;) Thanks for pointing it out!
A small comment: run go mod tidy to remove the unused dependencies and update vendor folder. |
default: | ||
panic("Generate should have failed to produce a valid key.") | ||
} | ||
|
||
csr, err = Generate(priv.(crypto.Signer), req) | ||
csr, err = Generate(priv.(crypto.Signer), req) // TODO: solve |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the problem the todo refers to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this was a remanent of thinking how to change some x509 things..
@@ -241,9 +244,30 @@ func (sp *StandardProvider) Generate(algo string, size int) (err error) { | |||
} | |||
sp.internal.keyPEM = pem.EncodeToMemory(p) | |||
|
|||
sp.internal.priv = priv | |||
case "ed25519": | |||
if size != (ed25519.PublicKeySize * 8) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The size parameter doesn't refer to the actual size of the public key, does it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yes.. this should probably refer to the private key size. I'll change this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is counterintuitively right. ed25519.PrivateKeySize accounts for both the public key (32 bytes) and the seed (32 bytes), totaling 64 bytes. ed25519.go
However, I do not see the point of specifying the key size, since ed25519 is always the same, as I know. I think the field size should be simply ignored when generating ed25519 keys.
Thank you for working on this. Is it ready to go? |
Looks like it's not quite ready to go. @claucece is no longer at Cloudflare, so we may need someone else to champion this PR. |
I stumbled into this trying to support ed25519 in fabric-ca. I merged the current master branch with this pull request's branch https://github.com/johannww/cfssl. Everything compiles. The only test failing is the one on github.com/cloudflare/cfssl/bundler, but it is also failing in the current master branch. I generated an ed25519 key successfully. It seems to work. I will leave some comments in the commit. |
I merged this PRs changes and master into my original PR #1061 |
closing in favor of #1061 |
This PR is WIP, and it is built upon: #1061 by @izolight .
@lgarofalo , I will be putting the new commits over here, so let me know if it is ok ;)