diff --git a/certbot/CHANGELOG.md b/certbot/CHANGELOG.md index 3a9221f42..3d236b4f7 100644 --- a/certbot/CHANGELOG.md +++ b/certbot/CHANGELOG.md @@ -10,7 +10,8 @@ Certbot adheres to [Semantic Versioning](https://semver.org/). ### Changed -* +* 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 faad45d6c..b36e87a1d 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 59f5f8a4e..e60ba4f11 100644 --- a/certbot/certbot/_internal/client.py +++ b/certbot/certbot/_internal/client.py @@ -513,6 +513,7 @@ class Client: 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 @@ class Client: "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 @@ class Client: :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/certbot/_internal/tests/client_test.py b/certbot/certbot/_internal/tests/client_test.py index 1e1b0219c..7e7bb9018 100644 --- a/certbot/certbot/_internal/tests/client_test.py +++ b/certbot/certbot/_internal/tests/client_test.py @@ -815,6 +815,25 @@ class ClientTest(ClientTestCommon): installer.rollback_checkpoints.assert_called_once_with() assert installer.restart.call_count == 1 + def test_choose_lineage_name(self): + sep = os.path.sep + invalid_domains = [f"exam{sep}ple.com"] + valid_domains = ["example.com"] + invalid_certname = f"foo{sep}.bar" + valid_certname = "foo.bar" + invalid_wildcard_domain = [f"*.exam{sep}ple.com"] + # Verify errors are raised when invalid lineagename is chosen. + with pytest.raises(errors.Error): + self.client._choose_lineagename(invalid_domains, None) + with pytest.raises(errors.Error): + self.client._choose_lineagename(invalid_domains, invalid_certname) + with pytest.raises(errors.Error): + self.client._choose_lineagename(valid_domains, invalid_certname) + with pytest.raises(errors.Error): + self.client._choose_lineagename(invalid_wildcard_domain, None) + # Verify no error is raised when invalid domain is overriden by valid certname. + self.client._choose_lineagename(invalid_domains, valid_certname) + class EnhanceConfigTest(ClientTestCommon): """Tests for certbot._internal.client.Client.enhance_config.""" diff --git a/certbot/docs/using.rst b/certbot/docs/using.rst index 133bff8aa..ad562ef8f 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