Closed Bug 1190603 Opened 4 years ago Closed 4 years ago

convert test_keysize.js to generate certificates at build time

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox42 --- affected
firefox43 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(3 files)

See bug 1174288. Since test_keysize.js involves some ECDSA certificates, we'll need a library that does that. https://github.com/amintos/PyECC is MIT-licensed and appears to be suitable for our purposes.
bug 1190603 - convert test_keysize.js to generate certificates at build time
Attachment #8646595 - Flags: review?(cykesiopka.bmo)
bug 1190603 - rename prime256v1 to secp256r1 in test_keysize.js to reduce confusion

OpenSSL refers to the curve in question as 'prime256v1', but rfc 5480,
mozilla::pkix, and the test framework refer to it as secp256r1, so we
should be consistent.
Attachment #8646596 - Flags: review?(cykesiopka.bmo)
r? to :gerv for the license on the new library (it's MIT)
r? to :gps for adding the new library
For the second changeset, it will conflict with bug 1183718, but since that's temporarily on hold, I figured I'd move ahead with these changes and we'll resolve the conflict later.
Assignee: nobody → dkeeler
Comment on attachment 8646594 [details]
MozReview Request: bug 1190603 - import PyECC library

https://reviewboard.mozilla.org/r/15173/#review14101

Please update the commit message to include exactly how these files were obtained. Since it looks like it was from a Git repo, it is sufficient to include the repo URL and the SHA-1 of the commit imported.

::: python/PyECC/.gitignore:1
(Diff revision 1)
> +*.py[co]

Please delete this file since it won't get picked up by anything.

::: python/PyECC/setup.py:1
(Diff revision 1)
> +#!/usr/bin/python2.4

#!/usr/bin/python2.4!!

This is a massive code smell. How old is this code? Are you sure there isn't something better out there? I know pycrypto is pretty popular. However, my guess is most other packages use Python C extensions or call into C libraries. And trust me, this is likely not a path you want to go down.

Fortunately, we don't run this file, so all is well.
Attachment #8646594 - Flags: review?(gps) → review+
Attachment #8646595 - Flags: review?(cykesiopka.bmo) → review+
Comment on attachment 8646595 [details]
MozReview Request: bug 1190603 - convert test_keysize.js to generate certificates at build time

https://reviewboard.mozilla.org/r/15175/#review14231

r+ with issues addressed.

::: security/manager/ssl/tests/unit/pycert.py:220
(Diff revision 1)
> +        # testing root certificates. At some point we should clean this

It would be nice to file a bug on this and reference it here so we don't forget.

::: security/manager/ssl/tests/unit/pykey.py:24
(Diff revision 1)
>  

rsa1024 etc should be documented here as well.

::: security/manager/ssl/tests/unit/pykey.py:26
(Diff revision 1)
>  (type, strength, signature algorithm, etc.).

The signature algorithm part should be removed, since we can specify it now.

::: security/manager/ssl/tests/unit/pykey.py:568
(Diff revision 1)
> +secp256k1Params = (long('fffffffffffffffffffffffffffffffffffffffffffffffffffffffefffffc2f', 16),

Would be nice to note that this is in the format expected by ecc.curves.DOMAINS - it's not obvious at first glance why the params are structured like this.

::: security/manager/ssl/tests/unit/test_keysize/ee_rsa_1024-int_rsa_1024-root_rsa_1016.pem.certspec:3
(Diff revision 1)
> +issuerKey:rsa1016

Looks like this should be rsa1024 instead.
Comment on attachment 8646596 [details]
MozReview Request: bug 1190603 - rename prime256v1 to secp256r1 in test_keysize.js to reduce confusion

https://reviewboard.mozilla.org/r/15177/#review14235

Ship It!
Attachment #8646596 - Flags: review?(cykesiopka.bmo) → review+
https://reviewboard.mozilla.org/r/15173/#review14101

Ok - I added those in.

> #!/usr/bin/python2.4!!
> 
> This is a massive code smell. How old is this code? Are you sure there isn't something better out there? I know pycrypto is pretty popular. However, my guess is most other packages use Python C extensions or call into C libraries. And trust me, this is likely not a path you want to go down.
> 
> Fortunately, we don't run this file, so all is well.

Huh - I hadn't even seen this. I think most of the implementation was written in 2010. As far as I can tell, the rest of the code is reasonably well implemented. This is the only ECC library that didn't require C extensions or C libraries.
https://reviewboard.mozilla.org/r/15175/#review14231

Thanks for the review!

> It would be nice to file a bug on this and reference it here so we don't forget.

Sounds good. Filed bug 1194419.

> rsa1024 etc should be documented here as well.

Good call. I also added the EC keys.

> Would be nice to note that this is in the format expected by ecc.curves.DOMAINS - it's not obvious at first glance why the params are structured like this.

Good idea - I added documentation.

> Looks like this should be rsa1024 instead.

Good catch.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7169f8df04db
https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e8c9f1dcf7a

I had forgotten there should be some non-determinism in ECDSA as used in production, but since we don't want non-determinism and the private keys are all public anyway, I mock-patched the implementation to be deterministic.
... and I did that in the wrong changeset. I'll fix it before I check in. (After/assuming :gerv r+s the library license.)
Comment on attachment 8646594 [details]
MozReview Request: bug 1190603 - import PyECC library

This license is fine. This is test code, right? If so, nothing else to do.

Gerv
Attachment #8646594 - Flags: review?(gerv) → review+
Yes, this is just test code. Thanks!
You need to log in before you can comment on or make changes to this bug.