gh-63284: Add support for TLS-PSK (pre-shared key) to the ssl module

[grantramsay]

Add support for TLS-PSK (pre-shared key) to the ssl module (plus documentation and unit-tests).

The referenced issue (#63284) is ~10 years old, but the information is still valid.

I've tested this by:

• connecting python TLS-PSK client/server (as in the unit-tests)
• connecting to openssl s_client/s_server from python:
  • openssl s_server -accept localhost:12345 -psk deadbeef -cipher PSK -verify_return_error -nocert
  • openssl s_client -connect localhost:12345 -psk deadbeef -cipher PSK -verify_return_error

Things I'm still uncertain on:

• It's not yet working with TLS 1.3 (I'm currently looking into this). [Fixed]
• Do I need to call PyErr_WriteUnraisable() if there's a python exception during a C callback? [Fixed]

This is my first python contribution.
I have probably got a few things wrong, but I'm happy to fix them up.

Thanks!

• Issue: No way to use TLS-PSK from python ssl #63284

[cpython-cla-bot]

All commit authors signed the Contributor License Agreement.

[grantramsay]

I figured out why it wasn't working with TLS 1.3.

The error on the client-side is:

ssl.SSLError: [SSL: ATTEMPT_TO_REUSE_SESSION_IN_DIFFERENT_CONTEXT] attempt to reuse session in different context (_ssl.c:996)

When initialising the SSLContext there is a call to SSL_CTX_set_session_id_context():

#define SID_CTX "Python"
    SSL_CTX_set_session_id_context(self->ctx, (const unsigned char *) SID_CTX,
                                   sizeof(SID_CTX));
#undef SID_CTX

The openssl man pages state this is a "server side only" operation:
https://www.openssl.org/docs/man1.0.2/man3/SSL_CTX_set_session_id_context.html

SSL_CTX_set_session_id_context, SSL_set_session_id_context - set context within which session can be reused (server side only)

The session id context becomes part of the session. The session id context is set by the SSL/TLS server. The SSL_CTX_set_session_id_context() and SSL_set_session_id_context() functions are therefore only useful on the server side.

Calling this on a client-side socket seems to result in unexpected behavior, where the client thinks it's trying to resume a non-existent session.

It looks like others have run into the same issue: maovidal/paho_sslpsk2_demo#2 (comment)

Disabling the call to SSL_CTX_set_session_id_context() for client-side SSLContext creation fixes it.
I've added another commit with this fix

[arhadthedev]

@jackjansen, @dstufft, @alex (as ssl module experts)

[grantramsay]

Modified the documentation for TLS 1.3.
I initially assumed it was a quirk of the OpenSSL TLS 1.3 implementation that meant "client-identity" had to be a non-empty string.

However this ticket explains that it is a change in TLS 1.3: openssl/openssl#8894 (comment)

TLSv1.3 explicitly states that an identity must be at least 1 byte long. But in TLSv1.2 an identity was allowed to have a zero length.

[grantramsay]

Hey just giving this a bump

[bedevere-bot]

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.

[grantramsay]

I have made the requested changes; please review again

[grantramsay]

I have made the requested changes; please review again

[bedevere-bot]

Thanks for making the requested changes!

@gpshead: please review the changes made to this pull request.

[bruno-at-bareos]

@gpshead is there anything that still need to be done to see this PR moving forward?

[grantramsay]

Hi @gpshead, it has been a while but I'm still interested in getting this merged.
Do you have any further review comments/suggestions?

I have made the requested changes; please review again

[doronz88]

I'm also very interested in this feature. Feel free to tag me if you need any help

[gpshead]

Thanks for the contributon!

FWIW the timestamp on the NEWS.d/next/ filename is irrelevant, it is only being used as a uniqueness collision avoidance mechanism. All of those get merged into one file during releases.

[grantramsay]

Awesome!! Thanks for taking the time to review!