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

ADD: support for the ed25519 curve #8

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

thearchitector
Copy link

@thearchitector thearchitector commented Feb 25, 2023

This PR adds support for the edwards25519 curve using the filippo.io/edwards25519 package (the public release of the low-level package used internally by the Go std lib).

@schollz I'm making this a draft PR because I'm running into some weirdness for which I don't have the Go nor EC mathematical background to understand (my experience with both extends only as far as this tool and PR). From what I can tell, there's something about the random scalars that cause it to incorrectly generate the Zᵤ and Zᵥ coordinates (they don't match). If you pin both the scalar values to known working ones, it always succeeds. If you pin one of them to a known failing one, it always fails. It feels like a mathematical property of some sort, but I looked at the integer values, byte values, and bit strings of the scalars and can't seem to figure out the misbehaving pattern.

Any input would be appreciated so that this (and by extension, croc) can support this curve

@thearchitector
Copy link
Author

@schollz @tscholl2 I'm still interested in getting this working, if either of you have the capacity to take a look.

@joshcangit
Copy link

joshcangit commented Jun 6, 2024

I'll say that more recent versions, since 1.20rc1, of Go already support ed25519.

If this project can be updated, then that extra package won't be needed anyway.

@thearchitector
Copy link
Author

thearchitector commented Jun 7, 2024

I'll say that more recent versions, since 1.20rc1, of Go already support ed25519.

If this project can be updated, then that extra package won't be needed anyway.

are you referring to crypto/ed25519? on 1.22.4 (latest), that package doesn't expose any arithmetic functions, and relies on crypto/internal/edwards25519 to do the math; that internal package says:

However, developers who do need to interact with low-level edwards25519 operations can use filippo.io/edwards25519, an extended version of this package repackaged as an importable module.

which is why I'm using that library here.

@joshcangit
Copy link

joshcangit commented Jun 7, 2024

Well, why not actually use the built-in crypto/internel/edwards25519 like you mentioned?

The notes also said the filippo.io one is not covered by Go 1 Compatibility Promise.
Wouldn't it be better to use the more guaranteed to remain compatible one?

But then, this would mean changing the Go version.
I would recommend 1.19.7 to 1.19.13 or 1.20.2 onwards.

Due to GO-2023-1621, there's a problem with P256 so versions 1.19beta1 to 1.19.6 and 1.20rc1 to 1.20.1 should be avoided.

This would be the case to refactor the NIST curves to use crypto/internal/nistec due to changes in Go crypto.

@schollz
Copy link
Owner

schollz commented Jun 7, 2024

@thearchitector thanks so much for this pr, sorry it took so long to get to it, will try to figure out whats going on

@schollz
Copy link
Owner

schollz commented Jun 7, 2024

@joshcangit, the crypto/internel/edwards25519 does not have IsOnCurve, a necessary function for what we are doing here, so it is not used.

@yonas
Copy link

yonas commented Jun 7, 2024

the crypto/internel/edwards25519 does not have IsOnCurve, a necessary function for what we are doing here, so it is not used.

@schollz Perhaps it would be safer - and end in the, easier - for IsOnCurve to be imported into crypto/internel/edwards25519.

@thearchitector Would you be able to submit a PR to get IsOnCurve merged into the standard library?

@schollz
Copy link
Owner

schollz commented Jun 7, 2024

@yonas great idea, I encourage you to submit a pr

@57194
Copy link

57194 commented Sep 5, 2024

IsOnCurve is deprecated anyway:

Deprecated: the CurveParams methods are deprecated and are not guaranteed to provide any security property. For ECDH, use the crypto/ecdh package. For ECDSA, use the crypto/ecdsa package with a Curve value returned directly from P224, P256, P384, or P521.

So I doubt a PR to re-implement this in stdlib would be accepted by Google.


Edit: FWIW, crypto/ecdh implements X25519:

X25519 returns a Curve which implements the X25519 function over Curve25519 (RFC 7748, Section 5).

Multiple invocations of this function will return the same value, so it can be used for equality checks and switch statements.

I'm not entirely sure how rfc7748 compares to ed25519 or if it's relevant or not. I'm far from a crypto expert.

Based on what crypto/internal/edwards25519 says, though, it seems like filippo.io/edwards25519 is the correct way to go.

@joshcangit
Copy link

From Google's documentation, they deprecated low-level usage of the crypto library for elliptical curves.
I can assume it's meant for use within the crypto and not for us, as in 3rd party usage.

So crypto not using IsOnCurve could mean they don't need it and might not add it.

That leaves the filippo one as the only viable option.

@thearchitector
Copy link
Author

so it sounds like the only thing standing in our way here is "why doesn't the math always work?" 😛

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

Successfully merging this pull request may close these issues.

5 participants