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

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

Open
ektrah opened this issue Jan 26, 2020 · 7 comments
Open

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

ektrah opened this issue Jan 26, 2020 · 7 comments

Comments

@ektrah
Copy link
Owner

ektrah commented Jan 26, 2020

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
Copy link

brycx commented Mar 24, 2020

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
Copy link

YasarYY commented Dec 8, 2020

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
Copy link

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
Copy link
Contributor

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
Copy link
Owner Author

ektrah commented Apr 10, 2021

@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
Copy link
Contributor

sigaloid commented Apr 10, 2021

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
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants