i just updated this credential in CI and created a calendar event to
help us remember to update it
with the calendar event, i don't think we need the code comment here. it
wasn't updated last time and it's one less thing for us to remember to
do next year
i don't think this PR requires two reviews
with the changes i made to CI and our calendar, i think i can say this
fixes#10563
Updated the link to the Cloudflare API tokens page for accuracy.
## 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.
- [x] Include your name in `AUTHORS.md` if you like.
## 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.
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
https://github.com/pyparsing/pyparsing/blob/dc009668d8025b96522eb7503e2ef84c2be58843/docs/whats_new_in_3_0_0.rst?plain=1#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
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
Will Greenberg