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

Implement Classic McEliece #378

Open
wants to merge 14 commits into
base: main
Choose a base branch
from
Open

Conversation

pufferffish
Copy link

@pufferffish pufferffish commented Oct 4, 2022

This is a draft of a Classic McEliece that implements all the parameters specified in the round 3 submission. It passes the KAT tests provided in the official website.

The current major problem right now is that public key generation takes so much time it causes tests on CI to timeout and fail. This is because the Gaussian Elimination loop inside pkGen in pk_gen.go can take millions of iterations for larger parameters. Vectorization should be able to make it faster by like 30x but I haven't implemented it yet. Currently there are only generic Go implementations.

Some test data outside of the KAT test are lifted from a Rust implementation. These test data are to ensure that some specific functions work correctly.

Closes #360

@armfazh armfazh added the new feature New functionality or module label Oct 10, 2022
@armfazh armfazh requested a review from cjpatton October 10, 2022 20:00
math/gf4096/gf4096.go Outdated Show resolved Hide resolved
math/gf8192/gf8192.go Outdated Show resolved Hide resolved
@armfazh
Copy link
Contributor

armfazh commented Oct 10, 2022

@octeep thanks for pushing this. Also, could you please squash your commits.

We will review this code closely.


Definitely key generation takes lots of time (ARM64 tests timed out, while x64's not).

What about adding a build constraint to compile this code only on x64?

What do you think @bwesterb ?

@bwesterb
Copy link
Member

What do you think @bwesterb ?

I'll have a look tomorrow.

@pufferffish
Copy link
Author

@octeep thanks for pushing this. Also, could you please squash your commits.

We will review this code closely.

Definitely key generation takes lots of time (ARM64 tests timed out, while x64's not).

What about adding a build constraint to compile this code only on x64?

What do you think @bwesterb ?

I've squashed my commit. This probably shouldn't be merged yet since the Classic McEliece team might soon release their round 4 specs and software soon (See this). It might be better to wait for them to publish the round 4 so this PR can be updated accordingly.

@pufferffish pufferffish changed the title Implement Classical McEliece Implement Classic McEliece Oct 10, 2022
@bwesterb
Copy link
Member

Thanks for this @octeep

This probably shouldn't be merged yet since the Classic McEliece team might soon release their round 4 specs and software soon

Ok.

Can I ask you how did this implementation came to be: is it a fresh implementation from the spec or did you translate an existing implementation (ref or rust)?

I had an initial look today. To set expectations: it will of course take quite some time to carefully review this.

Some initial comments:

  1. Key generation is very slow and I confirmed it's mostly in the Gaussian elimination. It's much slower than the C reference implementation. How much experience do you have with optimising for Go? If you're new to it: the Go compiler is very dumb. They favour compilation speed against optimisation potential. Dumb things like unrolling loops can make a big difference. I think an optimised Gaussian elimination might be worthwhile.
  2. Have you given thought to organising the types/code in such a way that it's easy to add in SIMD optimisations?
  3. Having megabytes of test data is unworkable. Please reduce that to a few or even a single hash.
  4. The comments on the code aren't terrible, definitely compared to most crypto code, but we'd like to have a high standard.

@pufferffish
Copy link
Author

pufferffish commented Oct 11, 2022

Thanks for this @octeep

This probably shouldn't be merged yet since the Classic McEliece team might soon release their round 4 specs and software soon

Ok.

Can I ask you how did this implementation came to be: is it a fresh implementation from the spec or did you translate an existing implementation (ref or rust)?

Most of the code is translated from the 'Optimized Implementation' in the round 3 NIST submission package by the Classic McEliece team. However, when some methods cannot directly be ported from C (for example due to pointer arithmetic), the code is lifted from the Rust implementation instead. The Rust implementation is also based on the Optimized Implementation as far as I can tell.

I had an initial look today. To set expectations: it will of course take quite some time to carefully review this.

Some initial comments:

  1. Key generation is very slow and I confirmed it's mostly in the Gaussian elimination. It's much slower than the C reference implementation. How much experience do you have with optimising for Go? If you're new to it: the Go compiler is very dumb. They favour compilation speed against optimisation potential. Dumb things like unrolling loops can make a big difference. I think an optimised Gaussian elimination might be worthwhile.

I unfortunately do not have much experience with Go optimization. I also want to note that when I compile the C optimized implementation with vectorization explicitly disabled, the performance is only slightly faster than Go. When I compile the C optimized implementation and tell gcc to show the optimization tree, I see that gcc has performed vectorization automatically, which is why it is much faster than Go.

The vec implementations in Addition Implementations from the NIST submission package seems to perform much faster even when I compiled it with vectorization disabled. Maybe this can be ported to generic Go for an optimized Gaussian Elimination?

  1. Have you given thought to organising the types/code in such a way that it's easy to add in SIMD optimisations?

If we are talking about optimizing pk_gen specifically then I don't think it would be too hard to add SIMD optimizations. Just add some build flags in the pk_gen template and some slight refactoring should be enough.

If we are talking about optimizing all the components with SIMD then no, I haven't given much thoughts into it.

  1. Having megabytes of test data is unworkable. Please reduce that to a few or even a single hash.

I will look into that. I don't think reducing it to a few hashes can work since the test cases contain sample inputs. Maybe we can compress it or represent these inputs in raw bytes rather than as hexadecimal ASCII.

  1. The comments on the code aren't terrible, definitely compared to most crypto code, but we'd like to have a high standard.

I will try to document the code to the best of my understanding.

@pufferffish
Copy link
Author

I have managed to trim down testdata.txt to 90kb. Would this be acceptable?

@bwesterb
Copy link
Member

bwesterb commented Oct 12, 2022

Thanks again for your efforts!

Most of the code is translated from the 'Optimized Implementation' in the round 3 NIST submission package by the Classic McEliece team. However, when some methods cannot directly be ported from C (for example due to pointer arithmetic), the code is lifted from the Rust implementation instead. The Rust implementation is also based on the Optimized Implementation as far as I can tell.

Ok! Please add appropriate attributions.

The vec implementations in Addition Implementations from the NIST submission package seems to perform much faster even when I compiled it with vectorization disabled. Maybe this can be ported to generic Go for an optimized Gaussian Elimination?

Yes, that's a great plan!

(For those reading along: vec packs a few field elements into uint64s.)

If we are talking about optimizing all the components with SIMD then no, I haven't given much thoughts into it.

I'll get back to this when I found time to properly study the code.

I will try to document the code to the best of my understanding.

Great! If you're unsure about certain parts, please leave a comment as such for us to fill in.

@bwesterb
Copy link
Member

I have managed to trim down testdata.txt to 90kb. Would this be acceptable?

@armfazh ?

@armfazh
Copy link
Contributor

armfazh commented Oct 12, 2022

I have managed to trim down testdata.txt to 90kb. Would this be acceptable?

After bzip2-ing that file, size gets down to 50 KB, which I think is acceptable.

@armfazh
Copy link
Contributor

armfazh commented Oct 12, 2022

As a general comment, please introduce // TODO comments in places where code optimization is needed, such as better number representation or SIMD.
They might be not resolved in this PR, but flagging them is good for further development.

@pufferffish
Copy link
Author

Thanks again for your efforts!

Most of the code is translated from the 'Optimized Implementation' in the round 3 NIST submission package by the Classic McEliece team. However, when some methods cannot directly be ported from C (for example due to pointer arithmetic), the code is lifted from the Rust implementation instead. The Rust implementation is also based on the Optimized Implementation as far as I can tell.

Ok! Please add appropriate attributions.

I'm not sure how I should add attributions here. I have added a notice on each mceliece.go saying that some code are translated from the Rust implementation, and included the author's name and github link. Do I need to do more than that? (e.g. include their MIT license)

The vec implementations in Addition Implementations from the NIST submission package seems to perform much faster even when I compiled it with vectorization disabled. Maybe this can be ported to generic Go for an optimized Gaussian Elimination?

Yes, that's a great plan!

I am trying to do that in the vec branch in my fork repo. I can't promise if I will ever finish that but you can track the progress there if interested.

(For those reading along: vec packs a few field elements into uint64s.)

If we are talking about optimizing all the components with SIMD then no, I haven't given much thoughts into it.

I'll get back to this when I found time to properly study the code.

I will try to document the code to the best of my understanding.

Great! If you're unsure about certain parts, please leave a comment as such for us to fill in.

@bwesterb
Copy link
Member

Ok! Please add appropriate attributions.

I have added a notice on each mceliece.go saying that some code are translated from the Rust implementation, and included the author's name and github link.

That should be enough for now.

I am trying to do that in the vec branch in my fork repo. I can't promise if I will ever finish that but you can track the progress there if interested.

Cool.

@pufferffish
Copy link
Author

Ok good news: I managed to port the vec implementation of pk_gen to go and tested it on mceliece6960119. The KAT test now went from 120s to 17s. Still not as fast as the C implementation but it is much better than the current Go one.

@bwesterb
Copy link
Member

Ok good news: I managed to port the vec implementation of pk_gen to go and tested it on mceliece6960119. The KAT test now went from 120s to 17s. Still not as fast as the C implementation but it is much better than the current Go one.

Good job!

@pufferffish
Copy link
Author

The round 4 version has been updated, see https://classic.mceliece.org/nist.html

I've updated the implementation to the round 4 specification. It passes KAT tests and finishes in 10 minutes on CI.

Copy link
Contributor

@armfazh armfazh left a comment

Choose a reason for hiding this comment

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

Hi @octeep, so far I have checked the field arithmetic, it looks ok and there are some minor changes requested.
I will continue the review of other parts of the code.

math/gf4096/gf_test.go Outdated Show resolved Hide resolved
math/gf4096/gf_test.go Outdated Show resolved Hide resolved
math/gf4096/gf_test.go Outdated Show resolved Hide resolved
math/gf4096/gf4096.go Outdated Show resolved Hide resolved
math/gf4096/gf4096.go Outdated Show resolved Hide resolved
math/gf4096/gf4096.go Outdated Show resolved Hide resolved
math/gf4096/gf4096.go Outdated Show resolved Hide resolved
Co-authored-by: Armando Faz <[email protected]>
@d-z-m
Copy link

d-z-m commented Jan 28, 2023

Hello 👋 , just wanted to say this change is desired, and that I hope code review proceeds in the near future! 🚀

@d-z-m
Copy link

d-z-m commented Feb 22, 2023

Hello, just wanted to inquire if there's been any movement on this! Would love to see this get reviewed soon.

@armfazh
Copy link
Contributor

armfazh commented Feb 22, 2023

Hi, I will continue with the review of this PR in the next couple days. Thanks for your patience.

@mariobm
Copy link

mariobm commented Mar 29, 2023

Any updates on this one?

@stv0g
Copy link

stv0g commented Jun 8, 2023

I am also really interested in this PR as I am planning to use this for the Go implementation of the WireGuard Rosenpass Key Exchange.

@stv0g
Copy link

stv0g commented Jun 8, 2023

I noticed that the current implementation differs from liboqs with respect to the ciphertext length.
See the following note:

https://classic.mceliece.org/mceliece-pc-20221023.pdf

@stv0g
Copy link

stv0g commented Jun 8, 2023

I've updated the implementation to the round 4 specification. It passes KAT tests and finishes in 10 minutes on CI.

The file headers are still mentioning round 3:

// Package mceliece348864 implements the IND-CCA2 secure key encapsulation mechanism
// mceliece348864 as submitted to round 3 of the NIST PQC competition and
// described in

@stv0g
Copy link

stv0g commented Jun 8, 2023

@armfazh Can we remove the on-hold label from the issue? Round 4 specs have been released. There is nothing blocking this from getting finished :)

https://classic.mceliece.org/nist.html

@pufferffish
Copy link
Author

I've updated the implementation to the round 4 specification. It passes KAT tests and finishes in 10 minutes on CI.

The file headers are still mentioning round 3:

// Package mceliece348864 implements the IND-CCA2 secure key encapsulation mechanism
// mceliece348864 as submitted to round 3 of the NIST PQC competition and
// described in

Thanks for pointing that out, I've fixed that in a new commit.

I noticed that the current implementation differs from liboqs with respect to the ciphertext length. See the following note:

https://classic.mceliece.org/mceliece-pc-20221023.pdf

It would appear that liboqs is still using the round 3 specification which explains the difference in ciphertext length. Mine follows the round 4 specification and the produced ciphertexts are checked against the KAT tests provided by the official Classic McEliece website.

@stv0g
Copy link

stv0g commented Aug 1, 2023

Hi @bwesterb, @pufferffish, @armfazh,

is there something I can do to support you in getting this implementation merged?
Like code-review, testing or something similar?

Thanks :)

@armfazh
Copy link
Contributor

armfazh commented Aug 2, 2023

@stv0g we appreciate a code review, our team will be providing feedback as we get more bandwidth.

Copy link

@stv0g stv0g left a comment

Choose a reason for hiding this comment

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

This is a preliminary review. I will need to go through the GF arithmetics and templates more thouroughly.

@@ -155,6 +155,16 @@ func Example_schemes() {
// Kyber512
// Kyber768
// Kyber1024
// mceliece348864
Copy link

Choose a reason for hiding this comment

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

Is there a reason you have named the schemes with all-lowercase letters while the other PQ KEM schemes use CamelCase?

Copy link

Choose a reason for hiding this comment

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

Copy link

Choose a reason for hiding this comment

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

Can you add Classic McEliece to the file-level comment?

Copy link

Choose a reason for hiding this comment

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

Can we generate this file using some meta-programming?

Copy link

Choose a reason for hiding this comment

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

Can we generate this file using some meta-programming?

}

var (
McElieceParam348864 = Param{
Copy link

Choose a reason for hiding this comment

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

Can we maybe consider also adding pc variants of Classic McEliece including a 32-byte plaintext confirmation.

This would be desirable as liboqs always adds/expects the plaintext confirmation which makes incompatible with this implementation.

See: https://classic.mceliece.org/mceliece-pc-20221023.pdf

SysT: 128,
}
Instances = []Instance{
{Name: "mceliece348864", Param: McElieceParam348864},
Copy link

Choose a reason for hiding this comment

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

I propose to use names in line with liboqs here:
https://github.com/open-quantum-safe/liboqs/blob/6e1f49aa48971b0fc8658e94ca41d1ad4fe480ee/src/kem/kem.h#L42

This also matches the capitalization of e.g. Kyber512

@pufferffish
Copy link
Author

@stv0g thanks for the review. It's been a while since I've worked on this and recently I've been busy with work so I can't promise I can get back to your reviews soon. If you'd like to I can give you write access to my fork repo so you can directly work on this pull request.

// expanding and updating the seed
err := shake256(r[:], seed[0:33])
if err != nil {
panic(err)
Copy link

Choose a reason for hiding this comment

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

Can we return the error in DeriveKeypair().

I would say panic() these code paths should be discouraged.


func (*scheme) DeriveKeyPair(seed []byte) (kem.PublicKey, kem.PrivateKey) {
if len(seed) != seedSize {
panic("seed must be of length EncapsulationSeedSize")
Copy link

Choose a reason for hiding this comment

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

Same as below. Can we return an error here?

@d-z-m
Copy link

d-z-m commented Oct 4, 2023

Hi there,

Interested in seeing this move forward, any updates?

@david415
Copy link

david415 commented Oct 23, 2023

Hi. I get that everyone is busy doing other things and I also understand that I'm essentially the most impatient person in the world which is why I've decided to simply lift all this code into a fork for use with katzenpost... with proper attribution of course.

However I would much rather this pull request get merged and the code maintained by you guys. I hate maintaining forks. But I hate waiting for things even more.

@david415
Copy link

It looks like code contributions from your code contributor has stalled. i suggested you fork his dev branch and open a new pull request... and fix all the code corrections you were telling them to fix. If you don't do this, then this PR will likely continue to fester here meanwhile Rome is burning... society cracking at the foundations, abortion clinics need PQ KEMs like this one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new feature New functionality or module on-hold
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement Classic McEliece
7 participants