search

element-hq / element-web

A glossy Matrix collaboration client for the web.
https://element.io
Apache License 2.0
10.82k stars 1.91k forks source link

Rip out legacy crypto code #26922

Open richvdh opened 5 months ago

richvdh commented 5 months ago

As a result of our work on Element R, lots of the js-sdk code is deprecated or unusable. Once Rust crypto becomes the default (related: https://github.com/element-hq/element-web/issues/26869), we should begin ripping out the legacy code.

Hopefully, as part of that, we can close https://github.com/element-hq/element-web/issues/17488.

richvdh commented 5 months ago

We'll need to make sure they are all deprecated first, of course: https://github.com/element-hq/element-web/issues/26919

richvdh commented 3 months ago

Blocked on https://github.com/element-hq/element-web/issues/27001

Johennes commented 2 months ago

The legacy crypto code treated libolm as an injectable dependency, loading it from the Olm global. This is crucial for environments that don't support WASM (such as React Native) and allows consumers of matrix-js-sdk to provide libolm based on their specific restrictions.

The Rust crypto code doesn't appear to have the same feature and relies on loading WASM. Removing the legacy crypto code would, therefore, effectively cut off the WASM-incapable environments – seemingly without a suitable alternative.

Additionally, for environments that can run WASM but don't require encryption, the bundle size is bloated significantly as per https://github.com/matrix-org/matrix-js-sdk/issues/4154.

Could we maybe provide the same global injection mechanism for the Rust crypto code?

richvdh commented 2 months ago

The Rust crypto code doesn't appear to have the same feature and relies on loading WASM. Removing the legacy crypto code would, therefore, effectively cut off the WASM-incapable environments – seemingly without a suitable alternative.

Yes, that is regrettable. Unfortunately we're not really in a position to maintain two crypto stacks, so I can't see that we can do much about it. The legacy crypto stack, apart from being a somewhat unmaintanable mess, contains known issues that we really don't have the time or resources to fix, and attempting to maintain it in parallel with the Rust stack would impede our ability to iterate the protocol to fix things in future.

I'd be interested to hear which applications require crypto but lack WASM support, but I think the die was cast at the point we decided to rewrite the existing stack in rust: matrix-js-sdk will not be supporting cryptography without WASM. Grand rewrite projects always come at a cost of someone's favourite feature, and in this case crypto-without-wasm is such a feature.

Additionally, for environments that can run WASM but don't require encryption, the bundle size is bloated significantly as per matrix-org/matrix-js-sdk#4154.

Could we maybe provide the same global injection mechanism for the Rust crypto code?

I don't entirely follow what's going on in that issue; certainly the intention is that, if you don't need crypto, you don't need the WASM artifact and you should configure your Webpack/Vite/whatever so that it isn't built in.

The global injection mechanism made crypto very hard to use as a developer. Having to inject things into global variables simply isn't a thing a modern Javascript developer expects to do. The only reason we had it was because of some US-export-regulation-hoop-jumping that stopped us shipping cryptography implementations from Github. It made testing hard and was generally a pain in the butt to maintain. I'm very happy to see the back of it :).

Johennes commented 2 months ago

Yes, that is regrettable. Unfortunately we're not really in a position to maintain two crypto stacks, so I can't see that we can do much about it. The legacy crypto stack, apart from being a somewhat unmaintanable mess, contains known issues that we really don't have the time or resources to fix, and attempting to maintain it in parallel with the Rust stack would impede our ability to iterate the protocol to fix things in future.

Yes, agreed and I'm not trying to make a case for maintaining both stacks. I'm merely advocating for having the same loading mechanism in the Rust stack that we currently have in the legacy stack.

I'd be interested to hear which applications require crypto but lack WASM support, but I think the die was cast at the point we decided to rewrite the existing stack in rust: matrix-js-sdk will not be supporting cryptography without WASM.

I don't know if there are other environments but, as I mentioned, React Native lacks WASM support. There is https://github.com/cawfree/react-native-webassembly but it hasn't seen any updates in a year and people seem to be struggling to make it work at all.

Grand rewrite projects always come at a cost of someone's favourite feature, and in this case crypto-without-wasm is such a feature.

The unfortunate thing is that this is not just an arbitrary tiny feature. You're shutting out an entire ecosystem and React Native isn't exactly an exotic technology. Using matrix-js-sdk in React Native has even been semi-officially supported through https://github.com/matrix-org/matrix-rn-sdk in the past.

I understand that the global object isn't exactly a great approach. I'm not really an expert on the technical details but would there be any other way in which we could allow consumers of the SDK to control the WASM loading?

richvdh commented 2 months ago

I understand that the global object isn't exactly a great approach. I'm not really an expert on the technical details but would there be any other way in which we could allow consumers of the SDK to control the WASM loading?

In theory, if you don't enable crypto in your client, you won't load the WASM artifact. That's why it's dynamically loaded. It should be a fairly simple matter of configuration in your bundler of choice.

Johennes commented 2 months ago

My use case is matrix-js-sdk in a React Native app with encryption. I'm currently using the official ASM build of libolm which, so far, works great with the crypto store from https://github.com/matrix-org/matrix-rn-sdk/pull/2.

richvdh commented 2 months ago

@Johennes perhaps I'm not understanding what you're asking for, then.

It seemed you had two points:

Johennes commented 2 months ago

Sorry, the first point is my actual problem. I clumsily tried to use the second point to underpin it, arguing that having the import injectable would have other benefits. But I think you're right that this particular case is probably solvable via bundler configuration.

I'm finding this pretty unfortunate overall, not least because matrix-js-sdk is a matrix-org project. Any chance you'd be open to an external contribution that makes matrix-sdk-crypto-wasm injectable?

richvdh commented 2 months ago

Any chance you'd be open to an external contribution that makes matrix-sdk-crypto-wasm injectable?

Possibly, but how does that help your situation?

Johennes commented 2 months ago

So basically with libolm I'm loading the legacy ASM build into the Olm global with

global.Olm = require('@matrix-org/olm/olm_legacy');

If the matrix-sdk-crypto-wasm import could be injected, I could maybe do something similar by translating the Rust Crypto WASM to ASM (e.g. via https://github.com/WebAssembly/binaryen#wasm2js) and providing that instead.

richvdh commented 2 months ago

I see. Interesting. We're somewhat off-topic here: could you open a separate issue and we can discuss further?

Johennes commented 2 months ago

Hm, looks like even with optimization flags, wasm2js produces a horrendous 40MB abomination out of matrix-sdk-crypto-wasm's WASM. And unsurprisingly that's bursting all memory limits when trying to bundle it on my laptop. So I suppose the path I'm trying to pursue here is doomed. 😞

I suppose I could look into providing it via a React Native "native module" but at that point I could probably just consume the entirety of matrix-rust-sdk as a native module.

So I guess I'll retreat Sorry for making a big fuzz about it. I hadn't anticipated the artifact to be this big.

It would be nice if we could document that WASM will be mandatory for encryption in the near future to prevent others from sinking their time into trying to use matrix-js-sdk with React Native.

richvdh commented 2 months ago

Yeah, I'm not surprised that transpiling the wasm-transpiled Rust into asm.js gives you an abomination :(.

Thinking further about this:

If anybody wanted to contribute and maintain alternative implementations of CryptoApi, they would be welcome (and maybe the existing libolm wrappers wouldn't be a terrible place to start). The problem is really that the core maintainers of matrix-js-sdk don't have time to maintain a second implementation, and I think it would need to live out-of-tree.

To that end: maybe the way to go here is to make the crypto implementation pluggable somehow. I haven't done much thinking about what that might look like in practice (it might be as simple as exposing a MatrixClient.setCryptoBackend method), but at least we now have CryptoApi, which gives a defined interface that such stacks have to implement.

But again, work to make it pluggable isn't something Element-the-company is likely to find time for in the near term, so such work would need to be contributed. I'd be happy to review PRs to that end though.

@Johennes interested in your thoughts here.

Johennes commented 2 months ago

Thanks for getting back on this, Rich.

Yes, fully acknowledged that maintaining two crypto stacks in the SDK isn't feasible. Making CryptoApi pluggable doesn't sound overly daunting for a contributor. Maintaining an implementation of it slightly more so. 🙈

What I'll need to figure out is how this would compare effort-wise to plugging the Rust SDK as a native module in React Native. I'll have to carve out some time to do this.

Generally, if you are open to having CryptoApi be pluggable, that's great and, I think, all I need to know for now. I can then come back and try to make that contribution once this becomes a problem in the project I'm working on and you're not blocked on ripping out the legacy crypto code.

Johennes commented 1 month ago

[...] Making CryptoApi pluggable doesn't sound overly daunting for a contributor. Maintaining an implementation of it slightly more so. 🙈

What I'll need to figure out is how this would compare effort-wise to plugging the Rust SDK as a native module in React Native. I'll have to carve out some time to do this.

I did some tests and it turns out using matrix-rust-sdk as a native module in React Native is more or less straightforward because we can reuse the bindings that were written for Element X. So while doing that still requires some efforts, it's probably better longterm than adopting maintenance of the libolm wrappers. That'll also spare us from having to shim Web Crypto and IndexedDb and all the other annoying hacks needed to integrate a node / browser library into React Native's weird environment.

Johennes commented 3 weeks ago

Since I've just discussed this question elsewhere and for the avoidance of doubt: I'm working on creating React Native bindings for the entire Rust SDK, not only for the crypto crate. So I'm not currently planning on contributing a change to make CryptoApi pluggable since, assuming what I'm doing works out, I wouldn't be using matrix-js-sdk at all anymore.

I think it should be possible to use the same approach to write a pluggable replacement for https://github.com/matrix-org/matrix-rust-sdk-crypto-wasm but I've hit other issues with using matrix-js-sdk in React Native (e.g. no crypto.subtle) that would still be a blocker.