Add support for ES256 #51

jannispinter · 2019-07-12T20:09:33Z

This PR adds support for ES256 and is based on #8 by @yonran.

Compared to #8, I have added several tests and made sure that everything passes without any errors in tox for both Python 2.7 and Python 3.7. In order to make it more useful, an implementation for JWA was added as well (note that #8 only added EC support for JWK). It is now possible to create a valid JWS with ES256, ES384 and ES512.

Finally, I have fixed several minor bugs and glitches that existed in #8. One obstacle was that P-521 coordinates are encoded in different lengths (depending on the most significant byte being 0x00 and other factors) and are not encoded fixed length as it happens to be in P-256 and P-384. The previous code failed for P-521 most of the time.

This is my first attempt to add EC support to josepy and I am very happy to get comments on this and improve it until it is ready to be merged.

bmw · 2019-07-16T20:52:32Z

Thanks for the PR @jannispinter! I'll try and take an in-depth look at this soon.

In the meantime, if you have any questions about specific pieces of this PR, let me know and I can probably give you some feedback sooner.

harrie91 · 2019-10-17T08:22:40Z

Hello there,
just wondering if there are any plans to accept this PR in the near future.
I'd like to see EC support in certbot.

bmw · 2019-10-17T17:57:51Z

We probably won't have time to review it for at least a few more weeks, but I too would like to see this PR merged sooner rather than later.

hansjoachimknobloch · 2020-01-09T16:35:05Z

Is there anything that can be done to advance the status this pull request?

bmw · 2020-01-09T19:05:36Z

Not really. If anyone sees/has any problems with the PR, it may be useful to suggest changes, but this is still blocked on someone from the Certbot team having time to review it.

We're planning on doing a thorough sweep of all of our community PRs in the next 6 weeks or so though so hopefully we can get this merged then.

hansjoachimknobloch · 2020-01-10T07:51:48Z

Great news! If there should be any issues during the review, don't hesitate to contact me and Jannis, the original submitter of the PR and former colleague of mine.

atombrella

I had a cursory look at this PR, and have a few comments. I think it appears to follow all of the specification in https://tools.ietf.org/html/rfc7518 @hansjoachimknobloch @jannispinter

atombrella · 2020-12-16T10:02:58Z

src/josepy/jwa_test.py

+
+    def test_sign_no_private_part(self):
+        from josepy.jwa import ES256
+        self.assertRaises(


I think this can be on one line.

Fixed by a5fb321

atombrella · 2020-12-16T10:04:15Z

src/josepy/jwk.py

@@ -248,7 +225,7 @@ def fields_from_json(cls, jobj):

            key = rsa.RSAPrivateNumbers(
                p, q, d, dp, dq, qi, public_numbers).private_key(
-                    default_backend())


What's the reason for this change?

My IDE must have auto-formatted this. I will revert this change.

Fixed by 2c3cabe

atombrella · 2020-12-16T10:07:07Z

src/josepy/jwk.py

+            'x': public.x,
+            'y': public.y,
+        })
+        params = dict((key, self._encode_param(value))


dict comprehensions are faster, and probably also more Pythonic. It works in Python 2.

{key: self._encode_param(value) for key, value in six.iteritems(params)}

Thanks! Fixed by 6e8d041

atombrella · 2020-12-16T10:14:13Z

src/josepy/testdata/ec_p256_key.pem

+MHcCAQEEIMUXjUASqqk7YRvKE5gXYaeBEGCJkitXVan2tssjJ9q3oAoGCCqGSM49
+AwEHoUQDQgAEjjQtV+fA7J/tK8dPzYq7jRPNjF8r5p6LW2R25S2Gw5UQ8DDz/zPs
+9gqwcfqGUZKWxbEWgWXv7S8zRBEZuae8Jw==
+-----END EC PRIVATE KEY-----


Just out of curiosity, how did you generate these keys? I think the README file should be updated with the command.

I generated them using OpenSSL
openssl ecparam -name prime256v1 -genkey -out ec_p256_key.pem
openssl ecparam -name secp384r1 -genkey -out ec_p384_key.pem
openssl ecparam -name secp521r1 -genkey -out ec_p521_key.pem

I will update the file testdata/README accordingly.

Fixed by 2f8bebe

atombrella · 2020-12-16T10:21:27Z

src/josepy/jwk.py

+        elif isinstance(self.key._wrapped, ec.EllipticCurvePrivateKey):
+            private = self.key.private_numbers()
+            public = self.key.public_key().public_numbers()
+            params.update({


Why not just do params['d'] = private.private_value ? It's ~3 times faster.

Fixed by 6e8d041

atombrella · 2020-12-16T10:23:05Z

src/josepy/util_test.py

+
+    def test_repr(self):
+        self.assertTrue(repr(self.p256_key).startswith(
+            '<ComparableECKey(<cryptography.hazmat.'))


Isn't it possible to add in the entire string/value?

No, unfortunately it will return the memory address as well which is different on every run:
<ComparableECKey(<cryptography.hazmat.backends.openssl.ec._EllipticCurvePrivateKey object at 0x6a0be0debd50>)>

jannispinter · 2020-12-16T14:39:00Z

Thank you @atombrella for your review! Please let me know if there is anything else that needs to be fixed with this PR.

atombrella · 2020-12-17T12:25:37Z

src/josepy/jwa.py

+    def sign(self, key, msg):
+        """Sign the ``msg`` using ``key``."""
+        # If cryptography library supports new style api (v1.4 and later)
+        new_api = hasattr(key, 'sign')


Why not just inline this?

atombrella · 2020-12-17T14:45:53Z

@jannispinter You're welcome. I'm not a member of the Certbot organization, but hopefully either @bmw @adferrand or @ohemorange can have a look at this. I had a look at the RFC, and I think this PR implements everything mentioned about EC.

Could you please also add an entry to CHANGELOG.md? I think 1.6.0 should be added as a header, with an undecided release date.

atombrella · 2020-12-17T14:46:25Z

And maybe the draft status is no longer necessary?

commonism · 2020-12-28T22:39:14Z

Works for me.
Using this branch to implement EC account keys server side.

One thing to add -

josepy/src/josepy/__init__.py

Lines 57 to 68 in c95193d

    
           from josepy.jwa import ( 
        
               HS256, 
        
               HS384, 
        
               HS512, 
        
               JWASignature, 
        
               PS256, 
        
               PS384, 
        
               PS512, 
        
               RS256, 
        
               RS384, 
        
               RS512, 
        
           )

The exports need to be updated.

diff --git a/src/josepy/__init__.py b/src/josepy/__init__.py
index 5f81e20f..4ceb7eda 100644
--- a/src/josepy/__init__.py
+++ b/src/josepy/__init__.py
@@ -65,6 +65,9 @@ from josepy.jwa import (
     RS256,
     RS384,
     RS512,
+    ES256,
+    ES384,
+    ES512,
 )

commonism · 2021-01-22T21:07:35Z

While I was successful having client talking to a server using the library, I was not able to talk to other servers using ES256 keys or have clients with ES256 account key capability (cpu.acmeshell) talk to the server.

There is a problem when verifying the ECDSA signature:

# print('\n'.join([i.reason_text.decode() for i in _errors_with_text(errors)]))
error:0D07207B:asn1 encoding routines:ASN1_get_object:header too long
error:0D068066:asn1 encoding routines:asn1_check_tlen:bad object header
error:0D07803A:asn1 encoding routines:asn1_item_embed_d2i:nested asn1 error

The josepy-ES powered server expects the signature in a different format.

The signature format is wrong, the library does not use the r || s format as defined in:
https://tools.ietf.org/html/rfc7515#appendix-A.3 .
Basically the same as mentioned here: funcool/buddy-sign#62 .

ECDSA_do_sign & ECDSA_do_verify exist to work with signatures without ASN1 - https://www.openssl.org/docs/man1.1.0/man3/ECDSA_verify.html .
Current cryptography lacks these exports: https://github.com/pyca/cryptography/blob/master/src/_cffi_src/openssl/ecdsa.py
Splitting the concat binary signature into r & s to use ECDSA_do_verify is shown here: https://github.com/Spomky-Labs/jose/blob/master/src/Algorithm/Signature/ECDSA.php#L101

commonism · 2021-01-24T12:17:29Z

I propose using asn1crypto to convert the ecdsa asn1 structures as required:

--- a/src/josepy/jwa.py	2020-12-28 20:03:20.515373893 +0100
+++ b/src/josepy/jwa.py	2021-01-24 13:15:02.416541955 +0100
@@ -6,6 +6,8 @@
@@ -5,6 +5,9 @@
 """
 import abc
 import logging
+import math
+
+from asn1crypto.core import SequenceOf, Integer
 
 import cryptography.exceptions
 from cryptography.hazmat.backends import default_backend
@@ -164,11 +167,20 @@
 class _JWAEC(JWASignature):
     kty = jwk.JWKEC
 
+    class RSSignature(SequenceOf):
+        _child_spec = Integer
+
     def __init__(self, name, hash_):
         super(_JWAEC, self).__init__(name)
         self.hash = hash_()
 
     def sign(self, key, msg):
+        sig = self._sign(key, msg)
+        seq = _JWAEC.RSSignature.load(sig)
+        return (r := seq[0].native).to_bytes(length=math.ceil(r.bit_length() / 8), byteorder='big') +\
+            (s := seq[1].native).to_bytes(length=math.ceil(s.bit_length() / 8), byteorder='big')
+
+    def _sign(self, key, msg):
         """Sign the ``msg`` using ``key``."""
         # If cryptography library supports new style api (v1.4 and later)
         new_api = hasattr(key, 'sign')
@@ -190,6 +202,14 @@
             raise errors.Error(str(error))
 
     def verify(self, key, msg, sig):
+        rlen = math.ceil(key._wrapped.key_size/8)
+        seq = _JWAEC.RSSignature([Integer(int.from_bytes(sig[0:rlen], byteorder="big")),
+                           Integer(int.from_bytes(sig[rlen:],
+                                                  byteorder="big"))])
+        asn1sig = seq.dump()
+        return self._verify(key, msg, asn1sig)
+
+    def _verify(self, key, msg, sig):
         """Verify the ``msg` and ``sig`` using ``key``."""
         # If cryptography library supports new style api (v1.4 and later)
         new_api = hasattr(key, 'verify')

adferrand · 2021-02-10T15:57:55Z

Hello @commonism! Thanks a lot for your help. If you want, you can also submit a PR based on this one, and I can merge it ultimately without squash to preserve the authorship from @jannispinter and you. Otherwise I think I can push commits on this PR while crediting you.

About the problematic of the signature format, I am quite hesitant to add a new dependency to josepy. How would it be complex to convert the ASN.1 format into the R || S format without adding asn1crypto?

By the way @atombrella, thanks a lot for your help in reviewing this PR, and of course @jannispinter and @yonran thanks a lot for all the work in the first place!

To clarify a little what is at stake on this PR, on top of what you propose in certbot/certbot#8569 @commonism, this would enable the possibility in Certbot to use ECDSA keys for the ACME accounts, for what Let'sEncrypt folks would be quite grateful.

adferrand · 2021-02-11T15:21:25Z

After a discussion with cryptography maintainers as mentionned above, in fact you can avoid a dependency on asn1crypto by using the decode_dss_signature method from cryptography.

https://cryptography.io/en/latest/hazmat/primitives/asymmetric/utils.html#cryptography.hazmat.primitives.asymmetric.utils.decode_dss_signature

You will get a tuple (R,S) from it given the bytes generated by the sig method on the EC key instance.

adferrand · 2021-02-11T16:33:19Z

I went ahead and applied the modifications proposed by @commonism, but using the methods provided by cryptography to encode/decode signatures from ASN.1 format.

adferrand

OK, this LGTM now!

I merge instead of squashing to preserve authorship.

Add support for ES256

8100733

bmw added the priority: unplanned Work that we believe should be done, but does not have a higher priority. label Mar 25, 2020

atombrella mentioned this pull request Dec 16, 2020

Implement JWKEs, to add support for JWK elliptic curves #74

Closed

atombrella reviewed Dec 16, 2020

View reviewed changes

jannispinter added 4 commits December 16, 2020 15:09

Fix formatting of test_sign_no_private_part()

a5fb321

Revert auto-indenting by IDE

2c3cabe

Update testdata/README with commands to generate EC keys

2f8bebe

Update assignment of values and use dict comprehensions

6e8d041

atombrella reviewed Dec 17, 2020

View reviewed changes

commonism mentioned this pull request Dec 31, 2020

certbot 1.10.1 fails to revoke ecdsa certificates using --cert-key certbot/certbot#8569

Closed

adferrand marked this pull request as ready for review January 7, 2021 20:11

adferrand mentioned this pull request Feb 11, 2021

Supporting ECDSA_SIG structures for ECDSA signing and verifying signatures pyca/cryptography#5811

Closed

Apply fixes proposed by @commonism, using cryptography methods.

f5dd050

adferrand force-pushed the ec-support branch from 1d50dcd to f5dd050 Compare February 11, 2021 16:22

adferrand changed the title ~~WIP: Add support for ES256~~ Add support for ES256 Feb 11, 2021

adferrand previously approved these changes Feb 11, 2021

View reviewed changes

Merge branch 'master' into pr/51

901291e

adferrand dismissed their stale review via 9134b65 February 11, 2021 16:40

adferrand force-pushed the ec-support branch 3 times, most recently from d7742cf to 7e19927 Compare February 11, 2021 17:30

Update changelog and avoid ES512 test (high risk to lack of entropy)

40b7fc8

adferrand force-pushed the ec-support branch from 7e19927 to 40b7fc8 Compare February 11, 2021 17:33

adferrand approved these changes Feb 11, 2021

View reviewed changes

adferrand merged commit bffb262 into certbot:master Feb 11, 2021

bmw mentioned this pull request Feb 11, 2021

Add support for Elliptic Curve keys #8

Closed

bmw mentioned this pull request Mar 10, 2021

EC signature padding #91

Merged

noahkw mentioned this pull request Mar 16, 2021

ecdsa account keys and certificates - [closed] noahkw/acmetk#47

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for ES256 #51

Add support for ES256 #51

jannispinter commented Jul 12, 2019

bmw commented Jul 16, 2019

harrie91 commented Oct 17, 2019

bmw commented Oct 17, 2019

hansjoachimknobloch commented Jan 9, 2020 •

edited

Loading

bmw commented Jan 9, 2020

hansjoachimknobloch commented Jan 10, 2020

atombrella left a comment •

edited

Loading

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

jannispinter Dec 16, 2020

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

jannispinter Dec 16, 2020

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

atombrella Dec 16, 2020

jannispinter Dec 16, 2020

jannispinter commented Dec 16, 2020

atombrella Dec 17, 2020

atombrella commented Dec 17, 2020

atombrella commented Dec 17, 2020

commonism commented Dec 28, 2020

commonism commented Jan 22, 2021

commonism commented Jan 24, 2021 •

edited

Loading

adferrand commented Feb 10, 2021 •

edited

Loading

adferrand commented Feb 11, 2021

adferrand commented Feb 11, 2021

adferrand left a comment

Add support for ES256 #51

Add support for ES256 #51

Conversation

jannispinter commented Jul 12, 2019

bmw commented Jul 16, 2019

harrie91 commented Oct 17, 2019

bmw commented Oct 17, 2019

hansjoachimknobloch commented Jan 9, 2020 • edited Loading

bmw commented Jan 9, 2020

hansjoachimknobloch commented Jan 10, 2020

atombrella left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jannispinter commented Dec 16, 2020

Choose a reason for hiding this comment

atombrella commented Dec 17, 2020

atombrella commented Dec 17, 2020

commonism commented Dec 28, 2020

commonism commented Jan 22, 2021

commonism commented Jan 24, 2021 • edited Loading

adferrand commented Feb 10, 2021 • edited Loading

adferrand commented Feb 11, 2021

adferrand commented Feb 11, 2021

adferrand left a comment

Choose a reason for hiding this comment

hansjoachimknobloch commented Jan 9, 2020 •

edited

Loading

atombrella left a comment •

edited

Loading

commonism commented Jan 24, 2021 •

edited

Loading

adferrand commented Feb 10, 2021 •

edited

Loading