Use an even more efficient format for kSTSPreloadList

RESOLVED FIXED in Firefox 57

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

Trunk
mozilla57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(3 attachments, 2 obsolete attachments)

+++ 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).
Whiteboard: [MemShrink] → [MemShrink:P3]
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
MozReview-Commit-ID: 1oR3ssnlUyA
Attachment #8895628 - Flags: review?(nfroyd)
Assignee: nobody → erahm
Status: NEW → ASSIGNED
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
Attachment #8895629 - Flags: review?(dkeeler)
Posted file convert.cpp
Script I used to covert the inc file.
Attachment #8895633 - Attachment mime type: text/x-c++src → text/plain
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 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.
Attachment #8895628 - Flags: review?(nfroyd) → review+
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.
Attachment #8896012 - Flags: review?(dkeeler)
Attachment #8895629 - Attachment is obsolete: true
Attachment #8895629 - Flags: review?(dkeeler)
Looks like ~232K size reduction of libxul.so.
(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 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?
Attachment #8896012 - Flags: review?(dkeeler) → review+
This just modifies nsSiteSecurityService with your suggestions. I went with
`GetPreloadStatus` and added an `aIncludeSubdomains` out param.
Attachment #8896042 - Flags: review?(dkeeler)
Attachment #8896012 - Attachment is obsolete: true
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*/"
Attachment #8896042 - Flags: review?(dkeeler) → review+
All three backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b89852598b1f at erahm's request.
Flags: needinfo?(erahm)
https://hg.mozilla.org/mozilla-central/rev/3ee86e160713
https://hg.mozilla.org/mozilla-central/rev/3849a13bc8ce
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/mozilla-central/rev/59594b3879cc
bustage fix after merge. r=merge a=merge
Flags: needinfo?(erahm)
You need to log in before you can comment on or make changes to this bug.