From dde4951be55cd72acb05ae1af3376fff731b4e1d Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 10:40:42 -0700 Subject: [PATCH 1/6] fix 691 --- letsencrypt-apache/letsencrypt_apache/configurator.py | 8 ++++++-- letsencrypt-apache/letsencrypt_apache/dvsni.py | 4 ++-- letsencrypt-apache/letsencrypt_apache/parser.py | 6 ++++-- .../letsencrypt_apache/tests/complex_parsing_test.py | 2 +- letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py | 1 + letsencrypt-apache/letsencrypt_apache/tests/util.py | 3 +++ 6 files changed, 17 insertions(+), 7 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 31b5f0bc5..87687e38d 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -482,7 +482,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.debug(msg) self.save_notes += msg - def prepare_server_https(self, port, temp=False): + def prepare_server_https(self, port): """Prepare the server for HTTPS. Make sure that the ssl_module is loaded and that the server @@ -493,7 +493,10 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ if "ssl_module" not in self.parser.modules: logger.info("Loading mod_ssl into Apache Server") - self.enable_mod("ssl", temp) + if self.config.func.__name__ == "auth": + self.enable_mod("ssl", temp=True) + else: + self.enable_mod("ssl", temp=False) # Check for Listen # Note: This could be made to also look for ip:443 combo @@ -1138,6 +1141,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if not self._chall_out: self.revert_challenge_config() self.restart() + self.parser.init_modules() def apache_restart(apache_init_script): diff --git a/letsencrypt-apache/letsencrypt_apache/dvsni.py b/letsencrypt-apache/letsencrypt_apache/dvsni.py index 11e69db61..b05156c5d 100644 --- a/letsencrypt-apache/letsencrypt_apache/dvsni.py +++ b/letsencrypt-apache/letsencrypt_apache/dvsni.py @@ -53,7 +53,7 @@ class ApacheDvsni(common.Dvsni): "le_dvsni_cert_challenge.conf") def perform(self): - """Peform a DVSNI challenge.""" + """Perform a DVSNI challenge.""" if not self.achalls: return [] # Save any changes to the configuration as a precaution @@ -62,7 +62,7 @@ class ApacheDvsni(common.Dvsni): # Prepare the server for HTTPS self.configurator.prepare_server_https( - str(self.configurator.config.dvsni_port), True) + str(self.configurator.config.dvsni_port)) responses = [] diff --git a/letsencrypt-apache/letsencrypt_apache/parser.py b/letsencrypt-apache/letsencrypt_apache/parser.py index e14569abc..da3fc97e7 100644 --- a/letsencrypt-apache/letsencrypt_apache/parser.py +++ b/letsencrypt-apache/letsencrypt_apache/parser.py @@ -51,7 +51,7 @@ class ApacheParser(object): # https://httpd.apache.org/docs/2.4/mod/core.html#ifmodule # This needs to come before locations are set. self.modules = set() - self._init_modules() + self.init_modules() # Set up rest of locations self.loc.update(self._set_locations()) @@ -60,13 +60,15 @@ class ApacheParser(object): # Sites-available is not included naturally in configuration self._parse_file(os.path.join(self.root, "sites-available") + "/*") - def _init_modules(self): + def init_modules(self): """Iterates on the configuration until no new modules are loaded. ..todo:: This should be attempted to be done with a binary to avoid the iteration issue. Else... parse and enable mods at same time. """ + # Since modules are being initiated... clear existing set. + self.modules = set() matches = self.find_dir("LoadModule") iterator = iter(matches) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py b/letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py index d6112a486..406b6c39e 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/complex_parsing_test.py @@ -18,7 +18,7 @@ class ComplexParserTest(util.ParserTest): self.setup_variables() # This needs to happen after due to setup_variables not being run # until after - self.parser._init_modules() # pylint: disable=protected-access + self.parser.init_modules() # pylint: disable=protected-access def tearDown(self): shutil.rmtree(self.temp_dir) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py index c362d4115..884ec9e62 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py @@ -22,6 +22,7 @@ class DvsniPerformTest(util.ApacheTest): config = util.get_apache_configurator( self.config_path, self.config_dir, self.work_dir) config.config.dvsni_port = 443 + config.config.func.__name__ = "auth" from letsencrypt_apache import dvsni self.sni = dvsni.ApacheDvsni(config) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index b544e06ee..0782bef25 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -92,6 +92,9 @@ def get_apache_configurator( config.prepare() + # Simulate a 'run' by default + config.config.func.__name__ = "run" + return config From 5ba6446458619e636c53a5e4e28f2847c9bf1fd4 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 11:07:54 -0700 Subject: [PATCH 2/6] Add additional test for prepare_server_https --- .../letsencrypt_apache/tests/configurator_test.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 6d16474b3..4a5cd4500 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -302,7 +302,9 @@ class TwoVhost80Test(util.ApacheTest): "NameVirtualHost", "*:80")) def test_prepare_server_https(self): - self.config.parser.modules.add("ssl_module") + mock_enable = mock.Mock() + self.config.enable_mod = mock_enable + mock_find = mock.Mock() mock_add_dir = mock.Mock() mock_find.return_value = [] @@ -312,7 +314,14 @@ class TwoVhost80Test(util.ApacheTest): self.config.parser.add_dir_to_ifmodssl = mock_add_dir self.config.prepare_server_https("443") + self.assertEqual(mock_enable.call_args[1], {"temp": False}) + + # Modifying base func call... to auth + self.config.config.func.__name__ = "auth" self.config.prepare_server_https("8080") + # Enable mod is temporary + self.assertEqual(mock_enable.call_args[1], {"temp": True}) + self.assertEqual(mock_add_dir.call_count, 2) def test_make_vhost_ssl(self): From d8663fbbc0e90bbdd54ae5a5bbe843c4ec16687e Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 11:30:39 -0700 Subject: [PATCH 3/6] Fix #690 --- letsencrypt/reverter.py | 6 +++++- letsencrypt/tests/reverter_test.py | 7 +++++++ 2 files changed, 12 insertions(+), 1 deletion(-) diff --git a/letsencrypt/reverter.py b/letsencrypt/reverter.py index 363de65f4..f2281ff5e 100644 --- a/letsencrypt/reverter.py +++ b/letsencrypt/reverter.py @@ -75,7 +75,11 @@ class Reverter(object): backups = os.listdir(self.config.backup_dir) backups.sort() - if len(backups) < rollback: + if len(backups) == 0: + logger.warning( + "Let's Encrypt hasn't modified your configuration, so rollback " + "isn't available.") + elif len(backups) < rollback: logger.warning("Unable to rollback %d checkpoints, only %d exist", rollback, len(backups)) diff --git a/letsencrypt/tests/reverter_test.py b/letsencrypt/tests/reverter_test.py index 59a7e4d9a..62c47f8d6 100644 --- a/letsencrypt/tests/reverter_test.py +++ b/letsencrypt/tests/reverter_test.py @@ -343,9 +343,16 @@ class TestFullCheckpointsReverter(unittest.TestCase): @mock.patch("letsencrypt.reverter.logger") def test_rollback_too_many(self, mock_logger): + # Test no exist warning... self.reverter.rollback_checkpoints(1) self.assertEqual(mock_logger.warning.call_count, 1) + # Test Generic warning + mock_logger.warning.call_count = 0 + self._setup_three_checkpoints() + self.reverter.rollback_checkpoints(4) + self.assertEqual(mock_logger.warning.call_count, 1) + def test_multi_rollback(self): config3 = self._setup_three_checkpoints() self.reverter.rollback_checkpoints(3) From ea7a427cca47e5e57a8cb1ba4383b9d92fa3eb03 Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 13:11:48 -0700 Subject: [PATCH 4/6] revert to simple enable_mod, disable_mod, enable_mod --- .../letsencrypt_apache/configurator.py | 16 +++++----------- letsencrypt-apache/letsencrypt_apache/dvsni.py | 2 +- .../tests/configurator_test.py | 15 ++------------- 3 files changed, 8 insertions(+), 25 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 87687e38d..01c9d4f30 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -482,7 +482,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): logger.debug(msg) self.save_notes += msg - def prepare_server_https(self, port): + def prepare_server_https(self, port, temp=False): """Prepare the server for HTTPS. Make sure that the ssl_module is loaded and that the server @@ -493,10 +493,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ if "ssl_module" not in self.parser.modules: logger.info("Loading mod_ssl into Apache Server") - if self.config.func.__name__ == "auth": - self.enable_mod("ssl", temp=True) - else: - self.enable_mod("ssl", temp=False) + self.enable_mod("ssl", temp=temp) # Check for Listen # Note: This could be made to also look for ip:443 combo @@ -955,6 +952,9 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): def enable_site(self, vhost): """Enables an available site, Apache restart required. + .. note:: Does not make sure that the site correctly works or that all + modules are enabled appropriately. + .. todo:: This function should number subdomains before the domain vhost .. todo:: Make sure link is not broken... @@ -968,12 +968,6 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): if self.is_site_enabled(vhost.filep): return - if vhost.ssl: - # TODO: Make this based on addresses - self.prepare_server_https("443") - if self.save_notes: - self.save() - if "/sites-available/" in vhost.filep: enabled_path = ("%s/sites-enabled/%s" % (self.parser.root, os.path.basename(vhost.filep))) diff --git a/letsencrypt-apache/letsencrypt_apache/dvsni.py b/letsencrypt-apache/letsencrypt_apache/dvsni.py index b05156c5d..c6c41dc51 100644 --- a/letsencrypt-apache/letsencrypt_apache/dvsni.py +++ b/letsencrypt-apache/letsencrypt_apache/dvsni.py @@ -62,7 +62,7 @@ class ApacheDvsni(common.Dvsni): # Prepare the server for HTTPS self.configurator.prepare_server_https( - str(self.configurator.config.dvsni_port)) + str(self.configurator.config.dvsni_port), True) responses = [] diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 4a5cd4500..71599bd1d 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -207,20 +207,11 @@ class TwoVhost80Test(util.ApacheTest): self.assertRaises( errors.MisconfigurationError, self.config.enable_mod, "ssl") - @mock.patch("letsencrypt.le_util.run_script") - @mock.patch("letsencrypt.le_util.exe_exists") - @mock.patch("letsencrypt_apache.parser.subprocess.Popen") - def test_enable_site(self, mock_popen, mock_exe_exists, mock_run_script): - mock_popen().returncode = 0 - mock_popen().communicate.return_value = ("Define: DUMP_RUN_CFG", "") - mock_exe_exists.return_value = True - + def test_enable_site(self): # Default 443 vhost self.assertFalse(self.vh_truth[1].enabled) self.config.enable_site(self.vh_truth[1]) self.assertTrue(self.vh_truth[1].enabled) - # Mod enabled - self.assertTrue(mock_run_script.called) # Go again to make sure nothing fails self.config.enable_site(self.vh_truth[1]) @@ -316,9 +307,7 @@ class TwoVhost80Test(util.ApacheTest): self.config.prepare_server_https("443") self.assertEqual(mock_enable.call_args[1], {"temp": False}) - # Modifying base func call... to auth - self.config.config.func.__name__ = "auth" - self.config.prepare_server_https("8080") + self.config.prepare_server_https("8080", temp=True) # Enable mod is temporary self.assertEqual(mock_enable.call_args[1], {"temp": True}) From 8bb02b80c1d91718043221d4a04cd8b117a3344b Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 13:12:15 -0700 Subject: [PATCH 5/6] fix typo --- letsencrypt/plugins/disco.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/letsencrypt/plugins/disco.py b/letsencrypt/plugins/disco.py index 51d251126..b6cdb1f99 100644 --- a/letsencrypt/plugins/disco.py +++ b/letsencrypt/plugins/disco.py @@ -215,7 +215,7 @@ class PluginsRegistry(collections.Mapping): Returns ``None`` if ``plugin`` is not found in the registry. """ - # use list instead of set beacse PluginEntryPoint is not hashable + # use list instead of set because PluginEntryPoint is not hashable candidates = [plugin_ep for plugin_ep in self._plugins.itervalues() if plugin_ep.initialized and plugin_ep.init() is plugin] assert len(candidates) <= 1 From 6aabb3839fed28bad9f1bc49417e4578957c163d Mon Sep 17 00:00:00 2001 From: James Kasten Date: Mon, 17 Aug 2015 13:27:15 -0700 Subject: [PATCH 6/6] address comments --- letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py | 1 - letsencrypt-apache/letsencrypt_apache/tests/util.py | 3 --- letsencrypt/reverter.py | 2 +- 3 files changed, 1 insertion(+), 5 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py index 884ec9e62..c362d4115 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/dvsni_test.py @@ -22,7 +22,6 @@ class DvsniPerformTest(util.ApacheTest): config = util.get_apache_configurator( self.config_path, self.config_dir, self.work_dir) config.config.dvsni_port = 443 - config.config.func.__name__ = "auth" from letsencrypt_apache import dvsni self.sni = dvsni.ApacheDvsni(config) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index 0782bef25..b544e06ee 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -92,9 +92,6 @@ def get_apache_configurator( config.prepare() - # Simulate a 'run' by default - config.config.func.__name__ = "run" - return config diff --git a/letsencrypt/reverter.py b/letsencrypt/reverter.py index f2281ff5e..8eed59156 100644 --- a/letsencrypt/reverter.py +++ b/letsencrypt/reverter.py @@ -75,7 +75,7 @@ class Reverter(object): backups = os.listdir(self.config.backup_dir) backups.sort() - if len(backups) == 0: + if not backups: logger.warning( "Let's Encrypt hasn't modified your configuration, so rollback " "isn't available.")