In memory key support

[jborean93]

Right now the only way to use a key file is to specify the filepath of the key. It would be great to support a key that is either specified as a string or with the PrivateKey type itself. The latter would help to support things like using a key stored in a yubikey or other HSM like device.

[tmds]

Sounds good.

API proposal:

PrivateKeyCredential(byte[] key);

[jborean93]

As well as that I was potentially thinking more along the lines of

PrivateKeyCredential(RSA key);
PrivateKeyCredential(ECDsa key);
PrivateKeyCredential(string content, bool isPath = true);

The first two would allow someone to specify the Rsa/ECDsa object directly allowing them to create it from their own source. You could simplify it and make the overload AsymmetricAlgorithm but then you'll still have to check the subtype is something we can utilise. Some benefits of offering this is so the key could be retrieved from;

• The Windows certificate store attached to a cert
  • Windows can also store the private key by itself through a Win32 API and have it accessible by name but that's a bit more difficult
• Build the key object through one of the many Import* methods on each type rather than be constrained by what this library can do
• Source a key from Azure KeyVault or other HSM like devices like a Yubikey
• Another way to load the key from memory depending on whatever the client desires

We could even show examples of some of the above which may require 3rd party APIs but at least they could implement it in their own code which calls Tmds.Ssh.

The last one I'm not sure on, it'll most be similar to your byte[] key option just a more convenient way to specify a PEM file by using a string. It may clutter the API a bit more, maybe it would be better as a static method like static PrivateKeyCredential CreateFromString(string pem, string? password); or something like that.

[tmds]

PrivateKeyCredential(RSA key);
PrivateKeyCredential(ECDsa key);

I find these very advanced for an end-user to deal with, so I think we shouldn't add them unless there is a more clear demand.

PrivateKeyCredential(string content, bool isPath = true);

The isPath to change the meaning of content isn't one that is typically used in BCL APIs. We should have separate methods:

Let's start with:

PrivateKeyCredential(ReadOnlySpan<char> rawData, string? password = null)

Additional constructors or static create methods can be added later when users make requests for them.

[tmds]

For reference: X509Certificate2 also has different constructors: some accepting file paths and others accepting content.

[tmds]

PrivateKeyCredential(RSA key);
PrivateKeyCredential(ECDsa key);

For these, we should use a signature like:

PrivateKeyCredential(Func<RSA> key);
PrivateKeyCredential(Func<ECDsa> key);

This enables us to create and dispose the key when needed.

[jborean93]

Sorry I haven't replied yet, I'm hoping to look into the ED25519 work to see what is involved there to figure out a nice solution for providing the AsymmetricAlgorithm object that'll work across all the keys.

[jborean93]

This enables us to create and dispose the key when needed.

Wouldn't it just be simpler to let the caller manage the key disposal rather than us do it. It'll mean they don't have to build the RSA/EDCSA object on each call and can just provide the keys directly.

This does bring up a pain point I didn't consider in the use case I was looking at here which is using a key store in Azure Key Vault. The signing operation should technically be done over an async task to avoid blocking so even this API probably won't work too well as the RSA/ECDSA signing calls are sync only.

[tmds]

Wouldn't it just be simpler to let the caller manage the key disposal rather than us do it. It'll mean they don't have to build the RSA/EDCSA object on each call and can just provide the keys directly.

If we reference an IDisposable from SshClientSettings then the settings become valid only for a specific scope.

The settings are meant to hold the configuration for an SshClient without any specific scope.

The signing operation should technically be done over an async task to avoid blocking so even this API probably won't work too well as the RSA/ECDSA signing calls are sync only.

We can update the API to allow async operations.

This does bring up a pain point I didn't consider in the use case I was looking at here which is using a key store in Azure Key Vault.

How does the interaction with the key vault look? What does it return?

[jborean93]

How does the interaction with the key vault look? What does it return?

I've built a POC with some local changes where PrivateKeyCredential can accept Func<RSA> or Func<ECDsa> as proposed. Here is a gist with the project and some PowerShell code that can generate the keys in a keyvault https://gist.github.com/jborean93/1ab75303c9cc45cfb383c119f2a08fe0.

This works for both RSA and ECDsa keys stored in Azure and relies on the RBAC and authentication in the Azure libs to secure the key. The way it works is you subclass the RSA or ECDsa classes and implement you own export parameters and signing methods that are used during authentication. This allows the key to never leave the HSM and the only data sent across the wire is the hash to sign.

The same functionality could be expanded for things like keys stored in any HSM like a Yubikey, they would have to implement their own API but it is certainly possible to do so with RSA/ECDsa.

The issues I see is

• This could block on IO and the RSA/ECDsa algorithms are sync only
  • A new API that can return the pub key bytes and signed bytes run as a Task would be needed
• This won't support Ed25519 keys as there is no BCL implementation
  • Azure KeyVault doesn't support this key type yet (probably waiting on .NET)
  • The above API could potentially solve this issue as well

[tmds]

This allows the key to never leave the HSM and the only data sent across the wire is the hash to sign.

Nice!

This could block on IO and the RSA/ECDsa algorithms are sync only

For async support, we need to change the signatures to:

PrivateKeyCredential(Func<CancellationToken, ValueTask<RSA>> key);
PrivateKeyCredential(Func<CancellationToken, ValueTask<ECDsa>> key);

The CancellationToken gets passed to the crypto class:

sealed class AzureRSA : RSA
{
    public AzureRSA(CryptographyClient client, RSAParameters publicParameters, CancellationToken cancellationToken)
    {
        _cancellationToken = cancellationToken;

and

_cryptoClient.Sign(sigAlgo, hash).Signature

becomes:

_cryptoClient.SignAsync(sigAlgo, hash, _cancellationToken).GetAwaiter().GetResult().Signature

This enables us to respect the timeout/cancellationToken which is what we care most about.

It does block a threadpool thread during the signing, but I think we can live with that.

[tmds]

If we'd want to do better, we'd need to provide an alternative abstraction to RSA/ECDsa that enables async signing and, at this time, I think will be quite some work compared to accepting the downside of blocking a threadpool thread.

After looking more closely at the example, I think we may want to add:

protected PrivateKeyCredential(Func<CancellationToken, ValueTask<IDisposable>> loadKey);

so you could:

class AzureKeyCredential : PrivateKeyCredential { ... }

The object returned by loadKey should be one of the expected types: RSA/ECDsa.

[jborean93]

If we'd want to do better, we'd need to provide an alternative abstraction to RSA/ECDsa that enables async signing and, at this time, I think will be quite some work compared to accepting the downside of blocking a threadpool thread.

Yea makes sense, probably not worth complicating and tying yourself into an API if there isn't a major reason to do so right now.

What your proposing with the cancellable task makes sense to me and sounds like a good middle ground.

[tmds]

@jborean93 let me know if you're interested to implement this, otherwise I'll take a shot at it in the coming week. If you'd like to implement it, I'll give some implementation hints.

[jborean93]

I think I gotta take a bit of a break for now, happy to review the changes if you get to it though :)

With all the recent changes I think the only remaining thing left I was looking for right now was ssh-agent support but that's probably quite a decent chunk of code so I won't be getting to it anytime soon.

[tmds]

This is part of 0.6.0 which was just uploaded to nuget.org.