search

briansmith / ring

Safe, fast, small crypto using Rust
Other
3.69k stars 697 forks source link

Refactor fe_isnonzero to fe_isnonzero_vartime #1893

Open tisonkun opened 8 months ago

tisonkun commented 8 months ago

This refers to #1827.

An intuitive solution according to @briansmith's comment. But I know little about crypto so perhaps some invariants are broken.

codecov[bot] commented 8 months ago

Codecov Report

Attention: Patch coverage is 88.88889% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 96.30%. Comparing base (9cb93e0) to head (407599a). Report is 31 commits behind head on main.

Files Patch % Lines
crypto/curve25519/curve25519.c 88.88% 0 Missing and 1 partial :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #1893 +/- ## ======================================= Coverage 96.29% 96.30% ======================================= Files 135 135 Lines 20663 20667 +4 Branches 226 228 +2 ======================================= + Hits 19898 19903 +5 Misses 730 730 + Partials 35 34 -1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

tisonkun commented 8 months ago

Thanks for your review @briansmith!

Do you think it's better to exclude changes around src/constant_time.rs in this PR and move forward, or we can wait for https://github.com/briansmith/ring/pull/1899 merged?

briansmith commented 8 months ago

Yes, I think we should limit this to the curve25519.c change and mem.h deletion.

tisonkun commented 8 months ago

Updated. PTAL @briansmith.

tisonkun commented 7 months ago

@briansmith ping as a reminder :D

tisonkun commented 6 months ago

@briansmith ping as a reminder :D

tisonkun commented 5 months ago

@briansmith I suppose the codecov regression is false positive..

tisonkun commented 2 months ago

@briansmith is there other blocker to this PR?