1382001 - Use an even more efficient format for kSTSPreloadList

Status: RESOLVED FIXED

(Core :: Security: PSM, enhancement)

Description

[Eric Rahm [:erahm]]

+++ This bug was initially created as a clone of Bug #1255425 +++

In bug 1380154 I'm working on a more efficient way of storing a large set of constant strings for the effective TLD service. We should be able to extend this approach to the HSTS preload list. In addition to reducing our binary size, it should also help with performance (the lookup should be quicker than our binary search of ~22k elements).

Comment 1

[Eric Rahm [:erahm]]

It looks like kSTSPreloadList is found in 'nsSTSPreloadList.inc' [1] generated by an xpcshell script [2] that downloads the list and converts it to a form suitable for using in Firefox. Ideally we'd convert the xpcshell script to output data in the gperf format expected by our dafsa script. Then we'd add a generated file hook to convert that to a header.

[1] http://searchfox.org/mozilla-central/source/security/manager/ssl/nsSTSPreloadList.inc
[2] http://searchfox.org/mozilla-central/source/security/manager/tools/getHSTSPreloadList.js

Comment 3

[Eric Rahm [:erahm]]

Attachment: Part 1: Handle gperf preamble in make_dafsa

MozReview-Commit-ID: 1oR3ssnlUyA

Comment 4

[Eric Rahm [:erahm]]

Attachment: Part 2: Use a DAFSA for kSTSPreloadList

This switches the STS preload list over to a more compact representation by
using a DAFSA. `getHSTSPreloadList.js` is updated to output data in the gperf
format expected by `make_dafsa.py`. We then add a generated file that gets
created by pumping `nsSTSPreloadList.inc` through `make_dafsa.py`.

`nsSiteSecurityService` is updated to use the DAFSA which either returns -1
(kNotFound) if an entry is not present or (0, 1) indicating whether or not to
use subdomains.

`nsSTSPreloadList.inc` is an automated conversion to the new gperf format.

MozReview-Commit-ID: ARKPJfD564T

Comment 5

[Eric Rahm [:erahm]]

Attachment: convert.cpp

Script I used to covert the inc file.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8895629 [details] [diff] [review]
Part 2: Use a DAFSA for kSTSPreloadList

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

::: security/manager/tools/getHSTSPreloadList.js
@@ +41,5 @@
>  "/* nsSiteSecurityService.cpp, you shouldn't be #including it.     */\n" +
>  "/*****************************************************************************/\n" +
>  "\n" +
> +"%%\n";
> +const FOOTER = "%%";

Sorry this file isn't quite tested yet, but the rest is okay to look over. I'll have a tested version tomorrow.

Comment 7

[Nathan Froyd [:froydnj]]

Comment on attachment 8895628 [details] [diff] [review]
Part 1: Handle gperf preamble in make_dafsa

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

This is fine, but the commit message should be modified to say something like "Handle gperf-like preamble in make_dafsa", because we're inspired by gperf, not blindly copying things from it.

Comment 8

[Eric Rahm [:erahm]]

Attachment: Part 2: Use a DAFSA for kSTSPreloadList

Updated patch with a getHSTSPreloadList.js that works. Tested by trimming down
the legacy inc file to the first 3 entries, hacking the getHosts function to
just return those 3 entries and confirmed the output format was correct. I have
not done a full run yet.

Confirmed that 'mach test security/manager/ssl/tests/unit' passes.

===============================================================================

This switches the STS preload list over to a more compact representation by
using a DAFSA. `getHSTSPreloadList.js` is updated to output data in the gperf
format expected by `make_dafsa.py`. We then add a generated file that gets
created by pumping `nsSTSPreloadList.inc` through `make_dafsa.py`.

`nsSiteSecurityService` is updated to use the DAFSA which either returns -1
(kNotFound) if an entry is not present or (0, 1) indicating whether or not to
use subdomains.

`nsSTSPreloadList.inc` is an automated conversion to the new gperf-like format.

Comment 9

[Eric Rahm [:erahm]]

Looks like ~232K size reduction of libxul.so.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> Comment on attachment 8895628 [details] [diff] [review]
> Part 1: Handle gperf preamble in make_dafsa
> 
> Review of attachment 8895628 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This is fine, but the commit message should be modified to say something
> like "Handle gperf-like preamble in make_dafsa", because we're inspired by
> gperf, not blindly copying things from it.

Updated and added an example of the input format and how it would be translated.

Comment 11

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8896012 [details] [diff] [review]
Part 2: Use a DAFSA for kSTSPreloadList

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

Cool! Looks good to me - just a couple of comments. In general, kNotFound should be Dafsa::kKeyNotFound, right?

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ +1373,5 @@
>  
> +// Indicates whether or not to include subdomains for the given host, if it
> +// exists. Only does exact host matching.
> +//
> +// Returns kNotFound if the host is not matched

Dafsa::kKeyNotFound?

@@ +1530,2 @@
>      SSSLOG(("%s is a preloaded HSTS host", aHost.get()));
> +    *aResult = aRequireIncludeSubdomains ? preload

I think it would be best to include a little helper that explicitly maps the value from the DAFSA to true/false (and asserts that it's not given Dafsa::kKeyNotFound or really anything other than 0 and 1).

::: security/manager/ssl/nsSiteSecurityService.h
@@ +208,5 @@
>                          bool aRequireIncludeSubdomains, uint32_t aFlags,
>                          const OriginAttributes& aOriginAttributes,
>                          bool* aResult, bool* aCached,
>                          SecurityPropertySource* aSource);
> +  int GetPreloadListEntry(const nsACString& aHost) const;

Maybe we should call this "GetPreloadStatus" or "GetPreloadedIncludeSubdomainValue" or something now?

Comment 12

[Eric Rahm [:erahm]]

Attachment: Part 2: Use a DAFSA for kSTSPreloadList

This just modifies nsSiteSecurityService with your suggestions. I went with
`GetPreloadStatus` and added an `aIncludeSubdomains` out param.

Comment 13

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8896042 [details] [diff] [review]
Part 2: Use a DAFSA for kSTSPreloadList

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

Cool - I like this solution.

::: security/manager/ssl/nsSiteSecurityService.cpp
@@ +1374,5 @@
> +// Checks if the given host is in the preload list.
> +//
> +// @param aHost The host to match. Only does exact host matching.
> +// @param aIncludeSubdomains Out, optional. Indicates whether or not to include
> +//        subdomains.

nit: let's also add that it is only set if the return value of the function is true

::: security/manager/ssl/nsSiteSecurityService.h
@@ +209,5 @@
>                          const OriginAttributes& aOriginAttributes,
>                          bool* aResult, bool* aCached,
>                          SecurityPropertySource* aSource);
> +  bool GetPreloadStatus(const nsACString& aHost,
> +                        bool* aIncludeSubdomains = nullptr) const;

nit: maybe annotate this "/*optional out*/"

Comment 14

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/29353fb6613bea80b8a320c1cf37f7cad7b537a1
Bug 1382001 - Part 1: Handle gperf-like preamble in make_dafsa. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd6770fadff3713fcdfb65e115f08e08cf50818e
Bug 1382001 - Part 2: Use a DAFSA for kSTSPreloadList. r=keeler

Comment 15

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe47b777980f814310837ad958e815cc8e17d84
Bug 1382001 - Part 3: Fix eslint. r=me

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

All three backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b89852598b1f at erahm's request.

Comment 17

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3ee86e160713756746d2fe12414f5e7f5576b4bf
Bug 1382001 - Part 1: Handle gperf-like preamble in make_dafsa. r=froydnj

https://hg.mozilla.org/integration/mozilla-inbound/rev/3849a13bc8ced0a25d30a466a4027d4934ab4941
Bug 1382001 - Part 2: Use a DAFSA for kSTSPreloadList. r=keeler

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/3ee86e160713
https://hg.mozilla.org/mozilla-central/rev/3849a13bc8ce

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/59594b3879cc
bustage fix after merge. r=merge a=merge