| msg104359 - (view) |
Author: Antoine Pitrou (pitrou) *  |
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 (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) * |
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) * |
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) * |
Date: 2010-05-02 00:43 |
| New patch with docs. |
|
|
| msg105058 - (view) |
Author: Antoine Pitrou (pitrou) * |
Date: 2010-05-05 16:43 |
| New patch after reindent of _ssl.c |
|
|
| msg105479 - (view) |
Author: Antoine Pitrou (pitrou) * |
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) * |
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) * |
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) * |
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) * |
Date: 2010-05-16 20:25 |
| A couple of buildbot failures led to fixes in r81234 and r81235. Everything should be fine now. |
|
|