search

jlouis / enacl

Erlang bindings for NaCl / libsodium
MIT License
197 stars 59 forks source link

aead_chacha20poly1305_* argument divergence #32

Closed ghost closed 4 years ago

ghost commented 6 years ago

The aeadchacha20poly1305* funcs in terms of parameter order are backwards... The idiom in libsodium is (Message, AdditionalData, Nonce, Key). The current implementation is the opposite (Key, Nonce, AdditonalData, Message).

In addition The do {} while(0) usage in these functions while surely harmless is confusing and IMHO of no use. It is assumed that the author was aiming for single entry/single exit, but it doesn't read well.

I will gladly do a PR for this and include some additional tests (i.e, 1 failure test where there should be 3 to 4).

This would be a breaking change, so wanted to get your thoughts before pushing something up.

jlouis commented 6 years ago

I think we should try to make the parameter order be the same as in the Libsodium call chain. But maybe @hanssv had a reason to implement them the other way around. I know that in a language with currying, the parameter order where Key and Nonce comes first means you can build a partial function over the current operation. But in Erlang, where no currying is present (sadly), I think it would be better to make this harder to abuse.

Though I'd like to wait a bit on Hans, because he (and by extension Aeternity) are likely users of that call already and they'll need a heads up to change it around.

The alternative to a do ... while (0) block is likely to have a cleanup label in the epilogue of the function body which you jump to. Personally, I think I would prefer the cleanup label, but this is mostly because I think this is more in the style C programmers tend to write code (I might be wrong, however).

The do-while cleanup I'd say: go ahead if you want to make the change. We should be able to test against correctness as long as we make sure the memzero happens.

hanssv commented 6 years ago

No, no fundamental underlying reason ;-) Just bad style from me...

We (Aeternity) have yet to switch back to main enacl (using our fork at the moment) so please fix this to you liking, and we can make the necessary adjustments when we do the switch.

ghost commented 6 years ago

@hanssv I'm not sure if it's bad style or not. I got some opinions on that, and it was half and half. Half liked it, half didn't get it.

jlouis commented 6 years ago

If we have the go-ahead from Hans here, I think we should aim to have the API reflect whatever libsodium is using if possible all the way. It does remove one layer of possible misinformation, which could otherwise happen.

While we can handle return values such as keypairs as #{ public := PK, secret := SK } so you have a harder time swapping those, I'm not sure we can make the input argument easier. Passing a map or record seems somewhat defeatist to me. So by all means, go ahead!

jlouis commented 4 years ago

I have a patch for this now, fixing the order.

There are other problems in the code which I also have fixes for:

This is good, because it means I can now break people's code if they have used the old API and rename the functions. While here, I'm also going to fix the parameter order (which is already in a branch slated for master)

jlouis commented 4 years ago

This has been solved and 1.0.0 is out.

Note: do test it before you go havoc in using as I've made a number of changes, some of which might have stability effect. OTOH, the code is in a far better place now moving forward.