quinn icon indicating copy to clipboard operation
quinn copied to clipboard

Update rustls to 0.22 and ring to 0.17

Open djc opened this issue 1 year ago • 9 comments

djc avatar Dec 04 '23 10:12 djc

I think you might also change the code in directory quinn/examples, like server.rs and client.rs.

rustls::Certificate has been replaced with CertificateDer from the new rustls-pki-types crate. Likewise, rustls::PrivateKey has been replaced with rustls_pki_types::PrivateKeyDer.

The code in examples does not compile.

getong avatar Dec 06 '23 22:12 getong

@getong there is a reason the PR is in draft state.

djc avatar Dec 07 '23 07:12 djc

In order to make ring support some architectures, I hope to upgrade ring to 0.17 as soon as possible

PikuZheng avatar Dec 24 '23 23:12 PikuZheng

In order to make ring support some architectures, I hope to upgrade ring to 0.17 as soon as possible

@PikuZheng I believe ring 0.17 is already available with Quinn. The existing Quinn 0.10 release uses Rustls 0.21, which has upgraded to ring 0.17 as of v0.21.8. You don't need to wait for Quinn to have Rustls 0.22 support for this.

cpu avatar Dec 27 '23 14:12 cpu

In order to make ring support some architectures, I hope to upgrade ring to 0.17 as soon as possible

@PikuZheng I believe ring 0.17 is already available with Quinn. The existing Quinn 0.10 release uses Rustls 0.21, which has upgraded to ring 0.17 as of v0.21.8. You don't need to wait for Quinn to have Rustls 0.22 support for this.

├── quinn v0.10.2
│   ├── bytes v1.5.0
│   ├── pin-project-lite v0.2.13
│   ├── quinn-proto v0.10.6
│   │   ├── bytes v1.5.0
│   │   ├── rand v0.8.5
│   │   │   ├── libc v0.2.151
│   │   │   ├── rand_chacha v0.3.1
│   │   │   │   ├── ppv-lite86 v0.2.17
│   │   │   │   └── rand_core v0.6.4
│   │   │   │       └── getrandom v0.2.11 (*)
│   │   │   └── rand_core v0.6.4 (*)
│   │   ├── ring v0.16.20
│   │   │   ├── libc v0.2.151
│   │   │   ├── once_cell v1.19.0
│   │   │   ├── spin v0.5.2
│   │   │   └── untrusted v0.7.1
│   │   ├── rustc-hash v1.1.0
│   │   ├── rustls v0.21.10 (*)
│   │   ├── slab v0.4.9 (*)
│   │   ├── thiserror v1.0.52 (*)
│   │   ├── tinyvec v1.6.0
│   │   │   └── tinyvec_macros v0.1.1
│   │   └── tracing v0.1.40 (*)

anything i'm missing?

PikuZheng avatar Dec 27 '23 15:12 PikuZheng

anything i'm missing?

No, this was my mistake. I forgot Quinn also has ring in its public API and didn't take the breaking change to update to 0.17 in the 0.10 release like Rustls did.

Sorry for the confusion. There is a change required in this repo to remove ring 0.16 from the requirements.

cpu avatar Dec 27 '23 15:12 cpu

We can now compile quinn-proto -- but there's still a bunch of test failures:

failures:
    tests::handshake_anti_deadlock_probe
    tests::large_initial
    tests::reject_missing_client_cert
    tests::server_can_send_3_inital_packets

If someone wants to lend a hand diagnosing/fixing these, that would be great. Interestingly, some of these seem to be triggering panics in the rustls deframer:

thread 'tests::server_can_send_3_inital_packets' panicked at /Users/djc/.cargo/git/checkouts/rustls-29bde2ca1adbde2d/aef485d/rustls/src/msgs/deframer.rs:490:8:
range end index 4468 out of range for slice of length 4096

djc avatar Jan 24 '24 21:01 djc

This patch to rustls fixes 3/4 failing tests:

diff --git a/rustls/src/msgs/deframer.rs b/rustls/src/msgs/deframer.rs
index 6da2d9d4..5d9d1fdd 100644
--- a/rustls/src/msgs/deframer.rs
+++ b/rustls/src/msgs/deframer.rs
@@ -253,7 +253,7 @@ impl MessageDeframer {
                 // We're joining a handshake message to the previous one here.
                 // Write it into the buffer and update the metadata.
 
-                DeframerBuffer::<QUIC>::copy(buffer, payload, meta.payload.end);
+                DeframerBuffer::<QUIC>::copy(buffer, payload, 0);
                 meta.message.end = end;
                 meta.payload.end += payload.len();

DeframerBuffer::<true>::copy automatically copies into the end of the payload, so passing meta.payload.end as the third argument double-counts the offset. This branch is probably only covered by tests which use multiple calls to read_hs to pass in a single handshake message.

That API seems a bit obscure. Maybe there's a clearer/better normalized way to express the abstraction? I don't fully understand why QUIC is a special case here to begin with, though.

Ralith avatar Jan 25 '24 06:01 Ralith

This fixes the remaining test:

diff --git a/quinn-proto/src/tests/mod.rs b/quinn-proto/src/tests/mod.rs
index f809ba1c..1b3b28de 100644
--- a/quinn-proto/src/tests/mod.rs
+++ b/quinn-proto/src/tests/mod.rs
@@ -379,9 +379,15 @@ fn reject_missing_client_cert() {
     let key = PrivatePkcs8KeyDer::from(CERTIFICATE.serialize_private_key_der());
     let cert = CertificateDer::from(util::CERTIFICATE.serialize_der().unwrap());
 
+    let mut store = RootCertStore::empty();
+    // `WebPkiClientVerifier` requires a nonempty store, so we stick our own certificate into it
+    // because it's convenient.
+    store
+        .add(CERTIFICATE.serialize_der().unwrap().into())
+        .unwrap();
     let config = rustls::ServerConfig::builder()
         .with_client_cert_verifier(
-            WebPkiClientVerifier::builder(Arc::new(RootCertStore::empty()))
+            WebPkiClientVerifier::builder(Arc::new(store))
                 .build()
                 .unwrap(),
         )

Ralith avatar Jan 25 '24 06:01 Ralith

I think this is blocked on the rustls-platform-verifier upgrade to rustls 0.23 for now.

djc avatar Mar 04 '24 08:03 djc

Hi @djc , should I do a fork and then open a PR targeting this branch or should I ask for access? (I already tried the gh tool but I think it uses the same permission and usual git)

gabrik avatar Apr 04 '24 12:04 gabrik

Probably just a fork + PR targeting this branch?

djc avatar Apr 04 '24 12:04 djc

Done PR here: https://github.com/quinn-rs/quinn/pull/1808

gabrik avatar Apr 04 '24 13:04 gabrik

PR: https://github.com/quinn-rs/quinn/pull/1809

gabrik avatar Apr 05 '24 15:04 gabrik

Made a deep pass; looks like only some minor cosmetic issues left.

Ralith avatar Apr 11 '24 05:04 Ralith

@Ralith I think this is ready for another round -- hoping it will be the last one!

djc avatar Apr 21 '24 21:04 djc

Release progress is tracked in #1737.

djc avatar Apr 22 '24 07:04 djc