Remove Crypto.get() singleton #2146
Conversation
There was a problem hiding this comment.
Code Review
This pull request effectively removes the Crypto.get() singleton, replacing it with dependency injection via Environment or direct constructor/method arguments. This is a significant improvement for testability and fine-grained control over cryptographic implementations.
Key changes include:
Cryptois now anabstract class, and methods likeencrypt,decrypt, andrandomBytes(renamed fromgetRandomData) are abstract, requiring concrete implementations.CertificateManagerhas been converted from a namespace to a class, now taking aCryptoinstance, which is a good structural improvement.- A new consolidated
MockCryptohas been introduced in@matter/general, replacing previous mock implementations. This simplifies testing setups. - Random number generation utilities have been modernized, for example, using
DataViewand a newBytes.asBigInt. - Crypto instances are now passed down through the component hierarchy, ensuring that dependencies are explicit.
- Platform-specific crypto initializations (Node.js, React Native) have been updated to register with the
Environmentor fall back to default implementations if their specific versions are disabled or problematic (e.g., Bun for Node.js crypto, AES-CCM for React Native'squick-crypto).
The changes are extensive but systematically applied across the codebase. The tests have been updated to reflect the new DI patterns, primarily using MockCrypto.
Overall, this is a solid refactoring that enhances the modularity and testability of the cryptography components.
49f12ef to
cacf4a9
Compare
I agree, am not dead set on approach and I expect will continue to morph. Here are some other considerations we may not have discussed yet:
|
This allows for more fine-grain control of entropy in tests. It also aligns dependency injection of crypto with storage, networking and higher-level components. And getting rid of singletons is good on its own I think. Manually passing crypto around is a little annoying but it's encapsulated pretty well in low-level packages. @matter/protocol is most affected, and users shouldn't notice at all. I exposed a "crypto" property on some of the more common contextual components such as Fabric so it doesn't need to be passed separately in all cases. Details: * Remove Crypto.get() and the static methods that mapped to Crypto.get() * Components now take crypto implementation as an input, either directly or via Environment * StandardCrypto registers in Environment.default; Node.js and react native implementations likewise install into correct environment * Made NodeJsEnvironment attempt to retrieve Crypto and/or Network from the initial default environment if the Node.js version is disabled * Converted CertificateManager to a class. It can be created and thrown away; this just allows us to invoke each of the 13 (formerly static) methods without passing in crypto as an argument every time * Converted Crypto back to an abstract class and moved utility functions such as randomUint8 to the instance so they are more accessible without passing in crypto implementation * Converted random number generation to native DataView, replaced getRandomUint*() with randomUint* getters, and modernized other methods * Replaced Uint8Array -> bigint conversion with a version that uses DataView rather than round tripping through strings * Removed MockCrypto from @matter/testing and nonentropic from @matter/general; consolidated to "MockCrypto" exported from @matter/general * There's not a clear point where crypto is "used" now, so I added a method to perform logging of active implementation. ServerNode and CommissioningController both invoke on startup * We now disable node.js crypto for Bun where we know it doesn't work but that StandardCrypto does * ReactNativeCrypto installs by default again, but uses the StandardCrypto version of encrypt/decrypt since quick crypto doesn't support AES-CCM * Those last two we will hopefully eventually replace with feature detection so we always install the highest-quality implementation of each method (that works)
d6345f6 to
2f73e99
Compare
This allows for more fine-grain control of entropy in tests. It also aligns dependency injection of crypto with storage, networking and higher-level components. And getting rid of singletons is good on its own I think.
Manually passing crypto around is a little annoying but it's encapsulated pretty well in low-level packages. @matter/protocol is most affected, and users shouldn't notice at all. I exposed a "crypto" property on some of the more common contextual components such as Fabric so it doesn't need to be passed separately in all cases.
Details:
Remove Crypto.get() and the static methods that mapped to Crypto.get()
Components now take crypto implementation as an input, either directly or via Environment
StandardCrypto registers in Environment.default; Node.js and react native implementations likewise install into correct environment
Made NodeJsEnvironment attempt to retrieve Crypto and/or Network from the initial default environment if the Node.js version is disabled
Converted CertificateManager to a class. It can be created and thrown away; this just allows us to invoke each of the 13 (formerly static) methods without passing in crypto as an argument every time
Converted Crypto back to an abstract class and moved utility functions such as randomUint8 to the instance so they are more accessible without passing in crypto implementation
Converted random number generation to native DataView, replaced getRandomUint*() with randomUint* getters, and modernized other methods
Replaced Uint8Array -> bigint conversion with a version that uses DataView rather than round tripping through strings
Removed MockCrypto from @matter/testing and nonentropic from @matter/general; consolidated to "MockCrypto" exported from @matter/general
There's not a clear point where crypto is "used" now, so I added a method to perform logging of active implementation. ServerNode and CommissioningController both invoke on startup
We now disable node.js crypto for Bun where we know it doesn't work but that StandardCrypto does
ReactNativeCrypto installs by default again, but uses the StandardCrypto version of encrypt/decrypt since quick crypto doesn't support AES-CCM
Those last two we will hopefully eventually replace with feature detection so we always install the highest-quality implementation of each method (that works)