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

switch to gyp, add tests and more methods #5

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

warner
Copy link
Collaborator

@warner warner commented Dec 14, 2012

Hi! Thanks for writing node-nacl! I've got some improvements for you.

First off, this switches from waf to gyp, which (I'm told) is the preferred modern node-0.8.x extension mechanism. It bypasses the upstream "do" script, instead it imports a modified copy of nacl (via the "import.py" script). This copy puts real .c and .h files in the "subnacl/" directory, so gyp can use all its own compiler flags and build process. I think this ought to fix the 32-vs-64-bit problems you were having. It also speeds up the build time immensely. The downside is that it always uses the portable "ref" version of the code, so you don't get the super-fast assembly versions (see https://blog.mozilla.org/warner/2012/01/19/improving-the-pynacl-build-process/ for my speed comparison of ref-vs-asm variants).

I also added a "raw" interface, require("nacl").nacl, which should map directly to the upstream C/C++ API. Personally I favor a more OOP-ish interface (like in my python-ed25519 package), since that makes it harder to accidentally swap the public and private keys, but I figure it's most useful to provide the same API as djb's library does, and then write separate wrappers on top of that.

I added access to some other nacl functions (secretbox, onetimeauth, stream). And I added some unit tests for most of them (copied from djb's papers on xsalsa20 and poly1305), using the "tap" test runner.

I'm new to node.js and gyp, so please let me know what could be improved.

thanks!
-Brian

@warner
Copy link
Collaborator Author

warner commented Dec 14, 2012

Two other notes:

  • it might not be obvious, but this removes the intermediate "build a library" step. It just compiles all the individual NACL files as if they were source files, and compiles the node_nacl.cc glue file in the same way, then links them all together into a .so. When I worked on python bindings for nacl, I found this worked much better (and was much more portable, since gyp or python knows what CFLAGS are needed to make a dynamically-loadable module) than building a library and then linking against it.
  • the crypto_sign algorithm in NACL is going to change in the next release to use Ed25519. (The code in the current 20110221 release is related, but uses a different and incompatible signature format). I'd recommend exposing this function under a different name (maybe crypto_sign_edwards25519sha512batch()), and not claiming the generic crypto_sign name quite yet. That way, folks who deploy this code now can keep using the same (longer) name, and not be surprised by incompatibilities when the new NACL fixes crypto_sign, but users who build systems after the change (or who can deploy new code) get to use the short name and the new algorithm.

@thejh
Copy link
Owner

thejh commented Dec 14, 2012

Oh, wow. That is great!

However, I haven't touched any nodejs stuff in months, so all my node-related work is currently pretty much abandoned. In case you want to take over package ownership in npm, just ask. Also, it doesn't seem to build on my machine:

> [email protected] install /home/jann/tmp/node_modules/nacl
> node-waf clean ; node-waf configure build

Nothing to clean (project not configured)
Checking for program g++ or c++          : /usr/bin/g++ 
Checking for program cpp                 : /usr/bin/cpp 
Checking for program ar                  : /usr/bin/ar 
Checking for program ranlib              : /usr/bin/ranlib 
Checking for g++                         : ok  
Checking for node path                   : not found 
Checking for node prefix                 : ok /usr/local 
'configure' finished successfully (0.122s)
Waf: Entering directory `/home/jann/tmp/node_modules/nacl/build'
[1/2] cxx: node_nacl.cc -> build/Release/node_nacl_1.o
Waf: Leaving directory `/home/jann/tmp/node_modules/nacl/build'
Build failed: 'NoneType' object has no attribute 'parent'
npm ERR! [email protected] install: `node-waf clean ; node-waf configure build`
npm ERR! `sh "-c" "node-waf clean ; node-waf configure build"` failed with 1
npm ERR! 
npm ERR! Failed at the [email protected] install script.
npm ERR! This is most likely a problem with the nacl package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR!     node-waf clean ; node-waf configure build
npm ERR! You can get their info via:
npm ERR!     npm owner ls nacl
npm ERR! There is likely additional logging output above.

npm ERR! System Linux 3.6.7
npm ERR! command "/usr/local/bin/node" "/usr/local/bin/npm" "install" "git://github.com/warner/node-nacl.git"
npm ERR! cwd /home/jann/tmp
npm ERR! node -v v0.8.10
npm ERR! npm -v 1.1.62
npm ERR! code ELIFECYCLE
npm ERR! 
npm ERR! Additional logging details can be found in:
npm ERR!     /home/jann/tmp/npm-debug.log
npm ERR! not ok code 0

@warner
Copy link
Collaborator Author

warner commented Dec 14, 2012

Hm, it should be using gyp, not waf (I deleted the wscript file entirely). I've only tested it on node-0.8.15 (on OS-X, via homebrew) though, maybe 0.8.10 behaves differently? I'll ask some more-node-experienced coworkers to review it.

I'd be happy to take it over. My npm user name is warner. Thanks!

@thejh
Copy link
Owner

thejh commented Dec 15, 2012

I did a npm owner add warner nacl - I think you should have access to the npm package now.

These take a few minutes to run, we might want to run just a subset.
The failing tests were modifying a random byte and then confirming that
authenticators were invalid. But with Math.round() they were sometimes
modifying a byte off the end of the Buffer, hence not modifying the
buffer at all, so the authenticator was still valid.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants