aiortc / aioquic

QUIC and HTTP/3 implementation in Python
BSD 3-Clause "New" or "Revised" License
1.66k stars 236 forks source link

Avoid dual-stack #516

Open catap opened 3 months ago

catap commented 3 months ago

OpenBSD doesn't support dual stack, that makes this broken on machine which hasn't got IPv6.

Anyway, a machine which has IPv6, still, can't access IPv4 hosts.

codecov[bot] commented 3 months ago

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 99.92%. Comparing base (ff3281f) to head (9173a62). Report is 21 commits behind head on main.

Files Patch % Lines
src/aioquic/asyncio/client.py 75.00% 4 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #516 +/- ## =========================================== - Coverage 100.00% 99.92% -0.08% =========================================== Files 25 25 Lines 4999 5124 +125 =========================================== + Hits 4999 5120 +121 - Misses 0 4 +4 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

jlaine commented 3 months ago

Just because OpenBSD doesn't support dual-stack doesn't sound like a reason to disable the feature for all OS's that do. Can we detect the fact dual-stack doesn't work? Or opt in for this behaviour?

catap commented 3 months ago

@jlaine well, I can do something like this.

jlaine commented 3 months ago

@jlaine well, I can do something like this.

Any suggestions on how to add OpenBSD to our CI pipeline?

catap commented 3 months ago

A few moths ago I did https://github.com/catap/shell whcih can be used as an example to add different system to github actions.

catap commented 3 months ago

@jlaine 👋

catap commented 2 months ago

I'd like to share a proof that with this PR both stacke: IPv4 and IPv6 working on my laptop under OpenBSD:

aioquic $ python3 examples/http3_client.py https://ipv6.google.com/                          
2024-07-06 17:37:32,138 INFO quic [47ef5729d8cb71aa] ALPN negotiated protocol h3
2024-07-06 17:37:32,241 INFO quic [47ef5729d8cb71aa] Duplicate CRYPTO data received for epoch Epoch.HANDSHAKE
2024-07-06 17:37:32,274 INFO client New session ticket received
2024-07-06 17:37:32,275 INFO client New session ticket received
2024-07-06 17:37:32,707 INFO client Response received for GET / : 1579 bytes in 0.6 s (0.022 Mbps)
2024-07-06 17:37:32,708 INFO quic [47ef5729d8cb71aa] Connection close sent (code 0x100, reason )
aioquic $ python3 examples/http3_client.py https://ipv4.google.com/ 
2024-07-06 17:37:36,969 INFO quic [954ed09015d58c2c] ALPN negotiated protocol h3
2024-07-06 17:37:37,004 INFO quic [954ed09015d58c2c] Duplicate CRYPTO data received for epoch Epoch.HANDSHAKE
2024-07-06 17:37:37,047 INFO client New session ticket received
2024-07-06 17:37:37,048 INFO client New session ticket received
2024-07-06 17:37:37,285 INFO client Response received for GET / : 21455 bytes in 0.3 s (0.548 Mbps)
2024-07-06 17:37:37,285 INFO quic [954ed09015d58c2c] Connection close sent (code 0x100, reason )
aioquic $ 
jlaine commented 2 months ago

@rthalley I'd really hate to introduce code we're not able to test, do you have any suggestions on how to proceed? Do you have any well-established projects in mind which have an example OpenBSD setup in their CI?

catap commented 2 months ago

@jlaine, here a way. Some time ago I write an article how to utilize GitHub Actions as a shell on different OS: https://kirill.korins.ky/articles/using-github-actions-as-a-temporary-shell/ which leads to create a demo repository https://github.com/catap/shell which uses https://github.com/vmactions which can be used to extend your CI to some additional OS.

jlaine commented 2 months ago

Looking into this a bit deeper, I don't think we need to inspect the platform at all, there is a socket.has_dualstack_ipv6() method since Python 3.8 (which is the oldest we support):

https://docs.python.org/3/library/socket.html#socket.has_dualstack_ipv6

This means we can probably get away with mocking this method and don't actually need to run the tests on BSD. Also I suspect OpenBSD is not the only BSD-family OS with this behaviour.

This Python bug has some relevant information: https://bugs.python.org/issue17561

catap commented 2 months ago

@jlaine I had ported this spring Scala-Native to different BSD and if I recall right the same behaviour is true for NetBSD. Anyway, as far as I recall FreeBSD supports dual-stack like Linux.

But it has much more differences with scoket strucutres length which should be ignored here.

catap commented 2 months ago

@jlaine like this I was able to pass almots all unit test on OpenBSD. Anyway, it has one more things: OpenBSD uses LibreSSL which doesn't support ED448, so I need this patch:

Index: tests/test_asyncio.py
--- tests/test_asyncio.py.orig
+++ tests/test_asyncio.py
@@ -192,14 +196,6 @@ class HighLevelTest(TestCase):
     async def test_connect_and_serve_with_ed25519_certificate(self):
         await self._test_connect_and_serve_with_certificate(
             *generate_ed25519_certificate(
-                alternative_names=["localhost"], common_name="localhost"
-            )
-        )
-
-    @asynctest
-    async def test_connect_and_serve_with_ed448_certificate(self):
-        await self._test_connect_and_serve_with_certificate(
-            *generate_ed448_certificate(
                 alternative_names=["localhost"], common_name="localhost"
             )
         )

Index: tests/test_tls.py
--- tests/test_tls.py.orig
+++ tests/test_tls.py
@@ -575,11 +575,6 @@ class ContextTest(TestCase):
             *generate_ed25519_certificate(common_name="example.com")
         )

-    def test_handshake_with_ed448_certificate(self):
-        self._test_handshake_with_certificate(
-            *generate_ed448_certificate(common_name="example.com")
-        )
-
     def test_handshake_with_alpn(self):
         client = self.create_client(alpn_protocols=["hq-20"])
         server = self.create_server(alpn_protocols=["hq-20", "h3-20"])

and, additionally, it has small differences inside API which I fixed by this patch:

Index: src/aioquic/_crypto.c
--- src/aioquic/_crypto.c.orig
+++ src/aioquic/_crypto.c
@@ -406,11 +406,5 @@ PyInit__crypto(void)
         return NULL;
     PyModule_AddObject(m, "HeaderProtection", HeaderProtectionType);

-    // ensure required ciphers are initialised
-    EVP_add_cipher(EVP_aes_128_ecb());
-    EVP_add_cipher(EVP_aes_128_gcm());
-    EVP_add_cipher(EVP_aes_256_ecb());
-    EVP_add_cipher(EVP_aes_256_gcm());
-
     return m;
 }

because all chipers are handled via static lookup inside LibreSSL.

I plan to ship it as the next PR.

catap commented 2 months ago

But it has much more differences with scoket strucutres length which should be ignored here.

I was wrong. After switch to 1.2.0 (before I've used 1.0.0) I need two more hacks inside unit tests:

Index: tests/test_connection.py
--- tests/test_connection.py.orig
+++ tests/test_connection.py
@@ -2790,7 +2790,7 @@ class QuicConnectionTest(TestCase):
             # window too strictly as its exact value depends on the size
             # of our ACKs, which depends on the execution time.
             self.assertEqual(client._loss.bytes_in_flight, 0)
-            self.assertGreaterEqual(client._loss.congestion_window, 13530)
+            self.assertGreaterEqual(client._loss.congestion_window, 13472)
             self.assertLessEqual(client._loss.congestion_window, 13540)

             # artificially raise received data counter

and

Index: tests/test_tls.py
--- tests/test_tls.py.orig
+++ tests/test_tls.py
@@ -118,7 +118,7 @@ def reset_buffers(buffers):
 class ContextTest(TestCase):
     def assertClientHello(self, data: bytes):
         self.assertEqual(data[0], tls.HandshakeType.CLIENT_HELLO)
-        self.assertGreaterEqual(len(data), 191)
+        self.assertGreaterEqual(len(data), 189)
         self.assertLessEqual(len(data), 564)

     def create_client(
rthalley commented 2 months ago

I like the plan to use socket.has_dualstack_ipv6() as then we can just run the relevant tests both ways in the CI. It's not quite the same as running on the actual OS, but likely good enough.

jlaine commented 2 months ago

Let's keep this PR narrowly focused on the socket stuff please :)

catap commented 2 months ago

@jlaine I had rebased this branch to the last main, and created dedicated PRs to addresses different small polish for OpenBSD

catap commented 1 month ago

@jlaine and ping here as well