From 2ac7a2a9eaeca167fccd8f10aa80f5ee9857ccae Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 20:23:01 +0200 Subject: [PATCH 01/15] Check configuration sanity for domain flag --- letsencrypt/cli.py | 30 +++++++++++++++++++++++++++++- letsencrypt/errors.py | 3 +++ letsencrypt/tests/cli_test.py | 22 ++++++++++++++++++++++ 3 files changed, 54 insertions(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index c1f3edb70..94ff59d91 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -7,6 +7,7 @@ import logging import logging.handlers import os import pkg_resources +import re import sys import time import traceback @@ -36,7 +37,12 @@ from letsencrypt import storage from letsencrypt.display import util as display_util from letsencrypt.display import ops as display_ops -from letsencrypt.errors import Error, PluginSelectionError, CertStorageError +from letsencrypt.errors import ( + CertStorageError, + ConfigurationError, + Error, + PluginSelectionError +) from letsencrypt.plugins import disco as plugins_disco @@ -1085,6 +1091,8 @@ def main(cli_args=sys.argv[1:]): # note: arg parser internally handles --help (and exits afterwards) plugins = plugins_disco.PluginsRegistry.find_all() args = prepare_and_parse_args(plugins, cli_args) + # Check command line parameters sanity, and error out in case of problem. + check_config_sanity(args) config = configuration.NamespaceConfig(args) zope.component.provideUtility(config) @@ -1139,6 +1147,26 @@ def main(cli_args=sys.argv[1:]): return args.func(args, config, plugins) +def check_config_sanity(args): + """Validate command line options and display error message if + requirements are not met. + + :param args: Command line options + :type args: :class:`argparse.Namespace` + + """ + # Domain checks + if args.domains is not None: + # Check if there's a wildcard domain + if any(True for d in args.domains if d.startswith("*")): + raise ConfigurationError("Error: Wildcard domains are not supported") + # Punycode + if any(True for d in args.domains if "xn--" in d): + raise ConfigurationError("Error: Punycode domains are not supported") + # Check for FQDN + fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 20:42:24 +0200 Subject: [PATCH 02/15] Use python2.6 compatible test assertions --- letsencrypt/tests/cli_test.py | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index bb6309968..bd41b0ebe 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -177,25 +177,23 @@ class CLITest(unittest.TestCase): def test_check_config_sanity_domain(self): # Punycode - self.assertRaisesRegexp(errors.ConfigurationError, - "Error: Punycode domains are not supported", - self._call, - ['-d', 'this.is.xn--ls8h.tld']) + self.assertRaises(errors.ConfigurationError, + self._call, + ['-d', 'this.is.xn--ls8h.tld']) # FQDN - self.assertRaisesRegexp(errors.ConfigurationError, - "Error: Requested domain is not FQDN", - self._call, - ['-d', 'comma,gotwrong.tld']) + self.assertRaises(errors.ConfigurationError, + "Error: Requested domain is not FQDN", + self._call, + ['-d', 'comma,gotwrong.tld']) # FQDN 2 - self.assertRaisesRegexp(errors.ConfigurationError, - "Error: Requested domain is not FQDN", - self._call, - ['-d', 'illegal.character=.tld']) + self.assertRaises(errors.ConfigurationError, + "Error: Requested domain is not FQDN", + self._call, + ['-d', 'illegal.character=.tld']) # Wildcard - self.assertRaisesRegexp(errors.ConfigurationError, - "Error: Wildcard domains are not supported", - self._call, - ['-d', '*.wildcard.tld']) + self.assertRaises(errors.ConfigurationError, + self._call, + ['-d', '*.wildcard.tld']) @mock.patch('letsencrypt.crypto_util.notAfter') @mock.patch('letsencrypt.cli.zope.component.getUtility') From b35ebafe16f13c20e942a48fdbf66cb71735287b Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 20:49:44 +0200 Subject: [PATCH 03/15] Fixed last half-assed commit to tests --- letsencrypt/tests/cli_test.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/letsencrypt/tests/cli_test.py b/letsencrypt/tests/cli_test.py index bd41b0ebe..da5286a10 100644 --- a/letsencrypt/tests/cli_test.py +++ b/letsencrypt/tests/cli_test.py @@ -182,12 +182,10 @@ class CLITest(unittest.TestCase): ['-d', 'this.is.xn--ls8h.tld']) # FQDN self.assertRaises(errors.ConfigurationError, - "Error: Requested domain is not FQDN", self._call, ['-d', 'comma,gotwrong.tld']) # FQDN 2 self.assertRaises(errors.ConfigurationError, - "Error: Requested domain is not FQDN", self._call, ['-d', 'illegal.character=.tld']) # Wildcard From d6a3286a677ce0c432d4aa082de4853985490511 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 20:50:39 +0200 Subject: [PATCH 04/15] Fixed the wildcard domain string match --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 94ff59d91..f6231c98a 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1158,7 +1158,7 @@ def check_config_sanity(args): # Domain checks if args.domains is not None: # Check if there's a wildcard domain - if any(True for d in args.domains if d.startswith("*")): + if any(True for d in args.domains if d.startswith("*.")): raise ConfigurationError("Error: Wildcard domains are not supported") # Punycode if any(True for d in args.domains if "xn--" in d): From ae080349c54af9fc3ebab40b75c8ea086ef2b672 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 21:04:48 +0200 Subject: [PATCH 05/15] Changed the errors import --- letsencrypt/cli.py | 39 +++++++++++++++++---------------------- 1 file changed, 17 insertions(+), 22 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index f6231c98a..e901246ce 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -29,6 +29,7 @@ from letsencrypt import configuration from letsencrypt import constants from letsencrypt import client from letsencrypt import crypto_util +from letsencrypt import errors from letsencrypt import interfaces from letsencrypt import le_util from letsencrypt import log @@ -37,12 +38,6 @@ from letsencrypt import storage from letsencrypt.display import util as display_util from letsencrypt.display import ops as display_ops -from letsencrypt.errors import ( - CertStorageError, - ConfigurationError, - Error, - PluginSelectionError -) from letsencrypt.plugins import disco as plugins_disco @@ -112,7 +107,7 @@ def _find_domains(args, installer): domains = args.domains if not domains: - raise Error("Please specify --domains, or --installer that " + raise errors.Error("Please specify --domains, or --installer that " "will help in domain names autodiscovery") return domains @@ -165,9 +160,9 @@ def _determine_account(args, config): try: acc, acme = client.register( config, account_storage, tos_cb=_tos_cb) - except Error as error: + except errors.Error as error: logger.debug(error, exc_info=True) - raise Error( + raise errors.Error( "Unable to register an account with ACME server") args.account = acc.id @@ -201,7 +196,7 @@ def _find_duplicative_certs(config, domains): try: full_path = os.path.join(configs_dir, renewal_file) candidate_lineage = storage.RenewableCert(full_path, cli_config) - except (CertStorageError, IOError): + except (errors.CertStorageError, IOError): logger.warning("Renewal configuration file %s is broken. " "Skipping.", full_path) continue @@ -273,7 +268,7 @@ def _treat_as_renewal(config, domains): br=os.linesep ), reporter_util.HIGH_PRIORITY) - raise Error( + raise errors.Error( "User did not use proper CLI and would like " "to reinvoke the client.") @@ -333,7 +328,7 @@ def _auth_from_domains(le_client, config, domains, plugins): # TREAT AS NEW REQUEST lineage = le_client.obtain_and_enroll_certificate(domains, plugins) if not lineage: - raise Error("Certificate could not be obtained") + raise errors.Error("Certificate could not be obtained") _report_new_cert(lineage.cert, lineage.fullchain) @@ -352,7 +347,7 @@ def set_configurator(previously, now): if previously: if previously != now: msg = "Too many flags setting configurators/installers/authenticators {0} -> {1}" - raise PluginSelectionError(msg.format(repr(previously), repr(now))) + raise errors.PluginSelectionError(msg.format(repr(previously), repr(now))) return now @@ -385,7 +380,7 @@ def diagnose_configurator_problem(cfg_type, requested, plugins): '"letsencrypt-auto certonly" to get a cert you can install manually') else: msg = "{0} could not be determined or is not installed".format(cfg_type) - raise PluginSelectionError(msg) + raise errors.PluginSelectionError(msg) def choose_configurator_plugins(args, config, plugins, verb): @@ -445,7 +440,7 @@ def run(args, config, plugins): # pylint: disable=too-many-branches,too-many-lo """Obtain a certificate and install.""" try: installer, authenticator = choose_configurator_plugins(args, config, plugins, "run") - except PluginSelectionError, e: + except errors.PluginSelectionError, e: return e.message domains = _find_domains(args, installer) @@ -478,7 +473,7 @@ def obtaincert(args, config, plugins): try: # installers are used in auth mode to determine domain names installer, authenticator = choose_configurator_plugins(args, config, plugins, "certonly") - except PluginSelectionError, e: + except errors.PluginSelectionError, e: return e.message # TODO: Handle errors from _init_le_client? @@ -503,7 +498,7 @@ def install(args, config, plugins): try: installer, _ = choose_configurator_plugins(args, config, plugins, "install") - except PluginSelectionError, e: + except errors.PluginSelectionError, e: return e.message domains = _find_domains(args, installer) @@ -1066,7 +1061,7 @@ def _handle_exception(exc_type, exc_value, trace, args): sys.exit("".join( traceback.format_exception(exc_type, exc_value, trace))) - if issubclass(exc_type, Error): + if issubclass(exc_type, errors.Error): sys.exit(exc_value) else: # Tell the user a bit about what happened, without overwhelming @@ -1132,7 +1127,7 @@ def main(cli_args=sys.argv[1:]): disclaimer = pkg_resources.resource_string("letsencrypt", "DISCLAIMER") if not zope.component.getUtility(interfaces.IDisplay).yesno( disclaimer, "Agree", "Cancel"): - raise Error("Must agree to TOS") + raise errors.Error("Must agree to TOS") if not os.geteuid() == 0: logger.warning( @@ -1159,14 +1154,14 @@ def check_config_sanity(args): if args.domains is not None: # Check if there's a wildcard domain if any(True for d in args.domains if d.startswith("*.")): - raise ConfigurationError("Error: Wildcard domains are not supported") + raise errors.ConfigurationError("Error: Wildcard domains are not supported") # Punycode if any(True for d in args.domains if "xn--" in d): - raise ConfigurationError("Error: Punycode domains are not supported") + raise errors.ConfigurationError("Error: Punycode domains are not supported") # Check for FQDN fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 21:08:50 +0200 Subject: [PATCH 06/15] Changed the checks to be more readable and better semantically --- letsencrypt/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index e901246ce..88d7e3d30 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1153,10 +1153,10 @@ def check_config_sanity(args): # Domain checks if args.domains is not None: # Check if there's a wildcard domain - if any(True for d in args.domains if d.startswith("*.")): + if any(d.startswith("*.") for d in args.domains): raise errors.ConfigurationError("Error: Wildcard domains are not supported") # Punycode - if any(True for d in args.domains if "xn--" in d): + if any("xn--" in d for d in args.domains): raise errors.ConfigurationError("Error: Punycode domains are not supported") # Check for FQDN fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 21:14:12 +0200 Subject: [PATCH 07/15] Added comment to clarify FQDN regex --- letsencrypt/cli.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 88d7e3d30..1f04ee173 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1158,7 +1158,8 @@ def check_config_sanity(args): # Punycode if any("xn--" in d for d in args.domains): raise errors.ConfigurationError("Error: Punycode domains are not supported") - # Check for FQDN + # FQDN, checks: + # Characters used, domain parts < 63 chars, tld > 3 < 6 chars fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 21:43:48 +0200 Subject: [PATCH 08/15] Refactored domain flag checks into their own helper function to clear space for future checks --- letsencrypt/cli.py | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 1f04ee173..c66060d48 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1152,17 +1152,29 @@ def check_config_sanity(args): """ # Domain checks if args.domains is not None: - # Check if there's a wildcard domain - if any(d.startswith("*.") for d in args.domains): - raise errors.ConfigurationError("Error: Wildcard domains are not supported") - # Punycode - if any("xn--" in d for d in args.domains): - raise errors.ConfigurationError("Error: Punycode domains are not supported") - # FQDN, checks: - # Characters used, domain parts < 63 chars, tld > 3 < 6 chars - fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? 3 < 6 chars + fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 21:46:57 +0200 Subject: [PATCH 09/15] Fixed the messages --- letsencrypt/cli.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index c66060d48..23ef8956a 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1165,16 +1165,16 @@ def _check_config_domain_sanity(domains): # Check if there's a wildcard domain if any(d.startswith("*.") for d in domains): raise errors.ConfigurationError( - "Error: Wildcard domains are not supported") + "Wildcard domains are not supported") # Punycode if any("xn--" in d for d in domains): raise errors.ConfigurationError( - "Error: Punycode domains are not supported") + "Punycode domains are not supported") # FQDN, checks: # Characters used, domain parts < 63 chars, tld > 3 < 6 chars fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 22:21:11 +0200 Subject: [PATCH 10/15] Added domain to renewer test mock --- letsencrypt/tests/renewer_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/letsencrypt/tests/renewer_test.py b/letsencrypt/tests/renewer_test.py index 0a39b7987..e76b6eb88 100644 --- a/letsencrypt/tests/renewer_test.py +++ b/letsencrypt/tests/renewer_test.py @@ -691,6 +691,7 @@ class RenewableCertTests(BaseRenewableCertTest): self.test_rc.configfile["renewalparams"]["tls_sni_01_port"] = "4430" self.test_rc.configfile["renewalparams"]["http01_port"] = "1234" self.test_rc.configfile["renewalparams"]["account"] = "abcde" + self.test_rc.configfile["renewalparams"]["domains"] = ["example.com"] mock_auth = mock.MagicMock() mock_pd.PluginsRegistry.find_all.return_value = {"apache": mock_auth} # Fails because "fake" != "apache" From 59b544be3b6ac41f0c6ab1556f2477e59d27cd67 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 22:26:01 +0200 Subject: [PATCH 11/15] Moved the validation to configurator --- letsencrypt/cli.py | 35 -------------------------------- letsencrypt/configuration.py | 39 +++++++++++++++++++++++++++++++++++- 2 files changed, 38 insertions(+), 36 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 23ef8956a..b8bdeff7d 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -1087,7 +1087,6 @@ def main(cli_args=sys.argv[1:]): plugins = plugins_disco.PluginsRegistry.find_all() args = prepare_and_parse_args(plugins, cli_args) # Check command line parameters sanity, and error out in case of problem. - check_config_sanity(args) config = configuration.NamespaceConfig(args) zope.component.provideUtility(config) @@ -1142,40 +1141,6 @@ def main(cli_args=sys.argv[1:]): return args.func(args, config, plugins) -def check_config_sanity(args): - """Validate command line options and display error message if - requirements are not met. - - :param args: Command line options - :type args: :class:`argparse.Namespace` - - """ - # Domain checks - if args.domains is not None: - _check_config_domain_sanity(args.domains) - -def _check_config_domain_sanity(domains): - """Helper method for check_config_sanity which validates - domain flag values and errors out if the requirements are not met. - - :param domains: List of domains - :type args: `list` of `string` - - """ - # Check if there's a wildcard domain - if any(d.startswith("*.") for d in domains): - raise errors.ConfigurationError( - "Wildcard domains are not supported") - # Punycode - if any("xn--" in d for d in domains): - raise errors.ConfigurationError( - "Punycode domains are not supported") - # FQDN, checks: - # Characters used, domain parts < 63 chars, tld > 3 < 6 chars - fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? 3 < 6 chars + fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 22:44:32 +0200 Subject: [PATCH 12/15] Making linter happy by removing the unused import --- letsencrypt/cli.py | 1 - 1 file changed, 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index b8bdeff7d..d343fcaa5 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -7,7 +7,6 @@ import logging import logging.handlers import os import pkg_resources -import re import sys import time import traceback From 0c4456bd7ed6127d410839086a02828bee15e917 Mon Sep 17 00:00:00 2001 From: Joona Hoikkala Date: Sun, 8 Nov 2015 22:57:58 +0200 Subject: [PATCH 13/15] Fixed the comment to be accurate --- letsencrypt/configuration.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/configuration.py b/letsencrypt/configuration.py index b18807c40..082a69c21 100644 --- a/letsencrypt/configuration.py +++ b/letsencrypt/configuration.py @@ -144,7 +144,7 @@ def _check_config_domain_sanity(domains): raise errors.ConfigurationError( "Punycode domains are not supported") # FQDN, checks: - # Characters used, domain parts < 63 chars, tld > 3 < 6 chars + # Characters used, domain parts < 63 chars, tld > 1 < 7 chars fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(? Date: Sun, 8 Nov 2015 23:05:33 +0200 Subject: [PATCH 14/15] Refactored the port check from NamespaceConfig init to the validation function --- letsencrypt/configuration.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/letsencrypt/configuration.py b/letsencrypt/configuration.py index 082a69c21..8fa3cb2dc 100644 --- a/letsencrypt/configuration.py +++ b/letsencrypt/configuration.py @@ -38,10 +38,6 @@ class NamespaceConfig(object): def __init__(self, namespace): self.namespace = namespace check_config_sanity(self) - if self.http01_port == self.tls_sni_01_port: - raise errors.Error( - "Trying to run http-01 and tls-sni-01 " - "on the same port ({0})".format(self.tls_sni_01_port)) def __getattr__(self, name): return getattr(self.namespace, name) @@ -122,6 +118,12 @@ def check_config_sanity(config): :type args: :class:`letsencrypt.interfaces.IConfig` """ + # Port check + if config.http01_port == config.tls_sni_01_port: + raise errors.Error( + "Trying to run http-01 and tls-sni-01 " + "on the same port ({0})".format(config.tls_sni_01_port)) + # Domain checks if config.namespace.domains is not None: _check_config_domain_sanity(config.namespace.domains) From d29ab2a5238fd4c8f2fefbde9bf80e8af0642c9d Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Mon, 9 Nov 2015 16:18:51 -0800 Subject: [PATCH 15/15] Make final tweaks for landing #1421 --- letsencrypt/cli.py | 3 +-- letsencrypt/configuration.py | 13 +++++++++---- letsencrypt/errors.py | 1 + 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index d343fcaa5..7227116e3 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -107,7 +107,7 @@ def _find_domains(args, installer): if not domains: raise errors.Error("Please specify --domains, or --installer that " - "will help in domain names autodiscovery") + "will help in domain names autodiscovery") return domains @@ -1085,7 +1085,6 @@ def main(cli_args=sys.argv[1:]): # note: arg parser internally handles --help (and exits afterwards) plugins = plugins_disco.PluginsRegistry.find_all() args = prepare_and_parse_args(plugins, cli_args) - # Check command line parameters sanity, and error out in case of problem. config = configuration.NamespaceConfig(args) zope.component.provideUtility(config) diff --git a/letsencrypt/configuration.py b/letsencrypt/configuration.py index 8fa3cb2dc..e70171675 100644 --- a/letsencrypt/configuration.py +++ b/letsencrypt/configuration.py @@ -37,6 +37,7 @@ class NamespaceConfig(object): def __init__(self, namespace): self.namespace = namespace + # Check command line parameters sanity, and error out in case of problem. check_config_sanity(self) def __getattr__(self, name): @@ -120,7 +121,7 @@ def check_config_sanity(config): """ # Port check if config.http01_port == config.tls_sni_01_port: - raise errors.Error( + raise errors.ConfigurationError( "Trying to run http-01 and tls-sni-01 " "on the same port ({0})".format(config.tls_sni_01_port)) @@ -134,7 +135,9 @@ def _check_config_domain_sanity(domains): domain flag values and errors out if the requirements are not met. :param domains: List of domains - :type args: `list` of `string` + :type domains: `list` of `string` + :raises ConfigurationError: for invalid domains and cases where Let's + Encrypt currently will not issue certificates """ # Check if there's a wildcard domain @@ -145,8 +148,10 @@ def _check_config_domain_sanity(domains): if any("xn--" in d for d in domains): raise errors.ConfigurationError( "Punycode domains are not supported") - # FQDN, checks: + # FQDN checks from + # http://www.mkyong.com/regular-expressions/domain-name-regular-expression-example/ # Characters used, domain parts < 63 chars, tld > 1 < 7 chars + # first and last char is not "-" fqdn = re.compile("^((?!-)[A-Za-z0-9-]{1,63}(?