Why do API users prefer byte arrays to Key instances?

[ektrah]

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]

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]

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]

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]

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]

@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]

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]

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.

[jesuslpm]

IMHO forcing developers to use key abstraction for security reasons is not a good idea. I believe this is somehow a paternalistic approach. Developers are responsible for keeping their keys secure, It's not your job or the library job. I followed a different approach, giving developers the ability to use libsodium SecureMemory. Take a look at https://libsodium.net/guide/SecureMemory.html, https://libsodium.net/api/LibSodium.SecureMemory-1.html, https://libsodium.net/api/LibSodium.SecureMemory.html. Source code: https://github.com/libsodium-net/LibSodium.Net/blob/main/src/LibSodium.Net/SecureMemory.cs

[samuel-lucas6]

@jesuslpm I think that really depends on whether the library is trying to be fully misuse-resistant or harder to misuse. If that's the goal, that approach and similar ideas make sense. Frank Denis actually did a talk on this kind of topic.

[someonestolemyusername]

IMHO forcing developers to use key abstraction for security reasons is not a good idea. I believe this is somehow a paternalistic approach. Developers are responsible for keeping their keys secure, It's not your job or the library job.

Fully agree here. Libraries should have a reasonable way to access the keys as a Span or byte array (or even an unsafe pointer which would discourage use as it requires unsafe compilation).

The point is there are developers who will still somehow manage to misuse a safe library and make it unsafe at any cost. But this type of strict library design turns off experienced developers who are trying to get work done also.

Hide it behind warning alarms, call it unsafe, do whatever is needed to discourage use, but ultimately provide a reasonable way to do it imo.

[athendrix]

Late to the party here, but the biggest reason I'd prefer byte[] over the native types is for easy database interoperability.

[Zetanova]

This issue comes from the OOP and java space,
where the user (the developer) is assumed to be untrusted
and need to be "protected" by themself.

The right view is to see the public API as a contact between the library and the user/developer,
where the library guaranties in a high degree of functional and binary compatibility of the API between versions.

All other internal used functions are not under this contract.
Maybe useful utility classes and functions can be placed under an "internals" namespace,
but don't need to be declared private or internal.

Beside this above, there is a need to protect a private key in memory,
but this is an issue for the user and not for the library itself.

Key protection runs down that the private key is only as short in memory as possible.
After use it needs to be overwritten securely by rnd or encryption,
where the encryption need to come from the host (WindowsManager, TPM, ect ..).

[samuel-lucas6]

Beside this above, there is a need to protect a private key in memory,
but this is an issue for the user and not for the library itself.

This is an issue for the library as well because you don't want to create copies of sensitive data that aren't being wiped. This is a lot easier said than done and can harm usability, like avoiding strings/using buffers for variable-length data, meaning the user has to manually truncate the buffer.

However, I think this is a good security goal and separates a library from the alternatives. C# supports this, whereas some other languages don't, so people should capitalise on that.

The point is there are developers who will still somehow manage to misuse a safe library and make it unsafe at any cost. But this type of strict library design turns off experienced developers who are trying to get work done also.

Hide it behind warning alarms, call it unsafe, do whatever is needed to discourage use, but ultimately provide a reasonable way to do it imo.

This is the trade-off with a harder to misuse/misuse-resistant library. You'll hear people recommending things like Tink, but a locked down approach renders implementing certain things impossible. Then people don't use more misuse-resistant algorithms due to their overhead, so there's a mismatch between what people are preaching and what actually gets used in practice.

I don't think there's a solution to this problem. Documentation and warnings are great, but some people will ignore them. You either do a higher-level library that's restrictive but protects people, a lower-level library where people can potentially shoot themselves in the foot, or both, except that's a ridiculous maintenance burden and people will likely just use the lower-level version.

The user is ultimately responsible for using a library and doing cryptography correctly, but the library should be designed and documented in a way that helps them do that. When people being paid to work on security-related products are still making basic mistakes, there's clearly room for improvement on the education front in this field. A big part of the problem is the academic nature of cryptography in my opinion, rendering it inaccessible/overwhelming for developers who don't already have a strong background in the subject. People with the knowledge are responsible for sharing it in an understandable manner, which isn't happening enough.