loading indutny/elliptic library to browser on demand #956

chesnokovilya · 2019-08-30T20:55:10Z

No description provided.

src/build.env.js

src/openpgp.js

src/util.js

test/crypto/elliptic.js

src/openpgp.js

src/util.js

twiss · 2019-09-09T15:01:48Z

src/build.env.js

+// Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301  USA
+
+export default {
+  use_indutny_elliptic: true,


Similarly, this should be set at runtime instead of compile time, but maybe you don't even need a config option for this. People could just do something like

openpgp.loadElliptic = () => { throw new Error('Elliptic is not included'); };

Then, you can also remove all the getUseElliptic checks, as it will automatically throw a useful error when it's trying to load elliptic.

This option prevents openpgp from tryng to load elliptic. I think this is useful. For example browserify has exclude and external options. At runtime we could check if elliptic should be loaded but not loaded.

The example code I posted would also prevent OpenPGP.js from trying to load elliptic, right? It is a bit more work than passing a flag, but the advantage is that you don't have to create a special build of OpenPGP.js, and that it simplifies OpenPGP.js because we can always just call loadElliptic before checking first.

Do you want to substitute function content using grunt replace command? It is not clear for me =).

src/openpgp.js

twiss · 2019-09-09T17:36:25Z

src/crypto/public_key/elliptic/curves.js

+  this.getIndutnyCurve = util.getUseElliptic() ? async name => {
+    const elliptic = getElliptic() || await loadElliptic(build.external_indutny_elliptic_path);
+    return new elliptic.ec(name);
+  } : undefined;


You could also put this function in indutnyKey.js - it doesn't use this, so it doesn't have to be a method.

IndutnyKey.js is not public now, I could make it public and move this function here.

src/crypto/public_key/elliptic/indutnyKey.js

twiss · 2019-10-10T13:01:23Z

src/config/config.js

+  indutny_elliptic_fetch_options: {
+    credentials: 'include'
+  }


This default credentials: 'include' is not necessary, it can be set by the application if needed. You can just default to {} for the options, I think.

twiss · 2019-10-10T13:03:58Z

package.json

    "tweetnacl": "github:openpgpjs/tweetnacl-js#1ef755f2b252a3e328ac739848d00e0dad76be2d",
    "web-stream-tools": "github:openpgpjs/web-stream-tools#dc4b05e8a272b45819233f3df735423432beacfc",
-    "email-addresses": "github:openpgpjs/email-addresses#686743c6452b44bafcd06d47db7f36ddf3f3f118"
+    "whatwg-fetch": "^2.0.3"


This is pretty nitpicky, but could you keep these packages in the order they were in? I know they're usually in alphabetic order, but the order they were in was to roughly separate what was needed only for development (everything up to sinon) and what packages end up in a build (everything starting from text-encoding-utf-8).
Also, even if you disagree with that, keeping them in the same order in this PR would make it easier to see what changed here.

Npm kinda do it by itself, once I run npm install elliptic. May be we could switch to the new ordering convention? It is bothersome to rearrange lines manually after each change, or switching to another git branch.

May be we could switch to the new ordering convention?

Yeah, we could do that, but preferably not in this PR, since it makes it harder to see what changed.
Once we switch to ES6 modules, we'll move the devDependencies starting from text-encoding-utf-8 to dependencies, so then is probably a good time to fix the ordering.

src/lightweight_helper.js

twiss · 2019-10-10T13:13:05Z

src/lightweight_helper.js

+      script.integrity = integrity;
+    }
+    script.onload = e => resolve(e);
+    script.onerror = e => reject(e);


Suggested change

script.onerror = e => reject(e);

script.onerror = () => reject(new Error('Failed to load ' + path));

I have researched a bit errors that are thrown here, they are network related. This promise would be resolved even if path is wrong, and actually elliptic has failed to load.

Example here https://codesandbox.io/s/intelligent-darwin-w9cxx

Ok. But e is not an error object, it is an event object. You should throw an error instead of an event. Even for network errors, "failed to load" is pretty descriptive.

It is actually error event is passed here, I would better catch it in loadEllipticPromise.

It is an error event, but not an error object. You could do

script.onerror = e => reject(new Error(e.message));

if you prefer, but you should always throw an error object.

twiss · 2019-10-10T13:34:40Z

src/util.js

+  getUseElliptic: function() {
+    return config.use_indutny_elliptic;
+  },
+


I'm not sure this function is really necessary - especially once you're only checking it in one place, you can just check

if (!config.use_indutny_elliptic) {

directly.

Then, in the test suite where you're checking this in a bunch of places, I'm not sure it's really necessary? We're never calling the test suite with config.use_indutny_elliptic = false, so it should always be available in the test suite.

I have this function, so I could avoid importing config in bunch of places. I think it is valid use-case.
Regarding tests, since we have option to disable elliptic, let's have test suite which would work with this option.

I have this function, so I could avoid importing config in bunch of places.

That's why I mentioned "especially once you're only checking it in one place", because then you only have to import config in one place. I don't think that importing util instead of importing config is better, when it is config that you really need to check.

The test suite already uses config in lots of places, you could search for config in the test directory to check.

src/config/config.js

src/lightweight_helper.js

src/crypto/public_key/elliptic/indutnyKey.js

combined commits: initial working nodeCrypto ecdh clean up Curve25519 key generation using tweetnacl.js Move KeyPair.sign/verify to ecdsa.js Move KeyPair.derive to ecdh.js fix test, remove deprecated Buffer() add only_constant_time_curves flag to config ecdh is working after refactor ecdsa webVerify is working node signature verify is working without elliptic. sign and verify are working for nodeCrypto use build.config.js renaming to build.env.js fixing KeyPair constructor wip: use grunt for lightweight build add full-build to gruntfile to change from back from lightweight option wip: grunt build --lightweigth is working now refactored use of build.env, fixes ecdsa.sign function WIP: fixed elliptic tests + add throw errors + use switch solve problem with hash streaming using a hack move toHash out of if/else clean up in config and tests for full build chnage travis unit test script clean up reverting changes to worker.min.js moved throw on lightbuild to if move to Array.From in ecdsa add polyfill for Array.from move toHash back to if/else - this would make lightbuild non-functional for streaming remove polyfill array.from, and lightbuild to the gruntfile remove double tests in travis.sh init undutnyKeyPair only on demand. Change KeyPair format. Style fix fixed tests build correctly exclude elliptic in lightweight mode add skiping non-working tests fix gruntfile.js add lightweight tag to tests clean up disabled some tests for lightbuild-webcrypto

We already have util.getFullBuild()

fix jwk key length error load indutny/elliptic on demand in browser context loading elliptic in browser functional WIP new build file, use elliptic lib with prepare script WIP: fix exclude elliptic build WIP: lightweight build with node use correct path WIP copy elliptic.min.js by grunt + almost working static elliptic download WIP eslint fixes add fix option to gruntfile eslint load elliptic to window.openpgp.elliptic add loadElliptic call to tests fix brainpool test

Just to be sure

This was broken in #922 (merged as part of #956). This would cause GPG to be unable to parse unencrypted secret keys, thinking they were encrypted. rfc4880bis-08 hints at this requirement, saying: o MPI of an integer representing the secret key, which is a scalar of the public EC point. Since scalar multiplication happens after masking the private key, this implies that we should serialize the private key after masking, as well.

chesnokovilya changed the title ~~WIP: loading indutny/elliptic library to the browser on demand~~ WIP: loading indutny/elliptic library to browser on demand Aug 30, 2019

chesnokovilya force-pushed the web-loading-testing branch 5 times, most recently from 987ef8e to e2ddb07 Compare September 6, 2019 07:25

chesnokovilya changed the title ~~WIP: loading indutny/elliptic library to browser on demand~~ loading indutny/elliptic library to browser on demand Sep 6, 2019

twiss reviewed Sep 9, 2019

View reviewed changes

This was referenced Sep 10, 2019

WIP: Curve25519 key generation using tweetnacl; refactor KeyPair #922

Closed

Creating light openpg version without indutny/elliptic #931

Closed

chesnokovilya force-pushed the web-loading-testing branch from e2ddb07 to 0c47a09 Compare September 13, 2019 08:37

chesnokovilya force-pushed the web-loading-testing branch 4 times, most recently from 2d94a8c to 025ecde Compare October 3, 2019 12:41

twiss reviewed Oct 10, 2019

View reviewed changes

chesnokovilya force-pushed the web-loading-testing branch 2 times, most recently from dfdb359 to f349e96 Compare October 15, 2019 13:26

chesnokovilya and others added 12 commits October 16, 2019 12:39

add lightweight build test to travis testing process

6cb4196

Don't export openpgp.build

e32994a

We already have util.getFullBuild()

Style

c399cc8

Fix verifying NIST signatures using Web Crypto when not streaming

efc939c

enable brainpool and secp256k1 tests in lightbuild

5d464b4

Fix test case with missing signature packet

01324f4

style changes

f8ab2d6

update npm build script to delete elliptic.min.js from dist

3b1a8ea

WIP: refactor loadScript to lightweight.js

c2900a6

fix brainpool test

d326311

chesnokovilya and others added 19 commits October 16, 2019 12:39

fix/remove build import

0a1af4d

remove build.env, now options are passed in config

da8331a

moved getIndutnyCurve to indutnyKey file

447d08f

add --lightweight browser test to travis

65dc913

remove loadScriptHelper

b687a7a

remove Indutny KeyPair object

0146561

add Gruntfile.js to eslint check

540aa48

add eol-last rule

193a1e6

add missing test file

d953aca

return and store promise from loading Elliptic in web browser

04fe6a8

style changes

8fccb24

mve ellipticPromise to loadElliptic

7007b10

clean up, modify eslint rule to error on empty lines

ebd2293

remove getUseElliptic function

1a3ce5a

remove exclude_elliptic_build in gruntfile

abd79bc

throw error in loadScript

8f0957c

restore package.json order

aecac10

Style and comment fixes

f792f74

rename ellipticPromise on file download to avoid confusion

baa68d3

chesnokovilya force-pushed the web-loading-testing branch from 6976196 to baa68d3 Compare October 16, 2019 05:40

twiss added 3 commits October 25, 2019 14:18

Fix loading indutny/elliptic when running Brainpool tests individually

77365a6

Switch back to openpgpjs/elliptic

2a893ed

Separate cacheFile for lightweight build

f138039

Just to be sure

twiss merged commit 08b7725 into openpgpjs:master Oct 25, 2019

snyk-bot mentioned this pull request Feb 23, 2020

[Snyk] Upgrade openpgp from 4.6.0 to 4.8.1 raindigi/wildduck#3

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loading indutny/elliptic library to browser on demand #956

loading indutny/elliptic library to browser on demand #956

chesnokovilya commented Aug 30, 2019

twiss Sep 9, 2019

chesnokovilya Sep 10, 2019 •

edited

twiss Sep 10, 2019

chesnokovilya Oct 1, 2019 •

edited

twiss Sep 9, 2019

chesnokovilya Sep 11, 2019

twiss Oct 10, 2019

chesnokovilya Oct 14, 2019

twiss Oct 10, 2019

chesnokovilya Oct 11, 2019

twiss Oct 15, 2019

twiss Oct 10, 2019

chesnokovilya Oct 11, 2019 •

edited

chesnokovilya Oct 11, 2019

twiss Oct 11, 2019

chesnokovilya Oct 14, 2019

twiss Oct 14, 2019

twiss Oct 10, 2019

chesnokovilya Oct 14, 2019

twiss Oct 14, 2019

chesnokovilya Oct 15, 2019

	script.onerror = e => reject(e);
	script.onerror = () => reject(new Error('Failed to load ' + path));

loading indutny/elliptic library to browser on demand #956

loading indutny/elliptic library to browser on demand #956

Conversation

chesnokovilya commented Aug 30, 2019

Choose a reason for hiding this comment

chesnokovilya Sep 10, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesnokovilya Oct 1, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesnokovilya Oct 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chesnokovilya Sep 10, 2019 •

edited

chesnokovilya Oct 1, 2019 •

edited

chesnokovilya Oct 11, 2019 •

edited