make x509.name.NameAttribute use Generic to overload get_attributes_for_oid

[prauscher]

see #12056: As NameAttribute.value is specified as str | bytes, but only uses bytes with NameOID.X500_UNIQUE_IDENTIFIER, using NameAttribute.value and expecting a string (which is correct for all other cases) leads to typing-errors.

By using a overload on get_attribute_for_oid with a Generic this can be avoided. I used the Generic syntax as defined in PEP-484 - while Python 3.12 introduced support for PEP-659 syntax, this would void support for older python versions, which is probably not wanted (now).

Consider this PR a first draft - I ran ruff and tests on my machine against them, but am more than happy for your remarks.

[prauscher]

Okay, this is way harder than I thought initially. Subtyping ObjectIdentifier really is not my first choice, but for overload of get_attributes_for_oid there must be a way to differentiate X500_UNIQUE_IDENTIFIER from the rest, as Literal[...] did not work

[alex]

Hmm, Literal[OID_CONST] not working does really throw a wrench into this. I'm not a fan of needing to maintain the two sets of subclasses here :-/

[hoefling]

A Literal[NameOID.X500_UNIQUE_IDENTIFIER] would work though if NameOID would be an enum.

[alex]

By which you mean, if it subclassed enum.Enum? The challenge there is that we do want to allow passing arbitrary OIDs, so we can't just use NameOID as the type.

[hoefling]

Hmm yeah after trying it out I see what you mean @alex - one would have to drag NameOID | ObjectIdentifier around, and the worst part the NameOID literals would need to be exhausted in a separate overload, leading to smth like

    @typing.overload
    def get_attributes_for_oid(
        self,
        oid: Literal[NameOID.X500_UNIQUE_IDENTIFIER],
    ) -> list[NameAttribute[bytes]]: ...

    @typing.overload
    def get_attributes_for_oid(
        self,
        oid: Literal[NameOID.COUNTRY_NAME]
        | Literal[NameOID.COMMON_NAME]
         ... # every NameOID besides X500_UNIQUE_IDENTIFIER goes in the union here
        | ObjectIdentifier,
    ) -> list[NameAttribute[str]]: ...

    def get_attributes_for_oid(
        self, oid: NameOID | ObjectIdentifier
    ) -> list[NameAttribute[AnyStr]]:
        return [i for i in self if i.oid == oid]

Sadly there is no obvious way of telling mypy "the return type is special for this special enum value, use the general one for the rest".

[hoefling]

This is the better variant of what I mean, with better visibility on both issues:

    # overload for the special value
    @typing.overload
    def get_attributes_for_oid(
        self,
        oid: Literal[NameOID.X500_UNIQUE_IDENTIFIER],
    ) -> list[NameAttribute[bytes]]: ...

    # overload for the rest
    @typing.overload
    def get_attributes_for_oid(
        self,
        oid: Literal[
            NameOID.COUNTRY_NAME,
            NameOID.COMMON_NAME,
            NameOID.LOCALITY_NAME,
            NameOID.ORGANIZATION_NAME,
            NameOID.GIVEN_NAME,
            ... # every NameOID besides X500_UNIQUE_IDENTIFIER goes in the union here
        ],
    ) -> list[NameAttribute[str]]: ...

    # overload for the arbitrary OID
    @typing.overload
    def get_attributes_for_oid(
        self,
        oid: ObjectIdentifier,
    ) -> list[NameAttribute[AnyStr]]: ...

    # implementation
    def get_attributes_for_oid(self, oid):
        return [i for i in self if i.oid == oid]

[alex]

Here's an idea: What if we added the type parameter on `NameAttribute`, but had get_attribtues_for_oid always return `NameAttribute[bytes | str]`. Users could then `typing.cast(NameAttribyte[str], ...)` it as appropriate. This isn't as good as a full solution, but is better than the status quo where `.value` is always `str| bytes`.

…

On Mon, Dec 2, 2024 at 4:15 PM Oleg Höfling ***@***.***> wrote: This is the better variant of what I mean, with better visibility on both issues: # overload for the special value @typing.overload def get_attributes_for_oid( self, oid: Literal[NameOID.X500_UNIQUE_IDENTIFIER], ) -> list[NameAttribute[bytes]]: ... # overload for the rest @typing.overload def get_attributes_for_oid( self, oid: Literal[ NameOID.COUNTRY_NAME, NameOID.COMMON_NAME, NameOID.LOCALITY_NAME, NameOID.ORGANIZATION_NAME, NameOID.GIVEN_NAME, ... # every NameOID besides X500_UNIQUE_IDENTIFIER goes in the union here ], ) -> list[NameAttribute[str]]: ... # overload for the arbitrary OID @typing.overload def get_attributes_for_oid( self, oid: ObjectIdentifier, ) -> list[NameAttribute[AnyStr]]: ... # implementation def get_attributes_for_oid(self, oid): return [i for i in self if i.oid == oid] — Reply to this email directly, view it on GitHub <#12068 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBDSSPK7E3TZKDSMLID2DTEVZAVCNFSM6AAAAABSWUIWX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJSHA3DGNRQGU> . 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.

[prauscher]

I think the cleanest way would be to add a keyword-argument force_str: bool = False to get_attributes_for_oid. Iff set to True, get_attributes_for_oid would always return NameAttribute[str] instead of NameAttribute[str | bytes], but raise TypeError iff the special OID is used?

[prauscher]

I removed the ugly parts and produced a base plate for future discussion - and while I was on it, I did a rebase against main. The main discussion now probably is if this is the final form and typing.cast is expected by users, or if we add some overload, probably based of on a simple boolean like force_str like described in my last comment.

[alex]

I think this looks good to me, but @reaperhulk how do you feel?

[reaperhulk]

This approach seems fine to me, feel free to mark it ready if it is and we can get it merged 😄

[prauscher]

Just to propose the alternative option, I crafted c4a2a5f: This adds return_string to get_attributes_for_oid to allow mypy to detect the correct typing. Sure, detecting by oid would be way nicer, but as discussed here this has major implications we do not want to go now.

[reaperhulk]

I don't really like having kwargs whose sole purpose is to improve hinting -- while the other approach isn't ideal, it is an improvement and doesn't create arguments with no runtime purpose.

[alex]

I tend to agree

…

On Wed, Dec 4, 2024, 10:56 PM Paul Kehrer ***@***.***> wrote: I don't really like having kwargs whose sole purpose is to improve hinting -- while the other approach isn't ideal, it is an improvement and doesn't create arguments with no runtime purpose. — Reply to this email directly, view it on GitHub <#12068 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBHQMTTNX2F2MEOQJUT2D7FFLAVCNFSM6AAAAABSWUIWX2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKMJZGAZTIMJRGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[prauscher]

Thanks for your opinion - I agree with it not being the nicest solution, I tried to think about something like hashlib.md5(usedforsecurity=False), which is not used for typing but quite similar. I reverted back to the backplate and marked the PR as ready.

[alex]

Thank you!