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

Curve renaming #528

Merged
merged 6 commits into from
Jun 6, 2024
Merged

Curve renaming #528

merged 6 commits into from
Jun 6, 2024

Conversation

matteosz
Copy link
Contributor

@matteosz matteosz commented Jun 3, 2024

This PR addresses partially #384, by:

  • Keeping Ed25519 as is
  • Changing name to variable Ed25519 (previously named curve25519)
  • Changing name of nist to p256

For a future PR we could add the curve25519 (Montgomery representation).

@matteosz matteosz self-assigned this Jun 3, 2024
Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add package level documentation comments for all of these packages to mention their differences and what they implement?

@@ -10,7 +10,7 @@
// are isomorphic to curves having c == 1.
//
// For details see Bernstein et al, "Twisted Edwards Curves", http://eprint.iacr.org/2008/013.pdf
package curve25519
package var_ed25519
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to change package comment on line 1 here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also renaming below "func Param25519() *Param" to "func ParamEd25519() *Param" might be best.

@@ -1,3 +1,3 @@
// Package nist implements cryptographic groups and ciphersuites
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need to change package level comment here

Copy link
Contributor

@AnomalRoil AnomalRoil left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall seeing there are no test vectors for Ed25519 using actual given values and checking computations are correct makes me worried.
But that's for another issue/PR I guess.

group/edwards25519/point.go Outdated Show resolved Hide resolved
Comment on lines 70 to 72
// NewKey returns a formatted curve25519 key (avoiding subgroup attack by requiring
// NewKey returns a formatted var_ed25519 key (avoiding subgroup attack by requiring
// it to be a multiple of 8). NewKey implements the kyber/util/key.Generator interface.
func (c *curve) NewKey(stream cipher.Stream) kyber.Scalar {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like an undesirable change.
Also @ineiti @pierluca this function seems to be applying the clamping from Ed25519, which probably is not the right thing to do for all the other params provided in the param.go file... It might introduce bias or big issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe returns a scalar suitable for use as a secret key instead of naming the curve @matteosz?

Copy link

sonarcloud bot commented Jun 6, 2024

Please retry analysis of this Pull-Request directly on SonarCloud

@AnomalRoil AnomalRoil merged commit ad38788 into drandmerge Jun 6, 2024
7 of 11 checks passed
@AnomalRoil AnomalRoil deleted the curve-rename branch June 6, 2024 10:31
Copy link

sonarcloud bot commented Jun 6, 2024

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

Successfully merging this pull request may close these issues.

2 participants