Closed Bug 971178 Opened 6 years ago Closed 6 years ago

Expand tests of core certificate verification logic to test insanity::pkix

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(4 files)

No description provided.
Summary: Update tests of core certificate verification logic to test insanity::pkix → Expand tests of core certificate verification logic to test insanity::pkix
Comment on attachment 8374338 [details] [diff] [review]
Part 1: Expand test_cert_signatures.js to test insanity::pkix

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

r+ with comment addressed.

::: security/manager/ssl/tests/unit/test_cert_signatures.js
@@ +48,5 @@
> +  run_test_in_mode(true);
> +  run_test_in_mode(false);
> +}
> +
> +function run_test_in_mode(useInsanity) {

This name is kind of unfortunate.

@@ +65,5 @@
> +                : 'Client,Server,Sign,Encrypt,SSL CA,Status Responder';
> +
> +  // insanity::pkix doesn't implement the Netscape Object Signer restriction.
> +  const ee_usage = 'Client,Server,Sign,Encrypt' + 
> +                   (useInsanity ? ',Object Signer' : '');

change this to be as the int_usage section (readability):

let ee_usage = useInsanity
               ? 'Client,Server,Sign,Encrypt,ObjectSigner'
               : 'Client,Server,Sign,Encrypt';
Attachment #8374338 - Flags: review?(cviecco) → review+
Comment on attachment 8374339 [details]
Part 2: Expand test_getchains.js to test insanity::pkix

Again there is an unfortunate name for the switch test function. test_with_insanity?
Attachment #8374339 - Flags: review?(cviecco) → review+
Attachment #8374393 - Attachment is patch: true
Attachment #8374393 - Attachment mime type: application/javascript → text/plain
Comment on attachment 8374393 [details] [diff] [review]
Part 3: Expand test_certificate_usages.js to test insanity::pkix.

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

r+ with comments addressed.

::: security/manager/ssl/tests/unit/test_certificate_usages.js
@@ +51,5 @@
> +                   'SSL CA',
> +                   allCAUsages,
> +                   useInsanity ? ''
> +                               : 'Client,Server,Sign,Encrypt,Status Responder'];
> +    

nit space.

@@ +52,5 @@
> +                   allCAUsages,
> +                   useInsanity ? ''
> +                               : 'Client,Server,Sign,Encrypt,Status Responder'];
> +    
> +  // insanity::pkix doesn't implement the Netscape Object Signer restriction.

this is correct, but hard to read. do as you did for allCAUsages
Attachment #8374393 - Flags: review?(cviecco) → review+
Attachment #8374393 - Attachment description: expand-test_certificate_usages.js → Part 3: Expand test_certificate_usages.js to test insanity::pkix.
Regarding the "run_test_in_mode" name: I agree it isn't great, but I think it is good enough. We understand what it means and there's a lot of other things to improve, even within our tests. So, I suggest we just let that go.
Comment on attachment 8374518 [details] [diff] [review]
Part 4: Expand test_intermediate_basic_usage_constraints.js to test insanity::pkix

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

::: security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints.js
@@ +1,4 @@
>  "use strict";
>  /* To regenerate the certificates for this test:
>   *
> + *      cd security/manager/ssl/tests/unit/test_intermediate_basic_usage_constraints

I am assuming you copied the file. The diff does not make that clear

@@ +21,5 @@
>  }
>  
> +function test_cert_for_usages(certChainNicks, expected_usages_string) {
> +  let certs = [];
> +  for (let i in certChainNicks) {

I dont like the idea of requiring at test time to know potential dependencies. But will allow.
Attachment #8374518 - Flags: review?(cviecco) → review+
(In reply to Camilo Viecco (:cviecco) from comment #9)
> I am assuming you copied the file. The diff does not make that clear

I'm not sure what you mean? I didn't really change anything with how the tests are generated. Did I make a mistaking with my change to the comment?

> I dont like the idea of requiring at test time to know potential
> dependencies. But will allow.

I don't quite understand what you mean here. Let's talk about it tomorrow. Things can be changed afterward if it is helpful to do so.

Thanks for doing these reviews so quickly!

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca16447717f3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7a95b0132c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5eaddcb75da
https://hg.mozilla.org/integration/mozilla-inbound/rev/da8f41eef4ba
You need to log in before you can comment on or make changes to this bug.