Repository navigation
Conversation
Documentation build overview
10 files changed ·
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #846 +/- ##
=======================================
Coverage ? 78.21%
=======================================
Files ? 41
Lines ? 4788
Branches ? 550
=======================================
Hits ? 3745
Misses ? 905
Partials ? 138 |
525daf9 to
0196097
Compare
3f80e15 to
12ae9d2
Compare
| try: | ||
| import OpenSSL.version | ||
| from cryptography.hazmat.primitives.serialization import ( | ||
| load_pem_private_key, |
There was a problem hiding this comment.
Let's try introducing new things as private from now on.
| load_pem_private_key, | |
| load_pem_private_key as _load_pem_private_key, |
| :py:attr:`~cryptography.x509.Certificate.issuer` and | ||
| :py:attr:`~cryptography.x509.Certificate.subject` instead of the deprecated | ||
| :py:meth:`X509.get_issuer <pyopenssl:OpenSSL.crypto.X509.get_issuer>` and | ||
| :py:meth:`X509.get_subject <pyopenssl:OpenSSL.crypto.X509.get_subject>`. |
There was a problem hiding this comment.
The newer syntax is
| :py:meth:`X509.get_subject <pyopenssl:OpenSSL.crypto.X509.get_subject>`. | |
| :external+pyopenssl:py:meth:`X509.get_subject <OpenSSL.crypto.X509.get_subject>`. |
| invalid private key password now raises ``ValueError`` or ``TypeError`` | ||
| instead of :py:exc:`OpenSSL.SSL.Error <pyopenssl:OpenSSL.SSL.Error>`. | ||
| The server certificate's issuer and subject DNs are now read via | ||
| :py:attr:`~cryptography.x509.Certificate.issuer` and |
There was a problem hiding this comment.
Why didn't you scope other foreign objects to their sites too?
| Stopped using several of :py:mod:`pyOpenSSL <pyopenssl:OpenSSL>`'s deprecated | ||
| APIs in the :py:class:`~cheroot.ssl.pyopenssl.pyOpenSSLAdapter`, replacing | ||
| them with :pypi:`cryptography`'s own APIs per ``pyOpenSSL``'s | ||
| `migration guidance <https://www.pyopenssl.org/en/latest/changelog.html>`_ |
There was a problem hiding this comment.
Isn't there a way to link this via intersphinx?
webknjaz
left a comment
There was a problem hiding this comment.
I somehow didn't realize we still didn't have pyopenssl in the optional dependencies in metadata. Probably for historic reasons. Might need to see if we ever said what the lowest supported was and mentioned bumping it in the changelog. Because we're testing quite new things in the CI..
Also, does this bring us closer to dropping old Python versions? It's probably time. They aren't particular burden at the moment but will be later. This is something to consider around Oct as Python 3.15.0 final is coming.
I haven't yet looked through the diff closely. Only made a few notes on the surface. Will have to look again later.
e2f4f75 to
775fa4a
Compare
|
Confirmed the optional-dependency gap is pre-existing. But this change needs
|
775fa4a to
475acee
Compare
475acee to
4548f63
Compare
pyOpenSSL 26.3.0+ emits DeprecationWarning for Context.set_passwd_cb, X509.get_issuer, and X509.get_subject -- our own filterwarnings=error config is what turns those into hard test failures, not pyOpenSSL itself. Add filterwarnings entries for the exact messages instead of the real migration tracked in cherrypy#846, as a lower-effort stopgap to keep the suite green against newer pyOpenSSL releases. Verified against a real pyOpenSSL 26.4.0 install (the lockfile-pinned 25.3.0 doesn't trigger these yet): the full suite genuinely fails without the filters (30 failures) and passes clean with them (200 passed, 0 failures).
pyOpenSSL 26.3.0 deprecated several OpenSSL.crypto/OpenSSL.SSL APIs in favor of `cryptography`'s own: Context.set_passwd_cb (recommending decrypting private keys with `cryptography` directly and passing the already-decrypted key to Context.use_privatekey instead), plus X509.get_issuer and X509.get_subject (recommending cryptography.x509's Certificate.issuer/.subject instead). Removed the verify-twice callback test: it tested an OpenSSL-callback quirk (being asked to confirm a password twice) that no longer applies once decryption happens directly, with no callback involved. Ref: pyOpenSSL changelog, version 26.3.0 (2026-06-12) https://www.pyopenssl.org/en/latest/changelog.html
The previous commit stops calling Context.set_passwd_cb, X509.get_issuer, and X509.get_subject, so these three filterwarnings entries (added as a stopgap in cherrypy#852) never match anything anymore.
4548f63 to
9aa44f4
Compare
| portend | ||
|
|
||
| pyopenssl | ||
| # >= 24.3.0 for accepting a `cryptography` key directly in |
There was a problem hiding this comment.
@julianz- side idea for a standalone effort: should we maybe have an additional constraint file similar to tests-constraints.in plugged in but with == pinned to the minimal supported versions for the lowest supported Pythons or something like this to produce better coverage matrix? The motivation is along the lines of pypa/pip-tools#2464.
| <OpenSSL.SSL.Context.use_privatekey>`, instead of using the deprecated | ||
| :external+pyopenssl:py:meth:`Context.set_passwd_cb | ||
| <OpenSSL.SSL.Context.set_passwd_cb>` callback; a wrong or invalid private key | ||
| password now raises ``ValueError`` or ``TypeError`` instead of |
There was a problem hiding this comment.
These are actually documented as exceptions in the stdlib docs so we can link them like so:
| password now raises ``ValueError`` or ``TypeError`` instead of | |
| password now raises :exc:`ValueError` or :exc:`TypeError` instead of |
| # >= 24.3.0 for accepting a `cryptography` key directly in | ||
| # `Context.use_privatekey()` -- older versions raise `TypeError: pkey | ||
| # must be a PKey instance`. | ||
| pyopenssl >= 24.3.0 |
There was a problem hiding this comment.
we should probably expose this as an extra but that's a separate discussion too
There was a problem hiding this comment.
Sure - sounds like a good idea.
| with open(self.private_key, 'rb') as key_file: | ||
| key_data = key_file.read() |
There was a problem hiding this comment.
Since supporting very old Pythons is no longer a concern, let's use pathlib APIs in new code. Also, pass encoding explicitly as this would get warnings/errors with python -bb.
Use pathlib.Path.read_bytes() instead of open()/with for reading the private key, now that supporting old Pythons isn't a constraint on new code. Cross-reference TypeError/ValueError in the changelog fragment instead of plain backticks.
| # `Proc-Type: 4,ENCRYPTED` line (traditional OpenSSL format). | ||
| # Checking this directly avoids touching `private_key_password` | ||
| # (which may prompt interactively) for keys that don't need it. | ||
| if b'ENCRYPTED' not in key_data: |
There was a problem hiding this comment.
Provided that we expect this to start like
b'-----BEGIN ENCRYPTED PRIVATE KEY-----'
wouldn't it be safer to check for exactly that?
| if b'ENCRYPTED' not in key_data: | |
| if b'ENCRYPTED' != key_data[11:20]: |
What if the file is like 500GB big, and we'd be loading all that into memory to scan for the marker? Shouldn't that be asserted early, then?
There was a problem hiding this comment.
It seems there are two different formats for PEM keys. The newer PKCS#8 format which begins:
-----BEGIN ENCRYPTED PRIVATE KEY-----
And the older traditional format
-----BEGIN RSA PRIVATE KEY-----
Proc-Type: 4,ENCRYPTED
So we need to handle both. Either way, the code goes on to read the full key data. So although we could limit this first check, it won't save us from a full read.
There was a problem hiding this comment.
I revised the code a bit to bound the initial check.
| ) | ||
| password = password.encode('utf-8') | ||
| elif not isinstance(password, bytes): | ||
| password = b'' # invalid types can't decrypt; let it fail below |
There was a problem hiding this comment.
In the light of https://sshhole.com, do we really want to have a magic value here? Why not None, then?
There was a problem hiding this comment.
Although.. looking at this, I guess this is probably like an interactive user-typed empty password input. So it's probably fine.
There was a problem hiding this comment.
Good point. The old code used b'' to trigger an error for invalid types, but None also works with cryptography, raising the same error. One thing I wonder about: a caller who supplies a password that isn't a string or bytes would get a misleading error saying no password was given at all, even though they did supply one. This is not something this change introduces though and fixing it would require more extensive changes.
| | None, | ||
| /, | ||
| ) -> bytes: ... | ||
| def _load_private_key(self): ... |
There was a problem hiding this comment.
Good call:
PrivateKeyTypes from cryptography.hazmat.primitives.asymmetric.types
Checking the whole key for the "ENCRYPTED" substring could, in theory, false-match something coincidental in the base64 body instead of the actual header. There are two different places that marker can legitimately appear -- a PKCS#8 header, or a `Proc-Type: 4,ENCRYPTED` line in the traditional OpenSSL format -- but both always fit within the first 256 bytes, so checking only that much avoids the false-match risk. This doesn't reduce how much we read overall: the rest of the file is read right after regardless of the check's result, since _load_pem_private_key() needs the whole thing either way. It only narrows where we look for the marker itself.
b'' was carried over from the old pyOpenSSL callback, where OpenSSL's own protocol specifically treated an empty return value as "abort". That contract doesn't apply to the new code, which passes the value straight to cryptography's load_pem_private_key() instead. Tested empirically: that function raises the exact same TypeError for b'' and for None, so there's no reason to keep the empty-bytes value -- None says what's actually meant instead of relying on a value that only happened to also work.
_load_pem_private_key() returns cryptography's PrivateKeyTypes (a Union of the concrete key types -- RSA, DSA, EC, Ed25519, etc.), confirmed from cryptography's own bundled type stub.
What kind of change does this PR introduce?
What do these changes do?
pyOpenSSL26.3.0 deprecated severalOpenSSL.crypto/OpenSSL.SSLAPIs in favor ofcryptography's own:Context.set_passwd_cb(recommending decrypting private keys with cryptography directly and passing the already-decrypted key toContext.use_privatekey instead), plusX509.get_issuerandX509.get_subject(recommendingcryptography.x509's Certificate.issuer/.subjectinstead).Removed the verify-twice callback test: it tested an OpenSSL-callback quirk (being asked to confirm a password twice) that no longer applies once decryption happens directly, with no callback involved.
Ref: pyOpenSSL changelog, version 26.3.0 (2026-06-12) https://www.pyopenssl.org/en/latest/changelog.html
Are there changes in behavior for the user?
ValueErrororTypeError(fromcryptography) instead ofOpenSSL.SSL.Error.pyOpenSSLAdapter._password_callbackmethod was removed entirely (it's an internal implementation detail, but technically overridable by downstream subclasses).pyOpenSSLAdapterno longer emitspyOpenSSL's deprecation warnings forContext.set_passwd_cb,X509.get_issuer, orX509.get_subject, and avoids a further deprecation warning fromContext.use_privatekeytriggered when passed a wrappedpyOpenSSLPKeyobject by handing it thecryptographykey directly instead.SSL_SERVER_I_DN/SSL_SERVER_S_DNWSGIenviron values are now built fromcryptography's certificate attributes instead of parsing pyOpenSSL's string representation. Output values are unchanged. I added a regression test for this (test_pyopenssl_adapter_environ_server_dn) and verified it passes unmodified against the pre-fix code too (checked out the oldpyopenssl.py, installedpyOpenSSL 26.0.0- the last version before these methods were deprecated - and ran the new test against that combination), confirming this isn't just internally consistent with the new code, it matches the old output too.Is it a substantial burden for the maintainers to support this?
No - it reduces a maintenance burden because the fix removes a reliance on
pyOpenSSLAPIs that are already deprecated and will eventually be removed. It also followspyOpenSSL's own documented migration guidance, and doesn't add any new dependency (cryptographyis already pulled in bypyOpenSSLitself).Related issue number
Discovered while discussing #845: this deprecation is currently blocking #808 (a pip-tools dependency bump) from passing CI.
Checklist
Tip
New to open source contributions? [Making a lovely PR] is a great read.
and description in grammatically correct, complete sentences
docs/) and inline docstrings reflect the changes(see good commit messages)
(can be done once the PR number is known; alternatively, you can
use a related issue number if one exists)
code of conduct