Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid dual-stack #516

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Avoid dual-stack #516

wants to merge 4 commits into from

Conversation

catap
Copy link

@catap catap commented Jun 22, 2024

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.

@catap catap mentioned this pull request Jun 22, 2024
Copy link

codecov bot commented Jun 22, 2024

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 ⚠️
Additional details and impacted files
@@             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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jlaine
Copy link
Contributor

jlaine commented Jun 22, 2024

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
Copy link
Author

catap commented Jun 22, 2024

@jlaine well, I can do something like this.

@jlaine
Copy link
Contributor

jlaine commented Jun 22, 2024

@jlaine well, I can do something like this.

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

@catap
Copy link
Author

catap commented Jun 22, 2024

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 catap force-pushed the openbsd branch 5 times, most recently from 3aa752d to 3c4a567 Compare June 22, 2024 22:53
@catap
Copy link
Author

catap commented Jun 26, 2024

@jlaine 👋

@catap
Copy link
Author

catap commented Jul 6, 2024

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
Copy link
Contributor

jlaine commented Jul 6, 2024

@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
Copy link
Author

catap commented Jul 7, 2024

@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
Copy link
Contributor

jlaine commented Jul 7, 2024

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
Copy link
Author

catap commented Jul 7, 2024

@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
Copy link
Author

catap commented Jul 7, 2024

@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
Copy link
Author

catap commented Jul 7, 2024

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
Copy link
Contributor

rthalley commented Jul 7, 2024

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
Copy link
Contributor

jlaine commented Jul 8, 2024

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

@@ -23,6 +23,7 @@ async def connect(
token_handler: Optional[QuicTokenHandler] = None,
wait_connected: bool = True,
local_port: int = 0,
dual_stack: bool = socket.has_dualstack_ipv6(),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would we want to disable dual stack on a system that supports it?

Copy link
Author

@catap catap Jul 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enforce binding to IPv6 only for example. Rigth now it binds to both protocol on Linux, and if you desier to bind only for IPv6 here no way.

catap added 3 commits July 8, 2024 15:28
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.
@catap
Copy link
Author

catap commented Jul 8, 2024

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

@catap
Copy link
Author

catap commented Aug 18, 2024

@jlaine and ping here as well

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants