Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds QUIC support to the wolfcrypt provider by implementing the necessary cryptographic operations and packet protection algorithms required by the QUIC protocol.
- Adds QUIC-specific header protection and packet encryption/decryption algorithms
- Implements support for AES-128/256-GCM and ChaCha20-Poly1305 ciphers for QUIC
- Configures cipher suites with QUIC key factories when the "quic" feature is enabled
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| wolfcrypt-rs/src/bindings.rs | Adds lint allowance for unnecessary transmutes |
| rustls-wolfcrypt-provider/src/types/mod.rs | Adds ChaCha cipher object type definition |
| rustls-wolfcrypt-provider/src/lib.rs | Reorganizes imports and adds conditional QUIC support to cipher suites |
| rustls-wolfcrypt-provider/src/hkdf.rs | Adds spacing for formatting |
| rustls-wolfcrypt-provider/src/error.rs | Adds spacing for formatting |
| rustls-wolfcrypt-provider/src/aead/quic.rs | Implements complete QUIC header protection and packet encryption |
| rustls-wolfcrypt-provider/Cargo.toml | Adds "quic" feature flag |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
julek-wolfssl
left a comment
There was a problem hiding this comment.
Good stuff. Does quic get tested in the current CI workflows?
gasbytes
left a comment
There was a problem hiding this comment.
Nice work.
There seems to be some formatting issues that the CI detected, please fix those.
Also address the github copilot reports (mainly some unwraps that need to be refactored to handle errors).
Run clippy locally before committing too, the CI should detect some lints with the newest version.
Thank you.
Thank you. maybe we need to add --features quic in order for quic code to be compiled.
Thank you. I have corrected the mentioned issues. Hopefully CI tests will runs without errors |
|
Hello @helkoulak, there are still some clippy reports in the CI/CD apparently, it would be great if you could try and fix them.
Yes, please feel free to update the current workflows to test that too. |
|
…the case of rejected early data
…Ed25519PrivateKeyDecode
…ders like aws and ring as the order affects the success of some rustls tests
…ey value are provided with DER PKCS8 formatted private key
…in Ed25519PrivateKey
…stls default provider to reduce the code changes by hand when testing
…AesGcmSetKey must be used and not wc_AesSetKey
julek-wolfssl
left a comment
There was a problem hiding this comment.
Great work. Still need to dive into src/aead.
.github/workflows/ubuntu-build.yml
Outdated
| - name: Run tests of rustls v0.23.35 | ||
| run: | | ||
| mkdir rustlsv0.23.35-test-workspace | ||
| cd rustlsv0.23.35-test-workspace | ||
| git clone https://github.com/rustls/rustls.git | ||
| cd rustls | ||
| git fetch --tags | ||
| selected_tag=$(git tag -l "v/0\.23\.35") | ||
| git checkout "$selected_tag" | ||
| cd .. | ||
| git clone https://github.com/helkoulak/rustls-wolfcrypt-provider.git | ||
| cd rustls-wolfcrypt-provider/ | ||
| git checkout quic-support | ||
| cd wolfcrypt-rs/ | ||
| make build | ||
| cd ../rustls-wolfcrypt-provider/ | ||
| cargo build --all-features --release | ||
| cd ../.. | ||
| git clone https://github.com/helkoulak/rustls_v0.23.35_test_files.git | ||
| cp -r ./rustls_v0.23.35_test_files/tests . | ||
| cp ./rustls_v0.23.35_test_files/Cargo.toml . | ||
| cp ./rustls_v0.23.35_test_files/provider_files/Cargo.toml ./rustls-wolfcrypt-provider/rustls-wolfcrypt-provider/ | ||
| rm -rf rustls_v0.23.35_test_files | ||
| cargo test -p tests --test all_suites --all-features | ||
| cd .. | ||
| rm -rf rustlsv0.23.35-test-workspace |
There was a problem hiding this comment.
Let's split this up into steps to make it easier to understand what failed for future devs.
.github/workflows/ubuntu-build.yml
Outdated
| mkdir rustlsv0.23.35-test-workspace | ||
| cd rustlsv0.23.35-test-workspace | ||
| git clone https://github.com/rustls/rustls.git | ||
| cd rustls | ||
| git fetch --tags | ||
| selected_tag=$(git tag -l "v/0\.23\.35") | ||
| git checkout "$selected_tag" | ||
| cd .. | ||
| git clone https://github.com/helkoulak/rustls-wolfcrypt-provider.git | ||
| cd rustls-wolfcrypt-provider/ | ||
| git checkout quic-support |
There was a problem hiding this comment.
Use https://github.com/marketplace/actions/checkout to fetch repos.
.github/workflows/ubuntu-build.yml
Outdated
| cd ../rustls-wolfcrypt-provider/ | ||
| cargo build --all-features --release | ||
| cd ../.. | ||
| git clone https://github.com/helkoulak/rustls_v0.23.35_test_files.git |
There was a problem hiding this comment.
Place these under .github/workflows/rustls or a similar path. I don't see a need to have this in a dedicated repo.
There was a problem hiding this comment.
If I understand correctly, you are using these files to make modifications to rustls/rustls? If yes, then I would prefer to use a patch file for this. The patch file should be applied with patch -p1 < <path/to/patch/file> in the rustls/rustls root dir.
There was a problem hiding this comment.
The idea here is to build a workspace that contains three packages, namely rustls, wolfcrypt-provider, and tests package. It is done this way to break the cyclic dependency between rustls and wolfcrypt-provider. The package tests is created from a modified copy of rustls/tests plus other files like build.rs and own Cargo.toml file.
Using a patch file will make in my opinion the script more difficult to understand. In addition to that, in each rustls version new tests are added and some are removed, so this makes the patch file valid for only a specific version of rustls.
There was a problem hiding this comment.
The same comments apply to macos-build.yml.
There was a problem hiding this comment.
I would consider moving the rustls tests into a separate workflow.
| pub fn provider() -> CryptoProvider { | ||
| pub fn default_provider() -> CryptoProvider { |
There was a problem hiding this comment.
Why is this necessary? Is this overloading something in rustls?
There was a problem hiding this comment.
Both providers, namely ring and aws-lc-rs, have it as default_provider(). So to make wolfcrypt more compatible with them and to do less manual work in the future testing rustls api, I have changed it this why.
| impl Ed25519PrivateKey { | ||
| /// Extract ED25519 private and if available public key values from a PKCS#8 DER formatted key | ||
| fn extract_key_pair(input_key: &[u8]) -> Result<([u8; 32], Option<[u8; 32]>), rustls::Error> { | ||
| let mut public_key_raw: [u8; 32] = [0; ED25519_PUB_KEY_SIZE as usize]; | ||
| let mut private_key_raw: [u8; 32] = [0; ED25519_KEY_SIZE as usize]; | ||
| let mut skip_bytes: usize; | ||
| let mut key_sub_slice = input_key; | ||
|
|
||
| const SHORT_FORM_LEN_MAX: u8 = 127; | ||
| const TAG_SEQUENCE: u8 = 0x30; | ||
| const TAG_OCTET_SEQUENCE: u8 = 0x04; | ||
| const TAG_OPTIONAL_SET_OF_ATTRIBUTES: u8 = 0x80; //Implicit, context-specific, and primitive underlying type (SET OF) | ||
| const TAG_OPTIONAL_PUBLIC_KEY_BIT_STRING: u8 = 0x81; //Implicit, context-specific, and primitive underlying type (BIT STRING) | ||
|
|
||
| // The input key is encoded in PKCS#8 DER format with a structure as in | ||
| // https://www.rfc-editor.org/rfc/rfc5958.html | ||
| // | ||
| // AsymmetricKeyPackage ::= SEQUENCE SIZE (1..MAX) OF OneAsymmetricKey | ||
|
|
||
| // OneAsymmetricKey ::= SEQUENCE { | ||
| // version Version, | ||
| // privateKeyAlgorithm PrivateKeyAlgorithmIdentifier, | ||
| // privateKey PrivateKey, | ||
| // attributes [0] Attributes OPTIONAL, | ||
| // ..., | ||
| // [[2: publicKey [1] PublicKey OPTIONAL ]], | ||
| // ... | ||
| // } | ||
|
|
||
| // The key structure must begin with a SEQUENCE tag with at least 2 bytes length if short | ||
| // length format is used | ||
| if key_sub_slice[0] != TAG_SEQUENCE || key_sub_slice.len() < 2 { | ||
| return Err(rustls::Error::General( | ||
| "Faulty PKCS#8 ED25519 private key structure".into(), | ||
| )); | ||
| } | ||
| // Check which length format and skip tag and length encoding bytes | ||
| if key_sub_slice[1] > SHORT_FORM_LEN_MAX { | ||
| skip_bytes = (2 + (key_sub_slice[1] & 0x7F)) as usize; | ||
| } else { | ||
| skip_bytes = 2; | ||
| } | ||
|
|
||
| // Skip version (3 bytes), algorithm ID sequence (0x30 + length encoding bytes + 5 ID bytes), | ||
| skip_bytes += 3 + 7; | ||
| key_sub_slice = input_key.get(skip_bytes..).unwrap(); | ||
|
|
||
| // Check if next bytes are 0x04, 0x22, 0x04, 0x20 | ||
| if !matches!( | ||
| key_sub_slice, | ||
| [TAG_OCTET_SEQUENCE, 0x22, TAG_OCTET_SEQUENCE, 0x20, ..] | ||
| ) { | ||
| return Err(rustls::Error::General( | ||
| "Faulty PKCS#8 ED25519 private key structure".into(), | ||
| )); | ||
| } | ||
|
|
||
| // Copy private key value | ||
| skip_bytes += 4; | ||
| key_sub_slice = input_key.get(skip_bytes..).unwrap(); | ||
| private_key_raw.copy_from_slice(&key_sub_slice[..ED25519_KEY_SIZE as usize]); | ||
| skip_bytes += ED25519_KEY_SIZE as usize; | ||
| key_sub_slice = input_key.get(skip_bytes..).unwrap(); | ||
|
|
||
| // Check if optional SET OF attributes exists and skip | ||
| if key_sub_slice.first() == Some(&TAG_OPTIONAL_SET_OF_ATTRIBUTES) { | ||
| skip_bytes += (2 + (key_sub_slice[1])) as usize; | ||
| key_sub_slice = input_key.get(skip_bytes..).unwrap(); | ||
| } | ||
|
|
||
| // Check if optional public key value exists. If exists, skip tag, length encoding byte, | ||
| // and bits-used byte | ||
| if key_sub_slice.first() == Some(&TAG_OPTIONAL_PUBLIC_KEY_BIT_STRING) { | ||
| public_key_raw.copy_from_slice(&key_sub_slice[3..(3 + ED25519_KEY_SIZE as usize)]); | ||
| Ok((private_key_raw, Some(public_key_raw))) | ||
| } else { | ||
| Ok((private_key_raw, None)) | ||
| } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I would prefer to use the ASN parsing in wolfcrypt. You should use wc_Ed25519PrivateKeyDecode which will pick up both priv and pub keys. Then wc_ed25519_export_key can be used to export the raw values. Only downside I see is that privKeySet and pubKeySet are not checked in wc_ed25519_export_key. Feel free to apply the following patch to wolfSSL. I'll submit a PR to wolfssl in a second.
diff --git a/wolfcrypt/src/ed25519.c b/wolfcrypt/src/ed25519.c
index a03efb5603..ff59e07310 100644
--- a/wolfcrypt/src/ed25519.c
+++ b/wolfcrypt/src/ed25519.c
@@ -1127,6 +1127,9 @@ int wc_ed25519_export_public(const ed25519_key* key, byte* out, word32* outLen)
return BUFFER_E;
}
+ if (!key->pubKeySet)
+ return PUBLIC_KEY_E;
+
*outLen = ED25519_PUB_KEY_SIZE;
XMEMCPY(out, key->p, ED25519_PUB_KEY_SIZE);
@@ -1368,7 +1371,7 @@ int wc_ed25519_export_private_only(const ed25519_key* key, byte* out, word32* ou
int wc_ed25519_export_private(const ed25519_key* key, byte* out, word32* outLen)
{
/* sanity checks on arguments */
- if (key == NULL || out == NULL || outLen == NULL)
+ if (key == NULL || !key->privKeySet || out == NULL || outLen == NULL)
return BAD_FUNC_ARG;
if (*outLen < ED25519_PRV_KEY_SIZE) {
@@ -1398,6 +1401,8 @@ int wc_ed25519_export_key(const ed25519_key* key,
/* export public part */
ret = wc_ed25519_export_public(key, pub, pubSz);
+ if (ret == PUBLIC_KEY_E)
+ ret = 0; /* ignore no public key */
return ret;
}
There was a problem hiding this comment.
I have applied the patch file provided but unfortunately with no luck. As I have explained earlier in our email conversations, the problem lies in the function wc_Ed25519PrivateKeyDecode. It is unable to parse/recognize the public key part found in a PKCS8 DER formatted key pair. Luckily I was able to debug and record where the error exactly happens. I will send it to you per Email.
There was a problem hiding this comment.
And the function extract_key_pair was the workaround
| pub static AES_128: HPAlgorithm = HPAlgorithm { | ||
| key_len: AES_128_KEY_LEN, | ||
| hp_mask: generate_mask_aes, | ||
| id: HPAlgorithmID::Aes128, | ||
| init: init_hp_aes_cipher, | ||
| }; | ||
|
|
||
| /// AES-256. | ||
| pub static AES_256: HPAlgorithm = HPAlgorithm { | ||
| key_len: AES_256_KEY_LEN, | ||
| hp_mask: generate_mask_aes, | ||
| id: HPAlgorithmID::Aes256, | ||
| init: init_hp_aes_cipher, | ||
| }; |
There was a problem hiding this comment.
I think its correct as it is. Because here we look for a program life time validity rather than fixed values
| /// AES-128 in GCM mode with 128-bit tags and 96 bit nonces. | ||
| pub static AES_128_GCM: AeadAlgorithm = AeadAlgorithm { | ||
| init: init_aes_gcm_cipher, | ||
| encrypt: encrypt_aes_gcm, | ||
| decrypt: decrypt_aes_gcm, | ||
| key_len: AES_128_KEY_LEN, | ||
| id: PacketKeyAlgorithmID::Aes128Gcm, | ||
| }; | ||
|
|
||
| /// AES-256 in GCM mode with 128-bit tags and 96 bit nonces. | ||
| pub static AES_256_GCM: AeadAlgorithm = AeadAlgorithm { | ||
| init: init_aes_gcm_cipher, | ||
| encrypt: encrypt_aes_gcm, | ||
| decrypt: decrypt_aes_gcm, | ||
| key_len: AES_256_KEY_LEN, | ||
| id: PacketKeyAlgorithmID::Aes256Gcm, | ||
| }; |
There was a problem hiding this comment.
I think it is correct this way
There was a problem hiding this comment.
Let's avoid unwrap. I see rust has some ways to use unwrap without panicking. libs shouldn't take down an app. None of the places where unwrap is used are places where failure is catastrophic. Instead an error should be returned and user should be allowed to handle it that way . Maybe unwrap_or_else is appropriate?
There was a problem hiding this comment.
I have replaced unwrap() completely with functions like ? and map_err to propagate errors instead of panicking.
gasbytes
left a comment
There was a problem hiding this comment.
Really nice work Hosam, I left a couple of comments.
There was a problem hiding this comment.
One thing that I noticed is that you compile and run the tests with --all-features. I understand that with wolfcrypt-provider feature only the wolfcrypt provider is going to be used against the rustls testsuite, which is great, but with --all-features awc-lc-rs and ring get compiled too even if they are not used.
Is it possible to remove them? Or are they hardcoded dependencies used in some way in the testsuite?
I think the command to run the testsuite with only the wolfcrypt provider (which is already compiled in) is:
cargo test -p tests --test all_suites --features wolfcrypt-provider,tls12 --no-default-features
or something similar.
There was a problem hiding this comment.
You are right. No need to use --all-features option. The tests run successfully using your suggested command with a few features added and it should be like this
cargo test -p tests --test all_suites --features wolfcrypt-provider,tls12,fips,zlib,prefer-post-quantum,logging --no-default-features
There was a problem hiding this comment.
Me and Juliusz were also thinking that it might be worth to add a feature to print the current provider being used via cargo, since you added the configuration option wolfcrypt-provider.
And add that step before running the testsuite, by grepping the output from stdout and confirming that we are running the full testsuite against the wolfcrypt-provider only.
That would be great.
There was a problem hiding this comment.
If I understand correctly, this is already done. The command that runs the tests targets only the runner file all_test_suites.rs. And in this runner file you have macros that are annotated with #[cfg(feature = "wolfcrypt-provider")] and print the sentence tests_with_wolfcrypt_. So as per my understanding, there is no way the tests will run against other providers than wolfcrypt-provider. Or did I miss something here?
|
|
||
| pub struct AesCipher { | ||
| aes_object: AesObject, | ||
| key: Vec<u8>, |
There was a problem hiding this comment.
This is a plain Vec, which means that it won't be zeroed when dropped.
Consider either using zeroize or implement a Drop call to automatically free resources after usage.
There was a problem hiding this comment.
I have implemented a drop call for both ciphers to zero the key vectors.
| } | ||
| } | ||
|
|
||
| fn set_key(&self, key: &[u8]) -> Result<(), Error> { |
There was a problem hiding this comment.
This function takes &self, and sets the key on the FFI object, but self.key remains None, contrary to the set_key function of AesCipher::set_key.
Can you please also store raw bytes key here too for consistency?
.github/workflows/macos-build.yml
Outdated
| cd rustls | ||
| git fetch --tags | ||
| selected_tag=$(git tag -l "v/0\.23\.35") | ||
| git checkout "$selected_tag" |
There was a problem hiding this comment.
Maybe instead of using git tag -l followed by checkout, it might be easier to do a checkout directly like this:
git checkout v/0.23.35
There was a problem hiding this comment.
No. Using the checkout action is the correct way.
| mod tests { | ||
| use hex_literal::hex; | ||
| use rustls::crypto::tls13::HkdfExpander; | ||
| use std::prelude::rust_2015::ToString; |
There was a problem hiding this comment.
Is this necessary? ToString is included in teh standard prelude:
https://doc.rust-lang.org/std/string/trait.ToString.html
There was a problem hiding this comment.
It is unnecessary actually, you are right. I have removed it.
rustls-wolfcrypt-provider/Cargo.toml
Outdated
| hex-literal = "0.4.1" | ||
|
|
||
|
|
||
|
|
| //! | ||
| //! See draft-ietf-quic-tls. | ||
|
|
||
| #![allow(clippy::type_complexity)] |
There was a problem hiding this comment.
Have you tried removing this to see if clippy comes back with any warnings/reports that might be worth fixing?
There was a problem hiding this comment.
By removing this lint, clippy will complain about a readability issue. It was fixed as seen in commit c0d2c73
…king and to propagate errors instead.
gasbytes
left a comment
There was a problem hiding this comment.
Great work Hosam, LGTM.
No description provided.