1085074 - Use clearer terms to refer to adequate and inadequate sizes for key size tests

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

(Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from Bug 622859 Comment #21)
> Please document what "OK" means (2048 bit?) and what "Bad" means (1024-bit).
> It might be better to just use the terms "2048-bit" and "1024-bit" instead
> of "Good" and "Bad". In particular, it is confusing that a "Bad" root is
> still trusted for DV. Normally, we'd consider a root "bad" if we wouldn't
> trust it at all.

Comment 1

[:Cykesiopka]

Attachment: bug1085074_v0.patch

This patch switches all the key size tests and scripts to the following format:

Previous Terminology | New Long Terminology | New Short Terminology
-------------------------------------------------------------------
OK                   | Adequate             | OK
Bad                  | Inadequate           | Not OK

Hopefully this is more clear. If not, I'll make a patch that just uses the terms 1024-bit and 2048-bit.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8507489 [details] [diff] [review]
bug1085074_v0.patch

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

I suggest the following change: Instead of using "OK"/"NotOK" in the certificate names, instead name the certificates with the explicit algorithm and key size ("2048"/"1024"):

Old: rsa-eeNotOK-intOK-caOK.
New: ee_rsa_1024-int_rsa_2048-root_rsa_2048.der.

Rationale: "OK" and "NotOK" is not very useful information. Also, it isn't precise because I cannot tell which algorithms/sizes are being considered "OK" and which are being considered "Not OK". Also, this naming convention will extend well to tests for ECDSA

Comment 3

[:Cykesiopka]

(In reply to Brian Smith (:briansmith, :bsmith, use NEEDINFO?) from comment #2)
> I suggest the following change: Instead of using "OK"/"NotOK" in the
> certificate names, instead name the certificates with the explicit algorithm
> and key size ("2048"/"1024"):
> 
> Old: rsa-eeNotOK-intOK-caOK.
> New: ee_rsa_1024-int_rsa_2048-root_rsa_2048.der.
> 
> Rationale: "OK" and "NotOK" is not very useful information. Also, it isn't
> precise because I cannot tell which algorithms/sizes are being considered
> "OK" and which are being considered "Not OK". Also, this naming convention
> will extend well to tests for ECDSA

SGTM, I'll make the change.

Comment 4

[:Cykesiopka]

Attachment: bug1085074_part1_v1.patch

This patch makes the adequate/OK and inadequate/notOK changes everywhere except for the cert names.

Comment 5

[:Cykesiopka]

Attachment: bug1085074_part2_v1.patch

This patch changes cert file names to the scheme described in Comment 2.

I went with a more flexible approach, but this does sacrifice readability.

Comment 6

[:Cykesiopka]

Comment on attachment 8510122 [details] [diff] [review]
bug1085074_part2_v1.patch

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

::: security/manager/ssl/tests/unit/test_keysize_ev.js
@@ +98,2 @@
>    // Reuse the existing test RSA EV root
> +  let rootCAOKCertFileName = keyType == "rsa"

Maybe this method should just become RSA specific to make it a bit cleaner - I'm not entirely sure this method extends well/at all to ECDSA (I probably should've thought ahead in Bug 622859 - oh well).

Comment 7

[:Cykesiopka]

Attachment: bug1085074_part3_v1.patch

This patch updates the OCSP Delegated Signer cert I missed in Part 2.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7848180294ab (not sure what is causing the M-oth failure - maybe Bug 552291)

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8510119 [details] [diff] [review]
bug1085074_part1_v1.patch

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

Sorry for the delay. Good work on this! Thanks for doing it.

::: security/manager/ssl/tests/unit/test_keysize.js
@@ +58,5 @@
>    check_ok_ca(load_cert(key_type + "-caOK", "CTu,CTu,CTu"));
>    check_ok_ca(load_cert(key_type + "-intOK-caOK", ",,"));
>    check_ok(certFromFile(key_type + "-eeOK-intOK-caOK.der"));
>  
> +  // Chain with a CA cert that has an inadequate size for DV

s/CA cert/root cert/

::: security/manager/ssl/tests/unit/test_keysize/generate.py
@@ +173,5 @@
>          key_type,
>          '-intOK-caBad',
>          ca_ext_text,
> +        caNotOK_key,
> +        caNotOK_cert,

Please use "root" instead of "ca" to refer to roots, here and elsewhere.

Comment 9

[:Cykesiopka]

Attachment: bug1085074_part1_switch-terminology-except-cert-names_v2.patch

+ Switch from "ca" to "root" in any touched lines (I'll submit a follow up to convert the rest later)
+ Fix a typo in the test_keysize_ev.js checkForKeyType() inadequate root cert case comment

Comment 10

[:Cykesiopka]

Attachment: bug1085074_part2_use-bit-sizes-cert-names_v2.patch

+ Switch from "ca" to "root" in any touched lines

Comment 11

[:Cykesiopka]

Attachment: bug1085074_part3_delegated-signer_v2.patch

+ Rebase on top of various other changes to the tlsserver DBs

Comment 12

[:Cykesiopka]

Thanks for the review!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=72341efca5bb
(The time outs and SM-r failure look unrelated)

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/903c47c8b078
https://hg.mozilla.org/integration/mozilla-inbound/rev/2157e6c5c069
https://hg.mozilla.org/integration/mozilla-inbound/rev/734d3bef2554

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/903c47c8b078
https://hg.mozilla.org/mozilla-central/rev/2157e6c5c069
https://hg.mozilla.org/mozilla-central/rev/734d3bef2554