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 hfs kyber nist round three #106

Closed

Conversation

david415
Copy link
Contributor

@david415 david415 commented Feb 7, 2021

Okay. This is ready for review. I got rid of all the .unwrap() calls except for the two located in the "generate" method:

    /// Generate a new private key.
    fn generate(&mut self, _rng: &mut dyn Random) {
        // Oqs uses their own random generator
        let kemalg = kem::Kem::new(kem::Algorithm::Kyber1024).unwrap();
        let (pk, sk) = kemalg.keypair().unwrap();
        self.alg = kemalg;
        self.pubkey = pk;
        self.privkey = sk;
    }

Should I change the function signature or keep it as is?

@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch 4 times, most recently from 5589f00 to 681d473 Compare February 7, 2021 15:01
Cargo.toml Outdated Show resolved Hide resolved
@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch from 681d473 to e20b9de Compare February 10, 2021 22:50
@david415
Copy link
Contributor Author

Note that until open-quantum-safe/liboqs#909 and cloudflare/circl#213 are resolved we should NOT merge this pull-request; The issue is that the liboqs Kyber is not compatible with Cloudflare's Golang Circl library's Kyber. I want them to be compatible because I'm using them both in my software. Obviously they should be compatible, but alas they are not.

That having been said, please do review this PR! I will make requested corrections and maintain this branch until the above compatibility issues are resolved. Thanks and cheers!

@bwesterb
Copy link

bwesterb commented Feb 12, 2021

As Thom pointed out, the current Rust crate implements Round 2 Kyber, whereas Circl has been updated to Round 3 Kyber. You'll have to wait for a release of libels or use an older version of Circl. katzenpost/noise#9 (comment)

Note that it is quite possible there will be more breaking changes to Kyber in the future.

@david415
Copy link
Contributor Author

david415 commented Feb 12, 2021

As Thom pointed out, the current Rust crate implements Round 2 Kyber, whereas Circl has been updated to Round 3 Kyber. You'll have to wait for a release of libels or use an older version of Circl. katzenpost/noise#9 (comment)

Note that it is quite possible there will be more breaking changes to Kyber in the future.

@bwesterb I'm hoping that the maintainer of snow @mcginty will allow us to just use the main branch of the oqs rust crate for the time being until a new version of the oqs crate is released. Or better yet, a specific commit id from the main branch that way it won't just break unexpectedly one day.

@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch 2 times, most recently from 8850ac9 to 515e75e Compare February 13, 2021 02:56
@david415
Copy link
Contributor Author

I've confirmed that this branch can interoperate with the Katzenpost fork of Go noise:
https://github.com/katzenpost/noise

Cargo.toml Outdated Show resolved Hide resolved
src/resolvers/default.rs Outdated Show resolved Hide resolved
src/resolvers/default.rs Outdated Show resolved Hide resolved
src/resolvers/default.rs Outdated Show resolved Hide resolved
@mcginty
Copy link
Owner

mcginty commented Feb 14, 2021

As Thom pointed out, the current Rust crate implements Round 2 Kyber, whereas Circl has been updated to Round 3 Kyber. You'll have to wait for a release of libels or use an older version of Circl. katzenpost/noise#9 (comment)
Note that it is quite possible there will be more breaking changes to Kyber in the future.

@bwesterb I'm hoping that the maintainer of snow @mcginty will allow us to just use the main branch of the oqs rust crate for the time being until a new version of the oqs crate is released. Or better yet, a specific commit id from the main branch that way it won't just break unexpectedly one day.

Given that Kyber support is still an experiment hidden behind a non-default feature flag, this seems fine.

@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch from 515e75e to 407222e Compare February 14, 2021 14:35
@david415
Copy link
Contributor Author

@mcginty Alright I've made the corrections you specified but now I'm wondering if you can help me figure out why the build fails:

user@computer:~/code/snow$ cargo build --features nist3_kyber1024
warning: panic message contains braces
   --> src/params/patterns.rs:580:48
    |
580 |         "handshake messages contain one of the {{'e1', 'ekem1'}} tokens, but not the other",
    |                                                ^^             ^^
    |
    = note: `#[warn(non_fmt_panic)]` on by default
    = note: this message is not used as a format string, but will be in a future Rust edition
help: add a "{}" format string to use the message literally
    |
580 |         "{}", "handshake messages contain one of the {{'e1', 'ekem1'}} tokens, but not the other",
    |         ^^^^^

warning: 1 warning emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.16s

@mcginty
Copy link
Owner

mcginty commented Feb 14, 2021

@mcginty Alright I've made the corrections you specified but now I'm wondering if you can help me figure out why the build fails:

user@computer:~/code/snow$ cargo build --features nist3_kyber1024
warning: panic message contains braces
   --> src/params/patterns.rs:580:48
    |
580 |         "handshake messages contain one of the {{'e1', 'ekem1'}} tokens, but not the other",
    |                                                ^^             ^^
    |
    = note: `#[warn(non_fmt_panic)]` on by default
    = note: this message is not used as a format string, but will be in a future Rust edition
help: add a "{}" format string to use the message literally
    |
580 |         "{}", "handshake messages contain one of the {{'e1', 'ekem1'}} tokens, but not the other",
    |         ^^^^^

warning: 1 warning emitted

    Finished dev [unoptimized + debuginfo] target(s) in 0.16s

That's a benign warning.

Your build is failing on windows because of liboqs (I imagine it was failing before your change as well - I just enabled Windows CI a few hours ago):

https://github.com/mcginty/snow/pull/106/checks?check_run_id=1897933780#step:4:661

@BlackHoleFox
Copy link
Contributor

The current issue the CI failures are seeing is due to bindgen. There's an issue similar to it here. Maybe we could try pointing it at the runner's clang installation in the environment variable?

@david415
Copy link
Contributor Author

The current issue the CI failures are seeing is due to bindgen. There's an issue similar to it here. Maybe we could try pointing it at the runner's clang installation in the environment variable?

I have no idea how to do that.

@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch from 407222e to 95ccb41 Compare February 14, 2021 21:38
tests/general.rs Show resolved Hide resolved
@@ -47,8 +47,7 @@ blake2 = { version = "0.9", optional = true }
rand = { version = "0.8", optional = true }
sha2 = { version = "0.9", optional = true }
x25519-dalek = { version = "1.1", optional = true }
pqcrypto-kyber = { version = "0.6", optional = true }
pqcrypto-traits = { version = "0.3", optional = true }
oqs = { git = "https://github.com/open-quantum-safe/liboqs-rust.git", rev = "6653b262d8548a8c93000a5b847b65c9b51e6e78", optional = true, default-features = false }

Choose a reason for hiding this comment

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

This is dangerous, in the context of publishing on https://crates.io. crates.io does not allow dependencies that are pulled from a Git repo. (See also RFC 2141.) There has to be a crate here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right. Can we convince the maintainers of the oqs crate to adopt a reasonable versioning scheme which progresses faster than every 6 months?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd really rather not have to fork oqs and fork snow. But the way things are now it's looking like I'm going to have to in order to actually do a "cargo publish". So sad.

@david415 david415 force-pushed the add_hfs_kyber_nist_round_three branch from 95ccb41 to d32d6bf Compare February 15, 2021 16:32
@mcginty mcginty closed this Nov 4, 2022
@mcginty
Copy link
Owner

mcginty commented Nov 4, 2022

Closing, since it's aged long enough.

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

Successfully merging this pull request may close these issues.

None yet

5 participants