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

Generate quantum resistant keypairs ahead of connecting #7432

Merged

Conversation

Serock3
Copy link
Contributor

@Serock3 Serock3 commented Jan 8, 2025


This change is Reviewable

@Serock3 Serock3 requested review from dlon and hulthe January 8, 2025 17:49
@Serock3 Serock3 self-assigned this Jan 8, 2025
Copy link

linear bot commented Jan 8, 2025

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 32 at r1 (raw file):

    // capacity of the channel by one
    let bufsize = bufsize.checked_sub(1).expect("bufsize must be at least 1");
    let (tx, rx) = mpsc::channel(bufsize);

mpsc::channel panics if the buffer size is 0, so spawn_keypair_worker(1) would also panic. Sadly, unlike std, tokio does not have rendezvous channels afaik :(

Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 21 at r1 (raw file):

type KeyPair = (PublicKey<'static>, SecretKey<'static>);

pub static KEYPAIR_RX: OnceLock<Mutex<mpsc::Receiver<KeyPair>>> = OnceLock::new();

⛏️ Definitely bikeshedding here, but I'd personally keep KEYPAIR_RX private and expose a public funcion to initialize it.

@Serock3 Serock3 force-pushed the generate-quantum-resistant-keypairs-ahead-of-connection-des-1614 branch 2 times, most recently from 394d151 to 472dd92 Compare January 9, 2025 09:28
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 57 at r1 (raw file):

        .recv()
        .await
        .expect("Failed to receive key pair, generating working expectedly closed.")

MORE BIKESHEDDING 🚲 🏚️

According to the Rust docs, the message in expect should tell us why we expect it not to panic; the assumptions you make. E.g. "keypair_worker should never exit while receiver exists" or something.

That said, these are more like guidelines than actual rules, yarr. 🏴‍☠️

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 3 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hulthe and @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 21 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

⛏️ Definitely bikeshedding here, but I'd personally keep KEYPAIR_RX private and expose a public funcion to initialize it.

+1

Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @dlon and @hulthe)


talpid-tunnel-config-client/src/classic_mceliece.rs line 21 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

+1

Done


talpid-tunnel-config-client/src/classic_mceliece.rs line 32 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

mpsc::channel panics if the buffer size is 0, so spawn_keypair_worker(1) would also panic. Sadly, unlike std, tokio does not have rendezvous channels afaik :(

Damn it. I think I've figured out a fix, but it became more verbose than I would have wanted. I added a fairly large note about this to motivate the choice, but maybe that isn't necessary?


talpid-tunnel-config-client/src/classic_mceliece.rs line 57 at r1 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

MORE BIKESHEDDING 🚲 🏚️

According to the Rust docs, the message in expect should tell us why we expect it not to panic; the assumptions you make. E.g. "keypair_worker should never exit while receiver exists" or something.

That said, these are more like guidelines than actual rules, yarr. 🏴‍☠️

Sure, I'll reword it. My impression was that we didn't bother following that convention, but maybe it doesn't hurt.

@Serock3 Serock3 force-pushed the generate-quantum-resistant-keypairs-ahead-of-connection-des-1614 branch from 610915a to 439a31f Compare January 9, 2025 13:18
@Serock3 Serock3 marked this pull request as ready for review January 9, 2025 13:18
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @dlon and @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 32 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Damn it. I think I've figured out a fix, but it became more verbose than I would have wanted. I added a fairly large note about this to motivate the choice, but maybe that isn't necessary?

tx.reserve() 💯


talpid-tunnel-config-client/src/classic_mceliece.rs line 57 at r1 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Sure, I'll reword it. My impression was that we didn't bother following that convention, but maybe it doesn't hurt.

It's up to you. Just though I'd mention the convention in case you wasn't aware of it :)


talpid-tunnel-config-client/src/classic_mceliece.rs line 29 at r4 (raw file):

/// It can take upwards of 200 ms to generate McEliece key pairs so it needs to be done before we
/// start connecting to the tunnel.
pub fn spawn_keypair_worker(bufsize: usize) -> mpsc::Receiver<KeyPair> {

Could you note the possible panic when bufsize=0 here in the docstring?

hulthe
hulthe previously approved these changes Jan 9, 2025
Copy link
Contributor

@hulthe hulthe left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


talpid-tunnel-config-client/src/classic_mceliece.rs line 29 at r4 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

Could you note the possible panic when bufsize=0 here in the docstring?

Nice, thanks :D

As `tokio::sync::mpsc` doesn't allow capacity to be zero,
we cannot support buffering only one key pair if we generate
it before sending. To get around this we use `reserve` to wait
for capacity before generating the key.
@Serock3 Serock3 force-pushed the generate-quantum-resistant-keypairs-ahead-of-connection-des-1614 branch from 3e065b8 to 7225364 Compare January 9, 2025 14:13
dlon
dlon previously approved these changes Jan 9, 2025
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Serock3)


talpid-tunnel-config-client/src/classic_mceliece.rs line 74 at r4 (raw file):

/// The first call will spawn the worker which immedietly starts to compute and buffer [`BUFSIZE`]
/// of key pairs.
pub fn get_or_init_keypair_receiver<'a>() -> &'a Mutex<mpsc::Receiver<KeyPair>> {

More bikeshedding: The receiver/return value does not have to be public

@Serock3 Serock3 dismissed stale reviews from dlon and hulthe via 38046e0 January 9, 2025 14:26
Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @hulthe)


talpid-tunnel-config-client/src/classic_mceliece.rs line 74 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

More bikeshedding: The receiver/return value does not have to be public

Done

dlon
dlon previously approved these changes Jan 9, 2025
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


talpid-tunnel-config-client/src/classic_mceliece.rs line 35 at r7 (raw file):

///
/// Panics if the buffer capacity is 0.
pub fn spawn_keypair_worker(bufsize: usize) -> mpsc::Receiver<KeyPair> {

Does this need to be pub?

Copy link
Contributor Author

@Serock3 Serock3 left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @dlon)


talpid-tunnel-config-client/src/classic_mceliece.rs line 35 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Does this need to be pub?

Suppose not

Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Serock3 Serock3 merged commit b3add67 into main Jan 9, 2025
50 checks passed
@Serock3 Serock3 deleted the generate-quantum-resistant-keypairs-ahead-of-connection-des-1614 branch January 9, 2025 14:39
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.

3 participants