This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Expose SSL contexts
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: pitrou Nosy List: eric.smith, exarkun, giampaolo.rodola, heikki, janssen, pitrou, vstinner
Priority: normal Keywords: patch

Created on 2010-04-27 20:56 by pitrou, last changed 2022-04-11 14:57 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
sslcontext.patch pitrou, 2010-05-01 20:56
sslcontext2.patch pitrou, 2010-05-02 00:43
sslcontext3.patch pitrou, 2010-05-05 16:43
sslcontext4.patch pitrou, 2010-05-16 15:39
Messages (11)
msg104359 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-04-27 20:56
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:
- specify the various options iteratively, rather than having to dump them all in the wrap_socket() arguments
- add methods to query information about the current options, key/cert, etc.
- solve issue3823 (you can build the context first, passing it the key/cert info, then drop privileges before creating any sockets)
- more easily share and reuse configuration information
- possibly add more powerful functionality such as sessions

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.
msg104360 - (view) Author: Jean-Paul Calderone (exarkun) * (Python committer) Date: 2010-04-27 20:59
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_`
msg104751 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-01 20:56
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.
msg104760 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-02 00:43
New patch with docs.
msg105058 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-05 16:43
New patch after reindent of _ssl.c
msg105479 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-10 22:58
I uploaded the latest patch at http://codereview.appspot.com/1124044
msg105848 - (view) Author: Heikki Toivonen (heikki) Date: 2010-05-16 04:42
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?
msg105857 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-16 10:45
> 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?

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.

> The documentation should mention that verify_mode=CERT_REQUIRED is recommended for security.

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)

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

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.

> 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?

Yes, there's a bit of cleanup work remaining.
msg105871 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-16 15:39
Here is a patch addressing Heikki's and Jean-Paul's review comments (including additional documentation and a test for capath).
msg105873 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-16 18:28
I've committed the patch in r81233. I'm going to watch the buildbots and close the issue if everything's fine.
msg105877 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-05-16 20:25
A couple of buildbot failures led to fixes in r81234 and r81235. Everything should be fine now.
History
Date User Action Args
2022-04-11 14:57:00adminsetgithub: 52796
2015-10-05 01:18:33berker.peksagsetfiles: - entry.tbp
2015-10-04 23:42:06Lance Warriorsetfiles: + entry.tbp
2010-05-16 20:25:15pitrousetstatus: open -> closed
resolution: accepted -> fixed
messages: + msg105877
2010-05-16 18:28:27pitrousetassignee: pitrou
resolution: accepted
messages: + msg105873
stage: patch review -> resolved
2010-05-16 15:39:57pitrousetfiles: + sslcontext4.patch

messages: + msg105871
2010-05-16 10:45:18pitrousetmessages: + msg105857
2010-05-16 04:42:22heikkisetmessages: + msg105848
2010-05-10 22:58:36pitrousetmessages: + msg105479
2010-05-07 00:28:49vstinnersetnosy: + vstinner
2010-05-05 16:44:09pitrousetfiles: + sslcontext3.patch

messages: + msg105058
2010-05-02 00:43:33pitrousetfiles: + sslcontext2.patch

messages: + msg104760
2010-05-01 20:56:58pitrousetfiles: + sslcontext.patch
keywords: + patch
messages: + msg104751

stage: needs patch -> patch review
2010-04-28 12:45:10pitrousetnosy: + heikki
2010-04-27 22:15:28eric.smithsetnosy: + eric.smith
2010-04-27 21:39:25pitroulinkissue8106 dependencies
2010-04-27 20:59:50exarkunsetnosy: + exarkun
messages: + msg104360
2010-04-27 20:56:41pitroucreate