Save the key backup key to 4S during bootstrapCrossSigning
#4542
Conversation
When setting up secret storage, if we have a key backup key in cache (like we do for the cross signing secrets).
src/rust-crypto/rust-crypto.ts
Outdated
| ]); | ||
|
|
||
| if (keyBackupKey && !keyBackupKeyInStorage) { | ||
| await this.secretStorage.store("m.megolm_backup.v1", encodeBase64(keyBackupKey)); |
There was a problem hiding this comment.
Is this the right way to do this? Seems like it, but I couldn't find a comparison.
There was a problem hiding this comment.
There should be an example in when you resetKeyBackup(). I think that is fine, maybe you could get it from the rust olmMachine directly to avoid a base64 -> byte array -> base64?
Shouldn't we check more than keyBackupKeyInStorage ? What if the 4S contains an old key it won't be updated? Maybe we should check that the key in cache is valid before adding it to 4S? Is the backup trusted?
Is there a link to an issue for this PR?
There was a problem hiding this comment.
Okay, I think that should be better. I've made it overwrite the key if different, as per the comment.
This came out as part of the key backup by default work, so it's not a huge bug currently, but would break the flow of setting up 4S after a key backup, hence no issue.
src/rust-crypto/rust-crypto.ts
Outdated
| ]); | ||
|
|
||
| if (keyBackupKey && !keyBackupKeyInStorage) { | ||
| await this.secretStorage.store("m.megolm_backup.v1", encodeBase64(keyBackupKey)); |
There was a problem hiding this comment.
There should be an example in when you resetKeyBackup(). I think that is fine, maybe you could get it from the rust olmMachine directly to avoid a base64 -> byte array -> base64?
Shouldn't we check more than keyBackupKeyInStorage ? What if the 4S contains an old key it won't be updated? Maybe we should check that the key in cache is valid before adding it to 4S? Is the backup trusted?
Is there a link to an issue for this PR?
saves converting it needlessly into a buffer to turn it back into a base64 string
richvdh
changed the title
bootstrapCrossSigning
| @@ -705,6 +705,47 @@ describe("RustCrypto", () => { | |||
| expect(resetKeyBackup.mock.calls).toHaveLength(2); | |||
| }); | |||
|
|
|||
| it("bootstrapSecretStorage saves megolm backup key if already cached", async () => { | |||
There was a problem hiding this comment.
Can we get an integ test for this as well, please? (One which works only through the public APIs of the js-sdk, together with fetchMock)
There was a problem hiding this comment.
I think this should do the trick?
When setting up secret storage, if we have a key backup key in cache (like we do for the cross signing secrets).
Checklist
public/exportedsymbols have accurate TSDoc documentation.