Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for scanning QR codes during verification, with Rust crypto #3565

Merged
merged 5 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 81 additions & 0 deletions spec/integ/crypto/verification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st
// 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.
const newBackendOnly = backend === "rust-sdk" ? test : test.skip;

/** the client under test */
let aliceClient: MatrixClient;

Expand Down Expand Up @@ -469,6 +473,64 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("verification (%s)", (backend: st
await verificationPromise;
expect(request.phase).toEqual(VerificationPhase.Done);
});

newBackendOnly("can verify another by scanning their QR code", async () => {
aliceClient = await startTestClient();
// we need cross-signing keys for a QR code verification
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, indicating it can show a QR code
returnToDeviceMessageFromSync(buildReadyMessage(transactionId, ["m.qr_code.show.v1", "m.reciprocate.v1"]));
await waitForVerificationRequestChanged(request);
expect(request.phase).toEqual(VerificationPhase.Ready);
expect(request.otherPartySupportsMethod("m.qr_code.show.v1")).toBe(true);

// the dummy device shows a QR code
const sharedSecret = "SUPERSEKRET";
const qrCodeBuffer = buildQRCode(
transactionId,
TEST_DEVICE_PUBLIC_ED25519_KEY_BASE64,
MASTER_CROSS_SIGNING_PUBLIC_KEY_BASE64,
sharedSecret,
);

// Alice scans the QR code
const sendToDevicePromise = expectSendToDeviceMessage("m.key.verification.start");
const verifier = await request.scanQRCode(qrCodeBuffer);

const requestBody = await sendToDevicePromise;
const toDeviceMessage = requestBody.messages[TEST_USER_ID][TEST_DEVICE_ID];
expect(toDeviceMessage).toEqual({
from_device: aliceClient.deviceId,
method: "m.reciprocate.v1",
transaction_id: transactionId,
secret: encodeUnpaddedBase64(Buffer.from(sharedSecret)),
});

expect(request.phase).toEqual(VerificationPhase.Started);
expect(request.chosenMethod).toEqual("m.reciprocate.v1");
expect(verifier.getReciprocateQrCodeCallbacks()).toBeNull();

const verificationPromise = verifier.verify();

// the dummy device confirms that Alice scanned the QR code, by replying with a done
returnToDeviceMessageFromSync(buildDoneMessage(transactionId));

// Alice also replies with a 'done'
await expectSendToDeviceMessage("m.key.verification.done");

// ... and the whole thing should be done!
await verificationPromise;
expect(request.phase).toEqual(VerificationPhase.Done);
});
});

describe("cancellation", () => {
Expand Down Expand Up @@ -794,3 +856,22 @@ function buildDoneMessage(transactionId: string) {
},
};
}

function buildQRCode(transactionId: string, key1Base64: string, key2Base64: string, sharedSecret: string): Uint8Array {
// https://spec.matrix.org/v1.7/client-server-api/#qr-code-format

const qrCodeBuffer = Buffer.alloc(150); // oversize
let idx = 0;
idx += qrCodeBuffer.write("MATRIX", idx, "ascii");
idx = qrCodeBuffer.writeUInt8(0x02, idx); // version
idx = qrCodeBuffer.writeUInt8(0x02, idx); // mode
idx = qrCodeBuffer.writeInt16BE(transactionId.length, idx);
idx += qrCodeBuffer.write(transactionId, idx, "ascii");

idx += Buffer.from(key1Base64, "base64").copy(qrCodeBuffer, idx);
idx += Buffer.from(key2Base64, "base64").copy(qrCodeBuffer, idx);
idx += qrCodeBuffer.write(sharedSecret, idx);

// truncate to the right length
return qrCodeBuffer.subarray(0, idx);
}
2 changes: 1 addition & 1 deletion spec/unit/rust-crypto/verification.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ function makeTestRequest(
): RustVerificationRequest {
inner ??= makeMockedInner();
outgoingRequestProcessor ??= {} as OutgoingRequestProcessor;
return new RustVerificationRequest(inner, outgoingRequestProcessor, undefined);
return new RustVerificationRequest(inner, outgoingRequestProcessor, []);
}

/** Mock up a rust-side VerificationRequest */
Expand Down
2 changes: 1 addition & 1 deletion src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2241,7 +2241,7 @@ export class MatrixClient extends TypedEventEmitter<EmittedEvents, ClientEventHa
this.cryptoCallbacks,
useIndexedDB ? RUST_SDK_STORE_PREFIX : null,
);
rustCrypto.supportedVerificationMethods = this.verificationMethods;
rustCrypto.setSupportedVerificationMethods(this.verificationMethods);

this.cryptoBackend = rustCrypto;

Expand Down
15 changes: 15 additions & 0 deletions src/crypto-api/verification.ts
Original file line number Diff line number Diff line change
Expand Up @@ -136,12 +136,27 @@ export interface VerificationRequest
/**
* Send an `m.key.verification.start` event to start verification via a particular method.
*
* This is normally used when starting a verification via emojis (ie, `method` is set to `m.sas.v1`).
*
* @param method - the name of the verification method to use.
*
* @returns The verifier which will do the actual verification.
*/
startVerification(method: string): Promise<Verifier>;

/**
* Start a QR code verification by providing a scanned QR code for this verification flow.
*
* Validates the QR code, and if it is ok, sends an `m.key.verification.start` event with `method` set to
* `m.reciprocate.v1`, to tell the other side the scan was successful.
*
* See also {@link VerificationRequest#startVerification} which can be used to start other verification methods.
*
* @param qrCodeData - the decoded QR code.
* @returns A verifier; call `.verify()` on it to wait for the other side to complete the verification flow.
*/
scanQRCode(qrCodeData: Uint8Array): Promise<Verifier>;

/**
* The verifier which is doing the actual verification, once the method has been established.
* Only defined when the `phase` is Started.
Expand Down
4 changes: 4 additions & 0 deletions src/crypto/verification/request/VerificationRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,6 +478,10 @@ export class VerificationRequest<C extends IVerificationChannel = IVerificationC
return verifier;
}

public scanQRCode(qrCodeData: Uint8Array): Promise<Verifier> {
throw new Error("QR code scanning not supported by legacy crypto");
}

/**
* sends the initial .request event.
* @returns resolves when the event has been sent.
Expand Down
24 changes: 17 additions & 7 deletions src/rust-crypto/rust-crypto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ import { EventType } from "../@types/event";
import { CryptoEvent } from "../crypto";
import { TypedEventEmitter } from "../models/typed-event-emitter";

const ALL_VERIFICATION_METHODS = ["m.sas.v1", "m.qr_code.scan.v1", "m.qr_code.show.v1", "m.reciprocate.v1"];

/**
* An implementation of {@link CryptoBackend} using the Rust matrix-sdk-crypto.
*/
Expand Down Expand Up @@ -558,7 +560,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
new RustVerificationRequest(
request,
this.outgoingRequestProcessor,
this.supportedVerificationMethods,
this._supportedVerificationMethods,
),
);
}
Expand All @@ -580,10 +582,18 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv

/**
* The verification methods we offer to the other side during an interactive verification.
*/
private _supportedVerificationMethods: string[] = ALL_VERIFICATION_METHODS;

/**
* Set the verification methods we offer to the other side during an interactive verification.
*
* If `undefined`, we will offer all the methods supported by the Rust SDK.
*/
public supportedVerificationMethods: string[] | undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a breaking change, any reason to not use a getter+setter?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't a public class, so it's not a breaking change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What makes it not public? Where is it marked as internal or experimental?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's just not exported through index.ts or browser-index.ts (ie, the public interface of the js-sdk).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither are a bunch of the things necessary to use the SDK so I don't think that's the rule. (Namely the Typed EventEmitter events which are necessary to set up listeners0.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neither are a bunch of the things necessary to use the SDK so I don't think that's the rule. (Namely the Typed EventEmitter events which are necessary to set up listeners0.

Not quite sure what you're referring to, but anything not being exported through index.js that we expect clients to be able to refer to is very definitely a bug.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not quite sure what you're referring to

#3506

Copy link
Member Author

@richvdh richvdh Jul 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes well, that's a bug, and there's the issue to track it. Great.

The point remains that (a) if anybody has tried using rust crypto at all outside the react-sdk I will eat my hat; (b) even if they have, using RustCrypto or RustVerificationRequest directly is an application bug. I won't obfuscate this API just to support someone relying on the internals of the library in this way.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3574 then, if my argument isn't convincing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now an @internal class.

public setSupportedVerificationMethods(methods: string[] | undefined): void {
// by default, the Rust SDK does not offer `m.qr_code.scan.v1`, but we do want to offer that.
this._supportedVerificationMethods = methods ?? ALL_VERIFICATION_METHODS;
}

/**
* Send a verification request to our other devices.
Expand All @@ -604,10 +614,10 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv

const [request, outgoingRequest]: [RustSdkCryptoJs.VerificationRequest, RustSdkCryptoJs.ToDeviceRequest] =
await userIdentity.requestVerification(
this.supportedVerificationMethods?.map(verificationMethodIdentifierToMethod),
this._supportedVerificationMethods.map(verificationMethodIdentifierToMethod),
);
await this.outgoingRequestProcessor.makeOutgoingRequest(outgoingRequest);
return new RustVerificationRequest(request, this.outgoingRequestProcessor, this.supportedVerificationMethods);
return new RustVerificationRequest(request, this.outgoingRequestProcessor, this._supportedVerificationMethods);
}

/**
Expand All @@ -634,10 +644,10 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv

const [request, outgoingRequest]: [RustSdkCryptoJs.VerificationRequest, RustSdkCryptoJs.ToDeviceRequest] =
await device.requestVerification(
this.supportedVerificationMethods?.map(verificationMethodIdentifierToMethod),
this._supportedVerificationMethods.map(verificationMethodIdentifierToMethod),
);
await this.outgoingRequestProcessor.makeOutgoingRequest(outgoingRequest);
return new RustVerificationRequest(request, this.outgoingRequestProcessor, this.supportedVerificationMethods);
return new RustVerificationRequest(request, this.outgoingRequestProcessor, this._supportedVerificationMethods);
}

///////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Expand Down Expand Up @@ -791,7 +801,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
if (request) {
this.emit(
CryptoEvent.VerificationRequestReceived,
new RustVerificationRequest(request, this.outgoingRequestProcessor, this.supportedVerificationMethods),
new RustVerificationRequest(request, this.outgoingRequestProcessor, this._supportedVerificationMethods),
);
}
}
Expand Down
Loading