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

crypto: Replace cryptoMode with DeviceIsolationMode concept #4429

Merged
merged 6 commits into from
Sep 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 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
60 changes: 35 additions & 25 deletions spec/integ/crypto/crypto.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,11 +82,13 @@ import { SecretStorageKeyDescription } from "../../../src/secret-storage";
import {
CrossSigningKey,
CryptoCallbacks,
CryptoMode,
DecryptionFailureCode,
DeviceIsolationMode,
EventShieldColour,
EventShieldReason,
KeyBackupInfo,
NoIsolation,
OnlySignedIsolation,
} from "../../../src/crypto-api";
import { E2EKeyResponder } from "../../test-utils/E2EKeyResponder";
import { IKeyBackup } from "../../../src/crypto/backup";
Expand Down Expand Up @@ -747,9 +749,34 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
);
});

newBackendOnly(
"fails with an error when cross-signed sender is required but sender is not cross-signed",
async () => {
describe("IsolationMode decryption tests", () => {
newBackendOnly(
"OnlySigned mode - fails with an error when cross-signed sender is required but sender is not cross-signed",
async () => {
const decryptedEvent = await setUpTestAndDecrypt(new OnlySignedIsolation());

// It will error as an unknown device because we haven't fetched
// the sender's device keys.
expect(decryptedEvent.isDecryptionFailure()).toBe(true);
expect(decryptedEvent.decryptionFailureReason).toEqual(DecryptionFailureCode.UNKNOWN_SENDER_DEVICE);
},
);

newBackendOnly(
"NoIsolation mode - Decrypts with warning when cross-signed sender is required but sender is not cross-signed",
async () => {
const decryptedEvent = await setUpTestAndDecrypt(new NoIsolation(false));

expect(decryptedEvent.isDecryptionFailure()).toBe(false);

expect(await aliceClient.getCrypto()!.getEncryptionInfoForEvent(decryptedEvent)).toEqual({
shieldColour: EventShieldColour.RED,
shieldReason: EventShieldReason.UNKNOWN_DEVICE,
});
},
);

async function setUpTestAndDecrypt(isolationMode: DeviceIsolationMode): Promise<MatrixEvent> {
// This tests that a message will not be decrypted if the sender
// is not sufficiently trusted according to the selected crypto
// mode.
Expand All @@ -760,7 +787,7 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });

// Start by using Invisible crypto mode
aliceClient.getCrypto()!.setCryptoMode(CryptoMode.Invisible);
aliceClient.getCrypto()!.setDeviceIsolationMode(isolationMode);

await startClientAndAwaitFirstSync();

Expand Down Expand Up @@ -807,26 +834,9 @@ describe.each(Object.entries(CRYPTO_BACKENDS))("crypto (%s)", (backend: string,
expect(event.isEncrypted()).toBe(true);

// it probably won't be decrypted yet, because it takes a while to process the olm keys
const decryptedEvent = await testUtils.awaitDecryption(event);
// It will error as an unknown device because we haven't fetched
// the sender's device keys.
expect(decryptedEvent.decryptionFailureReason).toEqual(DecryptionFailureCode.UNKNOWN_SENDER_DEVICE);

// Next, try decrypting in transition mode, which should also
// fail for the same reason
aliceClient.getCrypto()!.setCryptoMode(CryptoMode.Transition);

await event.attemptDecryption(aliceClient["cryptoBackend"]!);
expect(decryptedEvent.decryptionFailureReason).toEqual(DecryptionFailureCode.UNKNOWN_SENDER_DEVICE);

// Decrypting in legacy mode should succeed since it doesn't
// care about device trust.
aliceClient.getCrypto()!.setCryptoMode(CryptoMode.Legacy);

await event.attemptDecryption(aliceClient["cryptoBackend"]!);
expect(decryptedEvent.decryptionFailureReason).toEqual(null);
},
);
return await testUtils.awaitDecryption(event);
}
});

it("Decryption fails with Unable to decrypt for other errors", async () => {
expectAliceKeyQuery({ device_keys: { "@alice:localhost": {} }, failures: {} });
Expand Down
89 changes: 63 additions & 26 deletions src/crypto-api/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,11 @@ export interface CryptoApi {
globalBlacklistUnverifiedDevices: boolean;

/**
* The cryptography mode to use.
* The {@link DeviceIsolationMode} mode to use.
*
* @see CryptoMode
* @see DeviceIsolationMode
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
*/
setCryptoMode(cryptoMode: CryptoMode): void;
setDeviceIsolationMode(isolationMode: DeviceIsolationMode): void;

/**
* Return the current version of the crypto module.
Expand Down Expand Up @@ -657,38 +657,75 @@ export enum DecryptionFailureCode {
UNKNOWN_ENCRYPTION_ALGORITHM = "UNKNOWN_ENCRYPTION_ALGORITHM",
}

/** Enum kind for isolation mode, used to discriminate union.*/
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
export enum IsolationModeKind {
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
None,
OnlySigned,
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* The cryptography mode. Affects how messages are encrypted and decrypted.
* A type of device isolation mode used when encrypting or decrypting messages.
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
* Only supported by Rust crypto.
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
*
* Message encryption keys are shared with all devices in the room, except in case of
* verified user problems (see {@link errorOnVerifiedUserProblems}).
*
* Events from all senders are always decrypted (and should be decorated with message shields in case
* of authenticity warnings, see {@link EventEncryptionInfo}).
*/
export enum CryptoMode {
/**
* Message encryption keys are shared with all devices in the room, except for
* blacklisted devices, or unverified devices if
* `globalBlacklistUnverifiedDevices` is set. Events from all senders are
* decrypted.
*/
Legacy,
export class NoIsolation {
// Discriminated Union
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
public readonly kind: IsolationModeKind.None;
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved

/**
* Events are encrypted as with `Legacy` mode, but encryption will throw an error if a
* verified user has an unsigned device, or if a verified user replaces
* their identity. Events are decrypted only if they come from cross-signed
* devices, or devices that existed before the Rust crypto SDK started
* tracking device trust: other events will result in a decryption failure. (To access the failure
* reason, see {@link MatrixEvent.decryptionFailureReason}.)
* Optional behavior when sharing keys to remote devices.
* If set to true, sharing keys will fail (i.e. message sending will fail) with an error if:
* - The user was previously verified but is not anymore.
* - A verified user has some unverified devices (not cross-signed).
* If false, the keys will be distributed as usual.
* If set to false the client UX should display warnings to inform the user.
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
*/
Transition,
public errorOnVerifiedUserProblems: boolean;
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 option is not used yet in this PR. It is used for encrypting. Added here to showcase that IsolationMode is not just an enum but need more information

BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved

/**
* Message encryption keys are only shared with devices that have been cross-signed by their owner.
* Encryption will throw an error if a verified user replaces their identity. Events are
* decrypted only if they come from a cross-signed device other events will result in a decryption
* failure. (To access the failure reason, see {@link MatrixEvent.decryptionFailureReason}.)
*/
Invisible,
public constructor(errorOnVerifiedUserProblems: boolean) {
this.kind = IsolationModeKind.None;
this.errorOnVerifiedUserProblems = errorOnVerifiedUserProblems;
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
}
}

/**
* A type of device isolation mode used when encrypting or decrypting messages.
* Only supported by Rust crypto.
*
* Message encryption keys are only shared with devices that have been cross-signed by their owner.
* Encryption will throw an error if a verified user replaces their identity.
*
* Events are decrypted only if they come from a cross-signed device other events will result in a decryption
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
* failure. (To access the failure reason, see {@link MatrixEvent.decryptionFailureReason}.)
*/
export class OnlySignedIsolation {
Copy link
Member

Choose a reason for hiding this comment

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

WDYT about calling this OnlySignedDeviceIsolationMode ? I know it's a bit of a long name but I feel it gives a better indication of what it does.

(And... NoIsolationDeviceIsolationMode ?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I find the name very long and strange (NoIsolationIsolationMode).
What about namespaces? First time I see that, but would allow to do new DeviceIsolationMode.OnlySigned() so it's a clear indication that these are DeviceIsolationMode without the need of bigger name, also easy to discover?

export namespace DeviceIsolationMode {
    export class None {..}
    export class OnlySigned {..}
}

Copy link
Member

Choose a reason for hiding this comment

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

Per my monologue in #element-dev:matrix.org, I'm not enthusiastic about namespaces, because they seem to be deprecated (and we have a lint rule that enforces that we don't use them).

Copy link
Member

@richvdh richvdh Sep 25, 2024

Choose a reason for hiding this comment

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

Honestly, I don't find NoIsolationIsolationMode that bad, but I agree it's hardly elegant

Copy link
Contributor

Choose a reason for hiding this comment

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

OnlySignedDevicesIsolationMode and AllDevicesIsolationMode ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Renamed as per andy suggestion, also renamed the DeviceIsolationModeKind enum to still match classes names 20f5882

// Discriminated Union
public readonly kind: IsolationModeKind.OnlySigned;

public constructor() {
this.kind = IsolationModeKind.OnlySigned;
}
}

/**
* DeviceIsolationMode represents the mode of device isolation used when encrypting or decrypting messages.
* It can be one of two types: NoIsolation or OnlySignedIsolation.
*
* {@link NoIsolation}: In this mode, message encryption keys are shared with all devices in the room,
* except for blacklisted devices or unverified devices if certain conditions are met.
* Events from all senders are always decrypted.
*
* {@link OnlySignedIsolation}: In this mode, message encryption keys are only shared with devices
* that have been cross-signed by their owner. Events will be decrypted only if they come from
* a cross-signed device, other events will result in a decryption failure.
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
*/
export type DeviceIsolationMode = NoIsolation | OnlySignedIsolation;

/**
* Options object for `CryptoApi.bootstrapCrossSigning`.
*/
Expand Down
7 changes: 3 additions & 4 deletions src/crypto/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,9 +88,9 @@ import {
BootstrapCrossSigningOpts,
CrossSigningKeyInfo,
CrossSigningStatus,
CryptoMode,
decodeRecoveryKey,
DecryptionFailureCode,
DeviceIsolationMode,
DeviceVerificationStatus,
encodeRecoveryKey,
EventEncryptionInfo,
Expand Down Expand Up @@ -650,12 +650,11 @@ export class Crypto extends TypedEventEmitter<CryptoEvent, CryptoEventHandlerMap
}

/**
* Implementation of {@link Crypto.CryptoApi#setCryptoMode}.
* Implementation of {@link Crypto.CryptoApi#setDeviceIsolationMode}.
*/
public setCryptoMode(cryptoMode: CryptoMode): void {
public setDeviceIsolationMode(isolationMode: DeviceIsolationMode): void {
throw new Error("Not supported");
}

/**
* Implementation of {@link Crypto.CryptoApi#getVersion}.
*/
Expand Down
29 changes: 16 additions & 13 deletions src/rust-crypto/rust-crypto.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@ import {
CrossSigningStatus,
CryptoApi,
CryptoCallbacks,
CryptoMode,
Curve25519AuthData,
DecryptionFailureCode,
DeviceVerificationStatus,
Expand All @@ -61,6 +60,9 @@ import {
VerificationRequest,
encodeRecoveryKey,
deriveRecoveryKeyFromPassphrase,
DeviceIsolationMode,
NoIsolation,
IsolationModeKind,
} from "../crypto-api/index.ts";
import { deviceKeysToDeviceMap, rustDeviceToJsDevice } from "./device-converter.ts";
import { IDownloadKeyResult, IQueryKeysRequest } from "../client.ts";
Expand Down Expand Up @@ -107,7 +109,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
private readonly RECOVERY_KEY_DERIVATION_ITERATIONS = 500000;

private _trustCrossSignedDevices = true;
private cryptoMode = CryptoMode.Legacy;
private deviceIsolationMode: DeviceIsolationMode = new NoIsolation(false);

/** whether {@link stop} has been called */
private stopped = false;
Expand Down Expand Up @@ -259,7 +261,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
// through decryptEvent and hence get rid of this case.
throw new Error("to-device event was not decrypted in preprocessToDeviceMessages");
}
return await this.eventDecryptor.attemptEventDecryption(event, this.cryptoMode);
return await this.eventDecryptor.attemptEventDecryption(event, this.deviceIsolationMode);
}

/**
Expand Down Expand Up @@ -370,10 +372,10 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
}

/**
* Implementation of {@link Crypto.CryptoApi#setCryptoMode}.
* Implementation of {@link CryptoApi#setDeviceIsolationMode}.
*/
public setCryptoMode(cryptoMode: CryptoMode): void {
this.cryptoMode = cryptoMode;
public setDeviceIsolationMode(isolationMode: DeviceIsolationMode): void {
this.deviceIsolationMode = isolationMode;
}

/**
Expand Down Expand Up @@ -1754,24 +1756,25 @@ class EventDecryptor {
private readonly perSessionBackupDownloader: PerSessionKeyBackupDownloader,
) {}

public async attemptEventDecryption(event: MatrixEvent, cryptoMode: CryptoMode): Promise<IEventDecryptionResult> {
public async attemptEventDecryption(
event: MatrixEvent,
isolationMode: DeviceIsolationMode,
): Promise<IEventDecryptionResult> {
// add the event to the pending list *before* attempting to decrypt.
// then, if the key turns up while decryption is in progress (and
// decryption fails), we will schedule a retry.
// (fixes https://github.com/vector-im/element-web/issues/5001)
this.addEventToPendingList(event);

let trustRequirement;
switch (cryptoMode) {
case CryptoMode.Legacy:

switch (isolationMode.kind) {
case IsolationModeKind.None:
trustRequirement = RustSdkCryptoJs.TrustRequirement.Untrusted;
break;
case CryptoMode.Transition:
case IsolationModeKind.OnlySigned:
trustRequirement = RustSdkCryptoJs.TrustRequirement.CrossSignedOrLegacy;
break;
case CryptoMode.Invisible:
trustRequirement = RustSdkCryptoJs.TrustRequirement.CrossSigned;
break;
}

try {
Expand Down