Repository navigation
Conversation
|
tests incoming, but this PR should be otherwise reviewable |
Co-authored-by: Andrew Ferrazzutti <af_0_af@hotmail.com>
| LogService.debug("CryptoClient", "Running with device Ed25519 identity:", this.deviceEd25519); // info so all bots know for debugging | ||
|
|
||
| this.ready = true; | ||
| this.client.emit("crypto.ready"); |
There was a problem hiding this comment.
This looks like there are currently no listeners for this. Are there plans for there to be? IMO it's not all that useful to emit here, since the preparation of a MatrixClient / Intent already includes an await on crypto to be ready, i.e. there are already checks in place to make sure that crypto is prepared before it gets used.
There was a problem hiding this comment.
This is so that the bot can verify itself once the crypto is set up, i.e.
client.on("crypto.ready", async () => {
await client.crypto.confirmIdentityWithRecoveryKey(config.recoveryKey);
});
Co-authored-by: Andrew Ferrazzutti <af_0_af@hotmail.com>
|
To compliment this PR, I'm preparing a separate PR that adds a helper class for using the Secret Storage API, which will resemble the JS-SDK's |
I've added a test. Though CI of course will fail until we do a release of crypto-nodejs. @AndrewFerr do you have any more changes planned for |
No, I'm done. Thanks for asking! Go ahead with that release. |
| const ownDevice = await machine.getDevice(machine.userId, machine.deviceId); | ||
| if (ownDevice.isCrossSigningTrusted()) { |
There was a problem hiding this comment.
machine.getDevice can apparently return null, though I'd expect that to not happen after crypto is ready. Nevertheless, consider guarding against it here.
ownDevice is also never used elsewhere, so consider inlining it into the if condition.
| const ownDevice = await machine.getDevice(machine.userId, machine.deviceId); | |
| if (ownDevice.isCrossSigningTrusted()) { | |
| if ((await machine.getDevice(machine.userId, machine.deviceId))?.isCrossSigningTrusted()) { |
There was a problem hiding this comment.
Actually, if we can get our own device, then cross-signing won't work, so I'm going to throw an error in that case. (Though I wonder why TypeScript didn't complain about ownDevice possibly being null.)
There was a problem hiding this comment.
Though I wonder why TypeScript didn't complain about
ownDevicepossibly beingnull.
It's because the TSConfig file doesn't have the strict flag enabled. It can't be turned on yet, but I've been using it temporarily in local development to help test new code.
| if (bootstrapRequests.uploadKeysReq) { | ||
| this.engine.processOutgoingRequests([bootstrapRequests.uploadKeysReq]); | ||
| } | ||
| await client.doRequest("POST", "/_matrix/client/v3/keys/device_signing/upload", null, JSON.parse(bootstrapRequests.uploadSigningKeysReq)); | ||
| await this.engine.processOutgoingRequests([bootstrapRequests.uploadSignaturesReq]); |
There was a problem hiding this comment.
If the processing of uploadKeysReq does need to be awaited, it could be processed together with the uploadSignaturesReq:
| if (bootstrapRequests.uploadKeysReq) { | |
| this.engine.processOutgoingRequests([bootstrapRequests.uploadKeysReq]); | |
| } | |
| await client.doRequest("POST", "/_matrix/client/v3/keys/device_signing/upload", null, JSON.parse(bootstrapRequests.uploadSigningKeysReq)); | |
| await this.engine.processOutgoingRequests([bootstrapRequests.uploadSignaturesReq]); | |
| const outgoingRequests = [ | |
| bootstrapRequests.uploadSignaturesReq, | |
| ...(bootstrapRequests.uploadKeysReq ? [bootstrapRequests.uploadKeysReq] : []), | |
| ]; | |
| await client.doRequest("POST", "/_matrix/client/v3/keys/device_signing/upload", null, JSON.parse(bootstrapRequests.uploadSigningKeysReq)); | |
| await this.engine.processOutgoingRequests(outgoingRequests); |
There was a problem hiding this comment.
Actually, I was wrong -- I hadn't seen the docstrings on CrossSigningBootstrapRequests that explain the order in which the requests should be sent out. The order you have was correct.
The processing of uploadKeysReq should be awaited, though:
| if (bootstrapRequests.uploadKeysReq) { | |
| this.engine.processOutgoingRequests([bootstrapRequests.uploadKeysReq]); | |
| } | |
| await client.doRequest("POST", "/_matrix/client/v3/keys/device_signing/upload", null, JSON.parse(bootstrapRequests.uploadSigningKeysReq)); | |
| await this.engine.processOutgoingRequests([bootstrapRequests.uploadSignaturesReq]); | |
| if (bootstrapRequests.uploadKeysReq) { | |
| await this.engine.processOutgoingRequests([bootstrapRequests.uploadKeysReq]); | |
| } | |
| await client.doRequest("POST", "/_matrix/client/v3/keys/device_signing/upload", null, JSON.parse(bootstrapRequests.uploadSigningKeysReq)); | |
| await this.engine.processOutgoingRequests([bootstrapRequests.uploadSignaturesReq]); |
| let secretStorageKey: SecretStorageKey; | ||
| if (key === undefined) { | ||
| secretStorageKey = SecretStorageKey.createRandomKey(); | ||
| } else { | ||
| secretStorageKey = await this.getDefaultSecretStorageKey(key); | ||
| } |
There was a problem hiding this comment.
secretStorageKey can be made const and without a typehint by using a ternary operator:
| let secretStorageKey: SecretStorageKey; | |
| if (key === undefined) { | |
| secretStorageKey = SecretStorageKey.createRandomKey(); | |
| } else { | |
| secretStorageKey = await this.getDefaultSecretStorageKey(key); | |
| } | |
| const secretStorageKey = | |
| key === undefined | |
| ? SecretStorageKey.createRandomKey() | |
| : await this.getDefaultSecretStorageKey(key); |
| for (let i = 0; i < 5; i++) { | ||
| http1.when("PUT", "/user/%40alice%3Aexample.org/account_data/").respond(200, (path, obj) => { |
There was a problem hiding this comment.
This would benefit from a comment to explain why this loop needs to run 5 times. AIUI it's because createIdentity hits this endpoint that many times, to upload account data for the default key ID, the metadata of that key (i.e. algorithm & passphrase), and the 3 cross-signing secrets.
|
matrix-org/matrix-rust-sdk-crypto-nodejs#67 is finally available in Note that the tests may fail when run in bulk. If that gives CI trouble, these PRs may help: |
|
Changes made. I've also updated element-hq#87 to match the current changes in this PR |
Adds functions to cross-sign the bot's device using a recovery key.
Depends on matrix-org/matrix-rust-sdk-crypto-nodejs#67, so merging will need to wait for that, plus a release of matrix-rust-sdk-crypto-nodejs, plus bumping
package.json.Cross-signing can be done by doing something like:
Checklist
Signed-off-by: Hubert Chathi hubertc@element.io