Make keys immutable (#1264)

This makes it easier to reason about Key instances in e.g.
DigitalSignature implementations, because we know that the
Key is initialised with its data and will not change.

Co-authored-by: Wojciech Nagórski <wojtpl2@gmail.com>

Author: Rob-Hague

src/Renci.SshNet/Common/SshData.cs

@@ -231,7 +231,7 @@ protected ulong ReadUInt64()
         /// <returns>
         /// The <see cref="string"/> that was read.
         /// </returns>
-        protected string ReadString(Encoding encoding)
+        protected string ReadString(Encoding encoding = null)
         {
             return _stream.ReadString(encoding);
         }

src/Renci.SshNet/Common/SshDataStream.cs

@@ -249,12 +249,14 @@ public ulong ReadUInt64()
         /// <summary>
         /// Reads the next <see cref="string"/> data type from the SSH data stream.
         /// </summary>
-        /// <param name="encoding">The character encoding to use.</param>
+        /// <param name="encoding">The character encoding to use. Defaults to <see cref="Encoding.UTF8"/>.</param>
         /// <returns>
         /// The <see cref="string"/> read from the SSH data stream.
         /// </returns>
-        public string ReadString(Encoding encoding)
+        public string ReadString(Encoding encoding = null)
         {
+            encoding ??= Encoding.UTF8;
+
             var length = ReadUInt32();
 
             if (length > int.MaxValue)

src/Renci.SshNet/ConnectionInfo.cs

@@ -396,16 +396,16 @@ public ConnectionInfo(string host, int port, string username, ProxyTypes proxyTy
 
             HostKeyAlgorithms = new Dictionary<string, Func<byte[], KeyHostAlgorithm>>
                 {
-                    { "ssh-ed25519", data => new KeyHostAlgorithm("ssh-ed25519", new ED25519Key(), data) },
-                    { "ecdsa-sha2-nistp256", data => new KeyHostAlgorithm("ecdsa-sha2-nistp256", new EcdsaKey(), data) },
-                    { "ecdsa-sha2-nistp384", data => new KeyHostAlgorithm("ecdsa-sha2-nistp384", new EcdsaKey(), data) },
-                    { "ecdsa-sha2-nistp521", data => new KeyHostAlgorithm("ecdsa-sha2-nistp521", new EcdsaKey(), data) },
+                    { "ssh-ed25519", data => new KeyHostAlgorithm("ssh-ed25519", new ED25519Key(new SshKeyData(data))) },
+                    { "ecdsa-sha2-nistp256", data => new KeyHostAlgorithm("ecdsa-sha2-nistp256", new EcdsaKey(new SshKeyData(data))) },
+                    { "ecdsa-sha2-nistp384", data => new KeyHostAlgorithm("ecdsa-sha2-nistp384", new EcdsaKey(new SshKeyData(data))) },
+                    { "ecdsa-sha2-nistp521", data => new KeyHostAlgorithm("ecdsa-sha2-nistp521", new EcdsaKey(new SshKeyData(data))) },
 #pragma warning disable SA1107 // Code should not contain multiple statements on one line
-                    { "rsa-sha2-512", data => { var key = new RsaKey(); return new KeyHostAlgorithm("rsa-sha2-512", key, data, new RsaDigitalSignature(key, HashAlgorithmName.SHA512)); } },
-                    { "rsa-sha2-256", data => { var key = new RsaKey(); return new KeyHostAlgorithm("rsa-sha2-256", key, data, new RsaDigitalSignature(key, HashAlgorithmName.SHA256)); } },
+                    { "rsa-sha2-512", data => { var key = new RsaKey(new SshKeyData(data)); return new KeyHostAlgorithm("rsa-sha2-512", key, new RsaDigitalSignature(key, HashAlgorithmName.SHA512)); } },
+                    { "rsa-sha2-256", data => { var key = new RsaKey(new SshKeyData(data)); return new KeyHostAlgorithm("rsa-sha2-256", key, new RsaDigitalSignature(key, HashAlgorithmName.SHA256)); } },
 #pragma warning restore SA1107 // Code should not contain multiple statements on one line
-                    { "ssh-rsa", data => new KeyHostAlgorithm("ssh-rsa", new RsaKey(), data) },
-                    { "ssh-dss", data => new KeyHostAlgorithm("ssh-dss", new DsaKey(), data) },
+                    { "ssh-rsa", data => new KeyHostAlgorithm("ssh-rsa", new RsaKey(new SshKeyData(data))) },
+                    { "ssh-dss", data => new KeyHostAlgorithm("ssh-dss", new DsaKey(new SshKeyData(data))) },
                 };
 
             CompressionAlgorithms = new Dictionary<string, Type>

src/Renci.SshNet/PrivateKeyFile.cs

@@ -573,12 +573,14 @@ private static Key ParseOpenSshV1Key(byte[] keyFileData, string passPhrase)
             switch (keyType)
             {
                 case "ssh-ed25519":
-                    // public key
-                    publicKey = privateKeyReader.ReadBignum2();
+                    // https://datatracker.ietf.org/doc/html/draft-miller-ssh-agent-11#section-3.2.3
 
-                    // private key
+                    // ENC(A)
+                    _ = privateKeyReader.ReadBignum2();
+
+                    // k || ENC(A)
                     unencryptedPrivateKey = privateKeyReader.ReadBignum2();
-                    parsedKey = new ED25519Key(publicKey.Reverse(), unencryptedPrivateKey);
+                    parsedKey = new ED25519Key(unencryptedPrivateKey);
                     break;
                 case "ecdsa-sha2-nistp256":
                 case "ecdsa-sha2-nistp384":

src/Renci.SshNet/Security/Cryptography/DsaKey.cs

@@ -1,4 +1,5 @@
 using System;
+
 using Renci.SshNet.Common;
 using Renci.SshNet.Security.Cryptography;
 
@@ -15,57 +16,27 @@ public class DsaKey : Key, IDisposable
         /// <summary>
         /// Gets the P.
         /// </summary>
-        public BigInteger P
-        {
-            get
-            {
-                return _privateKey[0];
-            }
-        }
+        public BigInteger P { get; }
 
         /// <summary>
         /// Gets the Q.
         /// </summary>
-        public BigInteger Q
-        {
-            get
-            {
-                return _privateKey[1];
-            }
-        }
+        public BigInteger Q { get; }
 
         /// <summary>
         /// Gets the G.
         /// </summary>
-        public BigInteger G
-        {
-            get
-            {
-                return _privateKey[2];
-            }
-        }
+        public BigInteger G { get; }
 
         /// <summary>
         /// Gets public key Y.
         /// </summary>
-        public BigInteger Y
-        {
-            get
-            {
-                return _privateKey[3];
-            }
-        }
+        public BigInteger Y { get; }
 
         /// <summary>
         /// Gets private key X.
         /// </summary>
-        public BigInteger X
-        {
-            get
-            {
-                return _privateKey[4];
-            }
-        }
+        public BigInteger X { get; }
 
         /// <summary>
         /// Gets the length of the key.
@@ -94,46 +65,70 @@ protected internal override DigitalSignature DigitalSignature
         }
 
         /// <summary>
-        /// Gets or sets the public.
+        /// Gets the DSA public key.
         /// </summary>
         /// <value>
-        /// The public.
+        /// An array whose values are:
+        /// <list>
+        /// <item><term>0</term><description><see cref="P"/></description></item>
+        /// <item><term>1</term><description><see cref="Q"/></description></item>
+        /// <item><term>2</term><description><see cref="G"/></description></item>
+        /// <item><term>3</term><description><see cref="Y"/></description></item>
+        /// </list>
         /// </value>
         public override BigInteger[] Public
         {
             get
             {
                 return new[] { P, Q, G, Y };
             }
-            set
-            {
-                if (value.Length != 4)
-                {
-                    throw new InvalidOperationException("Invalid public key.");
-                }
-
-                _privateKey = value;
-            }
         }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="DsaKey"/> class.
         /// </summary>
-        public DsaKey()
+        /// <param name="publicKeyData">The encoded public key data.</param>
+        public DsaKey(SshKeyData publicKeyData)
         {
-            _privateKey = new BigInteger[5];
+            if (publicKeyData is null)
+            {
+                throw new ArgumentNullException(nameof(publicKeyData));
+            }
+
+            if (publicKeyData.Name != "ssh-dss" || publicKeyData.Keys.Length != 4)
+            {
+                throw new ArgumentException($"Invalid DSA public key data. ({publicKeyData.Name}, {publicKeyData.Keys.Length}).", nameof(publicKeyData));
+            }
+
+            P = publicKeyData.Keys[0];
+            Q = publicKeyData.Keys[1];
+            G = publicKeyData.Keys[2];
+            Y = publicKeyData.Keys[3];
         }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="DsaKey"/> class.
         /// </summary>
-        /// <param name="data">DER encoded private key data.</param>
-        public DsaKey(byte[] data)
-            : base(data)
+        /// <param name="privateKeyData">DER encoded private key data.</param>
+        public DsaKey(byte[] privateKeyData)
         {
-            if (_privateKey.Length != 5)
+            if (privateKeyData is null)
+            {
+                throw new ArgumentNullException(nameof(privateKeyData));
+            }
+
+            var der = new DerData(privateKeyData);
+            _ = der.ReadBigInteger(); // skip version
+
+            P = der.ReadBigInteger();
+            Q = der.ReadBigInteger();
+            G = der.ReadBigInteger();
+            Y = der.ReadBigInteger();
+            X = der.ReadBigInteger();
+
+            if (!der.IsEndOfData)
             {
-                throw new InvalidOperationException("Invalid private key.");
+                throw new InvalidOperationException("Invalid private key (expected EOF).");
             }
         }
 
@@ -147,7 +142,11 @@ public DsaKey(byte[] data)
         /// <param name="x">The x.</param>
         public DsaKey(BigInteger p, BigInteger q, BigInteger g, BigInteger y, BigInteger x)
         {
-            _privateKey = new BigInteger[5] { p, q, g, y, x };
+            P = p;
+            Q = q;
+            G = g;
+            Y = y;
+            X = x;
         }
 
         /// <summary>

src/Renci.SshNet/Security/Cryptography/ED25519Key.cs

@@ -11,13 +11,7 @@ namespace Renci.SshNet.Security
     /// </summary>
     public class ED25519Key : Key, IDisposable
     {
-#pragma warning disable IDE1006 // Naming Styles
-#pragma warning disable SX1309 // Field names should begin with underscore
-        private readonly byte[] privateKey = new byte[Ed25519.ExpandedPrivateKeySizeInBytes];
-#pragma warning restore SX1309 // Field names should begin with underscore
-#pragma warning restore IDE1006 // Naming Styles
         private ED25519DigitalSignature _digitalSignature;
-        private byte[] _publicKey = new byte[Ed25519.PublicKeySizeInBytes];
         private bool _isDisposed;
 
         /// <summary>
@@ -32,20 +26,16 @@ public override string ToString()
         }
 
         /// <summary>
-        /// Gets or sets the public.
+        /// Gets the Ed25519 public key.
         /// </summary>
         /// <value>
-        /// The public.
+        /// An array with <see cref="PublicKey"/> encoded at index 0.
         /// </value>
         public override BigInteger[] Public
         {
             get
             {
-                return new BigInteger[] { _publicKey.ToBigInteger2() };
-            }
-            set
-            {
-                _publicKey = value[0].ToByteArray().Reverse().TrimLeadingZeros().Pad(Ed25519.PublicKeySizeInBytes);
+                return new BigInteger[] { PublicKey.ToBigInteger2() };
             }
         }
 
@@ -78,52 +68,46 @@ protected internal override DigitalSignature DigitalSignature
         /// <summary>
         /// Gets the PublicKey Bytes.
         /// </summary>
-        public byte[] PublicKey
-        {
-            get
-            {
-                return _publicKey;
-            }
-        }
+        public byte[] PublicKey { get; }
 
         /// <summary>
         /// Gets the PrivateKey Bytes.
         /// </summary>
-        public byte[] PrivateKey
-        {
-            get
-            {
-                return privateKey;
-            }
-        }
+        public byte[] PrivateKey { get; }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ED25519Key"/> class.
         /// </summary>
-        public ED25519Key()
+        /// <param name="publicKeyData">The encoded public key data.</param>
+        public ED25519Key(SshKeyData publicKeyData)
         {
-        }
+            if (publicKeyData is null)
+            {
+                throw new ArgumentNullException(nameof(publicKeyData));
+            }
 
-        /// <summary>
-        /// Initializes a new instance of the <see cref="ED25519Key"/> class.
-        /// </summary>
-        /// <param name="pk">pk data.</param>
-        public ED25519Key(byte[] pk)
-        {
-            _publicKey = pk.TrimLeadingZeros().Pad(Ed25519.PublicKeySizeInBytes);
+            if (publicKeyData.Name != "ssh-ed25519" || publicKeyData.Keys.Length != 1)
+            {
+                throw new ArgumentException($"Invalid Ed25519 public key data ({publicKeyData.Name}, {publicKeyData.Keys.Length}).", nameof(publicKeyData));
+            }
+
+            PublicKey = publicKeyData.Keys[0].ToByteArray().Reverse().TrimLeadingZeros().Pad(Ed25519.PublicKeySizeInBytes);
+            PrivateKey = new byte[Ed25519.ExpandedPrivateKeySizeInBytes];
         }
 
         /// <summary>
         /// Initializes a new instance of the <see cref="ED25519Key"/> class.
         /// </summary>
-        /// <param name="pk">pk data.</param>
-        /// <param name="sk">sk data.</param>
-        public ED25519Key(byte[] pk, byte[] sk)
+        /// <param name="privateKeyData">
+        /// The private key data <c>k || ENC(A)</c> as described in RFC 8032.
+        /// </param>
+        public ED25519Key(byte[] privateKeyData)
         {
-            _publicKey = pk.TrimLeadingZeros().Pad(Ed25519.PublicKeySizeInBytes);
             var seed = new byte[Ed25519.PrivateKeySeedSizeInBytes];
-            Buffer.BlockCopy(sk, 0, seed, 0, seed.Length);
-            Ed25519.KeyPairFromSeed(out _publicKey, out privateKey, seed);
+            Buffer.BlockCopy(privateKeyData, 0, seed, 0, seed.Length);
+            Ed25519.KeyPairFromSeed(out var publicKey, out var privateKey, seed);
+            PublicKey = publicKey;
+            PrivateKey = privateKey;
         }
 
         /// <summary>