From d92936fba5c0ff76fbf1a9ceb7ce9685b0731ade Mon Sep 17 00:00:00 2001 From: Richard van der Hoff <1389908+richvdh@users.noreply.github.com> Date: Thu, 13 Jul 2023 12:11:13 +0100 Subject: [PATCH] Element-R: support for displaying QR codes during verification (#3588) * Support for showing QR codes * Emit `VerificationRequestEvent.Change` events when the verifier changes * Minor integ test tweaks * Handle transitions from QR code display to SAS * Fix naming * Add a test for `ShowQrCodeCallbacks.cancel` --- spec/integ/crypto/verification.spec.ts | 184 ++++++++++++++++++++++--- src/crypto-api/verification.ts | 4 +- src/rust-crypto/verification.ts | 133 +++++++++++++++--- 3 files changed, 279 insertions(+), 42 deletions(-) diff --git a/spec/integ/crypto/verification.spec.ts b/spec/integ/crypto/verification.spec.ts index 1a8ce32f1..56acc6d5c 100644 --- a/spec/integ/crypto/verification.spec.ts +++ b/spec/integ/crypto/verification.spec.ts @@ -81,12 +81,8 @@ const TEST_HOMESERVER_URL = "https://alice-server.com"; */ // we test with both crypto stacks... describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: string, initCrypto: InitCrypto) => { - // oldBackendOnly is an alternative to `it` or `test` which will skip the test if we are running against the - // Rust backend. Once we have full support in the rust sdk, it will go away. - const oldBackendOnly = backend === "rust-sdk" ? test.skip : test; - // newBackendOnly is the opposite to `oldBackendOnly`: it will skip the test if we are running against the legacy - // backend. + // backend. Once we drop support for legacy crypto, it will go away. const newBackendOnly = backend === "rust-sdk" ? test : test.skip; /** the client under test */ @@ -391,12 +387,11 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st expect(toDeviceMessage.transaction_id).toEqual(transactionId); }); - oldBackendOnly("can verify another via QR code with an untrusted cross-signing key", async () => { + it("can verify another via QR code with an untrusted cross-signing key", async () => { aliceClient = await startTestClient(); // QRCode fails if we don't yet have the cross-signing keys, so make sure we have them now. e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); await waitForDeviceList(); - expect(aliceClient.getStoredCrossSigningForUser(TEST_USER_ID)).toBeTruthy(); // have alice initiate a verification. She should send a m.key.verification.request const [requestBody, request] = await Promise.all([ @@ -434,16 +429,12 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st ); const sharedSecret = qrCodeBuffer.subarray(74 + txnIdLen); + // we should still be "Ready" and have no verifier + expect(request.phase).toEqual(VerificationPhase.Ready); + expect(request.verifier).toBeUndefined(); + // the dummy device "scans" the displayed QR code and acknowledges it with a "m.key.verification.start" - returnToDeviceMessageFromSync({ - type: "m.key.verification.start", - content: { - from_device: TEST_DEVICE_ID, - method: "m.reciprocate.v1", - transaction_id: transactionId, - secret: encodeUnpaddedBase64(sharedSecret), - }, - }); + returnToDeviceMessageFromSync(buildReciprocateStartMessage(transactionId, sharedSecret)); await waitForVerificationRequestChanged(request); expect(request.phase).toEqual(VerificationPhase.Started); expect(request.chosenMethod).toEqual("m.reciprocate.v1"); @@ -451,23 +442,25 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st // there should now be a verifier const verifier: Verifier = request.verifier!; expect(verifier).toBeDefined(); - expect(verifier.getReciprocateQrCodeCallbacks()).toBeNull(); // ... which we call .verify on, which emits a ShowReciprocateQr event - const verificationPromise = verifier.verify(); - const reciprocateQRCodeCallbacks = await new Promise((resolve) => { + const reciprocatePromise = new Promise((resolve) => { verifier.once(VerifierEvent.ShowReciprocateQr, resolve); }); + const verificationPromise = verifier.verify(); + const reciprocateQRCodeCallbacks = await reciprocatePromise; // getReciprocateQrCodeCallbacks() is an alternative way to get the callbacks expect(verifier.getReciprocateQrCodeCallbacks()).toBe(reciprocateQRCodeCallbacks); expect(verifier.getShowSasCallbacks()).toBeNull(); - // Alice confirms she is happy + // Alice confirms she is happy, which makes her reply with a 'done' + const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.done"); reciprocateQRCodeCallbacks.confirm(); + await sendToDevicePromise; - // that should satisfy Alice, who should reply with a 'done' - await expectSendToDeviceMessage("m.key.verification.done"); + // the dummy device replies with its own 'done' + returnToDeviceMessageFromSync(buildDoneMessage(transactionId)); // ... and the whole thing should be done! await verificationPromise; @@ -531,12 +524,102 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st await verificationPromise; expect(request.phase).toEqual(VerificationPhase.Done); }); + + it("can send an SAS start after QR code display", async () => { + aliceClient = await startTestClient(); + e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); + await waitForDeviceList(); + + // Alice sends a m.key.verification.request + const [, request] = await Promise.all([ + expectSendToDeviceMessage("m.key.verification.request"), + aliceClient.getCrypto()!.requestDeviceVerification(TEST_USER_ID, TEST_DEVICE_ID), + ]); + const transactionId = request.transactionId!; + + // The dummy device replies with an m.key.verification.ready, with an indication it can scan a QR code + // or do the emoji dance + returnToDeviceMessageFromSync( + buildReadyMessage(transactionId, ["m.qr_code.scan.v1", "m.sas.v1", "m.reciprocate.v1"]), + ); + await waitForVerificationRequestChanged(request); + expect(request.phase).toEqual(VerificationPhase.Ready); + + // Alice displays the QR code + const qrCodeBuffer = (await request.generateQRCode())!; + expect(qrCodeBuffer).toBeTruthy(); + expect(request.phase).toEqual(VerificationPhase.Ready); + expect(request.verifier).toBeUndefined(); + + // advance the clock, because the devicelist likes to sleep for 5ms during key downloads + await jest.advanceTimersByTimeAsync(10); + + // ... but Alice wants to do an SAS verification + const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.start"); + await request.startVerification("m.sas.v1"); + await sendToDevicePromise; + + // There should now be a `verifier` + const verifier: Verifier = request.verifier!; + expect(verifier).toBeDefined(); + expect(request.chosenMethod).toEqual("m.sas.v1"); + + // clean up the test + expectSendToDeviceMessage("m.key.verification.cancel"); + request.cancel(); + await expect(verifier.verify()).rejects.toBeTruthy(); + }); + + it("can receive an SAS start after QR code display", async () => { + aliceClient = await startTestClient(); + e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); + await waitForDeviceList(); + + // Alice sends a m.key.verification.request + const [, request] = await Promise.all([ + expectSendToDeviceMessage("m.key.verification.request"), + aliceClient.getCrypto()!.requestDeviceVerification(TEST_USER_ID, TEST_DEVICE_ID), + ]); + const transactionId = request.transactionId!; + + // The dummy device replies with an m.key.verification.ready, with an indication it can scan a QR code + // or do the emoji dance + returnToDeviceMessageFromSync( + buildReadyMessage(transactionId, ["m.qr_code.scan.v1", "m.sas.v1", "m.reciprocate.v1"]), + ); + await waitForVerificationRequestChanged(request); + expect(request.phase).toEqual(VerificationPhase.Ready); + + // Alice displays the QR code + const qrCodeBuffer = (await request.generateQRCode())!; + expect(qrCodeBuffer).toBeTruthy(); + expect(request.phase).toEqual(VerificationPhase.Ready); + expect(request.verifier).toBeUndefined(); + + // advance the clock, because the devicelist likes to sleep for 5ms during key downloads + await jest.advanceTimersByTimeAsync(10); + + // ... but the dummy device wants to do an SAS verification + returnToDeviceMessageFromSync(buildSasStartMessage(transactionId)); + await emitPromise(request, VerificationRequestEvent.Change); + + // Alice should now have a `verifier` + const verifier: Verifier = request.verifier!; + expect(verifier).toBeDefined(); + expect(request.chosenMethod).toEqual("m.sas.v1"); + + // clean up the test + expectSendToDeviceMessage("m.key.verification.cancel"); + request.cancel(); + await expect(verifier.verify()).rejects.toBeTruthy(); + }); }); describe("cancellation", () => { beforeEach(async () => { // pretend that we have another device, which we will start verifying e2eKeyResponder.addDeviceKeys(TEST_USER_ID, TEST_DEVICE_ID, SIGNED_TEST_DEVICE_DATA); + e2eKeyResponder.addCrossSigningData(SIGNED_CROSS_SIGNING_KEYS_DATA); aliceClient = await startTestClient(); await waitForDeviceList(); @@ -604,6 +687,50 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st expect(request.phase).toEqual(VerificationPhase.Cancelled); expect(verifier.hasBeenCancelled).toBe(true); }); + + it("can cancel in the ShowQrCodeCallbacks", async () => { + // have alice initiate a verification. She should send a m.key.verification.request + const [, request] = await Promise.all([ + expectSendToDeviceMessage("m.key.verification.request"), + aliceClient.getCrypto()!.requestDeviceVerification(TEST_USER_ID, TEST_DEVICE_ID), + ]); + const transactionId = request.transactionId!; + + // The dummy device replies with an m.key.verification.ready, with an indication it can scan the QR code + returnToDeviceMessageFromSync(buildReadyMessage(transactionId, ["m.qr_code.scan.v1"])); + await waitForVerificationRequestChanged(request); + expect(request.phase).toEqual(VerificationPhase.Ready); + + // we should now have QR data we can display + const qrCodeBuffer = (await request.generateQRCode())!; + expect(qrCodeBuffer).toBeTruthy(); + const sharedSecret = qrCodeBuffer.subarray(74 + transactionId.length); + + // the dummy device "scans" the displayed QR code and acknowledges it with a "m.key.verification.start" + returnToDeviceMessageFromSync(buildReciprocateStartMessage(transactionId, sharedSecret)); + await waitForVerificationRequestChanged(request); + expect(request.phase).toEqual(VerificationPhase.Started); + expect(request.chosenMethod).toEqual("m.reciprocate.v1"); + + // there should now be a verifier + const verifier: Verifier = request.verifier!; + expect(verifier).toBeDefined(); + + // ... which we call .verify on, which emits a ShowReciprocateQr event + const reciprocatePromise = emitPromise(verifier, VerifierEvent.ShowReciprocateQr); + const verificationPromise = verifier.verify(); + const reciprocateQRCodeCallbacks: ShowQrCodeCallbacks = await reciprocatePromise; + + // Alice complains that she didn't see the dummy device scan her code + const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.cancel"); + reciprocateQRCodeCallbacks.cancel(); + await sendToDevicePromise; + + // ... which should cancel the verifier + await expect(verificationPromise).rejects.toBeTruthy(); + expect(request.phase).toEqual(VerificationPhase.Cancelled); + expect(verifier.hasBeenCancelled).toBe(true); + }); }); describe("Incoming verification from another device", () => { @@ -779,6 +906,19 @@ function buildReadyMessage(transactionId: string, methods: string[]): { type: st }; } +/** build an m.key.verification.start to-device message suitable for the m.reciprocate.v1 flow, originating from the dummy device */ +function buildReciprocateStartMessage(transactionId: string, sharedSecret: Uint8Array) { + return { + type: "m.key.verification.start", + content: { + from_device: TEST_DEVICE_ID, + method: "m.reciprocate.v1", + transaction_id: transactionId, + secret: encodeUnpaddedBase64(sharedSecret), + }, + }; +} + /** build an m.key.verification.start to-device message suitable for the SAS flow, originating from the dummy device */ function buildSasStartMessage(transactionId: string): { type: string; content: object } { return { diff --git a/src/crypto-api/verification.ts b/src/crypto-api/verification.ts index 07d3dbbee..acbade99f 100644 --- a/src/crypto-api/verification.ts +++ b/src/crypto-api/verification.ts @@ -310,7 +310,7 @@ export enum VerifierEvent { ShowSas = "show_sas", /** - * QR code data should be displayed to the user. + * The user should confirm if the other side has scanned our QR code. * * The payload is the {@link ShowQrCodeCallbacks} object. */ @@ -325,7 +325,7 @@ export type VerifierEventHandlerMap = { }; /** - * Callbacks for user actions while a QR code is displayed. + * Callbacks for user actions to confirm that the other side has scanned our QR code. * * This is exposed as the payload of a `VerifierEvent.ShowReciprocateQr` event, or can be retrieved directly from the * verifier as `reciprocateQREvent`. diff --git a/src/rust-crypto/verification.ts b/src/rust-crypto/verification.ts index 5066ad1fa..85ba876d2 100644 --- a/src/rust-crypto/verification.ts +++ b/src/rust-crypto/verification.ts @@ -15,7 +15,7 @@ limitations under the License. */ import * as RustSdkCryptoJs from "@matrix-org/matrix-sdk-crypto-js"; -import { Emoji } from "@matrix-org/matrix-sdk-crypto-js"; +import { Emoji, QrState } from "@matrix-org/matrix-sdk-crypto-js"; import { ShowQrCodeCallbacks, @@ -30,6 +30,7 @@ import { } from "../crypto-api/verification"; import { TypedEventEmitter } from "../models/typed-event-emitter"; import { OutgoingRequest, OutgoingRequestProcessor } from "./OutgoingRequestProcessor"; +import { TypedReEmitter } from "../ReEmitter"; /** * An incoming, or outgoing, request to verify a user or a device via cross-signing. @@ -40,13 +41,16 @@ export class RustVerificationRequest extends TypedEventEmitter implements VerificationRequest { + /** a reëmitter which relays VerificationRequestEvent.Changed events emitted by the verifier */ + private readonly reEmitter: TypedReEmitter; + /** Are we in the process of sending an `m.key.verification.ready` event? */ private _accepting = false; /** Are we in the process of sending an `m.key.verification.cancellation` event? */ private _cancelling = false; - private _verifier: Verifier | undefined; + private _verifier: undefined | RustSASVerifier | RustQrCodeVerifier; /** * Construct a new RustVerificationRequest to wrap the rust-level `VerificationRequest`. @@ -62,15 +66,19 @@ export class RustVerificationRequest ) { super(); + this.reEmitter = new TypedReEmitter(this); + const onChange = async (): Promise => { - // if we now have a `Verification` where we lacked one before, wrap it. - if (this._verifier === undefined) { - const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification(); - if (verification instanceof RustSdkCryptoJs.Sas) { + const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification(); + + // If we now have a `Verification` where we lacked one before, or we have transitioned from QR to SAS, + // wrap the new rust Verification as a js-sdk Verifier. + if (verification instanceof RustSdkCryptoJs.Sas) { + if (this._verifier === undefined || this._verifier instanceof RustQrCodeVerifier) { this.setVerifier(new RustSASVerifier(verification, this, outgoingRequestProcessor)); - } else if (verification instanceof RustSdkCryptoJs.Qr) { - this.setVerifier(new RustQrCodeVerifier(verification, outgoingRequestProcessor)); } + } else if (verification instanceof RustSdkCryptoJs.Qr && this._verifier === undefined) { + this.setVerifier(new RustQrCodeVerifier(verification, outgoingRequestProcessor)); } this.emit(VerificationRequestEvent.Change); @@ -79,7 +87,12 @@ export class RustVerificationRequest } private setVerifier(verifier: RustSASVerifier | RustQrCodeVerifier): void { + // if we already have a verifier, unsubscribe from its events + if (this._verifier) { + this.reEmitter.stopReEmitting(this._verifier, [VerificationRequestEvent.Change]); + } this._verifier = verifier; + this.reEmitter.reEmit(this._verifier, [VerificationRequestEvent.Change]); } /** @@ -138,7 +151,11 @@ export class RustVerificationRequest // parlance. return this._accepting ? VerificationPhase.Requested : VerificationPhase.Ready; case RustSdkCryptoJs.VerificationRequestPhase.Transitioned: - return VerificationPhase.Started; + if (!this._verifier) { + // this shouldn't happen, because the onChange handler should have created a _verifier. + throw new Error("VerificationRequest: inner phase == Transitioned but no verifier!"); + } + return this._verifier.verificationPhase; case RustSdkCryptoJs.VerificationRequestPhase.Done: return VerificationPhase.Done; case RustSdkCryptoJs.VerificationRequestPhase.Cancelled: @@ -189,8 +206,9 @@ export class RustVerificationRequest /** the method picked in the .start event */ public get chosenMethod(): string | null { + if (this.phase !== VerificationPhase.Started) return null; + const verification: RustSdkCryptoJs.Qr | RustSdkCryptoJs.Sas | undefined = this.inner.getVerification(); - // TODO: this isn't quite right. The existence of a Verification doesn't prove that we have .started. if (verification instanceof RustSdkCryptoJs.Sas) { return "m.sas.v1"; } else if (verification instanceof RustSdkCryptoJs.Qr) { @@ -350,15 +368,20 @@ export class RustVerificationRequest * Only defined when the `phase` is Started. */ public get verifier(): Verifier | undefined { - return this._verifier; + // It's possible for us to have a Verifier before a method has been chosen (in particular, + // if we are showing a QR code which the other device has not yet scanned. At that point, we could + // still switch to SAS). + // + // In that case, we should not return it to the application yet, since the application will not expect the + // Verifier to be replaced during the lifetime of the VerificationRequest. + return this.phase === VerificationPhase.Started ? this._verifier : undefined; } /** * Stub implementation of {@link Crypto.VerificationRequest#getQRCodeBytes}. */ public getQRCodeBytes(): Buffer | undefined { - // TODO - return undefined; + throw new Error("getQRCodeBytes() unsupported in Rust Crypto; use generateQRCode() instead."); } /** @@ -367,8 +390,8 @@ export class RustVerificationRequest * Implementation of {@link Crypto.VerificationRequest#generateQRCode}. */ public async generateQRCode(): Promise { - // TODO - return undefined; + const innerVerifier: RustSdkCryptoJs.Qr = await this.inner.generateQrCode(); + return Buffer.from(innerVerifier.toBytes()); } /** @@ -396,8 +419,8 @@ export class RustVerificationRequest * @internal */ abstract class BaseRustVerifer extends TypedEventEmitter< - VerifierEvent, - VerifierEventHandlerMap + VerifierEvent | VerificationRequestEvent, + VerifierEventHandlerMap & VerificationRequestEventHandlerMap > { /** A promise which completes when the verification completes (or rejects when it is cancelled/fails) */ protected readonly completionPromise: Promise; @@ -424,6 +447,8 @@ abstract class BaseRustVerifer implements Verifier { + private callbacks: ShowQrCodeCallbacks | null = null; + public constructor(inner: RustSdkCryptoJs.Qr, outgoingRequestProcessor: OutgoingRequestProcessor) { super(inner, outgoingRequestProcessor); } + protected onChange(): void { + // if the other side has scanned our QR code and sent us a "reciprocate" message, it is now time for the + // application to prompt the user to confirm their side. + if (this.callbacks === null && this.inner.hasBeenScanned()) { + this.callbacks = { + confirm: () => this.confirmScanning(), + cancel: () => this.cancel(), + }; + } + } + /** * Start the key verification, if it has not already been started. * @@ -502,9 +540,61 @@ export class RustQrCodeVerifier extends BaseRustVerifer impl * or times out. */ public async verify(): Promise { + // Some applications (hello, matrix-react-sdk) may not check if there is a `ShowQrCodeCallbacks` and instead + // register a `ShowReciprocateQr` listener which they expect to be called once `.verify` is called. + if (this.callbacks !== null) { + this.emit(VerifierEvent.ShowReciprocateQr, this.callbacks); + } // Nothing to do here but wait. await this.completionPromise; } + + /** + * Calculate an appropriate VerificationPhase for a VerificationRequest where this is the verifier. + * + * This is abnormally complicated because a rust-side QR Code verifier can span several verification phases. + */ + public get verificationPhase(): VerificationPhase { + switch (this.inner.state()) { + case QrState.Created: + // we have created a QR for display; neither side has yet sent an `m.key.verification.start`. + return VerificationPhase.Ready; + case QrState.Scanned: + // other side has scanned our QR and sent an `m.key.verification.start` with `m.reciprocate.v1` + return VerificationPhase.Started; + case QrState.Confirmed: + // we have confirmed the other side's scan and sent an `m.key.verification.done`. + return VerificationPhase.Done; + case QrState.Reciprocated: + // although the rust SDK doesn't immediately send the `m.key.verification.start` on transition into this + // state, `RustVerificationRequest.scanQrCode` immediately calls `reciprocate()` and does so, so in practice + // we can treat the two the same. + return VerificationPhase.Started; + case QrState.Done: + return VerificationPhase.Done; + case QrState.Cancelled: + return VerificationPhase.Cancelled; + default: + throw new Error(`Unknown qr code state ${this.inner.state()}`); + } + } + + /** + * Get the details for reciprocating QR code verification, if one is in progress + * + * Returns `null`, unless this verifier is for reciprocating a QR-code-based verification (ie, the other user has + * already scanned our QR code), and we are waiting for the user to confirm. + */ + public getReciprocateQrCodeCallbacks(): ShowQrCodeCallbacks | null { + return this.callbacks; + } + + private async confirmScanning(): Promise { + const req: undefined | OutgoingRequest = this.inner.confirmScanning(); + if (req) { + await this.outgoingRequestProcessor.makeOutgoingRequest(req); + } + } } /** A Verifier instance which is used if we are exchanging emojis */ @@ -568,6 +658,13 @@ export class RustSASVerifier extends BaseRustVerifer implem } } + /** + * Calculate an appropriate VerificationPhase for a VerificationRequest where this is the verifier. + */ + public get verificationPhase(): VerificationPhase { + return VerificationPhase.Started; + } + /** * Get the details for an SAS verification, if one is in progress *