Implement MAYO #483

ilway25 · 2024-02-29T07:29:00Z

We chose to implement the newer version of MAYO proposed by the authors instead of the one submitted to NIST.

The authors proposed the change to the spec here:
"Nibbling MAYO: Optimized Implementations for AVX2 and Cortex-M4" by Ward Beullens, Fabio Campos, Sofía Celi, Basil Hess, Matthias J. Kannwischer.

This pure Go code is written based on the tricks described in that paper and in their reference C code, specifically the nibbling-mayo branch.

It also passes the KAT tests.

Closes #482

bwesterb · 2024-02-29T08:45:17Z

Thank you for this. I will review over the coming days.

bwesterb

This is only a partial review. There is a lot to like in here, but it still needs work to be easier to review: you need to explain how things are encoded, computed, etc.

Also, did you implement this from scratch or did you translate an existing implementation?

bwesterb · 2024-02-29T08:31:07Z

sign/mayo/templates/signapi.templ.go

+// The previous line (and this one up to the warning below) is removed by the
+// template generator.
+
+// Code generated from modePkg.templ.go. DO NOT EDIT.


bwesterb · 2024-02-29T08:32:36Z

sign/mayo/templates/params.templ.go

+	V = N - O
+
+	// The division by 2 converts the number of nibbles to bytes (when packed together).
+	// We don't explicitly round up beause given parameters ensure this will not happen.


bwesterb · 2024-02-29T08:36:12Z

sign/mayo/internal/common/nist/drbg.go

@@ -0,0 +1,71 @@
+// Package nist implements helpers to generate NIST's Known Answer Tests (KATs).


Why not use github.com/cloudflare/circl/internal/nist?

github.com/cloudflare/circl/internal/nist does not implement io.Reader, which was somehow used during the debug process when implementing MAYO. Can change back to this one.

Please ignore the previous comment. I think it would be better if github.com/cloudflare/circl/internal/nist implements io.Reader because MAYO's sign takes a random source, which is also used during KAT tests.

bwesterb · 2024-02-29T08:38:18Z

sign/mayo/mode1/internal/mayo_test.go

+		seed       string
+		expectedPk string
+	}{
+		{"MAYO_1", "7c9935a0b07694aa0c6d10e4db6b1add2fd81a25ccb14803", "5b61421edc1c90efaf6075560f0206173555c55eb845ef3a70ee5ef18618361883a0ade490e9e5bc51cd2b25fb4ec9147a2fa6735d8749ed7e149330a30b3ab62b7991c2494cb21d0bc011ceb357597b8afef939a1fe398421c88f43fb4fe380c10caa3a95e507ebae5a571fd958e0505d994566263be8db31ac833aa20ae7ec97d1a865bb292e89d583975009046053bf91ce7d73e9c23c4244071e17a1095f3918330559f323ce557d66b8771f0003cccaeea82314088d56455d72f258e4c35fc6ce2d195cdae99f359a8307ec15f27da4d0634531cd9a1a719121f7c9b99a0d0c24252316f1ea7c16d73f5ecf7125ad0c8d2e02574131504875af5ab6dea1c328e71122f0cdd09e826cc515b4ec160f31d63253dd8798b2d841e80a28479bcca320853a459d4d659df695eea4b5aeb8f45f6c5fce283c64dd6fe928d452be3723f8b448650ebb72acf231e3c27683afcdec22037f1af29c479db1a97448ce570fef082052c9ef45179ba62abd7dd156f17f4da51557383d40a9d544cdf5cac4f4cc1ce69326a3ccc611fb7a6d04126dc54c55c2286a796be18958846d53e1e3f55f3ac0f4e09d8f302878f4993e25c2164970b337d0364aa61eff5d85e41fe784fd57420a5481b9a15f02a47e3e49c0709a37352a50fd0feb7e7f9938173b3e3c570a79c2424f5dc5bf95363b6d5d764e1a38a1aa05a31bbb9858b51fe3cbb19eaade5d4f482260838f5273b042f6340236395f6c347d4bfb922752ad0b17f1bae8645a9a6b1d72e1e91b4828faa313dd85ec5795252dcd95aa33519a56d740cbab7b9ee13e1b8add763bd07a25455e44e60f664846e32eec329c46e0741b6649c34358905d9d01124c3c8ae14e3fb47cc6477ba8e63bd4a3937588251952f5232bb9999aef509414e11a36d110ea24bedc235c55ab6f80116b036845e291148135a32b0d44fe5a4e9ea9f3ae150e11c62ea91318129f79318d3506ba50db3dbe235427d405a5baa9e6192a17014dd19863087445eb4cc9b6164aab3d5af4e73bc3edd97c76dabba0bc26c43d70f7880a0f7c042465da77ae3f8a31fa65903d90797fc0b5deddf578d52940040276c9b415d880e3782e03a2965a6d3b265628c690bd1b096a355362eb751d528fbc6fa365d1cf3d39cf49d8296c28fe8eb52a31a07e877d1e57e91971cbbd14216db76a08da45258a1a801d0cb232a87280df4522329ecd9312e5489d90a39547735cbd11493d0d86389af40c1b9ec5a993d0cbc508da424392695e1cdf205a294122412fac5a2b4d6e0ed5eed7b4de18f0d4b81f746753fa8744ab845c18800befc926d3290531053bf2cecf23d1b6d7fa9fb8d4827e87abf574afe39a636dbb9fce48854c641981f236da01bd8a5901871891aac7fa1f02300101540e7b05665e5f19912ee898666b64bb3cef688577c7b5482e123c0e8fcc466302eca8fc02f35260569cc1d4b00662be924f188226dbc6086d4aaed1ff9c7604748a556d00f87823f9fafaccf752d645b76a5851723274737e4dd7d5d76fdb256e3f1ef733914eb36932eb1c4c666df436351fb817e7055fbfca2b3103fcec84233feaf3fefd600721d3ca1514ee26db6a608dd4333377a9e36dd2dbadb1651eb99d8e3b4fe6"},


You duplicate test vectors for each mode across each mode. You can put these tests in the sign/mayo/mayo_test.go instead.

Also perhaps just put the hash of the desired outcome here.

I do not quite agree with this suggestion, because

we do not have an extra Mode layer like Dilithium does

this is intended as the internal implementation unit test for each mayo variant, not integration test

Maybe just keep test vectors that belong to each mode in their respective module? (Then we need also to skip mayo_test.go ingen.go)

I see you added a sign/mayo/mayo_test.go. I like it. There are some duplicate tests between that one and in the subpackages.

bwesterb · 2024-03-03T22:31:40Z

sign/mayo/mode1/internal/mayo.go

+type (
+	PrivateKey [PrivateKeySize]byte
+	PublicKey  [PublicKeySize]byte
+)


In Circl we assume that the PrivateKey and PublicKey types can contain cached data, so it's more natural to use the expanded forms directly for PublicKey and PrivateKey.

Also it'll probably help with reducing allocations.

We made the conclusion that key expansion of pk and sk should go with keygen but not sign or verify.

Does this mean the keygen will expand the pk and sk, even though under some circumstances one just wants to store these new keys without doing signing and verification? (expansion will be wasted)

Does this mean the keygen will expand the pk and sk, even though under some circumstances one just wants to store these new keys without doing signing and verification? (expansion will be wasted)

Yes, that's a downside. Keygen is the rarest operation, so I think it's alright.

bwesterb · 2024-03-04T00:28:55Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func variableTime(sps []uint64, p1 []uint64, p2 []uint64, p3 []uint64, s []uint8) {


This function needs a better name, and a description of what it does.

bwesterb · 2024-03-04T00:31:32Z

sign/mayo/mode1/internal/matrix.go

+// p is always P, but is still kept to be consistent with other functions
+//
+//nolint:unparam
+func vecAddPacked(p int, in []uint64, acc []uint64) {


If p is always P, then just use P and not have the parameter.

Here a design choice was made so that this is consistent with other similar functions which can be sometimes called with p != P.

bwesterb · 2024-03-04T00:36:26Z

sign/mayo/mode1/internal/matrix.go

+func variableTime(sps []uint64, p1 []uint64, p2 []uint64, p3 []uint64, s []uint8) {
+	var accumulator [K * N][P * 16]uint64
+
+	// compute P * S^t = [ P1  P2 ] * [S1] = [P1*S1 + P2*S2]


Missing transpose on S1 and S2?

bwesterb · 2024-03-04T00:39:39Z

sign/mayo/mode1/internal/matrix.go

+}
+
+func variableTime(sps []uint64, p1 []uint64, p2 []uint64, p3 []uint64, s []uint8) {
+	var accumulator [K * N][P * 16]uint64


Explain the strategy.

bwesterb · 2024-03-04T00:40:57Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func aggregate(p int, bins [P * 16]uint64, out []uint64) {


Explain what this function does.

This function is only called with P. Why bother with the parameter?

bwesterb · 2024-03-05T02:52:35Z

sign/mayo/mode1/internal/matrix.go

+		t := in[i] & lsb
+		acc[i] ^= ((in[i] ^ t) >> 1) ^ (t * 9)
+	}
+}


This is Figure 2 method 3 right? Wouldn't method 2 be faster as we're not using AVX2 instructions?

Here we chose to multiply by x^-1 all the way through, unlike Method 3 which interleaves *x and *x^-1 which probably gives more parallelism on more complex CPUs. On my MacBook M1 Pro, Method 2 was not faster.

ilway25 · 2024-03-05T06:12:53Z

This is only a partial review. There is a lot to like in here, but it still needs work to be easier to review: you need to explain how things are encoded, computed, etc.

Also, did you implement this from scratch or did you translate an existing implementation?

Thanks for pointing out things to consider. No, it is not written not from scratch. The code basically follows the thought process of the reference code.

bwesterb · 2024-03-11T07:41:36Z

Thanks for pointing out things to consider. No, it is not written not from scratch. The code basically follows the thought process of the reference code.

You should add a comment acknowledging on which code you've based yours, and mention its license (and make sure it's compatible with Circl's license.)

armfazh · 2024-03-15T02:12:35Z

Thanks for pointing out things to consider. No, it is not written not from scratch. The code basically follows the thought process of the reference code.

You should add a comment acknowledging on which code you've based yours, and mention its license (and make sure it's compatible with Circl's license.)

about the licensing, I think it suffices to add a line in the gen.go file something such as:

This implementation is a port from the C implementation of MAYO [link} distributed under <LICENSE>.

and there is no need to include the NOTICE and LICENSE files.

armfazh

These are a few comments, not a full review. In the meantime @bwesterb will help you polishing the code, and I will review again after that.

armfazh · 2024-03-14T00:42:00Z

internal/nist/drbg.go

@@ -11,6 +12,8 @@ type DRBG struct {
 	v   [16]byte
 }

+var _ io.Reader = (*DRBG)(nil)


this is not really needed, as it only required to implement one method.

Yes it can be remove, but this is intended as a static check (self-contained safe guard) that DRBG does implement io.Reader.

this check can be done in a test file.

I agree with Armando.

armfazh · 2024-03-14T02:28:52Z

sign/mayo/example_test.go

+	//  mode := MAYO.Mode3
+
+	// Generates a keypair.
+	pk, sk, err := mode.GenerateKey(nil)


Suggested change

pk, sk, err := mode.GenerateKey(nil)

pk, sk, err := mode.GenerateKey(rand.Reader)

I disagree. nil is fine. (This is meant as a code example, and using rand.Reader is a really bad example.)

armfazh · 2024-03-14T02:31:14Z

sign/mayo/example_test.go

+
+	// Creates a signature on our message with the generated private key.
+	msg := []byte("Some message")
+	signature, _ := mode.Sign(sk2, msg, nil)


check error

armfazh · 2024-03-14T02:34:43Z

sign/mayo/example_test.go

+	if !mode.Verify(pk2, msg, signature) {
+		panic("incorrect signature")
+	}
+
+	fmt.Printf("O.K.")
+
+	// Output:
+	// Supported modes: [MAYO_1 MAYO_2 MAYO_3 MAYO_5]
+	// O.K.


Suggested change

if !mode.Verify(pk2, msg, signature) {

panic("incorrect signature")

}

fmt.Printf("O.K.")

// Output:

// Supported modes: [MAYO_1 MAYO_2 MAYO_3 MAYO_5]

// O.K.

ok := mode.Verify(pk2, msg, signature)

fmt.Printf("Is valid: %v", ok)

// Output:

// Supported modes: [MAYO_1 MAYO_2 MAYO_3 MAYO_5]

// IsValid: true

This files is copied from Dilithium. Could you explain what is the intention behind the modification here?

it's a suggestion to improve the code.

I think the original is fine.

bwesterb · 2024-03-24T18:28:37Z

@ilway25 Thank you so much for the quick changes on our preliminary review. I'll be travelling for another week. After that I'll sit down and continue the review. Thanks again.

bwesterb · 2024-04-15T12:45:46Z

internal/nist/drbg.go

@@ -11,6 +12,8 @@ type DRBG struct {
 	v   [16]byte
 }

+var _ io.Reader = (*DRBG)(nil)


I agree with Armando.

bwesterb · 2024-04-15T12:50:13Z

sign/mayo/example_test.go

+	//  mode := MAYO.Mode3
+
+	// Generates a keypair.
+	pk, sk, err := mode.GenerateKey(nil)


I disagree. nil is fine. (This is meant as a code example, and using rand.Reader is a really bad example.)

bwesterb · 2024-04-15T12:51:41Z

sign/mayo/example_test.go

+	if !mode.Verify(pk2, msg, signature) {
+		panic("incorrect signature")
+	}
+
+	fmt.Printf("O.K.")
+
+	// Output:
+	// Supported modes: [MAYO_1 MAYO_2 MAYO_3 MAYO_5]
+	// O.K.


I think the original is fine.

bwesterb · 2024-04-15T12:57:54Z

sign/mayo/mayo.go

+}
+
+// Mode is a certain configuration of the MAYO signature scheme.
+type Mode interface {


Dilithium has a separate Mode interface, because the Dilithium implementation predates the generic circl/sign.Scheme API. If I'd write Dilithium today, I wouldn't have included the Mode interface. Similarly, if it's not too much trouble, it'd be better to just stick to the generic signature API and do away with the mode interface here. I can do that for you after it's merged if it's too much of a chore.

bwesterb · 2024-04-15T12:59:05Z

sign/mayo/mayo_test.go

+		name string
+		want string
+	}{
+		{"MAYO_1", "d0da809f52866507b6354588cd44b712bac138a8363fde768adb92285b6e9865"},


Please mention the repository and commit hash from which you generated these.

bwesterb · 2024-04-15T14:40:33Z

sign/mayo/mode1/internal/mayo.go

+	// Variable time approach with table access where index depends on input:
+	calculatePStVarTime(pst[:], pk.p1[:], pk.p2[:], pk.p3[:], s[:])
+
+	// compute S * PST


bwesterb · 2024-04-15T14:41:07Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func calculateSPstVarTime(sps []uint64, s []uint8, pst []uint64) {


Add comment describing the function.

bwesterb · 2024-04-15T14:46:28Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func accumulate(p int, bins [P * 16]uint64, out []uint64) {


Explain what the function does.

bwesterb · 2024-04-15T14:47:39Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func vecAddPacked(in []uint64, acc []uint64) {


Explain what the function does.

bwesterb · 2024-04-15T15:06:49Z

sign/mayo/mode1/internal/matrix.go

+	}
+}
+
+func vecAddPacked(in []uint64, acc []uint64) {


I'm not sure it makes sense that this is a separate function.

bwesterb · 2024-04-15T15:11:08Z

The code improved a lot. Still it needs better documentation on the actual arithmetic to make review easier.

ilway25 added 2 commits February 29, 2024 13:32

Add MAYO

cd9fedf

Add MAYO to schemes

cf3060b

armfazh requested review from armfazh and bwesterb February 29, 2024 07:32

ilway25 changed the title ~~Implement Mayo~~ Implement MAYO Feb 29, 2024

bwesterb requested changes Mar 4, 2024

View reviewed changes

bwesterb reviewed Mar 5, 2024

View reviewed changes

ilway25 added 4 commits March 8, 2024 13:16

Add test for resampling

9cfb7b1

Make NIST DBRG implement io.Reader

8f34bba

mayo: fix signapi.go generation

20ad6a5

mayo: refine function names and explain the techniques better

1a3ff0e

ilway25 added 3 commits March 11, 2024 16:42

Applying changes after Bas' review.

29d480e

mayo: make key expansion part of keygen

5e88046

mayo: add Mode interface

65e1819

ilway25 force-pushed the mayo branch from ec80a67 to 65e1819 Compare March 12, 2024 05:38

ilway25 requested a review from bwesterb March 14, 2024 05:57

armfazh reviewed Mar 15, 2024

View reviewed changes

bwesterb reviewed Apr 15, 2024

View reviewed changes

		@@ -0,0 +1,71 @@
		// Package nist implements helpers to generate NIST's Known Answer Tests (KATs).

	pk, sk, err := mode.GenerateKey(nil)
	pk, sk, err := mode.GenerateKey(rand.Reader)

Implement MAYO #483

Are you sure you want to change the base?

Implement MAYO #483

Conversation

ilway25 commented Feb 29, 2024

bwesterb commented Feb 29, 2024

bwesterb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilway25 Mar 5, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilway25 Mar 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilway25 commented Mar 5, 2024

bwesterb commented Mar 11, 2024

armfazh commented Mar 15, 2024

armfazh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilway25 Mar 15, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwesterb commented Mar 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bwesterb commented Apr 15, 2024

ilway25 Mar 5, 2024 •

edited

ilway25 Mar 11, 2024 •

edited

ilway25 Mar 15, 2024 •

edited