Use BCL CipherMode enum for AesCipher class by scott-xu · Pull Request #1564 · sshnet/SSH.NET

scott-xu · 2024-12-28T01:47:06Z

Create a dedicated AesCtrCipher class just like AesGcmCipher class.

Relate to #1560

Rob-Hague · 2025-01-11T14:47:46Z

Is there a benefit to this?

scott-xu · 2025-01-12T10:44:04Z

FYI: some previous discussion: #865 (comment)

scott-xu · 2025-01-12T10:46:35Z

The main purpose is to use BCL as possible as it can.

Rob-Hague · 2025-02-02T17:43:54Z

Thanks, I remember the previous discussion, but I don't think it is worth changing the API right now. Maybe at a later time

Copilot

Pull Request Overview

This PR refactors the cipher implementation to use the BCL CipherMode enum instead of a custom AesCipherMode enum, and creates a dedicated AesCtrCipher class similar to the existing AesGcmCipher class. The changes remove support for DES-EDE3-CFB encryption and eliminate custom cipher mode implementations in favor of BCL-provided functionality.

Replaced custom AesCipherMode enum with BCL's System.Security.Cryptography.CipherMode
Created dedicated AesCtrCipher class for CTR mode encryption
Removed DES-EDE3-CFB support and associated test data
Simplified cipher implementations by removing custom mode classes (CFB, CTR, OFB, CBC)

Reviewed Changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
`src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs`	Refactored to use BCL `CipherMode` and simplified implementation
`src/Renci.SshNet/Security/Cryptography/Ciphers/AesCtrCipher.cs`	New dedicated class for AES-CTR mode
`src/Renci.SshNet/Security/Cryptography/Ciphers/TripleDesCipher.cs`	Refactored to use BCL `CipherMode`
`src/Renci.SshNet/PrivateKeyFile.*.cs`	Updated cipher instantiation to use BCL `CipherMode`
`src/Renci.SshNet/ConnectionInfo.cs`	Updated cipher configurations to use new class constructors
`test/*/CipherTest*.cs`	Updated tests to use BCL `CipherMode` and new `AesCtrCipher` class
Multiple cipher mode files	Removed custom implementations (CFB, CTR, OFB, CBC, CipherMode base)
`test/Data/Key.RSA.Encrypted.Des.Ede3.CFB.*`	Removed test data for unsupported DES-EDE3-CFB

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

src/Renci.SshNet/Security/Cryptography/Ciphers/TripleDesCipher.cs

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs

scott-xu · 2025-11-08T12:37:14Z

Is there a benefit to this?

One other benefit is the reduction of code. Less code means less maintenance.

scott-xu · 2025-11-08T12:58:26Z

IMHO, SSH.NET should be the client of SSH but not a cryptography library. That's the reason why I hide AesCipher and TripleDesCipher.

scott-xu · 2025-11-08T13:27:47Z

Could you please have another review of this PR please? @Rob-Hague

Rob-Hague · 2025-11-13T20:14:18Z

I am fine with dropping BlockCipher (and CipherMode), but I would rather not change AesCipher until there is a compelling reason, otherwise it is just churn. If you can put it back, I'll merge the rest

scott-xu · 2025-11-14T00:00:46Z

The reason is

BCL CipherMode does not have CTR mode.
OFB and CFB is not used anywhere.

scott-xu · 2025-11-08T12:52:44Z

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs

    /// AES cipher implementation.
    /// </summary>
-    public sealed partial class AesCipher : BlockCipher, IDisposable
+    internal sealed class AesCipher : SymmetricCipher, IDisposable


Note: this is a breaking change of public API

scott-xu · 2025-11-08T12:53:23Z

src/Renci.SshNet/Security/Cryptography/Ciphers/TripleDesCipher.cs

    /// Implements 3DES cipher algorithm.
    /// </summary>
-    public sealed partial class TripleDesCipher : BlockCipher, IDisposable
+    internal sealed class TripleDesCipher : SymmetricCipher, IDisposable


This is a breaking change of public API

scott-xu · 2025-11-15T04:04:40Z

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs

-            if (mode == AesCipherMode.OFB)
-            {
-                // OFB is not supported on modern .NET
-                _impl = new BlockImpl(key, new OfbCipherMode(iv), pkcs7Padding ? new Pkcs7Padding() : null);
-            }


OFB is not used anywhere

scott-xu · 2025-11-15T04:04:58Z

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs

-            else if (mode == AesCipherMode.CFB)
-            {
-                // CFB not supported on NetStandard 2.1
-                _impl = new BlockImpl(key, new CfbCipherMode(iv), pkcs7Padding ? new Pkcs7Padding() : null);
-            }


CFB is not used anywhere

scott-xu · 2025-11-15T04:05:23Z

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs

-            else if (mode == AesCipherMode.CTR)
-            {
-                // CTR not supported by the BCL, use an optimized implementation
-                _impl = new CtrImpl(key, iv);
-            }


BCL CipherMode does not have CTR mode

scott-xu · 2025-11-15T13:39:05Z

BTW, I would suggest renaming Renci.SshNet.Security.Cryptography.SymmetricCipher to Renci.SshNet.Security.Cryptography.AsymmetricBlockCipher. Unfortunately, it is a public class. But it should be rarely used. What do you think? @Rob-Hague

… class; Create a dedicated AesCtrCipher class just like AesGcmCipher class.

scott-xu · 2025-11-22T09:26:14Z

I am fine with dropping BlockCipher (and CipherMode), but I would rather not change AesCipher until there is a compelling reason, otherwise it is just churn. If you can put it back, I'll merge the rest

BlockCipher and CipherMode are dropped. BlockSize property is moved to Cipher abstract class.
Can you please share more information about not changing AesCipher?

scott-xu · 2025-11-22T09:26:41Z

BTW, I would suggest renaming Renci.SshNet.Security.Cryptography.SymmetricCipher to Renci.SshNet.Security.Cryptography.AsymmetricBlockCipher. Unfortunately, it is a public class. But it should be rarely used. What do you think? @Rob-Hague

SymmertricCipher is dropped.

scott-xu marked this pull request as ready for review December 28, 2024 01:51

scott-xu requested review from Rob-Hague, WojciechNagorski and drieseng as code owners December 28, 2024 01:51

scott-xu force-pushed the bcl-ciphermode branch from 166cd93 to e02f749 Compare December 28, 2024 01:56

Rob-Hague mentioned this pull request May 2, 2025

Consider dropping hand-written cipher modes #1560

Open

scott-xu marked this pull request as draft May 5, 2025 09:28

scott-xu marked this pull request as ready for review May 5, 2025 12:23

WojciechNagorski requested a review from Copilot October 23, 2025 10:17

Copilot AI reviewed Oct 23, 2025

View reviewed changes

src/Renci.SshNet/Security/Cryptography/Ciphers/TripleDesCipher.cs Outdated Show resolved Hide resolved

src/Renci.SshNet/Security/Cryptography/Ciphers/AesCipher.cs Outdated Show resolved Hide resolved

scott-xu force-pushed the bcl-ciphermode branch from 1b1c639 to 86d3a29 Compare November 8, 2025 09:47

scott-xu force-pushed the bcl-ciphermode branch from 86d3a29 to 0a89a35 Compare November 8, 2025 12:49

scott-xu force-pushed the bcl-ciphermode branch from 0a89a35 to 2b4037b Compare November 8, 2025 13:11

scott-xu force-pushed the bcl-ciphermode branch from 2b4037b to 98c45ed Compare November 15, 2025 01:53

scott-xu commented Nov 15, 2025

View reviewed changes

scott-xu force-pushed the bcl-ciphermode branch 2 times, most recently from d4891a9 to e0c4906 Compare November 15, 2025 13:32

Use BCL CipherMode enum for AesCipher class just like TripleDesCipher…

2ebb953

… class; Create a dedicated AesCtrCipher class just like AesGcmCipher class.

scott-xu force-pushed the bcl-ciphermode branch from e0c4906 to 2ebb953 Compare November 22, 2025 09:21

ECB is weak and not used anywhere.

d8ffc06

scott-xu force-pushed the bcl-ciphermode branch from de35184 to d8ffc06 Compare November 23, 2025 04:01

Merge branch 'develop' into bcl-ciphermode

388ded2

Uh oh!

Conversation

scott-xu commented Dec 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Rob-Hague commented Jan 11, 2025

Uh oh!

scott-xu commented Jan 12, 2025

Uh oh!

scott-xu commented Jan 12, 2025

Uh oh!

Rob-Hague commented Feb 2, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

scott-xu commented Nov 8, 2025

Uh oh!

scott-xu commented Nov 8, 2025

Uh oh!

scott-xu commented Nov 8, 2025

Uh oh!

Rob-Hague commented Nov 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scott-xu commented Nov 14, 2025

Uh oh!

scott-xu Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

scott-xu Nov 8, 2025

Choose a reason for hiding this comment

Uh oh!

scott-xu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

scott-xu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

scott-xu Nov 15, 2025

Choose a reason for hiding this comment

Uh oh!

scott-xu commented Nov 15, 2025

Uh oh!

scott-xu commented Nov 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

scott-xu commented Nov 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

scott-xu commented Dec 28, 2024 •

edited

Loading

Rob-Hague commented Nov 13, 2025 •

edited

Loading

scott-xu commented Nov 22, 2025 •

edited

Loading