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](https://github.com/certbot/certbot/blob/867b499f9b3934f88beffe53f021c6834350c2f1/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](https://github.com/certbot/certbot/blob/867b499f9b3934f88beffe53f021c6834350c2f1/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
this is the quickest somewhat sane fix for the current CI problem that i
could think of
the domain being used in the self-signed cert really seems irrelevant.
on my main test VPS, socket.gethostname returns "brad-certbot-dev". on
my laptop, it's "MacBookPro". i manually tested this branch a bit on my
VPS and nginx seems content
using a simple static string like this seems unlikely to break anything
to me and i think helps clearly identify where the self-signed cert is
coming from if ever causes a problem for anyone in the future
Several of these have been fixed, so let's update the requirement if
necessary and remove the warning catching.
`python-dateutil 2.9.0` was released Feb 29, 2024, so it's not widely
packaged in non-EOL major distros yet.
`pytest-cov 4.1.0` was released May 24, 2023.
Our pinned versions were already higher than these requirements.
Alternatively, we could just remove the warnings and not update the
minimum requirement, but I think it's nicer to note it in requirements
for anyone running our tests, like packagers.
We already require `poetry-plugin-export>=1.9.0`. `1.7.0` updated its
`requests-toolbelt` requirement to `>=1.0.0`, which is greater than the
minimum version needed to remove the warning.
It's a [drop-in
replacement](https://docs.astral.sh/uv/pip/compatibility/) that speeds
things up. I don't see any reason why not.
`--use-pep517` is [set by default](
https://docs.astral.sh/uv/pip/compatibility/#pep-517-build-isolation),
so we don't need it.
`--disable-pip-version-check` also does nothing on uv.
`uv` [uses
separate](https://docs.astral.sh/uv/pip/compatibility/#build-constraints)
`UV_BUILD_CONSTRAINT` and `UV_CONSTRAINT`. I just added it to both to do
the simplest thing here. We could split them.
We probably don't actually need to pipstrap pip anymore, I could take
that out.
What's happening with `parsedatetime` and `python-digitalocean` is that
they were always secretly wrong. Since `pip` compiles bytecode by
default, it was suppressing the errors. If you add the
`--compile-bytecode` flag to `uv`, it passes, but I don't think we
should do that. You can see the failure happen on main by passing
`--no-compile` to the pip args and running `certbot -r -e oldest`.
Now what I don't understand is that some places seem to say the `'\/'`
error from `parsedatetime` only started in python 3.12, whereas others
see it on earlier python. Perhaps pytest is vendorizing python or
something. Not too worried about that, needed to get updated anyway, and
it's an accurate oldest version based on our oldest OSes.
`python-digitalocean` is techincally newer than debian 11, but we've
made that decision before so it seems fine to me.
This uses the new `storage.atomic_rewrite` function to write only the
ARI-related value to the config file, leaving other values the same.
Updates `storage.atomic_rewrite` to handle the case where the
`"renewalparams"` section might be empty (which occurs in the new
unittests), and adds some comments.
Note: `atomic_rewrite` is mocked out as a no-op in
`test_resilient_ari_directory_fetches` and `test_renew_before_expiry`
because those tests have a simplistic config object on their
mock_renewable_cert that doesn't include a filename. If we try to write
the config in those tests we'd get an error (trying to write to `None`).
Since those tests aren't intended to test the "store and obey
Retry-After" behavior, I figured it's reasonable to skip it in the name
of testing; but of course, open to idea about the best way to navigate
this.
Part of #10355
Part of https://github.com/certbot/certbot/issues/10403
We were never actually updating the versions in certbot-ci and letstest.
Not that it really matters, but let's do that there as well.
Final part of https://github.com/certbot/certbot/issues/10403
I tested running `tools/snap/generate_dnsplugins_snapcraft.sh
certbot-dns-dnsimple` and it put the correct description in to the
`snapcraft.yaml` file.
Part of https://github.com/certbot/certbot/issues/10403.
As far as I can tell, "stick it in setup.py" is the official way of
handling complex dependencies. But since the version is static, we have
a little more choice here than we had with `certbot/pyproject.toml`.
We could put the version in the respective `pyproject.toml`s and read it
directly from the toml file with something like
[this](https://stackoverflow.com/a/78082561). Or otherwise load and
parse that file. The benefit of doing it that way is that all
non-certbot versions would be canonically in the `pyproject.toml`, and
also if we wanted we could use that same toml parsing to change the
version at release time instead of `sed`. I actually suspect `acme`,
`certbot-ci`, and `certbot-compatibility-test` will be the only ones
where we can completely delete `setup.py`, as the others all have
lockstep dependencies. (side note - we just never update `certbot-ci`
version. it's still set at `0.32.0.dev0`. there's no way this matters
but just noting.) I chose to do it this way instead because it seems
cleaner since we have to keep `setup.py` around anyway, but I don't have
a strong preference.
Based on what I've read, there's not actually a clean way to grab and
insert the version number within the toml file. This is due to [design
decisions](https://github.com/toml-lang/toml/issues/77) by the toml
authors. The clean `all` extras specification that we used in
`certbot/pyproject.toml` [seems to be an
outlier](https://github.com/pypa/setuptools/discussions/3627#discussioncomment-6476654)
because it's pip handling the self-reference, not toml.
This was causing oldest tests to fail on my mac, which has an open file
limit of 256. Locks were being released at exit, but there were more
than 256 tests being run at once. Holding onto the file descriptor for
temporary files was making us keep the files open.
I also removed unnecessary setUps and tearDowns in subclasses so that
this could be fixed in only one spot.
If you wanted to do any testing locally, I was throwing this in places:
```
import errno, os, resource
open_file_handles = []
for fd in range(resource.getrlimit(resource.RLIMIT_NOFILE)[0]):
try: os.fstat(fd)
except OSError as e:
if e.errno == errno.EBADF: continue
open_file_handles.append(fd)
print(f'location description: {len(open_file_handles)}')
```
Alternative to https://github.com/certbot/certbot/pull/10408/ and
https://github.com/certbot/certbot/pull/10415/ that fixes production
code for account meta and puts autouse fixtures in certbot and acme
tests. Overrides all `time.sleep` calls while we're at it.
Fixes the production code where it's simple/clean, and fixes the tests
for HTTPServer-based code because we just don't have that many mac users
using standalone.
The responsibility for atomic updates to a config file was previously
spread across different functions. This moves the atomic update to the
lowest level of the call graph.
Also, factor out the code that creates a ConfigObj based on various
inputs (archive dir, cert locations, and renewal params). This allows
cleanly reusing it across the "update" and "create new" paths.
Now the "create new config" code path doesn't have to do any renaming,
and the "update config" code path can assume the input file exists (and
error if it doesn't).
This will make it easier and cleaner to reuse the config-writing code in
#10377.
Part of #10355
this is the final part of
https://github.com/certbot/certbot/issues/10195. this fixes
https://github.com/certbot/certbot/issues/10195
the changes in the first commit were done automatically with the
command:
```
ruff check --fix --extend-select UP006 --unsafe-fixes
```
the second commit configures ruff to check for this to avoid regressions
thanks for bearing with me thru these somewhat large automatically
generated PRs ohemorange 🙏
