OPENSSH KeyReader for more keys

[darinkes]

Add support to parse OpenSSH Keys with ECDSA 256/384/521 and RSA.

Change-Id: Iaa9cce0f2522e5fee377a82cb252f81f0b7cc563

[huobazi]

+1

[xivk]

Anything I can do to help with this pull request? Willing to put in some work, this is something I need.

[jons]

seconded for needing this feature.

[aibars]

Any idea when does a new release including these changes will be released?

[epenet]

@darinkes there is a conflict on the branch. Could you maybe rebase it?

[epenet]

@drieseng is there anything we can do to help getting this merged in?

[drieseng]

@darinkes LGTM, with some comments. Please update README.md as well.

Some other things I'd like to discuss:

KeyHostAlgorithm

To me, either:

• KeyHostAlgorithm remains public, and we update PrivateKeyFile.HostKey to return KeyHostAlgorithm.
  -or-
• We change KeyHostAlgorithm to internal.

IDisposable

In PrivateKeyFile, we currently keep a "copy" of Key around to be able to dispose it.
We could instead implement IDisposable on KeyHostAlgorithm.

Regardless where we dispose the key, we should also check if the key effectively implements IDisposable instead of always casting to IDisposable like we do now. This allows us to remove the dummy IDisposable implementation from EcdsaKey and ED25519Key.

PrivateKeyFile unit tests

Right now, the unit tests for PrivateKeyFile are limited to verifying whether we do not throw an exception when loading a given key format. Shouldn't we verify whether the key information is correctly read, and cover more failure conditions?

[chrduc]

@drieseng@darinkes what is the ETA for your openssh_format_rsa PR? We are wanting this change ASAP

[darinkes]

@drieseng@darinkes what is the ETA for your openssh_format_rsa PR? We are wanting this change ASAP

I hope to have some spare time this weekend to include the comments of @drieseng

[drieseng]

@chrduc Don't forget that we all have day jobs. Consider sponsoring @darinkes and/or SSH.NET to show us your appreciation :p

[chrduc]

@drieseng@darinkes what is the ETA for your openssh_format_rsa PR? We are wanting this change ASAP

I hope to have some spare time this weekend to include the comments of @drieseng

Thanks @darinkes

[darinkes]

Updated the PR with suggestions from @drieseng.
Would be awesome if someone could take this PR for a test-ride.

[darinkes]

@darinkes LGTM, with some comments. Please update README.md as well.

✅

PrivateKeyFile unit tests

Right now, the unit tests for PrivateKeyFile are limited to verifying whether we do not throw an exception when loading a given key format. Shouldn't we verify whether the key information is correctly read, and cover more failure conditions?

I would also like to use new keys on each run instead of hardcoded resources.

[drieseng]

I would also like to use new keys on each run instead of hardcoded resources.

I don't see a problem with the hardcoded resources. They should also allow for predictable results to verify against.
What would the advantage be of using new keys?

[drieseng]

@darinkes Can you have a look at my other comments here?

[drieseng]

Would be awesome if someone could take this PR for a test-ride.

When I have some time, I'll add integrations tests. For now these integration tests are still closed source :(
I should also find time to open source these.

[darinkes]

Some other things I'd like to discuss:

KeyHostAlgorithm

To me, either:

• KeyHostAlgorithm remains public, and we update PrivateKeyFile.HostKey to return KeyHostAlgorithm.
  -or-
• We change KeyHostAlgorithm to internal.

No opinion on that.

IDisposable

In PrivateKeyFile, we currently keep a "copy" of Key around to be able to dispose it.
We could instead implement IDisposable on KeyHostAlgorithm.

Regardless where we dispose the key, we should also check if the key effectively implements IDisposable instead of always casting to IDisposable like we do now. This allows us to remove the dummy IDisposable implementation from EcdsaKey and ED25519Key.

Ack, makes sense.

PrivateKeyFile unit tests

Right now, the unit tests for PrivateKeyFile are limited to verifying whether we do not throw an exception when loading a given key format. Shouldn't we verify whether the key information is correctly read, and cover more failure conditions?

Ack, this should be added as well.

I would also like to use new keys on each run instead of hardcoded resources.

I don't see a problem with the hardcoded resources. They should also allow for predictable results to verify against.
What would the advantage be of using new keys?

My concern is the padding, to ensure its done right hardcoded resources just test it works for those keys.
But maybe just overthinking it.