From 15502bb64e50b75a4a5e46a29179492c1b5c9fda Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 17 Mar 2016 16:14:29 -0700 Subject: [PATCH 01/19] renew implies noninteractive should be a property of config Not a property of the config we change later --- letsencrypt/cli.py | 3 +++ letsencrypt/main.py | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 017e0a62e..d3ffd3441 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -409,6 +409,9 @@ class HelpfulArgumentParser(object): # Do any post-parsing homework here + if self.verb == "renew": + self.noninteractive_mode = True + # we get domains from -d, but also from the webroot map... if parsed_args.webroot_map: for domain in parsed_args.webroot_map.keys(): diff --git a/letsencrypt/main.py b/letsencrypt/main.py index 8d59993df..0148bdeca 100644 --- a/letsencrypt/main.py +++ b/letsencrypt/main.py @@ -685,9 +685,6 @@ def main(cli_args=sys.argv[1:]): displayer = display_util.NoninteractiveDisplay(sys.stdout) elif config.text_mode: displayer = display_util.FileDisplay(sys.stdout) - elif config.verb == "renew": - config.noninteractive_mode = True - displayer = display_util.NoninteractiveDisplay(sys.stdout) else: displayer = display_util.NcursesDisplay() zope.component.provideUtility(displayer) From b8ea2c19a38d870556496b980de2472b4e369d32 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Apr 2016 16:57:52 -0700 Subject: [PATCH 02/19] Lintian bug fix --- letsencrypt/cli.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index f2ad50d45..adcc32a5e 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -272,7 +272,7 @@ class HelpfulArgumentParser(object): # Do any post-parsing homework here if self.verb == "renew": - self.noninteractive_mode = True + parsed_args.noninteractive_mode = True # we get domains from -d, but also from the webroot map... if parsed_args.webroot_map: From 0bcc80756d16de6aed3365b0fa3d27c2e2e98855 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Apr 2016 17:10:30 -0700 Subject: [PATCH 03/19] Refactor config.server complexity out of parse_args --- letsencrypt/cli.py | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 8c2cd839a..75c7e116f 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -318,24 +318,7 @@ class HelpfulArgumentParser(object): parsed_args.noninteractive_mode = True if parsed_args.staging or parsed_args.dry_run: - if parsed_args.server not in (flag_default("server"), constants.STAGING_URI): - conflicts = ["--staging"] if parsed_args.staging else [] - conflicts += ["--dry-run"] if parsed_args.dry_run else [] - raise errors.Error("--server value conflicts with {0}".format( - " and ".join(conflicts))) - - parsed_args.server = constants.STAGING_URI - - if parsed_args.dry_run: - if self.verb not in ["certonly", "renew"]: - raise errors.Error("--dry-run currently only works with the " - "'certonly' or 'renew' subcommands (%r)" % self.verb) - parsed_args.break_my_certs = parsed_args.staging = True - if glob.glob(os.path.join(parsed_args.config_dir, constants.ACCOUNTS_DIR, "*")): - # The user has a prod account, but might not have a staging - # one; we don't want to start trying to perform interactive registration - parsed_args.tos = True - parsed_args.register_unsafely_without_email = True + self.set_test_server(parsed_args) if parsed_args.csr: if parsed_args.allow_subset_of_names: @@ -347,6 +330,30 @@ class HelpfulArgumentParser(object): return parsed_args + + def set_test_server(self, parsed_args): + "We have --staging/--dry-run; perform sanity check and set config.server" + + if parsed_args.server not in (flag_default("server"), constants.STAGING_URI): + conflicts = ["--staging"] if parsed_args.staging else [] + conflicts += ["--dry-run"] if parsed_args.dry_run else [] + raise errors.Error("--server value conflicts with {0}".format( + " and ".join(conflicts))) + + parsed_args.server = constants.STAGING_URI + + if parsed_args.dry_run: + if self.verb not in ["certonly", "renew"]: + raise errors.Error("--dry-run currently only works with the " + "'certonly' or 'renew' subcommands (%r)" % self.verb) + parsed_args.break_my_certs = parsed_args.staging = True + if glob.glob(os.path.join(parsed_args.config_dir, constants.ACCOUNTS_DIR, "*")): + # The user has a prod account, but might not have a staging + # one; we don't want to start trying to perform interactive registration + parsed_args.tos = True + parsed_args.register_unsafely_without_email = True + + def handle_csr(self, parsed_args): """Process a --csr flag.""" if parsed_args.verb != "certonly": From 5e3fc3a957eaaff225e21916b1241cd5a37ec522 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Apr 2016 17:14:29 -0700 Subject: [PATCH 04/19] Keep all --csr checks in HelpfulArgumentParser.handle_csr --- letsencrypt/cli.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 75c7e116f..74c51f5e3 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -321,9 +321,6 @@ class HelpfulArgumentParser(object): self.set_test_server(parsed_args) if parsed_args.csr: - if parsed_args.allow_subset_of_names: - raise errors.Error("--allow-subset-of-names " - "cannot be used with --csr") self.handle_csr(parsed_args) hooks.validate_hooks(parsed_args) @@ -361,6 +358,8 @@ class HelpfulArgumentParser(object): "when obtaining a new or replacement " "via the certonly command. Please try the " "certonly command instead.") + if parsed_args.allow_subset_of_names: + raise errors.Error("--allow-subset-of-names cannot be used with --csr") try: csr = le_util.CSR(file=parsed_args.csr[0], data=parsed_args.csr[1], form="der") From 526bc5cf84aeaca0669d87ada3ced32615efe8e9 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Wed, 6 Apr 2016 17:35:29 -0700 Subject: [PATCH 05/19] Refactor CSR importing from cli -> crypto_util More specifically: HelpfulArgumentParser.handle_csr -> crypto_util.import_csr_file --- letsencrypt/cli.py | 18 ++---------------- letsencrypt/crypto_util.py | 30 ++++++++++++++++++++++++++++++ letsencrypt/tests/client_test.py | 6 ++++-- 3 files changed, 36 insertions(+), 18 deletions(-) diff --git a/letsencrypt/cli.py b/letsencrypt/cli.py index 74c51f5e3..61fc57777 100644 --- a/letsencrypt/cli.py +++ b/letsencrypt/cli.py @@ -6,10 +6,8 @@ import logging import logging.handlers import os import sys -import traceback import configargparse -import OpenSSL import six import letsencrypt @@ -361,20 +359,8 @@ class HelpfulArgumentParser(object): if parsed_args.allow_subset_of_names: raise errors.Error("--allow-subset-of-names cannot be used with --csr") - try: - csr = le_util.CSR(file=parsed_args.csr[0], data=parsed_args.csr[1], form="der") - typ = OpenSSL.crypto.FILETYPE_ASN1 - domains = crypto_util.get_sans_from_csr(csr.data, OpenSSL.crypto.FILETYPE_ASN1) - except OpenSSL.crypto.Error: - try: - e1 = traceback.format_exc() - typ = OpenSSL.crypto.FILETYPE_PEM - csr = le_util.CSR(file=parsed_args.csr[0], data=parsed_args.csr[1], form="pem") - domains = crypto_util.get_sans_from_csr(csr.data, typ) - except OpenSSL.crypto.Error: - logger.debug("DER CSR parse error %s", e1) - logger.debug("PEM CSR parse error %s", traceback.format_exc()) - raise errors.Error("Failed to parse CSR file: {0}".format(parsed_args.csr[0])) + csrfile, contents = parsed_args.csr[0:2] + typ, csr, domains = crypto_util.import_csr_file(csrfile, contents) # This is not necessary for webroot to work, however, # obtain_certificate_from_csr requires parsed_args.domains to be set diff --git a/letsencrypt/crypto_util.py b/letsencrypt/crypto_util.py index 5fdcba843..2ca43f76f 100644 --- a/letsencrypt/crypto_util.py +++ b/letsencrypt/crypto_util.py @@ -6,6 +6,7 @@ """ import logging import os +import traceback import OpenSSL import pyrfc3339 @@ -171,6 +172,35 @@ def csr_matches_pubkey(csr, privkey): return False +def import_csr_file(csrfile, contents): + """Import a CSR file, which can be either PEM or DER. + + :param str csrfile: CSR filename + :param str contents: contens of the CSR file + + :rtype: tuple + + :returns: (le_util.CSR object representing the CSR, + OpenSSL FILETYPE_ representing DER or PEM, + list of domains requested in the CSR) + """ + try: + csr = le_util.CSR(file=csrfile, data=contents, form="der") + typ = OpenSSL.crypto.FILETYPE_ASN1 + domains = get_sans_from_csr(csr.data, OpenSSL.crypto.FILETYPE_ASN1) + except OpenSSL.crypto.Error: + try: + e1 = traceback.format_exc() + typ = OpenSSL.crypto.FILETYPE_PEM + csr = le_util.CSR(file=csrfile, data=contents, form="pem") + domains = get_sans_from_csr(csr.data, typ) + except OpenSSL.crypto.Error: + logger.debug("DER CSR parse error %s", e1) + logger.debug("PEM CSR parse error %s", traceback.format_exc()) + raise errors.Error("Failed to parse CSR file: {0}".format(csrfile)) + return typ, csr, domains + + def make_key(bits): """Generate PEM encoded RSA key. diff --git a/letsencrypt/tests/client_test.py b/letsencrypt/tests/client_test.py index cd6b11158..c3d5b322f 100644 --- a/letsencrypt/tests/client_test.py +++ b/letsencrypt/tests/client_test.py @@ -134,7 +134,7 @@ class ClientTest(unittest.TestCase): self.acme.fetch_chain.assert_called_once_with(mock.sentinel.certr) - # FIXME move parts of this to test_cli.py... + # FIXME move parts of this to crypto_util tests... @mock.patch("letsencrypt.client.logger") def test_obtain_certificate_from_csr(self, mock_logger): self._mock_obtain_certificate() @@ -144,9 +144,11 @@ class ClientTest(unittest.TestCase): # The CLI should believe that this is a certonly request, because # a CSR would not be allowed with other kinds of requests! mock_parsed_args.verb = "certonly" - with mock.patch("letsencrypt.client.le_util.CSR") as mock_CSR: + with mock.patch("letsencrypt.cli.crypto_util.le_util.CSR") as mock_CSR: mock_CSR.return_value = test_csr mock_parsed_args.domains = self.eg_domains[:] + mock_parsed_args.allow_subset_of_names = False + mock_parsed_args.csr = (mock.MagicMock(), mock.MagicMock()) mock_parser = mock.MagicMock(cli.HelpfulArgumentParser) cli.HelpfulArgumentParser.handle_csr(mock_parser, mock_parsed_args) From a7d0b1a7d38111eeea96bf3aaa0eedee46ecc234 Mon Sep 17 00:00:00 2001 From: Peter Eckersley Date: Thu, 12 May 2016 17:49:44 -0700 Subject: [PATCH 06/19] Address review comments --- certbot/cli.py | 2 +- certbot/crypto_util.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index 796925c1d..430384a5a 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -349,7 +349,7 @@ class HelpfulArgumentParser(object): def set_test_server(self, parsed_args): - "We have --staging/--dry-run; perform sanity check and set config.server" + """We have --staging/--dry-run; perform sanity check and set config.server""" if parsed_args.server not in (flag_default("server"), constants.STAGING_URI): conflicts = ["--staging"] if parsed_args.staging else [] diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index ceec6db71..07e7f9fd2 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -178,11 +178,11 @@ def import_csr_file(csrfile, contents): :param str csrfile: CSR filename :param str contents: contens of the CSR file - :rtype: tuple - :returns: (le_util.CSR object representing the CSR, - OpenSSL FILETYPE_ representing DER or PEM, + `OpenSSL.crypto.FILETYPE_PEM` or `OpenSSL.crypto.FILETYPE_ASN1`, list of domains requested in the CSR) + + :rtype: tuple """ try: csr = le_util.CSR(file=csrfile, data=contents, form="der") From 50421e99beefd319cc864fa90cd7ba13e9f41786 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 18 May 2016 09:56:34 -0700 Subject: [PATCH 07/19] Factor loading cert/req into its own function --- certbot/crypto_util.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 3f2267af2..41e675471 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -228,15 +228,20 @@ def pyopenssl_load_certificate(data): str(error) for error in openssl_errors))) -def _get_sans_from_cert_or_req(cert_or_req_str, load_func, - typ=OpenSSL.crypto.FILETYPE_PEM): +def _load_cert_or_req(cert_or_req_str, load_func, + typ=OpenSSL.crypto.FILETYPE_PEM): try: - cert_or_req = load_func(typ, cert_or_req_str) + return load_func(typ, cert_or_req_str) except OpenSSL.crypto.Error as error: logger.exception(error) raise + + +def _get_sans_from_cert_or_req(cert_or_req_str, load_func, + typ=OpenSSL.crypto.FILETYPE_PEM): # pylint: disable=protected-access - return acme_crypto_util._pyopenssl_cert_or_req_san(cert_or_req) + return acme_crypto_util._pyopenssl_cert_or_req_san(_load_cert_or_req( + cert_or_req_str, load_func, typ)) def get_sans_from_cert(cert, typ=OpenSSL.crypto.FILETYPE_PEM): From 8e17d7498de484857daead51b56f50b186341233 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 18 May 2016 10:14:15 -0700 Subject: [PATCH 08/19] Add get_names_from_csr --- certbot/crypto_util.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index 41e675471..b273cf59f 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -272,6 +272,27 @@ def get_sans_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): csr, OpenSSL.crypto.load_certificate_request, typ) +def get_names_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): + """Get a list of domains from a CSR, including the CN if it is set. + + :param str csr: CSR (encoded). + :param typ: `OpenSSL.crypto.FILETYPE_PEM` or `OpenSSL.crypto.FILETYPE_ASN1` + + :returns: A list of domain names. + :rtype: list + + """ + loaded_csr = _load_cert_or_req( + csr, OpenSSL.crypto.load_certificate_request, typ) + common_name = loaded_csr.get_subject().CN + + # Use a set to avoid duplication with CN and Subject Alt Names + domains = set() if common_name is None else set((common_name,)) + # pylint: disable=protected-access + domains.update(acme_crypto_util._pyopenssl_cert_or_req_san(loaded_csr)) + return list(domains) + + def dump_pyopenssl_chain(chain, filetype=OpenSSL.crypto.FILETYPE_PEM): """Dump certificate chain into a bundle. From 77e4be933cbfffaadda97d45e2dfb17672de56b5 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 18 May 2016 13:59:17 -0700 Subject: [PATCH 09/19] Simplify get_names_from_csr --- certbot/crypto_util.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index b273cf59f..1f87dc816 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -284,10 +284,8 @@ def get_names_from_csr(csr, typ=OpenSSL.crypto.FILETYPE_PEM): """ loaded_csr = _load_cert_or_req( csr, OpenSSL.crypto.load_certificate_request, typ) - common_name = loaded_csr.get_subject().CN - # Use a set to avoid duplication with CN and Subject Alt Names - domains = set() if common_name is None else set((common_name,)) + domains = set(d for d in (loaded_csr.get_subject().CN,) if d is not None) # pylint: disable=protected-access domains.update(acme_crypto_util._pyopenssl_cert_or_req_san(loaded_csr)) return list(domains) From 94549219c593dfc687b01f65ee89dc2287dae06e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Wed, 18 May 2016 14:06:32 -0700 Subject: [PATCH 10/19] Add get_names_from_csr tests --- certbot/tests/crypto_util_test.py | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index ff8d8142e..eade4861f 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -234,6 +234,36 @@ class GetSANsFromCSRTest(unittest.TestCase): [], self._call(test_util.load_vector('csr-nosans.pem'))) +class GetNamesFromCSRTest(unittest.TestCase): + """Tests for certbot.crypto_util.get_names_from_csr.""" + @classmethod + def _call(cls, *args, **kwargs): + from certbot.crypto_util import get_names_from_csr + return get_names_from_csr(*args, **kwargs) + + def test_extract_one_san(self): + self.assertEqual(['example.com'], self._call( + test_util.load_vector('csr.pem'))) + + def test_extract_two_sans(self): + self.assertEqual(set(('example.com', 'www.example.com',)), set( + self._call(test_util.load_vector('csr-san.pem')))) + + def test_extract_six_sans(self): + self.assertEqual( + set(self._call(test_util.load_vector('csr-6sans.pem'))), + set(("example.com", "example.org", "example.net", + "example.info", "subdomain.example.com", + "other.subdomain.example.com",))) + + def test_parse_non_csr(self): + self.assertRaises(OpenSSL.crypto.Error, self._call, "hello there") + + def test_parse_no_sans(self): + self.assertEqual(["example.org"], + self._call(test_util.load_vector('csr-nosans.pem'))) + + class CertLoaderTest(unittest.TestCase): """Tests for certbot.crypto_util.pyopenssl_load_certificate""" From 32d32dbc1279603a8cdc0cef7e0a0c42563f71ba Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 15:20:28 -0700 Subject: [PATCH 11/19] cli.py PEP8 fixes --- certbot/cli.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/certbot/cli.py b/certbot/cli.py index af4cd3013..3bd565496 100644 --- a/certbot/cli.py +++ b/certbot/cli.py @@ -347,7 +347,6 @@ class HelpfulArgumentParser(object): return parsed_args - def set_test_server(self, parsed_args): """We have --staging/--dry-run; perform sanity check and set config.server""" @@ -370,7 +369,6 @@ class HelpfulArgumentParser(object): parsed_args.tos = True parsed_args.register_unsafely_without_email = True - def handle_csr(self, parsed_args): """Process a --csr flag.""" if parsed_args.verb != "certonly": From 73c4e8f7a40b34e8dfed12d0e784e1aef0d25523 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 16:24:49 -0700 Subject: [PATCH 12/19] Cleanup test_obtain_certificate_from_csr --- certbot/tests/client_test.py | 74 +++++++++++++----------------------- 1 file changed, 27 insertions(+), 47 deletions(-) diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 148857be7..b7195a777 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -134,59 +134,39 @@ class ClientTest(unittest.TestCase): self.acme.fetch_chain.assert_called_once_with(mock.sentinel.certr) - # FIXME move parts of this to crypto_util tests... @mock.patch("certbot.client.logger") def test_obtain_certificate_from_csr(self, mock_logger): self._mock_obtain_certificate() - from certbot import cli test_csr = le_util.CSR(form="der", file=None, data=CSR_SAN) - mock_parsed_args = mock.MagicMock() - # The CLI should believe that this is a certonly request, because - # a CSR would not be allowed with other kinds of requests! - mock_parsed_args.verb = "certonly" - with mock.patch("certbot.cli.crypto_util.le_util.CSR") as mock_CSR: - mock_CSR.return_value = test_csr - mock_parsed_args.domains = self.eg_domains[:] - mock_parsed_args.allow_subset_of_names = False - mock_parsed_args.csr = (mock.MagicMock(), mock.MagicMock()) - mock_parser = mock.MagicMock(cli.HelpfulArgumentParser) - cli.HelpfulArgumentParser.handle_csr(mock_parser, mock_parsed_args) + auth_handler = self.client.auth_handler - # Now provoke an inconsistent domains error... - mock_parsed_args.domains.append("hippopotamus.io") - self.assertRaises(errors.ConfigurationError, - cli.HelpfulArgumentParser.handle_csr, mock_parser, mock_parsed_args) - - authzr = self.client.auth_handler.get_authorizations(self.eg_domains, False) - - self.assertEqual( - (mock.sentinel.certr, mock.sentinel.chain), - self.client.obtain_certificate_from_csr( - self.eg_domains, - test_csr, - authzr=authzr)) - # and that the cert was obtained correctly - self._check_obtain_certificate() - - # Test for authzr=None - self.assertEqual( - (mock.sentinel.certr, mock.sentinel.chain), - self.client.obtain_certificate_from_csr( - self.eg_domains, - test_csr, - authzr=None)) - - self.client.auth_handler.get_authorizations.assert_called_with( - self.eg_domains) - - # Test for no auth_handler - self.client.auth_handler = None - self.assertRaises( - errors.Error, - self.client.obtain_certificate_from_csr, + authzr = auth_handler.get_authorizations(self.eg_domains, False) + self.assertEqual( + (mock.sentinel.certr, mock.sentinel.chain), + self.client.obtain_certificate_from_csr( self.eg_domains, - test_csr) - mock_logger.warning.assert_called_once_with(mock.ANY) + test_csr, + authzr=authzr)) + # and that the cert was obtained correctly + self._check_obtain_certificate() + + # Test for authzr=None + self.assertEqual( + (mock.sentinel.certr, mock.sentinel.chain), + self.client.obtain_certificate_from_csr( + self.eg_domains, + test_csr, + authzr=None)) + auth_handler.get_authorizations.assert_called_with(self.eg_domains) + + # Test for no auth_handler + self.client.auth_handler = None + self.assertRaises( + errors.Error, + self.client.obtain_certificate_from_csr, + self.eg_domains, + test_csr) + mock_logger.warning.assert_called_once_with(mock.ANY) @mock.patch("certbot.client.crypto_util") def test_obtain_certificate(self, mock_crypto_util): From 53286863fefa47b7464340b8b46bbd93b5358932 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 16:26:09 -0700 Subject: [PATCH 13/19] Simplify import_csr_file --- certbot/crypto_util.py | 31 +++++++++++++------------------ 1 file changed, 13 insertions(+), 18 deletions(-) diff --git a/certbot/crypto_util.py b/certbot/crypto_util.py index ba368b15b..68e07e059 100644 --- a/certbot/crypto_util.py +++ b/certbot/crypto_util.py @@ -180,33 +180,28 @@ def csr_matches_pubkey(csr, privkey): return False -def import_csr_file(csrfile, contents): +def import_csr_file(csrfile, data): """Import a CSR file, which can be either PEM or DER. :param str csrfile: CSR filename - :param str contents: contens of the CSR file + :param str data: contents of the CSR file - :returns: (le_util.CSR object representing the CSR, - `OpenSSL.crypto.FILETYPE_PEM` or `OpenSSL.crypto.FILETYPE_ASN1`, + :returns: (`OpenSSL.crypto.FILETYPE_PEM` or `OpenSSL.crypto.FILETYPE_ASN1`, + le_util.CSR object representing the CSR, list of domains requested in the CSR) - :rtype: tuple + """ - try: - csr = le_util.CSR(file=csrfile, data=contents, form="der") - typ = OpenSSL.crypto.FILETYPE_ASN1 - domains = get_sans_from_csr(csr.data, OpenSSL.crypto.FILETYPE_ASN1) - except OpenSSL.crypto.Error: + for form, typ in (("der", OpenSSL.crypto.FILETYPE_ASN1,), + ("pem", OpenSSL.crypto.FILETYPE_PEM,),): try: - e1 = traceback.format_exc() - typ = OpenSSL.crypto.FILETYPE_PEM - csr = le_util.CSR(file=csrfile, data=contents, form="pem") - domains = get_sans_from_csr(csr.data, typ) + domains = get_names_from_csr(data, typ) except OpenSSL.crypto.Error: - logger.debug("DER CSR parse error %s", e1) - logger.debug("PEM CSR parse error %s", traceback.format_exc()) - raise errors.Error("Failed to parse CSR file: {0}".format(csrfile)) - return typ, csr, domains + logger.debug("CSR parse error (form=%s, typ=%s):", form, typ) + logger.debug(traceback.format_exc()) + continue + return typ, le_util.CSR(file=csrfile, data=data, form=form), domains + raise errors.Error("Failed to parse CSR file: {0}".format(csrfile)) def make_key(bits): From a48afd498c9c7a022eff72d8f903879f824cbe14 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 16:52:35 -0700 Subject: [PATCH 14/19] Start import_csr_file tests --- certbot/tests/crypto_util_test.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index eade4861f..af8308fd8 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -10,6 +10,7 @@ import zope.component from certbot import errors from certbot import interfaces +from certbot import le_util from certbot.tests import test_util @@ -159,6 +160,27 @@ class CSRMatchesPubkeyTest(unittest.TestCase): test_util.load_vector('csr.pem'), RSA256_KEY)) +class ImportCSRFileTest(unittest.TestCase): + """Tests for certbot.certbot_util.import_csr_file.""" + + @classmethod + def _call(cls, *args, **kwargs): + from certbot.crypto_util import import_csr_file + return import_csr_file(*args, **kwargs) + + def test_der_csr(self): + csrfile = test_util.vector_path('csr.der') + data = test_util.load_vector('csr.der') + + self.assertEqual( + (OpenSSL.crypto.FILETYPE_ASN1, + le_util.CSR(file=csrfile, + data=data, + form="der"), + ["example.com"],), + self._call(csrfile, data)) + + class MakeKeyTest(unittest.TestCase): # pylint: disable=too-few-public-methods """Tests for certbot.crypto_util.make_key.""" From 953d4957b8e32a74d894c2d3eeab56e0105ebf94 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 16:57:13 -0700 Subject: [PATCH 15/19] Add csr test --- certbot/tests/crypto_util_test.py | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index af8308fd8..d5a016320 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -180,6 +180,18 @@ class ImportCSRFileTest(unittest.TestCase): ["example.com"],), self._call(csrfile, data)) + def test_pem_csr(self): + csrfile = test_util.vector_path('csr.pem') + data = test_util.load_vector('csr.pem') + + self.assertEqual( + (OpenSSL.crypto.FILETYPE_PEM, + le_util.CSR(file=csrfile, + data=data, + form="pem"), + ["example.com"],), + self._call(csrfile, data)) + class MakeKeyTest(unittest.TestCase): # pylint: disable=too-few-public-methods """Tests for certbot.crypto_util.make_key.""" From 2c4c8c081c80d00be04149afd7da2396f52a0a93 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 17:00:06 -0700 Subject: [PATCH 16/19] Test bad csr --- certbot/tests/crypto_util_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/certbot/tests/crypto_util_test.py b/certbot/tests/crypto_util_test.py index d5a016320..eeea0f4ab 100644 --- a/certbot/tests/crypto_util_test.py +++ b/certbot/tests/crypto_util_test.py @@ -192,6 +192,11 @@ class ImportCSRFileTest(unittest.TestCase): ["example.com"],), self._call(csrfile, data)) + def test_bad_csr(self): + self.assertRaises(errors.Error, self._call, + test_util.vector_path('cert.pem'), + test_util.load_vector('cert.pem')) + class MakeKeyTest(unittest.TestCase): # pylint: disable=too-few-public-methods """Tests for certbot.crypto_util.make_key.""" From 3c11733006b03da942d0ee43a8d16c1c6bd40d77 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 17:32:37 -0700 Subject: [PATCH 17/19] fix --csr and --allow-subset-of-names test --- certbot/tests/cli_test.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index d7965a24e..62ec36d2a 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -349,8 +349,9 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods ['-d', '204.11.231.35']) def test_csr_with_besteffort(self): - args = ["--csr", CSR, "--allow-subset-of-names"] - self.assertRaises(errors.Error, self._call, args) + self.assertRaises( + errors.Error, self._call, + 'certonly --csr {0} --allow-subset-of-names'.format(CSR).split()) def test_run_with_csr(self): # This is an error because you can only use --csr with certonly From 0b85a8f1c8d26accccd148701f2e7ddd6065a6e2 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 17:37:48 -0700 Subject: [PATCH 18/19] Add a test for a CSR with no domains --- certbot/tests/cli_test.py | 6 ++++++ certbot/tests/testdata/csr-nonames.pem | 8 ++++++++ 2 files changed, 14 insertions(+) create mode 100644 certbot/tests/testdata/csr-nonames.pem diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index 62ec36d2a..c24da4989 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -362,6 +362,12 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods return assert False, "Expected supplying --csr to fail with default verb" + def test_csr_with_no_domains(self): + self.assertRaises( + errors.Error, self._call, + 'certonly --csr {0}'.format( + test_util.vector_path('csr-nonames.pem')).split()) + def _get_argument_parser(self): plugins = disco.PluginsRegistry.find_all() return functools.partial(cli.prepare_and_parse_args, plugins) diff --git a/certbot/tests/testdata/csr-nonames.pem b/certbot/tests/testdata/csr-nonames.pem new file mode 100644 index 000000000..abe1029ca --- /dev/null +++ b/certbot/tests/testdata/csr-nonames.pem @@ -0,0 +1,8 @@ +-----BEGIN CERTIFICATE REQUEST----- +MIH/MIGqAgEAMEUxCzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEw +HwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBMdGQwXDANBgkqhkiG9w0BAQEF +AANLADBIAkEArHVztFHtH92ucFJD/N/HW9AsdRsUuHUBBBDlHwNlRd3fp580rv2+ +6QWE30cWgdmJS86ObRz6lUTor4R0T+3C5QIDAQABoAAwDQYJKoZIhvcNAQELBQAD +QQBt9XLSZ9DGfWcGGaBUTCiSY7lWBegpNlCeo8pK3ydWmKpjcza+j7lF5paph2LH +lKWVQ8+xwYMscGWK0NApHGco +-----END CERTIFICATE REQUEST----- From 5cb0e0f264ef94187b774561329d2ab432e2634e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Fri, 20 May 2016 17:41:58 -0700 Subject: [PATCH 19/19] Add test for csr with inconsistent domains --- certbot/tests/cli_test.py | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/certbot/tests/cli_test.py b/certbot/tests/cli_test.py index c24da4989..4ae69e69d 100644 --- a/certbot/tests/cli_test.py +++ b/certbot/tests/cli_test.py @@ -368,6 +368,11 @@ class CLITest(unittest.TestCase): # pylint: disable=too-many-public-methods 'certonly --csr {0}'.format( test_util.vector_path('csr-nonames.pem')).split()) + def test_csr_with_inconsistent_domains(self): + self.assertRaises( + errors.Error, self._call, + 'certonly -d example.org --csr {0}'.format(CSR).split()) + def _get_argument_parser(self): plugins = disco.PluginsRegistry.find_all() return functools.partial(cli.prepare_and_parse_args, plugins)