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 ed448 signature scheme. #102

Merged
merged 5 commits into from
Apr 21, 2020
Merged

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Apr 8, 2020

Adds support for Ed448 digital signature scheme as described in RFC8032.

Relies on mlsbset (#99), goldilocks (#100), and internal/sha3 packages.

@armfazh armfazh requested review from wbl and bwesterb April 8, 2020 21:49
@armfazh armfazh self-assigned this Apr 8, 2020
@armfazh armfazh added the enhancement Improvement over something already in the project label Apr 8, 2020
@armfazh armfazh mentioned this pull request Apr 8, 2020
@armfazh
Copy link
Contributor Author

armfazh commented Apr 8, 2020

@claucece please take a look on this PR

@claucece
Copy link
Contributor

claucece commented Apr 9, 2020

Oh! Very nice! I'll check it tomorrow ;)

Copy link
Contributor

@claucece claucece left a comment

Choose a reason for hiding this comment

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

Hey! I added some comments as well as on the other PR ;)

type KeyPair struct{ private, public [Size]byte }

// GetPrivate returns a copy of the private key.
func (k *KeyPair) GetPrivate() PrivateKey { return makeCopy(&k.private) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Mmm.. I'm not so sure of this methods, as they seem more from the OOP thinking.. but maybe they can be useful ;)

_, _ = H.Write(signature[:Size])
_, _ = H.Write(kp.public[:])
_, _ = H.Write(message)
_, _ = H.Read(hRAM[:])
Copy link
Contributor

Choose a reason for hiding this comment

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

As the functionality of these lines is repeated with the previous ones, this can be extracted to a separated function, like 'hashWithDom', or something like it..

@codecov-io
Copy link

codecov-io commented Apr 14, 2020

Codecov Report

Merging #102 into master will increase coverage by 0.38%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #102      +/-   ##
==========================================
+ Coverage   79.21%   79.59%   +0.38%     
==========================================
  Files          60       61       +1     
  Lines        5551     5656     +105     
==========================================
+ Hits         4397     4502     +105     
  Misses       1107     1107              
  Partials       47       47              
Impacted Files Coverage Δ
sign/ed448/ed448.go 100.00% <100.00%> (ø)

)

func TestScalarMult(t *testing.T) {
const testTimes = 1 << 8
Copy link
Contributor

Choose a reason for hiding this comment

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

Not an issue that needs to be addressed right now, but it can be quite nice for things like this is to allow the iterations to vary based on the -short flag. For example:

func NumTrials() {
    if testing.Short() {
        return 16
    }
    return 1 << 8 // or more
}

If you want to get even more fancy, something I've done before is to also define custom -long and -stress flags. So you can do:

// Custom test command line flags.
var (
	long   = flag.Bool("long", false, "enable long running tests")
	stress = flag.Bool("stress", false, "enable stress tests (implies -long)")
)

// timeallowed returns how long a single test is allowed to take.
func timeallowed() time.Duration {
	switch {
	case testing.Short():
		return time.Second / 10
	case *long:
		return 30 * time.Second
	case *stress:
		return 2 * time.Minute
	default:
		return time.Second
	}
}

// Repeat the given trial function. The duration is controlled by custom
// command-line flags. The trial function returns whether it wants to continue
// testing.
//
//	-short		run for less time than usual
//	-long		allow more time
//	-stress		run for an extremely long time
func Repeat(t *testing.T, trial func(t *testing.T) bool) {
	start := time.Now()
	d := timeallowed()
	n := 1
	for time.Since(start) < d && trial(t) {
		n++
	}
	t.Logf("%d trials in %s", n, time.Since(start))
}

I usually put something like this in an internal/test package. Then you wrap your test in a test.Repeat(...) call and it will run until the time limit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a good suggestion, I will open a ticket for tracking your proposal.

@mmcloughlin
Copy link
Contributor

By the way... it's not pretty but it is possible to "stack" PRs in github. You have three PRs A -> B -> C, instead of having them all with target branch master you can set the target branch of B to be A, and target branch of C to be B. Landing them is mildly annoying.

My company has custom tooling for this and it's really sweet. Maybe we should open source it.

Just saying because this PR stack is intimidating to review!

@claucece
Copy link
Contributor

Oh @mmcloughlin , that is very awesome! Does the tool have a GUI or it's from the terminal? I always wanted something like it, and I keep an eye on: isaacs/github#867, isaacs/github#959.

I like this post around the subject very much: https://unhashable.com/stacked-pull-requests-keeping-github-diffs-small/

@mmcloughlin
Copy link
Contributor

Oh @mmcloughlin , that is very awesome! Does the tool have a GUI or it's from the terminal? I always wanted something like it, and I keep an eye on: isaacs/github#867, isaacs/github#959.

I like this post around the subject very much: https://unhashable.com/stacked-pull-requests-keeping-github-diffs-small/

Ah I just realized that our internal tool (we call it bonsai) might be hard to open source since it's tightly integrated with our submit queue, which is another custom tool for our huge monorepo.

I really hope Github comes up with an official solution for this. Seems likely they might... they've been coming out with so many new features since the Microsoft acquisition.

@armfazh
Copy link
Contributor Author

armfazh commented Apr 16, 2020

I addressed most of comments in the review, could you all give another look on this PR.
cc: @claucece @bwesterb

@claucece
Copy link
Contributor

Nice! Thanks for the changes @armfazh ;)

@armfazh armfazh force-pushed the addEd448-goldilocks branch from 2332b5b to 58214a0 Compare April 21, 2020 23:43
@armfazh armfazh merged commit 0442684 into cloudflare:master Apr 21, 2020
@armfazh armfazh deleted the addEd448-goldilocks branch July 24, 2020 23:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improvement over something already in the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants