-
-
Notifications
You must be signed in to change notification settings - Fork 939
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for OpenSSL PKCS#8 private key format #1496
Changes from 5 commits
e6d689a
a2f4f55
58f67ad
984c390
8be690c
7097607
623308f
0c3bcae
5f0869e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
#nullable enable | ||
using System; | ||
using System.Diagnostics; | ||
using System.Formats.Asn1; | ||
using System.Numerics; | ||
using System.Security.Cryptography; | ||
using System.Text; | ||
|
@@ -204,34 +203,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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's a nasty break. Let's try to avoid it There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). 😢 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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;
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
/// <param name="publickey">Value of publickey.</param> | ||
/// <param name="privatekey">Value of privatekey.</param> | ||
public EcdsaKey(string curve, byte[] publickey, byte[] privatekey) | ||
{ | ||
_impl = Import(GetCurveOid(curve), publickey, privatekey); | ||
} | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="EcdsaKey"/> class. | ||
/// </summary> | ||
/// <param name="data">DER encoded private key data.</param> | ||
public EcdsaKey(byte[] data) | ||
{ | ||
var der = new AsnReader(data, AsnEncodingRules.DER).ReadSequence(); | ||
_ = der.ReadInteger(); // skip version | ||
|
||
var privatekey = der.ReadOctetString().TrimLeadingZeros(); | ||
|
||
var s0 = der.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 0, isConstructed: true)); | ||
var curve = s0.ReadObjectIdentifier(); | ||
|
||
var s1 = der.ReadSequence(new Asn1Tag(TagClass.ContextSpecific, 1, isConstructed: true)); | ||
var pubkey = s1.ReadBitString(out _); | ||
|
||
der.ThrowIfNotEmpty(); | ||
|
||
_impl = Import(curve, pubkey, privatekey); | ||
_impl = Import(curve, publickey, privatekey.TrimLeadingZeros()); | ||
} | ||
|
||
#pragma warning disable CA1859 // Use concrete types when possible for improved performance | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
-----BEGIN ENCRYPTED PRIVATE KEY----- | ||
MIIBrTBXBgkqhkiG9w0BBQ0wSjApBgkqhkiG9w0BBQwwHAQIjn9BgD9X0loCAggA | ||
MAwGCCqGSIb3DQIJBQAwHQYJYIZIAWUDBAEqBBB3Zthr23nQDulzKryFEUTFBIIB | ||
UDW8/IR0K5DRScH4Cl7HOoK20aR+TmUOGczE027RL++iosgk5rYUpIKn0pxIKM0U | ||
StFGTqLz3G+bEh/Bm2Vt03Qv0Q2QZoX2e1Vktt32X2cLBNzGWfEpLuCD4vG8QDRW | ||
uGkE1NHxJKQTJWQt/gwGituyhMThGoE3ZcuqeLmRlhUSgRccO6WJ2HkNOW7TM5RB | ||
QbeBXmYB1H5S3FjpRAvd2p9dEzDsyquQaltFM4kekIxGjwiw5WSd+KsCGXFLa2Y2 | ||
OXvcjRIIqGBJr+xvEVA86TNTfad+sKGqGUFszRmnGXA+VxEZju2OCpVhxTLEMX4Q | ||
2vYz9i8jE78tpx7C6PTKoJe5FTdlTatvWvYD5cvcbazPUjuZbraI9ha4XvNtERGC | ||
J0voz/7yeuNkW1ofxTUOu+snGhySC4AXkC44eZG4wUPfuQAswP8dFiQi2BthgVyP | ||
kA== | ||
-----END ENCRYPTED PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
-----BEGIN PRIVATE KEY----- | ||
MIIBSgIBADCCASsGByqGSM44BAEwggEeAoGBALVl3fae2O4qwsAK95SUShX0KMUN | ||
P+yl/uT3lGH9T/ZptnHSlrTxnTWXCl0g91KEeCaEnDDhLxm4aCv1Ag4B/yvcM4u3 | ||
4qkmaNLy2LiAxiqdobZcNG61Pqwqd5IDkp38LBsn8tmb12xu9NalpUfOiSEB1cyC | ||
r4zFZMrm0wtdyJQVAhUArvojZKn/2DgGI2Kx0ghxZlgHxGECgYAOVJ434UAR3Hn6 | ||
lA5nWNfFOuUVH3W7nJaP0FQJiIPx7GUbdxO9qtDNTbWkWL3c9qx5+B7Ole4xM7cv | ||
yXPrNQUYDHCFlS+Ue2x3IeJrkdfZkH9ePP25y5A0J4/c+8XXvQaj4zA5nfw13oy5 | ||
Ptyd7d3Kq5tEDM8KiVdIhwkXjUA3PQQWAhQYRjs5PgIpnqG/euBPPh7EDZcnXg== | ||
-----END PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
-----BEGIN ENCRYPTED PRIVATE KEY----- | ||
MIHsMFcGCSqGSIb3DQEFDTBKMCkGCSqGSIb3DQEFDDAcBAithDR1n5wCYQICCAAw | ||
DAYIKoZIhvcNAgkFADAdBglghkgBZQMEASoEEMSzAQ3FSZ5hQHbmb7K9aB0EgZCS | ||
RVfVmMp1SBllrUMvdMEz5Zwvthaa1/M3Mc6MEVEzgROEXY3X+ywECU9q18aIOct+ | ||
m5bmFcRcwoxo/hj6fsnmeH567KRfnN4Al219azq5ccwTr68y8tasYsZHOFCkn3ve | ||
Hkzu0+gylHZGWqo5TWif9O9DrII/KszsoX86jJxhORwqCnxMmKQQ/gGvexpAYJA= | ||
-----END ENCRYPTED PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
-----BEGIN PRIVATE KEY----- | ||
MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgR2poUqAkEiJtWPJS | ||
HW/tjfkvAhAmuhx1NpgUvCXuIHShRANCAARAPkw7+f3KpINOzPDNWkCkvHlJAV5w | ||
Tll8OSDGpV0dB5ybUEA+jNnh4oY2EqfvaFPv2YuWn0ddf6g0Ry5VPzcf | ||
-----END PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
-----BEGIN ENCRYPTED PRIVATE KEY----- | ||
MIGbMFcGCSqGSIb3DQEFDTBKMCkGCSqGSIb3DQEFDDAcBAhiAnoQd2VMZwICCAAw | ||
DAYIKoZIhvcNAgkFADAdBglghkgBZQMEASoEENed/l3AFuDblxDnswMZXDcEQIpo | ||
fCVdEbDerN0Rrh9i+Ymu+qpEqGlc6jycwR3rPtyL9jy0k5kauBxRn3Z5uCSlGzJL | ||
JXxlMR+DWG6QDJdxrHI= | ||
-----END ENCRYPTED PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
-----BEGIN PRIVATE KEY----- | ||
MC4CAQAwBQYDK2VwBCIEIADMEXUw9TGuz7JykmHbzPOj8XebpZwo76iuxJtHkvAp | ||
-----END PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
-----BEGIN ENCRYPTED PRIVATE KEY----- | ||
MIIFHTBXBgkqhkiG9w0BBQ0wSjApBgkqhkiG9w0BBQwwHAQIr0PK2BHcNKsCAggA | ||
MAwGCCqGSIb3DQIJBQAwHQYJYIZIAWUDBAEqBBCClZC7H7nFtlsAd+cURqvqBIIE | ||
wOYCGtgoABpWHCk5chj4EF3t2T+Eu753vz5FudgbFkkoMFVmxKLGB0BYVyxMIws/ | ||
GLj6WYq8o51E867hr3f3XOc5nIJEffGSxF/fhYIMGuE9teOWYVTAc/vGlFzEWska | ||
cXGgffBT+DqA4uaLEgcuYHd+hjhlNhKmNIFDsIP40sbHMP31NhuQyd1a63sTUJro | ||
lcLEsESC1H4jru36RrrlRUrTjdgwPrraeu/GUw3Dq7Op5wTfLTsv06Ci3OAKWkGs | ||
SMog36KgvU834YC1btAHL/T/R7lE2QFp8bHPSHA0iigsDiO6NoGcn5Ktvj0yxYzm | ||
8T+uiKs0aEzHWRX+08J8iBIhbSuNm6z/J+CILT3ce5/1vE9Kc2Ml4iVAvwvIYvKB | ||
TO1ZPe4/8gou/pMAHZ39/4euywZpYn9dMSPrsbLt0Wmqg+vVg9mVPD57eZiBgj0j | ||
RqgOlrhymaIA/tDYQTt+DJOKVtGNMbPrXqVpkpaAbXVa7uXYq/euTi2OgzMOSIZj | ||
JFLCWyZo/kpHa6/scIiRjFEDRLqIuhgKca5c4HQ23tRrtWvI+kMd5FK6IAAUplOy | ||
rP1EOztLrwk7NmSpL/zbOJ585AgiGPkq8AAxWfEpMt5GTvcomMjQpv1bs82jLPSG | ||
dC3N+7AOgMQQBDamwfallDgDpvlS7+pgmQJVsv9oT+XoHIXrkD7q7junA8T8Zd9a | ||
LsBgFxMVid2X1PWRkC8+5M4zb7HZdqIyK/vI1kPVWvdUYTvRMHgXxa5wBW/kB3q9 | ||
CQYGfOvg/Gsk7h/WqzlApqvcNZ8OJsxkhlElr2KkH5R9W3snAGedm/fHd1XWpkkL | ||
tY7qHb9z4WW2QOZWBkFY1mfUTU0wrYexAsgLhVniHTy71zj3LLVmezPxDCIvYslX | ||
hSY9/cofCsTx4W5toh/jh1NMsgQgaN2dGgCmHL+PwQTN3bL13KtKQUogNmHxKBMc | ||
QZepJqoiiOtjT6LjBkQI9PY+Yzky4zToo/3Gh5i7ohYAABvT9MXtK4hbQP26JB0T | ||
GL8pVLJSUCBQFj+87sJVILW+n1ak8OoEZQLNgs/V6lbtSc+0TW6t1ixZs7Xxh6KJ | ||
1zLDu0IwebONt8R8KVLUPeV0aFAjr5R9KGNvaL+F0EexG+fNSe1vJ+iBf3fhQLb1 | ||
EDI20FGYBlMfSwtNw0OhKZ5cG1lSx3BUtW1/nl3Y0+sYhRRtP628JjZ2YYifJZdc | ||
+iNGSC2m4r6li9spPvQUyJLHb2vMIwHEqbAi+OE4fNCBVgm0vNPdhEZHAwa8HvRt | ||
iQd2vsEE1tKSfj8z0XQl0T3u4WDVB0DvQqsSjVIfDL+vxlCZ540l26KWd1rJJOWi | ||
fWRF4+MV07GQgoE8aL8bbLUNBJq5OJPTHSgUs7ke7D0sHas1Z2ue/LccZeMbi39U | ||
kTqAyB0ag2/aaqL3/sUo1mi2HZBtqjg4P2gI6ymeVHeSQu8Hx62mw58C8WmoRk12 | ||
3YfiHXZI6LHBt+CK+nIXvsf7ZXeHHVnvRJEJaeX+jrCyCKyqCz8r47ZUUb2sgqFf | ||
8hTpheQKvCbYM+XtkkkuiudVevY67xrHpjAjDIU/4BXxC5KvlBQ8i8u21qtSq70P | ||
fFQUbG2XVVscckIFQa8DuDc= | ||
-----END ENCRYPTED PRIVATE KEY----- |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,28 @@ | ||
-----BEGIN PRIVATE KEY----- | ||
MIIEuwIBADANBgkqhkiG9w0BAQEFAASCBKUwggShAgEAAoIBAQC5O1ef4Fq1fWgm | ||
6+Gp8lnDmNz+lwjElQ+a6gUIff5td8oEn/3iLE0RPNkFqzK9P+jNugBsIbepwk5j | ||
F/YER5MAhd7WMsChN3UYoLAy9k7KOew833n+UKHB92cFszOllhMZ+hTKVeZ7+bOO | ||
Mu78nSpeBHmXKT0cVP7HlgS1GXxVIeIOQspNnft3CGyqByz4+R+9gxQr4Lx6+d8T | ||
S2BaApmTQRq2XzuctbJVcHgvOFIO0YosI8A6Ctft9h+mUPAnZYrU3qcbQWfFbUeE | ||
N5Irt7ZNsBra9lCC8Vcxac7g781kqngI6k5F7KWJaF20oCOv/5wPjIN8+OGOMo5h | ||
/Fu91EbhAgEjAoIBACpWtPFX2jgcqheGX3dNVliX9/+tfljRnSq5JbjMV2l6d1GD | ||
p76rCk0VOOtaVL2K82I5JKsAZH6S0BamZB48PtuMUDD1qF9dIhRB/GNrf7kxz5ji | ||
n4qWFlg4jJOWrLgiTYJHyjzgbzJHtAM+14oyjVdReuC4+AZ509XZJaW8rrRfIBeq | ||
x7AOGLtJ4iZmiYrNuHfdJeNXFqTMd9rwERR4gh5H2gWVAy5h2jgaseEU7ZP6D+Cr | ||
D+RV3XOR4TC3Tp9lUMStXCn63B7DHG/CuTgIpQLNq9+EN+Lmwnlh3OEMFGCTIGAk | ||
j5/Xl0rKqo6IClqtX367OsPq8i42eVWipACK6AsCgYEA8GWLLfZ7zeeyEGScGfkc | ||
4ras7drXMfG9iv83MhNe11k6Nq03J9e2TCIuX96Xm1mEm8p72ci1Ij/jDua+W9dl | ||
9byHcxHSpvxq3iNZrzJraHAR6upomieqONv54GGDbuW1khBGTwG/oCldMBaRz50U | ||
aEQjKyiQNBftXvYF2uLO6nUCgYEAxUEscrTkgVvzHt1qeUCYNfpuoI5sh3g9x7Y5 | ||
440UiPWMijC8JdoTS33NThflJ03m6Oq7gOpeDNR014pwnFakU7vgwNHFPcJszPfp | ||
+KQme/FaX/6rQdqVi4JjyCbXhVhxOFQECujdz3jUg257JsY0sgD2NztEyewW3JVO | ||
81ilpT0CgYAUmv1NFSCN/eqw8q5LXn7RmqEbs6wLmGC0JIES67esDvZcdT897esN | ||
1wtKC8PaHZ2m9Bk+jYveXT9ZDHa3ajvwfd+5Z+18BwHYhq/qcgk01mfvkG9dq6Dg | ||
TV6Pk1Rol1i0v5D/dS2upHWzqinBVptZZO0SU+8aaHNuih3CTfR6fwKBgDK4/M0J | ||
8Z2bTSUxnwk8fulPBoEOrjF2sMz0WAdQKdoTQWVc/S5OBPYnqdJAqKO08jvklp1+ | ||
GC8vUT69MuZfbBWIFTjle9yuVn3ZWWvScEvBuCdQHWi0jNq7IPj0C4iwV6DFJZxn | ||
xAImYogyWi6KvRfUXJHcCl/O/pB+Kj6TI0e/AoGBAK3DGiOTNRJ9XfMO3pthIsHO | ||
BeCj9mv2VL6BKCA/vYPMg2ZbaBsDR+QzZm4mW1x9qQq/iBVf/XOijP7u37huPp2q | ||
35uXuFjPRWdNINka6jksoOJ//iBIat1uULZAtO5fYeNYJl2y57uZHAvtxdHjOUig | ||
T335HKF3G9znAdJ9iv8k | ||
-----END PRIVATE KEY----- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it necessary to remove this?
Same applies to the other key constructors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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-HagueThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 usingPrivateKeyFile
instead of eachKey
class.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This improvement does not require changing public API. It brings us zero benefits, and brings detriment to users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If everyone agrees, then I can add them back. Would you mind sharing your thoughts here? @drieseng @WojciechNagorski @mus65
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
.