search

sshnet / SSH.NET

SSH.NET is a Secure Shell (SSH) library for .NET, optimized for parallelism.
http://sshnet.github.io/SSH.NET/
MIT License
3.88k stars 917 forks source link

Add support for AEAD AES 128/256 GCM Ciphers (.NET 6.0 onward only) #1369

Closed scott-xu closed 2 months ago

scott-xu commented 2 months ago

This PR adds support for aes128-gcm@openssh.com and aes256-gcm@openssh.com described in https://datatracker.ietf.org/doc/html/rfc5647

Resolves https://github.com/sshnet/SSH.NET/issues/792 Contributes to https://github.com/sshnet/SSH.NET/issues/1356 Resolves https://github.com/sshnet/SSH.NET/issues/774 Resolves https://github.com/sshnet/SSH.NET/issues/773 Resolves https://github.com/sshnet/SSH.NET/issues/555 Resolves https://github.com/sshnet/SSH.NET/issues/477 Resolves https://github.com/sshnet/SSH.NET/issues/994 Closes https://github.com/sshnet/SSH.NET/pull/877

Notes:

  1. This PR does not add support for chacha20-poly1305@openssh.com described in https://datatracker.ietf.org/doc/html/draft-josefsson-ssh-chacha20-poly1305-openssh-00 because it requires ChaCha20 to encrypt/decrypt the packet length field. The BCL adds support for ChaCha20Ploy1305 since .NET 6.0, but no standalone ChaCha20 till now (2024-04-04). At the beginning, I created a branch named as "aes-gcm-and-chacha20-poly1305" in my fork and created a PR https://github.com/sshnet/SSH.NET/pull/1364. Then when I realize there's no direct way to implement chacha20-poly1305@openssh.com, I renamed the branch to aesgcm. The original PR is closed automatically after renaming. I have to create this new PR. Please refer the previous PR for review comment history. Thank @zybexXL @Rob-Hague for reviewing.
  2. The AES-GCM ciphers are inserted right after AES-CTR ciphers but before AES-CBC ciphers, which is kind of similar with OpenSSH: 
      debug2: ciphers ctos: chacha20-poly1305@openssh.com,aes128-ctr,aes192-ctr,aes256-ctr,aes128-gcm@openssh.com,aes256-gcm@openssh.com
  debug2: ciphers stoc: chacha20-poly1305@openssh.com,aes128-ctr,aes192-ctr,aes256-ctr,aes128-gcm@openssh.com,aes256-gcm@openssh.com

    Although Dictionary's order is not defined, from observation, it is in the same order with add. Anyway that would be another topic, see https://github.com/sshnet/SSH.NET/issues/719

Rob-Hague commented 2 months ago

Generally it looks good to me but I'll take a closer look soon.

One thing is that S.S.C.AesGcm has an IsSupported property, but only on >=NET 6. It probably makes sense to be checking that. I would suggest guarding AES-GCM support on #if NET6_0_OR_GREATER in order to keep it simple there (i.e. drop the support for AES-GCM on netstandard2.1). That would also mean only including the cipher in the ConnectionInfo if it is supported.

(side note: if you don't want to close #1356, replace "fix" with a non-keyword e.g. "contributes to")

Rob-Hague commented 2 months ago

fyi you can click this to see your PRs without needing to assign yourself:

image

scott-xu commented 2 months ago

Thanks

Dlyftservies commented 2 months ago

Thanks