Initial IP address support: use san.SAN types internally

[jsha]

In #10478 we added a san.SAN class, with two subclasses san.DNSName and san.IPAddress, so we can carry type information about identifiers through the Certbot code. This PR plumbs through those types in most Certbot-internal code. Note that this does not change the acme module, which uses messages.Identifier. It also tries to leave alone the code paths into plugins.

This does not add a CLI flag to request an IP address certificate. That will be in a followup PR.

Part of #10346

[ohemorange]

Took a look. What makes you say that combining and re-separating them will make the change more contained? It looks to me like this PR modifies each spot where domains might be in the first place, if only in the comments. From what I'm seeing, maybe just in storage.py having only the one names? Calling ipaddress.ip_address to separate them in the two times it's done isn't actually that bad, but I worry we'll forget to call it in new code in the future whereas for instance names (identifiers?) could combine two internal variables.

Actually, I'm tempted to suggest spinning up a few new types -- Domain = str, could even by automatically checked by our enforce domain sanity function, IPAddress or whatever as Union[ipaddress.IPv4Address, ipaddress.IPv6Address], both of which inherit from Identifier or whatever. Might be overengineering, but we've had bugs with ports being treated as int vs. str before, and this is literally what typechecking was made for. I think it's fine even if we have modify/add/deprecate public functions. What do you think?

[jsha]

What makes you say that combining and re-separating them will make the change more contained? It looks to me like this PR modifies each spot where domains might be in the first place, if only in the comments.

On first pass, it seemed daunting. In particular the public type signature of acme.crypto_util.make_csr is:

def make_csr(
    private_key_pem: bytes,
    domains: Optional[Union[set[str], list[str]]] = None,
    must_staple: bool = False,
    ipaddrs: Optional[list[Union[ipaddress.IPv4Address, ipaddress.IPv6Address]]] = None,

And I didn't relish propagating that last big type everywhere. However, as you point out, using some custom types and adding/modifying/deprecating public functions can make this easier. I can take another pass and see how this looks.

I'm thinking something like class SAN. I'd want to avoiding the use of the term Identifier, to avoid confusing with the JSON-typed ACME concept. Then subclasses DNSName and IPAddress, the latter of which would be a union of IPv4Address and IPv6Address.

[jsha]

As I start this, I'm remembering the other thing that seemed daunting: updating all the mock setup in all the tests seems likely to be a pretty big change. I'll see how it goes.

[jsha]

Alright, I added new types in san.py: san.SAN, san.DNSName, san.IPAddress. The changes wound up touching a lot of places, but I think they made good sense sense. And indeed, mypy helped my find a bunch of places that I didn't realize needed updating.

I wound up drawing a boundary at the interface with installer plugins: they still provide / receive strings, for now. We'll likely want to change that, but that seemed like a good place to stop.

I like the idea of making san.DNSName represent a syntactically-valid, lowercased, whitespace-stripped domain name. I didn't do that in this PR because there is error handling around invalid domain name inputs, and I want to make sure those errors are still handled in a place that gives a nice message to the user.

I'm marking this "ready for review" even though I still only have a single integration test for IP address certs. I'd like to get feedback on all the san.SAN changes before I base too much work on top of it.

[bmw]

ohemorange asked me to take a high level look at this so we could all get on the same page before we got deep into the weeds of review here

my biggest concerns are around API changes and trying to ensure that all references to things like "domains" are properly updated if the value can now be an IP address

API changes

for API changes, everything in certbot/src/certbot outside of the certbot/src/certbot/_internal directory is technically considered part of our public API. (the backstory here is in the early days of certbot, we didn't make a clear distinction between public and private, we'd occasionally change code that broke things for 3rd parties, so we drew a clear line largely based around what our own separately packaged plugins use and wrote up our backwards compatibility expectations for users.)

for this PR as it sits right now, all changes to files like crypto_util.py, display/ops.py, interfaces.py, and util.py should ideally be made in a backwards compatible way or with warnings about upcoming changes in behavior that we then implement in a new major version of certbot. i'm not sure the best way to handle this and many of the decisions will probably have to be made on a case by case basis

one idea that unfortunately may be kind of frustrating to consider now is continuing to keep SANs as a list of strings. right now, many of the changes to type signature of public APIs just change the return value of a function from list[str] to list[SAN]. based on things like our standalone plugin, it seems like if some of these strings are suddenly IP addresses, a good chunk of code will just continue to work. these values will also only be IP addresses instead of strings if the user requests IP certs. with these things in mind, i personally think we could get away with saying that the list of strings possibly containing an IP address now isn't a breaking change. do we want to:

a) continue to use a list of strings which may (greatly?) simplify the transition, allow any code that can continue to work without changes, and let users who try to use IP addresses report bugs to us or 3rd parties as they come up
b) make everyone preemptively change their code to handle the new SANs type

i personally lean towards (a). i think the end result of (b) is cleaner, but it feels like significantly more work to me and i'm not sure it's worth it

i think similar things could be done by not renaming names to sans on lineages. i kinda hate it, but a SAN is a subject alternative NAME anyway so it's not wrong

i'm very curious what the two of you think though. if y'all both prefer (b), i'm not against it, i'm just not looking forward to the migration involved here

updating variable names

with all that said about maybe trying to avoid unnecessary changes, i do basically want us to never have a variable named something like domain or domains that actually now refers to an IP address in our own code. i think that's just a bug waiting to happen

i think that currently happens in code like this where identifier.value is assumed to be a domain because it always was before. this value then gets passed through our plugin API either directly or as an achall which (sometimes) has domain attributes

i think we should fix all these up as part of this IP address work. unfortunately the way i'd probably do it is looking through all references in our acme and certbot code for the string "domain", but one of y'all may have a cleverer idea

@jsha, you probably already know this but just to make sure, i'd recommend waiting until the 3 of us are on the same page here before you do much more work implementing this

[jsha]

all changes to files like crypto_util.py, display/ops.py, interfaces.py, and util.py should ideally be made in a backwards compatible way or with warnings about upcoming changes in behavior that we then implement in a new major version of certbot. i'm not sure the best way to handle this and many of the decisions will probably have to be made on a case by case basis

Oop, yes, I got carried away and made incompatible changes to some functions in these files. Still, that was useful in terms of forcing errors when things called the old names / passed the old types.

If we go forward with the re-names / applying types, my proposal for these "externally facing" items would be to keep the new item with the new name (since most have been renamed), and restore the old one with the old name, marking it as deprecated. I think that'll be better than trying to keep old function names and adding optional / defaulted parameters. Defaulted parameters can wind up creating a lot of complexity that's hard to reason about, and then more difficult cleanup after a deprecation period (if we do ever clean up).

one idea that unfortunately may be kind of frustrating to consider now is continuing to keep SANs as a list of strings.

No worries about being frustrating. That's basically the first version of this PR, and we can revert to that commit easily. I knew when I set out on this type journey that the work was speculative.

if some of these strings are suddenly IP addresses, a good chunk of code will just continue to work

Yes, this is true. Although I was surprised about some of the places I stumbled on, like this sort function that sorts by second-level domain:

certbot/certbot/src/certbot/display/ops.py

Lines 129 to 137 in 4d5d5f7

	def _sort_names(FQDNs: Iterable[str]) -> list[str]:
	"""Sort FQDNs by SLD (and if many, by their subdomains)
	:param list FQDNs: list of domain names
	:returns: Sorted list of domain names
	:rtype: list
	"""
	return sorted(FQDNs, key=lambda fqdn: fqdn.split('.')[::-1][1:])

. Well, maybe that was the only one that actually depended on domain-name semantics.

b) make everyone preemptively change their code to handle the new SANs type

I think we're in agreement that we should not break the public API without a deprecation period. That requires some further diffs to this PR but nothing complicated.

For plugins and hooks, I've tried to keep the interfaces be strings containing domain names. The new san.SAN typing allows us to prevent IP addresses from reaching the plugins until we've decided how we want that to work.

i think similar things could be done by not renaming names to sans on lineages. i kinda hate it, but a SAN is a subject alternative NAME anyway so it's not wrong

Based on my proposal above I'd re-add RenewableCert.names, still returning a list of strings (containing both domain names and IP addresses) and keep RenewableCert.sans as the new thing. FWIW, renaming .names() was really helpful in making sure I found all the spots that called it and assumed domain names.

i do basically want us to never have a variable named something like domain or domains that actually now refers to an IP address in our own code.

The changes here to have a specific named type for "could be a domain name or an IP address" really helped a lot with this. I won't say I caught every instance (e.g. I missed that achall thing you pointed out), but I caught a lot more than I did with the first pass of this PR.

IMO, this is an argument in favor of using the new san.SAN type. If we're already taking the code churn of renaming so many variables (and function names), I'd like to also update type information to help us keep those renames straight. "Use mypy" is my best clever idea here. :D Also the __eq__ implementations in san.py that error on comparisons between san.DNSName and str helped catch a lot of things. Though probably mypy would have caught those first if my workflow had been "fix mypy, then fix tests" instead of "fix tests, then fix mypy".

[bmw]

If we go forward with the re-names / applying types, my proposal for these "externally facing" items would be to keep the new item with the new name (since most have been renamed), and restore the old one with the old name, marking it as deprecated. I think that'll be better than trying to keep old function names and adding optional / defaulted parameters.

i agree. this works for me!

i do basically want us to never have a variable named something like domain or domains that actually now refers to an IP address in our own code.

The changes here to have a specific named type for "could be a domain name or an IP address" really helped a lot with this. I won't say I caught every instance (e.g. I missed that achall thing you pointed out), but I caught a lot more than I did with the first pass of this PR.

that's helpful to know. after i found domain variables that now are now sometimes IPs that slipped thru, i just assumed it wasn't helping that much. i'm glad to hear that's wrong

in that case, if you and/or ohemorange are still up for going the route of the bigger cleanup which will almost certainly be nicer for us in the end, this high level approach sounds good to me

[jsha]

By the way, looking again at the example you pointed out, of AuthHandler and handle_authorizations / _choose_challenges, the reason adding the san.SAN type everywhere didn't catch those is that the internally-typed information from Certbot has passed through the acme module, losing its own types in favor of acme types. So perhaps a good takeaway is that the acme module also has some hidden assumptions about exclusively handling domain names, and those should be comprehensively addressed as well. If so, should it be as a precursor PR, follow-on, included in this PR, or completely independent?

[bmw]

i personally think it should be a separate PR if splitting it up isn't too difficult. i don't expect splitting it up to be too bad, but i could certainly be wrong

i don't have a strong preference on the ordering though

[bmw]

lgtm! let's get a 2nd reviewer in here

[bmw]

ohemorange, just in case it helps as it initially tripped me up a bit for whatever reason, i think our change to certbot/src/certbot/configuration.py is ok here because config.namespace.domains are now san.DNSName objects which already had their sanity enforced as part of their __init__ process

[ohemorange]

wow, appreciate you taking the time to do such thorough changes!

[ohemorange]

just noting that something you switch "names" to "sans" and sometimes you leave it as "names." I think it's fine either way, but noting it in case you had an opinion and didn't notice you missed some, or had a preference here. see _find_lineage_for_sans for more names-keeping.

[jsha]

In _find_lineage_for_sans, you're talking about this line?

    ident_names_cert, subset_names_cert = cert_manager.find_duplicative_certs(config, sans)

That makes sense; I mostly was updating function names that contained names to instead contain sans, when I updated those functions to have different parameter types or return values. Since these variables didn't have a type involving san.SAN I would have missed them.

I note that you say "more names-keeping", but I can't find the names-keeping on this line. Is there some?

[ohemorange]

Yes, that's the line I meant, there were others as well -- for example find_duplicative_certs keeps variables with names in them. config.allow_subset_of_names is another big one.

No, that's a mistype, this line is just what made me think of it. It should have said "some names-keeping."

Anyway, sounds like you didn't intend to change every instance of names to sans and just missed some accidentally, in which case I'm find with using names and sans largely interchangeably.

[ohemorange]

did you want to switch everything over to sans or nah? if not I'll go ahead and merge

[jsha]

I'm gonna leave remaining places that say "names" as-is, at least for now. Thanks for checking.