Skip to content
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

ED25519Key PrivateKey doesnt return all bytes #1548

Closed
darinkes opened this issue Dec 2, 2024 · 10 comments
Closed

ED25519Key PrivateKey doesnt return all bytes #1548

darinkes opened this issue Dec 2, 2024 · 10 comments

Comments

@darinkes
Copy link
Collaborator

darinkes commented Dec 2, 2024

Noticed after updating my SshNet.Agent Extension to the current 2024.2.0-Release
2024.1.0 is fine, though.

I would guess it could be fallout from: #1448

Please also see: darinkes/SshNet.Agent#8

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 2, 2024

Chaos.Nacl (old):

public static readonly int ExpandedPrivateKeySizeInBytes = 32 * 2;

PrivateKey = new byte[Ed25519.ExpandedPrivateKeySizeInBytes];

BouncyCastle (new):

public static readonly int SecretKeySize = 32;

PrivateKey = new byte[Ed25519.SecretKeySize];

The size of the privatekey byte array is too small now.

Diff could be something like that:

--- a/src/Renci.SshNet/Security/Cryptography/ED25519Key.cs
+++ b/src/Renci.SshNet/Security/Cryptography/ED25519Key.cs
@@ -86,7 +86,7 @@ public ED25519Key(SshKeyData publicKeyData)
             }
 
             PublicKey = publicKeyData.Keys[0].ToByteArray(isBigEndian: true).TrimLeadingZeros().Pad(Ed25519.PublicKeySize);
-            PrivateKey = new byte[Ed25519.SecretKeySize];
+            PrivateKey = new byte[Ed25519.SecretKeySize * 2];
         }
 
         /// <summary>
@@ -97,9 +97,9 @@ public ED25519Key(SshKeyData publicKeyData)
         /// </param>
         public ED25519Key(byte[] privateKeyData)
         {
-            PrivateKey = new byte[Ed25519.SecretKeySize];
+            PrivateKey = new byte[Ed25519.SecretKeySize * 2];
             PublicKey = new byte[Ed25519.PublicKeySize];
-            Buffer.BlockCopy(privateKeyData, 0, PrivateKey, 0, Ed25519.SecretKeySize);
+            Buffer.BlockCopy(privateKeyData, 0, PrivateKey, 0, Ed25519.SecretKeySize * 2);
             Ed25519.GeneratePublicKey(privateKeyData, 0, PublicKey, 0);
         }

I couldn't find a reasonable BouncyCastle constant.

With this patch ED25519 works again in the SshNet-Extensions.

@Rob-Hague
Copy link
Collaborator

Looks reasonable, do you want to put up a PR?

@scott-xu
Copy link
Collaborator

scott-xu commented Dec 3, 2024

What specific issue if the Ed25519 private key length is 32?

@scott-xu
Copy link
Collaborator

scott-xu commented Dec 3, 2024

Is the next 32 bytes public key?

@scott-xu
Copy link
Collaborator

scott-xu commented Dec 3, 2024

Is the next 32 bytes public key?

The answer is yes.

var privateKey = Convert.FromHexString("92-F0-5A-EA-18-A5-F3-08-92-B5-66-DD-A6-A4-CC-51-64-D0-66-39-B4-DF-B4-0A-84-1C-76-00-30-A3-F2-BC".Replace("-", string.Empty));
var publicKey = new byte[Ed25519.PublicKeySize];
Ed25519.GeneratePublicKey(privateKey, publicKey);
Console.WriteLine(Convert.ToHexString(publicKey));

Output:
4E56E8170D9CCB8D80B62E51AD7C17F0A2F1DCCFA9E20EFC7DF7DE7D32BF8A3A

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2024

What specific issue if the Ed25519 private key length is 32?

The issue is if you want to export your private key e.g. in to the SSH Agent or into openssh keyfile it will get rejected as malformed.

Sure, I could workaround this by appending the pub key myself to the private key bytes, but I think its an API breaking change and should behave like it did before.

@Rob-Hague
Copy link
Collaborator

agree, the behaviour should be preserved

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2024

@Rob-Hague I thought about opening an PR, but I'm not sure if there is a "good" BouncyCastle constant for this size.
Maybe it could be SecretKeySize + PublicKeySize, not sure....

@scott-xu
Copy link
Collaborator

scott-xu commented Dec 3, 2024

I would agree if there's no PublicKey property in ED25519Key class.

/// <summary>
/// Gets the PublicKey Bytes.
/// </summary>
public byte[] PublicKey { get; }
/// <summary>
/// Gets the PrivateKey Bytes.
/// </summary>
public byte[] PrivateKey { get; }

Regarding OpenSSH private key format, the value contains both private key and public key.
https://datatracker.ietf.org/doc/html/draft-miller-ssh-agent-11#section-3.2.3

The second value is a concatenation of the private key k and the public ENC(A) key.

Regarding PuTTY PPK format, the Ed25519 public key and private key are separated:
https://tartarus.org/~simon/putty-snapshots/htmldoc/AppendixC.html#ppk-privkey-eddsa

The public key data has already provided the public elliptic curve point. The private key stores:

  • mpint: the private exponent, which is the discrete log of the public point.

Regarding PKCS#8 format, the private key is solely private key.
https://www.rfc-editor.org/rfc/rfc5208#section-5
rfc5958 defines extra optional public key field, and has status "proposed standard".
Some interesting discussion: https://www.rfc-editor.org/rfc/rfc5958

In a word, I don't think we should revert to the wrong length just because it was there for long time.

How about add another constructor for ED25519Key class which takes both private key bytes and public key bytes? @darinkes
See my recent comments please: #1543 (comment)

@darinkes
Copy link
Collaborator Author

darinkes commented Dec 3, 2024

K, will do it myself then.

@darinkes darinkes closed this as completed Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants