Skip to content
This repository has been archived by the owner on Jul 21, 2023. It is now read-only.

feat: bring your own web crypto #150

Closed
wants to merge 2 commits into from
Closed

Conversation

alanshaw
Copy link
Member

@alanshaw alanshaw commented Jul 9, 2019

Changes webcrypto.js to check for native web crypto availability and falls back to using window.__crypto if not available.

If the user wants to bring their own Web Crypto API compatible implementation then they simply need to assign it to window.__crypto before they start using IPFS/libp2p.

Checks are done in the functions that require web crypto to give the user the flexibility to assign to window.__crypto before OR after they import libp2p-crypto. It also means that users have the ability to use other exported functions that do not require web crypto without having to worry about sorting their own implementation.

We use window.__crypto because window.crypto is a readonly property in secure context and always readonly in workers.

If window.crypto and window.__cypto are unavailable then an appropriate error message is reported to the user with a ERR_MISSING_WEB_CRYPTO code.

I've also added documentation to the README.

This is a backwards compatible change.

closes #149
resolves #105
resolves ipfs/js-ipfs#2017
resolves ipfs/js-ipfs#2153

cc @lidel

Changes `webcrypto.js` to check for native web crypto availability and falls
back to using `window.__crypto` if not available.

If the user wants to bring their own Web Crypto API compatible implementation
then they simply need to assign it to `window.__crypto` before they start using
IPFS.

Checks are done in the functions that require web crypto to give the user the
flexibility to assign to `window.__crypto` before OR after they import
`libp2p-crypto`. It also means that users have the ability to use other exported
functions that do not require web crypto without having to worry about sorting
their own implementation.

We use `window.__crypto` because `window.crypto` is a readonly property in
secure context and always readonly in workers.

If `window.crypto` and `window.__cypto` are unavailable then an appropriate
error message is reported to the user with a `ERR_MISSING_WEB_CRYPTO` code.

I've also added documentation to the README.

This is a backwards compatible change.

closes #149
resolves #105
resolves ipfs/js-ipfs#2017
resolves ipfs/js-ipfs#2153

License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
License: MIT
Signed-off-by: Alan Shaw <alan.shaw@protocol.ai>
@alanshaw alanshaw requested a review from achingbrain July 9, 2019 13:30
@achingbrain
Copy link
Member

What's the thinking behind requiring the user to modify global state instead of passing a crypto implementation in as an option?

@daviddias
Copy link
Member

I share @achingbrain's question. This will turn into a "very obscure and hard to understand how it works" hack.

I suggest doing the long way, passing the crypto module as a libp2p constructor option.

@jacobheun
Copy link
Contributor

A lot of modules leverage libp2p-crypto directly instead of pulling from libp2p itself. In order to pass crypto to libp2p and have the modules leverage it, its usage will need to change everywhere. DI could make this much easier to resolve.

@alanshaw
Copy link
Member Author

alanshaw commented Jul 9, 2019

Thinking behind:

Not to break changes.

Keep scope low.

Fix it soon.

You’d have to pass the crypto impl to each function since this module isn’t a class you instantiate it’s a grab bag of crypto functions.

Or refactor the module internally to become a class and somehow thread that through lots of other libp2p modules with DI or pass the config around everywhere.

Perhaps we could have a static setCryptoImpl on the module...but if we ever have two different versions in a bundle it won’t be shared.

I’m open to suggestions, or what about merging this and fixing it properly in the future?

This is what PRs are for 😅

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

We use window.__crypto because window.crypto is a readonly property in secure context and always readonly in workers.

I am afraid there may be good reasons for window.crypto being immutable: if a page has JS loaded from third party services such as ad networks, those could inject a proxy that does MITM on all crypto functions.

I imagine replacing global state with dependency injection around libp2p-crypto would decrease the surface for this type of exploits.

merging this and fixing it properly in the future?

My worry is that if we ship this.. people will use it. And even when we do the proper DI in the future, we will have to support this hack forever (inviting security issues), or we will break websites that use it 😿

@alanshaw alanshaw closed this Jul 10, 2019
@alanshaw alanshaw deleted the feat/byo-webcrypto branch July 10, 2019 11:14
alanshaw added a commit that referenced this pull request Jul 17, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs #149
refs #150
refs #105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
jacobheun pushed a commit that referenced this pull request Jul 22, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs #149
refs #150
refs #105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
Mikerah pushed a commit to ChainSafe/js-libp2p-crypto that referenced this pull request Aug 29, 2019
This PR simply detects missing web crypto and throws an error with an appropriate message.

This is a stepping stone that will help users understand the problem until we have time to do a refactor of this module and of all the modules that use it to enable optionally passing your own crypto implementation.

refs libp2p#149
refs libp2p#150
refs libp2p#105
refs ipfs/js-ipfs#2153
refs ipfs/js-ipfs#2017

License: MIT
Signed-off-by: Alan Shaw <alan@tableflip.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants