WIP: Adding Ed25519 support

[masonforest]

Hey @g-k thanks for the great work here!

In this PR I’m adding Ed25519 support.

go test -v -run TestWGExamples passes but other tests are still failing. I thought I’d get your feedback on some things before going any further: Ed25119 doesn’t require messages be hashed before signing. I’ve converted the HashFunc to a pointer on Algorithm. Now we can skip hashing if algorithm.HashFunc == nil. Is there some way we can avoid using pointers there or somewhere else to put the check? Let me know what you think.

If it’s easy to make some changes that pass the tests go for it! Or if you’ve got feedback I’d be happy to take another stab when I have a minute.

Thanks!

-Mason

[g-k]

Hey Mason, thanks for the PR it'd be good to have Ed25119 support.

Ed25119 doesn’t require messages be hashed before signing. I’ve converted the HashFunc to a pointer on Algorithm. Now we can skip hashing if algorithm.HashFunc == nil. Is there some way we can avoid using pointers there or somewhere else to put the check?

Pointers make sense. We can check that HashFunc is nil for Ed25119 and an accepted hash for other algorithms.

[masonforest]

Great!

I knew I had another question: Currently LoadPrivateKey returns an ecdsa.PrivateKey. We need the ability to load other key types.

We could either:

1. Add separate functions that return the different key types LoadEddsaPrivateKey, LoadEcdsaPrivateKey etc

or

2. Have that function return an interface{}

It seems like option 2 would require a lot more refactoring as any reference crypto.PrivateKey would need to be replaced with interface{}. Do you have an opinion one way or the other or is there another solution I'm not thinking of?

[g-k]

I'd like to avoid the interface{} in 2. since it's so generic.

I'd say either change LoadPrivateKey to return a crypto.PrivateKey to match its name instead of an ecdsa.PrivateKey or rename LoadPrivateKey to LoadEcdsaPrivateKey and go the 1 route. It's just used in the test helper AFAIK, so we're not tied to either API.

On Tue, Sep 29, 2020 at 5:38 PM Mason Fischer notifications@github.com wrote:

Great!

I knew I had another question: Currently LoadPrivateKey https://github.com/mozilla-services/go-cose/pull/70/files#diff-33a78ff37088f12c18f6ef27128b87dfR890 returns an ecdsa.PrivateKey. We need the ability to load other key types.

We could either:

1. Add separate functions that return the different key types LoadEddsaPrivateKey, LoadEcdsaPrivateKey etc

or

1. Have that function return an interface{}

It seems like option 2 would require a lot more refactoring as any reference crypto.PrivateKey would need to be replaced with interface{}. Do you have an opinion one way or the other or is there another solution I'm not thinking of?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/mozilla-services/go-cose/pull/70#issuecomment-701005228, or unsubscribe https://github.com/notifications/unsubscribe-auth/AABXKLKH726PLX7NIEDINZDSIJHWVANCNFSM4R6GLHYQ .

[masonforest]

Agreed on the ambiguity of interface{} 👍

I originally tried returning a crypto.PrivateKey but then when we match on these switch statements we have a crypto.PrivateKey instead of one of the specific key types 😕

I'll try writing type specific loaders and see how that turns out!

[masonforest]

Alight, I split LoadPrivateKey into two functions and that seems to have worked!

Now I'm down to two failing tests:

=== RUN   TestSignErrors
        Error Trace:    sign_verify_test.go:66
        Error:          Not equal:
                        expected: *errors.errorString(&errors.errorString{s:"hash function is not available"})
                        actual  : *errors.fundamental(Signer of type ES256 cannot generate a signature of type RSAES-OAEP w/ SHA-256)
        Test:           TestSignErrors

and

=== RUN   TestVerifyErrors
        Error Trace:    sign_verify_test.go:235
        Error:          Not equal:
                        expected: *errors.errorString(&errors.errorString{s:"hash function is not available"})
                        actual  : *errors.fundamental(invalid signature length: 14)
        Test:           TestVerifyErrors

Any idea how to build a test without linking hash functions? A quick search didn't come up with anything but I can continue to dig if need be...

[g-k]

Any idea how to build a test without linking hash functions?

Not offhand. I can take a look later this week. Rolling a separate set of tests for Ed25519 would be fine too.

[hwine]

Our apologies, we are no longer making changes to this repository. See README update. Thanks for your interest.