21.0.0 regression: FAILED tests/test_ssl.py::TestApplicationLayerProtoNegotiation::test_alpn_call_failure

[mgorny]

The 21.0.0 release (and git main) fail the following test:

FAILED tests/test_ssl.py::TestApplicationLayerProtoNegotiation::test_alpn_call_failure - Failed: DID NOT RAISE <class 'OpenSSL.SSL.E...

Does it perhaps rely on specific OpenSSL version or distribution patches?

Full log: tox.txt; excerpt below.

$ tox -e py39
GLOB sdist-make: /tmp/pyopenssl/setup.py
py39 create: /tmp/pyopenssl/.tox/py39
py39 installdeps: coverage>=4.2
py39 inst: /tmp/pyopenssl/.tox/.tmp/package/1/pyOpenSSL-21.1.0.dev0.zip
py39 installed: attrs==21.2.0,cffi==1.14.6,coverage==5.5,cryptography==3.4.8,flaky==3.7.0,iniconfig==1.1.1,packaging==21.0,pluggy==1.0.0,pretend==1.0.9,py==1.10.0,pycparser==2.20,pyOpenSSL @ file:///tmp/pyopenssl/.tox/.tmp/package/1/pyOpenSSL-21.1.0.dev0.zip,pyparsing==2.4.7,pytest==6.2.5,six==1.16.0,toml==0.10.2
py39 run-test-pre: PYTHONHASHSEED='2454934686'
py39 run-test: commands[0] | openssl version
OpenSSL 1.1.1l  24 Aug 2021
py39 run-test: commands[1] | coverage run --parallel -m OpenSSL.debug
pyOpenSSL: 21.1.0.dev
cryptography: 3.4.8
cffi: 1.14.6
cryptography's compiled against OpenSSL: OpenSSL 1.1.1l  24 Aug 2021
cryptography's linked OpenSSL: OpenSSL 1.1.1l  24 Aug 2021
Python's OpenSSL: OpenSSL 1.1.1l  24 Aug 2021
Python executable: /tmp/pyopenssl/.tox/py39/bin/python
Python version: 3.9.7 (default, Sep  1 2021, 06:25:44) 
[GCC 11.2.0]
Platform: linux
sys.path: ['/tmp/pyopenssl', '/usr/lib/python39.zip', '/usr/lib/python3.9', '/usr/lib/python3.9/lib-dynload', '/tmp/pyopenssl/.tox/py39/lib/python3.9/site-packages']
py39 run-test: commands[2] | coverage run --parallel -m pytest -v
========================================================= test session starts =========================================================
platform linux -- Python 3.9.7, pytest-6.2.5, py-1.10.0, pluggy-1.0.0 -- /tmp/pyopenssl/.tox/py39/bin/python
cachedir: .tox/py39/.pytest_cache
OpenSSL: b'OpenSSL 1.1.1l  24 Aug 2021'
cryptography: 3.4.8
rootdir: /tmp/pyopenssl, configfile: setup.cfg, testpaths: tests
plugins: flaky-3.7.0
[...]

============================================================== FAILURES ===============================================================
_____________________________________ TestApplicationLayerProtoNegotiation.test_alpn_call_failure _____________________________________

self = <tests.test_ssl.TestApplicationLayerProtoNegotiation object at 0x7f48b5facb80>

    def test_alpn_call_failure(self):
        """
        SSL_CTX_set_alpn_protos does not like to be called with an empty
        protocols list. Ensure that we produce a user-visible error.
        """
        context = Context(SSLv23_METHOD)
        with pytest.raises(Error):
>           context.set_alpn_protos([])
E           Failed: DID NOT RAISE <class 'OpenSSL.SSL.Error'>

tests/test_ssl.py:1937: Failed
========================================================== warnings summary ===========================================================
.tox/py39/lib/python3.9/site-packages/_pytest/config/__init__.py:1233
  /tmp/pyopenssl/.tox/py39/lib/python3.9/site-packages/_pytest/config/__init__.py:1233: PytestConfigWarning: Unknown config option: strict
  
    self._warn_or_fail_if_strict(f"Unknown config option: {key}\n")

tests/test_crypto.py:39
  /tmp/pyopenssl/tests/test_crypto.py:39: DeprecationWarning: PKCS#7 support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
    from OpenSSL.crypto import PKCS7, load_pkcs7_data

tests/test_crypto.py:40
  /tmp/pyopenssl/tests/test_crypto.py:40: DeprecationWarning: PKCS#12 support in pyOpenSSL is deprecated. You should use the APIs in cryptography.
    from OpenSSL.crypto import PKCS12, load_pkcs12

tests/test_ssl.py::TestContext::test_set_cipher_list[hello world:AES128-SHA1]
  /tmp/pyopenssl/tests/test_ssl.py:506: DeprecationWarning: str for cipher_list is no longer accepted, use bytes
    context.set_cipher_list(cipher_string)

tests/test_ssl.py::TestConnection::test_client_set_session
  /tmp/pyopenssl/tests/test_ssl.py:2685: DeprecationWarning: str for buf is no longer accepted, use bytes
    ctx.set_session_id("unity-test")

-- Docs: https://docs.pytest.org/en/stable/warnings.html
===Flaky Test Report===

test_gmtime_adj_notBefore passed 1 out of the required 1 times. Success!
test_gmtime_adj_notAfter passed 1 out of the required 1 times. Success!
test_set_cipher_list_no_cipher_match passed 1 out of the required 1 times. Success!

===End Flaky Test Report===
======================================================= short test summary info =======================================================
FAILED tests/test_ssl.py::TestApplicationLayerProtoNegotiation::test_alpn_call_failure - Failed: DID NOT RAISE <class 'OpenSSL.SSL.E...
======================================== 1 failed, 524 passed, 2 skipped, 5 warnings in 12.68s ========================================
ERROR: InvocationError for command /tmp/pyopenssl/.tox/py39/bin/coverage run --parallel -m pytest -v (exited with code 1)
_______________________________________________________________ summary _______________________________________________________________
ERROR:   py39: commands failed

[mgorny]

(or on OpenSSL built with SSLv3 support?)

[reaperhulk]

Interesting -- we're not seeing this test failure in our CI, but looking more closely we don't actually have tests that run against 1.1.1-latest (e.g. we don't test pyOpenSSL against the binary wheel most cryptography users get). This is a shortcoming of our CI we should rectify and then we can investigate a bit more with why this is happening. @mgorny does this occur on 20.0.1? If you run the test suite there with cryptography 3.4.8 does it pass or fail this test?

[mgorny]

@mgorny does this occur on 20.0.1? If you run the test suite there with cryptography 3.4.8 does it pass or fail this test?

It does not but I think it's a shortcoming in 20.0.1. A quick bisect points to the following commit:

commit 614d6737d84294b038eead384100e2a7a65f717b
Author: Maximilian Hils <git@maximilianhils.com>
Date:   Wed Feb 17 20:06:26 2021 +0100

    Check return code of SSL_[CTX_]set_alpn_protos (#993)
    
    * check return code of SSL_CTX_set_alpn_protos, fix #992
    
    * paint it black!
    
    * fix line lengths as well :upside_down_face:

 CHANGELOG.rst      |  3 +++
 src/OpenSSL/SSL.py | 21 +++++++++++++++++++--
 tests/test_ssl.py  |  9 +++++++++
 3 files changed, 31 insertions(+), 2 deletions(-)

This kinda makes sense given that this commit added the test ;-).

[njsmith]

CC @mhils

So I looked at this a bit. Findings so far:

The reason the test was added was because formerly, pyopenssl forgot to check for errors after calling SSL_set_alpn_protos at all, so if it failed it was leaving garbage on the openssl error stack and causing problems later. So the motivation for the test was partly b/c empty ALPN lists were broken, and partly to exercise the new error-checking code.

In new versions of openssl, an empty protos list is now legal, and explicitly disables ALPN. So this is no longer raising an error. I don't know how to provoke an error from SSL_set_alpn_protos in these new versions -- they do add checking that the ALPN proto string is valid, which is nice, but our wrapper code always generates valid proto strings, so we can't use this to test our wrapper code's error handling.

So, I see two options:

Option 1: We could reject empty protocol lists up-front in our wrapper code.

• Advantage: our wrapper code would behave consistently across OpenSSL versions
• Disadvantage: We no longer have any test coverage for SSL_set_alpn_protos leaving an error on the error stack
• Disadvantage: We stop people from accessing OpenSSL's new "disable ALPN" functionality -- though idk if this is actually useful to anyone, esp since support is inconsistent.

Option 2: We could allow through empty lists like now, but adapt the test to do something like: pass in an empty and discard any error, and then verify that the openssl error stack is clean.

I'm inclined to go with option 1. The only real risk is if we regressed by accidentally deleting the _openssl_assert around the calls to SSL_[CTX_]set_alpn_protos and someone found a way to provoke an error in this function, and those both seem unlikely enough that it's fine.

[reaperhulk]

I vote for option 1 so let's go down that path for now. We can always expose disable ALPN as a separate API later and if we're concerned about useless cruft on the error stack we can use cryptography's consume_errors logic.