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

feat: switch from num-bigint-dig to crypto-bigint #394

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

feat: switch from num-bigint-dig to crypto-bigint #394

wants to merge 69 commits into from

Conversation

dignifiedquire
Copy link
Member

@dignifiedquire dignifiedquire commented Nov 29, 2023 •
edited
Loading

Very, very WIP

Not anymore, this is ready for review.
Replaces all usage of num-bigint-dig based BigInt usage with the new crypto-bigint crate, using BoxedUint

Current known issue is that we do have a performance regression, which will be able to get rid of over time:

# crypto-bigint

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   7,184,387.50 ns/iter (+/- 425,598.69)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  13,453,579.10 ns/iter (+/- 686,276.31)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   9,260,832.80 ns/iter (+/- 30,013.38)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  16,610,079.40 ns/iter (+/- 251,292.53)

# master

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,117,479.15 ns/iter (+/- 31,334.30)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,337,437.55 ns/iter (+/- 88,624.39)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,414,348.80 ns/iter (+/- 12,585.71)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,685,650.00 ns/iter (+/- 11,105.71)

TODOs

  • switch internal storage for RsaPrivateKey
  • switch internal storage for RsaPublicKey
  • switch all code to use the new decrypt implementation
  • update public traits using BigUint to return owned versions
  • fix blinding implementation
  • switch decryption algorithm with precompute to use crypto-bigint ops
  • go through other algorithms and update what can be done without having primality checks implemented
  • review & update code for constant time operation
  • review & update code for performance
  • benchmarks

@dignifiedquire dignifiedquire mentioned this pull request Nov 29, 2023
Open
tarcieri
tarcieri reviewed Nov 29, 2023
View reviewed changes
src/algorithms/rsa.rs Outdated Show resolved Hide resolved
@tarcieri tarcieri mentioned this pull request Nov 30, 2023
Closed
tarcieri
tarcieri reviewed Dec 1, 2023
View reviewed changes
src/algorithms/rsa.rs Outdated Show resolved Hide resolved
@sebadob sebadob mentioned this pull request Dec 1, 2023
Open
@dignifiedquire
Copy link
Member Author

@dignifiedquire Any update on this pull request?

no, haven't had time to fix the afformentioned issues yet

@mepi262
Copy link

mepi262 commented Nov 5, 2024

@dignifiedquire
Thank you for your response !
I hope your another project will be succeed and become be able to concentrate on this pull request.

@dignifiedquire
Copy link
Member Author

@tarcieri current benchmarks

# crypto-bigint

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   7,184,387.50 ns/iter (+/- 425,598.69)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  13,453,579.10 ns/iter (+/- 686,276.31)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   9,260,832.80 ns/iter (+/- 30,013.38)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:  16,610,079.40 ns/iter (+/- 251,292.53)

# master

# macbook m1
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,117,479.15 ns/iter (+/- 31,334.30)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,337,437.55 ns/iter (+/- 88,624.39)

# AMD
test bench_rsa_2048_pkcsv1_decrypt      ... bench:   1,414,348.80 ns/iter (+/- 12,585.71)
test bench_rsa_2048_pkcsv1_sign_blinded ... bench:   1,685,650.00 ns/iter (+/- 11,105.71)

@tarcieri tarcieri mentioned this pull request Nov 13, 2024
Closed
@Fethbita Fethbita mentioned this pull request Dec 1, 2024
Merged
@Fethbita
Copy link

Fethbita commented Dec 1, 2024 •
edited
Loading

@tarcieri most things are working now 🎉

I would appreciate some help with debugging the last failures, seems the proptests are discovering some roundtrip issues in the encoding/decoding and the last regular test that is a problem is dealign with a 2049bit key

@dignifiedquire created #462 for fixing these issues. Take a look. Feel free to change anything.

Fethbita and others added 6 commits December 1, 2024 15:49
@Fethbita
Fix clippy and fmt issues
ec599ee
@Fethbita
Fix the doctests in Oaep
e15b6a4 
The `n` in both cases is 257 bytes, with first element being 0
Re-encoded the number into 256 bytes and now the decoding works.

Note that ff you want to keep the previous Base64 `n`, then the
BoxedUint must take 2056 as the `bits_precision` parameter
@dignifiedquire
small cleanup and feature fix
dcc85c6
@dignifiedquire
Merge remote-tracking branch 'origin/master' into const-crypto-biguint
4b68379
@dignifiedquire
remove unused ci step
b8312e9
@dignifiedquire
fix: handle wasm compiliaton
ffdc5c2
@dignifiedquire
Copy link
Member Author

CI is finally green again, time for lots of review and cleanup

@dignifiedquire
Copy link
Member Author

@zeerooth nostd builds should be working again

@dignifiedquire dignifiedquire marked this pull request as ready for review December 2, 2024 09:16
@dignifiedquire dignifiedquire changed the title [WIP]: switch to crypto-bigint for decryption [WIP]: feat: switch from num-bigint-dig to crypto-bigint Dec 2, 2024
@dignifiedquire dignifiedquire changed the title [WIP]: feat: switch from num-bigint-dig to crypto-bigint feat: switch from num-bigint-dig to crypto-bigint Dec 2, 2024
dignifiedquire
dignifiedquire commented Dec 2, 2024
View reviewed changes
src/algorithms/generate.rs
let mut pi = prime_limit / (prime_limit.ln() - 1f64);
#[cfg(not(feature = "std"))]
let mut pi = prime_limit / (libm::logf(prime_limit as f32) as f64 - 1f64);
Copy link
Member Author

Choose a reason for hiding this comment

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

this is not entirely correct or optimal, but I am not sure what other way there is

Copy link
Member

Choose a reason for hiding this comment

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

@fjarri is there anything in crypto-primes for this?

Copy link

Choose a reason for hiding this comment

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

No, nothing like that currently

@tarcieri
Copy link
Member

@dignifiedquire seems like you can remove "Very, very WIP" now

@dignifiedquire
Copy link
Member Author

@dignifiedquire seems like you can remove "Very, very WIP" now

🤣 yeah, finally

@benjaminbruun benjaminbruun mentioned this pull request Dec 23, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pinkforest pinkforest pinkforest left review comments

@Fethbita Fethbita Fethbita left review comments

@baloo baloo baloo left review comments

@fjarri fjarri fjarri left review comments

@tarcieri tarcieri tarcieri left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.