Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings
This repository was archived by the owner on Aug 5, 2021. It is now read-only.

Make signal-protocol compatible with JS module system #11

Open
elsehow wants to merge 2 commits into signalapp:master
base: master
Choose a base branch
Loading
from elsehow:master

Conversation

@elsehow
Copy link

@elsehow elsehow commented Dec 11, 2016
edited
Loading

This PR makes libsignal-protocol-javascript compatible with JS's module system. The test suite now requires the library, bundling it into a single dependency. We also add an integration test.

For backward compatibility, we still build dist/libsignal.js, which callers can include in their HTML. They can then refer to window.libsignal as before.

This package will not work in node due to a dependency on WebCrypto API. We could look into polyfills, or allow the caller pass in their own primatives (though I do NOT think this is a good idea). I think the node community would be very curious to hear WhisperSystem's recommendations on good native polyfills (NaCl perhaps?)

P.S. The scope of this PR increased dramatically from the initial PR. Sorry! Since the PR auto-updated on my big commit, I decided to just roll this whole PR into one.

Thank you for all of the amazing work on this library.

derhuerst, niksmac, blahah, strugee, michael-smith-nz, tristanhoy, danrubins, and rahulgi reacted with thumbs up emoji
Added instructions for setting up tests with Sauce Labs credentials.
Removed unnecessary dep on grunt-sass
@elsehow elsehow mentioned this pull request Dec 11, 2016
This PR makes libsignal-protocol-javascript compatible with JS's module system.
The test suite now requires the library, bundling it into a single dependency.
We also add an integration test.
For backward compatibility, we still build dist/libsignal.js, and attach
the library to window.libsignal.
A few points remain, for the contributor's consideration:
- This package will not work in node due to a dependency on WebCrypto API. We could look into polyfills, or allow the caller pass in their own primatives (though I do NOT think this is a good idea).
- A few things are listed as dependencies in package.json. However, we are bundling those into a file anyway, so they are really dev dependencies (npm will not need to install them).
This is a combination of 21 commits. Each commit's comments listed below.
browserifying some test bundle
Starting to bundle the tests.
There is now a file test/main.js that requires and executes all the tests.
Each test file is now responsible for requiring its own dependencies.
test/index.html will eventually do nothing more than require chai/mocha/etc test lib bundle (test/test.js, which is built in the gruntfile concat task), and the test suite bundle (build/test_main.js). Perhaps we should move this bundle to test to be like build.
Crypto.js tested + module-ified
Curve.js still relies on some stuff in Internal that we will have to track down eventually.
Continuing bundle-ifiabilty
Stopping me now is the way protos/WhsiperTextProtocol.proto is built
I assume this is an emscripten setting in grunt.
What I intend to do is remove all Internal = Internal | {} garbage
and replace with a single module.exports at the end.
This way, the native stuff with expose itself as a function/object.
SessionCipherTest passing
This one required fiddling with the way protos are built
I simplified stuff I think without breaking anything - just a one line change after some thinking. It seems that Whisper devs intended to add more protobufs eventually. I'm not sure if that's still in the plans, but they should be able to with some hacks. However, a more module-friendly dynamic loading solution may be easier to reason aout. Not bothering to write that for now.
KeyHelper and NumericFingerprint pass
Nothing fancy here, just requiring.
SessionBuilderTest passing
Also added a few untracked files.
After all tests pass, I'm realizing I'll need to go back and make the build workflow sane again for testing. The browserify step is really *only* for the tests.
Passing store tests
The store tests come in a set of 5.
Each gets initialized as a function,
and SignalProtocolStore_test.js ties them all together.
Where before the methods were being called from out of scope,
here we require each store test in test/main.js, then pass all of these functions into test/SiganlProtocolStore_test.js, which now exposes a higher-order function.
Passing all tests!
Woohoo!
Now it's time to deal with those final places where I'm "cheating." In test/index.html, there are three things from ../build/ (and one from ../src/, a worker) that are still being added to the HTML the old fashioned way, through script tags.
The first order of business is that stuff in build/. Figuring out how it's written to JS, and how to module-ify it (and where to include it later) will surely take some time.
Moving toward modular emscripten + worker
I finally figured out how to require emscripten code from browserify!
http://insertafter.com/en/blog/native-node-module.html?
Now that I can require curve stuff instead of appending,
i'ts time to get the webworkers into browserify too.
Of course, substack has written something for this:
https://github.com/substack/webworkify
and it looks like that'll do what I need
....but, I'll need to get it working with the existing manager, first
That's a mindfuck in itself because of all the zig-zagging references, even within the small file.
What I'll need to do first is make sure 1 worker can get launched and do its job.
After that, we can tidy up the references in that file,
then make sure all references to startWorker fall in line.
Fixed typo
The crypto all seems to work now.
Not sure what the webworker is for - will return to it once I figure that out.
Amazingly, the only thing left is a regular old npm module.
Why the unexpected behavior? Is Long the issue?
How can I force Long in browserify, if so?
Fixed webworkers
WebWorkers now seem to work with browserify.
Bundled tests lib in build/
They were getting written to test/ before - confusing, no computer-generated stuff in test/
That should be human-written source only.
Now everything in test/index.html points to something in build/
Working!
All bundled up!
The trick was just requiring the bundled file apparently (?)
Only things left to do are:
- Make sure blanket runs at end-of-tests
- Turn linting back on
- Write up changes in README
Added blanket; tests fail
After adding blanket back, tests are suddenly failing!
Not sure why.
Removing blanket again
Indeed, removing blanket fixes the failures.
Oh well, will leave it out for now.
All tests running
Preserving legacy builds
We still have a prebundled JS file for people who want to include in HTML.
Passing grunt jscs
Updated jshint
Linting back on
@elsehow elsehow changed the title (削除) Added test instructions, removed unused dev dep (削除ここまで) (追記) Making signal-protocol compatible with JS module system (追記ここまで) Dec 12, 2016
@elsehow elsehow changed the title (削除) Making signal-protocol compatible with JS module system (削除ここまで) (追記) Make signal-protocol compatible with JS module system (追記ここまで) Dec 12, 2016
Copy link

derhuerst commented Dec 13, 2016
edited
Loading

This package will not work in node due to a dependency on WebCrypto API. We could look into polyfills, or allow the caller pass in their own primatives (though I do NOT think this is a good idea). I think the node community would be very curious to hear WhisperSystem's recommendations on good native polyfills (NaCl perhaps?)

I also tried to polyfill WebCrypto using Node, so that this lib accepts it. Node's core crypto module supports some of the primitives used in WebCrypto afaik.

there's also a WebCrypto polyfill, but i'm not sure if is being kept up to date (& audited?).

Edit: There's another one which seems to be better maintained, but a few tests are missing.

Copy link

I'm really a crypto beginner, but i got started to poke into building a Node shim like this:

npm i webcrypto@anvilresearch/webcrypto
npm i bytebuffer@dcodeIO/bytebuffer.js
npm i protobufjs@dcodeIO/protobuf.js
// pre bundle
// 'use strict';
const window = Object.assign(Object.create(global), {
	crypto: require('webcrypto')
})
const libsignal = {}
const dcodeIO = {
	ByteBuffer: require('bytebuffer'),
	ProtoBuf: require('protobufjs')
}
;(function (window, libsignal, dcodeIO) {
// post bundle
})(global, libsignal, dcodeIO);

Copy link
Author

elsehow commented Dec 13, 2016
edited
Loading

@derhuerst Thanks for your reply. Since the Node shims are out of scope for this PR, I'm working on them here:

http://github.com/elsehow/signal-protocol/

For Crypto, I'm using node-webcrypto-ossl, which wraps OpenSSL. WebWorkers requires a shim as well.

Copy link

rmhrisk commented Dec 15, 2016
edited
Loading

FYI :

We maintain a few WebCrypto polyfills:
https://github.com/PeculiarVentures/webcrypto-liner/blob/master/BrowserSupport.md
https://github.com/PeculiarVentures/node-webcrypto-ossl
https://github.com/PeculiarVentures/node-webcrypto-p11

Our focus is test coverage and compat, you can see some of the tests here: https://peculiarventures.github.io/pv-webcrypto-tests/

WebCrypto-liner would be useful for your WebWorkers case.

We also have been thinking about extending these with ed25519 support and would consider prioritizing it if this project needed it.

Copy link

Stumbled upon this today and have been trying to get a working prototype up and running. Looks good so far, using mongoose-encrypt as a backing store for the long-term storage side of the app.

Would love to see this merged into the core repo as building my own shim for Java has been fruitless, so far.

Copy link
Author

elsehow commented Feb 13, 2017

@d3sandoval in the meantime, check https://github.com/elsehow/signal-protocol

DreaminDani and thekelvinliu reacted with thumbs up emoji

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /