[issue26173] test_ssl.bad_cert_test() exception handling

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Changes by Martin Panter :


--
resolution:  -> fixed
stage: commit review -> resolved
status: open -> closed

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Berker Peksag]

Berker Peksag added the comment:

Looks good to me. Thanks Martin.

--
nosy: +berker.peksag
stage: patch review -> commit review

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Martin Panter added the comment:

Updated patch; thanks for the comments Berker

--
Added file: http://bugs.python.org/file41763/separate-test-py3.v2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Martin Panter added the comment:

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

--
Added file: http://bugs.python.org/file41749/separate-test.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Roundup Robot]

Roundup Robot added the comment:

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

--
nosy: +python-dev

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Roundup Robot]

Roundup Robot added the comment:

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

--

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Martin Panter added the comment:

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

--
Added file: http://bugs.python.org/file41735/wrong-cert-py2.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Martin Panter added the comment:

wrong-cert-py3.patch is similar but also adds the wrongcert.pem file from 
Python 2.

--
keywords:  -buildbot
stage: needs patch -> patch review
Added file: http://bugs.python.org/file41736/wrong-cert-py3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

Changes by Martin Panter :


--
keywords: +patch
Added file: http://bugs.python.org/file41725/bad-cert-py3.patch

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com

[issue26173] test_ssl.bad_cert_test() exception handling

[Martin Panter]

New submission from Martin Panter:

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.

--
components: Tests
keywords: buildbot
messages: 258754
nosy: martin.panter
priority: normal
severity: normal
stage: needs patch
status: open
title: test_ssl.bad_cert_test() exception handling
versions: Python 2.7, Python 3.5, Python 3.6

___
Python tracker 

___
___
Python-bugs-list mailing list
Unsubscribe: 
https://mail.python.org/mailman/options/python-bugs-list/archive%40mail-archive.com