bpo-34670: Add TLS 1.3 post handshake auth by tiran · Pull Request #9460 · python/cpython (original) (raw)
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 our terms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation26 Commits1 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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.
| request a TLS client certificate at any time after the handshake. |
|---|
| When enabled on client-side sockets, the client signals the server that |
| is supports post-handshake authentication. |
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
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed
@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.
@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?
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.)
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?
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.
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
1st1 approved these changes Sep 21, 2018
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.
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
- send
- 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.)
@1st1 I updated the documentation and added a whatsnew. Please review my PR again.
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?
| .. method:: SSLSocket.verify_client_post_handshake() |
|---|
| Requests post-handshake authentication (PHA) from a TLS 1.3 client. PHA |
| can only be initiated for a TLS 1.3 connection from a server-side socket, |
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?
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, ...).
| @@ -777,6 +777,10 @@ def version(self): |
|---|
| current SSL channel. """ |
| return self._sslobj.version() |
| if HAS_TLSv1_3: |
| def verify_client_post_handshake(self): |
| return self._sslobj.verify_client_post_handshake() |
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?
| if self._sslobj: |
|---|
| return self._sslobj.verify_client_post_handshake() |
| else: |
| raise ValueError("No SSL wrapper around " + str(self)) |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ditto
1st1 approved these changes Sep 21, 2018
@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.
Add SSLContext.post_handshake_auth and SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake authentication.
Signed-off-by: Christian Heimes christian@python.org
Thanks @tiran for the PR 🌮🎉.. I'm working now to backport this PR to: 2.7, 3.6, 3.7.
🐍🍒⛏🤖
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
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
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
tiran added a commit to tiran/cpython that referenced this pull request
Add SSLContext.post_handshake_auth and SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake authentication.
Signed-off-by: Christian Heimes christian@python.orgq
https://bugs.python.org/issue34670. (cherry picked from commit 9fb051f)
Co-authored-by: Christian Heimes christian@python.org
tiran added a commit to tiran/cpython that referenced this pull request
Add SSLContext.post_handshake_auth and SSLSocket.verify_client_post_handshake for TLS 1.3 post-handshake authentication.
Signed-off-by: Christian Heimes christian@python.orgq
https://bugs.python.org/issue34670. (cherry picked from commit 9fb051f)
Co-authored-by: Christian Heimes christian@python.org
FYI I created bpo-34847 to track the discussion of asyncio PHA.
