Modify getHSTSPreloadList.js and genHPKPStaticPins.js to generate reports on what data resulted in the generated file

NEW
Assigned to

Status

()

Core
Security: PSM
P1
enhancement
8 months ago
4 months ago

People

(Reporter: mgoodwin, Assigned: mgoodwin)

Tracking

({stale-bug})

unspecified
stale-bug
Points:
---

Firefox Tracking Flags

(firefox57 wontfix)

Details

(Whiteboard: [psm-assigned])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

8 months ago
The data in the generated preload lists are useful for other uses (e.g. managing entries and their version applicability for the dynamic preload mechanisms). In addition, it would be useful (particularly in the case of genHPKPStaticPins.js) to see where an entry has come from (Mozilla's data or data from Google).
Comment hidden (mozreview-request)

Comment 2

8 months ago
mozreview-review
Comment on attachment 8882630 [details]
Bug 1377477 - Modify getHSTSPreloadList.js and genHPKPStaticPins.js to generate reports on what data resulted in the generated file ()

https://reviewboard.mozilla.org/r/153712/#review159988

Looks good in general, but I think we can simplify a few things. Also, some formatting nits (e.g. long lines).

::: security/manager/tools/genHPKPStaticPins.js:13
(Diff revision 1)
>  // 3. run `[path to]/run-mozilla.sh [path to]/xpcshell \
>  //                                  [path to]/genHPKPStaticpins.js \
>  //                                  [absolute path to]/PreloadedHPKPins.json \
>  //                                  [an unused argument - see bug 1205406] \
> -//                                  [absolute path to]/StaticHPKPins.h
> +//                                  [absolute path to]/StaticHPKPins.h \
> +//                                  [absolute path to]/StaticHPKPData.json \

We should probably include some documentation on what StatisHPKPData.json is (although I realize none of this is documented that well...)

::: security/manager/tools/genHPKPStaticPins.js:23
(Diff revision 1)
>    throw new Error("Usage: genHPKPStaticPins.js " +
>                    "<absolute path to PreloadedHPKPins.json> " +
>                    "<an unused argument - see bug 1205406> " +
> -                  "<absolute path to StaticHPKPins.h>");
> +                  "<absolute path to StaticHPKPins.h>" +
> +                  "(optional) <absolute path to StaticHPKPData.json>" +
> +                  "(optional) <absolute path to version.txt");

I think we can use AppConstants.MOZ_APP_VERSION: https://dxr.mozilla.org/mozilla-central/rev/211d4dd61025c0a40caea7a54c9066e051bdde8c/toolkit/modules/AppConstants.jsm#323

::: security/manager/tools/genHPKPStaticPins.js:80
(Diff revision 1)
>  // Open the output file.
>  var file = Cc["@mozilla.org/file/local;1"].createInstance(Ci.nsILocalFile);
>  file.initWithPath(arguments[2]);
>  var gFileOutputStream = FileUtils.openSafeFileOutputStream(file);
>  
> +let gJsonFileOutputStream;

nit: in general, 'var' instead of 'let' for global-scope declarations, but in particular for these I don't think they need to be global - just declare them in writeFile and pass jsonReportData to writeDomainList as necessary?

::: security/manager/tools/genHPKPStaticPins.js:92
(Diff revision 1)
> +let gVersion = "unknown";
> +if (arguments.length > 4) {
> +  gVersion = readFileToString(arguments[4]).trim();
> +}
> +
> +let gJSONReportData = {"fingerprints": [],

nit: let's have 'fingerprints' on the next line (I couldn't find it initially when I saw it referenced elsewhere)

::: security/manager/tools/genHPKPStaticPins.js:609
(Diff revision 1)
>      if (mozillaPins[key]) {
>        dump("Skipping duplicate pinset " + key + "\n");
>      } else {
>        dump("Writing pinset " + key + "\n");
>        writeFullPinset(certNameToSKD, certSKDToName, chromeImportedPinsets[key]);
> +      gJSONReportData.pinsets.push({"source": "Chrome static pinset", "name": chromeImportedPinsets[key].name, "hashes": chromeImportedPinsets[key].sha256_hashes});

nit: break up long lines

::: security/manager/tools/genHPKPStaticPins.js:645
(Diff revision 1)
>  
>  writeFile(certNameToSKD, certSKDToName, chromeImportedPinsets,
>            chromeImportedEntries);
>  
>  FileUtils.closeSafeFileOutputStream(gFileOutputStream);
> +if (gJsonFileOutputStream) {

Seems like this can go in writeFile.

::: security/manager/tools/getHSTSPreloadList.js:12
(Diff revision 1)
>  // 1. [obtain firefox source code]
>  // 2. [build/obtain firefox binaries]
>  // 3. run `[path to]/run-mozilla.sh [path to]/xpcshell \
>  //                                  [path to]/getHSTSPreloadlist.js \
> -//                                  [absolute path to]/nsSTSPreloadlist.inc'
> +//                                  [absolute path to]/nsSTSPreloadlist.inc' \
> +//                                  [absolute path to]/StaticHSTSData.json

This is optional, right? (also, documentation)

::: security/manager/tools/getHSTSPreloadList.js:268
(Diff revision 1)
>      writeTo(HEADER, fos);
> -    writeTo(getExpirationTimeString(), fos);
> +    var expirationTime = getExpirationTime();
> +    writeTo(getExpirationTimeString(expirationTime), fos);
> +
> +    let reportData = {
> +      "statuses":[],

nit: space after ':'

::: security/manager/tools/getHSTSPreloadList.js:502
(Diff revision 1)
>  }
>  // get the current preload list
>  var currentHosts = readCurrentList(arguments[0]);
> +// get the filename for the JSON report
> +if (arguments.length > 1) {
> +  gFilename = arguments[1];

I think we can avoid adding more globals here - let's just pass this to output (and writeEntry).
Attachment #8882630 - Flags: review?(dkeeler)
Priority: -- → P1
Whiteboard: [psm-assigned]
This is an assigned P1 bug without activity in two weeks. 

If you intend to continue working on this bug for the current release/iteration/sprint, remove the 'stale-bug' keyword.

Otherwise we'll reset the priority of the bug back to '--' on Monday, August 28th.
Keywords: stale-bug

Updated

5 months ago
Severity: normal → enhancement

Updated

4 months ago
status-firefox57: --- → wontfix
You need to log in before you can comment on or make changes to this bug.