fix more commentsunsquashed-postfix

9 files changed, 49 insertions, 32 deletions

diff --git a/certbot-postfix/certbot_postfix/constants.py b/certbot-postfix/certbot_postfix/constants.py
index 74b43d899..40a263a53 100644
--- a/certbot-postfix/certbot_postfix/constants.py
+++ b/certbot-postfix/certbot_postfix/constants.py
@@ -1,5 +1,9 @@
 """Postfix plugin constants."""
 
+# pylint: disable=unused-import, no-name-in-module
+from acme.magic_typing import Dict, Tuple, Union
+# pylint: enable=unused-import, no-name-in-module
+
 MINIMUM_VERSION = (2, 11,)
 
 # If the value of a default VAR is a tuple, then the values which
@@ -26,10 +30,10 @@ ACCEPTABLE_TLS_VERSIONS = ("TLSv1", "TLSv1.1", "TLSv1.2")
 # Variables associated with enabling opportunistic TLS.
 TLS_SERVER_VARS = {
     "smtpd_tls_security_level": ACCEPTABLE_SERVER_SECURITY_LEVELS,
-}
+} # type:Dict[str, Tuple[str, ...]]
 TLS_CLIENT_VARS = {
     "smtp_tls_security_level": ACCEPTABLE_CLIENT_SECURITY_LEVELS,
-}
+} # type:Dict[str, Tuple[str, ...]]
 # Default variables for a secure MTA server [receiver].
 DEFAULT_SERVER_VARS = {
     "smtpd_tls_auth_only": ("yes",),
@@ -39,14 +43,14 @@ DEFAULT_SERVER_VARS = {
     "smtpd_tls_mandatory_ciphers": ACCEPTABLE_CIPHER_LEVELS,
     "smtpd_tls_exclude_ciphers": EXCLUDE_CIPHERS,
     "smtpd_tls_eecdh_grade": ("strong",),
-}
+} # type:Dict[str, Tuple[str, ...]]
 
 # Default variables for a secure MTA client [sender].
 DEFAULT_CLIENT_VARS = {
     "smtp_tls_ciphers": ACCEPTABLE_CIPHER_LEVELS,
     "smtp_tls_exclude_ciphers": EXCLUDE_CIPHERS,
     "smtp_tls_mandatory_ciphers": ACCEPTABLE_CIPHER_LEVELS,
-}
+} # type:Dict[str, Tuple[str, ...]]
 
 CLI_DEFAULTS = dict(
     config_dir="/etc/postfix",
diff --git a/certbot-postfix/certbot_postfix/installer.py b/certbot-postfix/certbot_postfix/installer.py
index ad614f084..9ba92ef8f 100644
--- a/certbot-postfix/certbot_postfix/installer.py
+++ b/certbot-postfix/certbot_postfix/installer.py
@@ -17,6 +17,7 @@ from certbot_postfix import util
 
 # pylint: disable=unused-import, no-name-in-module
 from acme.magic_typing import Callable, Dict, List
+# pylint: enable=unused-import, no-name-in-module
 
 logger = logging.getLogger(__name__)
 
@@ -164,16 +165,13 @@ class Installer(plugins_common.Installer):
         return certbot_util.get_filtered_names(self.postconf.get(var)
                    for var in ('mydomain', 'myhostname', 'myorigin',))
 
-
     def _set_vars(self, var_dict):
-        """Sets all parameters in var_dict to config file.
+        """Sets all parameters in var_dict to config file. If current value is already set
+        as more secure (acceptable), then don't set/overwrite it.
         """
         for param, acceptable in six.iteritems(var_dict):
             if not util.is_acceptable_value(param, self.postconf.get(param), acceptable):
-                if isinstance(acceptable, tuple):
-                    self.postconf.set(param, acceptable[0], acceptable)
-                else:
-                    self.postconf.set(param, acceptable, (acceptable,))
+                self.postconf.set(param, acceptable[0], acceptable)
 
     def _confirm_changes(self):
         """Confirming outstanding updates for configuration parameters.
diff --git a/certbot-postfix/certbot_postfix/postconf.py b/certbot-postfix/certbot_postfix/postconf.py
index eb064bf2c..466e0e63e 100644
--- a/certbot-postfix/certbot_postfix/postconf.py
+++ b/certbot-postfix/certbot_postfix/postconf.py
@@ -6,6 +6,7 @@ from certbot_postfix import util
 
 # pylint: disable=unused-import, no-name-in-module
 from acme.magic_typing import Dict, List, Tuple
+# pylint: enable=unused-import, no-name-in-module
 
 class ConfigMain(util.PostfixUtilBase):
     """A parser for Postfix's main.cf file."""
@@ -132,7 +133,7 @@ class ConfigMain(util.PostfixUtilBase):
 def _parse_main_output(output):
     """Parses the raw output from Postconf about main.cf.
 
-    Expects the output to look like: 
+    Expects the output to look like:
 
     .. code-block:: none
 
diff --git a/certbot-postfix/certbot_postfix/tests/installer_test.py b/certbot-postfix/certbot_postfix/tests/installer_test.py
index ee46a1ec0..1bdd2c8b3 100644
--- a/certbot-postfix/certbot_postfix/tests/installer_test.py
+++ b/certbot-postfix/certbot_postfix/tests/installer_test.py
@@ -14,6 +14,7 @@ from certbot.tests import util as certbot_test_util
 
 # pylint: disable=unused-import, no-name-in-module
 from acme.magic_typing import Dict, Tuple, Union
+# pylint: enable=unused-import, no-name-in-module
 
 DEFAULT_MAIN_CF = {
     "smtpd_tls_cert_file": "",
@@ -196,17 +197,14 @@ class InstallerTest(certbot_test_util.ConfigTestCase):
             installer.deploy_cert("example.com", "cert_path", "key_path",
                                   "chain_path", "fullchain_path")
             changes = installer.postconf.get_changes()
-            expected = {} # type: Dict[str, Union[Tuple[str, str], str]]
+            expected = {} # type: Dict[str, Tuple[str, ...]]
             expected.update(constants.TLS_SERVER_VARS)
             expected.update(constants.DEFAULT_SERVER_VARS)
             expected.update(constants.DEFAULT_CLIENT_VARS)
             self.assertEqual(changes["smtpd_tls_key_file"], "key_path")
             self.assertEqual(changes["smtpd_tls_cert_file"], "cert_path")
             for name, value in six.iteritems(expected):
-                if isinstance(value, tuple):
-                    self.assertEqual(changes[name], value[0])
-                else:
-                    self.assertEqual(changes[name], value)
+                self.assertEqual(changes[name], value[0])
 
     @certbot_test_util.patch_get_utility()
     def test_tls_only(self, mock_util):
diff --git a/certbot-postfix/certbot_postfix/tests/util_test.py b/certbot-postfix/certbot_postfix/tests/util_test.py
index 94b549e19..fa38f83ab 100644
--- a/certbot-postfix/certbot_postfix/tests/util_test.py
+++ b/certbot-postfix/certbot_postfix/tests/util_test.py
@@ -37,11 +37,11 @@ class PostfixUtilBaseTest(unittest.TestCase):
             postfix._call()
             mock_output.assert_called_with(['executable'])
 
-    @mock.patch('certbot_postfix.util.verify_exe_exists')
-    def test_create_with_config(self, mock_verify):
+    def test_create_with_config(self):
         # pylint: disable=protected-access
-        postfix = self._create_object('exec', 'config_dir')
-        self.assertEqual(postfix._base_command, ['exec', '-c', 'config_dir'])
+        with mock.patch('certbot_postfix.util.verify_exe_exists'):
+            postfix = self._create_object('exec', 'config_dir')
+            self.assertEqual(postfix._base_command, ['exec', '-c', 'config_dir'])
 
 class PostfixUtilTest(unittest.TestCase):
     def setUp(self):
@@ -162,6 +162,10 @@ class TestUtils(unittest.TestCase):
         report_master_overrides('name', [('service/type', 'value')],
                                 acceptable_overrides=('value',))
 
+    def test_no_acceptable_value(self):
+        from certbot_postfix.util import is_acceptable_value
+        self.assertFalse(is_acceptable_value('name', 'value', None))
+
     def test_is_acceptable_value(self):
         from certbot_postfix.util import is_acceptable_value
         self.assertTrue(is_acceptable_value('name', 'value', ('value',)))
@@ -175,6 +179,8 @@ class TestUtils(unittest.TestCase):
     def test_is_acceptable_protocols(self):
         from certbot_postfix.util import is_acceptable_value
         # SSLv2 and SSLv3 are both not supported, unambiguously
+        self.assertFalse(is_acceptable_value('tls_mandatory_protocols_lol',
+            'SSLv2, SSLv3', None))
         self.assertFalse(is_acceptable_value('tls_protocols_lol',
             'SSLv2, SSLv3', None))
         self.assertFalse(is_acceptable_value('tls_protocols_lol',
diff --git a/certbot-postfix/certbot_postfix/util.py b/certbot-postfix/certbot_postfix/util.py
index fc87d20fe..f06989903 100644
--- a/certbot-postfix/certbot_postfix/util.py
+++ b/certbot-postfix/certbot_postfix/util.py
@@ -203,11 +203,14 @@ def verify_exe_exists(exe, message=None):
         raise errors.NoInstallationError(message)
 
 def report_master_overrides(name, overrides, acceptable_overrides=None):
-    """If the value for a parameter |name| is overridden by other services,
-    report a warning to notify the user.
+    """If the value for a parameter `name` is overridden by other services,
+    report a warning to notify the user. If `parameter` is a TLS version parameter
+    (i.e., `parameter` contains 'tls_protocols' or 'tls_mandatory_protocols'), then
+    `acceptable_overrides` isn't used each value in overrides is inspected for secure TLS
+    versions.
 
     :param str name: The name of the parameter that is being overridden.
-    :param list overrides: The values that other services are setting for |name|.
+    :param list overrides: The values that other services are setting for `name`.
         Each override is a tuple: (service name, value)
     :param tuple acceptable_overrides: Override values that are acceptable. For instance, if
         another service is overriding our parameter with a more secure option, we don't have
@@ -225,9 +228,12 @@ def report_master_overrides(name, overrides, acceptable_overrides=None):
         raise errors.PluginError("{0} is overridden with less secure options by the "
              "following services in master.cf:\n".format(name) + error_string)
 
-def is_acceptable_value(parameter, value, acceptable):
+
+def is_acceptable_value(parameter, value, acceptable=None):
     """ Returns whether the `value` for this `parameter` is acceptable,
-    given a string or tuple `acceptable`
+    given a tuple of `acceptable` values. If `parameter` is a TLS version parameter
+    (i.e., `parameter` contains 'tls_protocols' or 'tls_mandatory_protocols'), then
+    `acceptable` isn't used and `value` is inspected for secure TLS versions.
 
     :param str parameter:       The name of the parameter being set.
     :param str value:           Proposed new value for parameter.
@@ -235,16 +241,17 @@ def is_acceptable_value(parameter, value, acceptable):
     """
     # Check if param value is a comma-separated list of protocols.
     # Otherwise, just check whether the value is in the acceptable list.
-    if 'tls_protocols' in parameter:
+    if 'tls_protocols' in parameter or 'tls_mandatory_protocols' in parameter:
         return _has_acceptable_tls_versions(value)
     if acceptable is not None:
         return value in acceptable
     return False
 
+
 def _has_acceptable_tls_versions(parameter_string):
     """
     Checks to see if the list of TLS protocols is acceptable.
-    This means TLSv1.2 is supported, and neither SSLv2 nor SSLv3 are supported.
+    This requires that TLSv1.2 is supported, and neither SSLv2 nor SSLv3 are supported.
 
     Should be a string of protocol names delimited by commas, spaces, or colons.
 
@@ -255,9 +262,9 @@ def _has_acceptable_tls_versions(parameter_string):
     When these two modes are interspersed, the presence of a single non-negated protocol name
     (i.e. "TLSv1" rather than "!TLSv1") automatically excludes all other unnamed protocols.
 
-    In addition, the presence of a protocol name overrides any exclusion, so "SSLv3, !SSLv3"
-    supports SSLv3. This behavior isn't explicitly documented, so this method should return False
-    if it encounters contradicting statements about TLSv1.2, SSLv2, or SSLv3.
+    In addition, the presence of both a protocol name inclusion and exclusion isn't explicitly
+    documented, so this method should return False if it encounters contradicting statements
+    about TLSv1.2, SSLv2, or SSLv3. (for instance, "SSLv3, !SSLv3").
     """
     if not parameter_string:
         return False
diff --git a/certbot-postfix/docs/conf.py b/certbot-postfix/docs/conf.py
index eed404f1d..51d99aab5 100644
--- a/certbot-postfix/docs/conf.py
+++ b/certbot-postfix/docs/conf.py
@@ -103,7 +103,7 @@ if not on_rtd:  # only import and set the theme if we're building docs locally
 # Add any paths that contain custom static files (such as style sheets) here,
 # relative to this directory. They are copied after the builtin static files,
 # so a file named "default.css" will overwrite the builtin "default.css".
-html_static_path = ['_static']
+# html_static_path = ['_static']
 
 # Custom sidebar templates, must be a dictionary that maps document names
 # to template names.
@@ -187,4 +187,4 @@ intersphinx_mapping = {
 # -- Options for todo extension ----------------------------------------------
 
 # If true, `todo` and `todoList` produce output, else they produce nothing.
-todo_include_todos = True
\ No newline at end of file
+todo_include_todos = True
diff --git a/certbot-postfix/local-oldest-requirements.txt b/certbot-postfix/local-oldest-requirements.txt
new file mode 100644
index 000000000..cbcfc4d0c
--- /dev/null
+++ b/certbot-postfix/local-oldest-requirements.txt
@@ -0,0 +1,2 @@
+-e acme[dev]
+certbot[dev]==0.23.0
diff --git a/certbot-postfix/setup.py b/certbot-postfix/setup.py
index 85f12e100..83eed06d1 100644
--- a/certbot-postfix/setup.py
+++ b/certbot-postfix/setup.py
@@ -5,6 +5,7 @@ from setuptools import find_packages
 version = '0.24.0.dev0'
 
 install_requires = [
+    'acme>=0.25.0',
     'certbot>0.23.0',
     'setuptools',
     'six',