From 053697889d2b9bfd8a2c9c943ae601c4f1865c3d Mon Sep 17 00:00:00 2001 From: Daniel Aleksandersen Date: Tue, 17 Nov 2015 05:14:04 +0100 Subject: [PATCH 1/7] Add missing RPM requirement `redhat-rpm-config` provides the required `/usr/lib/rpm/redhat/redhat-hardened-cc1` to Fedora. --- bootstrap/_rpm_common.sh | 1 + 1 file changed, 1 insertion(+) diff --git a/bootstrap/_rpm_common.sh b/bootstrap/_rpm_common.sh index 5aca13cd4..042332f4b 100755 --- a/bootstrap/_rpm_common.sh +++ b/bootstrap/_rpm_common.sh @@ -40,6 +40,7 @@ if ! $tool install -y \ augeas-libs \ openssl-devel \ libffi-devel \ + redhat-rpm-config \ ca-certificates then echo "Could not install additional dependencies. Aborting bootstrap!" From 224eb1cd1a94835a5dbd3c59154050c841d207a8 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 30 Nov 2015 16:50:26 -0800 Subject: [PATCH 2/7] Stop using init-script --- .../letsencrypt_apache/configurator.py | 61 +++++-------------- 1 file changed, 16 insertions(+), 45 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 5777d204d..1849c0095 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -1186,16 +1186,25 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): le_util.run_script([self.conf("enmod"), mod_name]) def restart(self): - """Reloads apache server. + """Runs a config test and reloads the Apache server. - .. todo:: This function will be converted to using reload - - :raises .errors.MisconfigurationError: If unable to reload due - to a configuration problem, or if the reload subprocess - cannot be run. + :raises .errors.MisconfigurationError: If either the config test + or reload fails. """ - return apache_reload(self.conf("init-script")) + self.config_test() + self._reload() + + def _reload(self): + """Reloads the Apache server. + + :raises .errors.MisconfigurationError: If reload fails + + """ + try: + le_util.run_script([self.conf("ctl"), "-k", "graceful"]) + except errors.SubprocessError as err: + raise errors.MisconfigurationError(str(err)) def config_test(self): # pylint: disable=no-self-use """Check the configuration of Apache for errors. @@ -1317,44 +1326,6 @@ def _get_mod_deps(mod_name): return deps.get(mod_name, []) -def apache_reload(apache_init_script): - """Reloads the Apache Server. - - :param str apache_init_script: Path to the Apache init script. - - .. todo:: Try to use reload instead. (This caused timing problems before) - - .. todo:: On failure, this should be a recovery_routine call with another - reload. This will confuse and inhibit developers from testing code - though. This change should happen after - the ApacheConfigurator has been thoroughly tested. The function will - need to be moved into the class again. Perhaps - this version can live on... for testing purposes. - - :raises .errors.MisconfigurationError: If unable to reload due to a - configuration problem, or if the reload subprocess cannot be run. - - """ - try: - proc = subprocess.Popen([apache_init_script, "reload"], - stdout=subprocess.PIPE, - stderr=subprocess.PIPE) - - except (OSError, ValueError): - logger.fatal( - "Unable to reload the Apache process with %s", apache_init_script) - raise errors.MisconfigurationError( - "Unable to reload Apache process with %s" % apache_init_script) - - stdout, stderr = proc.communicate() - - if proc.returncode != 0: - # Enter recovery routine... - logger.error("Apache Reload Failed!\n%s\n%s", stdout, stderr) - raise errors.MisconfigurationError( - "Error while reloading Apache:\n%s\n%s" % (stdout, stderr)) - - def get_file_path(vhost_path): """Get file path from augeas_vhost_path. From c72e122943a8276f4c73e1e256d244d99eeef30e Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 30 Nov 2015 17:56:42 -0800 Subject: [PATCH 3/7] add add_deprecated_argument --- letsencrypt/le_util.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/letsencrypt/le_util.py b/letsencrypt/le_util.py index 25260d755..3d124e2f8 100644 --- a/letsencrypt/le_util.py +++ b/letsencrypt/le_util.py @@ -1,4 +1,5 @@ """Utilities for all Let's Encrypt.""" +import argparse import collections import errno import logging @@ -255,3 +256,23 @@ def safe_email(email): else: logger.warn("Invalid email address: %s.", email) return False + + +def add_deprecated_argument(add_argument, argument_name): + """Adds a deprecated argument with the name argument_name. + + Deprecated arguments are not shown in the help. If they are used on + the command line, a warning is shown stating that the argument is + deprecated and no other action is taken. + + :param callable add_argument: Function that adds arguments to an + argument parser/group. + :param str argument_name: Name of deprecated argument. + + """ + class ShowWarning(argparse.Action): + """Action to log a warning when an argument is used.""" + def __call__(self, unused1, unused2, unused3, option_string=None): + print "Use of {0} is deprecated".format(option_string) + + add_argument(argument_name, action=ShowWarning, help=argparse.SUPPRESS) From 4a2e40c365ec4fa5bb6d19aa0d997ede6f0a0820 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 30 Nov 2015 18:55:28 -0800 Subject: [PATCH 4/7] Added add_deprecated_argument tests --- letsencrypt/le_util.py | 12 ++++++---- letsencrypt/tests/le_util_test.py | 39 +++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 4 deletions(-) diff --git a/letsencrypt/le_util.py b/letsencrypt/le_util.py index 3d124e2f8..d127c9d64 100644 --- a/letsencrypt/le_util.py +++ b/letsencrypt/le_util.py @@ -6,8 +6,9 @@ import logging import os import platform import re -import subprocess import stat +import subprocess +import sys from letsencrypt import errors @@ -258,7 +259,7 @@ def safe_email(email): return False -def add_deprecated_argument(add_argument, argument_name): +def add_deprecated_argument(add_argument, argument_name, nargs): """Adds a deprecated argument with the name argument_name. Deprecated arguments are not shown in the help. If they are used on @@ -268,11 +269,14 @@ def add_deprecated_argument(add_argument, argument_name): :param callable add_argument: Function that adds arguments to an argument parser/group. :param str argument_name: Name of deprecated argument. + :param nargs: Value for nargs when adding the argument to argparse. """ class ShowWarning(argparse.Action): """Action to log a warning when an argument is used.""" def __call__(self, unused1, unused2, unused3, option_string=None): - print "Use of {0} is deprecated".format(option_string) + sys.stderr.write( + "Use of {0} is deprecated\n".format(option_string)) - add_argument(argument_name, action=ShowWarning, help=argparse.SUPPRESS) + add_argument(argument_name, action=ShowWarning, + help=argparse.SUPPRESS, nargs=nargs) diff --git a/letsencrypt/tests/le_util_test.py b/letsencrypt/tests/le_util_test.py index ed976f72d..87894f837 100644 --- a/letsencrypt/tests/le_util_test.py +++ b/letsencrypt/tests/le_util_test.py @@ -1,8 +1,10 @@ """Tests for letsencrypt.le_util.""" +import argparse import errno import os import shutil import stat +import StringIO import tempfile import unittest @@ -284,5 +286,42 @@ class SafeEmailTest(unittest.TestCase): self.assertFalse(self._call(addr), "%s failed." % addr) +class AddDeprecatedArgumentTest(unittest.TestCase): + """Test add_deprecated_argument.""" + def setUp(self): + self.parser = argparse.ArgumentParser() + + def _call(self, argument_name, nargs): + from letsencrypt.le_util import add_deprecated_argument + + add_deprecated_argument(self.parser.add_argument, argument_name, nargs) + + def test_warning_no_arg(self): + self._call("--old-option", 0) + stderr = self._get_argparse_warnings(["--old-option"]) + self.assertTrue("--old-option is deprecated" in stderr) + + def test_warning_with_arg(self): + self._call("--old-option", 1) + stderr = self._get_argparse_warnings(["--old-option", "42"]) + self.assertTrue("--old-option is deprecated" in stderr) + + def _get_argparse_warnings(self, args): + stderr = StringIO.StringIO() + with mock.patch("letsencrypt.le_util.sys.stderr", new=stderr): + self.parser.parse_args(args) + return stderr.getvalue() + + def test_help(self): + self._call("--old-option", 2) + stdout = StringIO.StringIO() + with mock.patch("letsencrypt.le_util.sys.stdout", new=stdout): + try: + self.parser.parse_args(["-h"]) + except SystemExit: + pass + self.assertTrue("--old-option" not in stdout.getvalue()) + + if __name__ == "__main__": unittest.main() # pragma: no cover From 0d6728f9cf142367885a95e3f19e65cf0b19a7e0 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 30 Nov 2015 18:57:48 -0800 Subject: [PATCH 5/7] Punctuation and deprecation --- letsencrypt-apache/letsencrypt_apache/configurator.py | 4 +--- letsencrypt/le_util.py | 2 +- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 1849c0095..fa12ccf03 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -95,13 +95,11 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): help="Path to the Apache 'a2enmod' binary.") add("dismod", default=constants.CLI_DEFAULTS["dismod"], help="Path to the Apache 'a2enmod' binary.") - add("init-script", default=constants.CLI_DEFAULTS["init_script"], - help="Path to the Apache init script (used for server " - "reload).") add("le-vhost-ext", default=constants.CLI_DEFAULTS["le_vhost_ext"], help="SSL vhost configuration extension.") add("server-root", default=constants.CLI_DEFAULTS["server_root"], help="Apache server root directory.") + le_util.add_deprecated_argument(add, "init-script", 1) def __init__(self, *args, **kwargs): """Initialize an Apache Configurator. diff --git a/letsencrypt/le_util.py b/letsencrypt/le_util.py index d127c9d64..7869fc9a5 100644 --- a/letsencrypt/le_util.py +++ b/letsencrypt/le_util.py @@ -276,7 +276,7 @@ def add_deprecated_argument(add_argument, argument_name, nargs): """Action to log a warning when an argument is used.""" def __call__(self, unused1, unused2, unused3, option_string=None): sys.stderr.write( - "Use of {0} is deprecated\n".format(option_string)) + "Use of {0} is deprecated.\n".format(option_string)) add_argument(argument_name, action=ShowWarning, help=argparse.SUPPRESS, nargs=nargs) From e4cf64c30ecd6c1a7cadc052ffc22d58f7bd38c5 Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Mon, 30 Nov 2015 19:13:50 -0800 Subject: [PATCH 6/7] Fix Apache tests --- .../letsencrypt_apache/configurator.py | 1 - .../letsencrypt_apache/constants.py | 1 - .../tests/configurator_test.py | 21 +++++-------------- .../letsencrypt_apache/tests/util.py | 6 +----- 4 files changed, 6 insertions(+), 23 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index fa12ccf03..319082934 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -7,7 +7,6 @@ import os import re import shutil import socket -import subprocess import time import zope.interface diff --git a/letsencrypt-apache/letsencrypt_apache/constants.py b/letsencrypt-apache/letsencrypt_apache/constants.py index 813eae582..202fc3e21 100644 --- a/letsencrypt-apache/letsencrypt_apache/constants.py +++ b/letsencrypt-apache/letsencrypt_apache/constants.py @@ -7,7 +7,6 @@ CLI_DEFAULTS = dict( ctl="apache2ctl", enmod="a2enmod", dismod="a2dismod", - init_script="/etc/init.d/apache2", le_vhost_ext="-le-ssl.conf", ) """CLI defaults.""" diff --git a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py index 0b6170e1d..d232a1dc4 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/configurator_test.py @@ -563,24 +563,13 @@ class TwoVhost80Test(util.ApacheTest): mock_script.side_effect = errors.SubprocessError("Can't find program") self.assertRaises(errors.PluginError, self.config.get_version) - @mock.patch("letsencrypt_apache.configurator.subprocess.Popen") - def test_restart(self, mock_popen): - """These will be changed soon enough with reload.""" - mock_popen().returncode = 0 - mock_popen().communicate.return_value = ("", "") - + @mock.patch("letsencrypt_apache.configurator.le_util.run_script") + def test_restart(self, _): self.config.restart() - @mock.patch("letsencrypt_apache.configurator.subprocess.Popen") - def test_restart_bad_process(self, mock_popen): - mock_popen.side_effect = OSError - - self.assertRaises(errors.MisconfigurationError, self.config.restart) - - @mock.patch("letsencrypt_apache.configurator.subprocess.Popen") - def test_restart_failure(self, mock_popen): - mock_popen().communicate.return_value = ("", "") - mock_popen().returncode = 1 + @mock.patch("letsencrypt_apache.configurator.le_util.run_script") + def test_restart_bad_process(self, mock_run_script): + mock_run_script.side_effect = [None, errors.SubprocessError] self.assertRaises(errors.MisconfigurationError, self.config.restart) diff --git a/letsencrypt-apache/letsencrypt_apache/tests/util.py b/letsencrypt-apache/letsencrypt_apache/tests/util.py index 1bc1fbe17..0c60373f2 100644 --- a/letsencrypt-apache/letsencrypt_apache/tests/util.py +++ b/letsencrypt-apache/letsencrypt_apache/tests/util.py @@ -75,11 +75,7 @@ def get_apache_configurator( in_progress_dir=os.path.join(backups, "IN_PROGRESS"), work_dir=work_dir) - with mock.patch("letsencrypt_apache.configurator." - "subprocess.Popen") as mock_popen: - # This indicates config_test passes - mock_popen().communicate.return_value = ("Fine output", "No problems") - mock_popen().returncode = 0 + with mock.patch("letsencrypt_apache.configurator.le_util.run_script"): with mock.patch("letsencrypt_apache.configurator.le_util." "exe_exists") as mock_exe_exists: mock_exe_exists.return_value = True From d4d51fe4354701ac0726f37ad8560a96ba8af5cd Mon Sep 17 00:00:00 2001 From: Brad Warren Date: Tue, 1 Dec 2015 13:17:19 -0800 Subject: [PATCH 7/7] Remove stray reference to init-script --- letsencrypt-apache/letsencrypt_apache/configurator.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/letsencrypt-apache/letsencrypt_apache/configurator.py b/letsencrypt-apache/letsencrypt_apache/configurator.py index 319082934..96e310565 100644 --- a/letsencrypt-apache/letsencrypt_apache/configurator.py +++ b/letsencrypt-apache/letsencrypt_apache/configurator.py @@ -137,8 +137,7 @@ class ApacheConfigurator(augeas_configurator.AugeasConfigurator): """ # Verify Apache is installed - for exe in (self.conf("ctl"), self.conf("enmod"), - self.conf("dismod"), self.conf("init-script")): + for exe in (self.conf("ctl"), self.conf("enmod"), self.conf("dismod")): if not le_util.exe_exists(exe): raise errors.NoInstallationError