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

Replace Curve25519 class with X25519 Key Agreement #838

Merged
merged 2 commits into from
Feb 11, 2023

Conversation

exceptionfactory
Copy link
Contributor

This pull request addresses an unresolved issue in the Curve25519DH implementation related to the use of Bouncy Castle for Curve25519 Diffie-Hellman Key Agreement as defined in RFC 8731.

The existing implementation creates an instance of ECParameterSpec but does not use it for key agreement processing. The existing implementation also uses a ported implementation of the Curve25519 algorithm in the djb.Curve25519 class.

This pull request refactors the Curve25519DH to use the standard Java Cryptography KeyPairGenerator and KeyAgreement interfaces, which are instantiated in the parent DHBase class using the standard X25519 algorithm identifier.

The KeyPairGenerator implementation is based on the registered Security Provider, which is Bouncy Castle in normal operation. Java 11 introduced direct support for X25519, so this implementation approach provides greater flexibility for future versions of SSHJ using the standard Java Cryptography interfaces.

Changing to the standard interfaces removes the need for the ported djb.Curve25519 implementation class, and also removes direct references to Bouncy Castle classes from the Curve25519DH class. These changes pass existing Key Exchange integration tests, and also include a unit test with basic key generation operations.

@hierynomus
Copy link
Owner

This will require a few more changes, because now the Curve25519 is no longer compatible with Java7 which is the target compilation version. I'm fine with removing support for older java versions and making the minimum version java11 from now on, but then you will also need to update the build files are part of this PR.

@exceptionfactory
Copy link
Contributor Author

Thanks for the feedback @hierynomus!

To clarify the comment on Java 11, these changes work without issue on Java 8, so there is no need to make the minimum project version Java 11.

The net effect of these changes is to use the Bouncy Castle implementation of KeyAgreement and KeyPairGenerator using the X25519. The current approach to Key Exchange registration appears to require Bouncy Castle, but to clarify the original comment, this approach could enable this Key Exchange method on Java 11 without the need for Bouncy Castle down the road.

@exceptionfactory
Copy link
Contributor Author

On a related note, if you are interested in making Java 8 the minimum version in a separate PR, that might have some other benefits, but based on testing these changes with Java 8, that should not be required in this PR.

@codecov-commenter
Copy link

codecov-commenter commented Feb 11, 2023

Codecov Report

Merging #838 (a343417) into master (154a202) will increase coverage by 0.19%.
The diff coverage is 92.59%.

@@             Coverage Diff              @@
##             master     #838      +/-   ##
============================================
+ Coverage     64.89%   65.09%   +0.19%     
+ Complexity     1465     1434      -31     
============================================
  Files           210      209       -1     
  Lines          8523     8061     -462     
  Branches        780      752      -28     
============================================
- Hits           5531     5247     -284     
+ Misses         2582     2407     -175     
+ Partials        410      407       -3     
Impacted Files Coverage Δ
...a/net/schmizz/sshj/transport/kex/Curve25519DH.java 92.85% <92.30%> (-7.15%) ⬇️
...t/schmizz/sshj/transport/kex/Curve25519SHA256.java 90.00% <100.00%> (ø)
.../sshj/transport/random/SingletonRandomFactory.java 75.00% <0.00%> (-25.00%) ⬇️
...a/net/schmizz/sshj/transport/random/JCERandom.java 77.27% <0.00%> (-9.10%) ⬇️
...main/java/net/schmizz/sshj/xfer/scp/SCPEngine.java 73.01% <0.00%> (-3.18%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

3 participants