Add support for DTLS timeouts #1180

jlaine · 2023-01-22T20:07:56Z

When performing a DTLS handshake, the DTLS state machine may need to be updated based on the passage of time, for instance in response to packet loss.

OpenSSL supports this by means of the DTLSv1_get_timeout and DTLSv1_handle_timeout methods, both of which are included in cryptography's bindings. This change adds Python wrappers for these methods in the Connection class.

alex · 2023-01-22T20:17:00Z

looks like CI has atrophied, #1181 should resolve it.

jlaine · 2023-01-22T20:28:33Z

looks like CI has atrophied, #1181 should resolve it.

Thanks for the quick response! Maintaining such a complex CI pipeline cannot be fun. Will rebase once CI stabilises.

jlaine · 2023-01-22T20:30:59Z

There is an error condition which I don't know how to trigger, which is DTLSv1_handle_timeout returning -1. I'd hate my PR to cause a coverage drop, any suggestions on how to handle this?

alex · 2023-01-22T20:35:11Z

From a quick skim of the OpenSSL source, it looks like if you call it repeatedly on the same socket you might be able to trigger it?

…

On Sun, Jan 22, 2023 at 3:31 PM Jeremy Lainé ***@***.***> wrote: There is an error condition which I don't know how to trigger, which is DTLSv1_handle_timeout returning -1. I'd hate my PR to cause a coverage drop, any suggestions on how to handle this? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

jlaine · 2023-01-22T21:47:05Z

From a quick skim of the OpenSSL source, it looks like if you call it repeatedly on the same socket you might be able to trigger it?

I can confirm that triggering a timeout 12 times in a row does indeed eventually trigger the error path, but at a heavy cost. As the timeout duration is doubled on each failure, it involves a lot of sleeping. The time for running test_ssl.py goes from 6s to a whopping 8mn!

alex · 2023-01-22T22:02:57Z

oof, is the timeout configurable?

…

On Sun, Jan 22, 2023 at 4:47 PM Jeremy Lainé ***@***.***> wrote: From a quick skim of the OpenSSL source, it looks like if you call it repeatedly on the same socket you might be able to trigger it? I can confirm that triggering a timeout 12 times in a row does indeed eventually trigger the error path, but at a heavy cost. As the timeout duration is doubled on each failure, it involves a *lot* of sleeping. The time for running test_ssl.py goes from 6s to a whopping 8mn! — Reply to this email directly, view it on GitHub <#1180 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBFVMX7XAYGKOADUGB3WTWTGLANCNFSM6AAAAAAUDFGF5M> . You are receiving this because you commented.Message ID: ***@***.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

jlaine · 2023-01-22T22:43:34Z

oof, is the timeout configurable?

There is no way to control the "maximum number of timeouts" as this is a hard-coded constant:

https://github.com/openssl/openssl/blob/b5557666bda56ce4b9464a3dbc65e2a1fa1e482b/include/openssl/dtls1.h#L52

There does seem to be a way to change the default "double the duration on each timeout" behaviour, but it's not trivial: there is a DTLS_set_timer_cb since OpenSSL 1.1.1:

https://www.openssl.org/docs/man1.1.1/man3/DTLS_set_timer_cb.html

It seems to be called here:

https://github.com/openssl/openssl/blob/c3bd630df0c3630c66155fb8c4baf54810d24695/ssl/d1_lib.c#L386

The catch is that DTLS_set_timer_cb is not present in cryptography's bindings, so this makes it quite an endeavour in the name of testing.

The only alternative I can think of is to mock lib.DTLSv1_handle_timeout to force it to return -1. This would admittedly make the test somewhat shallow, but it's something..

jlaine · 2023-02-09T10:01:54Z

@alex Any chance of getting this change reviewed please?

mhils

This looks reasonable to me so far. I guess the question @alex or @reaperhulk need to answer is if they want to expose DTLS_set_timer_cb for tests or if the current mocking there is good enough. I feel like the current mocking approach is good enough, but YMMV. :)

mhils · 2023-02-09T10:07:54Z

tests/test_ssl.py

+        assert seconds is not None
+
+        # Handle the timeout and check there is data to send.
+        time.sleep(seconds)


How long are we sleeping here roughly?

The first timeout is one second:

https://github.com/openssl/openssl/blob/4596c20b86871b2bb0f9a7f6b855c0b7f0d4fbf3/ssl/d1_lib.c#L273-L282

jlaine · 2023-02-09T12:39:42Z

FYI my usecase for this change is to finish moving aiortc (an implementation of WebRTC in Python) on top of pyOpenSSL and stop relying on the bindings provided by cryptography, which is bound to break at some point. Handling DTLS timeouts is the only bit missing.

reaperhulk · 2023-02-09T15:22:40Z

@mhils never feel like you can't merge things on your own recognizance here 😄 You're a maintainer just like we are!

Given that DTLS_set_timer_cb isn't in cryptography (and it doesn't appear we want to bind it) then the mock is sufficient for now. I'm all for work that avoids relying directly on cryptography's bindings. We do a good job of avoiding breaking pyOpenSSL, but make no such promises for anyone else!

reaperhulk · 2023-02-09T15:27:16Z

tests/test_ssl.py

+        # Testing this directly is prohibitively time consuming as the timeout
+        # duration is doubled on each retry, so the best we can do is to mock
+        # this condition.
+        with patch(


You can do this with pytest's monkeypatch by adding that arg to the test args and then doing something like:

monkeypatch.setattr(OpenSSL._util.lib, 'DTLSv1_handle_timeout', lambda x: -1)

This way you can avoid importing unittest.patch

Ah thanks I should have spotted the monkeypatch uses in other tests. Fixed.

mhils

Thanks! :)

This looks good % formatting stuff flagged by CI.

jlaine · 2023-02-13T08:11:30Z

Thanks! :)

This looks good % formatting stuff flagged by CI.

The formatting errors seem to be due to the release of black 23. I've included an additional commit which fixes these, but now CI seems to fail for some unrelated reason.. ?

mhils · 2023-02-13T10:47:40Z

Thanks. No further action required from your end here, I'll take care of this after #1185 is in.

When performing a DTLS handshake, the DTLS state machine may need to be updated based on the passage of time, for instance in response to packet loss. OpenSSL supports this by means of the `DTLSv1_get_timeout` and `DTLSv1_handle_timeout` methods, both of which are included in cryptography's bindings. This change adds Python wrappers for these methods in the `Connection` class.

jlaine · 2023-02-13T13:28:25Z

@mhils fantastic, thank you very much!

Support for DTLS timeouts was contributed upstream in PR pyca/pyopenssl#1180 which was released in version 23.1.0, so we can remove our local implementation.

jlaine mentioned this pull request Jan 22, 2023

[dtls] Use PyOpenSSL's API instead of low-level APIs aiortc/aiortc#813

Merged

jlaine force-pushed the dtls-timeout branch 2 times, most recently from 4eabb28 to 4c5978b Compare January 22, 2023 20:11

jlaine force-pushed the dtls-timeout branch 2 times, most recently from 6dcde67 to 2a2ff08 Compare January 22, 2023 21:43

jlaine force-pushed the dtls-timeout branch from 2a2ff08 to bd4c12a Compare January 22, 2023 22:59

mhils reviewed Feb 9, 2023

View reviewed changes

reaperhulk reviewed Feb 9, 2023

View reviewed changes

jlaine force-pushed the dtls-timeout branch 2 times, most recently from cea3eed to 14b3f69 Compare February 13, 2023 08:06

mhils previously approved these changes Feb 13, 2023

View reviewed changes

jlaine dismissed mhils’s stale review via e03ab50 February 13, 2023 08:09

mhils force-pushed the dtls-timeout branch from e03ab50 to 17c645d Compare February 13, 2023 12:24

mhils previously approved these changes Feb 13, 2023

View reviewed changes

update CHANGELOG

f41e616

mhils dismissed their stale review via f41e616 February 13, 2023 12:36

mhils approved these changes Feb 13, 2023

View reviewed changes

mhils merged commit 3517764 into pyca:main Feb 13, 2023

jlaine deleted the dtls-timeout branch February 13, 2023 13:28

jlaine mentioned this pull request Apr 1, 2023

[package] Use pyOpenSSL >= 23.1.0 which supports DTLS timeouts aiortc/aiortc#846

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for DTLS timeouts #1180

Add support for DTLS timeouts #1180

jlaine commented Jan 22, 2023 •

edited

alex commented Jan 22, 2023

jlaine commented Jan 22, 2023

jlaine commented Jan 22, 2023

alex commented Jan 22, 2023 via email

jlaine commented Jan 22, 2023

alex commented Jan 22, 2023 via email

jlaine commented Jan 22, 2023 •

edited

jlaine commented Feb 9, 2023

mhils left a comment •

edited

mhils Feb 9, 2023

jlaine Feb 9, 2023

jlaine commented Feb 9, 2023

reaperhulk commented Feb 9, 2023

reaperhulk Feb 9, 2023

jlaine Feb 13, 2023

mhils left a comment •

edited

jlaine commented Feb 13, 2023

mhils commented Feb 13, 2023

jlaine commented Feb 13, 2023

Add support for DTLS timeouts #1180

Add support for DTLS timeouts #1180

Conversation

jlaine commented Jan 22, 2023 • edited

alex commented Jan 22, 2023

jlaine commented Jan 22, 2023

jlaine commented Jan 22, 2023

alex commented Jan 22, 2023 via email

jlaine commented Jan 22, 2023

alex commented Jan 22, 2023 via email

jlaine commented Jan 22, 2023 • edited

jlaine commented Feb 9, 2023

mhils left a comment • edited

Choose a reason for hiding this comment

mhils Feb 9, 2023

Choose a reason for hiding this comment

jlaine Feb 9, 2023

Choose a reason for hiding this comment

jlaine commented Feb 9, 2023

reaperhulk commented Feb 9, 2023

reaperhulk Feb 9, 2023

Choose a reason for hiding this comment

jlaine Feb 13, 2023

Choose a reason for hiding this comment

mhils left a comment • edited

Choose a reason for hiding this comment

jlaine commented Feb 13, 2023

mhils commented Feb 13, 2023

jlaine commented Feb 13, 2023

jlaine commented Jan 22, 2023 •

edited

jlaine commented Jan 22, 2023 •

edited

mhils left a comment •

edited

mhils left a comment •

edited