Switch EV tests to SQL DB and partially clean up scripts

RESOLVED FIXED in mozilla36

Status

()

defect
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Tracking

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

No description provided.
Posted patch bug1089305_v0.patch (obsolete) — Splinter Review
This patch:
- Switches EV cert testing to the SQL DB
- Replaces generate_root_cert() with a CertUtils.generate_cert_generic() call (the code is nearly identical)
- Removes some pexpect uses (see Bug 969931 for why) and other unused imports
- Adds a call to print out information needed to enable EV for the generated root
- Gets rid of unneeded .p12 files

Asking for feedback for now, in case cert8 + key3 tests need to be kept around. If that's the case, I'll morph the bug as appropriate.

(I didn't include a regenerated root in this patch since it didn't seem necessary, but I can do so if wanted. Both the current and a regenerated root passed tests for me.)

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b1472c02ea5c
Attachment #8511596 - Flags: feedback?(dkeeler)
Comment on attachment 8511596 [details] [diff] [review]
bug1089305_v0.patch

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

This looks great. (Although, just to confirm - the modified certs in this patch are still signed by the old EV root as appropriate, right?)
We shouldn't need to keep cert8.db/key3.db around. It looks like we're moving towards using the SQL databases everywhere in production anyway.

::: security/manager/ssl/tests/unit/test_ev_certs/ev_root_generate.py
@@ +2,5 @@
>  
> +# After running this file you MUST update the compiled test EV root information
> +# to match the information of the root generated by this script. In addition,
> +# certs that chain up to this root in other folders will also need to be
> +# regenerated.

Hmmm - do we know exactly which certs those are? (Are those the certs generated by generate.py?) It would be helpful to list them here (or where they're likely to be, or something).

::: security/manager/ssl/tests/unit/test_ev_certs/generate.py
@@ +132,4 @@
>      import_untrusted_cert(ee_cert, prefix)
>  
>  
>  

Please remove the two extra unnecessary blank lines here (I guess pep 8 might say there's only 1 unnecessary line, so your call).
Attachment #8511596 - Flags: feedback?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #2)
> This looks great. (Although, just to confirm - the modified certs in this
> patch are still signed by the old EV root as appropriate, right?)

Yes.

> Hmmm - do we know exactly which certs those are? (Are those the certs
> generated by generate.py?) It would be helpful to list them here (or where
> they're likely to be, or something).

As discussed over IRC: the cert in question is evroot.der. Since the exact same message and the information required to enable EV for the root is printed by the script anyways, I'm just going to remove the comment.
Posted patch bug1089305_v1.patch (obsolete) — Splinter Review
Address points from Comment :
- Revert comment change at top of file and instead just remove the comment
+ Remove extra lines
Attachment #8511596 - Attachment is obsolete: true
Attachment #8513663 - Flags: review+
Thanks for the review.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=f28a06d69afa (OSX 10.8 tests are still pending 14 hours after I pushed to try, so I'm not going to bother waiting any longer.)
Keywords: checkin-needed
sorry had to back this out since we were seeing frequent android and b2g xpcshell test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3433085&repo=mozilla-inbound after this landing
Flags: needinfo?(cykesiopka.bmo)
That test's xpcshell.ini says this:

71 [test_ev_certs.js]
72 run-sequentially = hardcoded ports
73 # Bug 1009158: this test times out on Android
74 # Bug 1008316: Test needs modification to work on B2G
75 fail-if = os == "android" || buildapp == "b2g"

And now we appear to have this on b2g:

05:40:16 WARNING - TEST-UNEXPECTED-PASS | /builds/slave/test/build/tests/xpcshell/tests/security/manager/ssl/tests/unit/test_ev_certs.js | xpcshell return code: 0

That last line should probably be skip-if = os == "android", unless now it passes on Android as well, which would surprise me, since we're using the OCSP responder and I didn't think any of those tests worked on Android. It's probably worth trying to remove that line altogether, and maybe some of the related skip-if lines, if those tests now work on Android.
(In reply to Carsten Book [:Tomcat] from comment #7)
> sorry had to back this out since we were seeing frequent android and b2g
> xpcshell test failures like
> https://treeherder.mozilla.org/ui/logviewer.
> html#?job_id=3433085&repo=mozilla-inbound after this landing

(In reply to David Keeler (:keeler) [use needinfo?] from comment #8)
> That test's xpcshell.ini says this:
> 
> 71 [test_ev_certs.js]
> 72 run-sequentially = hardcoded ports
> 73 # Bug 1009158: this test times out on Android
> 74 # Bug 1008316: Test needs modification to work on B2G
> 75 fail-if = os == "android" || buildapp == "b2g"
> 
> And now we appear to have this on b2g:
> 
> 05:40:16 WARNING - TEST-UNEXPECTED-PASS |
> /builds/slave/test/build/tests/xpcshell/tests/security/manager/ssl/tests/
> unit/test_ev_certs.js | xpcshell return code: 0
> 
> That last line should probably be skip-if = os == "android", unless now it
> passes on Android as well, which would surprise me, since we're using the
> OCSP responder and I didn't think any of those tests worked on Android. It's
> probably worth trying to remove that line altogether, and maybe some of the
> related skip-if lines, if those tests now work on Android.

Ah, sorry about that! I thought this line said skip-if, which is why I didn't bother testing on Android or B2G. I'll investigate and see if this test now works properly on Android and B2G.
Flags: needinfo?(cykesiopka.bmo)
(The fix in Bug 1079436 should hopefully reduce/fix the intermittent OCSP failures, so I will wait until it the fix is in before I test Android and B2G.)
Depends on: 1079436
(In reply to Cykesiopka from comment #10)
> (The fix in Bug 1079436 should hopefully reduce/fix the intermittent OCSP
> failures, so I will wait until it the fix is in before I test Android and
> B2G.)

After looking at the OrangeFactor page, it turns out this test doesn't fail very often in general, so removing the dependency.
No longer depends on: 1079436
Posted patch bug1089305_v2.patch (obsolete) — Splinter Review
Based on findings from:
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=30577c552814
  https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=3b94081e0d3a

+ Change test_ev_certs.js to handle lack of EV on B2G
+ Remove fail-if line entirely
Attachment #8513663 - Attachment is obsolete: true
Attachment #8523384 - Flags: review?(dkeeler)
Bugzilla interdiff works as well, but here's the v1->v2 interdiff.
Comment on attachment 8523384 [details] [diff] [review]
bug1089305_v2.patch

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

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
> +// The test EV roots are only enabled in debug builds as a security measure.
> +//
> +// Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
> +// builds.
> +const gEVExpected = isDebugBuild && !("@mozilla.org/b2g-process-global;1" in Cc);

I put this here instead of in test_ev_certs.js because I hope to re-use it for test_keysize_ev.js in the near future.
Comment on attachment 8523385 [details] [diff] [review]
bug1089305_v1-v2_interdiff.patch

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

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
> +// The test EV roots are only enabled in debug builds as a security measure.
> +//
> +// Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
> +// builds.
> +const gEVExpected = isDebugBuild && !("@mozilla.org/b2g-process-global;1" in Cc);

We only need gEVExpected in test_ev_certs.js, right? Let's keep it there until it's also needed elsewhere.

::: security/manager/ssl/tests/unit/xpcshell.ini
@@ -69,5 @@
>  skip-if = os == "android" || os == "win"
>  [test_cert_signatures.js]
>  [test_ev_certs.js]
>  run-sequentially = hardcoded ports
> -# Bug 1009158: this test times out on Android

If this test doesn't time out on Android, then it's likely that other tests that only use GenerateOCSPResponse also work on Android. It would be great to identify and re-enable them.
Comment on attachment 8523384 [details] [diff] [review]
bug1089305_v2.patch

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

Great - r=me with comments addressed.

::: security/manager/ssl/tests/unit/head_psm.js
@@ +21,5 @@
> +// The test EV roots are only enabled in debug builds as a security measure.
> +//
> +// Bug 1008316: B2G doesn't have EV enabled, so EV is not expected even in debug
> +// builds.
> +const gEVExpected = isDebugBuild && !("@mozilla.org/b2g-process-global;1" in Cc);

Oh, I see. I think in general it's best to make these changes exactly when they're necessary (because, for instance, if we don't get around to adding that test, then this is here for no particularly good reason).
Attachment #8523384 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) [use needinfo?] from comment #15)
> If this test doesn't time out on Android, then it's likely that other tests
> that only use GenerateOCSPResponse also work on Android. It would be great
> to identify and re-enable them.

Sure - I'll see if they can be re-enabled.

(In reply to David Keeler (:keeler) [use needinfo?] from comment #16)
> Oh, I see. I think in general it's best to make these changes exactly when
> they're necessary (because, for instance, if we don't get around to adding
> that test, then this is here for no particularly good reason).

Noted. I'll move the line to test_ev_certs.js as well.
(In reply to Cykesiopka from comment #17)
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #15)
> > If this test doesn't time out on Android, then it's likely that other tests
> > that only use GenerateOCSPResponse also work on Android. It would be great
> > to identify and re-enable them.
> 
> Sure - I'll see if they can be re-enabled.

Thanks (and by the way, that doesn't need to happen in/for this bug - that can be in another bug).

> 
> (In reply to David Keeler (:keeler) [use needinfo?] from comment #16)
> > Oh, I see. I think in general it's best to make these changes exactly when
> > they're necessary (because, for instance, if we don't get around to adding
> > that test, then this is here for no particularly good reason).
> 
> Noted. I'll move the line to test_ev_certs.js as well.

Thanks.
+ Move gEVExpected to test_ev_certs.js

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4eb09ff0beff
Attachment #8523384 - Attachment is obsolete: true
Attachment #8523385 - Attachment is obsolete: true
Attachment #8524296 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a02b29bc7cac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.