-
Notifications
You must be signed in to change notification settings - Fork 73
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
Encryption improvements #285
Conversation
df23a79
to
da5f57b
Compare
f745f5b
to
edd7668
Compare
@@ -705,12 +706,16 @@ export class Bridge { | |||
const now = Date.now(); | |||
for (const [key, entry] of this.intents.entries()) { | |||
// Do not delete intents that sync. | |||
if (this.eeEventBroker?.shouldAvoidCull(entry.intent)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the previous version of the code, we ended up never culling any Intents because of an erronous return
statement (I'm good at those).
This change means that Intent jobs are cleaned up if they aren't used after INTENT_CULL_EVICT_AFTER_MS and the eeEventBroker is instantiated AND needs the user kept alive (e.g. they are the transport for encrypted messages in a room)
@@ -18,10 +20,12 @@ export interface ClientEncryptionSession { | |||
userId: string; | |||
deviceId: string; | |||
accessToken: string; | |||
syncToken: string|null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now store the sync token to avoid doing an initial sync on startup, which was causing unbounded amounts of memory to be used.
@@ -107,7 +112,7 @@ export class EncryptedEventBroker { | |||
// We should probably make these LRUs eventually | |||
private eventsPendingAS: WeakEvent[] = []; | |||
|
|||
private syncingClients = new Set<any>(); | |||
private syncingClients = new Map<string, any>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We now store the userId => client. The client from matrix-js-sdk doesn't have a type :(
@@ -142,6 +147,8 @@ export class EncryptedEventBroker { | |||
const existingUserForRoom = this.userForRoom.get(event.room_id); | |||
if (existingUserForRoom) { | |||
log.debug(`${existingUserForRoom} is listening for ${event.event_id}`); | |||
// XXX: Sometimes the sync stops working, calling this will wake it up. | |||
await this.startSyncingUser(existingUserForRoom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unfortunate but sometimes even though a user is syncing for messages, they stop working. Calling this function checks that the user is syncing.
@@ -170,15 +177,18 @@ export class EncryptedEventBroker { | |||
|
|||
const existingUser = membersForRoom.find((u) => [...this.userForRoom.values()].includes(u)); | |||
if (existingUser) { | |||
log.error(`${event.room_id} will be synced by ${existingUser}`); | |||
log.debug(`${event.room_id} will be synced by ${existingUser}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less noise
@@ -261,20 +271,36 @@ export class EncryptedEventBroker { | |||
this.onEphemeralEvent(presenceEv); | |||
} | |||
|
|||
private async onSync(userId: string, state: string, data: {nextSyncToken: string; catchingUp: boolean;}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Update the stored sync token on insert
/** | ||
* Start a sync loop for a given bridge user | ||
* @param userId The user whos matrix client should start syncing | ||
*/ | ||
public async startSyncingUser(userId: string) { | ||
log.info(`Starting to sync ${userId}`); | ||
public async startSyncingUser(userId: string): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function was refactored to:
- Check if a client is already syncing
- Check if a client has stopped syncing and needs restarting
// We *need* to sync before we can send a message. | ||
await this.ensureRegistered(); | ||
await this.encryption.ensureClientSyncingCallback(); | ||
let encrypted = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We previously started syncing any client that sent a message to a room, but that's inefficent if the room is unencrypted as we do not need to sync.
This code checks to see if a room is encrypted.
src/components/intent.ts
Outdated
@@ -767,6 +803,9 @@ export class Intent { | |||
else if (event.type === "m.room.power_levels") { | |||
this._powerLevels[event.room_id] = event.content as unknown as PowerLevelContent; | |||
} | |||
else if (event.type === "m.room.encryption") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure we catch any rooms that get encryption turned on :)
@@ -380,11 +380,11 @@ export class PrometheusMetrics { | |||
// TODO: Ideally these metrics would be on a different port. | |||
// For now, leave this unauthenticated. | |||
checkToken: false, | |||
handler: (_req: Request, res: Response) => { | |||
handler: async (_req: Request, res: Response) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This appears in #296 but I needed it to work for my encrypted bridge deployment #sorry
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to be sorry. Great work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blindly trusting ev.algorithm to be a string is a bit to trusting from my perspective.
Everything else looks good.
@@ -380,11 +380,11 @@ export class PrometheusMetrics { | |||
// TODO: Ideally these metrics would be on a different port. | |||
// For now, leave this unauthenticated. | |||
checkToken: false, | |||
handler: (_req: Request, res: Response) => { | |||
handler: async (_req: Request, res: Response) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to be sorry. Great work!
Co-authored-by: Christian Paul <christianp@matrix.org>
Fixes #284
Depends on #295
Sometimes we get into a state where the bridge isn't reliably syncing messages from client's and so ends up in a broken state, this aims to fix that.