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

Add support for X25519 and X448 #391

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

jvdsn
Copy link

@jvdsn jvdsn commented Mar 24, 2025

This pull request adds support for the X25519 and X448 to the server (also known as Curve25519 / Curve448 Diffie-Hellman). Three algorithms are implemented: KeyGen, KeyVer, and SSC (Shared Secret Computation). The different commits modify different layers of the server for easier review.

Some points that were considered while implementing this code:

  • RFC 7748 is the only reference used for this implementation.
  • All existing ECC code was part of "NIST.CVP.ACVTS.Libraries.Crypto/DSA.ECC" or "NIST.CVP.ACVTS.Libraries.Crypto/DSA.Ed". Because X25519 and X448 are not DSA algorithms, this is not a right fit for these algorithms. To avoid a very large refactor, the code was separated into a separate directory "NIST.CVP.ACVTS.Libraries.Crypto/XECDH".
  • As a result, there are also no general-purpose classes or interfaces for Montgomery curves or points on Montgomery curves. They were initially implemented, but cumbersome to work with as X25519/X448 are much more specialized. The algorithms rely on the theory, but use several shortcuts. For example, X25519/X448 only use the u coordinate (Montgomery points are (u, v)) as an input to the Diffie-Hellman process.
  • Initially, I considered an "ECX_Component" similar to the existing "ECC_Component", that was fully integrated in the existing KAS code. However, as KeyGen and KeyVer algorithms (similar to EdDSA) were added later, I decided to pull out the code and completely separate it from KAS.
  • The cryptographic code uses BitString but does not fully use its capabilities. For example, converting a BitString to an integer in a little-endian manner is explicitly implemented following the RFC. This was to
    a) Make sure the code clearly maps to the RFC
    b) Avoid any potential endianness issues since the BitString internally uses big-endian
  • For X25519/X448, the RFC requires that the implementation accepts basically any 32-byte (resp. 48-byte) string as a valid public key. Therefore, the KeyVer testing is very limited. As the private key is currently not provided to the IUT, pair-wise consistency is not in scope (similar to EdDSA). It could be considered to expand this, as long as compliance with the RFC is maintained.
  • Test cases for the cryptographic code are taken from RFC 7748. In particular, the iterated test cases are using only 1,000 iterations at this point. 1,000,000 should be possible but may take much longer (perhaps 30 minutes per test, on my hardware) 1,000,000 does not provide too much extra value compared to 1,000.

@jvdsn
Copy link
Author

jvdsn commented Mar 24, 2025

I am also very open to code style changes, I tried to follow the existing style but didn't see anything explicitly documented (e.g., line limit / wrapping)

@celic
Copy link
Collaborator

celic commented Mar 24, 2025

No formal code style guidelines. We use basic C# styling though I see several syntactic sugars on that page that didn't exist when we started the project.

It is strongly preferred for unit tests to finish in a matter of minutes or seconds. It is OK to define longer running tests but they should be identified with the [LongRunningCryptoTest] attribute. The iterated tests on 1000 iterations make sense here.

@celic
Copy link
Collaborator

celic commented Mar 24, 2025

Note if/when we accept this PR, it will not be merged here. This is an external repository used for our production code once published. We maintain an internal repository where this would be merged. Just a heads up if anyone looks at this PR later to see why it was "closed" versus "merged".

@jvdsn
Copy link
Author

jvdsn commented Mar 28, 2025

It is strongly preferred for unit tests to finish in a matter of minutes or seconds. It is OK to define longer running tests but they should be identified with the [LongRunningCryptoTest] attribute. The iterated tests on 1000 iterations make sense here.

Right now the 1,000 iterations finish in about 5 seconds on my machine. That test case is identified using [LongRunningCryptoTest] though, I just didn't have time to test the full 1,000,000 iterations yet. A naive extrapolation of the 1,000 iterations runtime indicates it should take about 1.4 hours, which would make [LongRunningCryptoTest] on that test case justified.

@jvdsn
Copy link
Author

jvdsn commented Mar 28, 2025

@celic additional tests were implemented in a2d3314

@livebe01 due to the force push, your commit to add the xecdh solution was overwritten. I can try to add it again, or maybe you can add it after this PR is merged?

@jvdsn jvdsn marked this pull request as ready for review March 29, 2025 00:26
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

Successfully merging this pull request may close these issues.

2 participants