Closed Bug 1085074 Opened 6 years ago Closed 5 years ago

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

Categories

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

defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

Attachments

(3 files, 4 obsolete files)

(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.
Attached patch bug1085074_v0.patch (obsolete) — Splinter Review
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.
Assignee: nobody → cykesiopka.bmo
Status: NEW → ASSIGNED
Attachment #8507489 - Flags: feedback?(brian)
Depends on: 622859
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
Attachment #8507489 - Flags: feedback?(brian) → feedback+
(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.
Attached patch bug1085074_part1_v1.patch (obsolete) — Splinter Review
This patch makes the adequate/OK and inadequate/notOK changes everywhere except for the cert names.
Attachment #8507489 - Attachment is obsolete: true
Attached patch bug1085074_part2_v1.patch (obsolete) — Splinter Review
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.
Attachment #8510119 - Flags: review?(brian)
Attachment #8510122 - Flags: review?(brian)
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).
Attached patch bug1085074_part3_v1.patch (obsolete) — Splinter Review
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)
Attachment #8511485 - Flags: review?(brian)
Attachment #8511485 - Flags: review?(brian) → review+
Attachment #8510122 - Flags: review?(brian) → review+
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.
Attachment #8510119 - Flags: review?(brian) → review+
+ 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
Attachment #8510119 - Attachment is obsolete: true
Attachment #8533010 - Flags: review+
+ Switch from "ca" to "root" in any touched lines
Attachment #8510122 - Attachment is obsolete: true
Attachment #8533012 - Flags: review+
+ Rebase on top of various other changes to the tlsserver DBs
Attachment #8511485 - Attachment is obsolete: true
Attachment #8533013 - Flags: review+
Thanks for the review!

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=72341efca5bb
(The time outs and SM-r failure look unrelated)
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.