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 Key utils 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) #454

Merged
merged 1 commit into from Dec 27, 2019

Conversation

@TrustHenry
Copy link
Member

TrustHenry commented Dec 23, 2019

This converts from SecretKey(Ed25519) to Scalar(X25519)
Later this will be used in the Signature of enrollment verification.
#205

@TrustHenry TrustHenry changed the title add SchnorrUtil Publickey(Ed25519) to Point(Curve25519) add SchnorrUtil 'pkToCurvePoint ' Publickey(Ed25519) to Point(Curve25519) Dec 23, 2019
KeyPair kp = KeyPair.fromSeed(
Seed.fromString(
"SCT4KKJNYLTQO4TVDPVJQZEONTVVW66YLRWAINWI3FZDY7U4JS4JJEI4"));
Point point = pkToCurvePoint(kp.address);

This comment has been minimized.

Copy link
@Geod24

Geod24 Dec 23, 2019

Member

Can you add the Scalar corresponding to this Point ?

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 24, 2019

Author Member

It's been solved with the help of Jay.

@@ -209,6 +212,25 @@ public bool verify (T) (const ref Point X, const ref Signature s, auto ref T dat
return S == RcX;
}

/// Publickey(Ed25519) to Point(Curve25519)
public static Point pkToCurvePoint (PublicKey pk) nothrow @nogc

This comment has been minimized.

Copy link
@Geod24

Geod24 Dec 23, 2019

Member
Suggested change
public static Point pkToCurvePoint (PublicKey pk) nothrow @nogc
public Point pkToCurvePoint (PublicKey pk) nothrow @nogc

static is unnecessary as this function is at module level (unlike in C)

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 24, 2019

Author Member

Applied the suggest.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 23, 2019

Codecov Report

Merging #454 into v0.x.x will increase coverage by 0.01%.
The diff coverage is 92.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x     #454      +/-   ##
==========================================
+ Coverage   87.61%   87.63%   +0.01%     
==========================================
  Files          54       54              
  Lines        3713     3727      +14     
==========================================
+ Hits         3253     3266      +13     
- Misses        460      461       +1
Flag Coverage Δ
#integration 53.3% <0%> (-0.14%) ⬇️
#unittests 86.19% <92.85%> (+0.02%) ⬆️
Impacted Files Coverage Δ
source/agora/common/crypto/Key.d 88.88% <92.85%> (+0.59%) ⬆️

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 5652d55...0520ab3. Read the comment docs.

@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch from fd8b35f to 15b1cc3 Dec 24, 2019
@TrustHenry TrustHenry changed the title add SchnorrUtil 'pkToCurvePoint ' Publickey(Ed25519) to Point(Curve25519) add SchnorrUtil 'skToCurveScalar ' SecretKey(Ed25519) to Scalar(X25519) Dec 24, 2019
@TrustHenry

This comment has been minimized.

Copy link
Member Author

TrustHenry commented Dec 24, 2019

Changed from existing code.
SKToCurveScalar method has been added.

@TrustHenry TrustHenry requested a review from Geod24 Dec 24, 2019
@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch from 15b1cc3 to f08a2a9 Dec 24, 2019
Copy link
Contributor

linked0 left a comment

Nice!

@TrustHenry TrustHenry changed the title add SchnorrUtil 'skToCurveScalar ' SecretKey(Ed25519) to Scalar(X25519) Add SchnorrUtil 'skToCurveScalar ' SecretKey(Ed25519) to Scalar(X25519) Dec 26, 2019
Copy link
Member

Geod24 left a comment

Amazing, it works ! After some research, turns out the curves are birationally equivalent. The author of libsodium recommends not using it, but for us, it's just a transitional measure.

But a few things need to be changed:

  1. agora.common.crypto.Schnorr is related to signature operation, while the data structure live in agora.common.crypto.Key (Stellar-style KeyPair) and agora.common.crypto.ECC (Pair).
    Since this function is only for conversion between data structures, it needs to go to either the ECC or Key module. Since we want to transition from Key to ECC, it is better to add the conversion function (and hence, the dependency) to the module we want to get rid of, that is, Key.

  2. It needs better documentation ;)

  3. It needs a better name... "sk" is too abbreviated.

/// SecretKey(Ed25519) to Scalar(X25519)
public Scalar skToCurveScalar (SecretKey secret) nothrow @nogc
{
ubyte[crypto_scalarmult_curve25519_BYTES] x25519_sk;

This comment has been minimized.

Copy link
@Geod24

Geod24 Dec 26, 2019

Member
Suggested change
ubyte[crypto_scalarmult_curve25519_BYTES] x25519_sk;
Scalar x25519_sk;

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 27, 2019

Author Member

Applied the suggest.

pair.v = scalar;
pair.V = pair.v.toPoint();

Pair signature_noise = Pair.random();

This comment has been minimized.

Copy link
@Geod24

Geod24 Dec 26, 2019

Member

It was quite hard to recognize what this is actually doing. You don't need to generate a signature noise yourself, there's one overload which does it for you.
Also, it would be much simpler to just do:

assert(pair.V.data == kp.address.data);

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 27, 2019

Author Member

Applied the suggest.

@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch 2 times, most recently from 1279806 to b738718 Dec 27, 2019
@@ -17,6 +17,8 @@

module agora.common.crypto.ECC;

import agora.common.crypto.Key;
import agora.common.crypto.Schnorr;

This comment has been minimized.

Copy link
@Geod24

Geod24 Dec 27, 2019

Member

The import is because of Pair right ? If so I think it'd be better to move Pair here.

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 27, 2019

Author Member

Pair is only used in unittest.
I removed agora.common.crypto.Schnorr;

This comment has been minimized.

Copy link
@TrustHenry

TrustHenry Dec 27, 2019

Author Member

I moved to Key.D

@TrustHenry TrustHenry changed the title Add SchnorrUtil 'skToCurveScalar ' SecretKey(Ed25519) to Scalar(X25519) Add ECC 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) Dec 27, 2019
@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch 3 times, most recently from 9d8e041 to ac7eeef Dec 27, 2019
@TrustHenry TrustHenry changed the title Add ECC 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) Add Key util 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) Dec 27, 2019
@TrustHenry TrustHenry changed the title Add Key util 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) Add Key utils 'secretKeyToCurveScalar' SecretKey(Ed25519) to Scalar(X25519) Dec 27, 2019
@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch from ac7eeef to e5dae4a Dec 27, 2019
Util to convert SecretKey(Ed25519) to Scalar(X25519)
The secretKeyToCurveScalar() function converts an SecretKey(Ed25519)
to a Scalar(X25519) secret key and stores it into x25519_sk.
@TrustHenry TrustHenry force-pushed the TrustHenry:schnorUtil branch from e5dae4a to 0520ab3 Dec 27, 2019
@TrustHenry

This comment has been minimized.

Copy link
Member Author

TrustHenry commented Dec 27, 2019

Ready for review.

@Geod24
Geod24 approved these changes Dec 27, 2019
Copy link
Member

Geod24 left a comment

LGTM

@Geod24 Geod24 merged commit 989041a into bpfkorea:v0.x.x Dec 27, 2019
5 of 9 checks passed
5 of 9 checks passed
MacOS (macOS-latest, dmd-2.089.1)
Details
MacOS (macOS-latest, dmd-master)
Details
Linux (ubuntu-latest, dmd-2.089.1) Linux (ubuntu-latest, dmd-2.089.1)
Details
Linux (ubuntu-latest, ldc-1.18.0) Linux (ubuntu-latest, ldc-1.18.0)
Details
Linux (ubuntu-latest, dmd-master) Linux (ubuntu-latest, dmd-master)
Details
Linux (ubuntu-latest, ldc-master) Linux (ubuntu-latest, ldc-master)
Details
Travis CI - Pull Request Build Passed
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/dockercloud Your tests passed in Docker Cloud
Details
@TrustHenry TrustHenry deleted the TrustHenry:schnorUtil branch Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.