Skip to content
This repository has been archived by the owner on Feb 25, 2019. It is now read-only.

Webcrypto API #7

Open
christiansmith opened this issue Aug 17, 2015 · 45 comments
Open

Webcrypto API #7

christiansmith opened this issue Aug 17, 2015 · 45 comments

Comments

@christiansmith
Copy link
Member

We've experimented with a number of JavaScript crypto libraries (sjcl, crypto-js, jsrsasign, etc) in this package for various purposes. The most important case is verifying signatures. In the meantime, the WebCrypto API has been maturing and will eventually be ready to replace some or all of these dependencies.

We need to explore the potential for rewriting some of this code using WebCrypto, take stock of it's readiness for general use, and consider implementing fallbacks for older browsers that will continue to require polyfills or additional optional dependencies.

@henrjk
Copy link
Contributor

henrjk commented Jan 12, 2016

I started looking into a webcrypto implementation (so far without fallbacks). I got the unit test level to
pass, but only when I changed the JWK used in the unit tests.

The jwk public key can be considered invalid if one shares the understanding of the spec as currently implemented by Chrome

The problem is the key's modulus value (n) which converted to a number has leading zeros.

"n": "AJ4bmyK...OrjOmM=" converts to the following hex number:

009e1b9b22....73ceae33a63
or in full:
009e1b9b22bf7cba0430fba247ab873969618c945014fba7571587b06f2ec0f9de2663f10863db6e8c959421ad0f5c6c7c7b72808bbbce17cd6dbd408875b9a5cddb8b593cb9d3370874144ee9deb9d36d32420e0c69bfa535779d0d531f5b6bf5e6eab93dfad034c48649a3bffe4b670b27380d0701fff7186f5fbbc825e799a1a4a9f2856a646eb1ebcae87c731f215332f08b946be6003522b8503fd4855f6cbaf2cb924b4c29ec7a104c889ee845f2fc6d8e262ee4a894b45239172bb64fa9fe7a386066f93958066c325dd599ddb06096794f8c16faedec523225e68bec9e7856b9dad58b6653aa35fe8259bd97acd7fa3d60b1b74359ed5dc73ceae33a63

This causes Chromes webcrypto implementation to issue DOMException: The JWK "n" member contained a leading zero.`. This lead me to this issue in rsa-pem-to-jwk and then to this Chrome Issue 383998: Reject JWK which use non-minimal octet representation for big integers

When I adjust the modulus of the test key by dropping the leading "00" and then converting it back to its base64url representation then Chrome accepts it.

It appears this is no longer (or never has been) an issue in pem-jwk which is used by our connect server's key generation. See issue dannycoates/pem-jwk#2 for a test case.

@christiansmith: Do you know how the test key came to be?

Is it ensured that jwk delivered by the connect server will not have this issue?

@christiansmith
Copy link
Member Author

@henrjk – good find on the modulus value!

I don't recollect the origins of the key. It's been quite a long time since initially writing this code. In light of your discovery, we should verify that the server is publishing well-formed JWKs.

Since originally posting this issue, it's been suggested that we consider using https://github.com/cisco/node-jose under the hood for all things JWT in both the client and the server code. That library works in both node and the browser and uses WebCrypto API "where feasible".

It doesn't cover some of the additional OIDC requirements, so there's still a need for much of the logic in https://github.com/anvilresearch/connect-jwt, at least on the server side.

@msamblanet and @bettiolo might have some helpful input on this idea.

We can discuss all this at the hangout on Thursday if you're able to make it.

@bettiolo
Copy link

@christiansmith I cannot join you today as I am pretty busy at work.

While implementing my OAuth 1.0 signature generation library I inlined in the client side bundle Google's crypto-js.

I am using jsonwebtoken pretty much everywhere.
For the OIDC Provider library that I wrote I used rsa-pem-to-jwk for format conversions and crypto module for random bytes.
I used also rsa-pem-from-mod-exp in the client implementation and unit tests.

Pretty much the standard stuff.

@henrjk
Copy link
Contributor

henrjk commented Jan 14, 2016

@christiansmith Sorry for the late notice, I will not make it to todays hangout.

To me the crypto stuff makes less sense if it is not done natively.

I could see us providing several choices:

  1. what is in master today.
  2. a webcrypto only solution
  3. a solution with fallbacks perhaps using node-jose

I am currently exploring 2, the webcrypto only implementation. The units tests so far pass on the following browsers:

INFO [Safari 9.0.2 (Mac OS X 10.10.5)]
INFO [Firefox 43.0.0 (Mac OS X 10.10.0)]
INFO [Chrome 47.0.2526 (Mac OS X 10.10.5)]
INFO [Edge 13.10586.0 (Windows 10 0.0.0)]
INFO [IE 11.0.0 (Windows 7 0.0.0)]

I have not yet tested against recent mobile platforms and also haven't done manual testing against the connect server.

I am currently less interested in working on a fallback solution although I'd be happy to learn more about node-jose. At first glance it looks pretty heavy weight for the browser side.

@henrjk
Copy link
Contributor

henrjk commented Jan 20, 2016

I am aiming to have this module use only webcrypto.
However I am designing the code to use two adapters for crypto operations:

  1. jwt-validator: does jwt validation, and also decodes the claims. Also has a lifecycle method to request the server public key (jwk).
  2. encryptor: provides encrypt/decrypt operations to serialize/deserialize a session, nonce generation and sha sums.

This project would implement these adaptors using the webcrypto API. It would also have an API to allow overriding these either replacing or as fallback with custom implementations.

There would be additional modules to implement these like code that is currently in master.

@henrjk
Copy link
Contributor

henrjk commented Jan 20, 2016

I am thinking to have us publish this module as npm CJS module intended to run inside a browser.
Note however that at least in theory the webcrypto API could also be available under node. See this issues where these is contemplated: nodejs/node#2833

The npm module code would be ES5 code while I am having the implementation sources in ES6.

@christiansmith
Copy link
Member Author

@henrjk it's really important that we start reviewing and discussing this work.

This area particularly requires extra eyes and healthy debate. Crypto is dangerous to "them" used correctly and dangerous to us(ers) if not. Complicating the matter, as I understand it there's not a great deal of support for WebCrypto API among cryptographers.

Can you give us a first look at the hangout tomorrow?

@henrjk
Copy link
Contributor

henrjk commented Jan 20, 2016

I pushed the latest code to my fork at https://github.com/henrjk/connect-js/tree/webcrypto.
The crypto code itself should be ready to review.
webcrypto code can be found in subtle-crypto-utils.js which is used by

Now the main file for which a prior version is in master is in anvil-connect.js.

Crypto related tests are in

All links here go to master so may shift as new commits are made.

@christiansmith and all. I'd be happy to go over this tomorrow in the hangout. Also I am curios to learn more about why there is no support for the WebCrypto API from cryptographers and whether there are better alternatives.
I definitely do not claim to be an cryptographer myself.

@christiansmith
Copy link
Member Author

@henrjk thanks for the code review in the hangout. This is great work! Look forward to testing it out.

@bettiolo
Copy link

😢 that I missed it

@henrjk
Copy link
Contributor

henrjk commented Jan 26, 2016

I have updated code at
https://github.com/henrjk/connect-js/tree/webcrypto

This code is now in a state that is (hopefully) pretty close to being done. However currently token claims are not verified. This is something I wanted to look into also. Aside from that it should be good for testing it out.

I also have an updated example fork at https://github.com/henrjk/connect-example-angularjs/commits/use-npm . The readme has a section Get connect-js client libraries which should help getting the example consumed.

Anyone who is consuming this, please let us know any issues you encounter and also if things just worked. Thank you!

@henrjk
Copy link
Contributor

henrjk commented Jan 26, 2016

I wanted to mention that I have created two projects which contains an iteration of the crypto code currently used in the master branch of connect-js. They are:

They can be used by an app which must run on browsers not supporting subtle.crypto (after shimming).
If this fork gets PRed to the mainline then it might make sense to also make the above available under the anvilresearch world.

@henrjk
Copy link
Contributor

henrjk commented Jan 28, 2016

I updated the code:

https://github.com/henrjk/connect-js/tree/webcrypto
https://github.com/henrjk/connect-example-angularjs/commits/use-npm

The updated example version will not work with the earlier connect-js version.

The connect-js library is now only dependent on angular (and q) for its tests.
Also I added a first cut of verifying claims. This is meant to be reviewed: The claim verification code is here.

Comments welcome!

@christiansmith
Copy link
Member Author

@henrjk this all looks excellent at first glance. Look forward to trying it out. Are you still free for pairing early next week? Monday or Tuesday should work for me.

@henrjk
Copy link
Contributor

henrjk commented Feb 2, 2016

Just updated both connect-js and connect-example-angularjs forks with some changes based on our pairing yesterday. Changes in connect-js were to:

  • added a dist script which generates bundles which can be included with a script tag. See commit for more information. However using CommonJS and npm makes more sense to me.
  • fixed the issues with using Array.find which caused problems in the example app.
  • jspm initialize now happens during npm install so that user does not have to install jspm globally or think about it.
  • fixed a minor other issue (order of local storage writes which could cause incomplete state in listeners).
  • updated the version to 0.2.3 in package json to make it easy to verify that one has the changes.

For connect-example-angularjs the following changes were made:

  • fix the problem were the destination would not update correctly
  • eliminate usage of bower
  • improve grunt --help (grunt-h) messages.
  • pull app configuration into separate area which can be populated with grunt config now after editing authconf.json. The registration script should now be executable.

I looked a bit into the rp/op session management and it is not perfectly working at the moment. Is this supposed to also work if you use a different browser such Firefox and Chrome?

I think this version could be used for more testing from interested parties.

@tomkersten
Copy link
Contributor

Awesome. I plan to give this a try...I'm hoping this week. I'll drop any input in here or gitter if I run into issues.

@henrjk
Copy link
Contributor

henrjk commented Feb 3, 2016

@tomkersten don't hesitate trying to reach me.

I just pushed a small improvement for register_with_anvil_connect.sh improve nvl error reporting to have better reporting when registration does not work.

I might continue pushing small changes like this as I discover issues.

@henrjk
Copy link
Contributor

henrjk commented Feb 3, 2016

pushed changes relating to make popup authorization work in both

@henrjk
Copy link
Contributor

henrjk commented Feb 3, 2016

Updated example readme

@christiansmith
Copy link
Member Author

@henrjk thanks for all your hard work on this. I look forward to merging the eventual PR!

Before we do that, it will be great to have feedback from users of the current release. Please everyone, give @henrjk's fork a try and let us know how it works for you.

Thanks @tomkersten. @hedleysmith, you might be interested as well (the fork works with npm).

@henrjk
Copy link
Contributor

henrjk commented Feb 3, 2016

I just pushed changes so that session management work in both

With work I mean that you can now open two windows with the example in a browser and then when the login state changes the other window properly reacts as well.
However at the moment this does not work when different browsers are involved. @christiansmith: should this work with different browsers?

@tomkersten
Copy link
Contributor

I'm pretty sure this depends on localStorage/cookies so it would be
session/browser-dependent...

Pretty sure. 😉

-tom

On Wed, Feb 3, 2016, at 10:02 PM, Henrich Kr�mer wrote:

I just pushed changes so that session management work in both

With work I mean that you can now open two windows with the example in a
browser and then when the login state changes the other window properly
reacts as well.
However at the moment this does not work when different browsers are
involved. @christiansmith: should this work with different browsers?


Reply to this email directly or view it on GitHub:
#7 (comment)

@henrjk
Copy link
Contributor

henrjk commented Feb 3, 2016

Thx! That's what I thought. But wanted to double check...

@henrjk
Copy link
Contributor

henrjk commented Feb 4, 2016

The OpenID Connect session management defines how to monitor the End-User's login status at the OpenID Provider on an ongoing basis so that the Relying Party can log out an End-User who has logged out of the OpenID Provider.
An OpenID Connect Provider (OP) server could (perhaps) associate two sessions from different browsers if they are for the same user. So if one session logs out the server could then also log out the other session and communicate the change to the browser (RP) with the specified OIDC session management.

Within a single browser the session management between multiple tabs/windows works solely using localStorage events at the moment. I validated this by commenting out the creation of the OP and PP frames used for OIDC session management and this continued to work.
With OP/RP frames enabled in this case, one can also see OIDC session management events but these are not currently needed to synchronize the authentication state.

I tried whether an OIDC session management event is generated when tokens expire but did not see this happening.

Unless there are cases where OIDC session management state changes would be triggered we should probably disable the RP/OP iframes as we can't really test them.
Did I miss a scenario (or made a mistake testing) where the OIDC session management could be tested?

@christiansmith
Copy link
Member Author

@henrjk@tomkersten is right, this is browser specific. I also think we don't want to link sessions between browsers.

The OIDC Session spec is among the most frustrating specifications I have ever worked with. It's not even entirely clear about it's purpose. The main utility, IMO, even though it's framed up as "log out an End-User on the RP who has logged out of the OpenID Provider", is single sign-on between multiple hosts/RPs.

The localStorage events you're referencing are something unique to Anvil, it's our way of getting that real-time feeling SSO between different Relying Parties (and coincidentally multiple tabs with the same app). It's outside the scope of the spec.

Without our enhancements, changes in login state would only be apparent between page loads. That doesn't work particularly well for SPAs.

We should pair on this particular subject because it's not well documented, horribly specified, and definitely one of the darker corners of the code.

@tomkersten
Copy link
Contributor

@henrjk I [finally] got around to trying this out (sorry for delay), but am running into a silly issue that I can't quite figure out...

I've downloaded your fork, built, and link it in jspm...then installed the link in another app using JSPM. I get an error about TextEncoderLite not being defined in ab-utils, but I see it imported at the top and can see the libs are installed, so I'm not sure what I'm doing wrong.

Any thoughts?

@tomkersten
Copy link
Contributor

I have @henrjk's fork working locally with an Aurelia application and it looks great. The API is nice and the code reads great. I have not done extensive testing with it, but the authentication against an existing provider works as I would expect. I will throw a few more scenarios as it, but for now, it's looking good.

@henrjk
Copy link
Contributor

henrjk commented Mar 28, 2016

@christiansmith asked me to prepare a PR for this at https://github.com/anvilresearch/connect-js/tree/webcrypto-api and perhaps also for the example.

I'd prefer to base these PRs on a cleaned up commit history for the following two branches:

Has anyone made changes based on the commits in the branches above (which are not in the origin)?
If so please let me know right away! Otherwise my plan is to create new branches with a cleaned up history and base the PRs on these new branches.

@henrjk henrjk mentioned this issue Mar 31, 2016
henrjk added a commit to henrjk/connect-example-angularjs that referenced this issue Mar 31, 2016
This adopts usage of the WebCrypto Api based connect-js library
as tracked on
anvilresearch/connect-js#7 (comment)
With this most API methods are now returning promises.

Summary of other  changes:

* Use npm and browserify to consume connect-js
* Adopt latest anvil cli

The changes were made over a period of time. Here are some of the more
notable original commits with comments:

COMMIT: callback_popup to check whether opener is available
COMMIT: index.html shows userInfo.name may not be available
COMMIT: add APP_AUTH_CALLBACK variable
COMMIT: establish scope.session in SigninCtrl and MainCtrl
COMMIT: scope to requireScope profile instead of realm
COMMIT: adopt new cli
COMMIT: Handle disconnected popup in passwordless login.

The passwordless login method sends the user a link in an email. When
the user follows this link a new browser window opens.

However the original popup window will not redirect and remains open
with a page allowing to resend the link.

The connect-js library was changed to handle this case so that it would
expect the page which is opened when the email link is followed
to handle the callback so that the session is populated appropriately
in localStorage.

connect-js now listens for that and then closes the popup window.
It also expect the email page to close itself.

This may not work for all browsers.  I did see this work in Chrome and Safari
but fail in Firefox.
Note that the popup Callback should behave in the same way it it is
redirected by the server, which happens when other authentication
methods are used.

This also adds the newer dependencies and improves logging similar to
how it was done in connect-js.

COMMIT: refresh angular app on passwordless login
COMMIT: adding jspm bundle-sfx for demo

COMMIT: Manual angular bootstrap

This was to avoid issue systemjs/systemjs#1032

COMMIT: Switch to using npm and browserify

This now requires anvil-connect-js ^0.2

bower is still used for shims and bootstrap.

Some bugs were addressed found during manual testing.
Also added more logging using bows.

COMMIT: Added section Get connect-js client libraries

I am uncertain about the remainder of the doc, but it looked OK to me at
first glance although I have not done a client registration for quite some
time.
@henrjk
Copy link
Contributor

henrjk commented May 5, 2016

Section github-rate-limits describes the changes I did in my fork to make the travis build pass.

In a nutshell:

  1. Create a personal access token for GitHub to this repository. The access token is equivalent with a password (so don't share unencrypted). Therefore setting a limited scope is a good idea. Scope: 'public-repo' was sufficient to build this (open source) project.
  2. The .travis.yml file expects that the `JSPM_GITHUB_AUTH_TOKEN=' is available. I would suggest to add this in Travis to the corresponding project settings.
    Alternatively as I did in my fork one can add it encrypted in the .travis.yml file. I think it like the project setting better although one can certainly argue either way.

@dmitrizagidulin
Copy link
Member

dmitrizagidulin commented May 31, 2016

@henrjk - I've been looking through your work on porting connect-js to use native webcrypto, at the webcrypto-api branch. WOW you have done a lot of work, great job btw! I have a couple of questions.

  1. Why the choice of jspm to bundle the library, instead of something like browserify or webpack?
  2. Do you have a sense of which browsers & browser versions the usage of webcrypto-shim opens up? As in, it seems to be the point of the rewrite to webcrypto is to stop using the various javascript crypto libraries, and the use of a shim sort of defeats that purpose.
  3. Do you think it would be possible to have bows be an optional dependency (maybe via dependency injection)? So that devs who want to use it can init & pass it in, and those that don't can use something built-in like console.log?

@dmitrizagidulin
Copy link
Member

Oh, one more question - what is tiny-emitter used for, in the design?

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

@dmitrizagidulin let me address your question 2 first.

webcrypto-shim Readme has more details about the supported browsers.

IE implemented crypto primitives with a different API. The shim provides an adapter so that IE 11 can then be used with the webcrypto API. The adapter code is not actually implementing any crypto primitives and is simply a thin layer.
In addition Safari 8+ still uses a prefix which the shim also takes care of.

From the web-crypto shim readme: These browsers have unprefixed and conforming webcrypto api implementations, so no need in shim:
Chrome 43+, Chrome for Android 44+,
Opera 24+,
Firefox 34+,
Edge 12+.

This comment way up lists browser version I have tested with. This looked all good to me.

Now while the tests are using the shim it is not part of the connect-js library. Users of the library can decide to use the shim or not. The example connect-example-angularjs/index.html for example does include the shim.

I will answer your other questions separately hopefully later today.

@dmitrizagidulin
Copy link
Member

re webcrypto-shim -- makes sense, thank you!

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

Regarding your bows question: "Do you think it would be possible to have bows be an optional dependency (maybe via dependency injection)? So that devs who want to use it can init & pass it in, and those that don't can use something built-in like console.log?"+

I investigated a bit about using optional dependencies for supporting fallback legacy crypto libraries under jspm and have a bunch of references of various discussions. For example here.
Ultimately I decided to not pursue conditional loading mostly because I could not convince myself that it would be a good thing for the fallback use case. In addition it appeared to me that jspm was not settled yet on how this should be done. If I understand correctly however the trajectory is that the dependency graph with explicit conditions would be captured and then during build time pruned based on what the condition variables actual values were.

Bows uses andlog which uses console.log. With andlog logging only happens if one sets debug in localStorage by typing this in console:

localStorage.debug = true

Ultimately this allows to not having to remove log statements from source which I prefer. What I liked about bows is mostly that one can add a 'module' prefix in a simple way to each log statement in the code. Also bows offers levels such as debug, error and more. I found it useful in a few cases to mark things as more than debug.

Ultimately I view the logging code only useful for development.

@dmitrizagidulin Do you have a broader use case for logging in mind? Does this make sense to you?

@dmitrizagidulin
Copy link
Member

I don't necessarily have a broader use case in mind, as far as logging. I'm only asking about it because it's a dependency we're bundling that's not part of the core functionality, seems to be a developer nicety.
So I was wondering if there was a way not to require it.

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

Although logging here is mostly a development concern, at least until webcrypto implementations mature, one might want to ensure/require that logging is available so that users can switched it on for troubleshooting. So I am not certain that making it optional at this stage would be the best.

However if others feel this should be optional I would not object. However I can't promise to have the cycles to investigate how to do this at the moment.

I am not fixated on bows in particular although I found it nice to work with.

Here is some more data about bows:

# The bows dist folder has andlog embedded
[email protected] dev (webcrypto2)$ wc dist/*.js
     143     451    4516 bows.js
       2      66    2524 bows.min.js

andlog alone is:

[email protected] dev (webcrypto2)$ wc andlog.js
      29      92     837 andlog.js

While it would probably good enough to use andlog instead of bows. I am not sure whether this makes much of a difference as there would still be a dependency.

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

@dmitrizagidulin regarding tiny-emitter.

This was introduced in commit f0f062f (see 'Handle disconnected popup in passwordless login' for more details).

This library supports asking credentials in a popup window. With a normal login this window redirects to a callback which then closes its parent window.
However with passwordless login (see Support passwordless login as an option) the login happens when an email link is activated which opens a new unrelated browser window. An 'authenticated' event was introduced for this and tiny-emitter was the most lightweight way I found to implement it. The popups parent window listens whether an authentication is established. The 'authenticated' event is emitted in response to a local storage 'anvil.connect' event when the user is authenticated.
This reacts to an authentication performed in another window or tab.

You can search for .emit and .on in the code. It is also used in the angular example where it was useful for Angular to react properly to login/logout discovered by OpenID Connect session management.

Note that passwordless login has not been pulled in yet.

@dmitrizagidulin
Copy link
Member

Got it, thanks :)

(Btw, I really appreciate you answering these 'why' questions, it's very helpful!)

One more - what's the difference between base64-js and text-encoder-lite? On first glance they seem to be doing a similar task?

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

Yes, I really appreciate your questions as well.

text-encoder-lite encodes/decodes UTF-8 to/from a byte array.
base64-js encodes/decodes base64 to/from a byte array.

There is a section about UTF-8 encoding/decoding in connect-js/devnotes.md.

@dmitrizagidulin
Copy link
Member

Ah, I see. And what are your thoughts on using https://github.com/inexorabletash/text-encoding instead of text-encoder-lite, since you mentioned them too?

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

@dmitrizagidulin Why jspm not webpack or browserify?

I would think that all these tools could be made to work in this context. jspm's ES6 support looked most compelling to me and in particular it seemed using ES6 modules might be the future.

@henrjk
Copy link
Contributor

henrjk commented May 31, 2016

https://github.com/inexorabletash/text-encoding is much larger because it supports many encodings we don't need.

@dmitrizagidulin
Copy link
Member

dmitrizagidulin commented May 31, 2016

I think jspm is a bit heavyweight for this project. As far as ES6 support - it's using babel anyway, so that'll work nicely with browserify or webpack too. (I really dislike jspm's load-on-demand approach.)
And the fact that it side-steps npm's node_modules directory and uses its own version of that.

@henrjk
Copy link
Contributor

henrjk commented Jun 1, 2016

I did not perceive the on demand loading as a problem during development. However I don't think that folks currently would use on demand loading in production and instead have jspm generate a single or a view bundles.

This module should definitely not impose consumers to having to use jspm, webpack, browserify or anything else.
The intent is to use jspm as a dev tool only which should not concern module consumers. Ultimately connect-js would be published to npm as CommonJS ES5 module (ES5) and consumers would not use jspm (unless they wanted to). The angular example for example uses browserify.

In addition one could release a script bundle. This bundle can be created with npm run dist which uses jspm to create a bundle. See also commit 7d06648.

@dmitrizagidulin Does this lessen your concerns?

@dmitrizagidulin
Copy link
Member

@henrjk I would still prefer not to use jspm here, even as a dev dependency. :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants