Symmetric keygen: JsThemis

[ilammy]

Implement symmetric key generation utilities described in RFC 1 (not available publicly at the moment). This is new API introduced in #560, now distributed to JsThemis wrapper.

Language API

JavaScript (Node.js)

Here's how it can be used:

const themis = required('jsthemis')

let key = new themis.SymmetricKey()
// key is Buffer, use that to access key bytes

// Can also wrap existing buffers
let buffer = Buffer.from([1, 2, 3, 4, 5, 6])
let anotherKey = new themis.SymmetricKey(buffer)

Yes, it looks exactly like WebAssembly API from #561.

Why is it special?

Node.js native extensions and V8 are hard.

Initially it was expected that JsThemis has an interface similar to WasmThemis. Something like this:

class SymmetricKey extends Buffer {
    constructor(array) {
        if (array) {
            super(array)
        } else {
            super(generateNewKey())
        }
    }
}

Note that SymmetricKey is a class which inherits from Node.js Buffer. Now consider the fact that JavaScript does not really have classes, it has objects, functions, and prototypes, with EcmaScript 6 classes being a nice syntax sugar on top of that.

It turned out to be not obvious how to implement JavaScript inheritance from native JavaScript classes given the API provided by V8 (and I'm not even talking about the outstanding quality of V8 documentation /s).

This is further complicated by the fact that all of the Buffer's constructors are considered deprecated, with Buffer.alloc() and Buffer.from() being the recommended way of constructing Buffers.

I'm in a bit of a loss here so effectively SymmetricKey is now

function SymmetricKey(array) {
    if (array) {
        return Buffer.from(array)
    } else {
        return generateKeyBuffer()
    }
}

This kinda works on the API level because classes are functions in JavaScript, but it's not strictly correct as new SymmetricKey() expression returns an instance of Buffer (not SymmetricKey). It's
fine since that's all API that we need at the moment, but it's not clean. However, after spending around 4 hours trying to understand how to do inheritance, I kinda gave up.

This should be enough for practical purposes for now. We will have to get back to this issue if we would like to provide our own wrapper over Buffer instances for all other keys, but for now it's good enough.

* fingers crossed *

Checklist

• Change is covered by automated tests
• Benchmark results are attached (not interesting)
• The coding guidelines are followed
• Public API has proper documentation
• Example projects and code samples are updated (later)
• Changelog is updated

[vixentael]

Looks fantastic, thank you!

[Lagovas]

if user pass own buffer, why we don't reuse it and return new allocated buffer?
CopyIntoBuffer use nan::CopyBuffer instead nan::NewBuffer which will reuse passed array

[Lagovas]

oh, I re-read comments and understand that we just copy passed buffer, not generate new key... but for what we do this or why it will useful for user? is I understand correctly, that when user pass some Buffer to constructor themis.SymmetricKey(someBuffer) we return node::Bufer object with same values?

[ilammy]

but for what we do this or why it will useful for user?

Well, we have to copy the buffer so that changes in the original buffer do not affect the key.

As for why this API exists at all, my original idea was that new themis.SymmetricKey(...) would return an instance of SymmetricKey — a type that inherits from Buffer, but is not a Buffer. Then it will be possible to restrict Secure Cell interfaces to work only with SymmetricKey instances which are guaranteed to contain only valid keys. In this case the users will need an ability to convert an arbitrary Buffer into SymmetricKey with JsThemis validating the key.

Currently new themis.SymmetricKey() returns instances of buffer, but even if that’s not fixed by the next release I would like to keep this API so that we teach the users to use it instead of using Buffers directly.

[Lagovas]

as I see, nan and nodejs buffer api provide two ways to create objects. New created with copy of values nan::FromBuffer and second way to reuse user's object new nan::NewBuffer. What about to be appropriate with their api and provide two ways too?

[ilammy]

Nan::NewBuffer() transfers the ownership over a char* buffer to Node.js Buffer object. That’s not what we want here since the original byte buffer is still owned by the original Buffer object. It’s a utility method to transfer buffers created by C++ code into JavaScript without making an unnecessary copy.

When the user does new SymmetricKey(someBuffer) it does not transfer someBuffer into SymmetricKey. Unfortunately, JavaScript is not Rust and even if we did reuse an existing Buffer as SymmetricKey, this will not prevent the user from using someBuffer again after the call. For example, they could modify the buffer and this will modify the key. I don’t think that this spooky action at the distance is an expected behavior so it’s better to copy the buffer with Nan::CopyBuffer() and avoid such issues.

[Lagovas]

It's okay if they know, that they transfer ownership. in such case they use new SymmetricKey(someBuffer) just to cast their buffer to correct type to use with themis in the future. If our function doesn't modify this buffer, it's okay if user use previously created object to re-use if he know, that don't need it in a future. For example (js pseudocode):

var buffer = Buffer.alloc(32)
var keys = ['32 length key1', '32 length key2']
var data = ['data1', 'data2']
var encrypted = []
for (i:=0; i<data.length; i++){
  var key = new themis.SymmetricKey(keys[i]);
  encryptedData = new themis.SecureCell(key).encrypt(data[i]);
  encrypted.push(encryptedData);
}

here is user reuse same buffer and use it just to cast his keys to correct type and use with themis SecureCell api.

But if we can't do like that, so we only can leave as is...

[ilammy]

The above code will work just fine. That's what a sane developer (i.e., one thinking like me) would probably write if they wanted to encrypt data with different keys.

The current copying implementation makes the following usage impossible:

var buffer = Buffer.alloc(32)
var key = new themis.SymmetricKey(buffer) // note that it's out of the loop
var cell = new themis.SecureCell(key)

var data = ['data1', 'data2']
var encrypted = []
for (var i = 0; i < data.length; i++) {
    getSomeKeyDataInto(buffer)
    encrypted.push(cell.encrypt(data[i]))
}

While it definitely has a rationale of a sort, I'd consider it premature optimization and, honestly, a sleeping footgun. It looks like the same cell is used for encryption while in fact the key is different.

Now imagine if the Buffer is actually a reference that's kept in a field or some object. And that object caches buffer instances so the same buffer may be reused later for some completely unrelated data. Even if you don't abuse the buffer like the above, it may still be accidentally changed.

That's why I think that copying the buffer is the most reasonable choice here.

[Lagovas]

actually here not checked that generated new key buffer. we should compare references/values to be sure, not that same length was used.

[ilammy]

Mmm... I’d say this is a bit excessive. But sure, we can add

var key1 = new addon.SymmetricKey()
var key2 = new addon.SymmetricKey()
assert.notDeepEqual(key1, key2) // checks that content is not the same
assert.notEqual(key1, key2) // checks that objects are distinct

Though I doubt that this test will prevent any coding mistakes.

[Lagovas]

I like to add it because we should be sure that our new layer of js code correctly uses themis core. If it was simple proxying calls like in c++/golang then it will be really overhead. But in wrappers like jsthemis/phpthemis where we deal with language wrapper API we should do more checks IMHO because we have more places for mistakes.

[ilammy]

Okay, now I get your point. That’s a pretty valid concern. Thank you for the explanation.

[Lagovas]

oh, I re-read comments and understand that we just copy passed buffer, not generate new key... but for what we do this or why it will useful for user? is I understand correctly, that when user pass some Buffer to constructor themis.SymmetricKey(someBuffer) we return node::Bufer object with same values?