Closed Bug 1173565 Opened 5 years ago Closed 5 years ago

convert test_pinning_dynamic.js and test_pinning_header_parsing.js to generate certificates at build time

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(1 file)

Same deal as bug 1166976, bug 1171557, and bug 1171819. test_pinning_dynamic/ has a few certificates we should convert to the new system. test_pinning_header_parsing.js uses certificates in test_pinning_dynamic/, so it has to be modified slightly as well.
bug 1173565 - convert test_pinning_dynamic.js to generate certificates at build time

Also fixes up references to test_pinning_dynamic certificates in test_pinning_header_parsing.js
Attachment #8621732 - Flags: review?(cykesiopka.bmo)
Comment on attachment 8621732 [details]
MozReview Request: bug 1173565 - convert test_pinning_dynamic.js to generate certificates at build time

https://reviewboard.mozilla.org/r/11035/#review9705

r+ with comments addressed.

::: security/manager/ssl/tests/unit/pycert.py:27
(Diff revision 1)
>  

subjectAlternativeName should be documented as a known extension.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:38
(Diff revision 1)
> -const PINNING_ROOT_KEY_HASH ="kXoHD1ZGyMuowchJwy+xgHlzh0kJFoI9KX0o0IrzTps=";
> +const PINNING_ROOT_KEY_HASH ="VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=";

Nit: Add space between = and "

::: security/manager/ssl/tests/unit/test_pinning_header_parsing.js:80
(Diff revision 1)
> -const PINNING_ROOT_KEY_HASH ="kXoHD1ZGyMuowchJwy+xgHlzh0kJFoI9KX0o0IrzTps=";
> +const PINNING_ROOT_KEY_HASH ="VCIlmPM9NkgFQtrs4Oa5TeFcDu6MWRTKSNdePEhOgD8=";

Nit: Add space between = and "

::: security/manager/ssl/tests/unit/test_pinning_dynamic/cn-a.pinning2.example.com-pinningroot.pem.certspec:2
(Diff revision 1)
> +subject:a.pinning2.example.com

Looks like this should have "subjectKey:alternate" as well?

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:52
(Diff revision 1)
> -  writeLine("b.pinning2.example.com:HPKP\t0\t0\t" + (now + 100000) + ",1,1,kXoHD1ZGyMuowchJwy+xgHlzh0kJFoI9KX0o0IrzTps=\n", outputStream);
> +  writeLine("b.pinning2.example.com:HPKP\t0\t0\t" + (now + 100000) + ",1,1," + PINNING_ROOT_KEY_HASH + "\n", outputStream);

Optional: these lines might look nicer using template strings.

::: security/manager/ssl/tests/unit/test_pinning_dynamic.js:96
(Diff revision 1)
> -  checkFail(certFromFile('cn-a.pinning2.example.com-badca.der'), "a.pinning2.example.com");
> +  checkFail(certFromFile('cn-a.pinning2.example.com-badca.pem'), "a.pinning2.example.com");

Optional: Append ".pem" in certFromFile() and loadCert() instead of repeating it here and below.

If the certs switch formats again some time in the future, hopefully less changes will be needed.

If not, it makes the lines shorter at least.
Attachment #8621732 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/11035/#review9765

Thanks for the review!

> Optional: Append ".pem" in certFromFile() and loadCert() instead of repeating it here and below.
> 
> If the certs switch formats again some time in the future, hopefully less changes will be needed.
> 
> If not, it makes the lines shorter at least.

Sounds like a plan.

> Looks like this should have "subjectKey:alternate" as well?

Good catch.
https://hg.mozilla.org/mozilla-central/rev/6c03a601e58e
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.