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

Web Crypto APIs #737

Open
wants to merge 72 commits into
base: develop
from
Open

Web Crypto APIs #737

wants to merge 72 commits into from

Conversation

@zone117x
Copy link
Member

zone117x commented Oct 17, 2019

Description

Closes #691

The goal of this PR is to make the cryptographic operations performed by blockstack.js more efficient -- less CPU & memory intensive and smaller file size (especially for mobile).

This primarily improves the performance of storage and authentication functions.

This PR includes a refactor of all usages of applicable cryptographic operations to use the W3C native Web Crypto APIs.

This includes:

  • aes-256-cbc encrypt & decrypt
  • sha-256 and sha-512 hash digests
  • hmac-sha256 digest
  • pbkdf2-sha256 and pbkdf2-sha512 password derivation

The ripemd160 hash digest is not available in the Web Crypto API, however, it has been converted to use a minimal and relatively fast JS lib as opposed to the large node.js crypto module polyfill.

The triplesec lib is no longer a direct blockstack.js dependency. It must now be provided by the consumer application. It was responsible for ~250KB of the blockstack.js dist, but is used only for decrypting legacy mnemonics. AFAIK no regular apps built on Blockstack use this. Only a few consumer apps use this function (e.g. blockstack-cli, blockstack-browser, wallet[?]). It has been made trivial for consumer apps to import the triplesec lib and pass it to blockstack.js.

The cheerio html parsing lib has the same treatment as above. It is used for social proofs, which is not directly used by typical apps. Consumer apps (e.g. Gaia, blockstack-browser) must import this dependency themselves.

The dist bundle size is now down to 550KB.

The linter rules were tightened up, especially around Promise/async/await handling. This helped catch several instance of bugs like `some_string_${promise_variable}` -> `some_string_${await promise_variable}`. The stricter rules also ended up requiring minor changes to some code that is not necessarily related to cryptographic operations.

For updates on our effected consumer apps, see:

Type of Change

  • New feature
  • API reference/documentation update

Does this introduce a breaking change?

This is a breaking change and requires a major version update.

Note: the Web Crypto APIs only provide async functions, so the changes to use these propagated up to several other functions in the call graphs.

I kept a list of all functions that were previously synchronous which now return promises. Most of these are not public API functions. These have been listed in the CHANGELOG:

handleSignedEncryptedContents
makeAuthResponse
encryptECIES
decryptECIES
encryptPrivateKey
decryptPrivateKey
encryptContent
decryptContent
aes256CbcEncrypt
aes256CbcDecrypt
hmacSha256

Are documentation updates required?

Possibly -- the API reference docs will be auto updated, but any tutorials referencing the above functions may need updated.

Checklist

  • Code is commented where needed
  • Unit test coverage for new or modified code paths
  • npm run test passes
  • Changelog is updated
  • Tag 1 of @yknl or @zone117x for review

Extra TODO

  • Ensure working on all supported browsers: Chrome, Firefox, Safari, Edge
    • (?) Determine minimum supported versions for each browser.
zone117x added 26 commits Sep 5, 2019
…crypto.

* Remove dependency on ripemd160 package, replace with node.js crypto usage.
* Import only what is used from the `crypto` lib.
…out removing it from the lib.
…f `elliptic` only used for secp256k1 -- good target for replacement
… and reducing docgen command to output html and json in a single-pass.
* develop: (42 commits)
  Update changelog
  Remove unneeded line
  Oops, 4 instead of 3 passes required.
  Test `GaiaHubError` values
  Include correct error code
  Fix error message test
  Modify getFile test to expect throw on 404 instead of null
  Add `await` when getting blockstackErrorFromResponse
  Fix style issues
  Convert to async function + ts syntax
  Make functions `async` to account for async method for getting error from rrsponse
  getgaiaErrorResponse method (allow text or json body)
  Fix test + styling issues
  Get rid of getResponseDescription, integrate GaiaHubError
  Include response data in GaiaHubError types
  ErrorType ->  ErrorData
  Add GaiaHubError, make gaia errors extend that instead
  FileNotFound  -> DoesNotExistError
  centralized gaia error handling function
  support PayloadTooLargeError
  ...

# Conflicts:
#	.vscode/launch.json
#	mdincludes/script-dist-file.md
#	package-lock.json
#	package.json
#	src/encryption/ec.ts
…pping the node `crypto` module; WebCrypto implementation coming later)
Extract minimal 4KB implementation of ripemd160 from browserify-crypto polyfilly.
* Moved crypto lib loader into util module, made all crypto loading calls async.
* Add dev dependency for webcrypto lib (has compatbility wrappers for node.js) for unit testing
…endency
…king and module transforms
…ion must provide the lib
* develop:
  #710 Change dist file CDN sample generator to ignore prerelease versions
  Fix core node endpoint preference from the user not being used for name lookups, removed redundant UserSession AppConfig code

# Conflicts:
#	package-lock.json
#	package.json
…or web browser environments
@zone117x zone117x self-assigned this Oct 17, 2019
@zone117x zone117x added this to the DevX 2019 Q4 - Sprint 1 milestone Oct 17, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Oct 17, 2019

Codecov Report

Merging #737 into develop will increase coverage by 0.53%.
The diff coverage is 77.7%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #737      +/-   ##
===========================================
+ Coverage    68.62%   69.15%   +0.53%     
===========================================
  Files           57       65       +8     
  Lines         3429     3693     +264     
  Branches       623      658      +35     
===========================================
+ Hits          2353     2554     +201     
- Misses         778      821      +43     
- Partials       298      318      +20
Impacted Files Coverage Δ
src/auth/userSession.ts 49.43% <ø> (ø) ⬆️
src/fetchUtil.ts 100% <ø> (ø) ⬆️
src/profiles/services/index.ts 100% <ø> (ø) ⬆️
src/profiles/profileSchemas/organization.ts 35.71% <0%> (ø) ⬆️
src/auth/protocolEchoDetection.ts 15.62% <0%> (-4.38%) ⬇️
src/profiles/profileSchemas/person.ts 90% <0%> (ø) ⬆️
src/profiles/profileSchemas/creativework.ts 35.71% <0%> (ø) ⬆️
src/auth/authSession.ts 9.43% <0%> (-1.68%) ⬇️
src/profiles/services/twitter.ts 83.33% <100%> (+10.6%) ⬆️
src/encryption/cryptoRandom.ts 100% <100%> (ø)
... and 44 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2018ea7...d7924c2. Read the comment docs.

zone117x added 16 commits Nov 13, 2019
…rs with hmac support
…6/sha512
… crypto and WebCrypto
…lyfill
@zone117x zone117x changed the title [WIP] Web Crypto APIs Web Crypto APIs Nov 18, 2019
@zone117x zone117x marked this pull request as ready for review Nov 18, 2019
@yknl

This comment has been minimized.

Copy link
Collaborator

yknl commented Nov 19, 2019

Thanks @zone117x for all the work on this PR. It looks mostly good for me, but I think the latest changes broke some of the tests. I'm getting 12 failed tests right now, mostly on the RIPEMD160 and SHA2 hash tests.

zone117x added 2 commits Nov 19, 2019
…d `URLSearchParams` support -- notably NodeJS v8.x and older browsers
…ith improved error handling
@zone117x

This comment has been minimized.

Copy link
Member Author

zone117x commented Nov 20, 2019

@yknl If you are still running into test failures, can you ensure Node.js >=10 is being used, and try wiping your node_modules directory and re-running npm i.
I just tested with both Node v10 and v12, and all tests are passing. Also just pushed a fix for v8 which allows all tests to pass.

@yknl
yknl approved these changes Dec 2, 2019
Copy link
Collaborator

yknl left a comment

LGTM

@zone117x zone117x removed their assignment Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.