bpo-37428: Don't set PHA verify flag on client side by tiran · Pull Request #14421 · python/cpython

tiran · 2019-06-27T11:31:44Z

SSLContext.post_handshake_auth = True no longer sets
SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the
option is documented as ignored for clients, OpenSSL implicitly enables cert
chain validation when the flag is set.

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue37428

ned-deily · 2019-06-28T17:18:23Z

FWIW, I tested a manual cherrypick of this to 3.7 on macOS with OpenSSL 1.1.1c and it didn't cause any new failures in test_ssl and it wasn't failing before in this environment. However, FTR, the following non-fatal traceback consistently shows up both before and after this change and both with and without test -j:

[...]
test_pha_required_nocert (test.test_ssl.TestPostHandshakeAuth) ... Exception in thread Thread-242:
Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/test/test_ssl.py", line 2293, in run
    msg = self.read()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/test/test_ssl.py", line 2270, in read
    return self.sslconn.read()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/ssl.py", line 920, in read
    return self._sslobj.read(len)
ssl.SSLError: [SSL: PEER_DID_NOT_RETURN_A_CERTIFICATE] peer did not return a certificate (_ssl.c:2508)

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/threading.py", line 926, in _bootstrap_inner
    self.run()
  File "/Library/Frameworks/Python.framework/Versions/3.7/lib/python3.7/test/test_ssl.py", line 2379, in run
    raise ssl.SSLError('tlsv13 alert certificate required')
ssl.SSLError: ('tlsv13 alert certificate required',)

ok
test_pha_setter (test.test_ssl.TestPostHandshakeAuth) ... ok
test_get_server_certificate_ipv6 (test.test_ssl.NetworkedTests) ... skipped "Resource 'ipv6.google.com' is not available"
test_timeout_connect_ex (test.test_ssl.NetworkedTests) ... ok

----------------------------------------------------------------------

Ran 153 tests in 76.861s

OK (skipped=10)
test_ssl passed in 1 min 16 sec

ned-deily · 2019-06-29T19:38:22Z

Anything we can do to expedite this? It's currently blocking 3.7.4 final and could shortly block 3.8.0 b2. @alex, would you be able to review this? Thanks!

alex

This looks right to me -- but I confess this code has changed a lot since I last looked at it, so I'd love if another person had eyes on it.

orsenthil · 2019-06-30T18:21:48Z

Lib/test/test_ssl.py

Is bpo37337 correct here? The issue addressed seems to be: https://bugs.python.org/issue37428

This might be more of a question.
I thouhght, the test should have two parts.

Server verifying SSL_VERIFY_POST_HANDSHAKE flag set.

Client which asserts SSL_VERIFY_POST_HANDSHAKE not set.

I could not figure out if the test covers both these scenarios.

I fixed the BPO number in the name of the test.

The new test verifies that client_context.post_handshake_auth does not implicitly enable server cert validation on the client side. There are already several other tests that verify several combinations of PHA.

the test case test_pha_required verifies that SSL_VERIFY_POST_HANDSHAKE on a server context works as expected. The server does request a client during handshake.

SSL_VERIFY_POST_HANDSHAKE must not be set on a client context. This was the original bug. The _set_verify_mode helper prevents this already.

Thanks for answering these, @tiran. LGTM.

SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org>

orsenthil

LGTM. Thank you!

miss-islington · 2019-07-01T06:29:20Z

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.7, 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington · 2019-07-01T06:29:21Z

I'm having trouble backporting to 3.8. Reason: 'Error 110 while writing to socket. Connection timed out.'. Please retry by removing and re-adding the needs backport to 3.8 label.

SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428 (cherry picked from commit f0f5930) Co-authored-by: Christian Heimes <christian@python.org>

bedevere-bot · 2019-07-01T06:29:32Z

GH-14493 is a backport of this pull request to the 3.7 branch.

miss-islington · 2019-07-01T06:29:33Z

Sorry, @tiran, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker f0f5930ac88482ef896283db5be9b8d508d077db 2.7

miss-islington · 2019-07-01T06:30:06Z

Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

miss-islington · 2019-07-01T06:30:08Z

Sorry @tiran, I had trouble checking out the 3.8 backport branch.
Please backport using cherry_picker on command line.
cherry_picker f0f5930ac88482ef896283db5be9b8d508d077db 3.8

…4421) SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428 (cherry picked from commit f0f5930) Co-authored-by: Christian Heimes <christian@python.org>

bedevere-bot · 2019-07-01T06:31:16Z

GH-14494 is a backport of this pull request to the 3.8 branch.

…H-14493) SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428 (cherry picked from commit f0f5930) Co-authored-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428

…4421) (pythonGH-14493) SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428 (cherry picked from commit f0f5930) Co-authored-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428

SSLContext.post_handshake_auth = True no longer sets SSL_VERIFY_POST_HANDSHAKE verify flag for client connections. Although the option is documented as ignored for clients, OpenSSL implicitly enables cert chain validation when the flag is set. Signed-off-by: Christian Heimes <christian@python.org> https://bugs.python.org/issue37428

…pile

tiran added type-bug An unexpected behavior, bug, or error needs backport to 2.7 DO-NOT-MERGE labels Jun 27, 2019

the-knights-who-say-ni added the CLA signed label Jun 27, 2019

bedevere-bot added the awaiting core review label Jun 27, 2019

tiran mentioned this pull request Jun 27, 2019

SSL_VERIFY_POST_HANDSHAKE implicitly enables cert validation openssl/openssl#9259

Closed

tiran force-pushed the bpo-37428-pha-server branch from 7fb91b6 to 217d48b Compare June 28, 2019 13:34

tiran removed the DO-NOT-MERGE label Jun 28, 2019

tiran force-pushed the bpo-37428-pha-server branch from 217d48b to 06c3021 Compare June 28, 2019 13:42

alex approved these changes Jun 29, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Jun 29, 2019

orsenthil reviewed Jun 30, 2019

View reviewed changes

tiran force-pushed the bpo-37428-pha-server branch from 06c3021 to 83d7c57 Compare June 30, 2019 20:16

orsenthil approved these changes Jul 1, 2019

View reviewed changes

tiran added the 🤖 automerge label Jul 1, 2019

miss-islington merged commit f0f5930 into python:master Jul 1, 2019

bedevere-bot removed the awaiting merge label Jul 1, 2019

bedevere-bot removed the needs backport to 3.7 label Jul 1, 2019

miss-islington self-assigned this Jul 1, 2019

tiran added needs backport to 3.8 and removed needs backport to 3.8 labels Jul 1, 2019

bedevere-bot removed the needs backport to 3.8 label Jul 1, 2019

tiran deleted the bpo-37428-pha-server branch July 1, 2019 07:06

rickprice added a commit to ActiveState/cpython that referenced this pull request Jan 26, 2024

CVE-2023-40217 Add in bpo-37428 (pythonGH-14421) so that code can com…

0dace47

…pile

rickprice added a commit to ActiveState/cpython that referenced this pull request Jan 26, 2024

CVE-2023-40217 Add in bpo-37428 (pythonGH-14421) so that code can com…

c2254d2

…pile

rickprice added a commit to ActiveState/cpython that referenced this pull request Jun 26, 2024

CVE-2023-40217 Add in bpo-37428 (pythonGH-14421) so that code can com…

ca55877

…pile

Uh oh!

Conversation

tiran commented Jun 27, 2019 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ned-deily commented Jun 28, 2019

Uh oh!

ned-deily commented Jun 29, 2019

Uh oh!

alex left a comment

Choose a reason for hiding this comment

Uh oh!

orsenthil Jun 30, 2019

Choose a reason for hiding this comment

Uh oh!

tiran Jun 30, 2019

Choose a reason for hiding this comment

Uh oh!

orsenthil Jul 1, 2019

Choose a reason for hiding this comment

Uh oh!

orsenthil left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington commented Jul 1, 2019

Uh oh!

miss-islington commented Jul 1, 2019

Uh oh!

bedevere-bot commented Jul 1, 2019

Uh oh!

miss-islington commented Jul 1, 2019

Uh oh!

miss-islington commented Jul 1, 2019

Uh oh!

miss-islington commented Jul 1, 2019

Uh oh!

bedevere-bot commented Jul 1, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

tiran commented Jun 27, 2019 •

edited by bedevere-bot

Loading