When making an ARI request, use the server listed in the cert's renewal
conf rather than the one passed in certbot's config.
Fixes#10339
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Another token of gratitude for a super useful tool and service.
More about codespell: https://github.com/codespell-project/codespell .
I personally introduced it to dozens if not hundreds of projects already
and so far only positive feedback.
CI workflow has 'permissions' set only to 'read' so also should be safe.
---------
Signed-off-by: Yaroslav O. Halchenko <debian@onerussian.com>
Fixes https://github.com/certbot/certbot/issues/10259
This PR moves post-hook execution from `main.renew` to
`renewal.handle_renewal_request` so that failed and renewed domains
actually get passed into post-hook execution as promised, even when
failures happened.
I suspect the original PR was being overly cautious by putting the whole
thing into a try/finally so that post-hooks definitely happen, but
`handle_renewal_request` is already full of exception catching. I
understand the worry about executing a pre-hook and then failing to
execute its matching post-hook, but the code really is already
structured to make sure that that won't happen. And then when we added
`FAILED_DOMAINS` and `RENEWED_DOMAINS`, we both kept that
overly-cautious hooks execution location, but also kept the error so we
have a summary at the end...which meant that if failures happened, the
env vars were never set.
If we really want to keep the `hooks.run_saved_post_hooks` call on the
outside of everything in main, we can, but then we will have to do one
of the following:
- pass in the output lists to be filled out during execution. not my
favorite pattern
- throw the output lists in the error object or make a wrapper error,
not sure, haven't looked at `errors.py` too closely
- stop raising that final error where we report failures at the very
bottom. it's a little outdated maybe but I do like it and I think people
are used to it
- raise that error in main, returning the number of parse and renewal
failures. this is my favorite of the options, but I still like it less
than what I've implemented here.
Here's the integration/regression test failing on main:
https://dev.azure.com/certbot/certbot/_build/results?buildId=9237&view=logs&j=fca58cec-e7ce-563a-f36f-5c233894d750
You can see here that that branch just has the integration test without
the fix (and removing other tests for efficiency):
https://github.com/certbot/certbot/compare/main...test-fail-env-on-main
It's the default, but just to be clear, this should definitely have two
reviewers.
certbot's standalone code contains confusing references to things like
`SSLSocket` which we were hoping to deprecate in
https://github.com/certbot/certbot/issues/10284. are they relevant?
they're sure not!
certbot's standalone plugin only supports HTTP-01 so comments about
things like `ACMETLSServer` and the completely unused `certs` variable
can be deleted
furthermore, the type of the different variables named things like
`http_01_resources` were wrong in multiple places. as can be seen in
certbot's standalone code, the type is
`Set[acme_standalone.HTTP01RequestHandler.HTTP01Resource]`. this is also
[the type used in acme.standalone's
tests](https://github.com/certbot/certbot/blob/723fe64d4dbc778f1e8fd0b93320c1a30dc6fa95/acme/src/acme/_internal/tests/standalone_test.py#L78-L81)
despite the file's type annotations saying it takes a different type. i
think the incorrect type annotations were never caught because mypy
can't fully make sense of our overly complex server classes here
finally, `from __future__ import annotations` was added to make [forward
references in type
annotations](https://mypy.readthedocs.io/en/stable/cheat_sheet_py3.html#forward-references)
easier
This is one solution to https://github.com/certbot/certbot/issues/10327.
It won't test an ARI check during a dry run, since it will just avoid
the mismatch problem by checking for dry run first and returning before
checking ARI. This PR will make the big error (actually a warning, but
red and scary) go away though.
I thought https://github.com/certbot/certbot/pull/9804/ was abandoned
but the author just missed my comment. I would like to accept that PR to
get it in, but in the process of updating the PR I wrote a nicer
changelog entry, so I would like to add that.
When a CA fails to issue a certificate after finalisation Certbot
currently prints the following unhelpful message:
```
An unexpected error occurred:
acme.errors.IssuanceError
```
This PR makes Certbot print the ACME error object from the order, as
such
```
An unexpected error occurred:
CAA error :: Invalid CAA: CAA prohibits issuance
```
## 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.
- [x] If the change being made is to a [distributed
component](https://certbot.eff.org/docs/contributing.html#code-components-and-layout),
edit the `master` section of `certbot/CHANGELOG.md` to include a
description of the change being made.
- [x] Add or update any documentation as needed to support the changes
in this PR.
- [x] Include your name in `AUTHORS.md` if you like.
Fixes#10312. This is perhaps overly detailed, but I was hoping that by
giving a viable path forward it would forestall requests to change it
back, add a flag to ignore ari, or otherwise change the behavior. Very
open to suggestions on wording/content/length/etc.
---------
Co-authored-by: Brad Warren <bmw@users.noreply.github.com>
Previously, we were constructing an ACME client for ARI checking that
used the global value for `server`, not the one recorded in a lineage's
renewal file.
This resulted in errors in the logs and failure to observe ARI for
lineages that used a non-default `--server` (e.g. staging or non-Let's
Encrypt CAs).
---------
Co-authored-by: ohemorange <ebportnoy@gmail.com>
[Recent changes](https://github.com/certbot/certbot/pull/10272/) to
`renewal.should_autorenew` assumed that if
`RenewableCert.configuration.renew_before_expiry` was set, that means
the user set it. That's wasn't true; we were throwing in a default value
if the user didn't set it. But there's no reason for that, especially
since we now set the default renewal time dynamically. Also, we were
writing out a commented `# renew_before_expiry = 30 days` without any
further documentation, in a file that we tell users they [shouldn't
really be
editing](https://eff-certbot.readthedocs.io/en/latest/using.html#modifying-the-renewal-configuration-file).
We now do neither of those things.
this is a follow up from https://github.com/certbot/certbot/pull/10286
and related to https://github.com/certbot/certbot/issues/10302
sorry i initially missed this! in #10286 our tests were just yelling at
me about the different augeas package needed, but python headers and a
compiler are also needed for things to work with an updated version of
python-augeas
i don't believe we need this change in our macOS instructions because:
1. homebrew doesn't split up python packages the way many linux distros
do. there is no equivalent python-dev package
2. if you're using homebrew, you already have a compiler because
[homebrew requires command line tools for
xcode](https://docs.brew.sh/Installation#macos-requirements)
3. "it works on my machine"
Follow-up to #10241. The acme module code is mostly the same, except the
switch to return a tuple containing Retry-After.
This includes the CLI-side work to call out to the new `renewal_time`
method when checking for renewal.
I moved `should_autorenew` from `storage.py` into `renewal.py`, where it
fits better (and also this solves an import cycle problem). To make the
edits more visible I split this into one commit for the move and [one
commit for the subsequent
edits](https://github.com/certbot/certbot/pull/10272/commits/4e137d9b00fb0a299723978d7a289b881716d36b#diff-fad906e31304c767d620bfd243f4c7adf1e63a3420fd634ee57a0f6651c182cf).
This does not yet attempt to store the Retry-After info, or failure
retries, in renewal configs. I figured since that's a pretty big chunk
of work and design on its own, I wanted to get interim feedback as is. I
think this PR would be okay to land with the current default crons /
systemd timers that run twice a day. I think we should implement storage
of retry information before increasing the frequency of runs. And if the
team would like to hold off on landing any ARI until that storage is
done, I'm good with that too. 👍🏻
Fixes#10266.
See example deprecation in
https://github.com/certbot/josepy/pull/207/files
I can add stacklevel=2, though I find that usually I just look at the
whole stack anyway when debugging, myself, so it doesn't really matter.
Fixes#10252.
See further discussion here: https://github.com/pypa/pip/issues/11457
We are doing option:
> Alternatively, enable the --use-pep517 pip option, possibly with
--no-build-isolation. The --use-pip517 flag will force pip to use the
modern mechanism for editable installs. --no-build-isolation may be
needed if your project has build-time requirements beyond setuptools and
wheel. By passing this flag, you are responsible for making sure your
environment already has the required dependencies to build your package.
Once the legacy mechanism is removed, --use-pep517 will have no effect
and will essentially be enabled by default in this context.
Major changes made here include:
- Add `--use-pep517` to use the modern mechanism, which will be the only
mechanism in future pip releases
- Change to `/src` layout to appease mypy, and because for editable
installs that really is the normal way these days.
- `cd acme && mkdir src && mv acme src/` etc.
- add `where='src'` argument to `find_packages` and add
`package_dir={'': 'src'},` in `setup.py`s
- update `MANIFEST.in` files with new path locations
- Update our many hardcoded filepaths
- Update `importlib-metadata` requirement to fix
double-plugin-entry-point problem in oldest tests
This PR adds `certbot-dns-cdmon` to the list of third-party plugins in
the documentation.
`certbot-dns-cdmon` enables DNS-01 challenge automation for domains
managed with cdmon's DNS.
PyPI: https://pypi.org/project/certbot-dns-cdmon/
Fixes
https://github.com/certbot/certbot/issues/9835#issuecomment-2717096178,
where our `RewriteEngine on` directive inserted at the beginning of a
virtualhost was overridden a `RewriteEngine Off` directive later. This
PR does the easy thing of placing `RewriteEngine on` in our
post-insert.
Previously we defaulted to renewing at 30 days before expiry, and
allowed users to customize the config file to set a different value.
Instead, we should renew when 1/3 of the lifetime is left, or for
shorter certificates (<10 days), when 1/2 of the lifetime is left.
This still allows explicitly configured values to take precedence.
---------
Co-authored-by: Will Greenberg <ifnspifn@gmail.com>
