Repository: certbot/certbot
Pull request: validate lineage name
Fixes [#6127](https://github.com/certbot/certbot/issues/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

- [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.
- [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.


Problem statement:
Don't allow special characters in lineage name
## My operating system is (include version):
Ubuntu 16.04.4

## I installed Certbot with (certbot-auto, OS package manager, pip, etc):
PPA, certbot 0.22.2

## I ran this command and it produced this output:
```bash
root@tom-vps:/etc/letsencrypt# certbot certonly --staging --webroot -d test.tomwassenberg.nl --webroot-path /var/www/acme-challenges/ --cert-name "/example"
Saving debug log to /var/log/letsencrypt/letsencrypt.log
Plugins selected: Authenticator webroot, Installer None
Obtaining a new certificate
Performing the following challenges:
http-01 challenge for test.tomwassenberg.nl
Using the webroot path /var/www/acme-challenges for all unmatched domains.
Waiting for verification...
Cleaning up challenges

IMPORTANT NOTES:
 - Congratulations! Your certificate and chain have been saved at:
   /etc/letsencrypt/live/example/fullchain.pem
   Your key file has been saved at:
   /etc/letsencrypt/live/example/privkey.pem
   Your cert will expire on 2018-09-14. To obtain a new or tweaked
   version of this certificate in the future, simply run certbot
   again. To non-interactively renew *all* of your certificates, run
   "certbot renew"
```

## Certbot's behavior differed from what I expected because:
Certbot should halt and return an error message stating that you can't use a slash in a lineage name. Instead, it continues normally and saves the regular output in `/etc/letsencrypt/{archive,live}/example`. However, when saving the renewal configuration, it becomes apparent the lineage name is parsed as a path because it is saved on the actual path `/example.conf`, so on the root of the filesystem. This shouldn't happen.

Also other permutations of names with special characters, like names with slashes between regular characters, or names starting with a hyphen, cause unexpected behavior.

Hints from issue discussion:
Thanks for reporting!
I'm trying to fix this by raising an error in the function `get_certnames` inside `cert_manager.py`.

"certname" appears in several places though, so I'm not sure if this will cover all cases where the user has input the certname.
Since we're not actually seeing people run into this, moving it to wishlist.
I'm removing the "good first issue" label because Erica assigned herself, but if Erica's not planning on working on this anytime soon, perhaps we should remove her assignment and add back the "good first issue" label.
Erica said she's not planning on implementing this issue, but wanted to make herself responsible for tracking it to make sure it gets done. If someone else wants to work on this, please do so!
Should we use a character whitelist or blacklist?

One might think [a-zA-Z0-9_-] will suffice, but I'm sure others might not think so :stuck_out_tongue: I'm sure we don't want *all* characters minus the slashes/hyphen as a possible char, but perhaps we'd like to allow other scripts like Cyrillic or Greek or allow Unicode chars.
My ideal would be to figure out and implement the behavior that most closely matches the things accepted in practice. That is, test the system and see if other scripts work perfectly fine, what characters break things, etc. Is it literally only slashes and hyphen that break things in practice? What's causing things to break; is it NGINX's code or something about how Certbot interacts with filenames?

Commit message: Use filepath seperators to determine lineagename validity

Patch to review:
diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md
index e429d80c3ec..2f22a930ef5 100644
--- a/certbot/CHANGELOG.md
+++ b/certbot/CHANGELOG.md
@@ -20,6 +20,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   removed in a future version of Certbot.
 * Packaged tests for all Certbot components besides josepy were moved inside
   the `_internal/tests` module.
+* Lineage name validity is performed for new lineages. `--cert-name` may no longer contain
+  filepath separators (i.e. `/` or `\\`, depending on the platform).
 
 ### Fixed
 
diff --git a/certbot/certbot/_internal/cli/__init__.py b/certbot/certbot/_internal/cli/__init__.py
index faad45d6c20..b36e87a1d44 100644
--- a/certbot/certbot/_internal/cli/__init__.py
+++ b/certbot/certbot/_internal/cli/__init__.py
@@ -139,6 +139,8 @@ def prepare_and_parse_args(plugins: plugins_disco.PluginsRegistry, args: List[st
         metavar="CERTNAME", default=flag_default("certname"),
         help="Certificate name to apply. This name is used by Certbot for housekeeping "
              "and in file paths; it doesn't affect the content of the certificate itself. "
+             "Certificate name cannot contain filepath separators (i.e. '/' or '\\', depending "
+             "on the platform). "
              "To see certificate names, run 'certbot certificates'. "
              "When creating a new certificate, specifies the new certificate's name. "
              "(default: the first provided domain or the name of an existing "
diff --git a/certbot/certbot/_internal/client.py b/certbot/certbot/_internal/client.py
index 59f5f8a4e31..e60ba4f1166 100644
--- a/certbot/certbot/_internal/client.py
+++ b/certbot/certbot/_internal/client.py
@@ -513,6 +513,7 @@ def obtain_and_enroll_certificate(self, domains: List[str], certname: Optional[s
             referred to the enrolled cert lineage, or None if doing a successful dry run.
 
         """
+        new_name = self._choose_lineagename(domains, certname)
         cert, chain, key, _ = self.obtain_certificate(domains)
 
         if (self.config.config_dir != constants.CLI_DEFAULTS["config_dir"] or
@@ -521,8 +522,6 @@ def obtain_and_enroll_certificate(self, domains: List[str], certname: Optional[s
                 "Non-standard path(s), might not work with crontab installed "
                 "by your operating system package manager")
 
-        new_name = self._choose_lineagename(domains, certname)
-
         if self.config.dry_run:
             logger.debug("Dry run: Skipping creating new lineage for %s", new_name)
             return None
@@ -559,13 +558,41 @@ def _choose_lineagename(self, domains: List[str], certname: Optional[str]) -> st
         :returns: lineage name that should be used
         :rtype: str
 
+        :raises errors.Error: If the chosen lineage name is invalid.
+
         """
+        # Remember chosen name for new lineage
+        lineagename = None
         if certname:
-            return certname
+            lineagename = certname
         elif util.is_wildcard_domain(domains[0]):
             # Don't make files and directories starting with *.
-            return domains[0][2:]
-        return domains[0]
+            lineagename = domains[0][2:]
+        else:
+            lineagename = domains[0]
+        # Verify whether chosen lineage is valid
+        if self._is_valid_lineagename(lineagename):
+            return lineagename
+        else:
+            raise errors.Error(
+                "The provided certname cannot be used as a lineage name because it contains "
+                "an illegal character (i.e. filepath separator)." if certname else
+                "Cannot use domain name as lineage name because it contains an illegal "
+                "character (i.e. filepath separator). Specify an explicit lineage name "
+                "with --cert-name.")
+
+    def _is_valid_lineagename(self, name: str) -> bool:
+        """Determines whether the provided name is a valid lineagename. A lineagename
+        is invalid when it contains filepath separators.
+
+        :param name: the lineage name to determine validity for
+        :type name: `str`
+
+        :returns: Whether the provided string constitutes a valid lineage name.
+        :rtype: bool
+
+        """
+        return os.path.sep not in name
 
     def save_certificate(self, cert_pem: bytes, chain_pem: bytes,
                          cert_path: str, chain_path: str, fullchain_path: str
diff --git a/certbot/docs/using.rst b/certbot/docs/using.rst
index 133bff8aa09..ad562ef8f5a 100644
--- a/certbot/docs/using.rst
+++ b/certbot/docs/using.rst
@@ -374,7 +374,9 @@ This returns information in the following format::
 
 ``Certificate Name`` shows the name of the certificate. Pass this name
 using the ``--cert-name`` flag to specify a particular certificate for the ``run``,
-``certonly``, ``certificates``, ``renew``, and ``delete`` commands. Example::
+``certonly``, ``certificates``, ``renew``, and ``delete`` commands. The certificate
+name cannot contain filepath separators (i.e. '/' or '\\', depending on the platform).
+Example::
 
   certbot certonly --cert-name example.com
 


Code review comments:
diff_hunk: @@ -20,6 +20,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/).
   removed in a future version of Certbot.
 * Packaged tests for all Certbot components besides josepy were moved inside
   the `_internal/tests` module.
+* Lineage name validity is performed for new lineages. `--cert-name` may no longer contain
+  filepath separators (i.e. `/` or `\\`, depending on the platform).
path: certbot/CHANGELOG.md
text: @user1:
I don't think we need to escape this here.
```suggestion
  filepath separators (i.e. `/` or `\`, depending on the platform).
```
Also, can you merge master into this branch and then move this changelog entry to the new 2.6.0 section? We really should fix https://github.com/certbot/certbot/issues/8272.