Add support for OpenSSL PKCS#8 private key format #1496

scott-xu · 2024-09-16T13:42:55Z

This PR adds support for PKCS#8 private key format (in practice, OpenSSL is used to generate the keys)

There are several options to add support for PKCS#8 private key format:

Use System.Security.Cryptography.Pkcs. It is from Microsoft and it supports exactly the same targets frameworks as SSH.NET. However, it is shipped as NuGet only which means we need to add it as a dependency for all target frameworks.
Call ImportPkcs8PrivateKey or ImportFromEncryptedPem from relevant concrete class. Both methods applied to netstandard2.1+ and Core 3.0+. However, I couldn't find a way to detect the key type before call the methods, for example a static method to load PKCS#8 private key and returns BCL's AsymmetricAlgorithm.
Use BouncyCastle's PemReader to load the private key and return BouncyCastle's AsymmetricKeyParameter. Or use BouncyCastle's underlying PrivateKeyFactory to CreateKey / DecryptKey which also returns BouncyCastle's AsymmetricKeyParameter. Then we need to bridge BouncyCastle's AsymmetricKeyParameter with SSH.NET's Key. This option may add some complexity to map types and unnecessary memory allocations.
My finally proposal is to use BouncyCastle to decrypt the PCKS#8 private key, read the PrivateKeyInfo Der structure and then read the parameters and key manually with AsnReader and finally construct the SSH.NET relevant concrete Key class.
With this option, there's no extra NuGet dependency and performance is balanced.

I also considered using Impl pattern for PrivateKeyFile class, but haven't had time to try. We can continue evolving this PR or create a new PR if needs be.

We can also consider dropping support for OpenSSL traditional private key format and drop ssh.com private key format as they are old and weak.

Fix #1090

Rob-Hague

The chosen approach seems reasonable. I don't think see why any of DsaKey, EcdsaKey, RsaKey types need to change (in particular, breaking changes)

Rob-Hague · 2024-09-16T19:39:32Z

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

-        /// Initializes a new instance of the <see cref="DsaKey"/> class.
-        /// </summary>
-        /// <param name="privateKeyData">DER encoded private key data.</param>
-        public DsaKey(byte[] privateKeyData)


Is it necessary to remove this?

Same applies to the other key constructors

I remove it because there could be other DER data structures. The current Key constructors with byte array parameter assume the data is DER encoded OpenSSL traditional format which is not always true. It could be PKCS#8 format as well.

In practice, most package consumers are not necessarily call Key constructors directly. It might not bring huge impacts.

Anyway, to gracefully deprecate the constructors with byte array parameter, we can apply Obsolete attribute to the constructors and remove them in the future release. What do you think? @Rob-Hague

I am sorry, I do not think there is a reason to break the existing contract.

I agree there is probably not many people using these constructors, and if we were starting from scratch we might do it differently. And although we make breaking changes practically every release, I think we can justify each of them. I do not believe this one can be justified.

As library developers, when we make a breaking change we are telling the user: "we expect you to bear the pain for our work". As a user, I can sometimes accept that when the user experience is improved and when it is easy to react to the change. Otherwise I find it annoying and frankly inconsiderate from the developers.

Whether we make this change or not, we experience no pain, so we should not cause any pain to users. We can instead improve things here by clarifying in the documentation that this constructor expects an OpenSSL format.

Sorry I didn't explain well.
The current Key constructors with byte array parameter assume the data is DER encoded OpenSSL traditional PKCS#1 format.
OpenSSL use PKCS#8 by default. It only uses PKCS#1 when option "-traditional" is specified.
The newer "genpkey" command even doesn't show "-traditional" option in the guide.

I understand the situation, I just don't think it is worth deleting the constructor or changing its behaviour, unless you can somehow detect which format it is using while reading. If not, then the constructor should stay the same

After some more thinking, I'm leaning towards to just delete those constructors.

We removed old target frameworks. People who are using old frameworks should target to new frameworks. See #1109
We removed legacy algorithms. People who are using legacy algorithms should use new algorithms. See #1442

This change only affects people who are directly calling the constructor with PKCS#1 data. They should use PKCS#8 data instead.

Note: People who are using PrivateKeyFile are not affected. I believe most people are using PrivateKeyFile instead of each Key class.

This improvement does not require changing public API. It brings us zero benefits, and brings detriment to users

If everyone agrees, then I can add them back. Would you mind sharing your thoughts here? @drieseng @WojciechNagorski @mus65

I agree removing it is too much. But imho it should at least be obsoleted since it is ambigious and confusing (.NET 9 recently made a similiar API obsolete). But then there should be an alternative public API people can move to (I assume there is? I haven't checked the whole PR).

I agree removing it is too much. But imho it should at least be obsoleted since it is ambigious and confusing (.NET 9 recently made a similiar API obsolete). But then there should be an alternative public API people can move to (I assume there is? I haven't checked the whole PR).

Thanks for sharing the info. I know I'm not alone.

BTW, Microsoft calls their new design "one method, one purpose" which is exactly what I want to apply for method static string GetCurveOid(string curve_s).

Rob-Hague · 2024-09-16T19:40:05Z

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

@@ -207,34 +206,12 @@ public EcdsaKey(SshKeyData publicKeyData)
        /// <summary>
        /// Initializes a new instance of the <see cref="EcdsaKey"/> class.
        /// </summary>
-        /// <param name="curve">The curve name.</param>
+        /// <param name="curve">The curve oid.</param>


That's a nasty break. Let's try to avoid it

By reading the DER data, we get the curve oid string directly. It looks strange if we convert curve oid string to curve name in PrivateKeyFile and then convert name back to oid string in Ecdsakey.

Yeah, it's strange and unfortunate, but it's the publicly exposed contract, so my other comment applies here too. If you like, you can change it to accept either the name or oid

We need to either document the breaking change in the release notes, or keep the current behavior but increase the code complexity (accept either name or oid). 😢

It should be the latter. Does not seem that complex to me

diff --git a/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs b/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs index 70e2e1a9..6f275b20 100644 --- a/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs +++ b/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs @@ -271,17 +271,20 @@ private static Impl Import(string curve_oid, byte[] publickey, byte[]? privateke private static string GetCurveOid(string curve_s) { - if (string.Equals(curve_s, "nistp256", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp256", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P256_OID_VALUE) { return ECDSA_P256_OID_VALUE; } - if (string.Equals(curve_s, "nistp384", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp384", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P384_OID_VALUE) { return ECDSA_P384_OID_VALUE; } - if (string.Equals(curve_s, "nistp521", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp521", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P521_OID_VALUE) { return ECDSA_P521_OID_VALUE; }

Sure from coding perspective, it is not complex. It is just the method signature definition. Now the first parameter could be either name or oid. Would it break single responsibility principle?

test/Data/Key.PKCS8.DSA.pub

Rob-Hague · 2024-09-17T12:07:05Z

src/Renci.SshNet/PrivateKeyFile.cs

+        /// <returns>The <see cref="EcdsaKey" />.</returns>
+        private static EcdsaKey ParseECPrivateKey_SEC1(byte[] keyData)
+        {
+            var keyReader = new AsnReader(keyData, AsnEncodingRules.DER);


Based on the comment above, should this be

Suggested change

var keyReader = new AsnReader(keyData, AsnEncodingRules.DER);

var keyReader = new AsnReader(keyData, AsnEncodingRules.BER);

?

DERs are subset of BERs. I think DERs are enough here since the data is generated using DERs?

I am reading this part of the comment: "receivers SHOULD be prepared to handle Basic Encoding Rules (BER) and DER"

In my understanding, that means using AsnEncodingRules.BER to match the comment

Actually I'm confused about the doc. Do you know why it says "BER and DER".

I guess they are trying to be explicit but they do not make it any clearer by doing so

Rob-Hague

Please restore the public constructors

Sorry about the conflicts

Rob-Hague · 2024-09-19T05:56:24Z

src/Renci.SshNet/PrivateKeyFile.cs

+        /// <returns>The <see cref="EcdsaKey" />.</returns>
+        private static EcdsaKey ParseECPrivateKey_SEC1(byte[] keyData)
+        {
+            var keyReader = new AsnReader(keyData, AsnEncodingRules.DER);


I am reading this part of the comment: "receivers SHOULD be prepared to handle Basic Encoding Rules (BER) and DER"

In my understanding, that means using AsnEncodingRules.BER to match the comment

Rob-Hague · 2024-09-19T05:57:47Z

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

@@ -207,34 +206,12 @@ public EcdsaKey(SshKeyData publicKeyData)
        /// <summary>
        /// Initializes a new instance of the <see cref="EcdsaKey"/> class.
        /// </summary>
-        /// <param name="curve">The curve name.</param>
+        /// <param name="curve">The curve oid.</param>


It should be the latter. Does not seem that complex to me

diff --git a/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs b/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs index 70e2e1a9..6f275b20 100644 --- a/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs +++ b/src/Renci.SshNet/Security/Cryptography/EcdsaKey.cs @@ -271,17 +271,20 @@ private static Impl Import(string curve_oid, byte[] publickey, byte[]? privateke private static string GetCurveOid(string curve_s) { - if (string.Equals(curve_s, "nistp256", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp256", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P256_OID_VALUE) { return ECDSA_P256_OID_VALUE; } - if (string.Equals(curve_s, "nistp384", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp384", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P384_OID_VALUE) { return ECDSA_P384_OID_VALUE; } - if (string.Equals(curve_s, "nistp521", StringComparison.OrdinalIgnoreCase)) + if (string.Equals(curve_s, "nistp521", StringComparison.OrdinalIgnoreCase) + || curve_s == ECDSA_P521_OID_VALUE) { return ECDSA_P521_OID_VALUE; }

Rob-Hague · 2024-09-19T06:01:12Z

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

-        /// Initializes a new instance of the <see cref="DsaKey"/> class.
-        /// </summary>
-        /// <param name="privateKeyData">DER encoded private key data.</param>
-        public DsaKey(byte[] privateKeyData)


This improvement does not require changing public API. It brings us zero benefits, and brings detriment to users

…ivatekey-pkcs8

…esting

Rob-Hague

Looks good, thanks for the changes

scott-xu · 2024-09-21T14:47:32Z

TBH, I don't like my changes. Life is short. I may leave for a while.
Best wishes for SSH.NET

Rob-Hague · 2024-09-21T14:58:19Z

Understood @scott-xu. I know it has not always been smooth sailing, but you make valuable contributions nonetheless. Thanks

Add support for OpenSSL PKCS#8 private key format

e6d689a

Rob-Hague reviewed Sep 16, 2024

View reviewed changes

scott-xu added 2 commits September 17, 2024 08:30

Update comments

a2f4f55

Convert public key to ssh format

58f67ad

Rob-Hague reviewed Sep 17, 2024

View reviewed changes

scott-xu marked this pull request as ready for review September 18, 2024 00:18

scott-xu requested review from drieseng and WojciechNagorski as code owners September 18, 2024 00:18

Rob-Hague requested changes Sep 19, 2024

View reviewed changes

Merge branch 'develop' of https://github.com/scott-xu/SSH.NET into pr…

984c390

…ivatekey-pkcs8

scott-xu marked this pull request as draft September 19, 2024 13:12

scott-xu added 4 commits September 19, 2024 22:55

Convert existing keys instead of generate new keys; Use DataRow for t…

8be690c

…esting

Minimize the change

7097607

Merge branch develop into privatekey-pkcs8

623308f

Minimize the change

0c3bcae

scott-xu force-pushed the privatekey-pkcs8 branch from dfee3ef to 0c3bcae Compare September 20, 2024 15:18

scott-xu marked this pull request as ready for review September 20, 2024 15:21

Fix build

5f0869e

Rob-Hague approved these changes Sep 21, 2024

View reviewed changes

Rob-Hague merged commit 548ef23 into sshnet:develop Sep 21, 2024
1 check passed

scott-xu deleted the privatekey-pkcs8 branch November 21, 2024 15:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for OpenSSL PKCS#8 private key format #1496

Add support for OpenSSL PKCS#8 private key format #1496

scott-xu commented Sep 16, 2024 •

edited

Loading

Rob-Hague left a comment

Rob-Hague Sep 16, 2024

scott-xu Sep 17, 2024

Rob-Hague Sep 17, 2024

scott-xu Sep 17, 2024

Rob-Hague Sep 17, 2024

scott-xu Sep 17, 2024 •

edited

Loading

Rob-Hague Sep 19, 2024

scott-xu Sep 19, 2024

mus65 Sep 19, 2024

scott-xu Sep 21, 2024

Rob-Hague Sep 16, 2024

scott-xu Sep 17, 2024

Rob-Hague Sep 17, 2024

scott-xu Sep 17, 2024

Rob-Hague Sep 19, 2024

scott-xu Sep 19, 2024

Rob-Hague Sep 17, 2024

scott-xu Sep 17, 2024

Rob-Hague Sep 19, 2024

scott-xu Sep 19, 2024

Rob-Hague Sep 19, 2024

Rob-Hague left a comment

Rob-Hague Sep 19, 2024

Rob-Hague Sep 19, 2024

Rob-Hague Sep 19, 2024

Rob-Hague left a comment

scott-xu commented Sep 21, 2024

Rob-Hague commented Sep 21, 2024

	var keyReader = new AsnReader(keyData, AsnEncodingRules.DER);
	var keyReader = new AsnReader(keyData, AsnEncodingRules.BER);

Add support for OpenSSL PKCS#8 private key format #1496

Add support for OpenSSL PKCS#8 private key format #1496

Conversation

scott-xu commented Sep 16, 2024 • edited Loading

Rob-Hague left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scott-xu Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob-Hague left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rob-Hague left a comment

Choose a reason for hiding this comment

scott-xu commented Sep 21, 2024

Rob-Hague commented Sep 21, 2024

scott-xu commented Sep 16, 2024 •

edited

Loading

scott-xu Sep 17, 2024 •

edited

Loading