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

Remove Traceur, V8-incompatible syntax sugar to run in node directly #15

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

Conversation

dcposch
Copy link

@dcposch dcposch commented Jan 23, 2016

Node v4 supports most of ES6 by default now! This makes debugging easier.

Changes

  • Remove traceur dependency

  • Remove some of the fancy ES6 syntax so that we can run in node directly

  • Clean up tests, remove jscs and jshint.

    (Locally I've replaced them with standard, getting rid of a good amount of unused code in the process, but I'll make a separate PR for that since it creates a huge diff.)

  • Remove dist folder. The main entry point is the readable index.js rather than the generated dist.js

@joebandenburg
Copy link
Owner

Unfortunately, this library is designed to be run by both Node.js and in the browser. Removing Traceur means that it won't run in the browser. It would still be possible via Webpack, but we'd certainly need to update the README to explain this.

@dcposch
Copy link
Author

dcposch commented Jan 26, 2016

You're right... I thought it worked, but apparently only in Chrome, FF, and Edge, not in IE or Safari. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*#Browser_compatibility

Annoying.

Curious, is anyone using libaxolotl in a webapp? Using it in a node program, an Electron app, or Chrome app seems reasonable, since all of those can be installed in a reasonably verifiable way (using shrinkwrap etc for reproducible builds, signing each release, etc).

Doing end-to-end encryption in an webapp, on the other hand, seems questionable. Every time you load the app, you're trusting the server to send you untampered-with code, at which point you might as well trust it to handle the messages as well.

@joebandenburg
Copy link
Owner

Curious, is anyone using libaxolotl in a webapp? Using it in a node program, an Electron app, or Chrome app seems reasonable, since all of those can be installed in a reasonably verifiable way (using shrinkwrap etc for reproducible builds, signing each release, etc).

That's a good point. I agree that it should not be used for online webapps currently - although I recently become aware that progress is being made in that area.

My concern is that there may be offline JavaScript environments out there that aren't ES6 compatible. As I said, though, it's perfectly reasonable to assume that they could use Babel or Traceur themselves so this library doesn't have to.

I can see the benefit, it would certainly simplify the build process for this library.

(Locally I've replaced them with standard,

What is standard?

I'm happy to drop JSCS and JSHint in favour of something better. I've used ESLint recently, which I like.

@@ -15,9 +15,9 @@
* along with this program. If not, see <http://www.gnu.org/licenses/>.
*/

import Axolotl from "./src/Axolotl";
import axolotlCrypto from "axolotl-crypto";
const Axolotl = require("./src/Axolotl")
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer to keep semicolons on the end of lines, please.

@alax
Copy link

alax commented Jan 26, 2016

@dcposch
Copy link
Author

dcposch commented Jan 26, 2016

standard uses ESHint under the hood, and is nice in that it requires no configuration. It's used by a bunch of projects including npm itself

If you prefer to keep the semicolons, I can use semistandard instead :)

@dcposch
Copy link
Author

dcposch commented Jan 27, 2016

Subresource integrity is cool by the way, but doesn't provide a way to install a webapp securely. You still trust the server every time you use the app.

What it does provide is a way to use CDNs etc securely: instead of just including, say, <script src="https://some-cdn.biz/yolo">, you include <script src="https://somecdn.biz/yolo" integrity="<sha512-hash>" />, so you no longer have to trust the CDN to serve untampered code. The user still has to trust you to serve correct code each time they open the webapp.

@joebandenburg
Copy link
Owner

It seems to me that, as it stands, this pull request is changing two things at once:

  1. Making the code run natively in Node 4.2.
  2. Changing the style of the code.

Would you mind moving the style changes out of this pull request? I think I'm sold on 1 but not on 2.

@jcbrand
Copy link

jcbrand commented Jan 27, 2016

Curious, is anyone using libaxolotl in a webapp? Using it in a node program, an Electron app, or Chrome app seems reasonable, since all of those can be installed in a reasonably verifiable way (using shrinkwrap etc for reproducible builds, signing each release, etc).

That's a good point. I agree that it should not be used for online webapps currently - although I recently become aware that progress is being made in that area.

For what it's worth, I'm keeping an eye on the development of this library because I'm considering using it to add OMEMO encryption support to converse.js which is a browser-JS XMPP chat client. OMEMO relies on Axolotl.

Converse.js is a purely client-side JS library that only communicates with an XMPP server and with no other backend web application. So this is a usecase where this library would be used purely in the browser.

Previously I've added OTR support (via otr.js ) to converse.js and it remains a relatively popular feature. People have since asked for OMEMO support as well.

Thanks and keep up the good work.

This makes debugging easier

Clean up tests, remove pure-cjs
@dcposch
Copy link
Author

dcposch commented Jan 27, 2016

@joebandenburg makes sense, fixed!

  • JSCS and JSHint are back
  • More simplification in the Gruntfile and package dependencies
  • All of the Grunt tasks in the Gruntfile work again

To get all Grunt tasks working again, I had to:

  • Modify the integration-test task to use the source directly (instead of expecting traceur output)
  • Remove Bower / Karma / the duplicated integration test (test/browser was almost a copy paste of test/node), do all testing in Node. libaxolotl-javascript will still work in environments like Electron, Chrome apps, NW.js using Browserify when necessary.
  • Remove the coverage task for now, since Blanket doesn't support ES6. If we need it back in the future, it looks like we can use Istanbul.

LMK if this looks reasonable, and also whether you want me to squash the commits.

@dcposch
Copy link
Author

dcposch commented Jan 27, 2016

$ npm test && ./node_modules/grunt-cli/bin/grunt integration-test
> [email protected] test /home/dc/email/libaxolotl-javascript
> grunt test

Running "jshint:all" (jshint) task
>> 27 files lint free.

Running "jscs:all" (jscs) task
>> 26 files without code style errors.

Running "mochaTest:unitTests" (mochaTest) task


  ArrayBufferUtils
    areEqual
      ✓ returns true for equal arrays 
      ✓ returns false for arrays of unequal length 
      ✓ returns false for arrays of equal length but unequal values 
    concat
      ✓ concats all buffers into one large buffer if passed as varargs 
      ✓ concats all buffers into one large buffer if passed as an array 
    stringify
      ✓ converts an arrayBuffer to a string 
    parse
      ✓ converts a string to an arrayBuffer 

  FakeCrypto
    generateKeyPair
      ✓ returns key pair 
    calculateAgreement
      ✓ works 

  Axolotl
    ✓ is frozen 
    methods
      generateIdentityKeyPair
        ✓ returns value from generateKeyPair 
        ✓ calls generateKeyPair once 
      generateRegistrationId
        ✓ calls randomInt once 
        ✓ returns non-extended registration ids in the range [1, 16380] 
        ✓ returns extended registration ids in the range [1, MAX_INT] 
      generatePreKeys
        ✓ returns a list of pre keys of length count 
        ✓ calls generateKeyPair count times 
        ✓ returns generateKeyPair output 
        ✓ returns records with ascending ids start from start 
        ✓ wraps ids when they reach 2^24-2 
      generateLastResortPreKey
        ✓ returns a single pre key 
        ✓ calls generateKeyPair once 
      generateSignedPreKey
        ✓ returns a single pre key 
        ✓ calls generateKeyPair once 
        ✓ calls sign once 
        ✓ calls sign with correct arguments 
    communication
      ✓ accepts a simple exchange 
      ✓ accepts a simple multi message exchange 
      ✓ sends the first message as a PreKeyWhipserMessage 
      ✓ sends the second message as a WhipserMessage 
      ✓ accepts multiple messages sent by one party 
      ✓ accepts out of order message delivery (sub ratchet) 
      ✓ accepts a complicated message exchange (70ms)
      ✓ rejects messages that come after too many dropped messages 
      ✓ rejects duplicate PreKeyWhisperMessage delivery 
      ✓ rejects duplicate WhisperMessage delivery 
      ✓ rejects message with bad mac 
      ✓ rejects bad signedPreKey signature 
      ✓ rejects preKeyBundle if neither preKey nor signedPreKey are present 
      ✓ accepts optional oneTimePreKey 
      ✓ queues and eventually completes concurrent encryptMessage 
      ✓ queues and eventually completes concurrent decryptPreKeyWhisperMessage 
      ✓ queues and eventually completes concurrent decryptWhisperMessage 
      ✓ rejects version 2 of PreKeyWhisperMessage 
      ✓ accepts out of order message delivery (main ratchet) (45ms)
      ✓ rejects messages that are too far out of order (main ratchet) (57ms)
      ✓ returns the identity key and registration id when decrypting a PreKeyWhisperMessage 
      simultaneous session initiation
        ✓ handles both parties simultaneously initiating sessions 
        ✓ handles a dropped PreKeyWhisperMessage 
        ✓ handles a dropped first WhisperMessage 
        ✓ handles a long exchange of session disagreements (261ms)

  HKDF
    deriveSecretsWithSalt
      ✓ works with small inputs 
      ✓ works with large inputs 

  PromiseInterfaceDecorator
    ✓ throws if the passed in object doesn't implement the required interface 
    ✓ wraps the object's methods so that they return promises 

  Session
    ✓ is initially empty 
    ✓ clones the passed in state 
    addState
      ✓ puts the session onto the front of sessions array 
      ✓ removes the last element if the list is full 
    removeState
      ✓ removes the session from the list 
    mostRecentState
      ✓ returns the head of the list 

  SessionCipher
    decryptMessage
      ✓ rejects message with wrong version number 


  62 passing (926ms)


Done, without errors.
Running "mochaTest:integrationTests" (mochaTest) task


  axolotl integration node
    ✓ produces the expected output 


  1 passing (12ms)


Done, without errors.

We can probably replace it later with Istanbul, so that we get
code coverage reports again.
@dcposch
Copy link
Author

dcposch commented Feb 1, 2016

It seems to me that, as it stands, this pull request is changing two things at once:

  1. Making the code run natively in Node 4.2.
  2. Changing the style of the code.

@joebandenburg I've updated the PR to match the existing code style. Thoughts?

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.

4 participants