Closed Bug 1290613 Opened 7 years ago Closed 7 years ago

clean up EV tests so additional testcases can be added more easily

Categories

(Core :: Security: PSM, defect, P1)

Product:
Core
Component:
Security: PSM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla51
Tracking Flags:
Tracking Status
firefox50 --- affected
firefox51 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(2 files)

bug 1290613 - remove unnecessary invalidIdentities parameter from startOCSPResponder
58 bytes, text/x-review-board-request
Cykesiopka
: review+
Details
bug 1290613 - test_ev_certs.js cleanup
58 bytes, text/x-review-board-request
Cykesiopka
: review+
mgoodwin
: review+
Details 
The EV tests in test_ev_certs.js have a few issues we should address. One is they synchronously verify certificates, causing networking to block the main thread (if we ever want to enforce that this doesn't happen, we have to fix all instances of this). Another is the testcases (and test files) are a bit confusingly named if we want to be able to add more testcases (such as with the CAB Forum EV OID - see bug 1243923). Also, the file can be cleaned up stylistically for ease of maintenance and continued development.
Comment on attachment 8784574 [details]
bug 1290613 - test_ev_certs.js cleanup

https://reviewboard.mozilla.org/r/73960/#review71952

Re. setting of security.onecrl.via.amo - this is correct for the purposes of this test but I think we should no longer be checking this pref when making a decision whether or not to bypass OCSP. I'll file a follow up.

::: security/manager/ssl/tests/unit/test_ev_certs/moz.build:7
(Diff revision 1)
>  # vim: set filetype=python:
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  # Temporarily disabled. See bug 1256495.

Do these still need to be disabled?
Attachment #8784574 - Flags: review?(mgoodwin) → review+
Comment on attachment 8784573 [details]
bug 1290613 - remove unnecessary invalidIdentities parameter from startOCSPResponder

https://reviewboard.mozilla.org/r/73958/#review72360

LGTM.
Attachment #8784573 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8784574 [details]
bug 1290613 - test_ev_certs.js cleanup

https://reviewboard.mozilla.org/r/73960/#review72362

Nice.

::: security/manager/ssl/tests/unit/test_ev_certs.js:43
(Diff revision 1)
> +    this.expectedEV = expectedEV;
> +    this.resolve = resolve;
> +    this.ocspResponder = ocspResponder;
> -}
> +  }
>  
> -function check_cert_err(cert_name, expected_error) {
> +  verifyCertFinished(aPRErrorCode, aVerifiedChain, aHasEVPolicy) {

Nit: We don't use the "a" prefix anywhere else in this file, so we probably shouldn't here either.

::: security/manager/ssl/tests/unit/test_ev_certs.js:55
(Diff revision 1)
>  }
>  
> -
> -function check_ee_for_ev(cert_name, expected_ev) {
> -  let cert = certdb.findCertByNickname(cert_name);
> -  checkEVStatus(certdb, cert, certificateUsageSSLServer, expected_ev);
> +function asyncTestEV(cert, expectedPRErrorCode, expectedEV,
> +                     expectedOCSPRequestPaths, ocspResponseTypes = undefined)
> +{
> +  let now = (new Date()).getTime() / 1000;

Nit: This can just be ```Date.now() / 1000```.
Same below.

::: security/manager/ssl/tests/unit/test_ev_certs.js:106
(Diff revision 1)
> -    ocspResponder.stop(run_next_test);
> +  let expectedErrorCode = expectSuccess
> +                        ? (gEVExpected ? PRErrorCodeSuccess
> +                                       : SEC_ERROR_POLICY_VALIDATION_FAILED)
> +                        : SEC_ERROR_POLICY_VALIDATION_FAILED;

ESLint is unhappy about the nested ternary here.
Let's rewrite this to not use a nested ternary, or just turn the rule off.

::: security/manager/ssl/tests/unit/test_ev_certs.js:164
(Diff revision 1)
> -    check_no_ocsp_requests("ev-valid", SEC_ERROR_POLICY_VALIDATION_FAILED);
> -  });
> +  return verifyWithDifferentOCSPResponseTypes(
> +    testcase, [ "good", "ancientstillvalid" ], false);
> +}
>  
> -  add_test(function () {
> -    check_no_ocsp_requests("non-ev-root", SEC_ERROR_POLICY_VALIDATION_FAILED);
> +// These should all verify as EV.
> +add_task(function* () {

If you feel like it, naming these functions produces more verbose output that can eliminate some of the ```do_print()```s, and makes debugging easier.

::: security/manager/ssl/tests/unit/test_ev_certs/no-ocsp-ee-path-int.pem.certspec:7
(Diff revision 1)
> +subject:no-ocsp-ee-path-int
> +issuerKey:ev
> +extension:basicConstraints:cA,
> +extension:keyUsage:cRLSign,keyCertSign
> +extension:authorityInformationAccess:http://www.example.com:8888/no-ocsp-ee-path-int/
> +extension:certificatePolicies:any

IIUC, this should be 1.3.6.1.4.1.13769.666.666.666.1.500.9.1 instead to mirror the "no-ocsp-int-path" case, and because we're not testing anyPolicy here.
Attachment #8784574 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8784574 [details]
bug 1290613 - test_ev_certs.js cleanup

https://reviewboard.mozilla.org/r/73960/#review71952

Ok - thanks.

> Do these still need to be disabled?

Yes, unfortunately.
Comment on attachment 8784574 [details]
bug 1290613 - test_ev_certs.js cleanup

https://reviewboard.mozilla.org/r/73960/#review72362

Thanks!

> Nit: We don't use the "a" prefix anywhere else in this file, so we probably shouldn't here either.

Ok - sounds good.

> Nit: This can just be ```Date.now() / 1000```.
> Same below.

Heh - I don't know why I keep forgetting this.

> ESLint is unhappy about the nested ternary here.
> Let's rewrite this to not use a nested ternary, or just turn the rule off.

Good call - that was just a mess.

> If you feel like it, naming these functions produces more verbose output that can eliminate some of the ```do_print()```s, and makes debugging easier.

Sounds good.

> IIUC, this should be 1.3.6.1.4.1.13769.666.666.666.1.500.9.1 instead to mirror the "no-ocsp-int-path" case, and because we're not testing anyPolicy here.

My intention was to have end-entity certificates have the test OID by default (unless testing the anyPolicy case) and have the intermediates have the anyPolicy OID by default (unless testing the test OID case), since that more closely reflects what we'll encounter in the wild. Then, as I wrote this, I realized I didn't quite follow that scheme in the patch, so I went back and fixed it (and added documentation).
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d465e3e3b6f2
remove unnecessary invalidIdentities parameter from startOCSPResponder r=Cykesiopka
https://hg.mozilla.org/integration/autoland/rev/fabfb2ff761e
test_ev_certs.js cleanup r=Cykesiopka,mgoodwin
https://hg.mozilla.org/mozilla-central/rev/d465e3e3b6f2
https://hg.mozilla.org/mozilla-central/rev/fabfb2ff761e
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.