From f092669347291463047707551a736b21c8200de9 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 16 May 2016 21:19:07 +0000 Subject: [PATCH 01/12] If cert_path provided - do not randomize it --- certbot/client.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/certbot/client.py b/certbot/client.py index 6f41a3a0b..fda7707f6 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -317,7 +317,13 @@ class Client(object): cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) + + if cert_path != constants.CLI_DEFAULTS['auth_cert_path']: + cert_file = le_util.safe_open(cert_path, chmod=0o644) + act_cert_path = cert_path + else: + cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) + try: cert_file.write(cert_pem) finally: From c0228ef1aa505a84a87529f76177f7dc6aa51214 Mon Sep 17 00:00:00 2001 From: sagi Date: Mon, 16 May 2016 22:11:15 +0000 Subject: [PATCH 02/12] Boulder integration scripts provides a cert_path --- tests/boulder-integration.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index 201343525..a1245e1c9 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -43,7 +43,7 @@ export CSR_PATH="${root}/csr.der" KEY_PATH="${root}/key.pem" \ common auth --csr "$CSR_PATH" \ --cert-path "${root}/csr/cert.pem" \ --chain-path "${root}/csr/chain.pem" -openssl x509 -in "${root}/csr/0000_cert.pem" -text +openssl x509 -in "${root}/csr/cert.pem" -text openssl x509 -in "${root}/csr/0000_chain.pem" -text common --domains le3.wtf install \ From 7e3c9399e54f2fb4960296e6211595707ff2dcf5 Mon Sep 17 00:00:00 2001 From: sagi Date: Tue, 17 May 2016 22:12:11 +0000 Subject: [PATCH 03/12] Use cli.set_by_cli to detect if the user explicitly set cert_path --- certbot/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/certbot/client.py b/certbot/client.py index fda7707f6..dd38e47b0 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -24,6 +24,7 @@ from certbot import interfaces from certbot import le_util from certbot import reverter from certbot import storage +from certbot import cli from certbot.display import ops as display_ops from certbot.display import enhancements @@ -318,7 +319,7 @@ class Client(object): cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - if cert_path != constants.CLI_DEFAULTS['auth_cert_path']: + if cli.set_by_cli('cert_path'): cert_file = le_util.safe_open(cert_path, chmod=0o644) act_cert_path = cert_path else: From e7374811294be55b45d8de30a36883f836da6590 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 19 May 2016 18:20:27 +0000 Subject: [PATCH 04/12] WIP --- certbot/client.py | 20 ++++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index dd38e47b0..6dd0420eb 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -292,6 +292,7 @@ class Client(object): key.pem, crypto_util.dump_pyopenssl_chain(chain), configuration.RenewerConfiguration(self.config.namespace)) + def save_certificate(self, certr, chain_cert, cert_path, chain_path, fullchain_path): """Saves the certificate received from the ACME server. @@ -318,12 +319,15 @@ class Client(object): cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - + """ if cli.set_by_cli('cert_path'): cert_file = le_util.safe_open(cert_path, chmod=0o644) act_cert_path = cert_path else: cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) + """ + cert_file, act_cert_path = _open_pem_file('cert_path', cert_path) + #import ipdb; ipdb.set_trace try: cert_file.write(cert_pem) @@ -331,7 +335,14 @@ class Client(object): cert_file.close() logger.info("Server issued certificate; certificate written to %s", act_cert_path) - + + if cli.set_by_cli('chain_path'): + #import ipdb; ipdb.set_trace() + pass + if cli.set_by_cli('fullchain_path'): + #import ipdb; ipdb.set_trace() + pass + cert_chain_abspath = None fullchain_abspath = None if chain_cert: @@ -569,6 +580,11 @@ def view_config_changes(config, num=None): rev.recovery_routine() rev.view_config_changes(num) +def _open_pem_file(cli_arg_path, pem_path): + if cli.set_by_cli(cli_arg_path): + return le_util.safe_open(pem_path, chmod=0o644), pem_path + else: + return le_util.unique_file(pem_path, 0o644) def _save_chain(chain_pem, chain_path): """Saves chain_pem at a unique path based on chain_path. From fde151848d4b1a29ada511db77308979ba247989 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 19 May 2016 19:11:25 +0000 Subject: [PATCH 05/12] Use set_by_cli for fullchain_path and chain_path --- certbot/client.py | 40 ++++++++++++++++------------------------ certbot/le_util.py | 3 ++- 2 files changed, 18 insertions(+), 25 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 6dd0420eb..3475312f0 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -319,15 +319,8 @@ class Client(object): cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - """ - if cli.set_by_cli('cert_path'): - cert_file = le_util.safe_open(cert_path, chmod=0o644) - act_cert_path = cert_path - else: - cert_file, act_cert_path = le_util.unique_file(cert_path, 0o644) - """ + cert_file, act_cert_path = _open_pem_file('cert_path', cert_path) - #import ipdb; ipdb.set_trace try: cert_file.write(cert_pem) @@ -335,21 +328,20 @@ class Client(object): cert_file.close() logger.info("Server issued certificate; certificate written to %s", act_cert_path) - - if cli.set_by_cli('chain_path'): - #import ipdb; ipdb.set_trace() - pass - if cli.set_by_cli('fullchain_path'): - #import ipdb; ipdb.set_trace() - pass - + cert_chain_abspath = None fullchain_abspath = None if chain_cert: chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert) - cert_chain_abspath = _save_chain(chain_pem, chain_path) + + chain_file, act_chain_path =\ + _open_pem_file('chain_path', chain_path) + fullchain_file, act_fullchain_path =\ + _open_pem_file('fullchain_path', fullchain_path) + + cert_chain_abspath = _save_chain(chain_pem, chain_file) fullchain_abspath = _save_chain(cert_pem + chain_pem, - fullchain_path) + fullchain_file) return os.path.abspath(act_cert_path), cert_chain_abspath, fullchain_abspath @@ -582,27 +574,27 @@ def view_config_changes(config, num=None): def _open_pem_file(cli_arg_path, pem_path): if cli.set_by_cli(cli_arg_path): - return le_util.safe_open(pem_path, chmod=0o644), pem_path + return le_util.safe_open(pem_path, chmod=0o644),\ + os.path.abspath(pem_path) else: return le_util.unique_file(pem_path, 0o644) -def _save_chain(chain_pem, chain_path): +def _save_chain(chain_pem, chain_file): """Saves chain_pem at a unique path based on chain_path. :param str chain_pem: certificate chain in PEM format - :param str chain_path: candidate path for the cert chain + :param str chain_file: chain file object :returns: absolute path to saved cert chain :rtype: str """ - chain_file, act_chain_path = le_util.unique_file(chain_path, 0o644) try: chain_file.write(chain_pem) finally: chain_file.close() - logger.info("Cert chain written to %s", act_chain_path) + logger.info("Cert chain written to %s", chain_file.name) # This expects a valid chain file - return os.path.abspath(act_chain_path) + return os.path.abspath(chain_file.name) diff --git a/certbot/le_util.py b/certbot/le_util.py index f5148b949..fe2577a4c 100644 --- a/certbot/le_util.py +++ b/certbot/le_util.py @@ -151,7 +151,8 @@ def _unique_file(path, filename_pat, count, mode): while True: current_path = os.path.join(path, filename_pat(count)) try: - return safe_open(current_path, chmod=mode), current_path + return safe_open(current_path, chmod=mode),\ + os.path.abspath(current_path) except OSError as err: # "File exists," is okay, try a different name. if err.errno != errno.EEXIST: From e1eb3eff164610b7359582478d02c3fbf4b8c6b9 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 19 May 2016 19:27:18 +0000 Subject: [PATCH 06/12] Improve code reuse --- certbot/client.py | 26 +++++++++----------------- 1 file changed, 9 insertions(+), 17 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 3475312f0..ee1ab8bb8 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -320,30 +320,27 @@ class Client(object): cert_pem = OpenSSL.crypto.dump_certificate( OpenSSL.crypto.FILETYPE_PEM, certr.body.wrapped) - cert_file, act_cert_path = _open_pem_file('cert_path', cert_path) + cert_file, abs_cert_path = _open_pem_file('cert_path', cert_path) try: cert_file.write(cert_pem) finally: cert_file.close() logger.info("Server issued certificate; certificate written to %s", - act_cert_path) + abs_cert_path) - cert_chain_abspath = None - fullchain_abspath = None if chain_cert: chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert) - chain_file, act_chain_path =\ + chain_file, abs_chain_path =\ _open_pem_file('chain_path', chain_path) - fullchain_file, act_fullchain_path =\ + fullchain_file, abs_fullchain_path =\ _open_pem_file('fullchain_path', fullchain_path) - cert_chain_abspath = _save_chain(chain_pem, chain_file) - fullchain_abspath = _save_chain(cert_pem + chain_pem, - fullchain_file) + _save_chain(chain_pem, chain_file) + _save_chain(cert_pem + chain_pem, fullchain_file - return os.path.abspath(act_cert_path), cert_chain_abspath, fullchain_abspath + return abs_cert_path, abs_chain_path, abs_fullchain_path def deploy_certificate(self, domains, privkey_path, cert_path, chain_path, fullchain_path): @@ -577,7 +574,8 @@ def _open_pem_file(cli_arg_path, pem_path): return le_util.safe_open(pem_path, chmod=0o644),\ os.path.abspath(pem_path) else: - return le_util.unique_file(pem_path, 0o644) + uniq = le_util.unique_file(pem_path, 0o644) + return uniq[0], os.path.abspath(uniq) def _save_chain(chain_pem, chain_file): """Saves chain_pem at a unique path based on chain_path. @@ -585,9 +583,6 @@ def _save_chain(chain_pem, chain_file): :param str chain_pem: certificate chain in PEM format :param str chain_file: chain file object - :returns: absolute path to saved cert chain - :rtype: str - """ try: chain_file.write(chain_pem) @@ -595,6 +590,3 @@ def _save_chain(chain_pem, chain_file): chain_file.close() logger.info("Cert chain written to %s", chain_file.name) - - # This expects a valid chain file - return os.path.abspath(chain_file.name) From 501c19ef2a254e44b768eb4cd57e5832f5afb792 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 19 May 2016 19:33:04 +0000 Subject: [PATCH 07/12] Syntax --- certbot/client.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/certbot/client.py b/certbot/client.py index ee1ab8bb8..8964686e6 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -338,7 +338,7 @@ class Client(object): _open_pem_file('fullchain_path', fullchain_path) _save_chain(chain_pem, chain_file) - _save_chain(cert_pem + chain_pem, fullchain_file + _save_chain(cert_pem + chain_pem, fullchain_file) return abs_cert_path, abs_chain_path, abs_fullchain_path From 3589b25dc3a7d4c0fb2477b21d6039a327bd1b52 Mon Sep 17 00:00:00 2001 From: sagi Date: Thu, 19 May 2016 19:35:38 +0000 Subject: [PATCH 08/12] Make lint happy --- certbot/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/certbot/client.py b/certbot/client.py index 8964686e6..4fc662948 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -292,7 +292,6 @@ class Client(object): key.pem, crypto_util.dump_pyopenssl_chain(chain), configuration.RenewerConfiguration(self.config.namespace)) - def save_certificate(self, certr, chain_cert, cert_path, chain_path, fullchain_path): """Saves the certificate received from the ACME server. From 7689de2ad8da074015df2150b455115f3b5816b5 Mon Sep 17 00:00:00 2001 From: sagi Date: Fri, 20 May 2016 01:18:50 +0000 Subject: [PATCH 09/12] Fix tests --- certbot/client.py | 12 +++++++++++- certbot/tests/client_test.py | 8 +++++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 4fc662948..818701f08 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -569,12 +569,22 @@ def view_config_changes(config, num=None): rev.view_config_changes(num) def _open_pem_file(cli_arg_path, pem_path): + """Open a pem file. + + If cli_arg_path was set by the client, open that. + Otherwise, uniquify the file path. + + :param str cli_arg_path: the cli arg name, e.g. cert_path + :param str pem_path: the pem file path to open + + :returns a file object + """ if cli.set_by_cli(cli_arg_path): return le_util.safe_open(pem_path, chmod=0o644),\ os.path.abspath(pem_path) else: uniq = le_util.unique_file(pem_path, 0o644) - return uniq[0], os.path.abspath(uniq) + return uniq[0], os.path.abspath(uniq[1]) def _save_chain(chain_pem, chain_file): """Saves chain_pem at a unique path based on chain_path. diff --git a/certbot/tests/client_test.py b/certbot/tests/client_test.py index 8ceefe8ae..49596fdee 100644 --- a/certbot/tests/client_test.py +++ b/certbot/tests/client_test.py @@ -221,7 +221,9 @@ class ClientTest(unittest.TestCase): mock.sentinel.key, domains, self.config.csr_dir) self._check_obtain_certificate() - def test_save_certificate(self): + @mock.patch("certbot.cli.helpful_parser") + def test_save_certificate(self, mock_parser): + # pylint: disable=too-many-locals certs = ["matching_cert.pem", "cert.pem", "cert-san.pem"] tmp_path = tempfile.mkdtemp() os.chmod(tmp_path, 0o755) # TODO: really?? @@ -232,6 +234,10 @@ class ClientTest(unittest.TestCase): candidate_cert_path = os.path.join(tmp_path, "certs", "cert.pem") candidate_chain_path = os.path.join(tmp_path, "chains", "chain.pem") candidate_fullchain_path = os.path.join(tmp_path, "chains", "fullchain.pem") + mock_parser.verb = "certonly" + mock_parser.args = ["--cert-path", candidate_cert_path, + "--chain-path", candidate_chain_path, + "--fullchain-path", candidate_fullchain_path] cert_path, chain_path, fullchain_path = self.client.save_certificate( certr, chain_cert, candidate_cert_path, candidate_chain_path, From b54497d8145e0999ac3f163ae38e3bc70a7a5549 Mon Sep 17 00:00:00 2001 From: sagi Date: Tue, 24 May 2016 19:33:13 +0000 Subject: [PATCH 10/12] Fix chain filename --- tests/boulder-integration.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/boulder-integration.sh b/tests/boulder-integration.sh index a1245e1c9..323ea004b 100755 --- a/tests/boulder-integration.sh +++ b/tests/boulder-integration.sh @@ -44,7 +44,7 @@ common auth --csr "$CSR_PATH" \ --cert-path "${root}/csr/cert.pem" \ --chain-path "${root}/csr/chain.pem" openssl x509 -in "${root}/csr/cert.pem" -text -openssl x509 -in "${root}/csr/0000_chain.pem" -text +openssl x509 -in "${root}/csr/chain.pem" -text common --domains le3.wtf install \ --cert-path "${root}/csr/cert.pem" \ From e93aeb88dd4da8e26fd6a4c257234ba990aae403 Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 25 May 2016 04:19:18 +0000 Subject: [PATCH 11/12] Fix docs --- certbot/client.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/certbot/client.py b/certbot/client.py index 514da879e..9ce326822 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -580,7 +580,8 @@ def _open_pem_file(cli_arg_path, pem_path): :param str cli_arg_path: the cli arg name, e.g. cert_path :param str pem_path: the pem file path to open - :returns a file object + :returns: a tuple of file object and its absolute file path + """ if cli.set_by_cli(cli_arg_path): return le_util.safe_open(pem_path, chmod=0o644),\ From d1df72d63c7eb9e590d05651f0f1a41caf234a1d Mon Sep 17 00:00:00 2001 From: sagi Date: Wed, 25 May 2016 19:06:25 +0000 Subject: [PATCH 12/12] Add the chain_cert is None case --- certbot/client.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/certbot/client.py b/certbot/client.py index 9ce326822..ba31f8760 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -328,7 +328,9 @@ class Client(object): logger.info("Server issued certificate; certificate written to %s", abs_cert_path) - if chain_cert: + if not chain_cert: + return abs_cert_path, None, None + else: chain_pem = crypto_util.dump_pyopenssl_chain(chain_cert) chain_file, abs_chain_path =\ @@ -339,7 +341,7 @@ class Client(object): _save_chain(chain_pem, chain_file) _save_chain(cert_pem + chain_pem, fullchain_file) - return abs_cert_path, abs_chain_path, abs_fullchain_path + return abs_cert_path, abs_chain_path, abs_fullchain_path def deploy_certificate(self, domains, privkey_path, cert_path, chain_path, fullchain_path):