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

DTLS support #2047

Merged
merged 49 commits into from
Jul 27, 2022
Merged

DTLS support #2047

merged 49 commits into from
Jul 27, 2022

Conversation

njsmith
Copy link
Member

@njsmith njsmith commented Jun 26, 2021

Here's an initial draft of #2010. It's not done -- in particular, needs a ton more tests and docs, as well as some strategy to avoid adding a hard dependency on pyopenssl -- but the core logic is all there in some form.

It depends on upstream changes pyca/cryptography#6138 and pyca/pyopenssl#1026, so the tests are all going to fail for now.

@codecov
Copy link

codecov bot commented Jun 26, 2021

Codecov Report

Merging #2047 (93851cf) into master (585a783) will decrease coverage by 0.54%.
The diff coverage is 92.65%.

@@            Coverage Diff             @@
##           master    #2047      +/-   ##
==========================================
- Coverage   99.56%   99.02%   -0.55%     
==========================================
  Files         115      119       +4     
  Lines       14856    16165    +1309     
  Branches     2837     3101     +264     
==========================================
+ Hits        14792    16007    +1215     
- Misses         41      111      +70     
- Partials       23       47      +24     
Impacted Files Coverage Δ
trio/testing/_fake_net.py 68.19% <68.19%> (ø)
trio/_dtls.py 96.92% <96.92%> (ø)
trio/tests/test_dtls.py 99.79% <99.79%> (ø)
trio/__init__.py 100.00% <100.00%> (ø)
trio/_socket.py 100.00% <100.00%> (ø)
trio/tests/test_fakenet.py 100.00% <100.00%> (ø)
trio/tests/test_socket.py 100.00% <100.00%> (ø)
trio/tests/test_ssl.py 99.86% <0.00%> (+0.55%) ⬆️
trio/_highlevel_ssl_helpers.py 100.00% <0.00%> (+11.76%) ⬆️

@njsmith njsmith marked this pull request as draft June 28, 2021 10:31
@njsmith
Copy link
Member Author

njsmith commented Sep 8, 2021

This is still blocked waiting on cryptography+pyopenssl to release, but I believe everything on our end is done, so if anyone is excited about reviewing, go ahead! Docs are viewable here: https://trio--2047.org.readthedocs.build/en/2047/reference-io.html#datagram-tls-support

Aside from the DTLS support itself, this also adds a MVP of a mock network layer for testing (#170). It's not exposed publicly yet, because so far it only supports UDP, the mechanism for manipulating packets in flight is awkward, and I just generally didn't want to make finalizing that API a blocker for this PR. But the DTLS tests use it extensively, and using it felt fabulous, so I'm excited.

This is a bit of a weird situation as far as reviewing goes, because this is a huge complex piece of work, and I got paid for it but y'all didn't. So like... any review is much appreciated, but please don't feel obliged to spend days on it or anything! It is not the end of the world if some of this code doesn't get intense scrutiny – it has an extremely thorough test suite and we can always fix stuff later.

Thanks again to [redacted] for sponsoring this work.

This works around a problem on windows where recvfrom on an unbound
socket errors out.
Noticed while testing on Ubuntu 18.04's weird old OpenSSL that this
test was getting stuck in an infinite loop, even though it passed with
more recent OpenSSL. I'm not sure why it was different, exactly, but
on closer examination, writing the test this way makes more sense
anyway, and now the tests pass on Ubuntu 18.04 too.
@njsmith
Copy link
Member Author

njsmith commented Jan 29, 2022

Update: pyopenssl has pushed their release, so this no longer needs hacky workarounds to test against the dev version. And I think I tracked down the last few test failures that were happening on pypy? Including finding a fun pypy bug: https://foss.heptapod.net/pypy/pypy/-/issues/3656

Let's see what CI says.

@gesslerpd
Copy link
Member

Seems like test coverage has gone down by a small amount causing the CI check failures.

@njsmith do you think this PR is ready for review/merge?

@pquentin
Copy link
Member

pquentin commented Apr 4, 2022

The coverage does go down but it's not blocking in my opinion.

@gesslerpd gesslerpd marked this pull request as ready for review April 26, 2022 15:32
@gesslerpd gesslerpd merged commit bcb6663 into python-trio:master Jul 27, 2022
@gesslerpd gesslerpd mentioned this pull request Jul 27, 2022
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