Repository navigation
Add san module - #10478
Add san module#10478
Conversation
Contains san.DNSName, san.IPAddress, and a parent class san.SAN.
This is the same validation as enforce_domain_sanity(), and the tests are copied as well.
|
Update: After writing down why I left validation of DNSName as a TODO, I decided to go ahead and try to do it. It actually winds up being not as disruptive as I thought in #10468, so I went ahead and incorporated it into this PR. Now both DNSName and IPAddress validate. Note this involves copying the test cases from
|
This copies acme.crypto_util.get_names_from_subject_and_extensions, returning the new `list[SAN]` type instead of `list[str]`. The tests are new to this PR. I couldn't find old tests for the existing functionality.
|
Another small update: based on changes in #10468 to restore compatibility, I concluded we also need |
ohemorange
left a comment
There was a problem hiding this comment.
couple nits to take or leave, a clarification, and one actual concern, but largely looking good!
This allows us to reuse the code from the function.
ohemorange
left a comment
There was a problem hiding this comment.
Note on return type of from_x509. Additionally, since get_identifiers_from_x509 is added here, is there a reason to keep the additional ip-grabbing code that's inside new_order, or can that just call the new function now? If it can, I'd prefer that be in here as well.
bmw
left a comment
There was a problem hiding this comment.
i have a couple of docstring nitpicks, otherwise, this lgtm!
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
if you dislike the general idea of this PR, feel free to just close it, but i'm scheduled to release the next version of certbot a week from today and i personally didn't like how [newsfragments](https://github.com/certbot/certbot/tree/main/newsfragments) is so empty despite us having done a lot of work on certbot lately this PR just adds a simple newsfragment highlighting/teasing the work jsha has been leading on support for IP address certificates which i imagine would be of interest to some people in the community ``` $ towncrier build --draft --version 5.2.0 Loading template... Finding news fragments... Rendering news fragments... Draft only -- nothing has been written. What is seen below is what would be written. ## 5.2.0 - 2025-11-25 ### Added - Support for Python 3.14 was added. ([#10477](#10477)) ### Changed - While nothing significant should have changed from the user's perspective, we've been doing a lot of internal refactoring in preparation for soon adding support for IP address certificates to Certbot. ([#10468](#10468), [#10478](#10478)) ```
see the thread at https://opensource.eff.org/eff-open-source/pl/f5yx4a4q4j8zjyqpmath494jge for details since it's only the `v5.2.0` github tag that's borked, we could in theory try and use like `v5.2.0-2` or something, but there are [places](https://github.com/certbot/certbot/blob/259dfadb43cc52cb06ba57883766497faaf72b59/.azure-pipelines/release.yml#L10) in the release pipeline that use the GH tag as input to the assets they build, so i think just skipping 5.2.0 altogether is simpler, easier, and safer with this change, here's the proposed changelog ``` $ towncrier build --draft --version 5.2.1 Loading template... Finding news fragments... Rendering news fragments... Draft only -- nothing has been written. What is seen below is what would be written. ## 5.2.1 - 2025-12-02 ### Added - Support for Python 3.14 was added. ([#10477](#10477)) ### Changed - While nothing significant should have changed from the user's perspective, we've been doing a lot of internal refactoring in preparation for soon adding support for IP address certificates to Certbot. ([#10468](#10468), [#10478](#10478)) ### Fixed - Removed `vhost_combined` and `vhost_common` log formats from included Apache configuration file. ([#9769](#9769)) - Due to a mistake on our end playing with GitHub's new [immutable releases](https://github.blog/changelog/2025-10-28-immutable-releases-are-now-generally-available/) feature that prevented our CI from uploading additional release assets, Certbot 5.2.0 was not and will not be uploaded to most platforms. Instead, that version number will be skipped and we'll go straight to 5.2.1. ([#10501](#10501))
Contains san.DNSName, san.IPAddress, and a parent class san.SAN.
Split out from #10468 as a standalone PR. To see examples of how it's intended to be used, please see that PR.
The constructor for DNSName incorporates the same validation done in
enforce_domain_sanity, and the tests fromenforce_domain_sanityare copied here as well. The goal is to deleteenforce_domain_sanityentirely as part of #10468.In support of #10346.