Sahamati / rahasya

The project aims to simplify the usage of ECC curve (curve25519) with Diffie-Hellman Key exchange. The work is inline with the Account Aggregator Specification.
Apache License 2.0
13 stars 20 forks source link

Using algorithm "X25519" for diffie hellman instead of EC algorithm with curve "Curve25519" to comply with RFC-7748 standard #15

Open hamirshekhawat opened 3 years ago

hamirshekhawat commented 3 years ago

The issue with using "Curve25519" is that it generates a 64 bytes key. For diffie-hellman key exchange, according to rfc-7748 [refer: https://tools.ietf.org/html/rfc7748#page-7] , the key should be 32 bytes. If a service uses openssl for generating x25519 key, the generated key is 32 bytes. This cannot be converted into the 64 bytes format that is currently accepted by BouncyCastle implementation. For the sake of inter-compatibility and proper standards, using "X25519" is recommended way of creating keys. Also please refer https://github.com/bcgit/bc-java/issues/251 issue in bouncycastle repo for better explanation. This also suggests to use full X25519 ECDH. If we decide to follow X25519 (RFC7748), I can create a PR to incorporate the changes. This should not affect the implementations used by banks and Account Aggregators.

peterdettman commented 3 years ago

I don't really understand your point. We already implement the "X25519" algorithm (for KeyAgreement, KeyFactory, KeyPairGenerator). Use of the "Curve25519" curve with EC/ECDH algorithms pre-dates RFC7748 and is not the same thing.

hamirshekhawat commented 3 years ago

@peterdettman That is true. The issue is with the repository https://github.com/gsasikumar/forwardsecrecy. It is using "Curve25519" with "EC" algorithm. This, as you said is not the same thing as RFC7748. For Diffie-Hellman Key exchange, X25519 algorithm should be used in https://github.com/gsasikumar/forwardsecrecy. @gsasikumar Please take a look at this issue and issue: https://github.com/bcgit/bc-java/issues/251#issuecomment-784294554. I recommend that we follow RFC7748 to have easier compatibility between all cryptography library and to follow the Account Aggregator spec. Many libraries don't support "EC" algorithm with curve "25519" as it is not standard. Let's follow standard specifications for better integrity, i.e. X25519 Algorithm.

saukap commented 3 years ago

@gsasikumar I second the proposal by @hamirshekhawat. It seems this reference implementation of Curve25519 has become the standard that technology companies are using to develop solutions for FIPs. They are simply using this repository as a micro-service. We are still in the early stages of the Sahmati AA ecosystem and hence we have the opportunity to fix this problem.

We should use the standard X25519 key-exchange format so that FIUs and other players in the ecosystem are free to use other libraries to perform encryption.

Besides other issues, it's not possible to read the generated public key using OpenSSL without dropping to their C level API.

hamirshekhawat commented 3 years ago

@saukap Very true. When more organisations join the ecosystem, bigger this problem will become. @gsasikumar I think we should fix this ASAP. Creating workaround for time being is not a good solution. The fix might take time to integrate but will be a step towards better ecosystem support. All orgs now rely on this repository, please do consider fixing it.

saukap commented 3 years ago

@gsasikumar I have created a pull request to serve as a demonstration of the kind of changes that @hamirshekhawat and I are arguing for.

16

gsasikumar commented 3 years ago

@saukap I just accepted your PR on the same so the reference implementation exists. I will need to ensure backward compatibility as well. So I will take some more time to make the changes to the existing code. Until then I am leaving this open. You can expect more discussion on this in the coming week.

Thanks and appreciate the contribution.

saukap commented 3 years ago

That's great @gsasikumar. Indeed, given that this repository is being used in production, it's worth making appropraite changes to the existing code.

Also, I agree that we'll need to keep the ECC curve25519 format around for some time to allow organizations to transition to the x25519 version. But I think we should mark the ECC version deprecated and notify the consumers (FIUs, AAs, FIPs) of the timeline when this will be removed. This is because if even some players (FIUs, FIPs) end up generating keys using curve25519, it will add an implementation overhead for the others in the ecosystem.

hamirshekhawat commented 3 years ago

Any update on this?

BhavyaSheth22 commented 2 years ago

Any update on this? What should an implementation of this ideally use?

gsasikumar commented 2 years ago

I made a backward compatible change in the X25519Service.java, Please check here #PR24 https://github.com/Sahamati/rahasya/pull/24/files

The respective test for the same is here

shethchintan7 commented 2 years ago

What is the conclusion on this? If we use x25519, is it supported across the ecosystem?

gsasikumar commented 2 years ago

I think we should have moved by now. But not sure who is coordinating with the ecosystem.

gsasikumar commented 2 years ago

Seems like everyone started using the latest code. can we close it?

shethchintan7 commented 2 years ago

I had checked in Sahamati Discord, according to people over there it seems this is not yet adapted and will be enforced in rebit 2.0 spec

gsasikumar commented 2 years ago

@shethchintan7 any idea when its due? Faster we move its better for everyone.

Ciantic commented 2 years ago

X25519 key is 64 bytes because it concats private key and public key, the private key is still 32 bytes, but it's also followed by public key of 32 bytes.