Deprecate `X509Extension` and `CRL` APIs

facutuesca commented 1 year ago

These have better alternatives in cryptography, and users should be pointed to use them instead.

Here is a list of packages that depend on pyOpenSSL, along with their importance to the ecosystem (number of direct+indirect dependents), current use of these APIs and places where pyOpenSSL is being used.

Format:

[x] package-name (number of dependents)
- Does it use either X509Extension or CRL
- List of places in its codebase where it uses pyOpenSSL

Dependents:

[x] twisted (121150)
- Does not use either API
- Uses library here, here, here, here, here and here
[x] aioquic (74820)
- Does not use either API
- Uses library here
[x] uvloop (67627)
- Does not use either API
- Uses library here (only for testing)
[x] asyncssh (60418)
- Does not use either API
- Uses library here
[x] pydap (54982)
- Does not use either API
- Does not use library, but has it as a dependency. PR to fix opened here
[ ] service-identity (25975)
- :warning: X509Extension API used here
- Uses library here
[x] pymongo (25627)
- Does not use either API
- Uses library here and here
[x] redis (22975)
- Does not use either API
- Uses library here
[x] azureml-core (6874)
- Source code not available
[x] cherrypi (2553)
- Does not use either API
- Uses library indirectly here through cheroot who uses it here
[ ] snowflake-connector-python (1194)
- :warning: X509Extension API used here
- Uses library here, here, here, here, here, here and here,
[x] pyexasol (511)
- Does not use either API
- Uses library here
[x] apache-airflow (495)
- Does not use either API
- Library does not seem to be currently used anywhere
[ ] mitmproxy (478)
- ⚠️ Uses X509Extension API here (in an example script)
- Uses library here, here, here, here, here, here, here, here, here, here, and here
[x] selenium-wire (424)
- Does not use either API
- Uses library indirectly through a vendored library (mitmproxy)
[x] scrapy (273)
- Does not use either API
- Uses library here, here, and here
[ ] certipy (135)
- ⚠️ Uses X509Extension API here and here
- Uses library here, here and here
[ ] python-glanceclient (108)
- ⚠️ Uses X509Extension API here
- Uses library here
[ ] acme (105)
- ⚠️ Uses X509Extension API here, here, here
- Uses library here, here, here, here, here, here, here, and here

facutuesca commented 1 year ago

Here's another list of important packages dependent on PyOpenSSL, this time sorted by # of downloads last month (I removed the packages already present in the previous list):

Format:

[x] package-name (number of downloads)
- Does it use either X509Extension or CRL
- List of places in its codebase where it uses pyOpenSSL

Dependents:

[x] urllib3 (388788102)
- Does not use either API
- Uses library here
[x] azure-cli-core (3788166)
- Does not use either API
- Uses library here, here, here, here, here and here
[x] apache-airflow-providers-google (1914071)
- Does not use either API
- Library does not seem to be currently used anywhere (https://github.com/urllib3/urllib3/blob/af7c78fa30f5a4e265911371d0c59b6baeddca0f/src/urllib3/contrib/pyopenssl.py)
[x] azure-servicemanagement-legacy (1427093)
- Does not use either API
- Uses library here
[x] pyvmomi (986291)
- Does not use either API
- Uses library here
[x] aws-sam-cli (871525)
- Does not use either API
- Uses library here
[x] auth0-python (740244)
- Does not use either API
- Library does not seem to be currently used anywhere
[x] pysaml2 (578310)
- Does not use either API
- Uses library here and here
[x] signxml (298553)
- Does not use either API
- Uses library here, here, here and here
[x] tinybird-cli (282228)
- Not open source (?)
[x] pydrive2 (228953)
- Does not use either API
- Library does not seem to be currently used anywhere
[x] pusher (221858)
- Does not use either API
- Does not use library directly
[x] httpie (218094)
- Does not use either API
- Does not use library directly

alex commented 1 year ago

I just realized there's at least one public API that relies on CRL: add_crl on X509Store. My recommendation would be to change that method to allow it to accept a pyca/cryptography CRL or a pyOpenSSL CRL. This gives users who rely on that method a deprecation-free path.

facutuesca commented 1 year ago

@alex To have add_crl accept a x509.CertificateRevocationList, we would need to convert it so that _lib.X509_STORE_add_crl() can take it. Currently, the logic for that is in CRL::from_cryptography() and _load_crl(), two functions that are in the set to be deprecated. Should we duplicate that logic in X509Store::add_crl, so that when those two are deprecated, add_crl() still works?

To put it in code, what we want is:

def add_crl(self, crl: Union["CRL", x509.CertificateRevocationList]) -> None:
    converted_crl = crl if isinstance(crl, CRL) else CRL.from_cryptography(crl)
    _openssl_assert(_lib.X509_STORE_add_crl(self._store, converted_crl._crl) != 0)

But since from_cryptography() (and _load_crl(), used by from_cryptography) are going to be deprecated, we would need to duplicate their logic in add_crl's definition

alex commented 1 year ago

I don't feel strongly about this.

On Wed, Sep 20, 2023 at 5:45 AM Facundo Tuesca @.***> wrote:

@alex https://github.com/alex To have add_crl accept a x509.CertificateRevocationList, we would need to convert it so that _lib.X509_STORE_add_crl() can take it. Currently, the logic for that is in CRL::from_cryptography() and _load_crl(), two functions that are in the set to be deprecated. Should we duplicate that logic in X509Store::add_crl, so that when those two are deprecated, add_crl() still works?

— Reply to this email directly, view it on GitHub https://github.com/pyca/pyopenssl/issues/1249#issuecomment-1727365344, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAAGBDD7T2MTR7JPWBUSJLX3K3NLANCNFSM6AAAAAA4T4HTW4 . You are receiving this because you were mentioned.Message ID: @.***>

-- All that is necessary for evil to succeed is for good people to do nothing.

facutuesca commented 1 year ago

I just realized there's at least one public API that relies on CRL: add_crl on X509Store. My recommendation would be to change that method to allow it to accept a pyca/cryptography CRL or a pyOpenSSL CRL. This gives users who rely on that method a deprecation-free path.

@alex Here's the PR for that: https://github.com/pyca/pyopenssl/pull/1252

pyca / pyopenssl