fields_to_partial_json() in jwk.py reads RSA private key

[therealcmj]

Using an RSA provided by an HSM I discovered a code path where josepy attempted to read the private key "numbers" when calling challenge.validation() on a DNS challenge (passing in the RSAKey object).

The specific issue is on line 269, and then the values from that key are then read in 274-279.

josepy/src/josepy/jwk.py

Line 269 in fbee9f2

private = self.key.private_numbers()

This in definitely incorrect since RSA private keys should be kept completely private, but does work when the the RSAPrivateKey allows you to read those numbers. However, if the RSA key is stored in an HSM this is (rightly) impossible.

Commenting out those lines appears to have no ill effect - at least on DNS challenges and Let's Encrypt's staging server. I was able to submit an order, retrieve the challenges, call .validate() on both the DNS and HTTP challenges, answer the DNS challenge, finalize the order, and retrieve the certificate.

I found a similar issue in the ECDSA code (lines 364, 366, 394), but did not do any testing on changing those.

I'm happy to provide a pull request if you would like, but thought I should open the issue first to discuss.

[wgreenberg]

hi, unfortunately i'm having trouble understanding the issue you're running into. did Certbot error out due to the call to private_numbers()? and if so, could you provide the traceback?

This in definitely incorrect since RSA private keys should be kept completely private, but does work when the the RSAPrivateKey allows you to read those numbers. However, if the RSA key is stored in an HSM this is (rightly) impossible.

i don't follow. a JWK representation of a private key has to contain the parameters of that key (see https://datatracker.ietf.org/doc/html/rfc7517). if you can't access the private key because it's embedded in an HSM, then you cannot create a JWK representation of it.

[therealcmj]

thumbprint.py

I will be the first to admit I'm very unfamiliar with this specific part of the tech stack, though I do understand PKI pretty well.

Without getting too far into the details of what I'm ultimately trying to accomplish... I am using Certbot's ACME library to interact with ACME server(s). And the ACME lib obviously in turn uses josepy to do the signing and key handling the ACME protocol requires. I have a key (in an HSM) and am using that to register and account and submit an order. And I wrote an RSAKey implementation to abstract out interactions with the HSM. All of that works perfectly fine - I can submit an order and I get back the authorization containing an array of challenges (represented by instances of challenges.DNS01, challenges.HTTP01, and others).

When I then call the validation() method on either of those the JOSE related code eventually calls back to my class and specifically the method that retrieves the private numbers. In the default RSAKey implementation you can read the private numbers, but when the key is in an HSM you can't read them out. So I throw a NotImplementedError to say so.

The code path is pretty deep so but here's the relevant part of the stack trace of of my test code:

  File "tester.py", line 190, in <module>
    dnsvalue, challenge, order = ac.submit_order(csr_pem)
                                 ~~~~~~~~~~~~~~~^^^^^^^^^
  File "/tmp/testing/ACMEWrapper.py", line 157, in submit_order
    dnsvalue = challenge.validation(self._client_acme.net.key)
  File "/tmp/testing/thirdparty/certbot/acme/src/acme/challenges.py", line 265, in validation
    return jose.b64encode(hashlib.sha256(self.key_authorization(
                                         ~~~~~~~~~~~~~~~~~~~~~~^
        account_key).encode("utf-8")).digest()).decode()
        ^^^^^^^^^^^^
  File "/tmp/testing/thirdparty/certbot/acme/src/acme/challenges.py", line 177, in key_authorization
    account_key.thumbprint(
    ~~~~~~~~~~~~~~~~~~~~~~^
        hash_function=self.thumbprint_hash_function)).decode()
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/testing/thirdparty/josepy/src/josepy/jwk.py", line 65, in thumbprint
    {k: v for k, v in self.to_json().items() if k in self.required},
                      ~~~~~~~~~~~~^^
  File "/tmp/testing/thirdparty/josepy/src/josepy/interfaces.py", line 149, in to_json
    return _serialize(self)
  File "/tmp/testing/thirdparty/josepy/src/josepy/interfaces.py", line 135, in _serialize
    return _serialize(obj.to_partial_json())
                      ~~~~~~~~~~~~~~~~~~~^^
  File "/tmp/testing/thirdparty/josepy/src/josepy/json_util.py", line 572, in to_partial_json
    jobj = self.fields_to_partial_json()
  File "/tmp/testing/thirdparty/josepy/src/josepy/jwk.py", line 269, in fields_to_partial_json
    private = self.key.private_numbers()
  File "/tmp/testing/OCIWrapper.py", line 436, in private_numbers
    raise NotImplementedError("Exporting RSA private keys is not supported.")
NotImplementedError: Exporting RSA private keys is not supported.

Commenting out the specific lines I mentioned originally had no ill effect and I was able to finalize my order. So it wasn't clear to me initially why it was being called.

But after digging deeper I think I understand the problem.

The Google says:

In the context of domain validation challenges (such as HTTP-01 or DNS-01), the authorization key is used to create a key authorization string. This string is calculated by concatenating the challenge token with the base64url-encoded thumbprint of the account's public key, separated by a period (.).

(emphasis mine)

This description come from https://datatracker.ietf.org/doc/html/rfc7638#section-3.2.1 which is the RFC mentioned in jwk.py's thumbprint() method. And it explains why my original hack worked - the private key isn't needed for the authorization string. But removing this is definitely wrong for a general purpose JOSE library.

So backing up, the actual code problem is that thumbprint() method is doing a to_json() of the entire key, pulling out the fields that are required for the thumbprint, and then hashing the result. Whereas it should flip the process - take the list of fields needed for the thumbprint, extract just those, and then hash the result.

I've looked at the code and (as someone just newly looking at it) it's not super obvious how to implement that in a clean and obvious way.

I also wrote a test driver (with implementations of both RSA and EC keys) to demonstrate the problem without the rest of Certbot or any other code.

[wgreenberg]

thanks for clarifying, i think i have a good idea of the problem now.

unfortunately, i don't think that changing how thumbprint() works is a super attractive option for us, since this is a fairly niche use-case. i wonder if subclassing JWKRSA/JWKEC and overriding thumbprint() could work for you, though? presumably your HSM provides some way of extracting the necessary fields to create a thumbprint (and if not, there's no way to create a valid key auth to begin with).

[therealcmj]

I think I solved it cleanly

I don't know why GitHub isn't liking my diff but it's just 2 lines of code plus a comment in the existing thumbprint() method:

        # As per the RFC, thumbprints are based solely upon the public key.
        # If the JWK represents a private key then construct a public key and retreive its thumbprint instead
        # this avoids problems when the RSA key is in an HSM with only a negligible change to code.
        if self != self.public_key():
            return self.TYPES[self.typ](key=self.key._wrapped.public_key()).thumbprint(hash_function)

therealcmj@fdef840

This checks out for RSA and EC keys and should work for any additional keys in the future.

[wgreenberg]

I don't know why GitHub isn't liking my diff but it's just 2 lines of code plus a comment in the existing thumbprint() method:

you're using single backticks, but for code blocks you want three: https://www.markdownguide.org/extended-syntax/#fenced-code-blocks

your solution seems reasonable, but again i think it's probably too niche for us to want to mainline. have you tried subclassing the JWK classes and using your code in an overridden thumbprint() method?

[wgreenberg]

or alternatively, just patch your HSM key class to return the required fields in its json method?

[therealcmj]

I could simply return None in the private_numbers() and private_bytes() methods. Though that's definitely not correct behavior the JOSE code seems to be unbothered by that. At least in all of the code paths that I've tested. I do worry that I didn't get anywhere near a full test. And also whether that would blow up in some other library if someone were to try to reuse this code. And since I do want to eventually bubble this up to something more official that's a valid long term concern.

In any case that's a workaround and doesn't change the fact that the existing JWK thumprint() code really is doing the wrong thing here. Since the thumbprint is based solely on the public key there is no reason for that code to even try to read the private key material.