Gradually increasing HSTS max-age #5912

joohoi · 2018-05-02T11:06:32Z

This PR adds the functionality to enhance Apache configuration to include HTTP Strict Transport Security header with a low initial max-age value.

The max-age value will get increased on every (scheduled) run of certbot renew regardless of the certificate actually getting renewed, if the last increase took place longer than ten hours ago. The increase steps are visible in constants.AUTOHSTS_STEPS.

Upon the first actual renewal after reaching the maximum increase step, the max-age value will be made "permanent" and will get value of one year.

To achieve accurate VirtualHost discovery on subsequent runs, a comment with unique id string will be added to each enhanced VirtualHost.

joohoi · 2018-05-02T13:06:39Z

The Py3 tests will fail until #5913 is merged and this branch is brought up to date.

bmw · 2018-05-04T21:12:59Z

certbot-apache/certbot_apache/constants.py

@@ -48,3 +48,16 @@

 HEADER_ARGS = {"Strict-Transport-Security": HSTS_ARGS,
               "Upgrade-Insecure-Requests": UIR_ARGS}
+
+AUTOHSTS_STEPS = [60, 300, 900, 3600, 21600, 43200, 86400]
+"""AutoHSTS increase steps: 5min, 15min, 1h, 6h, 12h, 24h"""


nit: We're missing the first step of 1 min here.

bmw · 2018-05-04T21:56:25Z

certbot-apache/certbot_apache/configurator.py

@@ -1470,6 +1471,63 @@ def _add_name_vhost_if_necessary(self, vhost):
        if need_to_save:
            self.save()

+    def find_vhost_by_id(self, id_str):


Everywhere this function is called outside of tests, no checks are done that the return type isn't None. This to me suggestions we should raise an exception here since we're failing to find the vhost we're supposed to be updating.

bmw · 2018-05-04T22:09:41Z

certbot-apache/certbot_apache/configurator.py

+        :type ssl_vhost: :class:`~certbot_apache.obj.VirtualHost` or None
+
+        """
+        try:


I think you can remove the try and except here. certbot.client.Client.apply_enhancement already catches PluginEnhancementAlreadyPresent errors, logs a message, and continues execution.

bmw · 2018-05-04T22:30:49Z

certbot-apache/certbot_apache/configurator.py

+            # AutoHSTS not enabled
+            return
+        curtime = time.time()
+        for id_str in managed:


nit: I'd use something like:

for id_str, config in managed.items():

for shorter lines in the body of the loop and to avoid repeatedly looking up the matching config values.

bmw · 2018-05-04T22:34:12Z

certbot-apache/certbot_apache/configurator.py

+            return
+        vhosts = []
+        # Copy, as we are removing from the dict inside the loop
+        for id_str in list(managed.keys()):


nit: I'd use something like:

for id_str, config in list(managed.items()):

bmw · 2018-05-04T22:49:57Z

certbot-apache/certbot_apache/configurator.py

+        Matches the private key path.
+        """
+
+        return any(


I don't think you need this any here. any iterates over the list calling bool on each item, returning True if any item in the list is True. All we're checking here is that the list is non-empty right?

If so, I think the most Pythonic thing to do is just return the list or use bool instead of any if you want True or False to be returned.

bmw · 2018-05-04T22:51:42Z

certbot-apache/certbot_apache/configurator.py

+            managed = self.storage.fetch("autohsts")
+        except KeyError:
+            # AutoHSTS not enabled
+            return


In the long run, I'd like to get auto_hsts saved in the renewal conf file and if this method is called and autohsts isn't in plugin storage, we blow up. This helps people get notified if .pluginstorage.json is deleted.

That sounds like a good way to address this. Let's look into it as a part of the larger discussion about the enhancement interfaces.

bmw · 2018-05-04T23:05:32Z

certbot-apache/certbot_apache/configurator.py

+                    "of {0} to VirtualHost in {1}\n".format(
+                        initial_maxage, ssl_vhost.filep))
+        self.save_notes += note_msg
+        self.save(note_msg)


If you're calling this method from the old enhancement system, you shouldn't call save here. This is done for you in certbot.client.Client.apply_enhancement.

This will get addressed as a part of the enhancement interfaces refactor.

bmw · 2018-05-04T23:08:31Z

certbot/cli.py

+        "--auto-hsts", action="store_true", dest="auto_hsts",
+        default=flag_default("auto_hsts"),
+        help="Automatically increase Strict-Transport-Security header maxAge "
+             "value over time. Implies --hsts.")


What happens when both --auto-hsts and --hsts are set?

Valid question. This isn't currently handled too well. I believe we should ignore --hsts and go with the --auto-hsts, as that's what the user most likely wants.

That'd be fine, but I personally would prefer just failing fast and telling the user to pick only one. This avoids us having to make a call about what we think the user wants and directly tells them that their command doesn't make sense. This could be done in certbot.cli.HelpfulArgumentParser.parse_args.

bmw · 2018-05-04T23:11:59Z

certbot-apache/certbot_apache/configurator.py

+        when running certbot with "renew" verb, regardless of the lineage
+        being renewed.
+        """
+        self._autohsts_update()


I thought the plan was to make another enhancement type for autohsts updates. Did you change your mind or do you want to do this in another PR?

I was thinking of doing this later, as a part of a redesign of the complex enhancements but looks like it would make most sense to implement it as a part of this PR as well. I'm answering this in more detail in the comment about the interfaces, and old enhancement system.

bmw

In addition to my inline comments, I think we need to be calling restart() somewhere after adding/modifying the HSTS value.

bmw · 2018-06-07T17:44:53Z

certbot/cli.py

@@ -1124,6 +1129,16 @@ def prepare_and_parse_args(plugins, args, detect_defaults=False):  # pylint: dis
        default=flag_default("redirect"),
        help="Do not automatically redirect all HTTP traffic to HTTPS for the newly "
             "authenticated vhost. (default: Ask)")
+    for enh in enhancements._INDEX:  # pylint: disable=protected-access


nit: Maybe we should make a simple function in the enhancements module that takes the add method of helpful and adds the flag for all enhancements. This is similar to what we do for plugins, breaks up this large function, and allows us to remove the pylint: disable.

Yeah, this makes it cleaner, and is now implemented.

bmw · 2018-06-07T17:46:19Z

certbot/cli.py

+        helpful.add(enh["cli_groups"], enh["cli_flag"], action=enh["cli_action"],
+                    dest=enh["cli_dest"], default=enh["cli_flag_default"],
+                    help=enh["cli_help"])
+    helpful.add(


Do we still need this here?

Sorry, this is some leftover cruft from the earlier iteration without the new enhancement system. Removed.

bmw · 2018-06-07T17:47:57Z

certbot/client.py

@@ -516,6 +516,7 @@ def enhance_config(self, domains, chain_path, ask_redirect=True):

        enhanced = False
        enhancement_info = (
+            ("auto_hsts", "auto_hsts", None),


I think this is cruft that can be removed. When will auto_hsts ever be returned from the call to supported_enhancements()?

Sorry, this is some leftover cruft from the earlier iteration without the new enhancement system. Removed.

bmw · 2018-06-07T18:00:01Z

certbot/plugins/enhancements.py

+    for enh in _INDEX:
+        enh_requested = hasattr(config, enh["cli_dest"])
+        enh_enabled = bool(getattr(config, enh["cli_dest"]))
+        if enh_requested and enh_enabled:


We duplicate these four lines elsewhere in this module and I think they can be simplified and this duplication removed.

I don't think we need this cast to bool here. Also, in what scenario are we worried about the config not having the enh["cli_dest"] attribute? I couldn't come up with one so I think we can remove it but if there is one, we can make use of the default value parameter to getattr and do this with one value rather than two.

Additionally, I wonder if we shouldn't make a simple generator function that yields the requested enhancements. This method could be public or private, but would look something like:

def enabled_enhancements(config): for enh in _INDEX: if getattr(config, enh["cli_dest"]): yield enh

Then this method could be as simple as:

def is_supported(config): return any(enabled_enhancements(config))

Up to you if you want to add the generator method, but I think the casting and using two variables when one will do should be fixed at least.

I like this approach, the now implemented generator sure yields (pun intended) cleaner and more readable code.

bmw · 2018-06-07T18:23:29Z

certbot/plugins/enhancements.py

+from acme.magic_typing import Dict, List, Any  # pylint: disable=unused-import, no-name-in-module
+
+
+def is_supported(config):


nit: I wonder if a name like is_requested or is_enabled is better here. Initially, I thought this function was checking if the requested enhancements were supported by the plugin. I also personally like are rather than is because there will be multiple enhancements in the future, but that's a super nitpick. I think you may have been trying to differentiate between enhancements defined here and defined elsewhere, but I personally think it's fine to just explain this in a comment.

If you disagree feel free to leave it as is.

Changed, and it's better this way, thanks!

bmw · 2018-06-07T23:32:11Z

certbot-apache/certbot_apache/configurator.py

+                    vhost = self.find_vhost_by_id(id_str)
+                except errors.PluginError:
+                    # Remove the orphaned AutoHSTS entry from pluginstorage
+                    self._autohsts.pop(id_str)


I think we should log about this here.

bmw · 2018-06-07T23:43:57Z

certbot-apache/certbot_apache/configurator.py

+                    continue
+                if self._autohsts_vhost_in_lineage(vhost, lineage):
+                    vhosts.append(vhost)
+                    self._autohsts.pop(id_str)


This is definitely a nit, but if Certbot gets interrupted between this line and the Apache config update down below, they'll never get the large max-age bump. Maybe we should keep the relevant id strings around and pop them afterwards?

Yeah, handling the modification later on in the code now.

bmw · 2018-06-07T23:49:26Z

certbot-apache/certbot_apache/tests/autohsts_test.py

+            self.assertTrue(
+                "VirtualHost with id orphan_id was not" in mock_log.call_args[0][0])
+
+


super nit: Too many blank lines.

bmw · 2018-06-07T23:55:35Z

certbot-apache/certbot_apache/constants.py

+AUTOHSTS_PERMANENT = 31536000
+"""Value for the last max-age of HSTS"""
+
+AUTOHSTS_FREQ = 36000


I still think this value and/or AUTOHSTS_STEPS should be increased. Do you disagree?

bmw · 2018-06-08T00:05:32Z

certbot/main.py

+        le_client = _init_le_client(config, authenticator=None, installer=installer)
+        le_client.enhance_config(domains, config.chain_path, ask_redirect=False)
+    if enhancements.is_supported(config):
+        enhancements.enable(lineage, domains, installer, config)


This enhancement can currently only be enabled with the enhance subcommand. I think in the long term we also want it to work for run and install.

If this was on purpose and you want to do it later, that's fine, otherwise, I think this should be moved to le_client.enhance_config.

I will add this functionality after #6097 lands.

bmw · 2018-06-14T23:04:05Z

certbot/plugins/enhancement_test.py

@@ -0,0 +1,179 @@
+"""Tests for new style enhancements"""


We still need to do #5912 (comment).

bmw · 2018-06-14T23:16:42Z

certbot/main.py

@@ -908,11 +909,15 @@ def enhance(config, plugins):
        if not domains:
            raise errors.Error("User cancelled the domain selection. No domains "
                               "defined, exiting.")
+
+    lineage = cert_manager.lineage_for_certname(config, config.certname)


Oh Python. Nice catch moving this outside of the if not config.chain_path branch to avoid an ugly crash down below when calling enhancements.enable.

bmw · 2018-06-14T23:17:52Z

certbot-apache/setup.py

@@ -8,7 +8,7 @@
 # acme/certbot version.
 install_requires = [
    'acme>0.24.0',
-    'certbot>=0.21.1',
+    'certbot>0.24.0',


This should now be set to certbot>0.25.1

bmw · 2018-06-14T23:24:20Z

certbot-apache/certbot_apache/configurator.py

+                    "of {0} to VirtualHost in {1}\n".format(
+                        initial_maxage, ssl_vhost.filep))
+        self.save_notes += note_msg
+        self.save(note_msg)


We need to call restart after this.

Also, while I missed this in my previous review, the way we're using save here is different from the other AutoHSTS methods and the old enhancement system. Here we're calling save after enabling HSTS for each domain, with each save creating a checkpoint, rather than creating one save at the end. I think the single save is nicer rather than creating up to 100 checkpoints (and doing 100 restarts) in a single run.

joohoi · 2018-06-15T17:11:31Z

These are now fixed, and the new style enhancements have been enabled for install and run verbs as well. There are a few tests I'd like to write for run and install verbs making sure that we:

Fail early if installer doesn't support requested enhancements
Don't try to proceed if we don't have config.certname (case with install verb when used with custom key & cert)
Enhancements get triggered

Functionality wise it should be ready for the next round though.

bmw · 2018-06-15T20:31:29Z

certbot-apache/certbot_apache/configurator.py

+        if _enhanced_vhosts:
+            note_msg = "Enabling AutoHSTS"
+            self.save(note_msg)
+            logger.info(note_msg)


I think we need to call restart() here.

bmw · 2018-06-15T21:27:39Z

certbot/tests/util.py

@@ -379,3 +380,26 @@ def hold_lock(cv, lock_path):  # pragma: no cover
    cv.notify()
    cv.wait()
    my_lock.release()
+
+
+class MockInstallerAutoHSTS(enhancements.AutoHSTSEnhancement):


I'm not sure if we need this class. I think we can get by with setting the spec attribute of the Mock class to enhancements.AutoHSTSEnhancement so it'll pass isinstance checks. Is there something else we get here?

We get the call counters for the interface methods, that are easy to check in tests. For this I'd like to keep it.

But what's wrong with installer.update_autohsts.call_count, installer.update_autohsts.called, installer.update_autohsts.call_args, etc?

This seems more conventional and takes less code. The only change with the current approach I see is you access these values on installer.update_counter rather than installer.update_autohsts.

You have a point, and the tests are now refactored. The tests for interfaces.GenericUpdater and interfaces.RenewDeployer were similar and handled in the same files, so I went ahead and refactored those too for consistency.

bmw · 2018-06-15T21:34:22Z

certbot/plugins/enhancement_test.py

+    @test_util.patch_get_utility()
+    def test_enhancement_updates(self, _, mock_select):
+        mock_select.return_value = (self.mockinstaller, None)
+        updater.run_generic_updaters(self.config, mock.MagicMock(), None)


Sorry to keep nitpicking this, but I think tests that are calling into other modules like updater should be deleted or moved to that module. I think we should just be testing the functions in enhancements here. The tests should be very simple.

This makes a lot of sense, and the tests have now been refactored as such.

bmw · 2018-06-15T21:34:50Z

certbot/plugins/enhancement_test.py

@@ -0,0 +1,103 @@
+"""Tests for new style enhancements"""


We should rename this file to the plural "enhancements" to match the name of the module.

bmw · 2018-06-19T17:48:37Z

certbot/tests/main_test.py

+
+    @mock.patch('certbot.main.plug_sel.record_chosen_plugins')
+    @mock.patch('certbot.main.plug_sel.pick_installer')
+    def test_install_enhancement_not_supported(self, mock_inst, _rec):


One more test location nit. This docstring for this test class is "Tests for certbot.main.enhance.", but this test and the two below it aren't testing the enhance method. I think the one calling run should be moved to RunTest. We don't have an InstallTest class, but I'd prefer we created one and put these methods there.

bmw · 2018-06-19T17:56:29Z

certbot/tests/renewupdater_test.py

@@ -75,6 +62,68 @@ def test_deployer_skip_dry_run(self, mock_log):
        self.assertEquals(mock_log.call_args[0][0],
                          "Skipping renewal deployer in dry-run mode.")

+    @mock.patch('certbot.plugins.selection.choose_configurator_plugins')
+    @test_util.patch_get_utility()


I don't think we need to call patch_get_utility here or on any of the tests below. I just ran tests with them removed and they still passed.

bmw · 2018-06-20T17:24:54Z

certbot/tests/main_test.py

+            self._call, ['enhance', '--auto-hsts', '--hsts'])
+
+    @mock.patch('certbot.main.plug_sel.choose_configurator_plugins')
+    def test_run_enhancement_not_supported(self, mock_choose):


It looks like part of my comment at #5912 (comment) was missed. Can we move this to the RunTest class in this file?

I'm very sorry, this is getting embarrassing. I'm apparently too eager to get the PR out of my hands. Moving the tests now.

No worries!

joohoi added 3 commits May 2, 2018 12:15

AutoHSTS code rebased on master

7a9e82d

Fixes to match the changes in master

8b88bc6

Python3 fixes

dc6f2eb

joohoi added 2 commits May 3, 2018 11:27

Merge remote-tracking branch 'origin/master' into autohsts_apache

c098c73

Make linter happy with metaclass registration

9513bb7

bmw requested changes May 4, 2018

View reviewed changes

joohoi added 9 commits May 7, 2018 16:28

Address small review comments

b4b2943

Use new enhancement interfaces

a641eef

Merge remote-tracking branch 'origin/master' into autohsts_apache

cc0638b

New style enhancement changes

195c962

Merge remote-tracking branch 'origin/master' into autohsts_apache

196f716

Test and annotation fixes

2bf0efe

Do not allow --hsts and --auto-hsts simultaneuously

8021f84

MyPy annotation fixes and added test

83c8397

Change oldest requrements to point to local certbot core version

cf3697a

joohoi force-pushed the autohsts_apache branch from 234abd0 to cf3697a Compare June 6, 2018 18:23

bmw requested changes Jun 8, 2018

View reviewed changes

Address review comments

5711dec

bmw requested changes Jun 14, 2018

View reviewed changes

joohoi added 3 commits June 15, 2018 13:51

Move tests and fix deps

a770704

Merge remote-tracking branch 'origin/master' into autohsts_apache

2c73f8b

Enable new style enhancements for run and install verbs

807cde0

bmw requested changes Jun 15, 2018

View reviewed changes

Fix tests

be299d9

joohoi force-pushed the autohsts_apache branch from 52aec89 to be299d9 Compare June 17, 2018 15:28

Test refactor

55db1d5

bmw requested changes Jun 19, 2018

View reviewed changes

New test class for main.install tests

e455754

bmw requested changes Jun 20, 2018

View reviewed changes

Move a test to a correct test class

b3399d7

bmw approved these changes Jun 21, 2018

View reviewed changes

bmw merged commit 3877af6 into master Jun 21, 2018

bmw deleted the autohsts_apache branch June 21, 2018 14:27

bmw added this to the 0.26.0 milestone Jul 11, 2018

		from acme.magic_typing import Dict, List, Any # pylint: disable=unused-import, no-name-in-module


		def is_supported(config):

		self.assertTrue(
		"VirtualHost with id orphan_id was not" in mock_log.call_args[0][0])

Gradually increasing HSTS max-age #5912

Gradually increasing HSTS max-age #5912

Conversation

joohoi commented May 2, 2018

joohoi commented May 2, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw May 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joohoi commented Jun 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joohoi commented May 2, 2018 •

edited

bmw May 7, 2018 •

edited