Introduces preferred addressing to the Quiche apps server and client examples #1634

Clendenin · 2023-10-18T14:40:42Z

This PR:

Introduces the preferred addressing feature, proposed here Adds Support for Preferred Addressing #1502, into the Quiche server app and client app examples
Is a superset of that PR since it is dependent on it. Therefore it contains all the commits from it. All new code exclusively relevant to this PR can be found in the large squashed commit found here: c209958 and any subsequent commits to resolve merge/rebase issues.
Based on this original PR by another contributor: add support for configuring and communicating preferred address #1208
Addresses issue: Support for "server's preferred address" #828

To reproduce:

Run server by: RUST_LOG=trace cargo run --bin quiche-server -- --cert apps/src/bin/cert.crt --key apps/src/bin/cert.key --enable-active-migration --max-active-cids 4 --preferred-address 127.0.0.1:4435
Run client by: RUST_LOG=trace cargo run --bin quiche-client -- --no-verify --enable-active-migration --perform-migration --max-active-cids 4 https://127.0.0.1:4433
The Quiche Client App initially migrates to the servers preferred address (127.0.0.1:4435). It then performs a subsequent migration to a new port of its own.
Confirm migration by seeing path validations, examples below, and other logs stating the number of written bytes on your migrated path.

quiche/src/lib.rs

Clendenin · 2024-07-01T15:14:27Z

quiche/src/lib.rs

+                /// IPv4: 4 bytes
+                /// Port: 2 bytes
+                /// IPv6: 16 bytes


"nit: these should use double quotes //" - Lucas

I believe my original intention was to have these serve as "Documentation Comments" to the constant below. This is what it looks like when the documentation for the constant is pulled up in my editor:

Lucas: "Switch these to regular //"

quiche/src/lib.rs

apps/src/client.rs

Clendenin · 2024-07-01T15:50:13Z

apps/src/bin/quiche-server.rs

+    // Create another preferred UDP listening socket that can be used for server
+    // migration.
+    let mut preferred_socket =
+        mio::net::UdpSocket::bind(preferred_address).unwrap();


Can this preferred socket take new connections not just migrated connections?
If a new connection starts here, is the preferred address provided over the transport params? What does this cause? - Lucas

Error Result if preferred address is same as current address - fail on startup - Lucas

Yeah that is a great question.
For the example app the app will accept connections starting on the servers preferred address. The server will still encode a "preferred address" transport parameter into the transport parameters it initially sends to the client. The client will receive the transport parameters and begin decoding them. It will see an encoded preferred address transport parameter and begin decoding it. The example app will then initiate probe_path() on this "new" preferred address. The probe_path() logic sees that this is an existing path. It will generate a Path_Challenge frame and then it does not do path migration since there in no migration to perform.

This separation of concerns makes sense to me. The probe_path / connection migration code should be resilient enough to not attempt migrations to the path it is already on.

Lucas: "Add a comment to explicitly state that this is intentional and not a bug"
Lucas to follow up with working group to further discuss.

LPardue · 2024-07-10T15:43:20Z

quiche/src/cid.rs

-    ///
-    /// Returns a list of tuples (DCID sequence number, Path ID), containing the
-    /// sequence number of retired DCIDs that were linked to their respective
-    /// Path ID.


This comment has been deleted but there was no code change. Is the comment actually wrong (and we can just fix that up in a quick standalone PR) or is there something else at hand?

The comment was out of date. iirc the function was changed but the comment was never updated.

In that case, please create a separate PR with the change

LPardue · 2024-07-18T13:07:29Z

apps/src/client.rs


            conn.on_timeout();
        }

        // Read incoming UDP packets from the socket and feed them to quiche,
        // until there are no more packets to read.
        for event in &events {
+            trace!("Event occured: {:?}", event);


This will be too spammy to land

I am confused. Isn't this file just to demonstrate how to use the code? I left this in because it is helpful to see what event occurred and when.

Lucas "Still remove this"

apps/src/client.rs

LPardue · 2024-07-18T13:12:55Z

quiche/src/lib.rs

+    /// Returns the server's preferred address transport parameters which can be
+    /// used to perform a connection migration.
+    pub fn server_preferred_address_params(
+        &mut self,


doesn't need to be mut

The internal call to new_destination_cid() requires it be mutable

Lucas: "Double check to see if you can remove this"

quiche/src/lib.rs

LPardue · 2024-07-18T13:25:05Z

quiche/src/lib.rs

+        let port_v4 = b.get_u16()?;
+
+        if ip_v4.is_unspecified() ^ (port_v4 == 0) {
+            return Err(Error::InvalidTransportParam);


I think this is trying to enforce https://datatracker.ietf.org/doc/html/rfc9000#section-18.2-4.32.1 ?

Servers MAY choose to only send a preferred address of one address family by sending an all-zero address and port (0.0.0.0:0 or [::]:0) for the other family. IP addresses are encoded in network byte order.

My concern is that this is called in the top-level [decode()](https://github.com/cloudflare/quiche/pull/1634/files#diff-2ac678416f8d7ea462a714681b63acedcbbb7f2c99e537ce61b7667b3bf4ac41R8589) function and would early abort something that is not technicall invalid

If the IPv4 address is unspecified (all 0s) and the associated port is also unspecified (all 0s) then the XOR operation will fail and this decode_v4 function will return Ok(None)

LPardue · 2024-07-18T17:02:37Z

apps/src/client.rs

+                // event occur and begin migration. The server will see
+                // `PathEvent::PeerMigrated` upon successful
+                // migration.
+                if let Some(addr_v4) = preferred_address_params.addr_v4 {


The selection of which preffered address to use is subjective - this is just an example. There's also bit of duplication across the branches that can be reduced. Consider extracting into a function like "select_and_probe_preferred_address"

I coalesced the branches into one to dry the code.

Lucas: "Put this into a function to make it more obvious to others"

LPardue · 2024-07-18T17:03:54Z

apps/src/client.rs

+        }
+
+        // Perform client side connection migration
+        if args.perform_migration &&


Also consider making this its own function

I don't quite follow, what is your reasoning to break this out into a function?

Lucas: "Split it out to enforce soc"

LPardue · 2024-07-18T17:04:56Z

quiche/src/cid.rs

-    ///
-    /// Returns a list of tuples (DCID sequence number, Path ID), containing the
-    /// sequence number of retired DCIDs that were linked to their respective
-    /// Path ID.


In that case, please create a separate PR with the change

quiche/src/frame.rs

quiche/src/lib.rs

apps/src/bin/quiche-server.rs

LPardue · 2024-07-18T17:52:40Z

apps/src/bin/quiche-server.rs

+                    // Create the new connection!
+                    #[allow(unused_mut)]
+                    // Automatically provisions SCID for preferred address.
+                    let mut conn = quiche::accept(


Is this a hangover? if we don't need the mut then lets not declare it as such

conn.set_keylog(Box::new(keylog)); needs a mutable ref

Lucas "Why have #[allow(unused_mut)]"

apps/src/bin/quiche-server.rs

Signed-off-by: Miguel Guarniz <[email protected]>

…s and builder

…tls()

…t argument

…rred addr test

Clendenin requested a review from a team as a code owner October 18, 2023 14:40

Clendenin changed the title ~~Introduce preferred addressing to the Quiche apps server and client examples~~ Introduces preferred addressing to the Quiche apps server and client examples Oct 18, 2023

Clendenin force-pushed the quiche-apps-preferred-addressing branch 2 times, most recently from 05d791a to b4a009c Compare April 12, 2024 22:06

Clendenin commented Jul 1, 2024

View reviewed changes

quiche/src/lib.rs Show resolved Hide resolved

Clendenin commented Jul 1, 2024

View reviewed changes

quiche/src/lib.rs Show resolved Hide resolved

Clendenin commented Jul 1, 2024

View reviewed changes

apps/src/client.rs Outdated Show resolved Hide resolved

Clendenin commented Jul 1, 2024

View reviewed changes

LPardue reviewed Jul 10, 2024

View reviewed changes

LPardue reviewed Jul 18, 2024

View reviewed changes

kckeiks and others added 18 commits August 6, 2024 09:50

add support for configuring and communicating preferred address

7c65073

Signed-off-by: Miguel Guarniz <[email protected]>

add validation of zero length cid in preferred address

54bf9dc

Signed-off-by: Miguel Guarniz <[email protected]>

add tests for handling zero length cid in preferred address

75234e8

Signed-off-by: Miguel Guarniz <[email protected]>

add docs

8fc0f23

Signed-off-by: Miguel Guarniz <[email protected]>

use SocketAddress when setting preferred address

28c78e2

Signed-off-by: Miguel Guarniz <[email protected]>

update type of stateless reset token in preferred address param

183d780

Signed-off-by: Miguel Guarniz <[email protected]>

Adds further testing, bug fixes, documentation, refactors, subroutine…

d523e88

…s and builder

cargo +nightly fmt

ff6ce29

Moved test config out of config and into test module

157a077

Moved scid assigment / check into function

4f90967

Removed preferred address helper functions

caf6b00

Moved zero scid + preferred address transport params check into with_…

7d89217

…tls()

Added check for client erroneously sending preferred address transpor…

2b5e2f8

…t argument

Fixed server peer preferred address check

fa01cee

Updated octet slicing

3a94a0e

Removed address_v4 and address_v6 helper functions

0dc371a

cargo +nightly fmt

f7f2607

Clippy fixes

201d713

Clendenin added 13 commits August 6, 2024 09:52

Moved buffer allocation

19aa136

Update comment for const size

84b7f45

Link to RFC added

d083fdf

Auto tracking DCID now

19eb7e4

Added a trace, removed a trace, refactored some code

499701f

Added info logs for migration:

1aa318c

Moved comment

d229776

Refactored comment

2fe7b75

DRY'd code

d8892a8

removed commented out code

f7aa4a3

Cargo nightly fmt

bc10645

cargo clippy

686db92

cargo fmt

045e5de

Clendenin force-pushed the quiche-apps-preferred-addressing branch from 8ae7f6e to 045e5de Compare August 6, 2024 15:02

Clendenin added 16 commits August 6, 2024 10:15

removed formatting change

c902d07

Removed comment change

7aedeff

updated error type

fd82fcc

Added documentation for set preferred addr

3bacd22

Cargo fmt

a459b2f

updated comment

fd74157

Removed doc comment

5737265

cargo +nightly fmt

5bda1d8

Added starting/preferred socket description to log

7063919

cargo +nightly fmt

137e6bc

Removed spammy traces

25b0f89

Added socket name to trace

05cbc63

Updated logging of new and repeat connections

6ecb5a6

Removed trace

7c006a5

removed trace path event peer migrated

63fdfa6

Refactored zero length connection ID test and added empty v4/v6 prefe…

98ac0e4

…rred addr test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduces preferred addressing to the Quiche apps server and client examples #1634

Introduces preferred addressing to the Quiche apps server and client examples #1634

Clendenin commented Oct 18, 2023 •

edited

Loading

Clendenin Jul 1, 2024 •

edited

Loading

Clendenin Jul 30, 2024 •

edited

Loading

Clendenin Aug 7, 2024

Clendenin Jul 1, 2024

Clendenin Jul 1, 2024

Clendenin Aug 1, 2024

Clendenin Aug 7, 2024

LPardue Jul 10, 2024

Clendenin Jul 12, 2024

LPardue Jul 18, 2024

LPardue Jul 18, 2024

Clendenin Aug 5, 2024

Clendenin Aug 7, 2024

LPardue Jul 18, 2024

Clendenin Aug 5, 2024

Clendenin Aug 7, 2024

LPardue Jul 18, 2024

Clendenin Aug 5, 2024

LPardue Jul 18, 2024

Clendenin Aug 6, 2024

Clendenin Aug 7, 2024

LPardue Jul 18, 2024

Clendenin Aug 6, 2024

Clendenin Aug 7, 2024

LPardue Jul 18, 2024

LPardue Jul 18, 2024

Clendenin Aug 6, 2024

Clendenin Aug 7, 2024

Introduces preferred addressing to the Quiche apps server and client examples #1634

Are you sure you want to change the base?

Introduces preferred addressing to the Quiche apps server and client examples #1634

Conversation

Clendenin commented Oct 18, 2023 • edited Loading

Clendenin Jul 1, 2024 • edited Loading

Choose a reason for hiding this comment

Clendenin Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clendenin commented Oct 18, 2023 •

edited

Loading

Clendenin Jul 1, 2024 •

edited

Loading

Clendenin Jul 30, 2024 •

edited

Loading