tokio-quiche: expose client provided DCID interface

[csujedihy]

#2234 allows the user to have control over DCID when creating a new client side connection. This PR exposes the interface on tokio-quiche under the same feature flag.

[jannes]

probably a question for the earlier MR, but why is this behind a feature flag? EDIT: i see it was Lucas requesting that on the initial MR. don't really agree, but it is what it is

[toidiu]

This change introduces a lot of conditional compilation which is very hard to maintain. Is there a way to reduce the changes to a smaller area?

Feature flagging code like is it very hard to maintain because its conditional compilation and we have to tests every variant. For example I dont think we test with custom-client-dcid in CI.

[csujedihy]

I agree the cfg gates make the code hard to reason. I tried to simplify this but in the end, there are 4 different cases. I heard @gregor-cf has been working on getting rid of the zero-copy feature flag. Maybe we will end up with just 2 cases in the future?

[toidiu]

Since buffer methods are not behind a feature you should be able to do the following and then match on zero_copy_enabled to either call the buffer_factory method or not.

Personally I think its worth using cfg-if since the if-else makes it easier to reason about correctness.

diff --git a/tokio-quiche/Cargo.toml b/tokio-quiche/Cargo.toml
index 32dbc212..107ee165 100644
--- a/tokio-quiche/Cargo.toml
+++ b/tokio-quiche/Cargo.toml
@@ -42,6 +42,7 @@ unexpected_cfgs = { level = "warn", check-cfg = ['cfg(capture_keylogs)'] }
 
 [dependencies]
 anyhow = { workspace = true }
+cfg-if = "1.0"
 boring = { workspace = true }
 buffer-pool = { workspace = true }
 crossbeam = { workspace = true, default-features = false }
diff --git a/tokio-quiche/src/quic/mod.rs b/tokio-quiche/src/quic/mod.rs
index 9cb6d3b4..ba6f08a9 100644
--- a/tokio-quiche/src/quic/mod.rs
+++ b/tokio-quiche/src/quic/mod.rs
@@ -200,6 +200,14 @@ where
     let mut client_config = Config::new(params, socket.capabilities)?;
     let scid = SimpleConnectionIdGenerator.new_connection_id();
 
+    cfg_if::cfg_if! {
+        if #[cfg(feature = "zero-copy")] {
+            let zero_copy_enabled = true;
+        } else {
+            let zero_copy_enabled = false;
+        }
+    };
+
     #[cfg(all(feature = "zero-copy", feature = "custom-client-dcid"))]
     let mut quiche_conn = if let Some(dcid) = &params.dcid {
         quiche::connect_with_dcid_and_buffer_factory(

[gregor-cf]

Actually, I think we don't need to check for zero-copy at all here. The QuicheConnection is already typed with the proper BufFactory and we can just use *with_buffer_factory. The compiler can do the rest:

    #[cfg(feature = "custom-client-dcid")]
    let mut quiche_conn = if let Some(dcid) = &params.dcid {
        quiche::connect_with_dcid_and_buffer_factory(
            host,
            &scid,
            dcid,
            socket.local_addr,
            socket.peer_addr,
            client_config.as_mut(),
        )?
    } else {
        quiche::connect_with_buffer_factory(
            host,
            &scid,
            socket.local_addr,
            socket.peer_addr,
            client_config.as_mut(),
        )?
    };

    #[cfg(not(feature = "custom-client-dcid"))]
    let mut quiche_conn = quiche::connect_with_buffer_factory(
        host,
        &scid,
        socket.local_addr,
        socket.peer_addr,
        client_config.as_mut(),
    )?;

[gregor-cf]

(bascially the cfg-gating in the original code was unnessary)

[csujedihy]

Pushed an iteration with that suggestion

[toidiu]

Actually, I think we don't need to check for zero-copy at all here. The QuicheConnection is already typed with the proper BufFactory and we can just use *with_buffer_factory. The compiler can do the rest:

quiche/tokio-quiche/src/quic/mod.rs

Lines 127 to 132 in b3a9aa7

	/// Alias of [quiche::Connection] used internally by the crate.
	#[cfg(feature = "zero-copy")]
	pub type QuicheConnection = quiche::Connection<crate::buf_factory::BufFactory>;
	/// Alias of [quiche::Connection] used internally by the crate.
	#[cfg(not(feature = "zero-copy"))]
	pub type QuicheConnection = quiche::Connection;

.

Yep good catch.

[LPardue]

Do not remove this being gated by a feature. The reasons for it being gated are clearly documented. Removing it highlights a lack of understanding those risks.

[gregor-cf]

Do not remove this being gated by a feature. The reasons for it being gated are clearly documented. Removing it highlights a lack of understanding those risks.

If you are concerned about making it easily accessible, would you be ok with removing the flag and marking the functions that take the dcid unsafe. This IMHO raises a higher bar even than a feature flag.
With a feature flag, we can end up with a large matrix of flags. E.g., the custom-client-id features is currently not enabled during CI runs.

[LPardue]

No I would not. This feature is pushing the boundaries of the QUIC standard. It is a huge footgun and needs to have friction to enable. Unsafe does not capture this.

I would rather remove the entire thing than compromise the security of QUIC connections because of CI challenges.

[toidiu]

the custom-client-id features is currently not enabled during CI runs.

Can we enable this in CI? It really should have been done when the feature was added.

[csujedihy]

the custom-client-id features is currently not enabled during CI runs.

Can we enable this in CI? It really should have been done when the feature was added.

I added a test run with the feature enabled in the nightly workflow.