bpo-33062: Add SSL renegotiation and key update by fantix · Pull Request #8620 · python/cpython · GitHub

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

TODOs:

News
Test renegotiation
Test key update
Test errors

https://bugs.python.org/issue33062

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

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.

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

OK! Thanks for the comments.

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

Hi @fantix

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

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

@tiran a gentle friendly ping :)

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

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.

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

@tiran any other comments please?

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!

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

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

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.

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.

@fantix, please resolve the merge conflict. Thanks!

@csabella oh right, yes will do!

@fantix, please resolve the merge conflict. Thanks!

@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!

This PR is stale because it has been open for 30 days with no activity.

closing as not planned.

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

github-actions Bot added the stale Stale PR or inactive for long period of time. label Sep 21, 2024

gpshead closed this Mar 22, 2025

vitaly-krugl mannequin mentioned this pull request Apr 10, 2022

ssl_renegotiate() doesn't seem to be exposed #77243

Closed

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

                   def verify_client_post_handshake(self):
                       return self._sslobj.verify_client_post_handshake()
+                  def key_update(self, updatetype):

bpo-33062: Add SSL renegotiation and key update by fantix · Pull Request #8620 · python/cpython · GitHub

Uh oh!

Uh oh!

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

bpo-33062: Add SSL renegotiation and key update#8620fantix wants to merge 1 commit intopython:mainpython/cpython:mainfrom fantix:bpo-33062/ssl-renegotiationfantix/cpython:bpo-33062/ssl-renegotiationCopy head branch name to clipboard

Conversation

fantix commented Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 Aug 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1st1 commented Aug 2, 2018

Uh oh!

tiran commented Aug 9, 2018

Uh oh!

fantix commented Aug 9, 2018

Uh oh!

1st1 commented Oct 1, 2018

Uh oh!

fantix commented Oct 1, 2018

Uh oh!

tiran commented Oct 2, 2018

Uh oh!

fantix commented Oct 3, 2018

Uh oh!

fantix commented Oct 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tiran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Oct 19, 2018

Uh oh!

csabella commented May 31, 2019

Uh oh!

fantix commented May 31, 2019

Uh oh!

csabella commented Jan 25, 2020

Uh oh!

jdevries3133 commented Jul 23, 2021 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 21, 2024

Uh oh!

gpshead commented Mar 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bpo-33062: Add SSL renegotiation and key update#8620
fantix wants to merge 1 commit into
python:mainfrom
fantix:bpo-33062/ssl-renegotiation

fantix commented Aug 2, 2018 •
edited
Loading

1st1 Aug 3, 2018 •
edited
Loading

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