as i mentioned at
https://github.com/certbot/certbot/pull/10509#discussion_r2601282033, i
didn't love how the tests were using `_DomainsAction` when i think the
leading underscore suggests the class is internal to the cli/cl_utils
module
this PR fixes that
also, i don't think this PR requires two reviews
this fixes the dependabot alerts those with access can see at
https://github.com/certbot/certbot/security/dependabot
i don't think those alerts are particularly relevant to us, but i think
it's good for us to update anyway
Cherry-picks #10509 which fixes#10506. This will eventually make its
way into the 5.2.2 point release
Co-authored-by: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Fixes#10506.
When --webroot-path was specified multiple times, Certbot was erroring
with `DNSName SAN compared to non-SAN`. That's because, in the
_WebrootPathAction that builds `namespace.webroot_path`, we were passing
`domain` (type `san.DNSName`) as the keys. The other code that modifies
or accesses `namespace.webroot_path` expects the keys to be of type
`str`. In particular `webroot.Authenticator._set_webroots` does:
```python
for achall in achalls:
self.conf("map").setdefault(achall.domain, webroot_path)
```
Where `achall.domain` is a `str`.
Two existing unittests would have caught this: `test_multiwebroot` and
`test_webroot_map_partial_without_perform`. However, they faked out the
parsing of the `--domains` flag, and that faked out code was not updated
in #10468. Since this bug is caused by an interaction between the types
produced by the `--domains` flag and those produced by the
`--webroot-path` flag, the tests failed to catch the problem. I've
updated the tests and confirmed that they fail before the fix is
applied.
This field is optional to maintain backwards compatibility. Note that
`AnnotatedChallenge` inherits from `jose.ImmutableMap`, which has a
[check in
__init__](4b74747670/src/josepy/util.py (L125-L131))
that all slots are provided. That check would not allow us to do a
backwards-compatible addition, so I implemented an `__init__` for each
of these subclasses that fills the fields without calling the parent
`__init__`, and so doesn't hit an error when `identifier` is absent.
I chose to use `acme.messages.Identifier` rather than
`certbot._internal.san.SAN` here because these are wrapped ACME types,
so they should use the ACME representation. Also, `AnnotatedChallenge`
is passed to plugins, so we need to pass a type that the plugins can
understand.
Additionally, `domain` is marked as deprecated.
Part of #10346
/cc @bmw, who noticed the issue with `AnnotatedChallenge`
[here](https://github.com/certbot/certbot/pull/10468#issuecomment-3403294394)
and provided additional feedback
[here](https://github.com/jsha/certbot/pull/2#issuecomment-3534895793).
Note that there's still some work to do to finish excising `domain`
assumptions from this portion of the code.
---------
Co-authored-by: ohemorange <ebportnoy@gmail.com>
in https://github.com/canonical/snapcraft/pull/5720, snapcraft made a
change. `snapcraft status certbot` output changed from something like
this:
```
Track Arch Channel Version Revision Progress
latest amd64 stable 5.1.0 5057 -
candidate ↑ ↑ -
beta 5.2.1 5214 -
edge 5.2.0.dev0 5210 -
arm64 stable 5.1.0 5058 -
candidate ↑ ↑ -
beta 5.2.1 5215 -
edge 5.2.0.dev0 5211 -
armhf stable 5.1.0 5056 -
candidate ↑ ↑ -
beta 5.2.1 5213 -
edge 5.2.0.dev0 5212 -
```
to this:
```
Track Arch Channel Version Revision Progress
latest amd64 stable 5.1.0 5057 -
latest amd64 candidate ↑ ↑ -
latest amd64 beta 5.2.1 5214 -
latest amd64 edge 5.2.0.dev0 5210 -
latest arm64 stable 5.1.0 5058 -
latest arm64 candidate ↑ ↑ -
latest arm64 beta 5.2.1 5215 -
latest arm64 edge 5.2.0.dev0 5211 -
latest armhf stable 5.1.0 5056 -
latest armhf candidate ↑ ↑ -
latest armhf beta 5.2.1 5213 -
latest armhf edge 5.2.0.dev0 5212 -
```
when its output is captured like it is in finish_release.py in the lines
above the code i'm modifying here
not matching on the beginning of lines makes this pattern a little less
strict, but based on the rest of the pattern and the output here, i
personally think this is fine
after carefully verifying this works with the current state of things, i
went ahead and finished the release with this change and it worked just
fine. instead, this PR proposes a way to fix things going forward
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](https://github.com/certbot/certbot/issues/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](https://github.com/certbot/certbot/issues/10468),
[#10478](https://github.com/certbot/certbot/issues/10478))
```
The Apache configuration `Include`d in automatically created
`[sitename]-le-ssl.conf` files was redefining the `vhost_combined`
`LogFormat`, but contrary to the comment before the redefinition, did
not include the virtual host server name in the log format. This is
particularly confusing because this redefinition is hard to find when
debugging logging issues, as log formats are not related to SSL/TLS
configuration, and the included configuration file is outside of
`/etc/apache2`.
Additionally, a `vhost_common` `LogFormat` was defined, but not used
anywhere.
The `LogFormat` directives were introduced in commit
68f85d9f1a. Several other directives that
do not directly pertain to configuring SSL/TLS were added in that
commit, and have gradually been removed over the years. This should be
the last such removal.
Delete the `LogFormat` directives from the Apache configuration files
(both old and current), and update the `ALL_SSL_OPTIONS_HASHES`.
Fixes#9769 File 'options-ssl-apache.conf' included in autocreated
'[sitename]-le-ssl.conf' has potentially problematic vhost_combined
LogFormat
Closes#10348.
## Pull Request Checklist
- [ ] The Certbot team has recently expressed interest in reviewing a PR
for this. If not, this PR may be closed due our limited resources and
need to prioritize how we spend them.
- [ ] If the change being made is to a [distributed
component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout),
add a description of your change to the `newsfragments` directory. This
should be a file called `<title>.<type>`, where `<title>` is either a
GitHub issue number or some other unique name starting with `+`, and
`<type>` is either `changed`, `fixed`, or `added`.
* For example, if you fixed a bug for issue number 42, create a file
called `42.fixed` and put a description of your change in that file.
- [ ] Add or update any documentation as needed to support the changes
in this PR.
- [ ] Include your name in `AUTHORS.md` if you like.
this pr is in response to https://words.filippo.io/compromise-survey/.
ohemorange and i read this late on a friday to (speaking for myself at
least) much panic as it has some very strong words to say about the
github actions trigger pull_request_target which we use. looking into
the issue more, i also found that the popular static analysis tool
[zizmor](https://github.com/zizmorcore/zizmor) flags any github actions
workflow that uses the pull_request_target trigger with the message:
```
error[dangerous-triggers]: use of fundamentally insecure workflow trigger
pull_request_target is almost always used insecurely
```
this only added to my concern
the general problem with pull_request_target is that it runs with
additional privileges (e.g. potential write access, access to secrets)
in an environment containing values that can be set by an attacker.
these values include things such as references to the arbitrary code
contained in the triggering pr and pr titles which have been used to
perform shell injection attacks. not carefully treating these values
like the untrusted data it is while executing code in the privileged
environment given to pull_request_target has resulted in many supply
chain attacks
that's not to say that pull_request_target CAN'T be used securely.
zizmor even has [an
issue](https://github.com/zizmorcore/zizmor/issues/1168) brainstorming
how to not warn about all uses of the trigger as some are clearly fine
and the only way to accomplish what the user wants. i'm going to argue
that our uses of the trigger are ok
looking through the links provided by filippo's blog and [zizmor's
docs](https://docs.zizmor.sh/audits/#dangerous-triggers), i think we can
break down attacks used against pull_request_target into roughly 2
categories:
1. shell injection: "Nx S1ingularity" and "Ultralytics" from filippo's
blog
2. checking out and running a PR's code: "Kong Ingress Controller" and
"Rspack" from filippo's blog and https://ptrpa.ws/nixpkgs-actions-abuse
from zizmor docs
i think none of our pull_request_target workflows have these problems.
none of them use a shell (the [zizmor
issue](https://github.com/zizmorcore/zizmor/issues/1168) i linked
earlier suggests that any pull_request_target workflow that uses a run
block should always be flagged as insecure). instead, our workflows just
call action-mattermost-notify which can be [pretty easily
audited](https://github.com/mattermost/action-mattermost-notify/blob/2.0.0/src/main.js)
(as all the other files in the repo are boilerplate). passing possible
attacker controlled values directly to an action written in another
language is one of the approaches for mitigating script injection
[recommended by
github](https://docs.github.com/en/actions/reference/security/secure-use#use-an-action-instead-of-an-inline-script).
our workflows also do not check out the triggering pr's code
despite all that, i took this opportunity to cleanup and harden things a
bit. i reduced the permissions for each workflow and confirmed they each
still work on my fork. i also pinned the mattermost action to an exact
version and added some inline documentation
with these changes, our github workflows trigger few to no
warnings/errors when checked with zizmor,
[octoscan](https://github.com/synacktiv/octoscan), and [openssf
scorecard](https://github.com/ossf/scorecard)
if this pr is approved, i'll make similar changes to our josepy repo
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
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.
this PR finally removes all uses of self-signed certificates from
certbot-nginx
i plan to create one last PR related to this deprecating
`acme.crypto_util.make_self_signed_cert` and moving the function to
certbot-compatibility-test which is the only place it's currently used
i think we could also do additional refactoring in certbot-nginx by
moving the _make_server_ssl call out of choose_or_make_vhost and make
deploy_cert responsible for calling it if the returned vhosts aren't
ssl. in this case, we could then skip updating cert and key directives a
second time as this is duplicate work if we just made the server ssl
i considered doing this, but it's a bigger refactor, breaks more tests,
and i'm not sure it really buys us much so i skipped it. i could take
this on or create an issue for it if you think it's important for us to
do for some reason tho ohemorange
this is another tiny piece of my nginx refactoring. with
https://github.com/certbot/certbot/pull/10455, this function is now
never called outside of tests with `create_if_no_match=False` so this PR
removes the unnecessary parameter
luckily, tests still Just Work™ with this change
Fixes https://github.com/certbot/certbot/issues/6180.
New output:
```
--deploy-hook DEPLOY_HOOK
Command to be run in a shell once for each successfully issued certificate, including on subsequent renewals. Unless --disable-hook-validation is
used, the command’s first word must be the absolute pathname of an executable or one found via the PATH environment variable. For this command, the
shell variable $RENEWED_LINEAGE will point to the config live subdirectory (for example, "/etc/letsencrypt/live/example.com") containing the new
certificates and keys; the shell variable $RENEWED_DOMAINS will contain a space-delimited list of renewed certificate domains (for example,
"example.com www.example.com") (default: None)
```
Pre and post hooks are still only shown in `renew` and `reconfigure`
help, though perhaps there is less confusion over those so it's not
necessary.
this is the first part of the nginx refactoring work i wanted to do.
ohemorange, if this conflicts with your work on updating our nginx ssl
config, please feel free to either ignore this for now or point me to
your branch after merging this and i can fix up any merge conflicts
myself like i previously offered
the main thing this PR does is create a new choose_or_make_vhosts
function with the current choose_vhosts behavior and makes choose_vhosts
only return existing ssl vhosts which i think is the behavior we want
when setting up HSTS or OCSP stapling. [this is what we do in
apache](867b499f9b/certbot-apache/src/certbot_apache/_internal/configurator.py (L1795-L1829)),
enabling HSTS or OCSP stapling on an HTTP vhost seems wrong/dangerous,
and since we don't have cert and key information in these [enhance
calls](867b499f9b/certbot/src/certbot/interfaces.py (L255)),
any SSL vhost we create will be left with snakeoil certs which also
seems very wrong
of course, this simple change to certbot-nginx's prod code required many
changes to its tests. the config file for headers.com was introduced
[here](https://github.com/certbot/certbot/pull/6068) specifically for
testing HSTS so i added the SSL configuration it needs to make work with
the choose_vhost changes. that then broke the IP tests for headers.com
that were added in https://github.com/certbot/certbot/pull/10145/ so i
created a new no-listens.com vhost for testing that
if this is merged, my plan in the next PR or two is to:
1. since choose_or_make_vhosts is now always called with
create_if_no_match=True in prod code, i plan to remove that variable,
make that the default behavior of the function, and fix up tests
2. then, since choose_or_make_vhosts is only called in deploy_cert, i
plan to pass the cert and key to it so it can be used in
_make_server_ssl instead of the dummy certs currently being used there
i could do more of this in this PR if you want ohemorange, but i figured
it rarely hurts to break things up especially when the code is kind of
tricky like it is (to me) here
