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 crypto mode setting for invisible crypto, and apply it to decrypting events #4407

Merged
merged 8 commits into from
Sep 18, 2024

Conversation

uhoreg
Copy link
Member

@uhoreg uhoreg commented Sep 16, 2024

Adds a global "crypto mode" setting to the crypto API (only works with Rust crypto), and changes the decryption settings based on that.

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • Sign-off given on the changes (see CONTRIBUTING.md).

@uhoreg uhoreg added the T-Feature Request to add a new feature which does not exist right now label Sep 17, 2024
@uhoreg uhoreg added T-Enhancement and removed T-Feature Request to add a new feature which does not exist right now labels Sep 17, 2024
@uhoreg
Copy link
Member Author

uhoreg commented Sep 17, 2024

This PR only handles the decryption side. Encrypting would be a separate PR, and would look something like

diff --git a/src/rust-crypto/RoomEncryptor.ts b/src/rust-crypto/RoomEncryptor.ts
index fcda295eb..ad07cdc15 100644
--- a/src/rust-crypto/RoomEncryptor.ts
+++ b/src/rust-crypto/RoomEncryptor.ts
@@ -36,6 +36,7 @@ import { HistoryVisibility } from "../@types/partials.ts";
 import { OutgoingRequestsManager } from "./OutgoingRequestsManager.ts";
 import { logDuration } from "../utils.ts";
 import { KnownMembership } from "../@types/membership.ts";
+import { CryptoMode } from "../crypto-api/index.ts";
 
 /**
  * RoomEncryptor: responsible for encrypting messages to a given room
@@ -122,7 +123,10 @@ export class RoomEncryptor {
      *
      * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices
      */
-    public async prepareForEncryption(globalBlacklistUnverifiedDevices: boolean): Promise<void> {
+    public async prepareForEncryption(
+        globalBlacklistUnverifiedDevices: boolean,
+        cryptoMode: CryptoMode,
+    ): Promise<void> {
         // We consider a prepareForEncryption as an encryption promise as it will potentially share keys
         // even if it doesn't send an event.
         // Usually this is called when the user starts typing, so we want to make sure we have keys ready when the
@@ -130,7 +134,7 @@ export class RoomEncryptor {
         // If `encryptEvent` is invoked before `prepareForEncryption` has completed, the `encryptEvent` call will wait for
         // `prepareForEncryption` to complete before executing.
         // The part where `encryptEvent` shares the room key will then usually be a no-op as it was already performed by `prepareForEncryption`.
-        await this.encryptEvent(null, globalBlacklistUnverifiedDevices);
+        await this.encryptEvent(null, globalBlacklistUnverifiedDevices, cryptoMode);
     }
 
     /**
@@ -142,7 +146,11 @@ export class RoomEncryptor {
      * @param event - Event to be encrypted, or null if only preparing for encryption (in which case we will pre-share the room key).
      * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices
      */
-    public encryptEvent(event: MatrixEvent | null, globalBlacklistUnverifiedDevices: boolean): Promise<void> {
+    public encryptEvent(
+        event: MatrixEvent | null,
+        globalBlacklistUnverifiedDevices: boolean,
+        cryptoMode: CryptoMode,
+    ): Promise<void> {
         const logger = new LogSpan(this.prefixedLogger, event ? (event.getTxnId() ?? "") : "prepareForEncryption");
         // Ensure order of encryption to avoid message ordering issues, as the scheduler only ensures
         // events order after they have been encrypted.
@@ -153,7 +161,7 @@ export class RoomEncryptor {
             })
             .then(async () => {
                 await logDuration(logger, "ensureEncryptionSession", async () => {
-                    await this.ensureEncryptionSession(logger, globalBlacklistUnverifiedDevices);
+                    await this.ensureEncryptionSession(logger, globalBlacklistUnverifiedDevices, cryptoMode);
                 });
                 if (event) {
                     await logDuration(logger, "encryptEventInner", async () => {
@@ -175,7 +183,11 @@ export class RoomEncryptor {
      * @param logger - a place to write diagnostics to
      * @param globalBlacklistUnverifiedDevices - When `true`, it will not send encrypted messages to unverified devices
      */
-    private async ensureEncryptionSession(logger: LogSpan, globalBlacklistUnverifiedDevices: boolean): Promise<void> {
+    private async ensureEncryptionSession(
+        logger: LogSpan,
+        globalBlacklistUnverifiedDevices: boolean,
+        cryptoMode: CryptoMode,
+    ): Promise<void> {
         if (this.encryptionSettings.algorithm !== "m.megolm.v1.aes-sha2") {
             throw new Error(
                 `Cannot encrypt in ${this.room.roomId} for unsupported algorithm '${this.encryptionSettings.algorithm}'`,
@@ -251,12 +263,22 @@ export class RoomEncryptor {
             rustEncryptionSettings.rotationPeriodMessages = BigInt(this.encryptionSettings.rotation_period_msgs);
         }
 
-        // When this.room.getBlacklistUnverifiedDevices() === null, the global settings should be used
-        // See Room#getBlacklistUnverifiedDevices
-        if (this.room.getBlacklistUnverifiedDevices() ?? globalBlacklistUnverifiedDevices) {
-            rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(true, false);
-        } else {
-            rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(false, false);
+        switch (cryptoMode) {
+            case CryptoMode.Legacy:
+                // When this.room.getBlacklistUnverifiedDevices() === null, the global settings should be used
+                // See Room#getBlacklistUnverifiedDevices
+                if (this.room.getBlacklistUnverifiedDevices() ?? globalBlacklistUnverifiedDevices) {
+                    rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(true, false);
+                } else {
+                    rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(false, false);
+                }
+                break;
+            case CryptoMode.Transition:
+                // FIXME: is this correct?
+                rustEncryptionSettings.sharingStrategy = CollectStrategy.deviceBasedStrategy(true, true);
+                break;
+            case CryptoMode.Invisible:
+                rustEncryptionSettings.sharingStrategy = CollectStrategy.identityBasedStrategy();
         }
 
         await logDuration(this.prefixedLogger, "shareRoomKey", async () => {
diff --git a/src/rust-crypto/rust-crypto.ts b/src/rust-crypto/rust-crypto.ts
index 4e4bf11e2..4c177f9c0 100644
--- a/src/rust-crypto/rust-crypto.ts
+++ b/src/rust-crypto/rust-crypto.ts
@@ -249,7 +249,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
             throw new Error(`Cannot encrypt event in unconfigured room ${roomId}`);
         }
 
-        await encryptor.encryptEvent(event, this.globalBlacklistUnverifiedDevices);
+        await encryptor.encryptEvent(event, this.globalBlacklistUnverifiedDevices, this.cryptoMode);
     }
 
     public async decryptEvent(event: MatrixEvent): Promise<IEventDecryptionResult> {
@@ -394,7 +394,7 @@ export class RustCrypto extends TypedEventEmitter<RustCryptoEvents, RustCryptoEv
         const encryptor = this.roomEncryptors[room.roomId];
 
         if (encryptor) {
-            encryptor.prepareForEncryption(this.globalBlacklistUnverifiedDevices);
+            encryptor.prepareForEncryption(this.globalBlacklistUnverifiedDevices, this.cryptoMode);
         }
     }
 

(I'm not sure exactly how transition mode would look), plus any needed changes to tests.

@uhoreg
Copy link
Member Author

uhoreg commented Sep 17, 2024

I'm not sure exactly what SonarCloud is complaining about. If I'm reading https://sonarcloud.io/component_measures?metric=new_coverage&selected=matrix-js-sdk%3Asrc%2Frust-crypto%2Frust-crypto.ts&view=list&pullRequest=4407&id=matrix-js-sdk right, it seems to thing that https://github.com/matrix-org/matrix-js-sdk/pull/4407/files#diff-1cdf58640fc7d7c465fab943126da6a0d5c9ab99301676278fdc362beea10c11R1891-R1892 is uncovered, but it clearly is covered, because the test checks for that error and gets it. And it's counting that as 2 lines out of the "8" lines in the PR? AFAICT, it's saying that all the other lines that are added by this PR in that file are covered. And it's saying that it has 2 (out of 7) lines in that file are uncovered.

* The cryptography mode. Affects how messages are encrypted and decrypted.
* Only supported by Rust crypto.
*/
export enum CryptoMode {
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'm not entirely sure if we want to have a single knob that affects both encryption and decryption, or if we want to be able to tweak the encryption and decryption settings separately. Having a single setting is possibly easier to use, but gives less control.

Copy link
Member

Choose a reason for hiding this comment

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

We think this is fine.

@dbkr
Copy link
Member

dbkr commented Sep 18, 2024

I see what you mean about sonarcloud: it seems very confused. Those lines aren't even in a different execution path so it doesn't even make sense. I guess we will have to override it.

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

LGTM other than a few nits

src/rust-crypto/rust-crypto.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Show resolved Hide resolved
src/crypto-api/index.ts Show resolved Hide resolved
src/crypto-api/index.ts Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
src/crypto-api/index.ts Outdated Show resolved Hide resolved
@richvdh richvdh merged commit dbb4828 into matrix-org:develop Sep 18, 2024
22 of 23 checks passed
@BillCarsonFr
Copy link
Member

This PR only handles the decryption side. Encrypting would be a separate PR, and would look something like

Please see here #4425

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants