classification
Title: test_ssl.bad_cert_test() exception handling
Type: Stage: resolved
Components: Tests Versions: Python 3.6, Python 3.5, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: berker.peksag, martin.panter, python-dev
Priority: normal Keywords: patch

Created on 2016-01-21 10:46 by martin.panter, last changed 2016-02-01 05:24 by martin.panter. This issue is now closed.

Files
File name Uploaded Description Edit
bad-cert-py3.patch martin.panter, 2016-01-27 11:50 Remove dead handler review
wrong-cert-py2.patch martin.panter, 2016-01-28 01:24 review
wrong-cert-py3.patch martin.panter, 2016-01-28 01:51 review
separate-test.patch martin.panter, 2016-01-29 09:04 review
separate-test-py3.v2.patch martin.panter, 2016-01-31 02:19 review
Messages (10)
msg258754 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-21 10:46
In bad_cert_test(), there are two OSError exception handler; one masking the other. In Python 3, I think we should remove the second (dead) handler. In Python 2, maybe the first OSError handler should catch socket.error instead.

Originally, in r80534, socket.error was caught and ignored, with the vague explanation “socket.error can really happen here”. Then revision 9297974604ff added an IOError handler, presumably to catch ENOENT for test_nonexisting_cert().

Later, in revisions 50d19c2fac82 and 9297974604ff, socket.error and IOError were both changed to OSError. I guess in Python 3 we should just catch all OSError exceptions and remove the second handler that only wants ENOENT.

In Python 2, there was a large backport of SSL functionality in revision 221a1f9155e2 (Issue 21308). It seems to have brought too much of the OSError alias changes with it. This is probably the cause of the following 2.7 builtbot failure:

http://buildbot.python.org/all/builders/x86%20XP-4%202.7/builds/3580/steps/test/logs/stdio
======================================================================
ERROR: test_nonexisting_cert (test.test_ssl.ThreadedTests)
Connecting with a non-existing cert file
----------------------------------------------------------------------
Traceback (most recent call last):
  File "d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\lib\test\test_ssl.py", line 2153, in test_nonexisting_cert
    "wrongcert.pem"))
  File "d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\lib\test\test_ssl.py", line 1889, in bad_cert_test
    s.connect((HOST, server.port))
  File "d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\lib\ssl.py", line 844, in connect
    self._real_connect(addr, False)
  File "d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\lib\ssl.py", line 835, in _real_connect
    self.do_handshake()
  File "d:\cygwin\home\db3l\buildarea\2.7.bolen-windows\build\lib\ssl.py", line 808, in do_handshake
    self._sslobj.do_handshake()
error: [Errno 10054] An existing connection was forcibly closed by the remote host

Errno 10054 is apparently ECONNRESET.
msg259082 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-28 01:24
There are more mistakes in the history of test_nonexisting_cert(). In revision 8a281bfc058d (Python 2.6), the method was added as testWrongCert(), with an existing but non-matching certificate file. But when this was ported to Python 3 in r66311, the wrongcert.pem file was not added, so Python 3 was actually testing the behaviour when the specified certificate file was missing. Then in r80596, the test method was renamed and a comment added assuming the Python 3 version with the missing file. However we already test the behaviour of missing files in test_errors().

I do not understand the ECONNRESET failure on Windows. Perhaps there is a race to do with the server closing the connection when the client should be reporting a certificate mismatch. It seems like a bug, and I suspect r80534 is not the correct fix. But I’m not in a position to investigate so I will leave that code as it is.

For Python 2 I propose wrong-cert-py2.patch:

* Rename WRONGCERT → NONEXISTINGCERT, not to be confused with wrongcert.pem
* Repurpose test_nonexisting_cert() → test_wrong_cert()
* Remove ENOENT exception handling from bad_cert_test()
msg259084 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-28 01:51
wrong-cert-py3.patch is similar but also adds the wrongcert.pem file from Python 2.
msg259187 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-29 03:09
New changeset 14463726a4a0 by Martin Panter in branch '2.7':
Issue #26173: Fix test_ssl confusion with non-existing cert and wrongcert.pem
https://hg.python.org/cpython/rev/14463726a4a0
msg259194 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-29 05:00
New changeset c75ae81b5058 by Martin Panter in branch '3.5':
Issue #26173: Fix test_ssl confusion with non-existing cert and wrongcert.pem
https://hg.python.org/cpython/rev/c75ae81b5058

New changeset 284b3de802b7 by Martin Panter in branch 'default':
Issue #26173: Merge wrongcert test from 3.5
https://hg.python.org/cpython/rev/284b3de802b7
msg259199 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-29 09:04
I was mistaken about what test_wrong_cert() is doing. It tells the _server_ to check the _client’s_ certificate (the opposite way around from HTTPS for example). Now I understand this, I see it is fine for the client to raise ECONNRESET if the server rejected its certificate.

In separate-test.patch, I decoupled test_wrong_cert() from the other three bad_cert_test() tests because they are testing different stuff. Changes to test_wrong_cert():

* Fix ResourceWarning in my previous change by closing the SSL socket
* Catch socket.error rather than OSError to fix the buildbot failures in 2.7 (The change does nothing in Python 3 because they are aliases.)
* Make test_wrong_cert() stricter by only allowing ECONNRESET or SSLError

Changes to the other three tests:

* Moved them to BasicSocketTests and do not run a server
* Only accept SSLError from wrap_socket(), drop OSError handling and connect() call
msg259275 - (view) Author: Martin Panter (martin.panter) * (Python committer) Date: 2016-01-31 02:19
Updated patch; thanks for the comments Berker
msg259278 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2016-01-31 09:49
Looks good to me. Thanks Martin.
msg259303 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-01-31 22:45
New changeset 6d068205b580 by Martin Panter in branch '3.5':
Issue #26173: Separate bad cert file tests and client rejection test
https://hg.python.org/cpython/rev/6d068205b580

New changeset 3b211ee66b82 by Martin Panter in branch 'default':
Issue #26173: Merge SSL tests from 3.5
https://hg.python.org/cpython/rev/3b211ee66b82
msg259310 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-02-01 04:12
New changeset 6f7b2b7a029a by Martin Panter in branch '2.7':
Issue #26173: Separate bad cert file tests and client rejection test
https://hg.python.org/cpython/rev/6f7b2b7a029a
History
Date User Action Args
2016-02-01 05:24:35martin.pantersetstatus: open -> closed
resolution: fixed
stage: commit review -> resolved
2016-02-01 04:12:00python-devsetmessages: + msg259310
2016-01-31 22:45:30python-devsetmessages: + msg259303
2016-01-31 09:49:23berker.peksagsetnosy: + berker.peksag

messages: + msg259278
stage: patch review -> commit review
2016-01-31 02:19:12martin.pantersetfiles: + separate-test-py3.v2.patch

messages: + msg259275
2016-01-29 09:04:28martin.pantersetfiles: + separate-test.patch

messages: + msg259199
2016-01-29 05:00:08python-devsetmessages: + msg259194
2016-01-29 03:09:22python-devsetnosy: + python-dev
messages: + msg259187
2016-01-28 01:51:44martin.pantersetkeywords: - buildbot
files: + wrong-cert-py3.patch
messages: + msg259084

stage: needs patch -> patch review
2016-01-28 01:24:36martin.pantersetfiles: + wrong-cert-py2.patch

messages: + msg259082
2016-01-27 11:50:46martin.pantersetfiles: + bad-cert-py3.patch
keywords: + patch
2016-01-21 10:46:23martin.pantercreate