ektrah / nsec

A modern and easy-to-use cryptographic library for .NET 8+ based on libsodium
https://nsec.rocks
MIT License
387 stars 55 forks source link

Why do API users prefer byte arrays to Key instances? #31

Open ektrah opened 4 years ago

ektrah commented 4 years ago

One of the goals with NSec is to improve on other cryptographic libraries by providing dedicated types for keys (Key and PublicKey), 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?

brycx commented 4 years ago

Please know that I'm not a user of NSec myself (yet) and have only a little experience with cryptography in .NET.

If not, how could API users be encouraged to keep their keys in Key instances?

Have you thought about the naming conventions for the Export function? Maybe calling it Export doesn't tell the user enough about the implications of using it. For example, the golang crypto package has a function called InsecureCipherSuites(), which returns a list of supported cipher suites that have security issues. It's separated from the standard function CipherSuites(), which returns supported cipher without security issues. This could force users to be aware of choosing less-secure options as they type out the words "Insecure".

I have used a similar approach in instances that store secret-key, calling the equivalent export function unprotected_as_bytes(). Sadly, I don't have any data I can use to determine whether this approach is actually effective.

YasarYY commented 3 years ago

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".

Zetanova commented 3 years ago

This is my first issue with NSec

I am trying to implement 'Noise_IKpsk2_25519_ChaChaPoly_BLAKE2b' like in https://www.wireguard.com/protocol/

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 ReadOnlySpan<byte> key overload.

           //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);
sigaloid commented 3 years ago

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?

ektrah commented 3 years ago

@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.)

sigaloid commented 3 years ago

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.

This makes me think that the design of the NSec API seems is a complete failure -- if every API user has to essentially write the same code to "fix" it.

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.

someonestolemyusername commented 1 year ago

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.