search

tekul / jose-jwt

Haskell implementation of JOSE/JWT standards
BSD 3-Clause "New" or "Revised" License
37 stars 22 forks source link

added ecsigning #25

Closed ProofOfKeags closed 3 years ago

tekul commented 6 years ago

Thanks for the PR! However, this looks similar to https://github.com/tekul/jose-jwt/pull/11 and as far as I know the issues discussed in there about are still relevant for the code from Crypto.PubKey.ECC.ECDSA.

ProofOfKeags commented 6 years ago

What would you recommend in that case, at the moment we are using the ES256 functionality off of my fork. Should we just remove support for SHA384? or is this a reason to block all of ECDSA signing.

tekul commented 6 years ago

I don't really know enough about EC signing to be able to say. I haven't studied the different EC code within Cryptonite in any detail.

I'm not aware of any problems with SHA384. That PR just happened to discover a bug in cryptonite which was fixed back then.

ProofOfKeags commented 6 years ago

Then either I didn't read the PR you linked properly but the only thing I see is related to EC timing issues. But I don't see the issue with this as EC timing attacks are a well documented problem in cryptonite and the exposure created by that is only relevant in certain scenarios. I think we should just document that they are subject to the same timing attacks in cryptonite and then merge this. Because it still has utility even the presence of potential timing issues.

ProofOfKeags commented 5 years ago

I'd like to revisit this PR. Will you merge this if I put documentation in it that it is subject to the same timing attacks as cryptonite? Not all systems are timing attack sensitive, and I think this would be helpful to those with that use case, it certainly was in my case, I'd like to get off my fork and back onto the main package.

tekul commented 4 years ago

Yeah, I understand it's not convenient as things stand. Would signing based on Ed25519/Ed448 suit your needs? The cryptonite code for that is based on a reasonably well-known C implementation.

ProofOfKeags commented 4 years ago

i mean, EdDSA is not part of the JWT standard afaik, which would make it incompatible other libs that we are interfacing with. I'm happy to continue using my fork if you don't want to merge it. I just figured that it would be better if the packages don't continue to diverge. Not sure what your dev/maintenance philosophy on this package is.

tekul commented 4 years ago

It is actually supported via a supplementary RFC https://tools.ietf.org/html/rfc8037 and quite a few libraries support it. It's something I'd like to support as an alternative to RSA.

ProofOfKeags commented 3 years ago

Is this still relevant or should I close it?

tekul commented 3 years ago

Yeah, sorry. I will close this. EdDSA is now supported as described in the RFC I linked above. I don't really want to add support for the ECDSA options which generally considered to be less secure and harder to implement correctly.