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.

Sign up for GitHub

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

crypto: Replace cryptoMode with DeviceIsolationMode concept #4429

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ab23354
crypto: Replace cryptoMode with DeviceIsolationMode concept
BillCarsonFr Sep 24, 2024
a7e16e8
use enum instead of string for the IsolationMode kind
BillCarsonFr Sep 24, 2024
1fe8ebc
Code review - Cleaning, renaming
BillCarsonFr Sep 24, 2024
df36fac
review: unneeded @see in doc
BillCarsonFr Sep 25, 2024
20f5882
review: Rename IsolationMode with better names
BillCarsonFr Sep 25, 2024
2a5df5d
review: quick cleaning and doc
BillCarsonFr Sep 25, 2024
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
57 changes: 21 additions & 36 deletions src/crypto-api/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,6 @@ export interface CryptoApi {

/**
* The {@link DeviceIsolationMode} mode to use.
*
* @see DeviceIsolationMode
*/
setDeviceIsolationMode(isolationMode: DeviceIsolationMode): void;

Expand Down Expand Up @@ -657,15 +655,14 @@ export enum DecryptionFailureCode {
UNKNOWN_ENCRYPTION_ALGORITHM = "UNKNOWN_ENCRYPTION_ALGORITHM",
}

/** Enum kind for isolation mode, used to discriminate union.*/
export enum IsolationModeKind {
None,
OnlySigned,
/** Base {@link DeviceIsolationMode} kind. */
export enum DeviceIsolationModeKind {
NoIsolation,
OnlySignedIsolation,
}

/**
* A type of device isolation mode used when encrypting or decrypting messages.
* Only supported by Rust crypto.
* A type of {@link DeviceIsolationMode}.
*
* Message encryption keys are shared with all devices in the room, except in case of
* verified user problems (see {@link errorOnVerifiedUserProblems}).
Expand All @@ -674,55 +671,43 @@ export enum IsolationModeKind {
* of authenticity warnings, see {@link EventEncryptionInfo}).
*/
export class NoIsolation {
// Discriminated Union
public readonly kind: IsolationModeKind.None;
public readonly kind = DeviceIsolationModeKind.NoIsolation;

/**
* 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.
*
* @param errorOnVerifiedUserProblems - 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:
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
* - The user was previously verified but is not anymore, or:
* - A verified user has some unverified devices (not cross-signed).
*
* If `false`, the keys will be distributed as usual. In this case, the client UX should display
* warnings to inform the user about problematic devices/users, and stop them hitting this case.
*/
public errorOnVerifiedUserProblems: boolean;

public constructor(errorOnVerifiedUserProblems: boolean) {
this.kind = IsolationModeKind.None;
public constructor(public readonly errorOnVerifiedUserProblems: boolean) {
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.
* A type of {@link DeviceIsolationMode}.
*
* 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
* 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}.)
*/
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 •
edited
Loading

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
Member

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 readonly kind = DeviceIsolationModeKind.OnlySignedIsolation;

public constructor() {
this.kind = IsolationModeKind.OnlySigned;
}
public constructor() {}
BillCarsonFr marked this conversation as resolved.
Show resolved Hide resolved
}

/**
* 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.
* It can be one of two types: {@link NoIsolation} or {@link OnlySignedIsolation}.
*
* {@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.
* Only supported by rust Crypto.
*/
export type DeviceIsolationMode = NoIsolation | OnlySignedIsolation;

Expand Down
6 changes: 3 additions & 3 deletions src/rust-crypto/rust-crypto.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ import {
deriveRecoveryKeyFromPassphrase,
DeviceIsolationMode,
NoIsolation,
IsolationModeKind,
DeviceIsolationModeKind,
} from "../crypto-api/index.ts";
import { deviceKeysToDeviceMap, rustDeviceToJsDevice } from "./device-converter.ts";
import { IDownloadKeyResult, IQueryKeysRequest } from "../client.ts";
Expand Down Expand Up @@ -1769,10 +1769,10 @@ class EventDecryptor {
let trustRequirement;

switch (isolationMode.kind) {
case IsolationModeKind.None:
case DeviceIsolationModeKind.NoIsolation:
trustRequirement = RustSdkCryptoJs.TrustRequirement.Untrusted;
break;
case IsolationModeKind.OnlySigned:
case DeviceIsolationModeKind.OnlySignedIsolation:
trustRequirement = RustSdkCryptoJs.TrustRequirement.CrossSignedOrLegacy;
break;
}
Expand Down