Skip to content

Add san module - #10478

Merged
bmw merged 8 commits into
certbot:mainfrom
jsha:add-sans
Nov 5, 2025
Merged

bmw merged 8 commits into
certbot:mainfrom
jsha:add-sans

Conversation

@jsha

@jsha jsha commented Oct 16, 2025 •

Copy link
Copy Markdown
Contributor

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 from enforce_domain_sanity are copied here as well. The goal is to delete enforce_domain_sanity entirely as part of #10468.

In support of #10346.

Contains san.DNSName, san.IPAddress, and a parent class san.SAN.
@jsha
jsha marked this pull request as ready for review October 16, 2025 21:54
@bmw
bmw requested review from a team and ohemorange and removed request for a team October 16, 2025 22:12
This is the same validation as enforce_domain_sanity(), and the tests are copied
as well.
@jsha

jsha commented Oct 16, 2025

Copy link
Copy Markdown
Contributor Author

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 EnforceDomainSanityTest in a way that is hard to diff. Here are the changes I made:

  • DNSName doesn't take bytes as input, so remove the case that passes .encode("utf-8").
  • Remove the u"" prefix on string literals, which lint tells me is not needed.
  • Add the -> None annotation on test cases, which mypy tells me is needed.

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.
@jsha

jsha commented Oct 21, 2025

Copy link
Copy Markdown
Contributor Author

Another small update: based on changes in #10468 to restore compatibility, I concluded we also need san.from_x509 to replace acme.crypto_util.get_names_from_subject_and_extensions, so I added it (plus some new tests) in this PR.

@ohemorange ohemorange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple nits to take or leave, a clarification, and one actual concern, but largely looking good!

Comment thread certbot/src/certbot/_internal/san.py Resolved
Comment thread certbot/src/certbot/_internal/san.py Resolved
Comment thread certbot/src/certbot/_internal/san.py Resolved
Comment thread certbot/src/certbot/_internal/san.py Resolved

@ohemorange ohemorange left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread certbot/src/certbot/_internal/san.py Resolved
ohemorange
ohemorange previously approved these changes Nov 3, 2025
@ohemorange
ohemorange requested a review from bmw November 3, 2025 21:59
@ohemorange ohemorange assigned bmw and unassigned ohemorange Nov 3, 2025

@bmw bmw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i have a couple of docstring nitpicks, otherwise, this lgtm!

Comment thread acme/src/acme/crypto_util.py Resolved
Comment thread certbot/src/certbot/_internal/san.py Resolved

@bmw bmw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! this lgtm!

@bmw
bmw merged commit ada2c54 into certbot:main Nov 5, 2025
ohemorange pushed a commit that referenced this pull request Nov 20, 2025
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 pushed a commit that referenced this pull request Nov 26, 2025
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))
```
ohemorange pushed a commit that referenced this pull request Dec 3, 2025
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))
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants