Closed Bug 1228794 Opened 4 years ago Closed 4 years ago

Convert test_getchain.js to generate certificates at build time

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(1 file, 1 obsolete file)

The certificates for test_getchain.js should be generated at build time for the reasons listed in Bug 1166976 comment 0.
Bug 1228794 - Convert test_getchain.js to generate certificates at build time.

With this change, CertUtils.py is no longer needed.
Attachment #8693448 - Flags: review?(dkeeler)
Comment on attachment 8693448 [details]
MozReview Request: Bug 1228794 - Convert test_getchain.js to generate certificates at build time.

https://reviewboard.mozilla.org/r/26455/#review24037

This looks good. See in particular the third comment, though.

::: security/manager/ssl/tests/unit/pycert.py:326
(Diff revision 1)
> +    """Takes a list of integers in the interval [0, 255] and returns

We should probably enforce that the length of serialBytes is <= 127.

::: security/manager/ssl/tests/unit/pycert.py:410
(Diff revision 1)
> +                raise UnknownSerialNumber(value)

I'm not sure that "unknown" is the best description for a serial number outside the supported range, but it's not a big deal.

::: security/manager/ssl/tests/unit/test_getchain.js:1
(Diff revision 1)
>  // -*- indent-tabs-mode: nil; js-indent-level: 2 -*-

The thing about this whole test is that it tests an API I think we should remove entirely (see bug 867473 and bug 1004580). That's why I initially considered it out of scope for bug 1174288).

I suppose it does test the case where we have two root CAs with the same subject DN but different trust settings. If we really wanted to test that, we should just use nsIX509CertDB.verifyCertNow directly instead of relying on the consistent-but-ultimately-undefined behavior of getChain/issuer.

I guess what I'm saying is if you feel like re-writing this test with that aim in mind, that would be cool. If you don't feel like it's worth the effort (and I'm not sure it is), then this is fine as-is.
https://reviewboard.mozilla.org/r/26455/#review24037

Thanks for the review!

> We should probably enforce that the length of serialBytes is <= 127.

Sure.

> I'm not sure that "unknown" is the best description for a serial number outside the supported range, but it's not a big deal.

Now that I revisit this, yes, it's a bit silly. I'll change the name to something like "InvalidSerialNumber".

> The thing about this whole test is that it tests an API I think we should remove entirely (see bug 867473 and bug 1004580). That's why I initially considered it out of scope for bug 1174288).
> 
> I suppose it does test the case where we have two root CAs with the same subject DN but different trust settings. If we really wanted to test that, we should just use nsIX509CertDB.verifyCertNow directly instead of relying on the consistent-but-ultimately-undefined behavior of getChain/issuer.
> 
> I guess what I'm saying is if you feel like re-writing this test with that aim in mind, that would be cool. If you don't feel like it's worth the effort (and I'm not sure it is), then this is fine as-is.

To be honest, I mainly converted this test just so I could get rid of CertUtils.py. I think I'll leave the test as-is - we can modify the test at a later point (maybe when we do rip out the API).
+ Enforce that the length of serialBytes is <= 127
+ Rename UnknownSerialNumber to more logical InvalidSerialNumber
Attachment #8693448 - Attachment is obsolete: true
Attachment #8694061 - Flags: review+
Try push is in comment 2.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5861b6467aa7
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.