Add support for DTLS timeouts by jlaine · Pull Request #1180 · pyca/pyopenssl (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation19 Commits2 Checks0 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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.

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

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.

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?

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.

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!

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.

@alex Any chance of getting this change reviewed please?

Member

@mhils mhils left a comment • Loading

Choose a reason for hiding this comment

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

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. :)

assert seconds is not None

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

Choose a reason for hiding this comment

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

How long are we sleeping here roughly?

Choose a reason for hiding this comment

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

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.

@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!

# 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(

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

mhils previously approved these changes Feb 13, 2023

Member

@mhils mhils left a comment • Loading

Choose a reason for hiding this comment

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

Thanks! :)

This looks good % formatting stuff flagged by CI.

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.. ?

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.

mhils previously approved these changes Feb 13, 2023

@mhils fantastic, thank you very much!

jlaine added a commit to jlaine/aiortc that referenced this pull request

Apr 1, 2023