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 curve25519-sha256 key exchange method #306

Merged
merged 7 commits into from
Dec 30, 2024
Merged

Conversation

scott-xu
Copy link
Contributor

@scott-xu scott-xu commented Dec 28, 2024

This PR adds support for curve25519-sha2 key exchange method described by https://datatracker.ietf.org/doc/html/rfc8731

Close #307

@scott-xu
Copy link
Contributor Author

The CI action is awaiting approval @tmds

@tmds tmds marked this pull request as ready for review December 28, 2024 13:02
@scott-xu scott-xu marked this pull request as draft December 28, 2024 16:46
@tmds
Copy link
Owner

tmds commented Dec 29, 2024

@scott-xu looks good! I'd like to share some code with the ECDHKeyExchange class by adding a base KeyExchange class. I'm going to highlight the regions of code that I think can be in the base class.

@scott-xu
Copy link
Contributor Author

That's exactly what I'm doing right now and is the reason why I convert it back to Draft.

@scott-xu
Copy link
Contributor Author

scott-xu commented Dec 29, 2024

I got some challenges when I try to put most logics in the base KeyExchange class and let derived class implement the difference.
For example, I tried to abstract ReadServerExchangeValue method and put the implementation in derived class. However it doesn't work because below reader is ref struct.

var reader = packet.GetReader();
reader.ReadMessageId(MessageId.SSH_MSG_KEX_ECDH_REPLY);
SshKey public_host_key = reader.ReadSshKey();
ReadServerExchangeValue(reader);
ReadOnlySequence<byte> exchange_hash_signature = reader.ReadStringAsBytes();
reader.ReadEnd();
return (
    public_host_key,
    exchange_hash_signature);
protected override void ReadServerExchangeValue(SequenceReader reader)
{
    q_s = reader.ReadStringAsECPoint();
}

@tmds
Copy link
Owner

tmds commented Dec 29, 2024

I got some challenges when I try to put most logics in the base KeyExchange class and let derived class implement the difference.

I don't want to try and split the key exchange in several abstract methods that a derived class then implements because of these challenges.

I would like to focus on the sections of code I commented on and let each derived class use them in their TryExchangeAsync implementation by calling the base class method.

@scott-xu scott-xu marked this pull request as ready for review December 29, 2024 14:30
src/Tmds.Ssh/KeyExchange.cs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
src/Tmds.Ssh/KeyExchange.cs Outdated Show resolved Hide resolved
src/Tmds.Ssh/KeyExchange.cs Outdated Show resolved Hide resolved
src/Tmds.Ssh/KeyExchange.cs Outdated Show resolved Hide resolved
@scott-xu
Copy link
Contributor Author

Might be good to use another PR for sntrup761x25519sha512 and mlkem768x25519sha256

@tmds tmds merged commit 234fc3e into tmds:main Dec 30, 2024
1 check passed
@tmds
Copy link
Owner

tmds commented Dec 30, 2024

@scott-xu, thank you for implementing curve25519-sha256 support!

@scott-xu scott-xu deleted the kex branch December 30, 2024 12:41
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.

Add support for curve25519-sha256 key exchange method
2 participants