Allow using an OpenSSL hashed directory for verification in X509Store #943
Conversation
orosam
force-pushed
the
x509-store-load-locations
branch
from
68ae03a
to
0f5e764
Compare
| (``bytes`` or ``unicode``). | ||
|
|
||
| :return: ``None`` if the locations were set successfully. | ||
| """ |
There was a problem hiding this comment.
What happens if both are NULL or both are provided?
There was a problem hiding this comment.
When both are None, the X509_STORE_load_locations() call will return 0, so an OpenSSL.crypto.Error will be raised. It's not very helpful, a ValueError might have been better, by checking the args explicitly. I did not want to diverge from OpenSSL.SSL.Context.load_verify_locations(), which behaves similarly, so I kept it this way. But since OpenSSL.SSL.Error is different from OpenSSL.crypto.Error anyway, I could add the check and raise a ValueError.
When both are provided, both "locations" will be used. Even better, load_locations can be called multiple times, to add more and more locations. Internally, OpenSSL just adds them to the store as X509_LOOKUPs. Should I update the docstring with this info?
There was a problem hiding this comment.
Since pyOpenSSL is generally a thin wrapper around OpenSSL's API I'm fine with it raising crypto.Error. If you could update the docstring here then I think this is pretty much good to go. Thanks!
There was a problem hiding this comment.
I've updated and also extended the docstring to mention that this method can be called multiple times, and added some tests.
One more question: I see that many methods have a ::versionadded line. Should I add one for load_locations()?
There was a problem hiding this comment.
(... and removed 2 empty lines in another force-push, sorry...)
There was a problem hiding this comment.
Yes a .. versionadded:: 20.0 would be appropriate.
tests/test_crypto.py
Outdated
| @@ -3884,6 +3949,38 @@ def test_get_verified_chain_invalid_chain_no_root(self): | |||
| assert exc.value.args[0][2] == "unable to get issuer certificate" | |||
| assert exc.value.certificate.get_subject().CN == "intermediate" | |||
|
|
|||
| @pytest.fixture | |||
| def ca_file(self, tmpdir): | |||
There was a problem hiding this comment.
I'm not particularly familiar with pytest's tmpdir fixture. Are these automatically cleaned up as part of the test suite?
There was a problem hiding this comment.
Yes, in a way. This fixture creates a unique temporary directory(*) for each test function invocation using it (so e.g. a new one for every parametric call), and performs a cleanup at the end of the pytest run. This cleanup keeps the temporary directories of the last 3 pytest runs, and deletes all others.
I admit I didn't give too much thought to this, as the tmpdir fixture is already used extensively in test_ssl.py, and indirectly via the tmpfile fixture defined in conftest.py.
(*) Technically the directory structure is like this: pytest_base_temporary_directory/per_pytest_run_temporary_directory/per_test_invocation_temporary_directory.)
There was a problem hiding this comment.
I had totally forgotten about the use in test_ssl because none of the maintainers of this project really like to think about it much 😂 . Thanks for the explanation anyway, this is fine.
orosam
force-pushed
the
x509-store-load-locations
branch
2 times, most recently
from
534fd5f
to
efbbdf5
Compare
Add X509Store.load_locations() to set a CA bundle file and/or an OpenSSL- style hashed CA/CRL lookup directory, similar to the already existing SSL.Context.load_verify_locations().
orosam
force-pushed
the
x509-store-load-locations
branch
from
efbbdf5
to
4739807
Compare
Changes: 20.0.1 (2020-12-15) ------------------- Backward-incompatible changes: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Deprecations: ^^^^^^^^^^^^^ Changes: ^^^^^^^^ - Fixed compatibility with OpenSSL 1.1.0. 20.0.0 (2020-11-27) ------------------- Backward-incompatible changes: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - The minimum ``cryptography`` version is now 3.2. - Remove deprecated ``OpenSSL.tsafe`` module. - Removed deprecated ``OpenSSL.SSL.Context.set_npn_advertise_callback``, ``OpenSSL.SSL.Context.set_npn_select_callback``, and ``OpenSSL.SSL.Connection.get_next_proto_negotiated``. - Drop support for Python 3.4 - Drop support for OpenSSL 1.0.1 and 1.0.2 Deprecations: ^^^^^^^^^^^^^ - Deprecated ``OpenSSL.crypto.loads_pkcs7`` and ``OpenSSL.crypto.loads_pkcs12``. Changes: ^^^^^^^^ - Added a new optional ``chain`` parameter to ``OpenSSL.crypto.X509StoreContext()`` where additional untrusted certificates can be specified to help chain building. `#948 <https://github.com/pyca/pyopenssl/pull/948>`_ - Added ``OpenSSL.crypto.X509Store.load_locations`` to set trusted certificate file bundles and/or directories for verification. `#943 <https://github.com/pyca/pyopenssl/pull/943>`_ - Added ``Context.set_keylog_callback`` to log key material. `#910 <https://github.com/pyca/pyopenssl/pull/910>`_ - Added ``OpenSSL.SSL.Connection.get_verified_chain`` to retrieve the verified certificate chain of the peer. `#894 <https://github.com/pyca/pyopenssl/pull/894>`_. - Make verification callback optional in ``Context.set_verify``. If omitted, OpenSSL's default verification is used. `#933 <https://github.com/pyca/pyopenssl/pull/933>`_ - Fixed a bug that could truncate or cause a zero-length key error due to a null byte in private key passphrase in ``OpenSSL.crypto.load_privatekey`` and ``OpenSSL.crypto.dump_privatekey``. `#947 <https://github.com/pyca/pyopenssl/pull/947>`_
Add
X509Store.load_locations()to set a CA bundle file and/or an OpenSSL-style hashed CA/CRL lookup directory.This exposes
X509_STORE_load_locations()from OpenSSL, similar to what the already existingSSL.Context.load_verify_locations()does forSSL_CTX_load_verify_locations().