Added TLS_method bindings for OpenSSL and LibreSSL #5483

th3b0x · 2020-10-06T19:39:10Z

Added bindings for TLS_method TLS_client_method and TLS_server_method with support for LibreSSL and OpenSSL
Added tests for TLS_method TLS_client_method and TLS_server_method with support for LibreSSL and OpenSSL

…he bindings with support for LibreSSL and OpenSSL Updated tests to cover new interfaces

th3b0x · 2020-10-06T19:51:09Z

NOTE: Build system cancelled 2 checks, so I need a contributor to "kick" those checks back on for the all-clear

Got distracted and had to make a new PR, because I borked my branch too much for a clean Rebase.

Current PR in relation to the TLS_method* interfaces is now #5483

I will then be able to proceed to adding the new SSL_CTX option bindings, as requested by @richvdh

Kindly requesting a review @reaperhulk (this successfully passed all checks in #5428 however, the commit log became very very mangled due to some careless mistakes I had made, so I have opted to sidestep Rebasing and simply create a new Branch and PR)

….0.2u users (resolving historical discrepancies within my branch; custodial merge-commit-push)

reaperhulk · 2020-10-07T03:00:32Z

tests/hazmat/bindings/test_openssl.py

+        Binding.lib.Cryptography_HAS_TLS_METHOD == 0,
+        reason="TLS_method requires OpenSSL >= 1.1.0",
+    )
+    def test_tls_ctx_options(self):


Duplicating these tests just to pass TLS_method isn't necessary.

Noted in the commit update (currently building). Thanks for catching that.

LibreSSL is a derivative of OpenSSL. OpenSSL maps SSLv23_method to TLS_method, thus behavior will always be identical between both sets of methods given the tests initially provided and the constants currently available.

reaperhulk · 2020-10-07T13:55:55Z

src/_cffi_src/openssl/ssl.py

+
+#if CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 && !CRYPTOGRAPHY_IS_LIBRESSL
+static const long Cryptography_HAS_TLS_METHOD = 0;
+const SSL_METHOD* (*TLS_method)(void) = NULL;


Why isn't this just #define TLS_method SSLv23_method and then TLS_method is available in all cases?

Lack of attention on my part, I failed to pull this in from the prior PR. Excellent catch and suggestion, I've committed and pushed. Thank you, very much.

reaperhulk · 2020-10-07T16:16:49Z

src/_cffi_src/openssl/ssl.py

@@ -755,4 +762,13 @@
 #else
 static const long Cryptography_HAS_TLSv1_3 = 1;
 #endif
+
+#if CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 && !CRYPTOGRAPHY_IS_LIBRESSL
+static const long Cryptography_HAS_TLS_METHOD = 0;


When defining TLS_method to be SSLv23_method you no longer need to conditionally remove the function. It will always be available. So the else block, the HAS_TLS_METHOD variable, and the cryptography_has_tls_method func in _conditional.py are not required.

I'd prefer this to be done using #define rather than assigning func ptrs

@reaperhulk Thank you for the very thorough and useful review (lots of great stuff that would've slipped through the cracks due to my inexperience with CFFI and lack of attention). I'm reviewing one last issue here related to how this functionally modifies program behavior. I suspect I'll have my final item ready for submission today or tomorrow. Hold tight.

(This update caused paramiko to fail its downstream build in Travis CI)

@reaperhulk

Initially, I was in agreement on this, but now I am thinking that it might be unwise to implement the TLS_method interfaces in this manner, because it would allow a developer to invoke TLS_method on OpenSSL implementations that might not support any version of TLS.

Additionally, I am beginning to think that I might need to actually implement more tests in this PR before it's ready due to the kinds of problems described in this Code Review for Google's BoringSSL: https://boringssl-review.googlesource.com/c/boringssl/+/8513/
I happen to know that the problems described by @davidben do happen to persist in OpenSSL 1.1.1g (because I performed an inspection earlier this year to identify potential avenues of exploitation related to the problems described in his BoringSSL commit comment).

As a specific test, I think it might be important to add a test related to whether SSL_OP_NO* causes "surprising" (dangerous) behavior as @davidben describes. I am personally wondering if we should specifically retain conditional removal, but replace cryptography_has_tls_method with a flag similar to CRYPTOGRAPHY_OPENSSL_LESS_THAN_110 that would indicate whether or not the library in use is a version of OpenSSL that retains the stupidly dangerous behavior of forcing the use of downgraded SSL/TLS methods when a user provides an SSL_OP bitmask that creates discontinuities in the SSL/TLS version ranges supported. (I would provide code to demonstrate what I'm talking about, but I think @davidben or yourself might already know what I mean).

Note: I have the jupyter notebook describing the issue on Github, but it's private and the code demonstrating the discontinuous range issue "got lost" because I apparently forgot to push it before deleting the folder. Not sure what happened actually, because (upon reinspection) it looks like I might have lost a little more than just the demo code.

Err, what is my location?

It's true I live in Washington, DC but I don't see the relevance?

TLS_method is intended as a drop-in replacement for SSLv23_method and we should treat it as such...

cryptography's bindings layer cannot enforce sanity in OpenSSL's APIs. That way lies madness. We can only expose their API and attempt use it safely/sanely within our own code. We don't even consider our bindings public surface area any more (pyOpenSSL is a special case that we ensure does not break).

I'm opposed to trying to test for behavioral quirks (even dangerous ones) since it almost definitionally can't be useful to this project. That effort is much better expended upstream in conversations with OpenSSL about how to improve/replace dangerous APIs.

Yeah there's no difference in behavior between SSLv23_method and TLS_method. The version disabling flags do indeed behave a little oddly but I don't think it's related to this. The less surprising API is SSL_set_min/max_proto_version if you all want to switch to those, but they only exist starting 1.1.0.

@alex I couldn't replicate the paramiko build failure.

note, I'm going to let this one sit for a while in case anyone is maintaining versions of OpenSSL that don't implement the normal behavior.

Meanwhile, I might try to push those interfaces from issue #5379

Simplified binding statement for TLS_method inclusion Removed Conditional Library build hooks Updated tests to discard use of Cryptography_HAS_TLS_METHOD

reaperhulk · 2020-10-08T02:06:07Z

src/_cffi_src/openssl/ssl.py

@@ -501,6 +506,7 @@
 """

 CUSTOMIZATIONS = """
+


Extraneous newline. Otherwise this LGTM now.

th3b0x · 2020-10-10T10:27:46Z

@reaperhulk keep it pending as long as you want (assuming it doesn't fail on Travis CI again)

th3b0x · 2020-10-14T01:10:37Z

Edit: detected a merge conflict, so I'm simply pulling the work into the active Pull Request to avoid unnecessary extra work.

@reaperhulk I'm going to move the rest of the fixes for #5379 into a separate PR.

Also, the Codecov check is failing on code unrelated to this PR, so I'm not sure what I ought to be doing with that.

From pyca#5379 : Added bindings for SSL / CTX interfaces to SET min and max protocol versions (added in OpenSSL 1.1.0). Added bindings for SSL / CTX interfaces to GET min and max protocol versions (added in OpenSSL 1.1.1). Added conditional build variables to allow compilation on systems not offering these interfaces via the compiled library.

From pyca#5379 : Added bindings for SSL / CTX interfaces to SET min and max protocol versions (added in OpenSSL 1.1.0). Added bindings for SSL / CTX interfaces to GET min and max protocol versions (added in OpenSSL 1.1.1). Added conditional build variables to allow compilation on systems not offering these interfaces via the compiled library. Merge branch 'Min_Proto_Bindings' of github.com:th3b0x/cryptography into th3b0x-TLS_method-patch

neheb · 2020-11-28T04:02:56Z

This looks like a better version of the one I have here: #4920

fixes pyca#5379 closes pyca#5483

fixes #5379 closes #5483

Added TLS_method TLS_client_method and TLS_server_method hooks into t…

07b8132

…he bindings with support for LibreSSL and OpenSSL Updated tests to cover new interfaces

th3b0x mentioned this pull request Oct 6, 2020

FFI interface to SSL_set_min_proto_version etc #5379

Closed

Merging so that Alex doesn't break me on the wheel like the OpenSSL 1…

dc9cde7

….0.2u users (resolving historical discrepancies within my branch; custodial merge-commit-push)

th3b0x force-pushed the th3b0x-TLS_method-patch branch from e4fd4f8 to dc9cde7 Compare October 6, 2020 22:21

reaperhulk reviewed Oct 7, 2020

View reviewed changes

Removed redundant tests for TLS_method bindings

a5f30c8

LibreSSL is a derivative of OpenSSL. OpenSSL maps SSLv23_method to TLS_method, thus behavior will always be identical between both sets of methods given the tests initially provided and the constants currently available.

reaperhulk reviewed Oct 7, 2020

View reviewed changes

Updated binding to backport TLS_method

f9b9e05

reaperhulk reviewed Oct 7, 2020

View reviewed changes

Code Review: simplify binding (per reaperhulk)

1814d4f

Simplified binding statement for TLS_method inclusion Removed Conditional Library build hooks Updated tests to discard use of Cryptography_HAS_TLS_METHOD

reaperhulk reviewed Oct 8, 2020

View reviewed changes

src/_cffi_src/openssl/ssl.py Outdated

@@ -501,6 +506,7 @@

"""

CUSTOMIZATIONS = """

Copy link

Member

reaperhulk Oct 8, 2020

Choose a reason for hiding this comment

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

Extraneous newline. Otherwise this LGTM now.

th3b0x marked this pull request as draft October 8, 2020 23:53

Removed extraneous newline

f1a5433

th3b0x marked this pull request as ready for review October 10, 2020 10:27

th3b0x marked this pull request as draft October 10, 2020 18:59

Jswinney117 mentioned this pull request Oct 13, 2020

Hi github/site-policy#307

Closed

th3b0x marked this pull request as ready for review October 14, 2020 01:02

th3b0x added 2 commits October 24, 2020 10:53

th3b0x marked this pull request as draft October 24, 2020 16:25

alex added a commit to alex/cryptography that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs

dff882a

fixes pyca#5379 closes pyca#5483

alex mentioned this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs #5595

Merged

alex added a commit to alex/cryptography that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs

8eaed59

fixes pyca#5379 closes pyca#5483

alex added a commit to alex/cryptography that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs

fd3e6c0

fixes pyca#5379 closes pyca#5483

alex added a commit to alex/cryptography that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs

ffc4f9b

fixes pyca#5379 closes pyca#5483

alex added a commit to alex/cryptography that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs

12da376

fixes pyca#5379 closes pyca#5483

reaperhulk closed this in #5595 Dec 1, 2020

reaperhulk pushed a commit that referenced this pull request Dec 1, 2020

Added tls bindings for new OpenSSL APIs (#5595)

a209669

fixes #5379 closes #5483

th3b0x mentioned this pull request Dec 2, 2020

Added missing tls bindings for new OpenSSL APIs #5599

Closed

github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added TLS_method bindings for OpenSSL and LibreSSL #5483

Added TLS_method bindings for OpenSSL and LibreSSL #5483

th3b0x commented Oct 6, 2020

th3b0x commented Oct 6, 2020 •

edited

Loading

reaperhulk Oct 7, 2020

th3b0x Oct 7, 2020

reaperhulk Oct 7, 2020

th3b0x Oct 7, 2020

reaperhulk Oct 7, 2020

th3b0x Oct 8, 2020 •

edited

Loading

th3b0x Oct 8, 2020 •

edited

Loading

alex Oct 9, 2020

alex Oct 9, 2020

reaperhulk Oct 9, 2020

davidben Oct 9, 2020

th3b0x Oct 10, 2020

reaperhulk Oct 8, 2020

th3b0x commented Oct 10, 2020

th3b0x commented Oct 14, 2020 •

edited

Loading

neheb commented Nov 28, 2020

Added TLS_method bindings for OpenSSL and LibreSSL #5483

Added TLS_method bindings for OpenSSL and LibreSSL #5483

Conversation

th3b0x commented Oct 6, 2020

th3b0x commented Oct 6, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3b0x Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

th3b0x Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

th3b0x commented Oct 10, 2020

th3b0x commented Oct 14, 2020 • edited Loading

neheb commented Nov 28, 2020

th3b0x commented Oct 6, 2020 •

edited

Loading

th3b0x Oct 8, 2020 •

edited

Loading

th3b0x Oct 8, 2020 •

edited

Loading

th3b0x commented Oct 14, 2020 •

edited

Loading