Implement complete binding

[whitequark]

As I've notified in #4, I went on to complete the binding. Unfortunately, I had to rewrite it almost entirely.

As a result it supports all NaCl primitives (not generic_hash or short_hash yet), is fully covered by tests, documented, and doesn't crash under heavy GC pressure.

I have changed the external interface. In my view, now it is both more convenient to use and more elegant to implement.

This PR depends on yallop/ocaml-ctypes#143, not yet merged.

[dsheets]

What do we gain from using OASIS for this library? I'm not very keen on fighting with OASIS to achieve changes to the build system like stub generation.

[whitequark]

Well, for me, adding OASIS was simple and it just worked, except the stub part.

In principle, I'm OK with replacing the OASIS cruft with an ocamlbuild config and a Makefile delegating to it, though I think that won't help much with stub generation. I'm quite opposed to the idea of using custom Makefiles rather than some simple, standard approach.

[dsheets]

This project should have a fairly simple basic build. I'm not sure what your concern is regarding custom Makefiles? Neither OASIS nor ocamlbuild can really address code generation stages.

[whitequark]

I think it is much preferable to use a common build system, well-known in community, rather than inventing your own with no good reason. This simplifies life of downstream users, especially packagers (e.g. there are oasis2opam and oasis2debian).

What code generation stages are you talking about?

[dsheets]

The old build system was not invented with no good reason. Why would you assume this? Like I said, the project is quite straightforward to build in the basic case.

I'm interested in keeping the build very obvious and providing for stub generation. I am opposed to OASIS unless it actually wins us something besides added complexity and a cluttered history. I've not found oasis2opam to be a win, either, as I nearly always have to modify its result. As for packagers, I plan on continuing to package ocaml-sodium for opam. If we want system packages, we should look to opam translation for those rather than OASIS.

[dsheets]

Overall, this is a really fantastic patchset! You've done a really excellent job here. I very much like your reorganization, use of modules, use of names, new signatures, tests, and more. There are a few issues that I noted and you seem to have already begun to address them.

Thank you! :-)

[whitequark]

I believe I have addressed all your concerns.

[dsheets]

This example needs updating with s/create_none/random_nonce/ and s/create_keypair/random_keypair/ and the new (sk, pk) tuple order.

[whitequark]

Fixed.

[whitequark]

I have addressed all your concerns.

[whitequark]

I have also updated everything to accommodate changes in yallop/ocaml-ctypes#143. This PR is now ready to be merged.

[whitequark]

By the way, what do you think would be the best naming scheme given the string→bytes change in OCaml >=4.02? Perhaps it makes sense to rename the modules Bigstring→Bigarray and String→Bytes.

[dsheets]

Probably a good idea for future sanity. Perhaps Bigstring should be Bigbytes? As I noted on IRC, both Lwt and Async will confusingly disagree with Lwt using string/bytes and Async using string/Bigstring. Oh, well.

[whitequark]

@dsheets I really like the Bigbytes idea. It's still confusing that Lwt_bytes ≡ Bigbytes, but oh well, you do what you can.

[whitequark]

@dsheets Now that I think about it, while I do like the naming for Bigbytes, I think we should maybe continue using the String interface.

Consider this, currently the Sodium.Box.String.to_public_key doesn't copy the key. Obviously this is wrong if the storage is supposed to be mutated. So we actually want the invariant.

Similarly, all of the functions which perform actual encryption too perform copying, for internal reasons (padding). It seems sane to expose this copying as accepting and emitting Strings.

Then you have to consider again how internally the interface revolves around blitting and mutating strings, even if just for the purpose of copying.

I need to think more about it.

[dsheets]

I'm not sure what you mean regarding Bigbytes vs String. It seems to me that there are two issues:

1. Mutability/ownership guarantees
2. Exposed types in interfaces

I absolutely think we should consider carefully the ownership policy of passed buffers whether they are String.t or Bigarray. I also think we should continue to offer interfaces to both types because some applications will expect one or the other and should not have to suffer another copy to get their preferred representation.

[whitequark]

@dsheets there are several parts to this issue.

First, there are keys, nonces, etc. Internally they are represented with strings, which makes sense, because they're immutable. While they are being constructed, though, they must be represented as bytes, because the C code will write to them. I think in this case I should make an unsafe cast from bytes to string, because in this case we are sure the bytes under construction are not aliased.

Second, there is conversion from externally supplied storage to keys/nonces/etc. When we're passed a string, there should be no copy, because strings are immutable. However, when we're passed a bigbytes (or bytes for that matter), there must be a copy.

Third, there are the cryptographic operations themselves. Here we don't care about mutability, because we don't capture a reference, but sometimes we do a copy ourselves (internally).

I think it may make sense that we provide a triple String/Bytes/Bigbytes interface, but I'm not sure yet.

[whitequark]

Ok. I have decided against providing String instantiations, mostly because the implementation burden is horrible. I'd have to significantly restructure the entire binding solely to deal with immutability. Also, given that this is a binding for a Networking and Cryptography library, I expect that most input data will be in bytes, or even more likely Bigbytes anyway.

I think this is now ready to merge.

[whitequark]

The support for direct-mapping pointers is almost merged into ctypes; see yallop/ocaml-ctypes#148. Use the following to build:

opam pin ctypes git://github.com/yallop/ocaml-ctypes#whitequark-bytes

[dsheets]

Thanks for your very substantial contribution. I've now merged your patch and released 0.2.0. Sorry for the delay in merging.

Thanks again,

David

[whitequark]

@dsheets I think you have missed the commit https://github.com/whitequark/ocaml-sodium/commit/ead98152e95f5e4e53d05ac16034f52032dde82d while merging (and possibly others?).

[dsheets]

I think I only missed ead9815. I did this to keep the naming of keypair consistent with:

1. http://nacl.cr.yp.to/box.html
2. https://github.com/jedisct1/libsodium
3. https://github.com/tel/saltine
4. https://github.com/freza/salt
5. https://github.com/stef/pysodium

[whitequark]

@dsheets I did that intentionally. Basically no other API except sodium uses "keypair". It's either "key_pair" or "KeyPair".

You're the project owner and you can do whatever you see fit, but I think that "key_pair" is more consistent with language usage. There's no such word or concept as "keypair", it's just two keys.

[dsheets]

A keypair is a secret key * public key where the public key is derived from the secret key. This is a specific construct that the NaCl designers decided to name with the compound keypair. I see your point regarding the ADJ-NOUN structure of the words that suggests one modify the other. To me, choosing between a compound name and a modified name is mostly a cosmetic choice.

In general, with naming decisions like this, I try to err on the side of consistency with a codebase, internal or external. Because of this, I think that the legacy name keypair and its specific use in the context of NaCl, libsodium, and related bindings outweighs the, perhaps more rational, alternative name.

I hope this makes sense and I apologize if I've frustrated you. Thank you, again, for your excellent contributions.

[whitequark]

No, it's not frustrating at all. It's not a decision I care about that much. keypair is fine.