Skip to content

Conversation

scott-xu
Copy link
Collaborator

@scott-xu scott-xu commented Apr 4, 2024

This PR adds support for [email protected] and [email protected] described in https://datatracker.ietf.org/doc/html/rfc5647

Resolves #792
Contributes to #1356
Resolves #774
Resolves #773
Resolves #555
Resolves #477
Resolves #994
Closes #877

Notes:

  1. This PR does not add support for [email protected] 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 Support AesGcm cipher #1364. Then when I realize there's no direct way to implement [email protected], 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: [email protected],aes128-ctr,aes192-ctr,aes256-ctr,[email protected],[email protected]
    debug2: ciphers stoc: [email protected],aes128-ctr,aes192-ctr,aes256-ctr,[email protected],[email protected]
    
    Although Dictionary's order is not defined, from observation, it is in the same order with add. Anyway that would be another topic, see Dictionary enumeration order is relied upon in ConnectionInfo despite being undefined behaviour #719

@scott-xu scott-xu requested a review from Rob-Hague April 4, 2024 08:54
@scott-xu scott-xu self-assigned this Apr 4, 2024
@zybexXL zybexXL mentioned this pull request Apr 4, 2024
@Rob-Hague
Copy link
Collaborator

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")

scott-xu added 2 commits April 4, 2024 20:17
Guard AES-GCM with `NET6_0_OR_GREATER`.
Insert AES-GCM ciphers right after AES-CTR ciphers but before AES-CBC ciphers, which is similar with OpenSSH:
```
debug2: ciphers ctos: [email protected],aes128-ctr,aes192-ctr,aes256-ctr,[email protected],[email protected]
debug2: ciphers stoc: [email protected],aes128-ctr,aes192-ctr,aes256-ctr,[email protected],[email protected]
```
Although Dictionary's order is not defined, from observation, it is in the same order with add. Anyway that would be another topic.
@scott-xu scott-xu changed the title Add support for AES 128/256 GCM Ciphers Add support for AEAD AES 128/256 GCM Ciphers Apr 4, 2024
scott-xu added 2 commits April 4, 2024 20:24
…mance" for `ConnectionInfo.Encryptions`.

Test `Aes128Gcm` and `Aes256Gcm` only when `NET6_0_OR_GREATER`
@scott-xu scott-xu changed the title Add support for AEAD AES 128/256 GCM Ciphers Add support for AEAD AES 128/256 GCM Ciphers (.NET 6.0 onward) Apr 4, 2024
@scott-xu scott-xu changed the title Add support for AEAD AES 128/256 GCM Ciphers (.NET 6.0 onward) Add support for AEAD AES 128/256 GCM Ciphers (.NET 6.0 onward only) Apr 5, 2024
@scott-xu scott-xu marked this pull request as draft April 8, 2024 00:26
@scott-xu scott-xu marked this pull request as ready for review April 8, 2024 04:03
@scott-xu scott-xu marked this pull request as draft April 9, 2024 11:02
@scott-xu scott-xu marked this pull request as ready for review April 9, 2024 11:19
Copy link
Collaborator

@Rob-Hague Rob-Hague left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you

@Rob-Hague Rob-Hague merged commit 3dc3fc8 into sshnet:develop Apr 18, 2024
@Rob-Hague
Copy link
Collaborator

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

image

@scott-xu scott-xu deleted the aesgcm branch April 18, 2024 22:02
@scott-xu
Copy link
Collaborator Author

Thanks

1 similar comment
@Dlyftservies
Copy link

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants