883582 - HSTS preload list automatic update needs to not remove URLs that are used in tests

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Phil Ringnalda (:philor)]

https://tbpl.mozilla.org/?rev=f691f7abfe33 and https://tbpl.mozilla.org/?tree=Mozilla-Aurora&rev=8ff76e6fe317 are strikes one and two, and you only get three strikes when you're doing things that require me to push backouts to those trees on a Saturday morning before I go to work.

The comment in http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js#49 explains that the test needs to be modified if the URLs it uses are removed from the list, but apparently bug 836097 forgot to teach the automatic update script how to read comments in tests.

Multiple possible ways to fix it:

* take removing things from the list out of the automatic script, only letting it add things, and do removals manually (a very amusing way of doing that being to still let the automatic script look for them, but have it use the bzexport Mercurial extension to create a patch, file a bug, and request review on the patch)

* have the automatic script not push to m-c and m-a, but instead push those trees to Try, and email you the tbpl URLs for its try pushes, and you can then push them if they succeed

* rather than using the actual list for the test, do what many similar tests in similar situations do, and temporarily replace the real list with a fake list that only has the things the test wants it to have

* add some invalid, fake, testing-only things to the regular list, teach the script not to check them, and have the test test for them

One way to lose the automatic updates completely:

* leave it going just like it is so it pushes the same thing and breaks the tests the same way next Saturday, so I have to push backouts to mozilla-central and mozilla-aurora first thing Saturday morning again

Comment 1

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: test changes

In the clear light of retrospect, this was a poor setup. Apologies for the extra work. Here's what I think we should do:
1. Modify the test to only use a smaller number of domains and only domains likely to remain on the preload list for a long time.
2. Have the automated updater not check in to mozilla-aurora (for now), and check in to mozilla-inbound instead of mozilla-central.
3. Manually uplift any changes that successfully make it to central from inbound.
4. If the updater behaves correctly for an acceptable period of time, turn back on automated uplifting to mozilla-aurora (or perhaps figure out how to automatically uplift when changes successfully go from inbound to central).

Comment 2

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(In reply to David Keeler (:keeler) from comment #1)
> 2. Have the automated updater not check in to mozilla-aurora (for now), and
> check in to mozilla-inbound instead of mozilla-central.

Coop, would you be able to take care of this? (barring any objections from anyone cc'd on this bug)

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 764410 [details] [diff] [review]
test changes

Review of attachment 764410 [details] [diff] [review]:
-----------------------------------------------------------------

philor wrote:
> One way to lose the automatic updates completely:
> 
> * leave it going just like it is so it pushes the same thing and breaks the
> tests the same way next Saturday, so I have to push backouts to
> mozilla-central and mozilla-aurora first thing Saturday morning again

These kinds of threats aren't necessary. More politeness, please. Thanks for reporting the issue.

Thanks David for fixing this.

::: security/manager/ssl/tests/unit/test_sts_preloadlist_perwindowpb.js
@@ +21,5 @@
> +// (so we can remove any state added to the sts service)
> +var hosts = ["bugzilla.mozilla.org", "login.persona.org",
> +             "subdomain.www.torproject.org", "subdomain.bugzilla.mozilla.org" ];
> +
> +function clearStsState() {

Please add a comment about how this removes knockout entries that removeStsState doesn't remove.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to David Keeler (:keeler) from comment #2)
> (In reply to David Keeler (:keeler) from comment #1)
> > 2. Have the automated updater not check in to mozilla-aurora (for now), and
> > check in to mozilla-inbound instead of mozilla-central.
> 
> Coop, would you be able to take care of this? (barring any objections from
> anyone cc'd on this bug)

I think we should really just keep things the way they are. I agree that we shouldn't auto-update mozilla-beta but I think mozilla-aurora is fine. 18 weeks from the aurora->beta merge allows the user to be protected with the HSTS preload through the entire time between release updates plus some slack time. If we only did the updates in mozilla-inbound/central then we'd need to change the 18 week window to be even longer.

I think the patch you posted is reasonable as far as reducing the likelihood of the test causing test failures. But, if we are still worried about it then we should just change the auto-updater so that it doesn't remove entries automatically.

Comment 5

[Chris Cooper [:coop] (he/him)]

(In reply to Brian Smith (:bsmith) from comment #4)
> I think we should really just keep things the way they are. I agree that we
> shouldn't auto-update mozilla-beta but I think mozilla-aurora is fine. 18
> weeks from the aurora->beta merge allows the user to be protected with the
> HSTS preload through the entire time between release updates plus some slack
> time. If we only did the updates in mozilla-inbound/central then we'd need
> to change the 18 week window to be even longer.
> 
> I think the patch you posted is reasonable as far as reducing the likelihood
> of the test causing test failures. But, if we are still worried about it
> then we should just change the auto-updater so that it doesn't remove
> entries automatically.

OK. If we do decide to stop updating aurora (or even m-c), just let me know. It's a quick patch to turn it on or off on the various branches.

Comment 6

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Attachment: test changes v1.1

Carrying over r+.

Comment 7

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/de2cab422ac0

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/de2cab422ac0

Comment 9

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

Uplifted with a=testonly
https://hg.mozilla.org/releases/mozilla-aurora/rev/6014e97def09