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.

Sign up for GitHub

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

Jump to bottom

dsa: migrate to crypto-bigint #906

Open
wants to merge 20 commits into
base: master
Choose a base branch
from
Open

dsa: migrate to crypto-bigint #906

wants to merge 20 commits into from

Conversation

baloo
Copy link
Member

@baloo baloo commented Feb 16, 2025

This is a rebase of #784

There are two commits on top that I think should fix tests. There is a bit of cleanup to do still.

@baloo baloo marked this pull request as draft February 16, 2025 08:50
@baloo
Copy link
Member Author

baloo commented Feb 16, 2025

cc @aumetra

@aumetra
Copy link
Contributor

aumetra commented Feb 16, 2025

Apologies, I was a bit absent lately. I'll have a look when I get the time (which should be in like two hours at max)

@baloo
Copy link
Member Author

baloo commented Feb 16, 2025 •
edited
Loading

No need to apologize! Please take care.
That was only to keep you updated on the progress and let you chime in as needed.

If there are specific things you wanted to be taken care of, please let me know I'd be happy to.

@baloo
Copy link
Member Author

baloo commented Feb 17, 2025 •
edited
Loading

out of the last 3 tests that still fail:

  • sign_message: that one needs the vectors to be adjusted, we're switching the backend and by doing so the primes we get from the RNG are different because their generation is now different.
  • signer_verifier_signature: that one is a precision that needs to be adjusted.
  • verify_signature: I'm not quite sure yet.

Overall having to adjust precisions seems to be ... inefficent? error-prone? I guess there is a way better method for doing that. I need to have a look at RSA how they did it there.

@baloo baloo force-pushed the baloo/dsa/crypto-bigint branch 2 times, most recently from a5b6804 to fde178e Compare February 18, 2025 05:58
@aumetra
Copy link
Contributor

aumetra commented Feb 19, 2025

Overall having to adjust precisions seems to be ... inefficent? error-prone? I guess there is a way better method for doing that. I need to have a look at RSA how they did it there.

Seems like even rsa did the same thing. Just via the .widen method which is, I guess, a newer method that didn't exist when I started this PR a year ago

@aumetra
Copy link
Contributor

aumetra commented Feb 19, 2025

Scratch that, it existed, I just didn't see it at that time

@tarcieri
Copy link
Member

@baloo the precision aspect is definitely a bit tricky. num-bigint does it automatically, however this leads to variable-time operation depending on the value’s precision which is unsuitable for private key operations

@baloo
Copy link
Member Author

baloo commented Feb 19, 2025

Oh I understand why it's done! I'm just not sure I know what I'm doing [insert here the picture of a dog].

@baloo baloo mentioned this pull request Feb 19, 2025
Closed
@baloo baloo force-pushed the baloo/dsa/crypto-bigint branch 3 times, most recently from 26e40ac to 03f4d7a Compare March 2, 2025 07:04
@baloo
Copy link
Member Author

baloo commented Mar 2, 2025

okay, it's definitely on the ugly side, but tests do pass \o/

I think I got the hang of it, I think I can clean things up from there, but ... tomorrow.

@aumetra
Copy link
Contributor

aumetra commented Mar 2, 2025

Nice! I mean I have a few hours to kill today, too, so I can go ahead and try to clean up some things, too. I'll PR it into your fork when I get to it

@tarcieri tarcieri mentioned this pull request Mar 2, 2025
Open
@baloo baloo force-pushed the baloo/dsa/crypto-bigint branch 4 times, most recently from a481afb to 71639a3 Compare March 3, 2025 02:54
baloo
baloo commented Mar 3, 2025
View reviewed changes
Copy link
Member Author

@baloo baloo left a comment

Choose a reason for hiding this comment

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

Those two NonZero I'm not sure about.

dsa/src/signing_key.rs
Comment on lines +121 to +124
// TODO(baloo): is there any way R could be zero here? Could it be any reason for K to be
// one?
let r_short = NonZero::new(r_short).unwrap();
let r = NonZero::new(r).unwrap();
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure about this unwrap, isn't there a way for r = g.modpow(k, p) % q to be zero?

dsa/src/signing_key.rs
Comment on lines +135 to +136
// TODO(baloo): is there any way S could be zero here?
let s = NonZero::new(s).unwrap();
Copy link
Member Author

@baloo baloo Mar 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Similarly here, s could be zero?

@baloo baloo marked this pull request as ready for review March 3, 2025 03:53
@baloo baloo force-pushed the baloo/dsa/crypto-bigint branch 2 times, most recently from 6e1a74c to c0a9f41 Compare March 3, 2025 03:55
@baloo baloo force-pushed the baloo/dsa/crypto-bigint branch from c0a9f41 to 5fd1ebc Compare March 3, 2025 03:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@baloo @aumetra @tarcieri