993186 - improve test_cert_eku/generate.py

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

In working on bug 991209, it was necessary to modify the generator for test_cert_eku.js (i.e. test_cert_eku/generate.py). However, I was unable to understand the generator well enough to make the necessary changes. Also, I came across a number of style issues that would be nice to fix for readability.

Comment 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch

I think what makes test_cert_eku/generate.py so confusing right now is that it handles both classic and mozilla::pkix verification (which is difficult, since they do differ significantly). This patch changes generate.py so it only handles mozilla::pkix, which makes it much simpler. Since we're moving away from classic verification, I think it's less important to support fully testing it. We can always file a follow-up bug to add back support to classic verification in another generator file.

Comment 2

[Camilo Viecco (:cviecco)]

Comment on attachment 8403006 [details] [diff] [review]
patch

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

While I will accept the removal of testing for the 'classic' code now, I think the code could be simpler if you have kept the nested tests.

::: security/manager/ssl/tests/unit/test_cert_eku/generate.py
@@ +69,2 @@
>  
>  function check_cert_err_generic(cert, expected_error, usage) {

Please remove this function and use instead checkCertErrorGeneric

@@ +78,5 @@
>  }
>  
> +function run_test() {
> +  load_cert("ca", "CT,CT,CT");
> +  Services.prefs.setBoolPref("security.use_mozillapkix_verification", true);

better yet, make it a conditional and fail if the pref is not true.

@@ -88,5 @@
> -  return check_cert_err_generic(cert, 0, certificateUsageSSLCA);
> -}
> -
> -function check_ok_ee(cert, usage) {
> -  return check_cert_err_generic(cert, 0, usage);

dont remove the OK ca and OK ee values, as it makes the decoding of successful values harder.

@@ +88,5 @@
> +def gen_int_js_output(int_string):
> +    error = "SEC_ERROR_INADEQUATE_CERT_TYPE"
> +    if (("NONE" in int_string or "SA" in int_string or "NS" in int_string) and
> +        "OS" not in int_string):
> +      error = "0"

I would have kept the  check_ok_ca function as it makes it clear that this case is successful.

@@ +107,2 @@
>  
> +def has_appropriate_usage(name_string, usage_abbreviation):

Please change name, this is more like 'has no EKU'

@@ +134,5 @@
> +        if ("SA" not in int_string and "NONE" not in int_string and
> +            "NS" not in int_string):
> +            return single_test_output(ee_name, cert_usage,
> +                                      "SEC_ERROR_INADEQUATE_CERT_TYPE")
> +        return single_test_output(ee_name, cert_usage, "0")

I would invert the logic here, if it is covered OK else INADEQUATE_CERT_TYPE,

if (SA or NONE OR NS in int_string) AND (SA or NONE in ee_string)
  return OK
else
  return fail.

An alternative would have been to keep my get_mozpkix_ee_error and get_int_error and return OK when both are 0 (usages are nested) and SEC_ERROR_INADEQUATE_CERT_TYPE otherwise. This would have elimnated the SSLserver logic and the logic below too.

Comment 3

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

(In reply to Camilo Viecco (:cviecco) from comment #2)
> @@ +78,5 @@
> >  }
> >  
> > +function run_test() {
> > +  load_cert("ca", "CT,CT,CT");
> > +  Services.prefs.setBoolPref("security.use_mozillapkix_verification", true);
> 
> better yet, make it a conditional and fail if the pref is not true.

Doesn't look like that works - I think because the pref that sets it to true is in firefox.js, which isn't read by xpcshell.

> @@ -88,5 @@
> > -  return check_cert_err_generic(cert, 0, certificateUsageSSLCA);
> > -}
> > -
> > -function check_ok_ee(cert, usage) {
> > -  return check_cert_err_generic(cert, 0, usage);
> 
> dont remove the OK ca and OK ee values, as it makes the decoding of
> successful values harder.

How so?

> @@ +88,5 @@
> > +def gen_int_js_output(int_string):
> > +    error = "SEC_ERROR_INADEQUATE_CERT_TYPE"
> > +    if (("NONE" in int_string or "SA" in int_string or "NS" in int_string) and
> > +        "OS" not in int_string):
> > +      error = "0"
> 
> I would have kept the  check_ok_ca function as it makes it clear that this
> case is successful.

How about renaming "error" to "expectedResult"?

> @@ +134,5 @@
> > +        if ("SA" not in int_string and "NONE" not in int_string and
> > +            "NS" not in int_string):
> > +            return single_test_output(ee_name, cert_usage,
> > +                                      "SEC_ERROR_INADEQUATE_CERT_TYPE")
> > +        return single_test_output(ee_name, cert_usage, "0")
> 
> I would invert the logic here, if it is covered OK else INADEQUATE_CERT_TYPE,
> 
> if (SA or NONE OR NS in int_string) AND (SA or NONE in ee_string)
>   return OK
> else
>   return fail.
> 
> An alternative would have been to keep my get_mozpkix_ee_error and
> get_int_error and return OK when both are 0 (usages are nested) and
> SEC_ERROR_INADEQUATE_CERT_TYPE otherwise. This would have elimnated the
> SSLserver logic and the logic below too.

That whole function is organized along the pattern "if (requirements not met): fail else: succeed", so I think inverting the logic of that one case would make it more confusing.

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v2

How does this look?

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: patch v3

Now with documentation!

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9ba4f44a828b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9ba4f44a828b