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

Deprecate X509Extension and CRL APIs #1249

Closed
14 of 20 tasks
facutuesca opened this issue Sep 11, 2023 · 5 comments · Fixed by #1255
Closed
14 of 20 tasks

Deprecate X509Extension and CRL APIs #1249

facutuesca opened this issue Sep 11, 2023 · 5 comments · Fixed by #1255

Comments

@facutuesca
Copy link
Contributor

facutuesca commented Sep 11, 2023

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:

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

Dependents:

@facutuesca facutuesca changed the title Deprecate X509.Extension and X509.CertificateRevocationList APIs Deprecate X509Extension and CRL APIs Sep 13, 2023
@facutuesca
Copy link
Contributor Author

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:

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

Dependents:

@alex
Copy link
Member

alex commented Sep 20, 2023

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
Copy link
Contributor Author

facutuesca commented Sep 20, 2023

@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
Copy link
Member

alex commented Sep 20, 2023 via email

@facutuesca
Copy link
Contributor Author

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: #1252

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 16, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants