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 support of customizing RNG for crypto algorithms #65

Merged
merged 10 commits into from
Apr 17, 2024

Conversation

Taowyoo
Copy link
Collaborator

@Taowyoo Taowyoo commented Apr 11, 2024

TL;DR

This PR picks changes in #64 to master.

Background

Current two RNG we provided in this crate does not meet FIPS requirements.

So, this PR updates all crypto algorithms that need to use RNG to have a function pointer for creating RNG. In this way, user could customize the RNG they want to use for each algorithm.

Changes

  • Update code and add fn with_rng_provider to enable user to choose RNG for each crypto algorithm implementation.
  • Make some more types pub, so user could easily customize current crypto algorithm implementations.
  • Add unit tests.
  • Fix latest warnings from nightly rust:
    • Remove unnecessary namespace path used in types.
    • Remove unnecessary Deref trait guard.

Copy link

codecov bot commented Apr 11, 2024

Codecov Report

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

Project coverage is 95.45%. Comparing base (41c8d29) to head (f6a3f1d).

Files Patch % Lines
rustls-mbedcrypto-provider/src/sign.rs 93.18% 3 Missing ⚠️
rustls-mbedcrypto-provider/src/fips_utils/mod.rs 91.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master      #65      +/-   ##
==========================================
+ Coverage   95.32%   95.45%   +0.12%     
==========================================
  Files          19       20       +1     
  Lines        2674     2750      +76     
==========================================
+ Hits         2549     2625      +76     
  Misses        125      125              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Taowyoo Taowyoo force-pushed the yx/ffdhe-custom-rng-support_master branch from b1d5486 to bbb909d Compare April 11, 2024 22:56
@Taowyoo Taowyoo requested a review from s-arash April 11, 2024 22:57
@Taowyoo Taowyoo force-pushed the yx/ffdhe-custom-rng-support_master branch from 6999404 to 64cfa72 Compare April 12, 2024 02:39
@Taowyoo Taowyoo changed the title [master] Add support of specify RNG for crypto algorithms [master] Add support of custom RNG for crypto algorithms Apr 12, 2024
@Taowyoo Taowyoo changed the title [master] Add support of custom RNG for crypto algorithms Add support of customizing RNG for crypto algorithms Apr 12, 2024
@Taowyoo Taowyoo self-assigned this Apr 12, 2024
@Taowyoo Taowyoo requested a review from zugzwang April 13, 2024 04:22
@Taowyoo Taowyoo force-pushed the yx/ffdhe-custom-rng-support_master branch from 492d7c3 to 55592de Compare April 15, 2024 04:56
@Taowyoo Taowyoo enabled auto-merge April 15, 2024 04:56
@Taowyoo Taowyoo disabled auto-merge April 15, 2024 04:57
rustls-mbedcrypto-provider/src/kx.rs Outdated Show resolved Hide resolved
rustls-mbedcrypto-provider/src/lib.rs Outdated Show resolved Hide resolved
@Taowyoo Taowyoo requested a review from zugzwang April 15, 2024 15:36
@Taowyoo Taowyoo force-pushed the yx/ffdhe-custom-rng-support_master branch from bdb28aa to 25935a1 Compare April 15, 2024 22:45
@Taowyoo
Copy link
Collaborator Author

Taowyoo commented Apr 15, 2024

Hi folks, I just refactor and create the wrapper type in a clearer way:

  • Refactor and rename KxGroup to KxGroupWrapper.
  • Rename KeyExchange to KeyExchangeImpl.
  • Refactor and rename DheKxGroup to DheKxGroupWrapper.
  • Rename DheActiveKeyExchange to DheActiveKeyExchangeImpl.
  • Refactor and rename MbedTlsPkSigningKey to MbedTlsPkSigningKeyWrapper.
  • Update unit tests accordingly.

I did not change all types that contains rng_provider because the name of those unchanged type is fine to contain rng_provider in it like MbedTlsSigner.

CC: @zugzwang @xinyufort

Current two RNG we provided in this crate does not
meet FIPS requirements. So, this commit updates
all crypto algorithms that need to use RNG to have
a function pointer for creating RNG. In this way,
user could customize the RNG they want to use for
each algorithm.

Other Changes:

- Add unit tests.
- Fix clippy warnings.
- Remove derived Debug implementation on ` agreement::Algorithm`.
- Update unit test.
- Improve rustdoc for `MbedRng`.
fix rustdoc warning
- Refactor and rename `KxGroup` to `KxGroupWrapper`.
- Rename `KeyExchange` to `KeyExchangeImpl`.
- Refactor and rename `DheKxGroup` to `DheKxGroupWrapper`.
- Rename `DheActiveKeyExchange` to `DheActiveKeyExchangeImpl`.
- Refactor and rename `MbedTlsPkSigningKey` to `MbedTlsPkSigningKeyWrapper`.
- Update unit tests accordingly.
@Taowyoo Taowyoo force-pushed the yx/ffdhe-custom-rng-support_master branch from 25935a1 to 9c6b488 Compare April 15, 2024 22:48
@Taowyoo Taowyoo requested review from xinyufort and zugzwang and removed request for zugzwang April 15, 2024 22:48
Copy link

@zugzwang zugzwang left a comment

Choose a reason for hiding this comment

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

KxGroup should represent either an ECDH or FFDH key exchange group.

Currently, KxGroup represents ECDH and DheKxGroup represents FFDH.

@Taowyoo Taowyoo requested a review from zugzwang April 16, 2024 20:37
Copy link

@zugzwang zugzwang left a comment

Choose a reason for hiding this comment

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

Small nit; use either Ffdhe or FFDhe, not both.

Copy link

@xinyufort xinyufort left a comment

Choose a reason for hiding this comment

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

just one nitpick, otherwise LGTM

rustls-mbedcrypto-provider/src/kx.rs Outdated Show resolved Hide resolved
@Taowyoo Taowyoo added this pull request to the merge queue Apr 17, 2024
Merged via the queue into master with commit 3feb464 Apr 17, 2024
15 checks passed
@Taowyoo Taowyoo deleted the yx/ffdhe-custom-rng-support_master branch April 17, 2024 23: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.

None yet

5 participants