The manual plugin offers environment variables for its hook called
CERTBOT_DOMAIN and CERTBOT_ALL_DOMAINS. I added CERTBOT_IDENTIFIER and
CERTBOT_ALL_IDENTIFIERS, while keeping the old variables for backwards
compatibility. Certbot will pass IP addresses in the CERTBOT_DOMAIN
environment variable rather than erroring out.
Part of #10346
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
https://github.com/certbot/certbot/pull/10146 was supposed to do this,
but because of multiple code paths, it did not. This PR simplifies the
code by creating a single code path.
In particular:
- `hooks.renew_hook()` is removed. There are now only calls to
`hooks.deploy_hook()`, which is called during certonly, run, and renew,
and runs both cli and directory hooks.
- `cli_config.renew_hook` is removed. Both `--renew-hook` (hidden option
kept for backwards compatibility purposes and `--deploy-hook` now set
`cli_config.deploy_hook`, which is used internally. When either or both
flags are used multiple times, the last value is kept, which is the
argparse default.
- references to running a "renew hook" internally are changed to "deploy
hook"
- To maintain downgrade compatibility, `deploy_hook` is written out to
renewal config files as `renew_hook`. This is achieved by translating to
and from `renew_hook` in `storage.py` and changing
`renewal.STR_CONFIG_ITEMS` to contain `deploy_hook`.
This results in the following behavior changes:
- Directory hooks are now run when getting a new cert using certonly/run
- If someone set a renew hook on the cli using `--renew-hook`, it would
previously not be run when getting a new (non-renewed) cert, but now
will be. But this option is hidden and should no longer be used anyway.
- When using `certbot reconfigure`, if someone sets `--renew-hook`
certbot will now also ask if someone would like to do a test run of the
new hook, whereas before it would only do so for `--deploy-hook`.
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
IPv6 addresses in URLs should be enclosed in square brackets.
Note: I chose to fix this by parsing the identifier rather than changing
the method signature. The obvious choice to change the method signature
would be to take `messages.Identifier`. We could even do this backwards
compatibly by taking `str | messages.Identifier`. However, `messages`
imports `challenges`, so referencing `messages.Identifier` here would
require an import loop.
I also considered implementing a new method on, e.g.
messages.Authorization that would take a challenge as a parameter. But
this would be suboptimal because the `uri` method really is specific to
the http-01 challenge type, so it's nice to have it implemented only on
the relevant class.
Fixes#10423.
In
https://github.com/certbot/certbot/pull/10409#issuecomment-3180214385,
we noted that a comment in
`certbot-ci/src/certbot_integration_tests/certbot_tests/test_main.py:test_renew_with_ec_keys`
says:
> since ecdsa is now default, the integration test is not actually
testing "When running non-interactively, if --key-type is unspecified
but the default value differs to the lineage key type, Certbot should
keep the lineage key type." as it says in the comment. To fix that, it
should be initially created with rsa, not ecdsa.
That is no longer accurate, since the default key type changed to
ecdsda.
This PR adds a new test that does what the comment specifies, and
updates the comment to reflect that the existing test no longer does
that.
i don't think the conversation at
https://github.com/certbot/certbot/pull/10495#discussion_r2699618989 is
urgent/important enough to make a github issue for it, but i also feel
like it's worth keeping a link around in case any devs have problems
with this code in the future. i think there was some good ideas in there
i don't think this PR requires two reviews
this PR is a follow up to my comment at
https://github.com/certbot/certbot/pull/10495#discussion_r2683527484
i dislike that jsha chose a slightly different column limit than
towncrier which caused it to generate awkwardly short lines in our
changelog. i also dislike contributors having to remember/know about
towncrier's wrapping behavior when writing changelog entries. i'd like
us to find a way to not have to worry about this
i initially looked into trying to set towncrier's line length limit to a
much higher value, but after searching around
https://towncrier.readthedocs.io/en/stable/configuration.html, i don't
think it's configurable
instead, i'm proposing here that we remove the wrapping entirely.
wrapping was added in response to the discussion at
https://github.com/certbot/certbot/pull/10379#discussion_r2243799585.
every markdown viewer i'm aware of automatically nicely handles wrapping
of long lines in markdown. most also automatically merge manually
wrapped lines to wrap based on the user's display/settings. finally,
since the changelog is automatically edited, i think it'll be rare for
certbot devs to have to manually edit the file
because of all this, i think whether the newsfragment was manually
wrapped or not is largely irrelevant, we shouldn't worry about it, and
we should let people wrap or their newsfragments or not as they see fit
i suppose alternatively we could just let towncrier double/awkwardly
wrap entries since i'm making the case that the wrapping doesn't matter,
but i personally have a slight preference for this approach. let me know
what y'all think
this is a quick fix for the problem i noticed at
https://github.com/certbot/certbot/issues/10526 and that issue is
tracking fixing our tooling to avoid this problem in the future
i personally don't think this PR requires two reviews
this fixes https://github.com/certbot/certbot/issues/10462 and i
personally think this is a nice change worth trying
now when people open (non-draft) PRs, one of us will automatically be
assigned hopefully helping to both automate and speed up the review
process
i personally don't think this PR requires two reviews
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__](https://github.com/certbot/josepy/blob/4b747476703fe4fff1aaccd76ebe570698bbf4f0/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.
