Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
hamirshekhawat opened this issue Feb 21, 2021 · 16 comments

Comments

@hamirshekhawat
Copy link

hamirshekhawat commented Feb 21, 2021

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 bcgit/bc-java#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.

@hamirshekhawat hamirshekhawat changed the title Using algorithm "X25519" for diffie hellman instead of "Curve25519" to comply with RFC-7748 standard Using algorithm "X25519" for diffie hellman instead of EC algorithm with curve "Curve25519" to comply with RFC-7748 standard Feb 23, 2021
@peterdettman
Copy link

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
Copy link
Author

hamirshekhawat commented Feb 24, 2021

@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: bcgit/bc-java#251 (comment). 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
Copy link
Contributor

saukap commented Mar 19, 2021

@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
Copy link
Author

hamirshekhawat commented Mar 19, 2021

@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
Copy link
Contributor

saukap commented Mar 20, 2021

@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
Copy link
Contributor

@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
Copy link
Contributor

saukap commented Mar 27, 2021

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
Copy link
Author

Any update on this?

@BhavyaSheth22
Copy link

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

@gsasikumar
Copy link
Contributor

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
Copy link

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

@gsasikumar
Copy link
Contributor

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

@gsasikumar
Copy link
Contributor

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

@shethchintan7
Copy link

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
Copy link
Contributor

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

@Ciantic
Copy link

Ciantic commented Nov 11, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants