Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
gpshead merged 22 commits intopython:mainfromgrantramsay:fix-issue-63284
Nov 27, 2023

Conversation

@grantramsay
Copy link
Contributor

@grantramsaygrantramsay commentedApr 2, 2023•
edited
Loading

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

ddurham2, holesch, arhadthedev, and maovidal reacted with thumbs up emoji
@ghost
Copy link

ghost commentedApr 2, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@arhadthedevarhadthedev added stdlibStandard Library Python modules in the Lib/ directory topic-SSL labelsApr 2, 2023
@grantramsaygrantramsayforce-pushed thefix-issue-63284 branch 2 times, most recently from7fab649 toad6c893CompareApril 3, 2023 04:04
@grantramsay
Copy link
ContributorAuthor

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 toSSL_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 toSSL_CTX_set_session_id_context() for client-side SSLContext creation fixes it.
I've added another commit with this fix

@arhadthedev
Copy link
Member

@jackjansen,@dstufft,@alex (as ssl moduleexperts)

SSL_CTX_set_session_id_context() is a server-side only operation.Using this on the client-side is causing authentication errors
@grantramsay
Copy link
ContributorAuthor

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
Copy link
ContributorAuthor

Hey just giving this a bump

@bedevere-bot
Copy link

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 phraseI 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.

@gpsheadgpshead self-assigned thisMay 30, 2023
@gpsheadgpshead added the type-featureA feature request or enhancement labelMay 30, 2023
RFC4279 states these are UTF-8.Add unit test using non-ASCII chars
The PR has missed the 3.12 merge window
@grantramsay
Copy link
ContributorAuthor

I have made the requested changes; please review again

@grantramsay
Copy link
ContributorAuthor

I have made the requested changes; please review again

@bedevere-bot
Copy link

Thanks for making the requested changes!

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

@bruno-at-bareos
Copy link

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

@grantramsay
Copy link
ContributorAuthor

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

bruno-at-bareos, tuxmaster5000, SamuelBoerlin, doronz88, and holesch reacted with thumbs up emoji

@doronz88
Copy link

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

@gpshead
Copy link
Member

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 reacted with hooray emoji

@gpsheadgpshead merged commite954ac7 intopython:mainNov 27, 2023
@grantramsay
Copy link
ContributorAuthor

Awesome!! Thanks for taking the time to review!

aisk pushed a commit to aisk/cpython that referenced this pull requestFeb 11, 2024
…odule (python#103181)Add support for TLS-PSK (pre-shared key) to the ssl module.---------Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>Co-authored-by: Gregory P. Smith <greg@krypto.org>
Glyphack pushed a commit to Glyphack/cpython that referenced this pull requestSep 2, 2024
…odule (python#103181)Add support for TLS-PSK (pre-shared key) to the ssl module.---------Co-authored-by: Oleg Iarygin <oleg@arhadthedev.net>Co-authored-by: Gregory P. Smith <greg@krypto.org>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@gpsheadgpsheadgpshead approved these changes

@dstufftdstufftAwaiting requested review from dstufft

@tirantiranAwaiting requested review from tiran

+1 more reviewer

@gramsay0gramsay0gramsay0 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@gpsheadgpshead

Labels

stdlibStandard Library Python modules in the Lib/ directorytopic-SSLtype-featureA feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@grantramsay@arhadthedev@bedevere-bot@bruno-at-bareos@doronz88@gpshead@gramsay0
[8]ページ先頭
©2009-2025 Movatter.jp