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

CVPN-1554 Remove liboqs and use WolfSSL's implementations #192

Merged
merged 6 commits into from
Nov 18, 2024

Conversation

kp-thomas-yau
Copy link
Contributor

@kp-thomas-yau kp-thomas-yau commented Nov 7, 2024

Description

This PR removes liboqs and would instead use WolfSSL's implementation of Kyber. Since WolfSSL would officially release their Kyber/ML-KEM implementations a few months later, we would use the git patch to essentially patch their PRs on top of the 5.7.4 release.

The patch consists of the commits and code changes from the following PR from WolfSSL:

Configuration for enabling ML-KEM/Kyber:

  1. For only ML-KEM:
    ./configure --enable-kyber
    ./configure --enable-kyber=all,ml-kem
  2. For just Kyber:
    ./configure --enable-kyber=all,original
  3. For ML-KEM and Kyber
    ./configure --enable-kyber=all,original,ml-kem
    ./configure --enable-kyber=all,ml-kem,original

Testing

Tested with helium-cli, lightway clients (with and without liboqs)
See: https://polymoon.atlassian.net/browse/CVPN-1554?focusedCommentId=1351477

CI pipelines for kp_lightway, lightway passed as well

https://github.com/expressvpn/lightway/pull/122

@CLAassistant
Copy link

CLAassistant commented Nov 7, 2024

CLA assistant check
All committers have signed the CLA.

@kp-thomas-yau kp-thomas-yau changed the title Cvpn 1554 remove liboqs and use new wolfssl CVPN-1554 Remove liboqs and use WolfSSL's implementations Nov 7, 2024
Copy link

github-actions bot commented Nov 7, 2024

Code coverage summary for 134071a:

Filename                             Regions    Missed Regions     Cover   Functions  Missed Functions  Executed       Lines      Missed Lines     Cover    Branches   Missed Branches     Cover
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
wolfssl-sys/src/lib.rs                     5                 0   100.00%           2                 0   100.00%          24                 0   100.00%           0                 0         -
wolfssl/src/chacha20_poly1305.rs          14                 2    85.71%           5                 0   100.00%          95                 2    97.89%           0                 0         -
wolfssl/src/context.rs                   172                83    51.74%          45                14    68.89%         344               131    61.92%           0                 0         -
wolfssl/src/error.rs                      19                 7    63.16%           5                 1    80.00%          51                 9    82.35%           0                 0         -
wolfssl/src/lib.rs                        65                11    83.08%          14                 2    85.71%          81                12    85.19%           0                 0         -
wolfssl/src/rng.rs                        19                 5    73.68%           4                 0   100.00%          47                 3    93.62%           0                 0         -
wolfssl/src/ssl.rs                       471               202    57.11%          80                24    70.00%        1060               276    73.96%           0                 0         -
------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
TOTAL                                    765               310    59.48%         155                41    73.55%        1702               433    74.56%           0                 0         -

✅ Region coverage 59% passes
✅ Line coverage 74% passes

@kp-thomas-yau kp-thomas-yau force-pushed the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch from b5a6004 to 1a2ebf2 Compare November 7, 2024 04:49
@kp-thomas-yau kp-thomas-yau force-pushed the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch 3 times, most recently from b6c883c to f6f46c1 Compare November 15, 2024 05:05
@kp-thomas-yau kp-thomas-yau marked this pull request as ready for review November 15, 2024 08:23
@kp-thomas-yau kp-thomas-yau requested a review from a team as a code owner November 15, 2024 08:23
wolfssl-sys/build.rs Outdated Show resolved Hide resolved
wolfssl-sys/patches/use-wolfssl-mlkem-kyber.patch Outdated Show resolved Hide resolved
@kp-thomas-yau kp-thomas-yau force-pushed the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch from f6f46c1 to f651f99 Compare November 15, 2024 09:08
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add context around this renaming and removal of the part of the patch ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have replaced the patch with their PR fix: wolfSSL/wolfssl#8196

Since now we are removing liboqs, HAVE_LIBOQS would not be defined and the patch in settings.h would not be compiled anyway.

wolfssl-sys/build.rs Show resolved Hide resolved
wolfssl-sys/build.rs Show resolved Hide resolved
@kp-thomas-yau kp-thomas-yau force-pushed the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch from f651f99 to 9b0c980 Compare November 18, 2024 02:53
Copy link
Contributor

@kp-mariappan-ramasamy kp-mariappan-ramasamy left a comment

Choose a reason for hiding this comment

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

Changes LGTM

Cargo.lock Outdated Show resolved Hide resolved
wolfssl-sys/Cargo.toml Show resolved Hide resolved
Remove liboqs and enable WolfSSL's own Kyber implementation via the flags.
See: wolfSSL/wolfssl#8183
We would use a patch to use WolfSSL's implementation of both Kyber and ML-KEM so that we can remove liboqs while maintaining support for Kyber at the moment. This patch uses commits and code changes from the following PR in WolfSSL:
- wolfSSL/wolfssl#8143
- wolfSSL/wolfssl#8172
- wolfSSL/wolfssl#8183
- wolfSSL/wolfssl#8185
Disable dilithium explicitly so that we can reduce WolfSSL size as we are not using it at the moment.
Use the official fix from WolfSSL PR: wolfSSL/wolfssl#8196 instead of our own implementation to enable private key fields in key share entry when we are using post-quantum KEM.
@kp-thomas-yau kp-thomas-yau force-pushed the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch from 9b0c980 to ab01e98 Compare November 18, 2024 03:24
Copy link
Contributor

@xv-raihaan-m xv-raihaan-m left a comment

Choose a reason for hiding this comment

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

LGTM

@kp-thomas-yau kp-thomas-yau merged commit 070fd55 into main Nov 18, 2024
10 checks passed
@kp-thomas-yau kp-thomas-yau deleted the CVPN-1554-remove-liboqs-and-use-new-wolfssl branch November 18, 2024 05:10
kp-thomas-yau added a commit that referenced this pull request Nov 18, 2024
Add relevant tests and different ML-KEM groups to be used in the Lightway.

The config flags can be found here: #192
kp-thomas-yau added a commit that referenced this pull request Nov 18, 2024
Add relevant tests and different ML-KEM groups to be used in the Lightway.

The config flags can be found here: #192
kp-thomas-yau added a commit that referenced this pull request Nov 18, 2024
Add relevant tests and different ML-KEM groups to be used in the Lightway.

The config flags can be found here: #192
kp-thomas-yau added a commit that referenced this pull request Nov 18, 2024
Add relevant tests and different ML-KEM groups to be used in the Lightway.

The config flags can be found here: #192
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.

4 participants