validate lineage name

[NiekPeeters]

Fixes #6127.

Added blacklist mechanism for lineage name that avoids characters that are known to cause issues. Check occurs only when creating new lineage; this should avoid introducing a breaking change for lineages with those characters that users may have already created.

Raise an error when illegal lineage name is found; and provides a user-friendly error description on how to resolve the issue. An illegal name shouldn't occur when the lineage name is inferred from the domain names (instead of an explicit --cert-name) as domain names shouldn't allow such characters. Nevertheless, this assumption might no longer hold after future adjustments to this blacklist mechanism (or even after introducing a whitelist mechanism), so the shown error description reflects whether the provided --cert-name contained the illegal name, or whether it was inferred from the domain names.

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, edit the master 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.
• Include your name in AUTHORS.md if you like.

[bmw]

Thanks for the PR!

I'm currently working through a bit of a backlog of work to do and review, but I plan to get to this myself soon unless someone else on the Certbot team happens to have the time and interest before I do.

[NiekPeeters]

Alright!
I still need to add tests to verify that errors are raised correctly, though, as communicated through mattermost. I intend to add them somewhere this week.

[bmw]

Nice! I agree with you that this needs tests, but I largely like your approach here, especially where you're validating the given certname and how you moved that check before attempting to obtain the certificate.

The main thought I have is on what characters should be allowed. I know #6127 (comment) suggested that there are problems with slashes or names starting with a hyphen. What's the problem with a hyphen? As far as I can tell, it's just argparse gets confused:

sudo certbot --cert-name -foo
usage: 
  certbot [SUBCOMMAND] [options] [-d DOMAIN] [-d DOMAIN] ...

Certbot can obtain and install HTTPS/TLS/SSL certificates.  By default,
it will attempt to use a webserver both for obtaining and installing the
certificate. 
certbot: error: argument --cert-name: expected one argument

I personally think this is fine and this PR wouldn't have any effect on this.

As far as I can tell, the main problem is path separators. Do you agree? We could do something fancier such as declare a dependency on something like pathvalidate, however, I worry that it's overkill, especially since any extra error conditions here is arguably a breaking change in Certbot. Unless you or anyone else has another case they're especially worried about, I think we should just check for the presence of os.path.sep characters and error only on those for now.

What do you think?

[NiekPeeters]

The hyphen was indeed included because #6127 mentioned it would be an issue. I have not observed erroneous behavior when cert-name is provided with a leading hyphen lineagename. So I agree, using os.path.sep for this makes sense (and it eliminates false positives that occur due to cross-platform usage - e.g. / on windows or \ on linux should be allowed). I'll change the behavior of the check and update documentation.

This, however, raises the following question: how should I test this? I could write a single test that dynamically generates an illegal name by invoking os.path.sep. Another method would be to create two (or three, or four?) tests that hardcode the specific illegal characters in the test for the respective platform (windows, linux, macos, etc..?). The latter would be, in my opinion, undesirable; I assume there are already methods in place to run all tests for every supported platform separately?

Therefore, I suggest I create a test by extending the ClientTest class (in certbot/certbot/_internal/tests/client_test.py) with another test method. This test then reads os.path.sep and inserts that separator somewhere in a constant lineagename (e.g. "example"). All this under the assumptions that the test pipelines account for running the tests on different environments/platforms. Would all this make sense?

[bmw]

Yep! That makes sense. We have tests that run on Linux, macOS, and Windows.

I noticed one small thing but otherwise once this has tests, this LGTM. Thanks NiekPeeters.

[NiekPeeters]

Done!

[bmw]

Looks great. Thanks again!