Detect WebCrypto API in web workers via `self` #586

leoselig · 2018-06-01T08:37:40Z

Problem

The following code has been used to detect the Web Crypto API:

function _detectSubtleCrypto(fn) {
  return (typeof window !== 'undefined' &&
    typeof window.crypto === 'object' &&
    typeof window.crypto.subtle === 'object' &&
    typeof window.crypto.subtle[fn] === 'function');
}

While this works fine in the browser main thread, it fails to detect the crypto API in Web Workers.

Solution

A typical solution to write code that works in Web Workers as well as the main thread is to use the self keyword that resolves to window in the main thread and the instance of WorkerGlobalScope in the web worker.

Since this is not supported yet in < IE 11, I'm introducing a getGlobalScope()-helper in the util-namespace.

As we are using node-forge in a web worker to avoid blocking the main thread for other operations, this sped up RSA key pair generation from ~1min in average to less than 3 seconds.

Feel free to tell me what should change to make this happen. :)

davidlehn · 2018-06-01T22:52:18Z

Is there an easy way to test this? Maybe some sort of modification to the legacy keygen interactive test?

This will allow for separate, karma-only test suites in the future.

leoselig · 2018-06-04T09:54:25Z

@davidlehn I wasn't very comfortable adding something to a "legacy" setup so I took the liberty of tweaking karma setup a bit. It now allows karma-only tests. This allowed me to setup a web worker without compromising the mocha tests (that only run in Node.js).

This does not reliably test the performance improvement but hopefully serves as a starting point to ensure, i.e., compatibility with web workers (or even other browser-only APIs).

I'm aware that this might have been too big of a leap forward, so be sure that I'm appreciating whatever feedback is incoming . :)

leoselig · 2018-06-04T09:55:31Z

I just realized, browserify is available as a bundler option.
Any particular reason to support both webpack and browserify?
As you can see by the use of the worker-loader, only supporting one or the other would be quite simpler.

davidlehn · 2018-06-04T19:47:13Z

I like that approach of having karma specific tests. I'll take a look at that test when I get some time.

I'm not sure how many people use browserify. It wasn't much effort to keep it working so we just kept it in the test setup. I imagine a future forge is going to go all-in on ES2017+ which is when I figured all the babel transpiling and polyfills would make browserify path more difficult. If it's possible to keep the tests working now, that would be nice. Not quite sure what that involves here. Maybe easier to just disable difficult things in browserify.

leoselig · 2018-06-27T08:15:46Z

Hey @davidlehn
Did you manage to take a closer look already? Any way I can help to finish this?

My proposal would be to drop support for running tests based on browserify. As far as I can tell there is no immediate benefit of running tests with both bundlers and webpack seems to have taken the lead in the community. If you agree, I'd be up to adapt this PR.

yieme · 2018-07-25T19:36:36Z

Love the idea of supporting service workers.

For what it's worth, I suspect the following may be cleaner and reduce the overhead of a function call:

util.getGlobalScope = this.global // node 
   || this.window // browser 
   || this.self // service worker

davidlehn · 2018-08-14T20:19:07Z

lib/rsa.js

-    typeof window.crypto === 'object' &&
-    typeof window.crypto.subtle === 'object' &&
-    typeof window.crypto.subtle[fn] === 'function');
+  return (typeof util.getGlobalScope() !== 'undefined' &&


Micro optimization would be to do lift out var globalScope = util.getGlobalScope() above to avoid repeated calls.

davidlehn · 2018-08-14T20:19:23Z

lib/rsa.js

-    typeof window.msCrypto === 'object' &&
-    typeof window.msCrypto.subtle === 'object' &&
-    typeof window.msCrypto.subtle[fn] === 'function');
+  return (typeof util.getGlobalScope() !== 'undefined' &&


Same as above, could use a globalScope var.

davidlehn · 2018-08-14T20:20:54Z

lib/util.js

+    return global;
+  }
+
+  return typeof self === "undefined" ? window : self;


style: use single quotes.

davidlehn · 2018-08-14T20:21:34Z

tests/karma/index.js

+require('../unit');
+
+// ...plus some tests that can only run in the browser (e.g. web worker compatibility)
+require('./rsa');


style: add newline

davidlehn · 2018-08-14T20:22:03Z

tests/karma/rsa.js

+describe('rsa', function() {
+  it('should generate key pairs when running forge in a web worker', function(done) {
+    // Make test worker call rsa.generateKeyPair() on its own side 
+    testWorker.postMessage({method:'rsa.generateKeyPair'});


style: space after the colon.

davidlehn · 2018-08-14T20:23:03Z

tests/karma/testWorker.js

+// We  define a light-weight protocol to simplify testing 
+self.addEventListener('message', function (event) {
+
+      // Test scripts call worker.postMessage(data)` – we can access the payload via event.data


style: 2 space indents and add end of file newline.

davidlehn · 2018-08-14T20:24:15Z

@leoselig sorry this sat for so long. added some code comments.

davidlehn · 2018-08-14T20:28:02Z

@leoselig Also probably should consider changing that util.getGlobalScope() function to a util.globalScope var as @yieme suggested to avoid unneeded recomputation.

These were mentioned in the PR review here: digitalbazaar#586 (comment)

This motivated by the discussion here: digitalbazaar#586 (comment) Essentially, using the `worker-loader` for webpack for the first time disclosed the challenges of supporting two bundlers at the same time. Since there is no dominant development use case for having two bundlers, removing browserify should reduce the maintenance overhead.

The evaluation function stays the same but is wrapped as an IEFE and executed once on module initialization. This should avoid unnecessary recomputations in the relying code.

These were mentioned in the PR review here: digitalbazaar#586 (comment)

This motivated by the discussion here: digitalbazaar#586 (comment) Essentially, using the `worker-loader` for webpack for the first time disclosed the challenges of supporting two bundlers at the same time. Since there is no dominant development use case for having two bundlers, removing browserify should reduce the maintenance overhead.

leoselig · 2018-08-24T09:02:54Z

@davidlehn No worries, it's OSS – I won't expect anyone to be ultra responsive :D

I changed the code as you asked for. I also, as I suggested, removed the browserify bundler option entirely. I believe this to be a good idea to reduce the overhead of maintaining both and didn't see any use case for it. (It also has the pleasant side effect of making my build green)

…r-generation-in-web-workers

leoselig · 2018-11-07T09:34:38Z

@davidlehn Sorry to ping you once again. We just merged the upstream back into my fork. Tests are still passing, apps work as expected.
This would be a beautiful time for another merge attempt 🙏

These were mentioned in the PR review here: #586 (comment)

This motivated by the discussion here: #586 (comment) Essentially, using the `worker-loader` for webpack for the first time disclosed the challenges of supporting two bundlers at the same time. Since there is no dominant development use case for having two bundlers, removing browserify should reduce the maintenance overhead.

davidlehn · 2019-01-31T07:20:57Z

@leoselig Made some modifications here: #645.
Let me know what you think.

davidlehn · 2019-01-31T18:45:24Z

@leoselig I updated #645 to keep browserify support. I dropped your commit and my revert commits of it. The reason to keep support is that people may be using browserify with forge. Hard to know. It still works so until we decide to specifically stop supporting browserify, we should keep testing it. The web worker test is trickier to deal with so I just omitted it while doing browserify tests. Can call that good enough for now.

These were mentioned in the PR review here: #586 (comment)

davidlehn · 2019-01-31T19:56:44Z

Merged via #645. Thanks!

leoselig · 2019-02-07T10:06:02Z

@davidlehn Fair enough. Just to clarify though, I was not suggesting to break node-forge for browserify users. I only wanted to remove it from the test execution since it seemed effectively irrelevant what bundler is used for the tests.

Thanks for merging this and for all the work on this package! :)

Detect WebCrypto API in web workers via self

8380e1e

leoselig added 2 commits June 4, 2018 11:46

Introduce different bunlde entry point for karma tests

14dda6b

This will allow for separate, karma-only test suites in the future.

Add karma test for web worker compatibility of key pair generation

6a70a74

davidlehn reviewed Aug 14, 2018

View reviewed changes

lib/util.js Outdated

return global;

}

return typeof self === "undefined" ? window : self;

Copy link

Member

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

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

style: use single quotes.

davidlehn reviewed Aug 14, 2018

View reviewed changes

leoselig added a commit to leoselig/forge that referenced this pull request Aug 24, 2018

Fix code formatting issues

a7fb8a8

These were mentioned in the PR review here: digitalbazaar#586 (comment)

leoselig added 3 commits August 24, 2018 10:48

Optimize getGlobalScope()-function to globalScope-constant

8df18fb

The evaluation function stays the same but is wrapped as an IEFE and executed once on module initialization. This should avoid unnecessary recomputations in the relying code.

Fix code formatting issues

9378d91

These were mentioned in the PR review here: digitalbazaar#586 (comment)

leoselig force-pushed the support-rsa-key-pair-generation-in-web-workers branch from 1803b04 to f470145 Compare August 24, 2018 08:48

mattnotmitt mentioned this pull request Aug 31, 2018

Operation request: RSA operations gchq/CyberChef#272

Closed

Merge remote-tracking branch 'origin/master' into support-rsa-key-pai…

3fd7cff

…r-generation-in-web-workers

davidlehn pushed a commit that referenced this pull request Jan 31, 2019

Fix code formatting issues

47ec5f7

These were mentioned in the PR review here: #586 (comment)

davidlehn mentioned this pull request Jan 31, 2019

Support rsa key pair generation in web workers #645

Merged

davidlehn pushed a commit that referenced this pull request Jan 31, 2019

Fix code formatting issues

54be0eb

These were mentioned in the PR review here: #586 (comment)

davidlehn closed this Jan 31, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect WebCrypto API in web workers via `self` #586

Detect WebCrypto API in web workers via `self` #586

leoselig commented Jun 1, 2018 •

edited

Loading

davidlehn commented Jun 1, 2018

leoselig commented Jun 4, 2018

leoselig commented Jun 4, 2018

davidlehn commented Jun 4, 2018

leoselig commented Jun 27, 2018

yieme commented Jul 25, 2018

davidlehn Aug 14, 2018

davidlehn Aug 14, 2018

davidlehn Aug 14, 2018

davidlehn Aug 14, 2018

davidlehn Aug 14, 2018

davidlehn Aug 14, 2018

davidlehn commented Aug 14, 2018

davidlehn commented Aug 14, 2018

leoselig commented Aug 24, 2018

leoselig commented Nov 7, 2018

davidlehn commented Jan 31, 2019

davidlehn commented Jan 31, 2019

davidlehn commented Jan 31, 2019

leoselig commented Feb 7, 2019

Detect WebCrypto API in web workers via self #586

Detect WebCrypto API in web workers via self #586

Conversation

leoselig commented Jun 1, 2018 • edited Loading

Problem

Solution

davidlehn commented Jun 1, 2018

leoselig commented Jun 4, 2018

leoselig commented Jun 4, 2018

davidlehn commented Jun 4, 2018

leoselig commented Jun 27, 2018

yieme commented Jul 25, 2018

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn Aug 14, 2018

Choose a reason for hiding this comment

davidlehn commented Aug 14, 2018

davidlehn commented Aug 14, 2018

leoselig commented Aug 24, 2018

leoselig commented Nov 7, 2018

davidlehn commented Jan 31, 2019

davidlehn commented Jan 31, 2019

davidlehn commented Jan 31, 2019

leoselig commented Feb 7, 2019

Detect WebCrypto API in web workers via `self` #586

Detect WebCrypto API in web workers via `self` #586

leoselig commented Jun 1, 2018 •

edited

Loading