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 secure renegotiation flag from TLS connection #1193

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

Conversation

ndmalc
Copy link

@ndmalc ndmalc commented Mar 21, 2023

Hello,

This is a pull request to add support for the SSL_get_secure_renegotiation_support function from OpenSSL.

Some simple test is added.

Best regards

@reaperhulk
Copy link
Member

These bindings have actually been removed from cryptography and will not be present in the next release. The CI should have caught this, but there's a bug in the cryptographyMain job that causes it to re-install the latest release (rather than using main) due to the version bounds in pyOpenSSL now.

I'm not entirely clear on the value of this though. RFC 5746 was published in early 2010. Is insecure renegotiation prevalent at this point?

@mhils
Copy link
Member

mhils commented Mar 22, 2023

Anecdotally we've had some people request this for mitmproxy (mitmproxy/mitmproxy#5449 and mitmproxy/mitmproxy#5698), but I don't have hard numbers. Do we know which pyOpenSSL/cryptography release disabled insecure renegotiation? CHANGELOG.rst isn't conclusive unfortunately.

@reaperhulk
Copy link
Member

OpenSSL would have disabled it, we never set an explicit value. It may or may not be listed in their changelog though…

@mhils
Copy link
Member

mhils commented Mar 22, 2023

https://www.openssl.org/news/cl30.txt says it's OpenSSL 3.0, so that means cryptography 37 (2022-04-26).

@reaperhulk
Copy link
Member

Those mitmproxy issues suggest it’d be helpful to have the insecure renegotiation flags from the other PR available. I’m not a huge fan but being able to communicate with old devices that can’t be updated makes sad sense. If you think we should be landing one or both of these PRs I can be okay with that. We’ll need to re-add the bindings to cryptography as well as fixing the cryptographyMain CI issue though.

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

Successfully merging this pull request may close these issues.

3 participants