Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement ECDSA keys and use them by default #563

Merged
merged 6 commits into from
Apr 19, 2023

Conversation

VincentVanlaer
Copy link
Member

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.

Closes #559

@codecov
Copy link

codecov bot commented Mar 8, 2023

Codecov Report

Merging #563 (ee96cb3) into master (5d54fde) will not change coverage.
The diff coverage is 100.00%.

❗ Current head ee96cb3 differs from pull request most recent head f74a9b6. Consider uploading reports for the commit f74a9b6 to get more accurate results

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     
Impacted Files Coverage Δ
trustme/__init__.py 100.00% <100.00%> (ø)
trustme/_cli.py 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

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
@pquentin
Copy link
Member

pquentin commented Mar 9, 2023

Nice! Have you figured out why the coverage is dropping? codecov is not telling what lines are missing. 🤦

@VincentVanlaer
Copy link
Member Author

Seems like the codecov interface started working again, I showed the changes for me. The one on the else if in _generate_key requires a no cover, since there are no other enum values. It could be changed to an else, but with an else if mypy catches missing implementations for new key types. The other missing coverage is on the check I added to CA.from_pem to make type checking pass. However, maybe its better to drop this and just add a type: ignore, since technically limiting the cert type is a breaking change. It would be nice to use https://cryptography.io/en/latest/hazmat/primitives/asymmetric/#cryptography.hazmat.primitives.asymmetric.types.CertificatePublicKeyTypes when typing functions, but that's very new.

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.
Copy link
Member

@pquentin pquentin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the long delay. This looks good, I just have a question left.

Comment on lines 220 to 221
elif self is KeyType.ECDSA: # pragma: no cover
return ec.generate_private_key(ec.SECP256R1())
Copy link
Member

@pquentin pquentin Mar 31, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@VincentVanlaer
Copy link
Member Author

Seems like the codecov uploader we use got removed: codecov/python-standard#31

@pquentin pquentin mentioned this pull request Apr 17, 2023
@pquentin
Copy link
Member

Ouch, I did not realize we used that uploader here. #574 switches to an alternative approach.

@pquentin pquentin merged commit 1571357 into python-trio:master Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider adding an option for ECDSA keys
2 participants