diff --git a/certbot-postfix/certbot_postfix/installer.py b/certbot-postfix/certbot_postfix/installer.py index be80b88af..11d948cb6 100644 --- a/certbot-postfix/certbot_postfix/installer.py +++ b/certbot-postfix/certbot_postfix/installer.py @@ -42,6 +42,7 @@ class Installer(plugins_common.Installer): def add_parser_arguments(cls, add): add("ctl", default=constants.CLI_DEFAULTS["ctl"], help="Path to the 'postfix' control program.") + # This directory points to Postfix's configuration directory. add("config-dir", default=constants.CLI_DEFAULTS["config_dir"], help="Path to the directory containing the " "Postfix main.cf file to modify instead of using the " @@ -52,6 +53,7 @@ class Installer(plugins_common.Installer): help="Name of the policy file that we should write to in config-dir.") def _verify_setup(self): + # TODO (sydneyli): do this pass def __init__(self, *args, **kwargs): @@ -71,8 +73,12 @@ class Installer(plugins_common.Installer): self.postfix = None self.policy_file = None self._enhance_func = {"starttls-policy": self._enable_policy_list} + # Since we only need to enable policy once for all domains, + # keep track of whether this enhancement was already called. + self._starttls_policy_enabled = False def _ensure_ca_certificates_exist(self): + # TODO (sydneyli): This might block starttls-everywhere # TODO (sydneyli): Ensure `ca-certificates` is installed correctly, or that # /etc/ssl/certs/ even has certificates in it, probably via a sanity check using # `openssl` command? @@ -124,7 +130,8 @@ class Installer(plugins_common.Installer): """ if self._get_version() < constants.MINIMUM_VERSION: - raise errors.NotSupportedError('Postfix version is too old') + version_string = '.'.join([str(n) for n in constants.MINIMUM_VERSION]) + raise errors.NotSupportedError('Postfix version must be at least %s', version_string) def _lock_config_dir(self): """Stop two Postfix plugins from modifying the config at once. @@ -216,11 +223,14 @@ class Installer(plugins_common.Installer): check_override=util.report_master_overrides) def _enable_policy_list(self, domain, options): + if self._starttls_policy_enabled: + return + self._starttls_policy_enabled = True # pylint: disable=unused-argument try: from starttls_policy import policy except ImportError: - raise ImportError('STARTTLS Everywhere policy Python module not installed!') + raise errors.PluginError('STARTTLS Everywhere policy Python module not installed!') if options is None: policy = policy.Config() else: @@ -232,10 +242,6 @@ class Installer(plugins_common.Installer): def enhance(self, domain, enhancement, options=None): """Raises an exception for request for unsupported enhancement. - - :raises .PluginError: this is always raised as no enhancements - are currently supported - """ try: func = self._enhance_func[enhancement] @@ -254,7 +260,7 @@ class Installer(plugins_common.Installer): :rtype: list """ - return ['starttls-policy'] # TODO(sydneyli): Call this starttls-policy? + return ['starttls-policy'] def save(self, title=None, temporary=False): """Creates backups and writes changes to configuration files. diff --git a/certbot-postfix/certbot_postfix/postconf.py b/certbot-postfix/certbot_postfix/postconf.py index d2af43174..6c34adbcb 100644 --- a/certbot-postfix/certbot_postfix/postconf.py +++ b/certbot-postfix/certbot_postfix/postconf.py @@ -5,6 +5,8 @@ from certbot import errors from certbot_postfix import util +# TODO (sydneyli): tox-ify and make sure this runs in Python 3. + class ConfigMain(util.PostfixUtilBase): """A parser for Postfix's main.cf file.""" @@ -36,28 +38,28 @@ class ConfigMain(util.PostfixUtilBase): self._master_db[param_name].append((service, value)) def get_default(self, name): - """Retrieves default value of parameter |name| from postfix parameters. + """Retrieves default value of parameter `name` from postfix parameters. :param str name: The name of the parameter to fetch. - :rtype str: The default value of parameter |name|. + :rtype str: The default value of parameter `name`. """ out = self._get_output(['-d', name]) _, value = next(_parse_main_output(out), (None, None)) return value def get(self, name): - """Retrieves working value of parameter |name| from postfix parameters. + """Retrieves working value of parameter `name` from postfix parameters. :param str name: The name of the parameter to fetch. - :rtype str: The value of parameter |name|. + :rtype str: The value of parameter `name`. """ if name in self._updated: return self._updated[name] return self._db[name] def get_master_overrides(self, name): - """Retrieves list of overrides for parameter |name| in postfix's Master config + """Retrieves list of overrides for parameter `name` in postfix's Master config file. - :returns: List of tuples (service, value), meaning that parameter |name| - is overridden as |value| for |service|. + :returns: List of tuples (service, value), meaning that parameter `name` + is overridden as `value` for `service`. :rtype `list` of `tuple` of `str: """ if name in self._master_db: @@ -65,33 +67,26 @@ class ConfigMain(util.PostfixUtilBase): return None def set(self, name, value, check_override=None): - """Sets parameter |name| to |value|. - If |name| is overridden by a particular service in `master.cf`, calls - `check_override` on |name|, and the set of overrides. + """Sets parameter `name` to `value`. + If `name` is overridden by a particular service in `master.cf`, calls + `check_override` on `name`, and the set of overrides. Note that this function does not flush these parameter values to main.cf; - To do that, use |flush|. + To do that, use `flush`. :param str name: The name of the parameter to set. :param str value: The value of the parameter. """ if name not in self._db: - return # TODO: error here + return # Check to see if this parameter is overridden by master. + # TODO: comment the below overrides = self.get_master_overrides(name) if check_override is not None and overrides is not None: check_override(name, overrides) - # We've updated this name before. - if name in self._updated: - if value == self._updated[name]: - return - if value == self._db[name]: - del self._updated[name] - return - # We haven't updated this name before. - else: - # If we're just setting the default value, ignore. - if value != self._db[name]: - self._updated[name] = value + if value != self._db[name]: + self._updated[name] = value + elif name in self._updated: + del self._updated[name] def flush(self): """Flushes all parameter changes made using "self.set" to "main.cf". diff --git a/certbot-postfix/certbot_postfix/tests/installer_test.py b/certbot-postfix/certbot_postfix/tests/installer_test.py index 07b80d60d..a6f55bf5a 100644 --- a/certbot-postfix/certbot_postfix/tests/installer_test.py +++ b/certbot-postfix/certbot_postfix/tests/installer_test.py @@ -11,7 +11,6 @@ from certbot import errors from certbot.tests import util as certbot_test_util class InstallerTest(certbot_test_util.ConfigTestCase): - # pylint: disable=too-many-public-methods def setUp(self): super(InstallerTest, self).setUp() diff --git a/certbot-postfix/certbot_postfix/tests/postconf_test.py b/certbot-postfix/certbot_postfix/tests/postconf_test.py index 1b0cc376b..b28a20b05 100644 --- a/certbot-postfix/certbot_postfix/tests/postconf_test.py +++ b/certbot-postfix/certbot_postfix/tests/postconf_test.py @@ -8,6 +8,9 @@ import string import tempfile import unittest +# TODO (sydneyli): Mock out calls to postconf +# TODO (sydneyli): inherit certbot.tests.util.TempDirTestCase + def _rand_str(n): """Returns a random string with length n, for use as a temporary directory.""" return ''.join([random.choice(string.lowercase) for _ in xrange(n)]) diff --git a/certbot-postfix/certbot_postfix/util.py b/certbot-postfix/certbot_postfix/util.py index 0eb794ff1..ba4ac9468 100644 --- a/certbot-postfix/certbot_postfix/util.py +++ b/certbot-postfix/certbot_postfix/util.py @@ -29,14 +29,6 @@ class PostfixUtilBase(object): self._set_base_command(config_dir) self.config_dir = None - def update_dir(self, config_dir): - """Updates the directory of the configuration files for Postfix. - - :param str config_dir: The path containing the Postfix configuration files. - """ - self.config_dir = config_dir - self._set_base_command(config_dir) - def _set_base_command(self, config_dir): self._base_command = [self.executable] if config_dir is not None: diff --git a/certbot/client.py b/certbot/client.py index a0d9257df..ee38fdc5e 100644 --- a/certbot/client.py +++ b/certbot/client.py @@ -481,7 +481,7 @@ class Client(object): if enhancement_name in supported: if config_name == "redirect" and config_value is None: config_value = enhancements.ask(enhancement_name) - if config_name == "starttls_policy" and len(config_value) > 0: + if config_name == "starttls_policy" and config_value is not None: option = config_value if config_value: self.apply_enhancement(domains, enhancement_name, option) diff --git a/certbot/constants.py b/certbot/constants.py index 3a330aeca..e19a99747 100644 --- a/certbot/constants.py +++ b/certbot/constants.py @@ -60,7 +60,7 @@ CLI_DEFAULTS = dict( hsts=None, uir=None, staple=None, - starttls_policy="", + starttls_policy=None, strict_permissions=False, pref_challs=[], validate_hooks=True,