RSA SHA-2 implementation proposal

[Rob-Hague]

Background

The public key algorithm ssh-rsa which uses RSA and the SHA-1 hash algorithm, is considered deficient. OpenSSH version 8.8 (released on 2021-09-26) disables ssh-rsa by default. See RFC 8332 and the OpenSSH 8.8 release notes.

This library does not currently support the stronger public key algorithms using RSA with SHA-2 (rsa-sha2-256 and rsa-sha2-512) resulting in Host key algorithm negotiation failed or similar. It is probably the number one problem by number of issues and comments.

There is a workaround/solution provided in #825, #971 (and subsequently #1156). The workaround creates new RsaKey and RsaDigitalSignature classes for SHA-256 (and presumably more for SHA-512). The duplication is made understandably due to the library's coupling between the Key and DigitalSignature types (see also #1156 (comment)). The coupling is also understandable because until now one type of key has only had one type of signature. However, the workaround is IMO not adequate for the library implementation.

This issue is a proposal for the implementation. I have opened PR #1170 to allow specifying the hash algorithm in the RsaDigitalSignature constructor. This proposal builds on that.

Problem 1: specifying the hash algorithm when instantiating a KeyHostAlgorithm

The KeyHostAlgorithm type performs signing/verification and (de)serialisation of the data to the binary message format. A KeyHostAlgorithm takes a Key, and the signature is performed via the Key.Sign and Key.VerifySignature methods, which are non-virtual and simply forward to the DigitalSignature.Sign and DigitalSignature.Verify methods. Because KeyHostAlgorithm just accepts a Key, there is no way to specify the signature type.

Solution 1: add RsaKey constructors taking a factory method

We add two new constructors to RsaKey taking a Func<RsaKey, RsaDigitalSignature> parameter. When the DigitalSignature getter is invoked, if the delegate has been supplied then it will be invoked with a parameter of this, and the RsaDigitalSignature returned from the delegate is returned from the getter. If the delegate has not been supplied, the implementation will keep its current behaviour of instantiating a SHA-1 RsaDigitalSignature.

public class RsaKey : Key, IDisposable
{
    // Existing
    public RsaKey();
    public RsaKey(byte[] data);
    public RsaKey(BigInteger modulus, BigInteger exponent, BigInteger d, BigInteger p, BigInteger q, BigInteger inverseQ)

    // Proposed
+   public RsaKey(Func<RsaKey, RsaDigitalSignature> digitalSignatureFactory);
+   public RsaKey(byte[] data, Func<RsaKey, RsaDigitalSignature> digitalSignatureFactory);
}

Usage

KeyHostAlgorithm alg = new("rsa-sha2-256", new RsaKey(key => new RsaDigitalSignature(key, HashAlgorithmName.SHA256)));

Advantages

• No breaking changes
• The changes are limited to RSA i.e. only where it is needed

Disadvantages

• The RsaKey type is concerned with the implementation of the signature. (Although it is already)

Solution 2: supply a DigitalSignature instance to KeyHostAlgorithm

By adding a settable DigitalSignature property or a new constructor parameter to KeyHostAlgorithm, we can allow specifying the signature to be used.

Usage

RsaKey key = new();
KeyHostAlgorithm alg = new("rsa-sha2-256", key, new RsaDigitalSignature(key, HashAlgorithmName.SHA256));

Advantages

• Non-breaking, we can continue to sign via the Key if a DigitalSignature has not been supplied

Disadvantages

• We cannot be certain that the supplied DigitalSignature uses the supplied Key, which may be a source of bugs and issues.
• Having two ways to perform the signature (via the Key and via the DigitalSignature) may be a source of confusion.

Solution 3: decouple Key and DigitalSignature

We remove the signature functionality from Key and obsolete its DigitalSignature property. KeyHostAlgorithm would then gain a DigitalSignature property and constructors taking a DigitalSignature instead of a Key. DigitalSignature would gain a Key property. The old constructors would be obsoleted akin to:

+[Obsolete("Use the KeyHostAlgorithm(string, DigitalSignature) ctor in order to specify the digital signature")]
public KeyHostAlgorithm(string name, Key key)
    : base(name)
{
    Key = key;
+   // Key.DigitalSignature is changed from 'protected abstract' to 'protected internal virtual'
+   DigitalSignature = key.DigitalSignature
+       ?? throw new InvalidOperationException($"No {nameof(DigitalSignature)} found. This constructor is obselete.");
}

Usage

RsaKey key = new();
KeyHostAlgorithm alg = new("rsa-sha2-256", new RsaDigitalSignature(key, HashAlgorithmName.SHA256));

Advantages

• The two-way, chicken-and-egg dependency between Key and DigitalSignature no longer exists

Disadvantages

• Big breaking change
• The changes would apply unnecessarily to all key algorithms, not just RSA

Problem 2: specifying multiple host algorithms for one private key

An RSA private key should be able to be used for multiple host key algorithms (ssh-rsa, rsa-sha2-256 and rsa-sha2-512). Currently, PrivateKeyFile implements the IPrivateKeySource interface:

public interface IPrivateKeySource
{
    HostAlgorithm HostKey { get; }
}

which is what's supplied and enumerated during authentication:

SSH.NET/src/Renci.SshNet/PrivateKeyAuthenticationMethod.cs

Line 41 in 9f1699b

public PrivateKeyAuthenticationMethod(string username, params IPrivateKeySource[] keyFiles)

Solution 1: introduce a new interface

We add a new interface which can provide multiple host algorithms:

// See comments on naming below
+public interface IExtendedPrivateKeySource : IPrivateKeySource
+{
+   IReadOnlyList<HostAlgorithm> HostKeys { get; }
+}

-public class PrivateKeyFile : IPrivateKeySource, IDisposable
+public class PrivateKeyFile : IExtendedPrivateKeySource, IDisposable
{ }

The enumeration of IPrivateKeySources during authentication would make a check for whether the instance is an IExtendedPrivateKeySource and then enumerate its HostKeys too. The HostKeys property is an IReadOnlyList to allow counting and indexing such that the HostAlgorithms can be supplied in order of preference. IExtendedPrivateKeySource inherits IPrivateKeySource because PrivateKeyAuthenticationMethod would be the only place it is used in the library, meaning every implementation of IExtendedPrivateKeySource would have to implement IPrivateKeySource anyway. IPrivateKeySource.HostKey would generally defer to IExtendedPrivateKeySource .HostKeys[0] representing the most preferred algorithm.

On naming, the IPrivateKeySource is IMO named slightly awkwardly: functionally, it is an abstraction of something which provides a HostAlgorithm, with no explicit relevance to a private key. And if we wanted such an abstraction for a different use, we would either be using this interface having an unrelated name, or an interface with the same definition but a different name. Both of these would be strange.

The interface was introduced in bc99ada (March 22) meaning it does not exist in the latest released package, but there were a number of requests for it so it is safest to assume that it is in use in local builds.

While I would strongly prefer not to make any breaking changes in this feature, I would be comfortable with an exception to rename IPrivateKeySource to something more relevant, like IHostAlgorithmProvider with an obsoletion message at error severity, before the next release. The new interface proposed above would be called IHostAlgorithmsProvider. This solution 1 includes the rename.

Advantages

• Suitable, future-proof names for interfaces

Disadvantages

• Breaks people implementing IPrivateKeySource in local builds
• Inheriting IPrivateKeySource in the new interface may be awkward and unnecessary (I do not feel strongly about it)

Solution 1b: the same as solution 1 but without renaming IPrivateKeySource

We should then decide whether to keep the new interface in line with the naming e.g. IExtendedPrivateKeySource or go the more generic route of IHostAlgorithmsProvider.

Advantages

• Breaks nobody

Disadvantages

• We are left with strangely named interface(s) which do not match their function

Solution 2: add HostKeys property to PrivateKeyFile only

And then during authentication, check if the IPrivateKeySource is a concrete PrivateKeyFile and enumerate its HostKeys

Disadvantages

• Defeats the purpose of the interface, which was well-requested.

Solution 3: change the IPrivateKeySource interface

Change the HostKey property to a collection, or add the collection property directly to the interface.

Advantages

• Avoids the need for another interface

Disadvantages

• Breaks people implementing IPrivateKeySource in local builds (even more than a rename)

---

Thanks for reading down to here. My personal preference is for solution 1 to problem 1, and solution 1a to problem 2. I would be willing to implement that relatively quickly.

[Rob-Hague]

Despite my initial preference for solution 1 (the factory delegate) as a targeted, non-breaking change, it feels somewhat unsatisfying and fudge-y. I have been thinking of a variant combining solutions 2 and 3:

Like in solution 3, we do change Key's DigitalSignature property to protected internal virtual, but we do not obsolete it, instead documenting it as a possibly null-returning, default implementation of a DigitalSignature. We either remove the Sign and VerifySignature methods from Key (breaking, less confusing), or we keep them but make them throw when DigitalSignature is null (less breaking, more confusing and quite an unfriendly API).

Then, like in solution 2, we add constructors to KeyHostAlgorithm taking a DigitalSignature and a corresponding property:

public class KeyHostAlgorithm : HostAlgorithm
{
+   public KeyHostAlgorithm(string name, Key key, DigitalSignature digitalSignature);
+   public KeyHostAlgorithm(string name, Key key, byte[] data, DigitalSignature digitalSignature);
+   public DigitalSignature DigitalSignature { get; }
}

This property will be used for signing. The existing constructors will set the property by accessing key.DigitalSignature and throwing if it is null.

This feels like less fudge than solution 1, less aggressive than solution 3, and slightly less confusing than solution 2 by no longer forcing an implementation of Key.DigitalSignature which may not be used. But it is still confusing to have the Sign and VerifySignature methods on Key especially if they might now throw without being able to tell in advance. So ideally they would at least be obsoleted.

---

As for problem 2, I am warming to solution 3 and just taking the break before IPrivateKeySource makes a release:

+[Obsolete("Use IHostAlgorithmsProvider instead. See https://github.com/sshnet/SSH.NET/issues/1174 for details.",
+   error: true)]
+[EditorBrowsable(EditorBrowsableState.Never)]
public interface IPrivateKeySource
{
    HostAlgorithm HostKey { get; }
}

+public interface IHostAlgorithmsProvider
+{
+   IReadOnlyList<HostAlgorithm> HostAlgorithms { get; }
+}

-public class PrivateKeyFile : IPrivateKeySource, IDisposable
+public class PrivateKeyFile : IHostAlgorithmsProvider, IDisposable
{ }

in order to keep things simple while we can. Tagging @darinkes, @Kim-SSi as prior implementers of this interface in case you have any thoughts there.

[Rob-Hague]

We either remove the Sign and VerifySignature methods from Key (breaking, less confusing), or we keep them but make them throw when DigitalSignature is null (less breaking, more confusing and quite an unfriendly API).

Ok ignore my musings there. We can just change Key.DigitalSignature from protected abstract to protected internal abstract instead of protected internal virtual and then we are not breaking and not introducing unfriendly behaviour to Key.Sign and Key.VerifySignature. Key.DigitalSignature can be documented as providing a 'default' signature and KeyHostAlgorithm.DigitalSignature will take its value when one hasn't been explicitly supplied in the KeyHostAlgorithm constructor.

[WojciechNagorski]

Awesome description. I need some time to familiarize myself with this.

[WojciechNagorski]

Version 2023.0.0 has been published https://www.nuget.org/packages/SSH.NET/2023.0.0