Implement ECDSA keys and use them by default #563
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## master #563 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 10 10
Lines 1164 1181 +17
Branches 46 47 +1
=========================================
+ Hits 1164 1181 +17
|
ECDSA keys can be generated an order of magnitude faster than RSA keys. This commit adds an implementation for ECDSA, and adds parameters to CA.__init__, CA.issue_certificate and CA.create_child_ca. The default is ECDSA, given that OpenSSL and browsers support this for at least 10 years. This also adds an option to the cli to switch between RSA and ECDSA. The basic test and the SSL server test have been parametrized over both types of keys. This ensures that RSA support is not accidentally broken, while keeping most of the tests fast.
Cryptography provides types since version 3.4.4 and the relevant interfaces have been typed since version 35.0.0
|
Nice! Have you figured out why the coverage is dropping? codecov is not telling what lines are missing. 🤦 |
|
Seems like the codecov interface started working again, I showed the changes for me. The one on the |
This would be a breaking change and it makes codecov happier. This could be reintroduced in the future using a larger set of allowed keys, for example using cryptography.hazmat.primitives.asymmetric.types.CertificatePublicKeyTypes.
trustme/__init__.py
Outdated
| elif self is KeyType.ECDSA: # pragma: no cover | ||
| return ec.generate_private_key(ec.SECP256R1()) |
There was a problem hiding this comment.
I'm not sure I'm following, what is not covered in the case where the key is not KeyType.RSA nor KeyType.ECDSA? Maybe we can we have a else clause that raises a ValueError in that case?
There was a problem hiding this comment.
Do you mean that we should also add a test for that branch or put the no coverage on the else or also add a test for that? I'm a bit hesitant to add such a check since tgis is already encoded in the type. In the current case, mypy will also warn if one of the branches is forgotten, which would no longer be the case.
I don't have strong feelings for either way, let me know what you prefer and I will implement it.
There was a problem hiding this comment.
Either works, what I want to avoid is having a nocover pragma on a covered branch, which is what we have here, right?
My understanding is that to avoid that we need an else branch, even if mypy knows it will never be reached. Once we have that branch, let's use the simplest way to make all tools happy (nocover/type ignore/an unit test).
How does that sound?
There doesn't seem to be a way to tell coverage to ignore the the case when nothing matches in a if-elif group.
|
Seems like the codecov uploader we use got removed: codecov/python-standard#31 |
|
Ouch, I did not realize we used that uploader here. #574 switches to an alternative approach. |
ECDSA keys can be generated an order of magnitude faster than RSA keys. This commit adds an implementation for ECDSA, and adds parameters to
CA.__init__,CA.issue_certificateandCA.create_child_ca. The default is ECDSA, given that OpenSSL and browsers support this for at least 10 years. This also adds an option to the cli to switch between RSA and ECDSA.The basic test and the SSL server test have been parametrized over both types of keys. This ensures that RSA support is not accidentally broken, while keeping most of the tests fast.
Closes #559