Repository navigation
Expose SSL contexts #52796
Description
| BPO | 8550 |
|---|---|
| Nosy | @pitrou, @vstinner, @ericvsmith, @giampaolo |
| Files |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
assignee = 'https://github.com/pitrou'
closed_at = <Date 2010-05-16.20:25:15.583>
created_at = <Date 2010-04-27.20:56:41.029>
labels = ['type-feature', 'library']
title = 'Expose SSL contexts'
updated_at = <Date 2015-10-05.01:18:33.487>
user = 'https://github.com/pitrou'bugs.python.org fields:
activity = <Date 2015-10-05.01:18:33.487>
actor = 'berker.peksag'
assignee = 'pitrou'
closed = True
closed_date = <Date 2010-05-16.20:25:15.583>
closer = 'pitrou'
components = ['Library (Lib)']
creation = <Date 2010-04-27.20:56:41.029>
creator = 'pitrou'
dependencies = []
files = ['17173', '17178', '17225', '17368']
hgrepos = []
issue_num = 8550
keywords = ['patch']
message_count = 11.0
messages = ['104359', '104360', '104751', '104760', '105058', '105479', '105848', '105857', '105871', '105873', '105877']
nosy_count = 7.0
nosy_names = ['exarkun', 'janssen', 'pitrou', 'vstinner', 'eric.smith', 'giampaolo.rodola', 'heikki']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'enhancement'
url = 'https://bugs.python.org/issue8550'
versions = ['Python 3.2']
Activity
pitrou commentedon Apr 27, 2010
We should expose SSL contexts at the Python level, and rework SSL sockets to use those objects internally (rather than creating their own private context).
It would allow to:
The way I see it, the existing wrap_socket() module-level function would be kept for compatibility; context objects would expose their own wrap_socket() method, without all the arguments of course.
exarkun commentedon Apr 27, 2010
For reference:
http://pyopenssl.sourceforge.net/pyOpenSSL.html/openssl-context.html
http://www.heikkitoivonen.net/m2crypto/api/M2Crypto.SSL.Context%27.Context-class.html
and
man -k SSL_CTX_pitrou commentedon May 1, 2010
Here is a patch exposing SSL contexts as the "SSLContext" class. Also, SSL sockets are refactored to create a standalone SSLContext object, unless you create them using the new SSLContext.wrap_socket(). Please note that SSLContexts do not expose much more information than SSL sockets previously did. New SSLContext functionality (such as options) can be added later.
Docs are missing, but tests are there.
pitrou commentedon May 2, 2010
New patch with docs.
pitrou commentedon May 5, 2010
New patch after reindent of _ssl.c
pitrou commentedon May 10, 2010
I uploaded the latest patch at http://codereview.appspot.com/1124044
heikki commentedon May 16, 2010
Since SSLv2 is insecure, could you at least add a warning for that protocol? I think there was a separate issue for removing it altogether, but could a warning be added here?
The documentation should mention that verify_mode=CERT_REQUIRED is recommended for security.
There should probably be an example of using SSL context in the documentation.
I think you need to expose SSL_CTX_set_options(). Currently the code just sets all options, which means that the default protocol SSLv23 will accept SSLv2 which is insecure. Most people would want to probably do something like ctx.set_options(SSL_OP_ALL | SSL_OP_NO_SSLv2). Documentation should also mention that this is recommended for security. See man SSL_CTX_set_options.
Otherwise I could not see issues with the code, apart from the still #if 0'd out sections and commented out sections, which you are planning on doing something about, right?
pitrou commentedon May 16, 2010
I think it should be a separate issue (since it also applies to the
legacy API). I agree it's reasonable to issue a warning. I don't think
we should remove it until OpenSSL itself does, though.
I think we should recommend CERT_OPTIONAL. A server running with
CERT_REQUIRED would refuse clients without a client certificate, which
is probably not common practice for most servers.
(CERT_OPTIONAL is SSL_VERIFY_PEER, and
CERT_REQUIRED is SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT.
The OpenSSL doc says there's no different between both when in client
mode)
There is a separate issue for it (whose patch I will update to use the
new context API when it is committed):
http://bugs.python.org/issue4870
Do note that OpenSSL 1.0.0 disables SSLv2 by default when using SSLv23,
by the way.
Yes, there's a bit of cleanup work remaining.
pitrou commentedon May 16, 2010
Here is a patch addressing Heikki's and Jean-Paul's review comments (including additional documentation and a test for capath).
pitrou commentedon May 16, 2010
I've committed the patch in r81233. I'm going to watch the buildbots and close the issue if everything's fine.
pitrou commentedon May 16, 2010
A couple of buildbot failures led to fixes in r81234 and r81235. Everything should be fine now.