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.

Sign up for GitHub

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

Jump to bottom

x/crypto/nacl: add support for sign #24350

Closed
FlorianUekermann opened this issue Mar 11, 2018 · 7 comments
Closed

x/crypto/nacl: add support for sign #24350

FlorianUekermann opened this issue Mar 11, 2018 · 7 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Unreleased

Comments

@FlorianUekermann
Copy link
Contributor

FlorianUekermann commented Mar 11, 2018 •
edited

cc @agl

There was a ticket for this 2 years ago (#15222).
The reasoning for not adding sign in the comments on that ticket was to wait for NaCl to add Ed25519 support. However, there hasn't been an update to the original codebase since 2011, while other implementations have added Ed25519 support.

I don't really understand the hesitation in adding Ed25519 sign, given that both TweetNaCl (by the original NaCl authors) as well as libsodium (which is by far the most popular implementation) and several other implementations do exactly that (to my understanding, but I'm not an expert).
Furthermore it seems like the existing Ed25519 implementation contains all the necessary pieces.

Given the popularity of the NaCl interface supporting all of the core functionality seems quite important. Could you comment on the situation? If you want to move forward with this I'm happy to contribute some code.
@gopherbot gopherbot added this to the Unreleased milestone Mar 11, 2018
@andybons andybons added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 12, 2018
@andybons
Copy link
Member

/cc @FiloSottile

@agl
Copy link
Contributor

agl commented Mar 12, 2018

Agreed that NaCl is probably not going to add support for Ed25519. Now that the RFC is published, and we have Ed25519 in x/crypto, adding a small wrapper around it would be fine. One thing to keep in mind is that the RFC defines the signatures to be non-malleable by requiring s to be minimal. We might want to standardise on that if things like libsodium have.

@FlorianUekermann
Copy link
Contributor Author

FlorianUekermann commented Mar 13, 2018 •
edited

I took a look at the libsodium signature checking code regarding the non-malleability question. It looks like it does check for non-malleability by default. I'll document the details here so you can double check.

The call chain looks like this crypto_sign_open() -> crypto_sign_ed25519_open() -> crypto_sign_ed25519_verify_detached() -> sc25519_is_canonical()

That last call only happens if ED25519_COMPAT is not defined.

sc25519_is_canonical() does the 0<=S<l check described in RFC8032 , at least it looks like that to me (someone double check those two lines in the do-while loop please) and is mostly identical the code introduced in jedisct1/libsodium@62911ed.

So it looks like libsodium does conform to the RFC. What does golang.org/x/crypto/ed25519 do?

@FlorianUekermann FlorianUekermann mentioned this issue Mar 13, 2018
Closed
@andybons andybons added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. and removed NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. labels Mar 13, 2018
@gopherbot
Copy link

Change https://golang.org/cl/100436 mentions this issue: ed25519: require canonical signatures

@agl
Copy link
Contributor

agl commented Mar 13, 2018

I think ed25519 should enforce canonical signatures too if libsodium is. Please see linked CL.

@FlorianUekermann
Copy link
Contributor Author

Great. That makes nacl/sign fairly trivial. I have the code and tests, which I'll send as soon as the CL is merged.

gopherbot pushed a commit to golang/crypto that referenced this issue Mar 13, 2018
@agl
ed25519: require canonical signatures
c4a91bd 
https://tools.ietf.org/html/rfc8032#section-5.1.7 requires that s be in
the range [0, order) in order to prevent signature malleability. This is
a new requirement in the RFC so the ed25519 package predates it. This
change aligns the code with the RFC.

The linked bug says that libsodium is also enforcing this check by
default.

See golang/go#24350

Change-Id: Ib69ce7c9e5a58971cbe225318d9fd87660bd5e4b
Reviewed-on: https://go-review.googlesource.com/100436
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/100578 mentions this issue: x/crypto/nacl/sign: add package

maxtaco pushed a commit to keybase/go-crypto that referenced this issue Mar 28, 2018
@agl @maxtaco
ed25519: require canonical signatures
ea797db 
https://tools.ietf.org/html/rfc8032#section-5.1.7 requires that s be in
the range [0, order) in order to prevent signature malleability. This is
a new requirement in the RFC so the ed25519 package predates it. This
change aligns the code with the RFC.

The linked bug says that libsodium is also enforcing this check by
default.

See golang/go#24350

Change-Id: Ib69ce7c9e5a58971cbe225318d9fd87660bd5e4b
Reviewed-on: https://go-review.googlesource.com/100436
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Mar 20, 2019
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 28, 2022
@FlorianUekermann @agl
nacl/sign: add package
99a28df 
This change adds the equivalents of crypto_sign, crypto_sign_open and
crypto_sign_keypair in TweetNaCl and libsodium using the Ed25519 system.
The original NaCl codebase does not contain functions with identical
semantics but its documentation stated the intent of using Ed25519 in
future releases.

Fixes golang/go#24350

Change-Id: I4c3c86b4875f2f718ad9299c2274b4ad9e11fbeb
Reviewed-on: https://go-review.googlesource.com/100578
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@FlorianUekermann @agl
nacl/sign: add package
273c2fa 
This change adds the equivalents of crypto_sign, crypto_sign_open and
crypto_sign_keypair in TweetNaCl and libsodium using the Ed25519 system.
The original NaCl codebase does not contain functions with identical
semantics but its documentation stated the intent of using Ed25519 in
future releases.

Fixes golang/go#24350

Change-Id: I4c3c86b4875f2f718ad9299c2274b4ad9e11fbeb
Reviewed-on: https://go-review.googlesource.com/100578
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
c-expert-zigbee pushed a commit to c-expert-zigbee/crypto_go that referenced this issue Mar 29, 2022
@FlorianUekermann @c-expert-zigbee
nacl/sign: add package
0831d7f 
This change adds the equivalents of crypto_sign, crypto_sign_open and
crypto_sign_keypair in TweetNaCl and libsodium using the Ed25519 system.
The original NaCl codebase does not contain functions with identical
semantics but its documentation stated the intent of using Ed25519 in
future releases.

Fixes golang/go#24350

Change-Id: I4c3c86b4875f2f718ad9299c2274b4ad9e11fbeb
Reviewed-on: https://go-review.googlesource.com/100578
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
LewiGoddard pushed a commit to LewiGoddard/crypto that referenced this issue Feb 16, 2023
@FlorianUekermann @agl
nacl/sign: add package
a4acb39 
This change adds the equivalents of crypto_sign, crypto_sign_open and
crypto_sign_keypair in TweetNaCl and libsodium using the Ed25519 system.
The original NaCl codebase does not contain functions with identical
semantics but its documentation stated the intent of using Ed25519 in
future releases.

Fixes golang/go#24350

Change-Id: I4c3c86b4875f2f718ad9299c2274b4ad9e11fbeb
Reviewed-on: https://go-review.googlesource.com/100578
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
BiiChris pushed a commit to BiiChris/crypto that referenced this issue Sep 15, 2023
@FlorianUekermann @agl
nacl/sign: add package
a5a1f2b 
This change adds the equivalents of crypto_sign, crypto_sign_open and
crypto_sign_keypair in TweetNaCl and libsodium using the Ed25519 system.
The original NaCl codebase does not contain functions with identical
semantics but its documentation stated the intent of using Ed25519 in
future releases.

Fixes golang/go#24350

Change-Id: I4c3c86b4875f2f718ad9299c2274b4ad9e11fbeb
Reviewed-on: https://go-review.googlesource.com/100578
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Unreleased
Development

No branches or pull requests
4 participants
@agl @andybons @FlorianUekermann @gopherbot