feat: Fix and enable connection migration tests #410

larseggert · 2024-10-14T09:17:19Z

No description provided.

testcases.py

For example, to avoid using zero-len CIDs.

larseggert · 2024-11-04T14:59:48Z

larseggert · 2024-11-06T13:16:40Z

@marten-seemann this is ready from my side.

larseggert · 2024-11-08T13:30:45Z

@marten-seemann conflict is resolved.

mxinden · 2024-12-16T13:21:22Z

@larseggert @marten-seemann I assume this is still worth merging?

PiotrSikora · 2024-12-16T18:55:57Z

FYI, using this branch with ghcr.io/mozilla/neqo-qns@3d9351310775 doesn't fix those tests for me:

~/quic-interop-runner$ ./run.py --client neqo --server neqo --test rebind-port,rebind-addr,connectionmigration
Saving logs to logs_2024-12-16T13:47:18.
Server: neqo. Client: neqo. Running test case: rebind-port
First server packet on new path (('193.167.100.100', 443), ('193.167.0.100', 57607)) did not contain a PATH_CHALLENGE frame
Layer QUIC
:       QUIC Connection information
        Expert Info (Note/Protocol): Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Severity level: Note
        Group: Protocol
        Packet Length: 27
        QUIC Short Header
        0... .... = Header Form: Short Header (0)
        .1.. .... = Fixed Bit: True
        ..0. .... = Spin Bit: False
        Remaining Payload: eb2b35fd069b454771d4d5e4fd39a8a4ba0bd1dda2b803ce7148

Server: neqo. Client: neqo. Running test case: rebind-addr
At least one QUIC packet could not be decrypted
First server packet on new path (('193.167.100.100', 443), ('193.167.0.224', 59022)) did not contain a PATH_CHALLENGE frame
Layer QUIC
:       QUIC Connection information
        Expert Info (Note/Protocol): Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Severity level: Note
        Group: Protocol
        Packet Length: 27
        QUIC Short Header
        0... .... = Header Form: Short Header (0)
        .0.. .... = Fixed Bit: False
        ..1. .... = Spin Bit: True
        Remaining Payload: a761a55f7de9efd6426cb682a15da8fdb421917c52c7a905bb56

At least one QUIC packet could not be decrypted
Server saw these client addresses: {'193.167.0.3', '193.167.0.100', '193.167.0.224', '193.167.0.71'}
At least one QUIC packet could not be decrypted
First server packet on new path (('193.167.100.100', 443), ('193.167.0.224', 59022)) did not contain a PATH_CHALLENGE frame
Layer QUIC
:       QUIC Connection information
        Expert Info (Note/Protocol): Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Severity level: Note
        Group: Protocol
        Packet Length: 27
        QUIC Short Header
        0... .... = Header Form: Short Header (0)
        .0.. .... = Fixed Bit: False
        ..1. .... = Spin Bit: True
        Remaining Payload: a761a55f7de9efd6426cb682a15da8fdb421917c52c7a905bb56

Server: neqo. Client: neqo. Running test case: connectionmigration
First server packet on new path (('fd00:cafe:cafe:100::100', 4443), ('fd00:cafe:cafe::100', 34845)) did not contain a PATH_CHALLENGE frame
Layer QUIC
:       QUIC Connection information
        Expert Info (Note/Protocol): Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Severity level: Note
        Group: Protocol
        Packet Length: 1232
        QUIC Short Header
        0... .... = Header Form: Short Header (0)
        .0.. .... = Fixed Bit: False
        ..0. .... = Spin Bit: False
        Remaining Payload: e3678bf3e8f6161e0d4c19bc5b3f81094ee54443d31cab012f1c631b139fef9358a1689f…

Server saw these client addresses: {'fd00:cafe:cafe::100'}
Server saw only a single client IP address in use; test broken?
First server packet on new path (('fd00:cafe:cafe:100::100', 4443), ('fd00:cafe:cafe::100', 34845)) did not contain a PATH_CHALLENGE frame
Layer QUIC
:       QUIC Connection information
        Expert Info (Note/Protocol): Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Unknown QUIC connection. Missing Initial Packet or migrated connection?
        Severity level: Note
        Group: Protocol
        Packet Length: 1232
        QUIC Short Header
        0... .... = Header Form: Short Header (0)
        .0.. .... = Fixed Bit: False
        ..0. .... = Spin Bit: False
        Remaining Payload: e3678bf3e8f6161e0d4c19bc5b3f81094ee54443d31cab012f1c631b139fef9358a1689f…

Server saw these client addresses: {'fd00:cafe:cafe::100'}
Server saw only a single client IP address in use; test broken?
Run took 0:02:34.886099
+------+-------------+
|      |     neqo    |
+------+-------------+
| neqo |     ✓()     |
|      |     ?()     |
|      | ✕(BP,BA,CM) |
+------+-------------+

Also, s2n-quic has an alternative patch (https://github.com/aws/s2n-quic/blob/main/.github/interop/runner.patch) for those tests, and the rebinding tests are passing for me across multiple implementations when using it.

larseggert · 2024-12-16T20:07:16Z

Those Wireshark messages probably mean you have a version installed that is too old.

PiotrSikora · 2024-12-16T20:14:08Z

Those Wireshark messages probably mean you have a version installed that is too old.

$ tshark -v
TShark (Wireshark) 4.0.17 (Git v4.0.17 packaged as 4.0.17-0+deb12u1).
$ editcap -v
Editcap (Wireshark) 4.0.17 (Git v4.0.17 packaged as 4.0.17-0+deb12u1).

Isn't that recent enough? README.md says 3.5.0+... Although, I'm not sure what the "development version" refers to, I suspect it's a leftover from before 4.x stable release?

larseggert · 2024-12-17T06:30:47Z

Those are too old. I use TShark (Wireshark) 4.5.0rc0-830-g10cc65f41321 (v4.5.0rc0-830-g10cc65f41321). at the moment.

PiotrSikora · 2024-12-17T09:41:12Z

Thanks, that helped!

However, even with tshark built from the same commit as yours (10cc65f413214ee66ea0d2a3cb04c6ef65bad079), the connection migration test still fails for me:

$ ./run.py --client neqo --server neqo --test rebind-port,rebind-addr,connectionmigration
Saving logs to logs_2024-12-17T03:40:19.
Server: neqo. Client: neqo. Running test case: rebind-port
Server saw these paths used: {(('193.167.100.100', 443), ('193.167.0.100', 44827)), (('193.167.100.100', 443), ('193.167.0.100', 57607)), (('193.167.100.100', 443), ('193.167.0.100', 59022))}
Server: neqo. Client: neqo. Running test case: rebind-addr
Server saw these paths used: {(('193.167.100.100', 443), ('193.167.0.224', 59022)), (('193.167.100.100', 443), ('193.167.0.71', 39968)), (('193.167.100.100', 443), ('193.167.0.100', 55795)), (('193.167.100.100', 443), ('193.167.0.3', 38910))}
Server saw these client addresses: {'193.167.0.100', '193.167.0.71', '193.167.0.224', '193.167.0.3'}
Server saw these paths used: {(('193.167.100.100', 443), ('193.167.0.224', 59022)), (('193.167.100.100', 443), ('193.167.0.71', 39968)), (('193.167.100.100', 443), ('193.167.0.100', 55795)), (('193.167.100.100', 443), ('193.167.0.3', 38910))}
Server: neqo. Client: neqo. Running test case: connectionmigration
Server saw these paths used: {(('fd00:cafe:cafe:100::100', 443), ('fd00:cafe:cafe::100', 43132)), (('fd00:cafe:cafe:100::100', 4443), ('fd00:cafe:cafe::100', 43132))}
Server saw these client addresses: {'fd00:cafe:cafe::100'}
Server saw only a single client IP address in use; test broken?
Server saw these paths used: {(('fd00:cafe:cafe:100::100', 443), ('fd00:cafe:cafe::100', 43132)), (('fd00:cafe:cafe:100::100', 4443), ('fd00:cafe:cafe::100', 43132))}
Server saw these client addresses: {'fd00:cafe:cafe::100'}
Server saw only a single client IP address in use; test broken?
Run took 0:02:57.550582
+------+----------+
|      |   neqo   |
+------+----------+
| neqo | ✓(BP,BA) |
|      |   ?()    |
|      |  ✕(CM)   |
+------+----------+

Note that the connection migration test is using IPv6, and not IPv4 like the rebinding tests (which are passing now), so perhaps the issue is elsewhere?

README.md

Co-authored-by: Piotr Sikora <[email protected]>

marten-seemann

Do we need to bump the WIRESHARK_CACHEKEY to build a new Wireshark version?

marten-seemann · 2025-01-13T14:26:08Z

README.md

-```bash
-pip3 install -r requirements.txt
-```
+   ```bash
+   pip3 install -r requirements.txt
+   ```


This doesn't look correct.

It is? You gotta indent the block if it's supposed to be part of the bullet.

I see, makes sense.

marten-seemann · 2025-01-13T14:30:58Z

testcases.py

+    @staticmethod
+    def testname(p: Perspective):
+        return "transfer"
+


This means that implementation that don't implement path validation yet (like quic-go 🤐) will fail this test. I think this is ok, since this is a mandatory part of the RFC. Might lead to some complaints though if the matrix is getting slightly more red as a result of merging this PR.

No progress without pain.

lxin · 2025-01-21T15:25:51Z

@larseggert I'm adding the ConnectionMigration testcase for linux QUIC where I set preferred address ('193.167.100.100', 444) from server side. However, it failed in the check() of TestCaseAddressRebinding (the parent class of TestCaseConnectionMigration):

        if len(ips) <= 1:
            logging.info(
                "Server saw only a single client IP address in use; test broken?"
            )
            return TestResult.FAILED

I don't understand why it requires client source IP address to change after the client migrates its dest IP to the server's preferred address? did you see any description on RFC for that?

Thanks.

larseggert · 2025-01-21T15:33:48Z

The assumption for TestCaseConnectionMigration was that the client needs to migrate from IPv4 to IPv6 (or vice versa) to the server's preferred address, because the simulator nodes only have one IPv4 and IPv6 address each.

I don't think simply sending another IPv4 address in the TPs works?

lxin · 2025-01-21T15:47:56Z

oh, I see the idea now, thanks for the explanation.

larseggert · 2025-01-23T15:23:39Z

@lxin actually, I misremembered and what I said was incorrect. TestCaseConnectionMigration should work when migrating to a preferred address of the same address family as is currently used. The server can just use a different port for the preferred address. So there is a bug.

lxin · 2025-01-23T17:09:43Z

@larseggert I think it’s fine for this to serve as a test case for QUIC’s ability to handle migration between IPv4 and IPv6 (or vice versa), just the test case description should explicitly states this purpose. Otherwise, users might interpret it as writing migration within the same address family, as I initially did.

This is a great test case for migration across address families to me. It might be worth requiring implementations to fully support this scenario, rather than simply 'working around' it by limiting the migration to the same address family to pass the test case.

BTW, with the fix you posted in #419, it now passes in the Linux QUIC when the migration involves only a port change.

Thanks.

mxinden reviewed Oct 16, 2024

View reviewed changes

testcases.py Show resolved Hide resolved

mxinden approved these changes Oct 27, 2024

View reviewed changes

larseggert added 6 commits November 3, 2024 11:15

feat: Fix and enable connection migration tests

6623b30

Client might need to know which test this is

538c786

For example, to avoid using zero-len CIDs.

More fixes

298264d

More

c5b0ba1

More fixes

9dc67c2

Fixes

2324a3d

larseggert force-pushed the feat-migration branch from 094645a to 2324a3d Compare November 3, 2024 13:28

Do not require path validation if only the port rebinds

028d6f1

larseggert added 2 commits November 4, 2024 16:32

Fixes

4ba6e90

Require path challenge on port change

86427d1

Merge branch 'master' into feat-migration

3432b55

larseggert mentioned this pull request Nov 11, 2024

feat: Option for no zero-len CIDs for client mozilla/neqo#2171

Merged

PiotrSikora reviewed Dec 17, 2024

View reviewed changes

README.md Outdated Show resolved Hide resolved

Update README.md

cde2270

Co-authored-by: Piotr Sikora <[email protected]>

marten-seemann reviewed Jan 13, 2025

View reviewed changes

larseggert added 2 commits January 13, 2025 16:46

Bump WIRESHARK_CACHEKEY

1b4e1f5

Merge branch 'master' into feat-migration

9b5d2fe

marten-seemann merged commit f57bcf2 into quic-interop:master Jan 13, 2025
1 check passed

larseggert deleted the feat-migration branch January 21, 2025 15:59

larseggert mentioned this pull request Jan 23, 2025

CM bug when migrating within one address family #419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Fix and enable connection migration tests #410

feat: Fix and enable connection migration tests #410

larseggert commented Oct 14, 2024

larseggert commented Nov 4, 2024

larseggert commented Nov 6, 2024

larseggert commented Nov 8, 2024

mxinden commented Dec 16, 2024

PiotrSikora commented Dec 16, 2024

larseggert commented Dec 16, 2024

PiotrSikora commented Dec 16, 2024

larseggert commented Dec 17, 2024

PiotrSikora commented Dec 17, 2024

marten-seemann left a comment

marten-seemann Jan 13, 2025

larseggert Jan 13, 2025

marten-seemann Jan 13, 2025

marten-seemann Jan 13, 2025

larseggert Jan 13, 2025

lxin commented Jan 21, 2025

larseggert commented Jan 21, 2025 •

edited

Loading

lxin commented Jan 21, 2025

larseggert commented Jan 23, 2025

lxin commented Jan 23, 2025

feat: Fix and enable connection migration tests #410

feat: Fix and enable connection migration tests #410

Conversation

larseggert commented Oct 14, 2024

larseggert commented Nov 4, 2024

larseggert commented Nov 6, 2024

larseggert commented Nov 8, 2024

mxinden commented Dec 16, 2024

PiotrSikora commented Dec 16, 2024

larseggert commented Dec 16, 2024

PiotrSikora commented Dec 16, 2024

larseggert commented Dec 17, 2024

PiotrSikora commented Dec 17, 2024

marten-seemann left a comment

Choose a reason for hiding this comment

marten-seemann Jan 13, 2025

Choose a reason for hiding this comment

larseggert Jan 13, 2025

Choose a reason for hiding this comment

marten-seemann Jan 13, 2025

Choose a reason for hiding this comment

marten-seemann Jan 13, 2025

Choose a reason for hiding this comment

larseggert Jan 13, 2025

Choose a reason for hiding this comment

lxin commented Jan 21, 2025

larseggert commented Jan 21, 2025 • edited Loading

lxin commented Jan 21, 2025

larseggert commented Jan 23, 2025

lxin commented Jan 23, 2025

larseggert commented Jan 21, 2025 •

edited

Loading