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

Expose some DTLS-related features #1026

Merged
merged 16 commits into from
Nov 2, 2021
Merged

Conversation

njsmith
Copy link
Contributor

@njsmith njsmith commented Jun 26, 2021

This is enough to make it at least possible to set up a DTLS association and exchange encrypted data, though a high quality implementation still requires a ton of scaffolding code (see python-trio/trio#2010).

Depends on pyca/cryptography#6138

@njsmith njsmith marked this pull request as draft June 28, 2021 10:31
@njsmith njsmith force-pushed the dtls-wrappers branch 2 times, most recently from 71811ba to ce44736 Compare June 30, 2021 19:22
@njsmith njsmith marked this pull request as ready for review June 30, 2021 19:59
@njsmith
Copy link
Contributor Author

njsmith commented Jun 30, 2021

For now I'm just skipping the new DTLS test when running against older cryptography. I guess the actual plan would be to wait for cryptography to release, bump the minimum cryptography version to the new release, and then drop the skipif? But everything else is good to go AFAICT.

@reaperhulk
Copy link
Member

Would you mind shifting the CI changes to a separate PR? We can merge those immediately. Also, is it a hard requirement that we raise our version minimum or can we feature detect here in a sane fashion? (It's okay if the answer to that is no)

Also looks like we have a variety of missing lines of coverage, although that may just be codecov being stupid, I haven't looked.

@njsmith
Copy link
Contributor Author

njsmith commented Sep 30, 2021

Also, is it a hard requirement that we raise our version minimum or can we feature detect here in a sane fashion? (It's okay if the answer to that is no)

Depends on what kind of feature detection you want. Having DTLS_METHOD and friends automagically hide themselves from the SSL namespace seems like maybe more trouble than any of us are motivated to deal with. OTOH the tests still pass just fine with the older cryptography, as long as you skip that one DTLS test.

@njsmith
Copy link
Contributor Author

njsmith commented Sep 30, 2021

Also AFAICT all the missed coverage lines are to handle obscure things that can allegedly happen with libressl/old openssl/etc., so the issue is that pyopenssl doesn't test against these in CI. Do we care? I think the worst case is that people using exotic openssl-like libraries might get the wrong exception.

@njsmith njsmith mentioned this pull request Sep 30, 2021
@njsmith njsmith force-pushed the dtls-wrappers branch 3 times, most recently from b7119f5 to 02b4b6c Compare October 4, 2021 08:25
@njsmith
Copy link
Contributor Author

njsmith commented Oct 4, 2021

The unrelated CI fixes/py 2 dropping/etc. have now all been merged, and I rebased this on top of them. So now this PR is just the DTLS changes, and I guess r4r.

src/OpenSSL/SSL.py Outdated Show resolved Hide resolved
tests/test_ssl.py Outdated Show resolved Hide resolved
src/OpenSSL/SSL.py Show resolved Hide resolved
@reaperhulk
Copy link
Member

black is angry

@njsmith
Copy link
Contributor Author

njsmith commented Oct 31, 2021 via email

Copy link
Member

@reaperhulk reaperhulk left a comment

Choose a reason for hiding this comment

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

A bit more missing coverage than I'd prefer, but most of that is fixable if we add a 1.1.0 builder to our CI...which I am not volunteering for.

@reaperhulk reaperhulk merged commit e84e7b5 into pyca:main Nov 2, 2021
@njsmith njsmith deleted the dtls-wrappers branch November 2, 2021 07:11
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants