on main if you run tools/pinning/current/repin.sh and run our unit
tests, they will fail due to new deprecation warnings from pyparsing.
the cause of these warnings is described at
dc009668d8/docs/whats_new_in_3_0_0.rst (L613-L708)
this PR fixes these warnings and updates our minimum required pyparsing
version to 3.0 where the new naming convention is available. i ran our
full test suite on the first commit here and it passed
i don't think it's worth trying to keep compatibility with pyparsing<3
unless we get a request for us to do so which i really doubt we will
A few largely unused functions/types have been deprecated in our effort
to remove our pyOpenSSL dependency:
* Deprecated: `certbot.crypto_util.get_sans_from_cert`
* Deprecated: `certbot.crypto_util.get_names_from_cert`
* Deprecated: `certbot.crypto_util.get_names_from_req`
* Deprecated: `certbot.crypto_util.import_csr_file` (and replaced by
`certbot.crypto_util.read_csr_file`)
* Deprecated: `acme.crypto_util.Format`
`read_csr_file` now always returns a PEM formatted CSR, since that's
what was happening in practice, and therefore lets us stop having to
return a `Format`, so we will be able to stop importing it.
first half of #10433
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Fixes #10518.
`tools/pinning/current/repin.sh` is not run; only pytest version is
updated. This is because `pypinning` had a bunch of syntax changes that
seem simply but I believe should be in a separate PR, which I think
should be done after this to collect all repin changes.
As discussed further in #10518, these issues were caused by pytest's
internalization of pytest-subtest, which had several implementation
changes.
To fix these, we simply no longer use subtest in the failing tests. The
test in acme is now parametrized instead, and the tests in apache only
ever had a single parameter.
To use parametrization in the acme test, I converted `DNSTest` from
unittest to pytest style, which was pretty straightforward. The only
note there is that while it would be nice to make `ec_secp384r1_key` a
fixture, you [can't use fixtures in
parameters](https://github.com/pytest-dev/pytest/issues/349). You could
use requests, but that seemed less clear and messier, because then you'd
be checking the value of the parameter and only sometimes loading it.
Could also make it a global variable, but that didn't really seem
necessary, as it's only called twice. Happy to consider other options,
not strongly tied to this one, just seemed nicest to me.
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
i'm creating this PR in response to
47c5c88fe1
where ohemorange manually deleted .DS_Store on jsha's PR
rather than doing that or having new certbot devs manually configure
their system to ignore these files, let's just do it for them
i don't think this PR requires two reviews
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__](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
