Remove x509 ABCs

[alex]

In numerous places we only support the concrete classes, and there's no real use case for people to implement the ABCs themselves.

Therefore, we should just drop the ABCs, and replace them with the concrete base classes.

Tasks

Preview Give feedback

1. SignedCertificateTimestamp remove sct abc #11995
2. Certificate remove Certificate abc #11989
3. RevokedCertificate
4. CertificateRevocationList remove crl abc #11991
5. CertificateSigningRequest remove csr abc #11994
6. OCSPRequest remove OCSPRequest abc #11990
7. OCSPSingleResponse remove OCSPSingleResponse abc #11993
8. OCSPResponse remove OCSPResponse abc #11992

[TreavVasu]

Hi @alex
Is this up for taking?

How I plan to implement this:

Taking Certificate class as an example :

Link to class

class Certificate(metaclass=abc.ABCMeta):

Removing (metaclass=abc.ABCMeta) and adding it as new-style class
class Certificate:

Addressing the changes further:

Remove @abc.abstractmethod Decorators

then

Remove @abc.abstractproperty with @property

then
Retain Property Decorators (@Property) Where Needed

I am bit confused on how to Implement methods and properties here should return as here of find the linked function it sounds a bit ddumb I know but I think with little direction I can pull this off.

Let me know if I am missing something or wrong somewhere, I'll try to cover those cases again and revert back
Thanks for reading

[alex]

We'd be happy to take contributions for this yes!

In terms of what needs to be done: These classes need to be deleted, and the concrete implementations should be exposed with the same name, and then we have to make sure the mypy types work correctly for them.

(With teh small asterisk that RevokedCertificate is more complicated, and should be saved for last.)

[TreavVasu]

Thanks @alex
I'll try my best

Sure, I hope you can help me on places where RevokedCertificate gets too complex :)

[alex]

Please do each of these in separate pull requests, not one big one.

…

On Fri, Oct 18, 2024 at 5:09 PM treavvasu ***@***.***> wrote: Thanks @alex <https://github.com/alex> I'll try my best Sure, I hope you can help me on places where RevokedCertificate gets too complex :) — Reply to this email directly, view it on GitHub <#11437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBDZXJEXQ5O44EDYTMDZ4F2JNAVCNFSM6AAAAABMTCYXKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRTGIZTMNRYGE> . 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.

[TreavVasu]

Thanks I was about to ask for this too.
U saved me here XD

[abbra]

The removal of Certificate abc broke FreeIPA. We implement our shim on top of the x509.Certificate with additional logic.

[alex]

Can you link to that code?

…

On Fri, Nov 29, 2024, 10:05 AM Alexander Bokovoy ***@***.***> wrote: The removal of Certificate abc broke FreeIPA. We implement our shim on top of the x509.Certificate with additional logic. — Reply to this email directly, view it on GitHub <#11437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBD5XXZNVZ4H7LBXJHT2DB7CLAVCNFSM6AAAAABMTCYXKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBXHE4TMNBRGI> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[abbra]

https://github.com/freeipa/freeipa/blob/master/ipalib/x509.py#L91 and the issues are visible in this ticket: https://pagure.io/freeipa/issue/9708

[alex]

Looks like the proximate way this fails is that you cannot subclass Certificate. If we made that possible again, would it resolve the issue?

…

On Fri, Nov 29, 2024 at 11:54 AM Alexander Bokovoy ***@***.***> wrote: https://github.com/freeipa/freeipa/blob/master/ipalib/x509.py#L91 and the issues are visible in this ticket: https://pagure.io/freeipa/issue/9708 — Reply to this email directly, view it on GitHub, or unsubscribe. 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.

[abbra]

Yes, I think so. It is literally because python doesn't see a single flag in the tp_flags that allows to subclass.

[alex]

Can you test with #12077 and verify it works?

…

On Fri, Nov 29, 2024 at 12:03 PM Alexander Bokovoy ***@***.***> wrote: Yes, I think so. It is literally because python doesn't see a single flag in the tp_flags that allows to subclass. — Reply to this email directly, view it on GitHub <#11437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBENQWMZIXNGYXNX4432DCM5JAVCNFSM6AAAAABMTCYXKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMBYGE3DCOJXHE> . 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.

[abbra]

@alex does not work yet, although there is a progress:

# python3 test.py
Traceback (most recent call last):
  File "//test.py", line 4, in <module>
    cert = load_der_x509_certificate(bcert)
  File "/usr/lib/python3.13/site-packages/ipalib/x509.py", line 472, in load_der_x509_certificate
    return IPACertificate(
        crypto_x509.load_der_x509_certificate(data, backend=default_backend())
    )
TypeError: No constructor defined for IPACertificate

I built cryptography 43.0 with two additional patches: from the PR that removed abc for Certificate class and then from the PR #12077. Looking through the pyo3 documentation, it appears you need to define new constructor: https://pyo3.rs/v0.23.2/class#constructor

By default, it is not possible to create an instance of a custom class from Python code. To declare a constructor, you need to define a method and annotate it with the #[new] attribute. Only Python's new method can be specified, init is not available.

[alex]

So the challenge is that we don't want Certificate to have a constructor -- the only way to construct the concrete Certificate is the various public APIs.

[abbra]

I'm looking if we can replace inheritance by simply consuming x509.Certificate. We effectively do that ourselves in almost all places in FreeIPA anyway.

[abbra]

I created a PR freeipa/freeipa#7614 that moves FreeIPA to treat x509.Certificate as a resource within our class. There were few changes I needed to add to IPA code to handle that with PyCA prior to 44.0.0 but nothing we cannot sustain going forward.

We are going to run more comprehensive tests this week but I think the change to get inheritance back is not needed. I'll report back once we transitioned.

[alex]

Great, thank you!

…

On Mon, Dec 2, 2024 at 11:27 AM Alexander Bokovoy ***@***.***> wrote: I created a PR freeipa/freeipa#7614 <freeipa/freeipa#7614> that moves FreeIPA to treat x509.Certificate as a resource within our class. There were few changes I needed to add to IPA code to handle that with PyCA prior to 44.0.0 but nothing we cannot sustain going forward. We are going to run more comprehensive tests this week but I think the change to get inheritance back is not needed. I'll report back once we transitioned. — Reply to this email directly, view it on GitHub <#11437 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBHYB2H3GQHWJ7F2DGT2DSDANAVCNFSM6AAAAABMTCYXKSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJSGA2TAMBWGQ> . 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.

[reaperhulk]

That's great to hear -- thanks for working on this @abbra!