Add crypto methods for export and import of secrets bundle by t3chguy · Pull Request #4227 · matrix-org/matrix-js-sdk

t3chguy · 2024-06-03T08:35:47Z

Split out from #4134

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

BillCarsonFr

Some minor comments

BillCarsonFr · 2024-06-03T09:23:33Z

src/@types/matrix-sdk-crypto-wasm.d.ts

+import type * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-wasm";
+
+declare module "@matrix-org/matrix-sdk-crypto-wasm" {
+    interface OlmMachine {


Why do we need this? isn't it already in the index.d.ts of matrix-sdk-crypto-wasm?

Yes but with return types of any - see matrix-org/matrix-sdk-crypto-wasm#110

This should be fixed on the wasm side; matrix-org/matrix-sdk-crypto-wasm#123 does so but it will need a release cycle :/

Well, that PR doesn't help with the return type of SecretsBundle.to_json, to be fair. On the other hand: do you actually need to introspect the result of SecretsBundle.to_json outside of tests (where I would be inclined to do some @ts-ignoreing rather than have a separate definition to keep in step)?

Well, that PR doesn't help with the return type of SecretsBundle.to_json, to be fair. On the other hand: do you actually need to introspect the result of SecretsBundle.to_json outside of tests (where I would be inclined to do some @ts-ignoreing rather than have a separate definition to keep in step)?

It should still be a useful type otherwise you'll get junk into the import method. If it isn't meant to be introspectable then it should be a branded type so a round-trip export->import works without needing to rely on any

src/crypto/index.ts

BillCarsonFr · 2024-06-03T09:29:10Z

spec/unit/rust-crypto/rust-crypto.spec.ts

+                },
+            };
+            await rustCrypto.importSecretsForQrLogin(bundle);
+            await expect(rustCrypto.exportSecretsForQrLogin()).resolves.toEqual(expect.objectContaining(bundle));


Maybe we could check the getCrossSigningStatus? and see that the identity is there and trusted?

This causes the test to hang on OlmMachine::getIdentity and I have 0 introspection on the rust side of things to debug this.

Yes, getCrossSigningStatus will want to fire off /keys/query requests and wait for them to complete, so that is more of an integration-test thing.

That said: some integration tests here might be nice?

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

richvdh · 2024-06-03T10:32:13Z

Is this related to MSC4108?

richvdh · 2024-06-03T10:38:51Z

spec/unit/rust-crypto/rust-crypto.spec.ts

+                },
+            };
+            await rustCrypto.importSecretsForQrLogin(bundle);
+            await expect(rustCrypto.exportSecretsForQrLogin()).resolves.toEqual(expect.objectContaining(bundle));


Yes, getCrossSigningStatus will want to fire off /keys/query requests and wait for them to complete, so that is more of an integration-test thing.

That said: some integration tests here might be nice?

src/crypto-api.ts

src/rust-crypto/rust-crypto.ts

t3chguy · 2024-06-03T20:41:19Z

Is this related to MSC4108?

That is one consumer of such functions, but the Crypto crate doesn't seem to state that it is specific to MSC4108 so neither will this implementation be. Updated.

t3chguy · 2024-06-03T20:42:23Z

That said: some integration tests here might be nice?

I disagree. The docs on the methods I'm calling don't describe fully what it does so I cannot really form a full integration test without it being brittle with the exception of the assumption that importing and exporting round-trip successfully.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

richvdh · 2024-06-04T17:40:31Z

The docs on the methods I'm calling don't describe fully what it does so I cannot really form a full integration test without it being brittle with the exception of the assumption that importing and exporting round-trip successfully.

I don't really understand your objection here, I'm afraid. The docs on the rust crate say that the methods import/export the cross-signing keys, and indeed your own documentation (yay, documentation, thank you!) says that of the new methods in CryptoApi.

So can't we test that? We could export the secrets bundle from one MatrixClient, and import it into another, and then check that the second client has the same cross-signing keys as the first.

~~Frankly, if your argument is "I can't write an integration test because I don't understand what it dos~~ (edit: removed this, it wasn't what I meant to say)

I think a better argument is that these are very thin wrappers, and the cost/benefit of writing a complicated integration test is low, given the meat of the functionality should already be tested at the rust-sdk level.

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy · 2024-06-04T17:49:58Z

We could export the secrets bundle from one MatrixClient, and import it into another, and then check that the second client has the same cross-signing keys as the first.

We already do. I exported a bundle from a test account & baked it into the test, import it and validate that exporting yields the same bundle once again.

…hguy/oidc-qr-crypto

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

richvdh · 2024-06-05T08:33:22Z

errr I seem to have pressed enter halfway through editing that comment, so sorry for the half-formed thoughts

richvdh · 2024-06-05T08:35:39Z

We could export the secrets bundle from one MatrixClient, and import it into another, and then check that the second client has the same cross-signing keys as the first.

We already do. I exported a bundle from a test account & baked it into the test, import it and validate that exporting yields the same bundle once again.

So we do. Apparently I need to read better.

richvdh

Thanks. Sorry this took a while to review.

richvdh · 2024-06-05T08:42:30Z

src/rust-crypto/rust-crypto.ts

+     * Implementation of {@link CryptoApi#importSecretsBundle}.
+     */
+    public async importSecretsBundle(
+        secrets: Parameters<NonNullable<CryptoApi["importSecretsBundle"]>>[0],


I still think there's way too much type magic going on here. As a caller of this function, how am I meant to build one of these things?

... I'm not going to block the PR further on it, though.

As a caller of this function, how am I meant to build one of these things?

you shouldn't construct manually, only ever exported by the matching export function

richvdh · 2024-06-05T08:44:02Z

I want to do some work in RustCrypto that will conflict with this, so merging

Add crypto methods for OIDC QR code login

30c8ba6

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy added the T-Enhancement label Jun 3, 2024

github-actions bot deployed to PR Documentation Preview June 3, 2024 08:37 View deployment

t3chguy marked this pull request as ready for review June 3, 2024 08:55

t3chguy requested review from a team as code owners June 3, 2024 08:55

t3chguy requested review from BillCarsonFr, richvdh, robintown and uhoreg June 3, 2024 08:55

BillCarsonFr approved these changes Jun 3, 2024

View reviewed changes

t3chguy self-assigned this Jun 3, 2024

Improve test

30e586a

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview June 3, 2024 09:42 View deployment

Revert test due to hang inside Rust.

fc59cf0

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview June 3, 2024 09:51 View deployment

richvdh requested changes Jun 3, 2024

View reviewed changes

richvdh mentioned this pull request Jun 3, 2024

MSC4108 support OIDC QR code login #4134

Merged

Iterate

4a00109

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy requested a review from richvdh June 3, 2024 20:52

github-actions bot deployed to PR Documentation Preview June 3, 2024 20:52 View deployment

Merge branch 'develop' into t3chguy/oidc-qr-crypto

235d1d4

github-actions bot deployed to PR Documentation Preview June 3, 2024 20:58 View deployment

t3chguy changed the title ~~Add crypto methods for OIDC QR code login~~ Add crypto methods for export and import of secrets bundle Jun 3, 2024

Update test name

af07b84

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

t3chguy added 2 commits June 4, 2024 18:57

Merge remote-tracking branch 'origin/t3chguy/oidc-qr-crypto' into t3c…

7bfe091

…hguy/oidc-qr-crypto

Update test name

ff83fe8

Signed-off-by: Michael Telatynski <7t3chguy@gmail.com>

github-actions bot deployed to PR Documentation Preview June 4, 2024 17:59 View deployment

richvdh approved these changes Jun 5, 2024

View reviewed changes

richvdh added this pull request to the merge queue Jun 5, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 5, 2024

t3chguy added this pull request to the merge queue Jun 5, 2024

Merged via the queue into develop with commit a3cea8c Jun 5, 2024

t3chguy deleted the t3chguy/oidc-qr-crypto branch June 5, 2024 09:51

Uh oh!

Conversation

t3chguy commented Jun 3, 2024

Uh oh!

BillCarsonFr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Jun 3, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

t3chguy commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t3chguy commented Jun 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

t3chguy commented Jun 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

richvdh commented Jun 5, 2024

Uh oh!

richvdh commented Jun 5, 2024

Uh oh!

richvdh left a comment

Choose a reason for hiding this comment

Uh oh!

richvdh Jun 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

richvdh commented Jun 5, 2024

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

t3chguy commented Jun 3, 2024 •

edited

Loading

t3chguy commented Jun 3, 2024 •

edited

Loading

richvdh commented Jun 4, 2024 •

edited

Loading

t3chguy commented Jun 4, 2024 •

edited

Loading

richvdh Jun 5, 2024 •

edited

Loading