From 18da7dfce2e9128b2e27c89935a9f3be4385a03e Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sun, 8 Nov 2015 14:19:58 -0600 Subject: [PATCH 01/23] Implement @pde's suggestions for Apache From this IRC log: 2015-11-02 16:31:29 @pdeee for >= 2.4.8: 2015-11-02 16:32:23 @pdeee add new SSLCertificateFile pointing to fullchain.pem 2015-11-02 16:33:10 @pdeee remove all preexisting SSLCertificateFile, SSLCertificateChainFile, SSLCACertificatePath, and possibly other fields subject to careful research :) 2015-11-02 16:33:21 @pdeee for < 2.4.8: 2015-11-02 16:34:03 @pdeee add SSLCertificateFile pointing to cert.pem 2015-11-02 16:34:42 @pdeee and SSLCertificateChainFile pointing to chain.pem 2015-11-02 16:34:50 xamnesiax gotcha 2015-11-02 16:34:55 @pdeee remove all preexisting/conflicting entries 2015-11-02 16:35:19 xamnesiax Am I correct to assume that this can all be done from deploy_certs in the apache configurator? 2015-11-02 16:36:32 xamnesiax deploy_cert * 2015-11-02 16:36:48 @pdeee I think so 2015-11-02 16:36:59 @pdeee again, jdkasten may wish to say more Pull strings out for find_dir A bit of logging Add version logging Logging, temporarily remove one branch of the conditional for testing Fix bad directive stringgrabbing code Fix directive removal logic Grab string from tree to be removed --- .../letsencrypt_apache/configurator.py | 24 ++++++++++++------- 1 file changed, 16 insertions(+), 8 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d376fe4b6..173be4104 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -212,14 +212,22 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) # Assign the final directives; order is maintained in find_dir - self.aug.set(path["cert_path"][-1], cert_path) - self.aug.set(path["cert_key"][-1], key_path) - if chain_path is not None: - if not path["chain_path"]: - self.parser.add_dir( - vhost.path, "SSLCertificateChainFile", chain_path) - else: - self.aug.set(path["chain_path"][-1], chain_path) + if self.version >= (2, 4, 8): + logger.debug("Apache version (%s) is >= 2.4.8", + ".".join(map(str,self.version))) + for directive in ["SSLCertificateKeyFile", "SSLCertificateChainFile", + "SSLCACertificatePath"]: + logging.debug("Trying to delete directive '%s'", directive) + directive_tree = self.parser.find_dir(directive, None, vhost.path) + logging.debug(directive_tree) + if directive_tree: + logger.debug("Removing directive %s", directive) + self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) + logging.debug("fullchain path: %s", fullchain_path) + self.aug.set(path["cert_path"][-1], fullchain_path) + elif self.version < (2, 4, 8): + logger.debug("Apache version (%s) is < 2.4.8", + ".".join(map(str,self.version))) # Save notes about the transaction that took place self.save_notes += ("Changed vhost at %s with addresses of %s\n" From 1d2ba931b37cfca5d37d123d124a785d63f53121 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sun, 8 Nov 2015 16:47:09 -0600 Subject: [PATCH 02/23] Improve the implementation of the suggestion Write the code to set directives Fix logging in _remove_existing_ssl_directives Fix logging statement --- .../letsencrypt_apache/configurator.py | 29 ++++++++++--------- 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 173be4104..eb8268e33 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -213,21 +213,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Assign the final directives; order is maintained in find_dir if self.version >= (2, 4, 8): - logger.debug("Apache version (%s) is >= 2.4.8", - ".".join(map(str,self.version))) - for directive in ["SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCACertificatePath"]: - logging.debug("Trying to delete directive '%s'", directive) - directive_tree = self.parser.find_dir(directive, None, vhost.path) - logging.debug(directive_tree) - if directive_tree: - logger.debug("Removing directive %s", directive) - self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) - logging.debug("fullchain path: %s", fullchain_path) self.aug.set(path["cert_path"][-1], fullchain_path) elif self.version < (2, 4, 8): - logger.debug("Apache version (%s) is < 2.4.8", - ".".join(map(str,self.version))) + self.aug.set(path["cert_path"][-1], cert_path) + self.aug.set(path["chain_path"][-1], chain_path) # Save notes about the transaction that took place self.save_notes += ("Changed vhost at %s with addresses of %s\n" @@ -583,6 +572,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Update Addresses self._update_ssl_vhosts_addrs(vh_p) + # Remove existing SSL directives + logging.info("Removing existing SSL directives") + self._remove_existing_ssl_directives(vh_p) + # Add directives self._add_dummy_ssl_directives(vh_p) @@ -651,6 +644,16 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs + def _remove_existing_ssl_directives(self, vh_path): + for directive in ["SSLCertificateKeyFile", "SSLCertificateChainFile", + "SSLCACertificatePath", "SSLCertificateFile"]: + logger.debug("Trying to delete directive '%s'", directive) + directive_tree = self.parser.find_dir(directive, None, vh_path) + logger.debug("Parser found %s", directive_tree) + if directive_tree: + logger.debug("Removing directive %s", directive) + self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) + def _add_dummy_ssl_directives(self, vh_path): self.parser.add_dir(vh_path, "SSLCertificateFile", "insert_cert_file_path") From b26c13893864d15c3fbc7c646df003c50b1e9463 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sun, 8 Nov 2015 17:37:28 -0600 Subject: [PATCH 03/23] Wire in everything, remove cert_key Add debug. Will be removed. Logging --- .../letsencrypt_apache/configurator.py | 34 ++++++++++++------- 1 file changed, 22 insertions(+), 12 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index eb8268e33..ecb6fe09a 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -192,39 +192,51 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): path["cert_path"] = self.parser.find_dir( "SSLCertificateFile", None, vhost.path) - path["cert_key"] = self.parser.find_dir( - "SSLCertificateKeyFile", None, vhost.path) # Only include if a certificate chain is specified if chain_path is not None: path["chain_path"] = self.parser.find_dir( "SSLCertificateChainFile", None, vhost.path) - if not path["cert_path"] or not path["cert_key"]: + if not path["cert_path"]: # Throw some can't find all of the directives error" logger.warn( - "Cannot find a cert or key directive in %s. " + "Cannot find a cert directive in %s. " "VirtualHost was not modified", vhost.path) # Presumably break here so that the virtualhost is not modified raise errors.PluginError( - "Unable to find cert and/or key directives") + "Unable to find cert directive") logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) # Assign the final directives; order is maintained in find_dir if self.version >= (2, 4, 8): + logger.debug("Apache version (%s) is >= 2.4.8", + ".".join(map(str,self.version))) + set_cert_path = fullchain_path + logger.debug(fullchain_path) + logger.debug(path["cert_path"][-1]) self.aug.set(path["cert_path"][-1], fullchain_path) elif self.version < (2, 4, 8): + logger.debug("Apache version (%s) is < 2.4.8", + ".".join(map(str,self.version))) + set_cert_path = cert_path self.aug.set(path["cert_path"][-1], cert_path) - self.aug.set(path["chain_path"][-1], chain_path) + if not path["chain_path"]: + self.parser.add_dir(vhost.path, + "SSLCertificateChainFile", chain_path) + else: + self.aug.set(path["chain_path"][-1], chain_path) + + with open("%s/sites-available/%s" % (self.parser.root, os.path.basename(vhost.filep))) as f: + logger.debug(f.read()) # Save notes about the transaction that took place self.save_notes += ("Changed vhost at %s with addresses of %s\n" - "\tSSLCertificateFile %s\n" - "\tSSLCertificateKeyFile %s\n" % + "\tSSLCertificateFile %s\n" % (vhost.filep, ", ".join(str(addr) for addr in vhost.addrs), - cert_path, key_path)) + set_cert_path)) if chain_path is not None: self.save_notes += "\tSSLCertificateChainFile %s\n" % chain_path @@ -573,7 +585,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self._update_ssl_vhosts_addrs(vh_p) # Remove existing SSL directives - logging.info("Removing existing SSL directives") + logger.info("Removing existing SSL directives") self._remove_existing_ssl_directives(vh_p) # Add directives @@ -657,8 +669,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _add_dummy_ssl_directives(self, vh_path): self.parser.add_dir(vh_path, "SSLCertificateFile", "insert_cert_file_path") - self.parser.add_dir(vh_path, "SSLCertificateKeyFile", - "insert_key_file_path") self.parser.add_dir(vh_path, "Include", self.mod_ssl_conf) def _add_name_vhost_if_necessary(self, vhost): From e63fa279a489b552ff770bb9aaac4e7de17ba1f6 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Mon, 9 Nov 2015 17:04:21 -0600 Subject: [PATCH 04/23] Reintroduce cert_key, remove bad logging --- .../letsencrypt_apache/configurator.py | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ecb6fe09a..d4a738925 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -192,20 +192,22 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): path["cert_path"] = self.parser.find_dir( "SSLCertificateFile", None, vhost.path) + path["cert_key"] = self.parser.find_dir( + "SSLCertificateKeyFile", None, vhost.path) # Only include if a certificate chain is specified if chain_path is not None: path["chain_path"] = self.parser.find_dir( "SSLCertificateChainFile", None, vhost.path) - if not path["cert_path"]: + if not path["cert_path"] or not path["cert_key"]: # Throw some can't find all of the directives error" logger.warn( - "Cannot find a cert directive in %s. " + "Cannot find a cert or key directive in %s. " "VirtualHost was not modified", vhost.path) # Presumably break here so that the virtualhost is not modified raise errors.PluginError( - "Unable to find cert directive") + "Unable to find cert and/or key directives") logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) @@ -214,29 +216,27 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.debug("Apache version (%s) is >= 2.4.8", ".".join(map(str,self.version))) set_cert_path = fullchain_path - logger.debug(fullchain_path) - logger.debug(path["cert_path"][-1]) self.aug.set(path["cert_path"][-1], fullchain_path) + self.aug.set(path["cert_key"][-1], key_path) elif self.version < (2, 4, 8): logger.debug("Apache version (%s) is < 2.4.8", ".".join(map(str,self.version))) set_cert_path = cert_path self.aug.set(path["cert_path"][-1], cert_path) + self.aug.set(path["cert_key"][-1], key_path) if not path["chain_path"]: self.parser.add_dir(vhost.path, "SSLCertificateChainFile", chain_path) else: self.aug.set(path["chain_path"][-1], chain_path) - with open("%s/sites-available/%s" % (self.parser.root, os.path.basename(vhost.filep))) as f: - logger.debug(f.read()) - # Save notes about the transaction that took place self.save_notes += ("Changed vhost at %s with addresses of %s\n" - "\tSSLCertificateFile %s\n" % + "\tSSLCertificateFile %s\n" + "\tSSLCertificateKeyFile %s\n" % (vhost.filep, ", ".join(str(addr) for addr in vhost.addrs), - set_cert_path)) + set_cert_path, key_path)) if chain_path is not None: self.save_notes += "\tSSLCertificateChainFile %s\n" % chain_path @@ -669,6 +669,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _add_dummy_ssl_directives(self, vh_path): self.parser.add_dir(vh_path, "SSLCertificateFile", "insert_cert_file_path") + self.parser.add_dir(vh_path, "SSLCertificateKeyFile", + "insert_key_file_path") self.parser.add_dir(vh_path, "Include", self.mod_ssl_conf) def _add_name_vhost_if_necessary(self, vhost): From 30c44ef1e274be6873c5a4adcda5269f25b7690e Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Mon, 9 Nov 2015 17:42:38 -0600 Subject: [PATCH 05/23] Fix lint errors --- letsencrypt-apache/letsencrypt_apache/configurator.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d4a738925..5d5907895 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -214,13 +214,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Assign the final directives; order is maintained in find_dir if self.version >= (2, 4, 8): logger.debug("Apache version (%s) is >= 2.4.8", - ".".join(map(str,self.version))) + ".".join(str(i) for i in self.version)) set_cert_path = fullchain_path self.aug.set(path["cert_path"][-1], fullchain_path) self.aug.set(path["cert_key"][-1], key_path) elif self.version < (2, 4, 8): logger.debug("Apache version (%s) is < 2.4.8", - ".".join(map(str,self.version))) + ".".join(str(i) for i in self.version)) set_cert_path = cert_path self.aug.set(path["cert_path"][-1], cert_path) self.aug.set(path["cert_key"][-1], key_path) @@ -663,8 +663,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): directive_tree = self.parser.find_dir(directive, None, vh_path) logger.debug("Parser found %s", directive_tree) if directive_tree: - logger.debug("Removing directive %s", directive) - self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) + logger.debug("Removing directive %s", directive) + self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) def _add_dummy_ssl_directives(self, vh_path): self.parser.add_dir(vh_path, "SSLCertificateFile", From 1f6ef1f4b158018d732cbfc52144cfef221822fb Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Tue, 10 Nov 2015 15:54:54 -0600 Subject: [PATCH 06/23] Add tests for existing cert removal and newcert directives --- .../tests/configurator_test.py | 47 +++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 7c2137c45..b44b8bdda 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -230,6 +230,38 @@ class TwoVhost80Test(util.ApacheTest): self.config.enable_site, obj.VirtualHost("asdf", "afsaf", set(), False, False)) + def test_deploy_cert_newssl(self): + self.config = util.get_apache_configurator( + self.config_path, self.config_dir, self.work_dir, version=(2, 4, 16)) + + self.config.parser.modules.add("ssl_module") + self.config.parser.modules.add("mod_ssl.c") + + # Get the default 443 vhost + self.config.assoc["random.demo"] = self.vh_truth[1] + self.config.deploy_cert( + "random.demo", "example/cert.pem", "example/key.pem", + "example/cert_chain.pem", "example/fullchain.pem") + self.config.save() + + # Verify ssl_module was enabled. + self.assertTrue(self.vh_truth[1].enabled) + self.assertTrue("ssl_module" in self.config.parser.modules) + + loc_cert = self.config.parser.find_dir( + "sslcertificatefile", "example/fullchain.pem", self.vh_truth[1].path) + loc_key = self.config.parser.find_dir( + "sslcertificateKeyfile", "example/key.pem", self.vh_truth[1].path) + + # Verify one directive was found in the correct file + self.assertEqual(len(loc_cert), 1) + self.assertEqual(configurator.get_file_path(loc_cert[0]), + self.vh_truth[1].filep) + + self.assertEqual(len(loc_key), 1) + self.assertEqual(configurator.get_file_path(loc_key[0]), + self.vh_truth[1].filep) + def test_deploy_cert(self): self.config.parser.modules.add("ssl_module") self.config.parser.modules.add("mod_ssl.c") @@ -347,6 +379,21 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual(len(self.config.vhosts), 5) + def test_remove_existing_ssl_directives(self): + # pylint: disable=protected-access + BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", "SSLCertificateChainFile", + "SSLCACertificatePath", "SSLCertificateFile"] + for directive in BOGUS_DIRECTIVES: + self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) + self.config.save() + self.config._remove_existing_ssl_directives(self.vh_truth[0].path) + self.config.save() + + for directive in BOGUS_DIRECTIVES: + self.assertEqual( + self.config.parser.find_dir(directive, None, self.vh_truth[0].path), + []) + def test_make_vhost_ssl_extra_vhs(self): self.config.aug.match = mock.Mock(return_value=["p1", "p2"]) self.assertRaises( From 211c2bb33d668c13f8d2e89fb95c88806737d1dd Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Tue, 10 Nov 2015 19:41:30 -0600 Subject: [PATCH 07/23] Remove SSLCACertificatePath from removed directives SSLCACertificatePath is sometimes important to preserve. --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- .../letsencrypt_apache/tests/configurator_test.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 5d5907895..d8e929079 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -658,7 +658,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def _remove_existing_ssl_directives(self, vh_path): for directive in ["SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCACertificatePath", "SSLCertificateFile"]: + "SSLCertificateFile"]: logger.debug("Trying to delete directive '%s'", directive) directive_tree = self.parser.find_dir(directive, None, vh_path) logger.debug("Parser found %s", directive_tree) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index b44b8bdda..f6cef0470 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -381,8 +381,8 @@ class TwoVhost80Test(util.ApacheTest): def test_remove_existing_ssl_directives(self): # pylint: disable=protected-access - BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCACertificatePath", "SSLCertificateFile"] + BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", + "SSLCertificateChainFile", "SSLCertificateFile"] for directive in BOGUS_DIRECTIVES: self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) self.config.save() From 108757e3323bf236ac567559b1d8eb0230fb7a15 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Thu, 12 Nov 2015 17:45:33 -0600 Subject: [PATCH 08/23] Fall back to old cert method if fullchain isn't provided --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d8e929079..d891c39a9 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -212,13 +212,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) # Assign the final directives; order is maintained in find_dir - if self.version >= (2, 4, 8): + if self.version >= (2, 4, 8) and fullchain_path is not None: logger.debug("Apache version (%s) is >= 2.4.8", ".".join(str(i) for i in self.version)) set_cert_path = fullchain_path self.aug.set(path["cert_path"][-1], fullchain_path) self.aug.set(path["cert_key"][-1], key_path) - elif self.version < (2, 4, 8): + else: # fall back to old SSL cert method logger.debug("Apache version (%s) is < 2.4.8", ".".join(str(i) for i in self.version)) set_cert_path = cert_path From 0af0beaeb7f83eff4fd17078f9df8688e08abeca Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Thu, 12 Nov 2015 22:27:05 -0600 Subject: [PATCH 09/23] Remove useless SSL removal on non-SSL vhosts --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d891c39a9..154b19f5a 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -584,10 +584,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Update Addresses self._update_ssl_vhosts_addrs(vh_p) - # Remove existing SSL directives - logger.info("Removing existing SSL directives") - self._remove_existing_ssl_directives(vh_p) - # Add directives self._add_dummy_ssl_directives(vh_p) From 16659b54333c379f44270c62b4c6ab3791531623 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Fri, 13 Nov 2015 15:49:59 -0600 Subject: [PATCH 10/23] Add `minus` option to _remove_existing_ssl_directives() Add test case as well. --- .../letsencrypt_apache/configurator.py | 7 ++--- .../tests/configurator_test.py | 26 +++++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 154b19f5a..c52f3fc70 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -652,9 +652,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs - def _remove_existing_ssl_directives(self, vh_path): - for directive in ["SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCertificateFile"]: + def _remove_existing_ssl_directives(self, vh_path, minus={}): + directives_to_remove = list({"SSLCertificateKeyFile", "SSLCertificateChainFile", + "SSLCertificateFile"} - set(minus)) + for directive in directives_to_remove: logger.debug("Trying to delete directive '%s'", directive) directive_tree = self.parser.find_dir(directive, None, vh_path) logger.debug("Parser found %s", directive_tree) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index f6cef0470..0632db30a 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -386,6 +386,7 @@ class TwoVhost80Test(util.ApacheTest): for directive in BOGUS_DIRECTIVES: self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) self.config.save() + self.config._remove_existing_ssl_directives(self.vh_truth[0].path) self.config.save() @@ -394,6 +395,31 @@ class TwoVhost80Test(util.ApacheTest): self.config.parser.find_dir(directive, None, self.vh_truth[0].path), []) + def test_remove_existing_ssl_directives_minus_some(self): + # pylint: disable=protected-access + BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", + "SSLCertificateChainFile", "SSLCertificateFile"] + MINUS_DIRECTIVES = ["SSLCertificateKeyFile", "SSLCertificateFile"] + for directive in BOGUS_DIRECTIVES: + self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) + self.config.save() + + self.config._remove_existing_ssl_directives(self.vh_truth[0].path, + minus=MINUS_DIRECTIVES) + self.config.save() + + for directive in BOGUS_DIRECTIVES: + if directive not in MINUS_DIRECTIVES: + self.assertEqual( + self.config.parser.find_dir(directive, None, self.vh_truth[0].path), + [], + msg="directive %s should have been removed" % directive) + else: + self.assertNotEqual( + self.config.parser.find_dir(directive, None, self.vh_truth[0].path), + [], + msg="directive %s should still exist" % directive) + def test_make_vhost_ssl_extra_vhs(self): self.config.aug.match = mock.Mock(return_value=["p1", "p2"]) self.assertRaises( From 9bf1b99b5bfcb5d7ab743b8b49068ba5bd8a463b Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Fri, 13 Nov 2015 17:16:50 -0600 Subject: [PATCH 11/23] Remove existing SSL directives for SSL vhosts --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index c52f3fc70..ccb0ebc62 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -265,6 +265,12 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Try to find a reasonable vhost vhost = self._find_best_vhost(target_name) if vhost is not None: + if vhost.ssl: + # remove existing SSL directives (minus the ones we'll use anyway, + # since we want to preserve order) + self._remove_existing_ssl_directives( + vhost, + minus=['SSLCertificatePath', 'SSLCertificateKeyFile']) if not vhost.ssl: vhost = self.make_vhost_ssl(vhost) From 361b67276ebea50c19ea3c291410f048ea34cfc4 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sat, 14 Nov 2015 11:26:42 -0600 Subject: [PATCH 12/23] Rewrite certificate install logic Tests are being written --- .../letsencrypt_apache/configurator.py | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ccb0ebc62..44821b262 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -210,25 +210,27 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "Unable to find cert and/or key directives") logger.info("Deploying Certificate to VirtualHost %s", vhost.filep) + logger.debug("Apache version is %s", + ".".join(str(i) for i in self.version)) - # Assign the final directives; order is maintained in find_dir - if self.version >= (2, 4, 8) and fullchain_path is not None: - logger.debug("Apache version (%s) is >= 2.4.8", - ".".join(str(i) for i in self.version)) - set_cert_path = fullchain_path - self.aug.set(path["cert_path"][-1], fullchain_path) - self.aug.set(path["cert_key"][-1], key_path) - else: # fall back to old SSL cert method - logger.debug("Apache version (%s) is < 2.4.8", - ".".join(str(i) for i in self.version)) + if self.version < (2, 4, 8) or (chain_path and not fullchain_path): + # install SSLCertificateFile, SSLCertificateKeyFile, and SSLCertificateChainFile directives set_cert_path = cert_path self.aug.set(path["cert_path"][-1], cert_path) self.aug.set(path["cert_key"][-1], key_path) - if not path["chain_path"]: - self.parser.add_dir(vhost.path, - "SSLCertificateChainFile", chain_path) - else: - self.aug.set(path["chain_path"][-1], chain_path) + if chain_path is not None: + if not path["chain_path"]: + self.parser.add_dir(vhost.path, + "SSLCertificateChainFile", chain_path) + else: + self.aug.set(path["chain_path"][-1], chain_path) + else: + if not fullchain_path: + raise errors.PluginError("Please provide the --fullchain-path\ + option pointing to your full chain file") + set_cert_path = fullchain_path + self.aug.set(path["cert_path"][-1], fullchain_path) + self.aug.set(path["cert_key"][-1], key_path) # Save notes about the transaction that took place self.save_notes += ("Changed vhost at %s with addresses of %s\n" From 425bb98bed32c904ec696e9174556305d0fbb40b Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sat, 14 Nov 2015 11:40:51 -0600 Subject: [PATCH 13/23] Fix lint warnings --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 44821b262..4f4e61d9f 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -214,7 +214,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): ".".join(str(i) for i in self.version)) if self.version < (2, 4, 8) or (chain_path and not fullchain_path): - # install SSLCertificateFile, SSLCertificateKeyFile, and SSLCertificateChainFile directives + # install SSLCertificateFile, SSLCertificateKeyFile, + # and SSLCertificateChainFile directives set_cert_path = cert_path self.aug.set(path["cert_path"][-1], cert_path) self.aug.set(path["cert_key"][-1], key_path) @@ -660,7 +661,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs - def _remove_existing_ssl_directives(self, vh_path, minus={}): + def _remove_existing_ssl_directives(self, vh_path, minus=None): + minus = minus or {} directives_to_remove = list({"SSLCertificateKeyFile", "SSLCertificateChainFile", "SSLCertificateFile"} - set(minus)) for directive in directives_to_remove: From 691abdc377ec38226f25d17e8ee03b84c4578988 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sat, 14 Nov 2015 12:00:08 -0600 Subject: [PATCH 14/23] Fix for py26 (it doesn't have set literals) --- letsencrypt-apache/letsencrypt_apache/configurator.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 4f4e61d9f..ed58b770a 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -662,9 +662,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs def _remove_existing_ssl_directives(self, vh_path, minus=None): - minus = minus or {} - directives_to_remove = list({"SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCertificateFile"} - set(minus)) + minus = minus or set() + directives_to_remove = list(set(["SSLCertificateKeyFile", "SSLCertificateChainFile", + "SSLCertificateFile"]) - set(minus)) for directive in directives_to_remove: logger.debug("Trying to delete directive '%s'", directive) directive_tree = self.parser.find_dir(directive, None, vh_path) From a1e6db2144bd80bf35a667bd8e9c3193c133f7b2 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sat, 14 Nov 2015 14:27:38 -0600 Subject: [PATCH 15/23] Fix logic in which the --fullchain error would never be hit --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ed58b770a..17617b1d2 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -213,7 +213,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.debug("Apache version is %s", ".".join(str(i) for i in self.version)) - if self.version < (2, 4, 8) or (chain_path and not fullchain_path): + if self.version < (2, 4, 8): # install SSLCertificateFile, SSLCertificateKeyFile, # and SSLCertificateChainFile directives set_cert_path = cert_path From e6113698f23d347b14c579ec65f61baddbeb7383 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sat, 14 Nov 2015 14:28:17 -0600 Subject: [PATCH 16/23] Test that no fullchain throws an error --- .../letsencrypt_apache/tests/configurator_test.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 0632db30a..d792ea59d 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -262,6 +262,20 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual(configurator.get_file_path(loc_key[0]), self.vh_truth[1].filep) + def test_deploy_cert_newssl_no_fullchain(self): + self.config = util.get_apache_configurator( + self.config_path, self.config_dir, self.work_dir, version=(2, 4, 16)) + + self.config.parser.modules.add("ssl_module") + self.config.parser.modules.add("mod_ssl.c") + + # Get the default 443 vhost + self.config.assoc["random.demo"] = self.vh_truth[1] + self.assertRaises(errors.PluginError, + lambda: self.config.deploy_cert( + "random.demo", "example/cert.pem", "example/key.pem", + "example/cert_chain.pem")) + def test_deploy_cert(self): self.config.parser.modules.add("ssl_module") self.config.parser.modules.add("mod_ssl.c") From 62f19496da76b7bf4ab5a86eda5acdbd5d705232 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sun, 15 Nov 2015 23:00:18 -0600 Subject: [PATCH 17/23] Rewrite vhost cleaning logic --- .../letsencrypt_apache/configurator.py | 38 ++++++++++--------- 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 17617b1d2..e610010a0 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -268,19 +268,21 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): # Try to find a reasonable vhost vhost = self._find_best_vhost(target_name) if vhost is not None: - if vhost.ssl: - # remove existing SSL directives (minus the ones we'll use anyway, - # since we want to preserve order) - self._remove_existing_ssl_directives( - vhost, - minus=['SSLCertificatePath', 'SSLCertificateKeyFile']) if not vhost.ssl: vhost = self.make_vhost_ssl(vhost) self.assoc[target_name] = vhost return vhost - return self._choose_vhost_from_list(target_name) + vhost = self._choose_vhost_from_list(target_name) + if vhost.ssl: + # remove duplicated or conflicting ssl directives + self._deduplicate_directives(vhost.path, + ["SSLCertificateFile", "SSLCertificateKeyFile"]) + # remove all problematic directives + self._remove_directives(vhost.path, ["SSLCertificateChainFile"]) + + return vhost def _choose_vhost_from_list(self, target_name): # Select a vhost from a list @@ -661,17 +663,17 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs - def _remove_existing_ssl_directives(self, vh_path, minus=None): - minus = minus or set() - directives_to_remove = list(set(["SSLCertificateKeyFile", "SSLCertificateChainFile", - "SSLCertificateFile"]) - set(minus)) - for directive in directives_to_remove: - logger.debug("Trying to delete directive '%s'", directive) - directive_tree = self.parser.find_dir(directive, None, vh_path) - logger.debug("Parser found %s", directive_tree) - if directive_tree: - logger.debug("Removing directive %s", directive) - self.aug.remove(re.sub(r"/\w*$", "", directive_tree[-1])) + def _deduplicate_directives(self, vh_path, directives): + for directive in directives: + while len(self.parser.find_dir(directive, None, vh_path, False)) > 1: + directive_path = self.parser.find_dir(directive, None, vh_path, False) + self.aug.remove(re.sub(r"/\w*$", "", directive_path[0])) + + def _remove_directives(self, vh_path, directives): + for directive in directives: + while len(self.parser.find_dir(directive, None, vh_path, False)) > 0: + directive_path = self.parser.find_dir(directive, None, vh_path, False) + self.aug.remove(re.sub(r"/\w*$", "", directive_path[0])) def _add_dummy_ssl_directives(self, vh_path): self.parser.add_dir(vh_path, "SSLCertificateFile", From 76320c2d3758f645310fb91b7d7292fb4edc3afb Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Sun, 15 Nov 2015 23:00:42 -0600 Subject: [PATCH 18/23] Test vhost cleaning --- .../tests/configurator_test.py | 90 ++++++++++++------- 1 file changed, 56 insertions(+), 34 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index d792ea59d..e70c797bc 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -122,6 +122,37 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual( self.vh_truth[1], self.config.choose_vhost("none.com")) + @mock.patch("letsencrypt_apache.display_ops.select_vhost") + def test_choose_vhost_cleans_vhost_ssl(self, mock_select): + for directive in ["SSLCertificateFile", "SSLCertificateKeyFile", + "SSLCertificateChainFile", "SSLCACertificatePath"]: + for _ in range(10): + self.config.parser.add_dir(self.vh_truth[1].path, directive, ["bogus"]) + self.config.save() + mock_select.return_value = self.vh_truth[1] + + vhost = self.config.choose_vhost("none.com") + self.config.save() + + with open(vhost.filep) as f: + print f.read() + + loc_cert = self.config.parser.find_dir( + 'SSLCertificateFile', None, self.vh_truth[1].path, False) + loc_key = self.config.parser.find_dir( + 'SSLCertificateKeyFile', None, self.vh_truth[1].path, False) + loc_chain = self.config.parser.find_dir( + 'SSLCertificateChainFile', None, self.vh_truth[1].path, False) + loc_cacert = self.config.parser.find_dir( + 'SSLCACertificatePath', None, self.vh_truth[1].path, False) + + self.assertEqual(len(loc_cert), 1) + self.assertEqual(len(loc_key), 1) + + self.assertEqual(len(loc_chain), 0) + + self.assertEqual(len(loc_cacert), 10) + @mock.patch("letsencrypt_apache.display_ops.select_vhost") def test_choose_vhost_select_vhost_non_ssl(self, mock_select): mock_select.return_value = self.vh_truth[0] @@ -393,46 +424,37 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual(len(self.config.vhosts), 5) - def test_remove_existing_ssl_directives(self): + def test_deduplicate_directives(self): # pylint: disable=protected-access - BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", - "SSLCertificateChainFile", "SSLCertificateFile"] - for directive in BOGUS_DIRECTIVES: - self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) + DIRECTIVE = "Foo" + for _ in range(10): + self.config.parser.add_dir(self.vh_truth[1].path, DIRECTIVE, ["bar"]) self.config.save() - self.config._remove_existing_ssl_directives(self.vh_truth[0].path) + self.config._deduplicate_directives(self.vh_truth[1].path, [DIRECTIVE]) self.config.save() - for directive in BOGUS_DIRECTIVES: + self.assertEqual( + len(self.config.parser.find_dir( + DIRECTIVE, None, self.vh_truth[1].path, False)), + 1) + + def test_remove_directives(self): + # pylint: disable=protected-access + DIRECTIVES = ["Foo", "Bar"] + for directive in DIRECTIVES: + for _ in range(10): + self.config.parser.add_dir(self.vh_truth[1].path, directive, ["baz"]) + self.config.save() + + self.config._remove_directives(self.vh_truth[1].path, DIRECTIVES) + self.config.save() + + for directive in DIRECTIVES: self.assertEqual( - self.config.parser.find_dir(directive, None, self.vh_truth[0].path), - []) - - def test_remove_existing_ssl_directives_minus_some(self): - # pylint: disable=protected-access - BOGUS_DIRECTIVES = ["SSLCertificateKeyFile", - "SSLCertificateChainFile", "SSLCertificateFile"] - MINUS_DIRECTIVES = ["SSLCertificateKeyFile", "SSLCertificateFile"] - for directive in BOGUS_DIRECTIVES: - self.config.parser.add_dir(self.vh_truth[0].path, directive, ["bogus"]) - self.config.save() - - self.config._remove_existing_ssl_directives(self.vh_truth[0].path, - minus=MINUS_DIRECTIVES) - self.config.save() - - for directive in BOGUS_DIRECTIVES: - if directive not in MINUS_DIRECTIVES: - self.assertEqual( - self.config.parser.find_dir(directive, None, self.vh_truth[0].path), - [], - msg="directive %s should have been removed" % directive) - else: - self.assertNotEqual( - self.config.parser.find_dir(directive, None, self.vh_truth[0].path), - [], - msg="directive %s should still exist" % directive) + len(self.config.parser.find_dir( + directive, None, self.vh_truth[1].path, False)), + 0) def test_make_vhost_ssl_extra_vhs(self): self.config.aug.match = mock.Mock(return_value=["p1", "p2"]) From e5e7cef6d688e1699082dd8aed84853bc94655b3 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Wed, 18 Nov 2015 19:22:14 -0600 Subject: [PATCH 19/23] Fix conditional for fullchain_path edge cases --- letsencrypt-apache/letsencrypt_apache/configurator.py | 2 +- .../letsencrypt_apache/tests/configurator_test.py | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index e610010a0..8ec7cda8d 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -213,7 +213,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.debug("Apache version is %s", ".".join(str(i) for i in self.version)) - if self.version < (2, 4, 8): + if self.version < (2, 4, 8) or (chain_path and not fullchain_path): # install SSLCertificateFile, SSLCertificateKeyFile, # and SSLCertificateChainFile directives set_cert_path = cert_path diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index e70c797bc..96ac2c023 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -304,8 +304,7 @@ class TwoVhost80Test(util.ApacheTest): self.config.assoc["random.demo"] = self.vh_truth[1] self.assertRaises(errors.PluginError, lambda: self.config.deploy_cert( - "random.demo", "example/cert.pem", "example/key.pem", - "example/cert_chain.pem")) + "random.demo", "example/cert.pem", "example/key.pem")) def test_deploy_cert(self): self.config.parser.modules.add("ssl_module") From b19c9d858cbbb29a4a3a81fcc5e8366d88987d15 Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Wed, 18 Nov 2015 21:12:53 -0600 Subject: [PATCH 20/23] Fix a few nits, coverage --- .../letsencrypt_apache/configurator.py | 2 ++ .../letsencrypt_apache/tests/configurator_test.py | 13 +++++++++++++ 2 files changed, 15 insertions(+) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 8ec7cda8d..d80d27d1c 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -225,6 +225,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): "SSLCertificateChainFile", chain_path) else: self.aug.set(path["chain_path"][-1], chain_path) + else: + raise errors.PluginError("--chain-path is required for your version of Apache") else: if not fullchain_path: raise errors.PluginError("Please provide the --fullchain-path\ diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 96ac2c023..9f30153c3 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -306,6 +306,19 @@ class TwoVhost80Test(util.ApacheTest): lambda: self.config.deploy_cert( "random.demo", "example/cert.pem", "example/key.pem")) + def test_deploy_cert_old_apache_no_chain(self): + self.config = util.get_apache_configurator( + self.config_path, self.config_dir, self.work_dir, version=(2, 4, 7)) + + self.config.parser.modules.add("ssl_module") + self.config.parser.modules.add("mod_ssl.c") + + # Get the default 443 vhost + self.config.assoc["random.demo"] = self.vh_truth[1] + self.assertRaises(errors.PluginError, + lambda: self.config.deploy_cert( + "random.demo", "example/cert.pem", "example/key.pem")) + def test_deploy_cert(self): self.config.parser.modules.add("ssl_module") self.config.parser.modules.add("mod_ssl.c") From ca6a77bb1dfd8114123cab1837f86ff4693ee48a Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Wed, 18 Nov 2015 21:30:46 -0600 Subject: [PATCH 21/23] Fix tests Remove debugging print from tests Fix lint warnings --- .../letsencrypt_apache/tests/configurator_test.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 9f30153c3..58aac1216 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -131,12 +131,9 @@ class TwoVhost80Test(util.ApacheTest): self.config.save() mock_select.return_value = self.vh_truth[1] - vhost = self.config.choose_vhost("none.com") + self.config.choose_vhost("none.com") self.config.save() - with open(vhost.filep) as f: - print f.read() - loc_cert = self.config.parser.find_dir( 'SSLCertificateFile', None, self.vh_truth[1].path, False) loc_key = self.config.parser.find_dir( From d737546dd709fe5a1f3b8b99ca973b4d3e08a2dc Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Fri, 20 Nov 2015 16:31:01 -0600 Subject: [PATCH 22/23] Split off cleaning into a method (fixes a subtle bug) --- .../letsencrypt_apache/configurator.py | 18 +++--- .../tests/configurator_test.py | 55 +++++++++---------- 2 files changed, 36 insertions(+), 37 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index d80d27d1c..ff95eef95 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -183,6 +183,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ vhost = self.choose_vhost(domain) + self._clean_vhost(vhost) # This is done first so that ssl module is enabled and cert_path, # cert_key... can all be parsed appropriately @@ -276,15 +277,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.assoc[target_name] = vhost return vhost - vhost = self._choose_vhost_from_list(target_name) - if vhost.ssl: - # remove duplicated or conflicting ssl directives - self._deduplicate_directives(vhost.path, - ["SSLCertificateFile", "SSLCertificateKeyFile"]) - # remove all problematic directives - self._remove_directives(vhost.path, ["SSLCertificateChainFile"]) - - return vhost + return self._choose_vhost_from_list(target_name) def _choose_vhost_from_list(self, target_name): # Select a vhost from a list @@ -665,6 +658,13 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): return ssl_addrs + def _clean_vhost(self, vhost): + # remove duplicated or conflicting ssl directives + self._deduplicate_directives(vhost.path, + ["SSLCertificateFile", "SSLCertificateKeyFile"]) + # remove all problematic directives + self._remove_directives(vhost.path, ["SSLCertificateChainFile"]) + def _deduplicate_directives(self, vh_path, directives): for directive in directives: while len(self.parser.find_dir(directive, None, vh_path, False)) > 1: diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 58aac1216..d5ea540c5 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -122,34 +122,6 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual( self.vh_truth[1], self.config.choose_vhost("none.com")) - @mock.patch("letsencrypt_apache.display_ops.select_vhost") - def test_choose_vhost_cleans_vhost_ssl(self, mock_select): - for directive in ["SSLCertificateFile", "SSLCertificateKeyFile", - "SSLCertificateChainFile", "SSLCACertificatePath"]: - for _ in range(10): - self.config.parser.add_dir(self.vh_truth[1].path, directive, ["bogus"]) - self.config.save() - mock_select.return_value = self.vh_truth[1] - - self.config.choose_vhost("none.com") - self.config.save() - - loc_cert = self.config.parser.find_dir( - 'SSLCertificateFile', None, self.vh_truth[1].path, False) - loc_key = self.config.parser.find_dir( - 'SSLCertificateKeyFile', None, self.vh_truth[1].path, False) - loc_chain = self.config.parser.find_dir( - 'SSLCertificateChainFile', None, self.vh_truth[1].path, False) - loc_cacert = self.config.parser.find_dir( - 'SSLCACertificatePath', None, self.vh_truth[1].path, False) - - self.assertEqual(len(loc_cert), 1) - self.assertEqual(len(loc_key), 1) - - self.assertEqual(len(loc_chain), 0) - - self.assertEqual(len(loc_cacert), 10) - @mock.patch("letsencrypt_apache.display_ops.select_vhost") def test_choose_vhost_select_vhost_non_ssl(self, mock_select): mock_select.return_value = self.vh_truth[0] @@ -433,6 +405,33 @@ class TwoVhost80Test(util.ApacheTest): self.assertEqual(len(self.config.vhosts), 5) + def test_clean_vhost_ssl(self): + # pylint: disable=protected-access + for directive in ["SSLCertificateFile", "SSLCertificateKeyFile", + "SSLCertificateChainFile", "SSLCACertificatePath"]: + for _ in range(10): + self.config.parser.add_dir(self.vh_truth[1].path, directive, ["bogus"]) + self.config.save() + + self.config._clean_vhost(self.vh_truth[1]) + self.config.save() + + loc_cert = self.config.parser.find_dir( + 'SSLCertificateFile', None, self.vh_truth[1].path, False) + loc_key = self.config.parser.find_dir( + 'SSLCertificateKeyFile', None, self.vh_truth[1].path, False) + loc_chain = self.config.parser.find_dir( + 'SSLCertificateChainFile', None, self.vh_truth[1].path, False) + loc_cacert = self.config.parser.find_dir( + 'SSLCACertificatePath', None, self.vh_truth[1].path, False) + + self.assertEqual(len(loc_cert), 1) + self.assertEqual(len(loc_key), 1) + + self.assertEqual(len(loc_chain), 0) + + self.assertEqual(len(loc_cacert), 10) + def test_deduplicate_directives(self): # pylint: disable=protected-access DIRECTIVE = "Foo" From f2ccc228a3eaf5731d62169683539b005a3cc21c Mon Sep 17 00:00:00 2001 From: Liam Marshall Date: Mon, 23 Nov 2015 13:17:24 -0600 Subject: [PATCH 23/23] Remove code path that will never get hit --- letsencrypt-apache/letsencrypt_apache/configurator.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index ff95eef95..98cd5b407 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -221,11 +221,8 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): self.aug.set(path["cert_path"][-1], cert_path) self.aug.set(path["cert_key"][-1], key_path) if chain_path is not None: - if not path["chain_path"]: - self.parser.add_dir(vhost.path, - "SSLCertificateChainFile", chain_path) - else: - self.aug.set(path["chain_path"][-1], chain_path) + self.parser.add_dir(vhost.path, + "SSLCertificateChainFile", chain_path) else: raise errors.PluginError("--chain-path is required for your version of Apache") else: