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

Recert to fix test failures with newer Python versions #210

Merged
merged 2 commits into from
Nov 18, 2021

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Nov 18, 2021

Newer Python versions have stronger TLS defaults, and therefore reject
the certificates currently used by the test suite. Regenerate them
to fix most of the test failures.

This also requires bumping the protocol versions used to test min/max
version attributes, as Python no longer allows TLS<1.2.

Unfortunately, test_min_tls_version still fails and I wasn't able to
figure out a way to fix it.

Issue #192

@temoto
Copy link
Member

temoto commented Nov 18, 2021

That min_tls_version is giving me nightmares.

@mgorny
Copy link
Contributor Author

mgorny commented Nov 18, 2021

Well, I'm open to ideas. For the record, the test failure I get now is:

________________________________________________________ test_min_tls_version _________________________________________________________
tests/test_https.py:109: in test_min_tls_version
    http.request(uri)
.tox/py39/lib/python3.9/site-packages/httplib2/__init__.py:1725: in request
    (response, content) = self._request(
.tox/py39/lib/python3.9/site-packages/httplib2/__init__.py:1441: in _request
    (response, content) = self._conn_request(conn, request_uri, method, body, headers)
.tox/py39/lib/python3.9/site-packages/httplib2/__init__.py:1363: in _conn_request
    conn.connect()
.tox/py39/lib/python3.9/site-packages/httplib2/__init__.py:1155: in connect
    self.sock = self._context.wrap_socket(sock, server_hostname=self.host)
/usr/lib/python3.9/ssl.py:500: in wrap_socket
    return self.sslsocket_class._create(
/usr/lib/python3.9/ssl.py:1040: in _create
    self.do_handshake()
/usr/lib/python3.9/ssl.py:1309: in do_handshake
    self._sslobj.do_handshake()
E   ssl.SSLError: [SSL: SSLV3_ALERT_HANDSHAKE_FAILURE] sslv3 alert handshake failure (_ssl.c:1145)

During handling of the above exception, another exception occurred:
tests/test_https.py:112: in test_min_tls_version
    assert e.reason in ("UNSUPPORTED_PROTOCOL", "VERSION_TOO_LOW")
E   AssertionError: assert 'SSLV3_ALERT_HANDSHAKE_FAILURE' in ('UNSUPPORTED_PROTOCOL', 'VERSION_TOO_LOW')
E    +  where 'SSLV3_ALERT_HANDSHAKE_FAILURE' = SSLError(1, '[SSL: SSLV3_ALERT_HANDSHAKE_FAILURE] sslv3 alert handshake failure (_ssl.c:1145)').reason

@mgorny
Copy link
Contributor Author

mgorny commented Nov 18, 2021

(I'm sorry, I've accidentally pasted pypy3 version instead of CPython 3.9 as intended)

@temoto
Copy link
Member

temoto commented Nov 18, 2021

My best result was either it fails with all combantions of min/max check in tests or it passes without the fix applied. So I thought maybe the correct testing approach should be special Python+openssl build that allows all versions only to find that x >= min setting works correctly somewhere. So maybe that test isn't worth troubles and can be deleted. I think that's better than having test that doesn't catch problem.

@mgorny
Copy link
Contributor Author

mgorny commented Nov 18, 2021

Hmm, I think you should be actually able to override allowed SSL/TLS versions at runtime. I'm going to try if I can make it work via that.

@mgorny
Copy link
Contributor Author

mgorny commented Nov 18, 2021

Nope, sorry, no idea how to make it work. FWICS Python just issues SSL_CTX_set_min_proto_version() based on the hardcoded default, so it should be overrideable via setting minimum_version and maximum_version accordingly but for some reason I seem to get the version error from server init.

@temoto
Copy link
Member

temoto commented Nov 18, 2021

Yup. We have this code that looks like it calls Python stdlib properly and it's manually verified. Will rely on shoulders of upstream.

@mgorny
Copy link
Contributor Author

mgorny commented Nov 18, 2021

Do you want me to update the PR somehow or will you take it from here?

@temoto
Copy link
Member

temoto commented Nov 18, 2021

@mgorny i found another recert in CI fixing branch, it has additional step to automation in extracting serial. If you don't mind and this version works for you (tests pass on my pc)?

Copy link
Contributor Author

@mgorny mgorny left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, test_min_tls_version still fails for me but if it passes for you, then please merge. It's still a major improvement and I can live with skipping this one test on Gentoo.

Comment on lines +751 to +752
if x509 is None:
raise Exception("x509_serial requires package cryptography installed")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I follow the logic here but it seems that:

  • on Python 3.6+, missing cryptography will cause an ImportError above
  • on older versions of Python, cryptography won't be loaded at all and this will unconditionally cause the relevant tests to fail

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cryptography was added to requirements-test so will be installed during test

for older versions there is if x509 condition with fallback to hardcoded cert serial that will go away after dropping py2

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed test_min_tls_version

@temoto temoto merged commit f77f29c into httplib2:master Nov 18, 2021
@github-pages github-pages bot temporarily deployed to github-pages November 18, 2021 20:53 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants