Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSL tests failed due to expired svn.python.org SSL certificate #70128

Closed
yan12125 mannequin opened this issue Dec 24, 2015 · 29 comments
Closed

SSL tests failed due to expired svn.python.org SSL certificate #70128

yan12125 mannequin opened this issue Dec 24, 2015 · 29 comments
Assignees
Labels
tests Tests in the Lib/test dir

Comments

@yan12125
Copy link
Mannequin

yan12125 mannequin commented Dec 24, 2015

BPO 25940
Nosy @malemburg, @birkenfeld, @larryhastings, @benjaminp, @alex, @skrah, @vadmium, @koobs, @jamadden, @yan12125, @ewdurbin
Files
  • svn-cert.patch: [obsolete]
  • local-server.patch
  • local-server.v2.patch: Change tests to use local server
  • pythontest.patch: Use self-signed.pythontest.net
  • pythontest-set-ca.patch: For pythontestdotnet repo
  • use-pythontest.v2.patch: Use self-signed.pythontest.net
  • local-server.v3.patch
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/vadmium'
    closed_at = <Date 2016-03-27.04:22:18.240>
    created_at = <Date 2015-12-24.20:47:46.458>
    labels = ['tests']
    title = 'SSL tests failed due to expired svn.python.org SSL certificate'
    updated_at = <Date 2016-03-27.04:22:18.238>
    user = 'https://github.com/yan12125'

    bugs.python.org fields:

    activity = <Date 2016-03-27.04:22:18.238>
    actor = 'martin.panter'
    assignee = 'martin.panter'
    closed = True
    closed_date = <Date 2016-03-27.04:22:18.240>
    closer = 'martin.panter'
    components = ['Tests']
    creation = <Date 2015-12-24.20:47:46.458>
    creator = 'yan12125'
    dependencies = []
    files = ['41488', '41512', '41515', '41532', '41589', '41590', '41630']
    hgrepos = []
    issue_num = 25940
    keywords = ['patch']
    message_count = 29.0
    messages = ['256969', '256994', '256996', '256997', '256999', '257000', '257152', '257181', '257442', '257443', '257452', '257453', '257578', '257588', '257589', '257749', '257762', '258076', '258078', '258086', '258100', '258200', '258203', '258253', '258259', '258262', '258352', '262504', '262507']
    nosy_count = 14.0
    nosy_names = ['lemburg', 'georg.brandl', 'geertj', 'larry', 'benjamin.peterson', 'alex', 'skrah', 'python-dev', 'martin.panter', 'koobs', 'jmadden', 'yan12125', 'vincent-legoll', 'EWDurbin']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue25940'
    versions = ['Python 2.7', 'Python 3.2', 'Python 3.3', 'Python 3.4', 'Python 3.5', 'Python 3.6']

    @yan12125
    Copy link
    Mannequin Author

    yan12125 mannequin commented Dec 24, 2015

    The certificate of svn.python.org expires at Thu 24 Dec 2015 08:28:32 PM CST GMT, about 20 minutes ago. Please update its certificate or lots of tests in Lib\test\test_ssl.py fails with SSL: CERTIFICATE_VERIFY_FAILED.

    @yan12125 yan12125 mannequin added the tests Tests in the Lib/test dir label Dec 24, 2015
    @koobs
    Copy link

    koobs commented Dec 25, 2015

    I'm here from (duplicate) bpo-25950

    @malemburg
    Copy link
    Member

    Regardless of the cert having expired, I think SSL tests either should use the main python.org site, e.g. https://www.python.org/ or the domain we specifically have for stdlib tests: http://pythontest.net/.

    Benjamin: If you need a cert for pythontest.net, I can issue one next week.

    Since the tests use CA Cert as root CA for the domain, I'm not sure whether getting a non-CA Cert for svn.python.org would be a good thing. If possible, I can issue one as well, but would have to know how svn.python.org is managed.

    @alex
    Copy link
    Member

    alex commented Dec 25, 2015

    I agree the tests shouldn't rely on a legacy domain like svn.python.org

    In the mean time Ernest is working on getting a valid cert set up.

    @alex
    Copy link
    Member

    alex commented Dec 25, 2015

    There's good news and bad news, which do you want first?

    Good news, great!

    svn.python.org now has a certificate that's not expired, and it's even trusted by major trust stores.

    Bad news?

    The tests rely on the cert for svn.python.org specifically be a cacert one. IMO this is a bug in the tests and should be fixed.

    @ewdurbin
    Copy link
    Member

    Currently svn.python.org is served from one of the last remaining "unmanaged" boxes in our infrastructure, dinsdale.python.org.

    If a cacert signed certificate can be supplied, it can be installed. I see a tree of them in /etc/apache2/ssl/cacert/{2010,2011,2013}. Previously it seems that they were supplied by instantssl as i see certs from 2005 and 2007 there.

    Tying ourselves to a single CA for these tests indefinitely doesn't seem to be the greatest idea though.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Dec 29, 2015

    New changeset 9d0c15425b59 by Stefan Krah in branch 'default':
    Issue bpo-25940: Make buildbots usable again until a solution is found.
    https://hg.python.org/cpython/rev/9d0c15425b59

    @skrah
    Copy link
    Mannequin

    skrah mannequin commented Dec 29, 2015

    Setting to blocker because I've disabled the offending tests.

    @skrah skrah mannequin added the release-blocker label Dec 29, 2015
    @vadmium
    Copy link
    Member

    vadmium commented Jan 4, 2016

    Assuming it is practical, would it be appropriate to convert the failing tests to connect to a local server running in a background Python thread? Perhaps using the existing ThreadedEchoServer. That way the tests wouldn’t require an internet connection, and the certificates used etc should be all contained within the Python source tree.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 4, 2016

    In the meantime, this patch changes the tests to use the new DST root certificate. This seems to be the minimum to make the tests pass again, but someone else should probably review it because I’m not too experienced with SSL certificate stuff.

    @malemburg
    Copy link
    Member

    On 04.01.2016 08:43, Martin Panter wrote:

    In the meantime, this patch changes the tests to use the new DST root certificate. This seems to be the minimum to make the tests pass again, but someone else should probably review it because I’m not too experienced with SSL certificate stuff.

    Thanks. I still think that we should use a more stable
    resource for this other than svn.python.org, e.g. the
    https://www.pythontest.net/ setup which was created specifically
    for stdlib tests and which can more easily be adapted to
    fit the needs of the tests than a public resource which is meant
    for other purposes.

    The local server idea may also work, but would have to run on
    a non-privileged port.

    @koobs
    Copy link

    koobs commented Jan 4, 2016

    Can't / Shouldn't these be mocked?

    @vadmium
    Copy link
    Member

    vadmium commented Jan 6, 2016

    Koobs: What thing are you suggesting to mock? The remote server perhaps, which is what I want to do too?

    Here is a patch that implements my local SSL server idea by reusing ThreadedEchoServer.

    There is one problem with local-server.patch, and I don’t know how to fix it properly. At the end of test_bio_handshake() and test_bio_read_write_data(), the unwrap() call keeps raising SSL_ERROR_SYSCALL and the test hangs in an infinite loop. I could work around this by making the server call unwrap() before shutting the socket down. But it would be better to fix ssl_io_loop() if anyone understands what is going wrong.

    @vadmium
    Copy link
    Member

    vadmium commented Jan 6, 2016

    After looking into the unwrap() problem more, I think I understand the problem, and believe making the server call unwrap() is the correct way forward. SSL_ERROR_SYSCALL is Open SSL’s way of saying the connection was shut down insecurely (among other things). So I propose local-server.v2.patch which passes all the tests without hanging.

    I am inclined to commit my first patch soon, which is fairly simple and should make the buildbots happy. Then people can have a chance to review the second (now third) patch, which is more involved.

    Does the problem really need to be fixed in the 3.2 branch, or is 3.5+ and 2.7 good enough?

    @koobs
    Copy link

    koobs commented Jan 6, 2016

    As many branches as we can muster, ideally all, at least 3.4, 3.5 default and 2.7 please. Back porting is a massive pain, and downstream OS's want to retain integrity of tests to ensure quality of Python in their respective ecosystems

    @vadmium
    Copy link
    Member

    vadmium commented Jan 8, 2016

    In python-dev I offered to make a patch to switch to https://self-signed.pythontest.net. Here is my patch so far.

    There is one outstanding problem though: test_get_ca_certs_capath() fails. I think it is because the self-signed certificate does not have a “CA basic constraints” flag set, and SSLContext.get_ca_certs() no longer lists the certificate. Maybe someone else can help. I suspect this would need to be fixed on the server side (or find another server with a certificate signed with this flag set?), but I could easily be wrong :).

    @vincent-legoll
    Copy link
    Mannequin

    vincent-legoll mannequin commented Jan 8, 2016

    Maybe if the server change is not approved you can still push the part of the patch that un-hardwire that server name everywhere...

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    This is a patch against the pythontestdotnet repository to change the certificate to have the CA flag set. It should be applied in conjunction with an (upcoming) update to the test suite, otherwise test_httplib will fail with certificate verification errors.

    If I push this patch to the pythontest repository, will the server immediately update itself, or are other steps required?

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    Here is the corresponding patch for the test suite. This version should fix all the SSL tests as long as the server is updated.

    @malemburg
    Copy link
    Member

    The patch looks good. I only have one question:

    Why are you removing this part ? ...

    @@ -1684,13 +1688,8 @@
                 try:
                     ret = func(*args)
                 except ssl.SSLError as e:
    -                # Note that we get a spurious -1/SSL_ERROR_SYSCALL for
    -                # non-blocking IO. The SSL_shutdown manpage hints at this.
    -                # It *should* be safe to just ignore SYS_ERROR_SYSCALL because
    -                # with a Memory BIO there's no syscalls (for IO at least).
                     if e.errno not in (ssl.SSL_ERROR_WANT_READ,
    -                                   ssl.SSL_ERROR_WANT_WRITE,
    -                                   ssl.SSL_ERROR_SYSCALL):
    +                                   ssl.SSL_ERROR_WANT_WRITE):
                         raise
                     errno = e.errno
                 # Get any data from the outgoing BIO irrespective of any error, and

    @vadmium
    Copy link
    Member

    vadmium commented Jan 12, 2016

    Marc-Andre: This is a fix or workaround for the problem I first described in <https://bugs.python.org/issue25940#msg257578\>. It looks like the code was written by Geert Jansen in bpo-21965. I suspect it is not right, but I am not familiar enough with the Open SSL API to be certain.

    Geert: can you shed any light on why ssl_io_loop() in /Lib/test/test_ssl.py catches SSL_ERROR_SYSCALL and immediately retries the call? I found that when the call is unwrap(), and the remote end has shut down the TCP connection without a secure SSL-level shutdown, this calls unwrap() over and over in an infinite loop.

    Geert’s comment, that my latest patch removes, mentions a spurious SSL_ERROR_SYSCALL for non-blocking IO, especially for SSL_shutdown(), which is what the Python-level unwrap() method calls. Even though the OS-level socket is blocking, I guess from Open SSL’s point of view it is doing non-blocking IO through the BIO interface.

    The manual page <https://www.openssl.org/docs/manmaster/ssl/SSL_shutdown.html\> does mention a quirk with SSL_ERROR_SYSCALL, but only for a return value of zero, indicating the operation is half done. Python’s SSL module does not seem to pay any attention to any error codes if SSL_shutdown() returns zero; see <https://hg.python.org/cpython/annotate/v3.5.1/Modules/_ssl.c#l2051\>. It just retries once, and then either returns the socket or None, never raising an exception. When the infinite loop problem occurs, the SSL_shutdown() return value is negative, indicating failure.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 14, 2016

    New changeset 81b3beea7e99 by Georg Brandl in branch '3.2':
    Issue bpo-25940: Backport self-signed.pythontest.net testing for test_httplib
    https://hg.python.org/cpython/rev/81b3beea7e99

    New changeset adf750b1252d by Martin Panter in branch '3.2':
    Issue bpo-25940: Use self-signed.pythontest.net in SSL tests
    https://hg.python.org/cpython/rev/adf750b1252d

    New changeset e167a9296947 by Martin Panter <vadmium> in branch '3.3':
    Issue bpo-25940: Merge self-signed.pythontest.net testing from 3.2 into 3.3
    https://hg.python.org/cpython/rev/e167a9296947

    New changeset 32bcb9aa286e by Martin Panter <vadmium> in branch '3.4':
    Issue bpo-25940: Merge self-signed.pythontest.net testing from 3.3 into 3.4
    https://hg.python.org/cpython/rev/32bcb9aa286e

    New changeset 0585c0089466 by Martin Panter <vadmium> in branch '3.4':
    Issue bpo-25940: Update new SSL tests for self-signed.pythontest.net
    https://hg.python.org/cpython/rev/0585c0089466

    New changeset c3aa4b48b905 by Martin Panter <vadmium> in branch '3.5':
    Issue bpo-25940: Merge self-signed.pythontest.net testing from 3.4 into 3.5
    https://hg.python.org/cpython/rev/c3aa4b48b905

    New changeset f5f14d65297e by Martin Panter <vadmium> in branch '3.5':
    Issue bpo-25940: Update new SSL tests for self-signed.pythontest.net
    https://hg.python.org/cpython/rev/f5f14d65297e

    New changeset 2f2b42a8b34d by Martin Panter <vadmium> in branch 'default':
    Issue bpo-25940: Merge self-signed.pythontest.net testing from 3.5
    https://hg.python.org/cpython/rev/2f2b42a8b34d

    New changeset ac94418299bd by Martin Panter <vadmium> in branch 'default':
    Issue bpo-25940: test_ssl is working again
    https://hg.python.org/cpython/rev/ac94418299bd

    @koobs
    Copy link

    koobs commented Jan 14, 2016

    Wow. *thank you* Georg and Martin :)

    @vadmium
    Copy link
    Member

    vadmium commented Jan 15, 2016

    In the 3.3 branch, I got a failure in test_ssl.ThreadedTests.test_dh_params(): “SSLError: [SSL] dh key too small (_ssl.c:548)”. But the failure also happens in the revision before my merge, so I think it must be a separate problem.

    Also, I am seeing Windows 7+ buildbots are failing the new version of test_connect_ex_error(). On Linux, connecting to self-signed.pythontest.net:444 gives EHOSTUNREACH, but it seems we get ETIMEDOUT on Windows:

    AssertionError: 10060 not found in (10061, 10065, 10035)

    I will update the test to check for that error code as well, since all it really needs to test is that _some_ sensible error code is returned.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2016

    New changeset fb7131939508 by Martin Panter in branch '2.7':
    Issue bpo-25940: Use self-signed.pythontest.net in SSL tests
    https://hg.python.org/cpython/rev/fb7131939508

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jan 15, 2016

    New changeset b2036b717028 by Martin Panter in branch '3.2':
    Issue bpo-25940: On Windows, connecting to port 444 returns ETIMEDOUT
    https://hg.python.org/cpython/rev/b2036b717028

    New changeset c5cae7366835 by Martin Panter in branch '3.3':
    Issue bpo-25940: Merge ETIMEDOUT fix from 3.2 into 3.3
    https://hg.python.org/cpython/rev/c5cae7366835

    New changeset a806ed21bd82 by Martin Panter in branch '3.4':
    Issue bpo-25940: Merge ETIMEDOUT fix from 3.3 into 3.4
    https://hg.python.org/cpython/rev/a806ed21bd82

    New changeset bd9d5bc8bc95 by Martin Panter in branch '3.5':
    Issue bpo-25940: Merge ETIMEDOUT fix from 3.4 into 3.5
    https://hg.python.org/cpython/rev/bd9d5bc8bc95

    New changeset 32a4e7b337c9 by Martin Panter in branch 'default':
    Issue bpo-25940: Merge ETIMEDOUT fix from 3.5
    https://hg.python.org/cpython/rev/32a4e7b337c9

    @vadmium
    Copy link
    Member

    vadmium commented Jan 16, 2016

    Here is an updated version of my local server patch. I merged it with the committed changes, and fixed a couple of mistakes. This patch does not need to be applied to the older branches.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 27, 2016

    New changeset 3da812602881 by Martin Panter in branch 'default':
    Issue bpo-25940: Use internal local server more in test_ssl
    https://hg.python.org/cpython/rev/3da812602881

    @vadmium
    Copy link
    Member

    vadmium commented Mar 27, 2016

    Okay so for the record, the maintainence branches were changed over from svn.python.org to self-signed.pythontest.net, and just now I changed the 3.6 branch to use a local server for most tests.

    @vadmium vadmium closed this as completed Mar 27, 2016
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    tests Tests in the Lib/test dir
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants