bpo-33062: Add SSL renegotiation and key update #8620

fantix · 2018-08-02T08:51:29Z

@1st1 This PR is for testing new asyncio/sslproto.py.

TODOs:

News
Test renegotiation
Test key update
Test errors

https://bugs.python.org/issue33062

1st1 · 2018-08-02T17:43:25Z

Lib/ssl.py

-class SSLObject:
+class _KeyUpdateMixin:
+    if HAS_TLSv1_3:
+        def key_update(self, updatetype):


Do we want an else branch with these methods raising a NotImplementedError?

1st1 · 2018-08-02T17:45:51Z

Modules/_ssl.c

@@ -1050,6 +1050,107 @@ _ssl__SSLSocket_do_handshake_impl(PySSLSocket *self)
    return NULL;
 }

+#if defined(TLS1_3_VERSION) && !defined(OPENSSL_NO_TLS1_3)


I think you need to provide alternative implementations of these functions raising NotImplementedError for the #else branch (otherwise this might not compile at all).

Make sense, will do.

Actually the #if should be inside the function, otherwise argument clinic will have hard time figuring out what you are doing.

Yeah it'll be safer that way, will do (currently it seems clinic is clever enough to copy the ifdef to .h file, and make it an empty METHODDEF macro if false, so that the same ifdef won't be necessary when using the METHODDEF).

currently it seems clinic is clever enough to copy the ifdef to .h file, and make it an empty METHODDEF macro if false, so that the same ifdef won't be necessary when using the METHODDEF

That's cool, I didn't know it's that smart :) Then feel free to keep your code as is.

1st1 · 2018-08-02T17:46:33Z

Modules/_ssl.c

+    _PySSL_UPDATE_ERRNO_IF(ret == 0, self, ret);
+    PySSL_END_ALLOW_THREADS
+
+    if (PyErr_CheckSignals())


Use { even for single statement ifs (as per updated PEP 7)

OK! Thanks for the comments.

1st1 · 2018-08-02T17:48:18Z

@tiran Christian, this is a WIP, but would be great to get a green light from you in general.

tiran · 2018-08-09T10:16:49Z

Hi @fantix

thanks for your patch! It's a promising start. I'm currently travelling and will look into your PR next week.

fantix · 2018-08-09T10:19:16Z

Thanks @tiran Christian! In the meanwhile I'll get the remaining TODOs done.

1st1 · 2018-10-01T16:08:28Z

@tiran a gentle friendly ping :)

fantix · 2018-10-01T16:31:21Z

Thanks! I’m working on a rebase and update here, shall commit in 18 hours.

tiran · 2018-10-02T16:16:44Z

I'm not a fan of mixin classes... The functions don't do much work, so it should be easy to move the common code to C.

fantix · 2018-10-03T12:17:47Z

Ah yes, that looks a bit duplicate, will fix, thanks!

fantix · 2018-10-19T07:56:05Z

@tiran any other comments please?

asvetlov · 2018-10-19T09:43:29Z

Lib/ssl.py

+Raises NotImplementedError if the TLS implementation doesn't support
+TLS 1.3.)
+
+:param updatetype: KeyUpdateTypes


In Python we don't use sphinx markup in docstrings, documentation is not authogenerated but written manually.

Ah, thanks for this one! I'll update docs too.

This approach looks strange and uncommon to me. I opened #9972, which I believe is a better approach to handle common doc strings.

Yes it is! I'll rebase up to #9972 once it is merged. Thanks!

tiran

Please document the new methods and constants in the module documentation. The doc strings are less relevant and should be shorter. You can also inlcude a whatsnew

tiran · 2018-10-19T17:51:51Z

Lib/ssl.py

+Raises NotImplementedError if the TLS implementation doesn't support
+TLS 1.3.)
+
+:param updatetype: KeyUpdateTypes


This approach looks strange and uncommon to me. I opened #9972, which I believe is a better approach to handle common doc strings.

tiran · 2018-10-19T17:52:50Z

Lib/ssl.py

@@ -780,6 +785,23 @@ def version(self):
    def verify_client_post_handshake(self):
        return self._sslobj.verify_client_post_handshake()

+    def key_update(self, updatetype):


With TLS 1.3, you can also force an immediate rekey. From the documentation, https://www.openssl.org/docs/man1.1.1/man3/SSL_key_update.html

Alternatively SSL_do_handshake() can be called to force the update to take place immediately.

Yeah I also found that during writing docs,

Alternatively do_handshake() can be called to force the update to take place immediately.

Do you think it a better approach to wrap OpenSSL API into high level API like this:

def key_update(self, updatetype, *, deferred=False): self._sslobj.key_update(updatetype) if not deferred: self._sslobj.do_handshake()

Or stay with OpenSSL-style API and document the details? Now I prefer the high-level one, leaving the low-level API with the _ssl module.

bedevere-bot · 2018-10-19T17:54:42Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

csabella · 2019-05-31T11:14:02Z

@fantix, please resolve the merge conflict. Thanks!

fantix · 2019-05-31T19:24:03Z

@csabella oh right, yes will do!

csabella · 2020-01-25T13:06:54Z

@fantix, please resolve the merge conflict. Thanks!

jdevries3133 · 2021-07-23T03:06:14Z

@csabella @fantix I went ahead and resolved the merge conflicts. The conflicts were only in auto-generated files, so I just regenerated them and also fixed one test that had broken in the interim since these changes were made. You can see the diff here, although I didn't open a new PR; I'm not sure whether I should, or what I should do next, for that matter:

main...jdevries3133:[bpo-33063](https://bugs.python.org/issue33063)-ssl-renegotiation

I did have a few notes about how I fixed a broken test, including a question:

In one of the new tests, there was a check that the OpenSSL version is 1.1.
I believe that python must be built againt OpenSSL 1.1 now, right? I removed
the check because it was using a variable which is now undefined. Presumably
it was exported from SSL when the test was originally written.

In any case, if we do need to check the version, I can just use
ssl.OPENSSL_VERSION_INFO, but if I understand correctly, the check is not
necessary. If that's not right, just let me know!

the-knights-who-say-ni added the CLA signed label Aug 2, 2018

bedevere-bot added the awaiting review label Aug 2, 2018

1st1 reviewed Aug 2, 2018

View reviewed changes

1st1 self-assigned this Aug 2, 2018

1st1 requested a review from tiran August 2, 2018 17:47

fantix force-pushed the bpo-33062/ssl-renegotiation branch from bbf733e to 45a906a Compare August 13, 2018 10:51

fantix changed the title ~~bpo-33062: Add SSL renegotiation and key update (WIP)~~ bpo-33062: Add SSL renegotiation and key update Aug 13, 2018

fantix mentioned this pull request Sep 21, 2018

bpo-34670: Add TLS 1.3 post handshake auth #9460

Merged

fantix force-pushed the bpo-33062/ssl-renegotiation branch from 45a906a to 1f6d47b Compare October 2, 2018 10:31

fantix force-pushed the bpo-33062/ssl-renegotiation branch from 1f6d47b to 6bc07a9 Compare October 12, 2018 07:21

bpo-33062: Added SSL renegotiate and key_update

a953365

fantix force-pushed the bpo-33062/ssl-renegotiation branch from 6bc07a9 to a953365 Compare October 13, 2018 03:13

asvetlov reviewed Oct 19, 2018

View reviewed changes

tiran requested changes Oct 19, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Oct 19, 2018

ezio-melotti removed the CLA signed label Jul 13, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-33062: Add SSL renegotiation and key update #8620

bpo-33062: Add SSL renegotiation and key update #8620

fantix commented Aug 2, 2018 •

edited

1st1 Aug 2, 2018

1st1 Aug 2, 2018

fantix Aug 3, 2018

1st1 Aug 3, 2018

fantix Aug 3, 2018

1st1 Aug 3, 2018 •

edited

1st1 Aug 2, 2018

fantix Aug 3, 2018

1st1 commented Aug 2, 2018

tiran commented Aug 9, 2018

fantix commented Aug 9, 2018

1st1 commented Oct 1, 2018

fantix commented Oct 1, 2018

tiran commented Oct 2, 2018

fantix commented Oct 3, 2018

fantix commented Oct 19, 2018

asvetlov Oct 19, 2018

fantix Oct 19, 2018

tiran Oct 19, 2018

fantix Oct 23, 2018

tiran left a comment

tiran Oct 19, 2018

tiran Oct 19, 2018

fantix Oct 23, 2018

bedevere-bot commented Oct 19, 2018

csabella commented May 31, 2019

fantix commented May 31, 2019

csabella commented Jan 25, 2020

jdevries3133 commented Jul 23, 2021 •

edited by bedevere-bot

bpo-33062: Add SSL renegotiation and key update #8620

Are you sure you want to change the base?

bpo-33062: Add SSL renegotiation and key update #8620

Conversation

fantix commented Aug 2, 2018 • edited

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

1st1 Aug 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1st1 commented Aug 2, 2018

tiran commented Aug 9, 2018

fantix commented Aug 9, 2018

1st1 commented Oct 1, 2018

fantix commented Oct 1, 2018

tiran commented Oct 2, 2018

fantix commented Oct 3, 2018

fantix commented Oct 19, 2018

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

tiran left a 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

bedevere-bot commented Oct 19, 2018

csabella commented May 31, 2019

fantix commented May 31, 2019

csabella commented Jan 25, 2020

jdevries3133 commented Jul 23, 2021 • edited by bedevere-bot

fantix commented Aug 2, 2018 •

edited

1st1 Aug 3, 2018 •

edited

jdevries3133 commented Jul 23, 2021 •

edited by bedevere-bot