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.
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>
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
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)}')
```
this is another part of https://github.com/certbot/certbot/issues/10195
these changes were done automatically with the command:
```
ruff check --fix --extend-select UP006 --unsafe-fixes certbot-apache
```
Alternative implementation for #7908.
In this PR:
- set up ruff in CI (add to `tox.ini`, mark dep in `certbot/setup.py`)
- add a `ruff.toml` that ignores particularly annoying errors. I think
line length isn't actually necessary to set with this workflow since
we're not checking it but putting it there for future usage.
- either fix or ignore the rest of the errors that come with the default
linting configuration. fixed errors are mostly unused variables. ignored
are usually where we're doing weird import things for a specific reason.
something weird happened to the changelog in
https://github.com/certbot/certbot/pull/10319. a 4.2.0 entry was added
below the entry for `5.0.0 - main` despite 4.2.0 not having been
released. since it's sounding like we're expecting our next release to
be 4.2.0 and not 5.0, i merged these two changelog entries into one for
4.2.0
i also modified our setup.py files to use 4.2.0.dev0 instead of
5.0.0.dev0 altho this isn't strictly necessary because our release
script will automatically set all version numbers to whatever version we
give it on the command line before building the release
Part of #10183
> Option 4. Stop updating old files with security improvements. If
people want to be on old software they can but then they're not getting
the nice new things. We can either warn or not warn if we see people
using them, either on certbot install (what, who's installing new
certbot on these machines), new cert, cert renewal, or certbot update.
The second two would require code changes, I'm pretty sure. I don't
think we should warn too often because that's how we get people to
silence all output. This is a little weird because we don't usually keep
around deprecated things. We could also warn loudly and see if people
complain. Or do some sort of brownout.
This PR warns every time certbot is run. We could make it run less often
(only when a new config file is installed, probably), but that's a more
extensive code change, and honestly I think it's probably fine? But I
can change it.
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#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
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.
I don't love the `Any` in that `Callable`, but I can't find a way to fix
it. In practice, it's either going to be `str` or `None`, but we pass an
`options` that's typed as `List[str] | str | None`, and one of the
functions has a header with a strict `str`. I tried various unions of
things and it wasn't working and I decided it's not worth it.
```
$ mypy --strict certbot-apache/certbot_apache/_internal/configurator.py
Success: no issues found in 1 source file
```
We could also leave the `jose.JWKRSA` call as-is and add
`--implicit-reexport`, or explicitly export `JWKRSA` in `josepy`, but I
think just cleaning the calls up is nice.
```
$ mypy --strict certbot-apache/certbot_apache/_internal/tests/util.py
Success: no issues found in 1 source file
```
```
$ mypy --strict certbot-apache/certbot_apache/_internal/http_01.py
Success: no issues found in 1 source file
$ mypy --strict certbot-apache/certbot_apache/_internal/obj.py
Success: no issues found in 1 source file
```
PEP 526 says to declare types of unpacked tuples beforehand:
https://peps.python.org/pep-0526/#global-and-local-variable-annotations.
Could have just declared it in apache, but improved the acme return type
while I was at it.
Once again, `typing.Pattern` is deprecated in favor of `re.Pattern` so
changing that while parametrizing the type
in https://github.com/certbot/certbot/pull/9124 we had the problem of
certbot-nginx's `Addr.fromstring` method possibly returning None which
is not possible in the `Addr` method in the certbot base class or in
certbot-apache. we fixed this by telling mypy the common
`Addr.fromstring` method returns an `Optional[Addr]` (despite it
actually always returning an `Addr`) and then unnecessarily complicating
certbot-apache's code a bit. the need for extra complexity with this
approach is going even further in
https://github.com/certbot/certbot/pull/10151 where we have to use
`cast` to assure mypy that the type isn't actually `Optional`. i
personally don't like all this
```
$ mypy --strict certbot-apache/certbot_apache/_internal/parser.py
Success: no issues found in 1 source file
```
`typing.Pattern` is deprecated in python 3.9 in favor of using
`re.Pattern` directly, and also wants to be subscripted with its type.
`python-augeas` types can be found in
a1e84a7e58/augeas/__init__.py
`typing.Type` is deprecated in favor of built-in `type`. In strict
mode,`find_ancestors` needs to be more specific about what it actually
returns, due to covariance and generics and such.
```
$ mypy --strict certbot-apache/certbot_apache/_internal/dualparser.py
Success: no issues found in 1 source file
```
There are a quite a lot of imports that are unused.
F541 is Unnecessary f-interpolation without placeholders
E711 is incorrect use of == for boolean and None comparisons
## Pull Request Checklist
- [x] 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),
edit the `main` section of `certbot/CHANGELOG.md` to include a
description of the change being made.
- [ ] Add or update any documentation as needed to support the changes
in this PR.
- [x] Include your name in `AUTHORS.md` if you like.
---------
Co-authored-by: Mads Jensen <atombrella@users.noreply.github.com>
