Closed Bug 1002696 Opened 10 years ago Closed 10 years ago

Factor shared code in genHPKPStaticPins.js and getHSTSPreloadList.js

Categories

(Core :: Security: PSM, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla32

People

(Reporter: mmc, Assigned: mmc)

References

Details

Attachments

(1 file, 1 obsolete file)

genHPKPStaticpins.js is not hooked up into the build anywhere, so static_pins.h will slowly go out of date, and we will not notice bitrot in the generator file.

This script should be integrated into http://mxr.mozilla.org/mozilla-central/source/security/manager/tools/getHSTSPreloadList.js which already takes care of this problem.
Blocks: 744204
No longer blocks: 772756, hpkp, 951315
We are going to keep these separate but they should share code as much as possible.
Summary: Integrate static_pins.h generation into the build infrastructure → Factor shared code in genHPKPStaticPins.js and getHSTSPreloadList.js
Assignee: nobody → mmc
Comment on attachment 8416128 [details] [diff] [review]
Minimum set of changes to make genHPKPStaticPins.js productionizable (

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

LGTM
Attachment #8416128 - Flags: review+
Attachment #8416128 - Flags: review?(dkeeler)
Comment on attachment 8416128 [details] [diff] [review]
Minimum set of changes to make genHPKPStaticPins.js productionizable (

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

Great - r=me with nits addressed.

::: security/manager/tools/genHPKPStaticPins.js
@@ +10,5 @@
> +//                                  [absolute path to]/PreloadedHPKPins.json \
> +//                                  [absolute path to]/default-ee.der \
> +//                                  [absolute path to]/StaticHPKPins.h
> +
> +if (arguments.length < 3) {

nit: arguments.length != 3?

@@ +33,5 @@
>  " * file, You can obtain one at http://mozilla.org/MPL/2.0/. */\n" +
>  "\n" +
>  "/*****************************************************************************/\n" +
>  "/* This is an automatically generated file. If you're not                    */\n" +
> +"/* PublicKeyPinningService.cpp, you shouldn't be #including it.               */\n" +

nit: one less space after "#including it." (so the end of the line aligns)

@@ +52,5 @@
>  "} StaticPinset;\n";
>  
> +// Command-line arguments
> +var gStaticPins = parseJson(arguments[0]);
> +var gTestDerFile = arguments[1];

maybe gTestCertFile?

@@ +100,5 @@
>  }
>  
>  // Returns a pair of maps [certNameToSKD, certSDKToName] between cert
>  // nicknames and digests of the SPKInfo for the mozilla trust store
> +function loadNSSCertinfo(testDerFile) {

We're missing an opportunity to have a German joke here ("derTestFile" :)

@@ +134,5 @@
>    return [certNameToSKD, certSDKToName];
>  }
>  
> +function parseJson(filename) {
> +  dump("parsing " + filename + "\n");

nit: not sure this dump is necessary

@@ +235,5 @@
>  }
>  
>  // ****************************************************************************
>  // This is where the action happens:
> +let [certNameToSKD, certSDKToName] = loadNSSCertinfo(gTestDerFile);

nit: while you're changing this line, it would be nice to have spaces after/before [/]
Attachment #8416128 - Flags: review?(dkeeler) → review+
Fixed and the tree is closed!

> We're missing an opportunity to have a German joke here ("derTestFile" :)

True story, I thought for the longest time that all of the crypto authors must be German (or German-philes) because of the naming conventions. D'oh!
Attachment #8416150 - Flags: review+
Attachment #8416150 - Flags: checkin+
Attachment #8416128 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/e4fe6d86d885
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.