Typing for get_values_for_type is incorrect

[reaperhulk]

cryptography/src/cryptography/x509/extensions.py

Lines 1392 to 1401 in 032a7f3

	def get_values_for_type(
	self, type: typing.Type[GeneralName]
	) -> typing.List[GeneralName]:
	# Return the value of each GeneralName, except for OtherName instances
	# which we return directly because it has two important properties not
	# just one value.
	objs = (i for i in self if isinstance(i, type))
	if type != OtherName:
	objs = (i.value for i in objs)
	return list(objs)

This is currently defined as returning a list of GeneralName objects, but it actually returns a list of strings or a list of OtherName objects (which is a type of GeneralName). However, typing.Union[typing.List[str], typing.List[OtherName]] causes mypy to get confused as well. We should figure out how to make this (and the other instances of this same signature in extensions.py) correct.

[reaperhulk]

Actually it's worse; GeneralName values can be the following:

• str
• Name
• ObjectIdentifier
• IPv4Address/Network, IPv6Address/Network

And of course this method also returns a list of OtherName objects as well.

[reaperhulk]

@mathiasertl do you have any ideas on sane typing here?

[mathiasertl]

First, let me note that it took me 10 minutes to find out why the function was not just returning a list of GeneralName instances. Of course I'm just one data point, but for me the function code was hard to read.

But of course, you're right about the bug. The situation is worse yet, because off course the same is also true for SubjectAlternativeName, IssuerAlternativeName and CertificateIssuer. What I found out in my projects as well, is that functions that take/return lot's of different types are all great until you try to typehint them...

I see two possibilities right now (but will try to think of others too):

Option ONE: We typehint the return value to List[Union[str, Name, ...]]. The absolute downside is of course that mypy wont know about get_values_for_type(DNSName) returning only strings.

A side note here: I don't yet get why Typing.Union[typing.List[str], typing.List[OtherName]] causes troubles. We had that same issue in #5897. I will investigate that further.

Option TWO: We could try to overload the function (sorry @alex ;-)) to something like that (not tried it out yet, just pseudo code):

@overload
def get_values_for_type(self, type: x509.IPAddress) -> Union[ipaddress.IPv4Address, ...]:
    ...


@overload
def get_values_for_type(self, type: x509.RegisteredID) -> x509.ObjectIdentifier:
    ...

...

def get_values_for_type(self, type: NOTE-1-BELOW) -> Union[ipaddress.IPv4Address, ...]:
    ....

NOTE-1: Maybe we can just give x509.GeneralName here, we have to try that out.

I'm also not sure how/if overloading works if we give one of the stubs type: GeneralName) -> str as a "catch all" overload. We'll have to try that out too.

[alex]

I think the overload approach is probably best. I'm pretty sure you'll need to use `typing.Type[x509.IPAddress]` though.

…

On Sat, Mar 6, 2021 at 2:51 PM Mathias Ertl ***@***.***> wrote: First, let me note that it took me 10 minutes to find out *why* the function was not just returning a list of GeneralName instances. Of course I'm just one data point, but for me the function code was hard to read. But of course, you're right about the bug. The situation is worse yet, because off course the same is also true for SubjectAlternativeName, IssuerAlternativeName and CertificateIssuer. What I found out in my projects as well, is that functions that take/return lot's of different types are all great until you try to typehint them... I see two possibilities right now (but will try to think of others too): Option ONE: We typehint the return value to List[Union[str, Name, ...]]. The absolute downside is of course that mypy wont know about get_values_for_type(DNSName) returning only strings. A side note here: I don't yet get why Typing.Union[typing.List[str], typing.List[OtherName]] causes troubles. We had that same issue in #5897 <#5897>. I will investigate that further. Option TWO: We could try to overload the function (sorry @alex <https://github.com/alex> ;-)) to something like that (not tried it out yet, just pseudo code): @overload def get_values_for_type(self, type: x509.IPAddress) -> Union[ipaddress.IPv4Address, ...]: ... @overload def get_values_for_type(self, type: x509.RegisteredID) -> x509.ObjectIdentifier: ... ... def get_values_for_type(self, type: NOTE-1-BELOW) -> Union[ipaddress.IPv4Address, ...]: .... NOTE-1: Maybe we can just give x509.GeneralName here, we have to try that out. I'm also not sure how/if overloading works if we give one of the stubs type: GeneralName) -> str as a "catch all" overload. We'll have to try that out two. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5889 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAAGBBM2MDAJAFLHW6QN2TTCKBTPANCNFSM4YQQZBRA> .

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

[mathiasertl]

There's also more underlying issues here, some we already discovered in other recent PRs:

mypy does not update the type of re-assigned variables. If I reveal_type() in the function as it is now:

        objs = (i for i in self if isinstance(i, type))
        reveal_type(objs)  # typing.Generator[GeneralName*, None, None]
        if type != OtherName:
            objs = (i.value for i in objs)
            reveal_type(objs)  # STILL typing.Generator[GeneralName*, None, None]

The solution here could be to simply return directly, instead. Update: That's why mypy didn't complain about return-typehint either.

I already tried a first (incomplete) type: Union[Type[OtherName], Type[DNSName], ...] and then mypy then doesn't get the type of the Generator anymore, so it's Generatory[Any, ...] after that. :-(

[mathiasertl]

Alright, I have some good news and some bad news.

The good news: Using overloading as described above (of course with Type, as @alex mentioned), works well. With this python code (comments are line numbers):

import ipaddress
  
from cryptography import x509
from cryptography.x509.oid import ObjectIdentifier

names = x509.GeneralNames(
    [   
        x509.DNSName("example.com"),
        x509.IPAddress(ipaddress.IPv4Address("127.0.0.1")),
    ]
)
reveal_type(names.get_values_for_type(x509.DNSName))  # 12
reveal_type(names.get_values_for_type(x509.UniformResourceIdentifier))  # 13
reveal_type(names.get_values_for_type(x509.RFC822Name))  # 14
reveal_type(names.get_values_for_type(x509.DirectoryName))  # 15
reveal_type(names.get_values_for_type(x509.RegisteredID))  # 16
reveal_type(names.get_values_for_type(x509.IPAddress))  # 17
reveal_type(names.get_values_for_type(x509.OtherName))  # 18

I get what I think we should get (right?):

test-5889.py:12: note: Revealed type is 'builtins.list[builtins.str]'
test-5889.py:13: note: Revealed type is 'builtins.list[builtins.str]'
test-5889.py:14: note: Revealed type is 'builtins.list[builtins.str]'
test-5889.py:15: note: Revealed type is 'builtins.list[cryptography.x509.name.Name]'
test-5889.py:16: note: Revealed type is 'builtins.list[cryptography.hazmat._oid.ObjectIdentifier]'
test-5889.py:17: note: Revealed type is 'builtins.list[Union[ipaddress.IPv4Address, ipaddress.IPv6Address, ipaddress.IPv4Network, ipaddress.IPv6Network]]'
test-5889.py:18: note: Revealed type is 'builtins.list[cryptography.x509.general_name.OtherName]'

The bad news: overloading is not inheritable, it seems. I tried moving this to a dedicated interface class, and it doesn't seem to work. I only found python/mypy#5146 which is going in that direction. In any case, I couldn't find a variant that does what it's supposed to do.

That means that the function declaration for get_values_for_type is a whopping 55 lines (without the actual implementation). And that is, once for every function that implements this.

You can view my changes here: https://github.com/mathiasertl/cryptography/tree/bugfix/issue-5889-typehint-get-values-for-types

[mathiasertl]

So bottom line: If you're fine with those changes (in general), I'll make a PR of that branch to start the detailed review process.

[alex]

I don't really see that we have any alternative. Please go ahead and send that PR. Thanks for working on this!

…

On Sun, Mar 7, 2021 at 6:00 AM Mathias Ertl ***@***.***> wrote: So bottom line: If you're fine with those changes (in general), I'll make a PR of that branch to start the detailed review process. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

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