1089305 - Switch EV tests to SQL DB and partially clean up scripts

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[:Cykesiopka]

Attachment: bug1089305_v0.patch

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

Comment 2

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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).

Comment 3

[:Cykesiopka]

(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.

Comment 4

[:Cykesiopka]

Attachment: bug1089305_v1.patch

Address points from Comment :
- Revert comment change at top of file and instead just remove the comment
+ Remove extra lines

Comment 5

[:Cykesiopka]

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.)

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b4665be856d7

Comment 7

[Carsten Book [:Tomcat]]

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

Comment 8

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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.

Comment 9

[:Cykesiopka]

(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.

Comment 10

[:Cykesiopka]

(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.)

Comment 11

[:Cykesiopka]

(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.

Comment 12

[:Cykesiopka]

Attachment: bug1089305_v2.patch

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

Comment 13

[:Cykesiopka]

Attachment: bug1089305_v1-v2_interdiff.patch

Bugzilla interdiff works as well, but here's the v1->v2 interdiff.

Comment 14

[:Cykesiopka]

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 15

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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 16

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

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).

Comment 17

[:Cykesiopka]

(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.

Comment 18

[Dana Keeler (she/her) (use needinfo) (:keeler for reviews)]

(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.

Comment 19

[:Cykesiopka]

Attachment: bug1089305_v3.patch

+ Move gEVExpected to test_ev_certs.js

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=4eb09ff0beff

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a02b29bc7cac

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a02b29bc7cac