Closed
Bug 1382001
Opened 7 years ago
Closed 7 years ago
Use an even more efficient format for kSTSPreloadList
Categories
(Core :: Security: PSM, enhancement)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: erahm, Assigned: erahm)
References
Details
(Whiteboard: [MemShrink:P3])
Attachments
(3 files, 2 obsolete files)
2.87 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
480 bytes,
text/plain
|
Details | |
3.04 MB,
patch
|
keeler
:
review+
|
Details | Diff | Splinter Review |
+++ 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).
Assignee | ||
Updated•7 years ago
|
Whiteboard: [MemShrink] → [MemShrink:P3]
Assignee | ||
Comment 1•7 years ago
|
||
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
Assignee | ||
Comment 3•7 years ago
|
||
MozReview-Commit-ID: 1oR3ssnlUyA
Attachment #8895628 -
Flags: review?(nfroyd)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
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)
Assignee | ||
Comment 5•7 years ago
|
||
Script I used to covert the inc file.
Assignee | ||
Updated•7 years ago
|
Attachment #8895633 -
Attachment mime type: text/x-c++src → text/plain
Assignee | ||
Comment 6•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 8•7 years ago
|
||
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)
Assignee | ||
Updated•7 years ago
|
Attachment #8895629 -
Attachment is obsolete: true
Attachment #8895629 -
Flags: review?(dkeeler)
Assignee | ||
Comment 9•7 years ago
|
||
Looks like ~232K size reduction of libxul.so.
Assignee | ||
Comment 10•7 years ago
|
||
(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+
Assignee | ||
Comment 12•7 years ago
|
||
This just modifies nsSiteSecurityService with your suggestions. I went with `GetPreloadStatus` and added an `aIncludeSubdomains` out param.
Attachment #8896042 -
Flags: review?(dkeeler)
Assignee | ||
Updated•7 years ago
|
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+
Assignee | ||
Comment 14•7 years ago
|
||
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
Assignee | ||
Comment 15•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/1fe47b777980f814310837ad958e815cc8e17d84 Bug 1382001 - Part 3: Fix eslint. r=me
All three backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b89852598b1f at erahm's request.
Flags: needinfo?(erahm)
Assignee | ||
Comment 17•7 years ago
|
||
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•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/3ee86e160713 https://hg.mozilla.org/mozilla-central/rev/3849a13bc8ce
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 19•7 years ago
|
||
Pushed by archaeopteryx@coole-files.de: https://hg.mozilla.org/mozilla-central/rev/59594b3879cc bustage fix after merge. r=merge a=merge
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(erahm)
You need to log in
before you can comment on or make changes to this bug.
Description
•