classification
Title: ssl errors checking
Type: Stage:
Components: Versions: Python 3.2
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: giampaolo.rodola Nosy List: exarkun, giampaolo.rodola, janssen, pitrou
Priority: normal Keywords: patch

Created on 2010-08-28 19:19 by giampaolo.rodola, last changed 2010-09-10 09:37 by giampaolo.rodola. This issue is now closed.

Files
File name Uploaded Description Edit
ssl-errors.patch giampaolo.rodola, 2010-08-29 18:03
Messages (11)
msg115172 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-28 21:01
> SSLError: _ssl.c:296: Both the key & certificate files must be
> specified for server-side operation
> 
> I would change this behavior in SSLSocket constructor and raise
> ValueError if server_side is True and certfile is None.

Good idea.

> Also, the message coming from the C code should be adjusted to state
> than keyfile argument is not mandatory.

The message is arguably technically correct: you need both a (private)
key and a certificate. It's simply that they can be put in the same
file.

> >>> s = ssl.wrap_socket(socket.socket(), server_side=1)
> >>> s.connect(('blogger.com', 443))
> >>> 
> 
> For consistency I would expect something like ValueError("can't
> connect in server-side mode") on connect().

Indeed.

> ssl.SSLError: [Errno 336445442] _ssl.c:1604: error:140DC002:SSL
> routines:SSL_CTX_use_certificate_chain_file:system lib
> >>> 
> 
> A simple "IOError No such file or directory 'xxx'" exception would be
> a lot more clear.

Agreed, but the OpenSSL error reporting system looks too convoluted (or
braindead) to easily allow such aliasing of errors. If you have an idea,
don't hesitate to share :)

> ssl.SSLError: [Errno 336445449] _ssl.c:1604: error:140DC009:SSL
> routines:SSL_CTX_use_certificate_chain_file:PEM lib
> >>> 
> 
> If possible, the error should be more clear about what happened.
> Something like "malformed certfile was provided" or something.

Same as above: the error message and numeric code come from OpenSSL, not
from us.
msg115177 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-29 10:31
>> A simple "IOError No such file or directory 'xxx'" exception would be
>> a lot more clear.
> Agreed, but the OpenSSL error reporting system looks too convoluted (or
> braindead) to easily allow such aliasing of errors. If you have an 
> idea, don't hesitate to share :)

The only idea which comes to mind is try to open() the file before calling load_cert_chain().
That would automatically also take care of permission errors, etc..
Not very clean, but... :-\

>> If possible, the error should be more clear about what happened.
>> Something like "malformed certfile was provided" or something.
> Same as above: the error message and numeric code come from OpenSSL, not
> from us.

No ideas here. I googled for some OpenSSL API to verify the certificate, which we can even possibly expose in ssl.py, but I couldn't find any. I guess we can't do nothing about this.
msg115179 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-29 16:00
> The only idea which comes to mind is try to open() the file before calling load_cert_chain().
> That would automatically also take care of permission errors, etc..
> Not very clean, but... :-\

It's vulnerable to various issues such as race conditions (for example,
you open() the file while it still exists but it doesn't exist anymore
when OpenSSL opens it again).

A clean way to do this would be to use lower-level APIs such as
PEM_read_X509(), so that we can pass our own FILE* to OpenSSL. But it is
also much more code to write.

That said, have you checked the system errno at this point? Perhaps it
gives us enough information (if it hasn't been cleared by
OpenSSL... :/).

> No ideas here. I googled for some OpenSSL API to verify the
> certificate, which we can even possibly expose in ssl.py, but I
> couldn't find any.

I don't think that would change anything, since the verification APIs
would probably give you the exact same error message.
msg115182 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-29 18:03
Fortunately errno was set.
Patch in attachment introduces the following changes:

=== 1 ===

Before:
>>> ssl.wrap_socket(socket.socket(), server_side=1)
>>>

Now:
>>> ssl.wrap_socket(socket.socket(), server_side=1)
ValueError: certfile must be specified for server-side operations


=== 2 ===

Before:
>>> s = ssl.wrap_socket(socket.socket(), server_side=1, certfile='Lib/test/keycert.pem')
>>> s.connect(('blogger.com', 443))
>>>

Now:
>>> s = ssl.wrap_socket(socket.socket(), server_side=1, certfile='Lib/test/keycert.pem')
>>> s.connect(('blogger.com', 443))
ValueError: can't connect in server-side mode


=== 3 ===

Before:
>>> os.path.exists('xxx')
False
>>> ssl.wrap_socket(socket.socket(), certfile='xxx')
ssl.SSLError: [Errno 336445442] _ssl.c:1604: error:140DC002:SSL routines:SSL_CTX_use_certificate_chain_file:system lib

Now:
>>> os.path.exists('xxx')
False
>>> ssl.wrap_socket(socket.socket(), certfile='xxx')
IOError: [Errno 2] No such file or directory


=== 4 ===

Before:
>>> os.path.exists('xxx')
False
>>> ctx = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
>>> ctx.load_verify_locations('xxx')
ssl.SSLError: [Errno 185090050] _ssl.c:1676: error:0B084002:x509 certificate routines:X509_load_cert_crl_file:system lib

Now:
>>> os.path.exists('xxx')
False
>>> ctx = ssl.SSLContext(ssl.PROTOCOL_TLSv1)
>>> ctx.load_verify_locations('xxx')
IOError: [Errno 2] No such file or directory
msg115183 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-29 18:11
>>> ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
>>> ctx.wrap_socket(socket.socket(), server_side=1)
<ssl.SSLSocket object, fd=3, family=2, type=1, proto=0>
>>> 

I'm not sure how to raise ValueError("certfile must be specified") here as SSLContext class doesn't store certfile information, at least at Python level. Any hint?
msg115184 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-29 18:20
> >>> ctx = ssl.SSLContext(ssl.PROTOCOL_SSLv23)
> >>> ctx.wrap_socket(socket.socket(), server_side=1)
> <ssl.SSLSocket object, fd=3, family=2, type=1, proto=0>
> >>> 
> 
> I'm not sure how to raise ValueError("certfile must be specified")
> here as SSLContext class doesn't store certfile information, at least
> at Python level. Any hint?

Actually, you shouldn't raise the error, since the certfile can be
specified after wrapping the socket.
msg115188 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-29 19:33
@Antoine: ok, thanks.

This is now committed in r84352. See also r84351 which raises ValueError if non-zero flag argument is provided for sendall().
http://bugs.python.org/msg115166 was the original message at the top of this discussion which I accidentally removed.
msg115190 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-08-29 19:44
> @Antoine: ok, thanks.
> 
> This is now committed in r84352.

I don't think it's ok to test for the IOError message ("No such file"),
because it comes from the OS and can therefore change from platform to
platform. Instead, you should check the value of the "errno" attribute
on the IOError objects.

Also:

+            except IOError as x:
+                if support.verbose:
+                    sys.stdout.write("\nsocket.error is %s\n" % str(x))

The message should be changed (IOError instead of socket.error).
msg115196 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-08-29 20:57
You're right. Committed in r84355.
msg115307 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-09-01 14:52
Some buildbots have started failing exactly after this commit:

======================================================================
ERROR: test_errors (test.test_ssl.BasicSocketTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_ssl.py", line 186, in test_errors
    s = ssl.wrap_socket(sock, server_side=True, certfile=CERTFILE)
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/ssl.py", line 414, in wrap_socket
    ciphers=ciphers)
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/ssl.py", line 137, in __init__
    self.context.load_cert_chain(certfile, keyfile)
ssl.SSLError: [Errno 185057381] _ssl.c:1609: error:0B07C065:x509 certificate routines:X509_STORE_add_cert:cert already in hash table

======================================================================
ERROR: test_protocol_sslv2 (test.test_ssl.ThreadedTests)
Connecting to an SSLv2 server with various client options
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_ssl.py", line 80, in f
    return func(*args, **kwargs)
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_ssl.py", line 1100, in test_protocol_sslv2
    try_protocol_combo(ssl.PROTOCOL_SSLv2, ssl.PROTOCOL_SSLv2, True)
  File "/srv/buildbot/buildarea/3.x.bolen-ubuntu/build/Lib/test/test_ssl.py", line 969, in try_protocol_combo
    ctx.load_cert_chain(CERTFILE)
ssl.SSLError: [Errno 336445442] _ssl.c:1609: error:140DC002:SSL routines:SSL_CTX_use_certificate_chain_file:system lib

See e.g. http://www.python.org/dev/buildbot/builders/x86%20Ubuntu%203.x/builds/1870

This is probably dependent on the OpenSSL version.
msg115328 - (view) Author: Giampaolo Rodola' (giampaolo.rodola) * (Python committer) Date: 2010-09-01 19:31
Committed r84400 which should fix the first test failure.
I'll take a look at the buildbots to see how it goes.
As for the second failure I have no idea at the moment.
History
Date User Action Args
2010-09-10 09:37:29giampaolo.rodolasetstatus: open -> closed
2010-09-01 19:31:31giampaolo.rodolasetmessages: + msg115328
2010-09-01 14:52:01pitrousetstatus: closed -> open

messages: + msg115307
2010-08-29 20:57:36giampaolo.rodolasetmessages: + msg115196
2010-08-29 19:44:05pitrousetmessages: + msg115190
2010-08-29 19:33:43giampaolo.rodolasetstatus: open -> closed
assignee: giampaolo.rodola
resolution: fixed
messages: + msg115188
2010-08-29 18:20:02pitrousetmessages: + msg115184
2010-08-29 18:11:08giampaolo.rodolasetmessages: + msg115183
2010-08-29 18:03:12giampaolo.rodolasetfiles: + ssl-errors.patch
keywords: + patch
messages: + msg115182
2010-08-29 16:00:08pitrousetmessages: + msg115179
2010-08-29 11:24:32giampaolo.rodolasetmessages: - msg115166
2010-08-29 10:31:45giampaolo.rodolasetmessages: + msg115177
2010-08-28 21:01:49pitrousetmessages: + msg115172
2010-08-28 19:20:11giampaolo.rodolasetnosy: + exarkun
2010-08-28 19:19:02giampaolo.rodolacreate