-
Notifications
You must be signed in to change notification settings - Fork 55
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
Why do API users prefer byte arrays to Key instances? #31
Comments
Please know that I'm not a user of NSec myself (yet) and have only a little experience with cryptography in .NET.
Have you thought about the naming conventions for the I have used a similar approach in instances that store secret-key, calling the equivalent export function |
Well, I do it only for public keys and nonces only when I need to pass them to other cryptolibs over embedded platforms, on which I cannot install NSec. To be more general, API users may not prefer or don't readily know how to save & load Key instances, i.e. methods like serialization, json, etc. Some methods could be added to these classes to provide this functionality (i.e. SaveSerialized()), but maybe "the juice isn't worth the squeeze". |
This is my first issue with NSec I am trying to implement 'Noise_IKpsk2_25519_ChaChaPoly_BLAKE2b' The biggest Problem with this Key class is, that it is a class and needs heap allocation. Don't know if the code is totally correct, it is only to display the need for a //temp = HMAC(initiator.chaining_key, msg.unencrypted_ephemeral)
var key = Key.Import(MacAlgorithm.Blake2b_256, chaining_key, KeyBlobFormat.RawPrivateKey);
MacAlgorithm.Blake2b_256.Mac(key, t1.Slice(32, 32), temp);
//initiator.chaining_key = HMAC(temp, 0x1)
key = Key.Import(MacAlgorithm.Blake2b_256, temp, KeyBlobFormat.RawPrivateKey);
MacAlgorithm.Blake2b_256.Mac(key, new byte[] { 0x1 }, chaining_key);
//temp = HMAC(initiator.chaining_key, DH(initiator.ephemeral_private, responder.static_public))
key = Key.Import(MacAlgorithm.Blake2b_256, chaining_key, KeyBlobFormat.RawPrivateKey);
SignatureAlgorithm.Ed25519.Sign(ephemeral_key, responser.static_public, t1.Slice(0, 32));
MacAlgorithm.Blake2b_256.Mac(key, t1.Slice(0, 32), temp);
//initiator.chaining_key = HMAC(temp, 0x1)
key = Key.Import(MacAlgorithm.Blake2b_256, temp, KeyBlobFormat.RawPrivateKey);
MacAlgorithm.Blake2b_256.Mac(key, new byte[] { 0x1 }, chaining_key); |
To be honest I don't import/export unless necessary, but I did some benchmarking and generating 10,000 keys takes ~320 ms then importing them all is ~300. This is far below what I was expecting, meaning I can import tens of thousands of keys a second. var sw1 = Stopwatch.StartNew();
for (int j = 0; j < 10000; j++)
{
var key = Key.Create(SignatureAlgorithm.Ed25519,
new KeyCreationParameters() {ExportPolicy = KeyExportPolicies.AllowPlaintextExport});
pkeylist.Add(Convert.ToBase64String(key.Export(KeyBlobFormat.RawPrivateKey)));
}
Console.WriteLine(sw1.ElapsedMilliseconds);
var sw2 = Stopwatch.StartNew();
foreach (var t in pkeylist)
{
var key = Key.Import(SignatureAlgorithm.Ed25519, Convert.FromBase64String(t), KeyBlobFormat.RawPrivateKey);
}
Console.WriteLine(sw2.ElapsedMilliseconds); Now of course wasted compute time is wasted compute time. But were you more concerned about the ms time regarding generation/import/export, or signature verification? |
@sigaloid I'm mostly concerned about the API design. It seems that a lot (most?) projects on GitHub that reference NSec contain some variant of the following code: class MyKeyPair
{
private byte[] privateKey;
private byte[] publicKey;
public MyKeyPair()
{
var key = new Key(SignatureAlgorithm.Ed25519, new KeyCreationParameters { ... });
this.privateKey = key.Export(KeyBlobFormat.RawPrivateKey);
this.publicKey = key.Export(KeyBlobFormat.RawPublicKey);
}
public MyKeyPair(byte[] privateKey, byte[] publicKey)
{
this.privateKey = privateKey;
this.publicKey = publicKey;
}
public byte[] Sign(byte[] data)
{
var key = Key.Import(SignatureAlgorithm.Ed25519, this.privateKey, KeyBlobFormat.RawPrivateKey);
return SignatureAlgorithm.Ed25519.Sign(key, data);
}
public bool Verify(byte[] data, byte[] signature)
{
var key = PublicKey.Import(SignatureAlgorithm.Ed25519, this.publicKey, KeyBlobFormat.RawPublicKey);
return SignatureAlgorithm.Ed25519.Verify(key, data, signature);
}
} (Note how the key gets exported on creation and then re-imported on every Sign/Verify operation.) So, apparently, a lot of API users seem to think, "Well, this library is nice, but it doesn't meet my needs. However, I can easily work around that with some exports/imports!" This makes me think that the design of the NSec API is a complete failure -- if every API user has to essentially write the same code to "fix" it. Wouldn't it be so much better if API users didn't have to "fix" the API before they can use it? I just have a hard time understanding what's wrong with doing it the following way... class MyKeyPair
{
private Key key;
public MyKeyPair()
{
this.key = Key.Create(SignatureAlgorithm.Ed25519);
}
public MyKeyPair(byte[] privateKey, byte[] publicKey)
{
this.key = Key.Import(SignatureAlgorithm.Ed25519, privateKey, KeyBlobFormat.RawPrivateKey);
}
public byte[] Sign(byte[] data)
{
return SignatureAlgorithm.Ed25519.Sign(this.key, data);
}
public bool Verify(byte[] data, byte[] signature)
{
return SignatureAlgorithm.Ed25519.Verify(this.key.PublicKey, data, signature);
}
} In my eyes, this is simple, clean, fast, and type-safe... The only "flaw" I see is that it doesn't store the key as a byte array. (Thanks to everyone who has commented so far, by the way! There are some valuable insights, but I'm still quite puzzled over this.) |
That certainly does make more sense in a lot of cases. For my current use case, though, I have a database of public keys and may have to verify a signature from any of them at any time, so depending on the number of keys in the database, it may not make as much sense to import every key and keep them in memory, but rather to import on use (and possibly keep it in memory then). Though for most normal uses, you don't have that many keys to handle, so makes no sense to import/export.
Absolutely not; writing poor code with a library is not a reflection of the library's quality, but the author's skills. Perhaps add a warning to the documentation that exporting and importing should not be done, but a single key instance should be passed around if you're going to be using the same key. |
FWIW I think the NSec API is pretty good. It's just difficult to appreciate how many developers don't have the time or interest or experience to pause and think about what they're doing. |
One of the goals with NSec is to improve on other cryptographic libraries by providing dedicated types for keys (
Key
andPublicKey
), nonces (Nonce
), etc. instead of passing everything around as byte arrays.A quick look around at some GitHub repositories using NSec reveals that API users seem to dislike that entirely. The added safety is quickly defeated by keys being exported right after creation into byte arrays and re-imported for every single encryption/decryption/etc. operation (which is not only error prone but also really slow due to the way keys are stored internally).
Why? 😕
If this feature causes more inconvenience than safety, should it just be removed?
If not, how could API users be encouraged to keep their keys in
Key
instances?The text was updated successfully, but these errors were encountered: