From 5c6dd6211cb7a44d91f4cebb56a7328d22462f98 Mon Sep 17 00:00:00 2001 From: Lucas Leblow Date: Fri, 8 Dec 2023 12:08:12 -0700 Subject: [PATCH] Fixing user registration concurrency issues --- .../connections-manager.service.ts | 19 +++- .../nest/registration/registration.service.ts | 90 +++++++++++++------ .../registerCertificate.saga.ts | 24 ++++- .../savedOwnerCertificate.saga.ts | 23 +---- packages/types/src/community.ts | 4 +- 5 files changed, 103 insertions(+), 57 deletions(-) diff --git a/packages/backend/src/nest/connections-manager/connections-manager.service.ts b/packages/backend/src/nest/connections-manager/connections-manager.service.ts index 6a321327c0..abcdbb9e4b 100644 --- a/packages/backend/src/nest/connections-manager/connections-manager.service.ts +++ b/packages/backend/src/nest/connections-manager/connections-manager.service.ts @@ -318,11 +318,25 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI await this.launchCommunity(payload) this.logger(`Created and launched community ${payload.id}`) this.serverIoProvider.io.emit(SocketActionTypes.NEW_COMMUNITY, { id: payload.id }) + + if (!payload.ownerCsr || !payload.permsData) { + throw new Error("Owner CSR and PermsData required to create community!") + } + // In launchCommunity, we initialize the StorageService and also + // the RegistrationService for the first time. And then now, once + // those are ready, we register the owner's certificate. + const cert = await this.registrationService.registerOwnerCertificate({ communityId: payload.id, userCsr: payload.ownerCsr, permsData: payload.permsData }) + if (payload.certs) { + // Hacking, perhaps make certs.certificate optional + payload.certs.certificate = cert || '' + } + await this.localDbService.put(LocalDBKeys.COMMUNITY, payload) } public async launchCommunity(payload: InitCommunityPayload) { this.logger('Launching community: peers:', payload.peers) this.communityState = ServiceState.LAUNCHING + // Perhaps we should call this data something else, since we already have a Community type. // It seems like InitCommunityPayload is a mix of various connection metadata. const communityData: InitCommunityPayload = await this.localDbService.get(LocalDBKeys.COMMUNITY) @@ -424,6 +438,7 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI this.serverIoProvider.io.emit(SocketActionTypes.PEER_DISCONNECTED, payload) }) await this.storageService.init(_peerId) + await this.registrationService.init(this.storageService) this.logger('storage initialized') } @@ -444,10 +459,6 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI this.registrationService.on(RegistrationEvents.ERROR, payload => { emitError(this.serverIoProvider.io, payload) }) - this.registrationService.on(RegistrationEvents.NEW_USER, async payload => { - await this.storageService?.saveCertificate(payload) - }) - this.registrationService.on(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, payload => { if (payload.id) { this.storageService.resolveCsrReplicatedPromise(payload.id) diff --git a/packages/backend/src/nest/registration/registration.service.ts b/packages/backend/src/nest/registration/registration.service.ts index d245239755..fbc1b3c76b 100644 --- a/packages/backend/src/nest/registration/registration.service.ts +++ b/packages/backend/src/nest/registration/registration.service.ts @@ -3,6 +3,7 @@ import { EventEmitter } from 'events' import { extractPendingCsrs, issueCertificate } from './registration.functions' import { ErrorCodes, ErrorMessages, PermsData, RegisterOwnerCertificatePayload, SocketActionTypes } from '@quiet/types' import { RegistrationEvents } from './registration.types' +import { StorageService } from '../storage/storage.service' import Logger from '../common/logger' @Injectable() @@ -10,6 +11,7 @@ export class RegistrationService extends EventEmitter implements OnModuleInit { private readonly logger = Logger(RegistrationService.name) public certificates: string[] = [] private _permsData: PermsData + private storageService: StorageService constructor() { super() @@ -19,31 +21,13 @@ export class RegistrationService extends EventEmitter implements OnModuleInit { this.on( RegistrationEvents.REGISTER_USER_CERTIFICATE, async (payload: { csrs: string[]; certificates: string[]; id: string }) => { - await this.issueCertificates(payload) + await this.registerUserCertificates({ csrs: payload.csrs, id: payload.id }) } ) } - private async issueCertificates(payload: { csrs: string[]; certificates: string[]; id?: string }) { - // Lack of permsData means that we are not the owner of the - // community in the official model of the app, however anyone can - // modify the source code, put malicious permsData here, issue - // false certificates and try to trick other users. To prevent - // that, peers verify that anything that is written to the - // certificate store is signed by the owner. - if (!this._permsData) { - if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id }) - return - } - const pendingCsrs = await extractPendingCsrs(payload) - - await Promise.all( - pendingCsrs.map(async csr => { - await this.registerUserCertificate(csr) - }) - ) - - if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id }) + public init(storageService: StorageService) { + this.storageService = storageService } public set permsData(perms: PermsData) { @@ -53,15 +37,26 @@ export class RegistrationService extends EventEmitter implements OnModuleInit { } } - public async registerOwnerCertificate(payload: RegisterOwnerCertificatePayload): Promise { + public async registerOwnerCertificate(payload: RegisterOwnerCertificatePayload): Promise { + if (!this.storageService) { + throw new Error("Storage Service must be initialized before the Registration Service") + } + // FIXME: We should resolve problems with events order and we should set permsData only on LAUNCH_REGISTRART socket event in connectionsManager. this._permsData = payload.permsData const result = await issueCertificate(payload.userCsr.userCsr, this._permsData) if (result?.cert) { + await this.storageService.certificatesStore.addCertificate(result.cert) + // Not sure if this is necessary + const certs = await this.storageService.certificatesStore.loadAllCertificates() + if (!certs.includes(result.cert)) { + throw new Error("Cert wasn't added to CertificateStore correctly") + } this.emit(SocketActionTypes.SAVED_OWNER_CERTIFICATE, { communityId: payload.communityId, network: { certificate: result.cert }, }) + return result?.cert } else { this.emit(SocketActionTypes.ERROR, { type: SocketActionTypes.REGISTRAR, @@ -72,10 +67,53 @@ export class RegistrationService extends EventEmitter implements OnModuleInit { } } - public async registerUserCertificate(csr: string): Promise { - const result = await issueCertificate(csr, this._permsData) - if (result?.cert) { - this.emit(RegistrationEvents.NEW_USER, { certificate: result.cert }) + // Apparently, JS will run each function to completion. So assuming + // we do not have multiple threads, this function should run to + // completion before it is called again. Because we take the CSRs + // and then get the latest view of certificates, filter CSRs and + // then update certificates in a single function, we should never + // issue a CSR that is contained in the certificates list, at least + // in this function... as long as there is no other code that + // updates the certificates list. Something else to consider is that + // because this function is called asynchronously, there may be + // several invocations with different CSRs and they may run in an + // unexpected order. We could address that if it's an issue, but I + // think that might only affect the order of CSR registration. + public async registerUserCertificates(payload: { csrs: string[]; id?: string }) { + if (!this.storageService) { + throw new Error("Storage Service must be initialized before the Registration Service") } + + // Lack of permsData means that we are not the owner of the + // community in the official model of the app, however anyone can + // modify the source code, put malicious permsData here, issue + // false certificates and try to trick other users. To prevent + // that, peers verify that anything that is written to the + // certificate store is signed by the owner. + if (!this._permsData) { + if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id }) + return + } + + const certificates = await this.storageService.certificatesStore.loadAllCertificates() + + const pendingCsrs = await extractPendingCsrs({ csrs: payload.csrs, certificates: certificates as string[] }) + + await Promise.all( + pendingCsrs.map(async csr => { + const result = await issueCertificate(csr, this._permsData) + if (result?.cert) { + await this.storageService.certificatesStore.addCertificate(result.cert) + // Not sure if this is necessary + const certs = await this.storageService.certificatesStore.loadAllCertificates() + if (!certs.includes(result.cert)) { + throw new Error("Cert wasn't added to CertificateStore correctly") + } + this.emit(RegistrationEvents.NEW_USER, { certificate: result.cert }) + } + }) + ) + + if (payload.id) this.emit(RegistrationEvents.FINISHED_ISSUING_CERTIFICATES_FOR_ID, { id: payload.id }) } } diff --git a/packages/state-manager/src/sagas/identity/registerCertificate/registerCertificate.saga.ts b/packages/state-manager/src/sagas/identity/registerCertificate/registerCertificate.saga.ts index aef5910547..a83bd1c186 100644 --- a/packages/state-manager/src/sagas/identity/registerCertificate/registerCertificate.saga.ts +++ b/packages/state-manager/src/sagas/identity/registerCertificate/registerCertificate.saga.ts @@ -2,11 +2,13 @@ import { applyEmitParams, type Socket } from '../../../types' import { type PayloadAction } from '@reduxjs/toolkit' import { apply, select, put } from 'typed-redux-saga' import { communitiesSelectors } from '../../communities/communities.selectors' +import { identitySelectors } from '../../identity/identity.selectors' import { identityActions } from '../identity.slice' import { type RegisterOwnerCertificatePayload, type RegisterUserCertificatePayload, SocketActionTypes, + type InitCommunityPayload, } from '@quiet/types' import { communitiesActions } from '../../communities/communities.slice' @@ -22,16 +24,30 @@ export function* registerCertificateSaga( } if (currentCommunity.CA?.rootCertString) { - const payload: RegisterOwnerCertificatePayload = { - communityId: action.payload.communityId, - userCsr: action.payload.userCsr, + const identity = yield* select(identitySelectors.selectById(currentCommunity.id)) + if (!identity?.userCsr || !currentCommunity?.rootCa) { + console.error("User CSR or root cert missing", identity?.userCsr, currentCommunity?.rootCa) + return + } + + const payload: InitCommunityPayload = { + id: currentCommunity.id, + peerId: identity.peerId, + hiddenService: identity.hiddenService, + certs: { + // Hacking, perhaps make certs.certificate optional + certificate: identity.userCertificate || '', + key: identity.userCsr.userKey, + CA: [currentCommunity.rootCa], + }, + ownerCsr: identity.userCsr, permsData: { certificate: currentCommunity.CA.rootCertString, privKey: currentCommunity.CA.rootKeyString, }, } - yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.REGISTER_OWNER_CERTIFICATE, payload)) + yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.CREATE_COMMUNITY, payload)) } else { if (!isUsernameTaken) { yield* put(communitiesActions.launchCommunity(action.payload.communityId)) diff --git a/packages/state-manager/src/sagas/identity/savedOwnerCertificate/savedOwnerCertificate.saga.ts b/packages/state-manager/src/sagas/identity/savedOwnerCertificate/savedOwnerCertificate.saga.ts index 5631401ee2..16c612d24b 100644 --- a/packages/state-manager/src/sagas/identity/savedOwnerCertificate/savedOwnerCertificate.saga.ts +++ b/packages/state-manager/src/sagas/identity/savedOwnerCertificate/savedOwnerCertificate.saga.ts @@ -10,26 +10,5 @@ export function* savedOwnerCertificateSaga( socket: Socket, action: PayloadAction['payload']> ): Generator { - let communityId: string = action.payload - - if (!communityId) { - communityId = yield* select(communitiesSelectors.currentCommunityId) - } - - const community = yield* select(communitiesSelectors.selectById(communityId)) - const identity = yield* select(identitySelectors.selectById(communityId)) - if (!identity?.userCertificate || !identity?.userCsr || !community?.rootCa) return - - const payload: InitCommunityPayload = { - id: communityId, - peerId: identity.peerId, - hiddenService: identity.hiddenService, - certs: { - certificate: identity.userCertificate, - key: identity.userCsr.userKey, - CA: [community.rootCa], - }, - } - - yield* apply(socket, socket.emit, applyEmitParams(SocketActionTypes.CREATE_COMMUNITY, payload)) + // TODO: Remove? } diff --git a/packages/types/src/community.ts b/packages/types/src/community.ts index fbf3017c58..b3fee2ce97 100644 --- a/packages/types/src/community.ts +++ b/packages/types/src/community.ts @@ -1,4 +1,4 @@ -import { type HiddenService, type PeerId, type Identity } from './identity' +import { type HiddenService, type PeerId, type PermsData, type Identity, type UserCsr } from './identity' import { InvitationPair } from './network' export interface Community { @@ -59,6 +59,8 @@ export interface InitCommunityPayload { hiddenService: HiddenService certs?: Certificates peers?: string[] + ownerCsr?: UserCsr + permsData?: PermsData } export interface LaunchRegistrarPayload {