GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-34670: Add TLS 1.3 post handshake auth by tiran · Pull Request #9460 · python/cpython

Conversation

@tiran

@tiran tiran commented Sep 20, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes [email protected]q

https://bugs.python.org/issue34670

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@tiran tiran added needs backport to 3.6 labels Sep 20, 2018
@tiran tiran assigned alex, benjaminp, 1st1 and ned-deily Sep 20, 2018
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Sep 20, 2018
@bedevere-bot bedevere-bot added the awaiting merge label Sep 20, 2018
@tiran

tiran commented Sep 20, 2018

Copy link
Copy Markdown
Member Author

Technically this is a new feature. However we need the new feature in older releases. Otherwise HTTP clients cannot enable PHA and therefore won't work with mod_ssl and other services that conditionally request TLS client cert auth based on HTTP method or path.

Yury, you may need to add the verify_client_post_handshake method to asyncio's SSL support.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@1st1

1st1 commented Sep 20, 2018

Copy link
Copy Markdown
Member

cc @fantix

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fantix
fantix reviewed Sep 21, 2018
View reviewed changes
Comment thread Doc/library/ssl.rst Outdated

@fantix fantix Sep 21, 2018

Copy link
Copy Markdown
Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(trivial) typo s/is/it

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tiran tiran Sep 21, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@fantix

fantix commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

@1st1 Looks cool, shall I try to add the API in asyncio?

In the meanwhile, I'll update PR #8620 to follow the same style @tiran is using here.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@1st1

1st1 commented Sep 21, 2018

Copy link
Copy Markdown
Member

@1st1 Looks cool, shall I try to add the API in asyncio?

What kind of API do we need to add? Is it a new function or a new keyword argument?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fantix

fantix commented Sep 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Ah, I think it will be a new method as _SSLProtocolTransport.verify_client_post_handshake with no argument (simply exposing SSLObject.verify_client_post_handshake), so that an asyncio SSL server may request the peer to send PHA client certificate any time after the handshake. From OpenSSL source code, this method only set a flag in its state machine, thus no I/O involved. I believe the actual CertificateRequest is then sent with any following I/O operations on the same SSLObject. I'll double check to make sure it shall work as expected.

(In addition, I'm thinking about whether it is useful to actually wait for the client certificate and return the verification result instead.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@1st1

1st1 commented Sep 21, 2018

Copy link
Copy Markdown
Member

So what object will get the new method? Loop or Server or something else? I think we need a new issue with a design for this ;) @tiran any advice?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@fantix

fantix commented Sep 21, 2018

Copy link
Copy Markdown
Contributor

I think it'll be the SSL transport object _SSLProtocolTransport. But yeah, I'd be happy to learn more advices and come up with a proposal in asyncio.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@1st1

1st1 commented Sep 21, 2018

Copy link
Copy Markdown
Member

I think it'll be the SSL transport object _SSLProtocolTransport.

Alright, that makes sense.

But yeah, I'd be happy to learn more advices and come up with a proposal in asyncio.

I think we should land this PR and open a new bugs.python.org to discuss new asyncio API.

Also cc @asvetlov

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1
1st1 approved these changes Sep 21, 2018
View reviewed changes
@ned-deily

ned-deily commented Sep 21, 2018 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

In principle, I am OK with backporting this to 3.7.x and 3.6.x (as long as it does not introduce any user-visible incompatibilities) since it is in the special category of network security best practices. I do think it should go into master first and get some buildbot exposure, preferably with all three current OpenSSL levels we support (to make sure we don't break 1.0.2x and 1.1.0x), before backporting to maintenance branches. So I don't think we should try to push this into the imminent 3.7.1 and 3.6.7 releases.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran force-pushed the bpo34670-tls-pha branch from 2fa4e8b to 5e53cd9 Compare September 21, 2018 07:39
@tiran

tiran commented Sep 21, 2018

Copy link
Copy Markdown
Member Author

I have fixed the typo, enhanced the documentation and added more test cases.

For now I'd just add the method call to the transport. As @fantix pointed out, the method doesn't perform any IO by itself. A typical scenario may looks like this

  • client
    • send HTTP GET /path
  • server
    • recv
    • verify_client_post_handshake
    • send HTTP Connection Upgrade (emits CertRequest message)
  • client
    • recv
    • send upgrade confirmation (emits Certificate, CertificateVerify, Finish message)
  • server
    • recv
    • verify certificate
    • send payload or error (may emit TLS alert for unknown, invalid, or missing cert)
  • client
    • recv (receive TLS alert or server response)

(Note, I'm not fully sure that I got the HTTP part right.)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran force-pushed the bpo34670-tls-pha branch 2 times, most recently from 241c9f6 to 397ef68 Compare September 21, 2018 09:03
@tiran

tiran commented Sep 21, 2018

Copy link
Copy Markdown
Member Author

@1st1 I updated the documentation and added a whatsnew. Please review my PR again.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@1st1

1st1 commented Sep 21, 2018

Copy link
Copy Markdown
Member

A typical scenario may looks like this

Makes sense.

For now I'd just add the method call to the transport.

Another option would be to use get_extra_info() API to get something like an "SSLExtra" object with additional APIs. Otherwise we'll have to add a new transport mixin type (to asyncio/transports.py). Anyways, let's open a new issue for that and discuss it there.

I assume this is going to be a new API in 3.8, or do we need to backport this new API to 3.6/3.7?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1
1st1 reviewed Sep 21, 2018
View reviewed changes
Comment thread Doc/library/ssl.rst Outdated

@1st1 1st1 Sep 21, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it raise an error if it's not TLS 1.3?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@tiran tiran Sep 22, 2018

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does raise an error if any precondition isn't met (not TLS 1.3, called on client side, PHA not enabled, before or during handshake, ...).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
1st1
1st1 reviewed Sep 21, 2018
View reviewed changes
Comment thread Lib/ssl.py Outdated

@1st1 1st1 Sep 21, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to always expose the method but raise NotImplementedError if no TLS 1.3 is available?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
1st1
1st1 reviewed Sep 21, 2018
View reviewed changes
Comment thread Lib/ssl.py Outdated

@1st1 1st1 Sep 21, 2018

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
1st1
1st1 approved these changes Sep 21, 2018
View reviewed changes
@tiran

tiran commented Sep 22, 2018

Copy link
Copy Markdown
Member Author

@1st1 I changed the implementation as requested. The property and method are now always available. Without TLS 1.3, the property is read-only and returns None, the method raises NotImplementedError.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran force-pushed the bpo34670-tls-pha branch 2 times, most recently from 66895eb to 91d1a3b Compare September 22, 2018 06:44
@tiran
bpo-34670: Add TLS 1.3 post handshake auth
8418bf5
Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes <[email protected]>
@tiran
tiran force-pushed the bpo34670-tls-pha branch from 91d1a3b to 8418bf5 Compare September 22, 2018 06:45
@tiran tiran added the 🤖 automerge label Sep 23, 2018
@miss-islington
miss-islington merged commit 9fb051f into python:master Sep 23, 2018
@miss-islington

miss-islington commented Sep 23, 2018

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the awaiting merge label Sep 23, 2018
@miss-islington

miss-islington commented Sep 23, 2018

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington miss-islington assigned miss-islington and unassigned alex, benjaminp, 1st1 and ned-deily Sep 23, 2018
@miss-islington

miss-islington commented Sep 23, 2018

Copy link
Copy Markdown
Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@miss-islington

miss-islington commented Sep 23, 2018

Copy link
Copy Markdown
Contributor

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 9fb051f032c36b9f6086b79086b4d6b7755a3d70 2.7

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot

bedevere-bot commented Sep 23, 2018

Copy link
Copy Markdown

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@bedevere-bot bedevere-bot removed the needs backport to 3.7 label Sep 23, 2018
tiran added a commit to tiran/cpython that referenced this pull request Sep 23, 2018
@tiran
[3.7] bpo-34670: Add TLS 1.3 post handshake auth (pythonGH-9460)
69f6f4e
Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes <[email protected]>q

https://bugs.python.org/issue34670.
(cherry picked from commit 9fb051f)

Co-authored-by: Christian Heimes <[email protected]>
tiran added a commit to tiran/cpython that referenced this pull request Sep 23, 2018
@tiran
[3.6] bpo-34670: Add TLS 1.3 post handshake auth (pythonGH-9460)
b405e15
Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes <[email protected]>q

https://bugs.python.org/issue34670.
(cherry picked from commit 9fb051f)

Co-authored-by: Christian Heimes <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.6 label Sep 23, 2018
@bedevere-bot

bedevere-bot commented Sep 23, 2018

Copy link
Copy Markdown

GH-9507 is a backport of this pull request to the 3.6 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tiran
tiran deleted the bpo34670-tls-pha branch September 23, 2018 07:02
miss-islington pushed a commit that referenced this pull request Sep 23, 2018
@tiran @miss-islington
[3.7] bpo-34670: Add TLS 1.3 post handshake auth (GH-9460) (GH-9505)
2756ef3
Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes <[email protected]>q

https://bugs.python.org/issue34670.
(cherry picked from commit 9fb051f)

Co-authored-by: Christian Heimes <[email protected]>



https://bugs.python.org/issue34670
miss-islington pushed a commit that referenced this pull request Sep 23, 2018
@tiran @miss-islington
[3.6] bpo-34670: Add TLS 1.3 post handshake auth (GH-9460) (GH-9507)
94812f7
Add SSLContext.post_handshake_auth and
SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake
authentication.

Signed-off-by: Christian Heimes <[email protected]>q

https://bugs.python.org/issue34670.
(cherry picked from commit 9fb051f)

Co-authored-by: Christian Heimes <[email protected]>



https://bugs.python.org/issue34670
@fantix

fantix commented Sep 30, 2018 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

FYI I created bpo-34847 to track the discussion of asyncio PHA.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@njsmith njsmith mentioned this pull request Oct 18, 2018
Support TLS 1.3 post handshake auth in trio.ssl python-trio/trio#741
Open
@csabella csabella mentioned this pull request Feb 28, 2019
[3.7] Add versionadded directives to ssl.minimum_version and ssl.maxi… #12101
Merged
@fantix fantix mannequin mentioned this pull request Apr 10, 2022
asyncio: Add PHA for TLS 1.3 #79028
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@1st1 1st1 1st1 approved these changes
+1 more reviewer
@fantix fantix fantix left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@miss-islington miss-islington

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@tiran @1st1 @fantix @ned-deily @miss-islington @bedevere-bot @alex @benjaminp @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.