TimothyClaeys / pycose

A Python implementation of the COSE specification (CBOR Object Signing and Encryption) described in RFC 8152.
https://tools.ietf.org/html/rfc8152
Other
39 stars 24 forks source link

Is `ecdsa` dependency required? #97

Open letmaik opened 2 years ago

letmaik commented 2 years ago

This is the only place where the ecdsa library is used:

https://github.com/TimothyClaeys/pycose/blob/5a08c024fefd7656db7c476f868e1ac82bf44459/pycose/algorithms.py#L19-L21 https://github.com/TimothyClaeys/pycose/blob/5a08c024fefd7656db7c476f868e1ac82bf44459/pycose/algorithms.py#L182-L197

I'm wondering whether the following functions of the cryptography package would be sufficient to handle this:

If the above is not a perfect fit, what is missing?

TimothyClaeys commented 2 years ago

Last time I checked (at least a year ago), the problem was that the ECDSA implementation in the cryptography package was an non-deterministic implementation while the COSE spec needs a deterministic ECDSA.

letmaik commented 2 years ago

I see, alright. I found https://github.com/pyca/cryptography/issues/6810 which is closed because OpenSSL does not support it. I wonder how COSE implementations in other languages deal with that issue, for example go-cose or t_cose. I know that the latter has an OpenSSL backend.

letmaik commented 2 years ago

I just checked the COSE RFC and it says:

Implementations SHOULD use a deterministic version of ECDSA such as the one defined in [RFC6979].

So it's not a MUST, but a SHOULD. I'm assuming most libraries use the non-deterministic variant implicitly.

TimothyClaeys commented 2 years ago

We could maybe make it an optional dependency. If users want the deterministic one they can install it with pip install pycose[deterministic], otherwise we default to the non-deterministic implementation.

Clearly mentioning that in case of the deterministic ECDSA, we have an additional dependency on the ecdsa package.

letmaik commented 2 years ago

Making it optional seems ok but I wouldn't automatically use ecdsa if it happens to be installed, after all it can come in through a dependency of some other package. My main concern are the very explicit notes at https://github.com/tlsfuzzer/python-ecdsa#security and the bold sentence at the beginning saying it should not be used in production settings.

I think pycose isn't the place to address this problem and it should probably just use the non-deterministic variant. Once upstream libraries like OpenSSL and cryptography support the deterministic variant, then pycose should switch to that.

letmaik commented 1 year ago

@TimothyClaeys What do you think?

letmaik commented 1 year ago

See also a similar discussion on go-cose: https://github.com/veraison/go-cose/issues/73#issuecomment-1172520960

TimothyClaeys commented 1 year ago

Ok, I agree with your proposal. Let's remove the ecdsa dependencies from the code. It will require some updates on the unit tests too, because all ECDSA-based tests are now using the non-deterministic ones. We will need new test vectors because the ones that I used (from the COSE-WG examples repository: https://github.com/cose-wg/Examples) are all deterministic as far as I know.