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

Consider adding an option for ECDSA keys #559

Closed
VincentVanlaer opened this issue Mar 5, 2023 · 5 comments · Fixed by #563
Closed

Consider adding an option for ECDSA keys #559

VincentVanlaer opened this issue Mar 5, 2023 · 5 comments · Fixed by #563

Comments

@VincentVanlaer
Copy link
Member

VincentVanlaer commented Mar 5, 2023

ECDSA keys can be generated much faster.

Benchmarks

Benchmarks of the tests using hyperfine:

RSA

  Time (mean ± σ):     14.322 s ±  0.495 s    [User: 14.252 s, System: 0.079 s]
  Range (min … max):   13.412 s … 14.958 s    10 runs

ECDSA (using secp521r1)

  Time (mean ± σ):     776.9 ms ±  21.7 ms    [User: 738.8 ms, System: 68.0 ms]
  Range (min … max):   753.7 ms … 823.5 ms    10 runs

For these last results, startup time becomes relevant, as pytest reports ~0.55s

Diff for the ECDSA benchmarks

diff --git a/tests/test_trustme.py b/tests/test_trustme.py
index c9de4f8..324d8f9 100644
--- a/tests/test_trustme.py
+++ b/tests/test_trustme.py
@@ -69,7 +69,7 @@ def test_basics() -> None:
 
     today = datetime.datetime.today()
 
-    assert b"BEGIN RSA PRIVATE KEY" in ca.private_key_pem.bytes()
+    assert b"BEGIN EC PRIVATE KEY" in ca.private_key_pem.bytes()
     assert b"BEGIN CERTIFICATE" in ca.cert_pem.bytes()
 
     private_key = load_pem_private_key(
@@ -80,9 +80,9 @@ def test_basics() -> None:
     assert ca_cert.not_valid_before <= today <= ca_cert.not_valid_after
 
     public_key1 = private_key.public_key().public_bytes(
-        Encoding.PEM, PublicFormat.PKCS1)
+        Encoding.PEM, PublicFormat.SubjectPublicKeyInfo)
     public_key2 = ca_cert.public_key().public_bytes(
-        Encoding.PEM, PublicFormat.PKCS1)
+        Encoding.PEM, PublicFormat.SubjectPublicKeyInfo)
     assert public_key1 == public_key2
 
     assert ca_cert.issuer == ca_cert.subject
diff --git a/trustme/__init__.py b/trustme/__init__.py
index a00c960..3b416cb 100644
--- a/trustme/__init__.py
+++ b/trustme/__init__.py
@@ -12,7 +12,7 @@ import idna
 from cryptography import x509
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives import hashes
-from cryptography.hazmat.primitives.asymmetric import rsa
+from cryptography.hazmat.primitives.asymmetric import rsa, ec
 from cryptography.hazmat.primitives.serialization import (
     PrivateFormat, NoEncryption
 )
@@ -219,11 +219,7 @@ class CA:
         organization_unit_name: Optional[str] = None,
     ) -> None:
         self.parent_cert = parent_cert
-        self._private_key = rsa.generate_private_key(
-            public_exponent=65537,
-            key_size=_KEY_SIZE,
-            backend=default_backend()
-        )
+        self._private_key = ec.generate_private_key(ec.SECP521R1)
         self._path_length = path_length
 
         name = _name(
@@ -353,11 +349,7 @@ class CA:
                 "Must specify at least one identity or common name"
             )
 
-        key = rsa.generate_private_key(
-            public_exponent=65537,
-            key_size=_KEY_SIZE,
-            backend=default_backend()
-        )
+        key = ec.generate_private_key(ec.SECP521R1)
 
         ski_ext = self._certificate.extensions.get_extension_for_class(
             x509.SubjectKeyIdentifier)
@sethmlarson
Copy link

Thanks for opening this! To me this sounds like a good enhancement if it's provided as an option, any thoughts on how you'd incorporate that into the existing API?

Also would you be willing to contribute the change if we can agree on a design?

@VincentVanlaer
Copy link
Member Author

My suggestion would be to add a keytype parameter to CA.__init__ and CA.issue_cert with the following enum as type:

class KeyType(Enum):
    RSA = 0
    ECDSA = 1
    # Can easily be extended with more keytypes or more specific keytypes, e.g.
    RSA_4096 = 2

CA.__init__ could then have KeyType.RSA as default and CA.issue_cert could use the same keytype as the parent by default.

I'm happy to implement this.

@njsmith
Copy link
Member

njsmith commented Mar 6, 2023

Are ECDSA keys universally supported? If so then we should probably also make them the default, because it's effectively a free speedup.

Having an optional arg to CA.__init__ and CA.issue_cert makes sense to me. I'd rather that the default be a constant though, rather than inheriting from the parent.

@pquentin
Copy link
Member

pquentin commented Mar 6, 2023

Browsers have wide support according to https://developers.cloudflare.com/ssl/reference/browser-compatibility/:

SNI and ECDSA certificates work with these modern browsers:

Desktop Browsers installed on Windows Vista or OS X 10.6 or later:

  • Internet Explorer 7
  • Firefox 2
  • Opera 8 (with TLS 1.1 enabled)
  • Google Chrome v5.0.342.0
  • Safari 2.1

Mobile Browsers:

  • Mobile Safari for iOS 4.0
  • Android 3.0 (Honeycomb) and later
  • Windows Phone 7

I haven't been able to figure out when OpenSSL introduced support for that specific key for certificates and I'm not sure what else we need to check. I'm also happy to make this the default if we find that support is good.

@VincentVanlaer
Copy link
Member Author

Given this stackoverflow question was posted in 2012, ecdsa certs have been supported in openssl since at least version 1.0.1. I think it is fine in that case to switch to ecdsa certs.

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 a pull request may close this issue.

4 participants