diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 31b5f0bc5..01c9d4f30 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -493,7 +493,7 @@ 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) + self.enable_mod("ssl", temp=temp) # Check for Listen # Note: This could be made to also look for ip:443 combo @@ -952,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... @@ -965,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))) @@ -1138,6 +1135,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..c6c41dc51 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 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/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 6d16474b3..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]) @@ -302,7 +293,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 +305,12 @@ class TwoVhost80Test(util.ApacheTest): self.config.parser.add_dir_to_ifmodssl = mock_add_dir self.config.prepare_server_https("443") - self.config.prepare_server_https("8080") + self.assertEqual(mock_enable.call_args[1], {"temp": False}) + + self.config.prepare_server_https("8080", temp=True) + # 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): 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 diff --git a/letsencrypt/reverter.py b/letsencrypt/reverter.py index 363de65f4..8eed59156 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 not backups: + 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)