Skip to content

Commit

Permalink
fix: Various fixes related to peers, CSRs and backend startup
Browse files Browse the repository at this point in the history
Fixes for the following issues:
- Peers can be deleted if CSRs don't sync
- Backend starting before the frontend is ready, resulting in missed events
- Adding duplicate CSRs
  • Loading branch information
leblowl committed Apr 18, 2024
1 parent ce027b2 commit 20fa367
Show file tree
Hide file tree
Showing 30 changed files with 201 additions and 427 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -209,7 +209,7 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
const network = await this.localDbService.getNetworkInfo()

if (community && network) {
const sortedPeers = await this.localDbService.getSortedPeers(community.peerList)
const sortedPeers = await this.localDbService.getSortedPeers(community.peerList ?? [])
this.logger('launchCommunityFromStorage - sorted peers', sortedPeers)
if (sortedPeers.length > 0) {
community.peerList = sortedPeers
Expand Down Expand Up @@ -517,6 +517,7 @@ export class ConnectionsManagerService extends EventEmitter implements OnModuleI
agent: this.socksProxyAgent,
localAddress: this.libp2pService.createLibp2pAddress(onionAddress, peerId.toString()),
targetPort: this.ports.libp2pHiddenService,
// Ignore local address
peers: peers ? peers.slice(1) : [],
psk: Libp2pService.generateLibp2pPSK(community.psk).fullKey,
}
Expand Down
15 changes: 3 additions & 12 deletions packages/backend/src/nest/local-db/local-db.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,21 +95,12 @@ export class LocalDbService {
}
}

// I think we can move this into StorageService (keep this service
// focused on CRUD).
public async getSortedPeers(
peers?: string[] | undefined,
peers: string[],
includeLocalPeerAddress: boolean = true
): Promise<string[]> {
if (!peers) {
const currentCommunity = await this.getCurrentCommunity()
if (!currentCommunity) {
throw new Error('No peers were provided and no community was found to extract peers from')
}
peers = currentCommunity.peerList
if (!peers) {
throw new Error('No peers provided and no peers found on current stored community')
}
}

const peersStats = (await this.get(LocalDBKeys.PEERS)) || {}
const stats: NetworkStats[] = Object.values(peersStats)
const network = await this.getNetworkInfo()
Expand Down
14 changes: 9 additions & 5 deletions packages/backend/src/nest/socket/socket.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -47,36 +47,40 @@ export class SocketService extends EventEmitter implements OnModuleInit {
}

async onModuleInit() {
this.logger('init:started')
this.logger('init: Started')

this.attachListeners()
await this.init()

this.logger('init:finished')
this.logger('init: Finished')
}

public async init() {
const connection = new Promise<void>(resolve => {
this.serverIoProvider.io.on(SocketActionTypes.CONNECTION, socket => {
this.logger('init: connection')
resolve()
socket.on(SocketActionTypes.START, async () => {
resolve()
})
})
})

await this.listen()

this.logger('init: Waiting for frontend to connect')
await connection
this.logger('init: Frontend connected')
}

private readonly attachListeners = (): void => {
// Attach listeners here
this.serverIoProvider.io.on(SocketActionTypes.CONNECTION, socket => {
this.logger('socket connection')
this.logger('Socket connection')

// On websocket connection, update presentation service with network data
this.emit(SocketActionTypes.CONNECTION)

socket.on(SocketActionTypes.CLOSE, async () => {
this.logger('Socket connection closed')
this.emit(SocketActionTypes.CLOSE)
})

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,8 @@ export class CertificatesRequestsStore extends EventEmitter {
this.loadedCertificateRequests()
})

// TODO: Load CSRs in case the owner closes the app before issuing
// certificates
// @ts-ignore
await this.store.load({ fetchEntryTimeout: 15000 })
this.logger('Initialized')
}

Expand Down Expand Up @@ -76,7 +76,7 @@ export class CertificatesRequestsStore extends EventEmitter {
await parsedCsr.verify()
await this.validateCsrFormat(csr)
} catch (err) {
console.error('Failed to validate user csr:', csr, err?.message)
console.error('Failed to validate user CSR:', csr, err?.message)
return false
}
return true
Expand All @@ -99,15 +99,13 @@ export class CertificatesRequestsStore extends EventEmitter {
.map(e => {
return e.payload.value
})
this.logger('Total CSRs:', allEntries.length)

this.logger('DuplicatedCertBug', { allEntries })
const allCsrsUnique = [...new Set(allEntries)]
this.logger('DuplicatedCertBug', { allCsrsUnique })
await Promise.all(
allCsrsUnique
.filter(async csr => {
const validation = await CertificatesRequestsStore.validateUserCsr(csr)
this.logger('DuplicatedCertBug', { validation, csr })
if (validation) return true
return false
})
Expand All @@ -121,8 +119,9 @@ export class CertificatesRequestsStore extends EventEmitter {
filteredCsrsMap.set(pubKey, csr)
})
)
this.logger('DuplicatedCertBug', '[...filteredCsrsMap.values()]', [...filteredCsrsMap.values()])
return [...filteredCsrsMap.values()]
const validCsrs = [...filteredCsrsMap.values()]
this.logger('Valid CSRs:', validCsrs.length)
return validCsrs
}

public clean() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,6 @@ describe('CertificatesStore', () => {

await certificatesStore.addCertificate(certificate)

// @ts-expect-error - getCertificates is protected
const certificates = await certificatesStore.getCertificates()

expect(certificates).toContain(certificate)
Expand All @@ -149,7 +148,6 @@ describe('CertificatesStore', () => {

await certificatesStore.addCertificate(certificate)

// @ts-expect-error - getCertificates is protected
const certificates = await certificatesStore.getCertificates()

expect(certificates).not.toContain(certificate)
Expand All @@ -161,7 +159,6 @@ describe('CertificatesStore', () => {

certificatesStore.updateMetadata(communityMetadata)

// @ts-expect-error - getCertificates is protected
jest.spyOn(certificatesStore, 'getCertificates').mockResolvedValue([certificate1, certificate2])

const certificates = await certificatesStore.loadAllCertificates()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ export class CertificatesStore extends EventEmitter {
* as specified in the comment section of
* https://github.com/TryQuiet/quiet/issues/1899
*/
protected async getCertificates() {
public async getCertificates(): Promise<string[]> {
if (!this.store) {
return []
}
Expand Down Expand Up @@ -190,7 +190,8 @@ export class CertificatesStore extends EventEmitter {

const validCerts = validCertificates.filter(i => i != undefined)
this.logger(`Valid certificates: ${validCerts.length}`)
return validCerts
// TODO: Why doesn't TS infer this properly?
return validCerts as string[]
}

public async getCertificateUsername(pubkey: string) {
Expand Down
2 changes: 1 addition & 1 deletion packages/backend/src/nest/storage/storage.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -519,7 +519,7 @@ describe('StorageService', () => {
'MIIDHjCCAsMCAQAwSTFHMEUGA1UEAxM+NnZ1MmJ4a2k3NzdpdDNjcGF5djZmcTZ2cGw0a2Uza3pqN2d4aWNmeWdtNTVkaGh0cGh5ZmR2eWQub25pb24wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATMpfp2hSfWFL26OZlZKZEWG9fyAM1ndlEzO0kLxT0pA/7/fs+a5X/s4TkzqCVVQSzhas/84q0WE99ScAcM1LQJoIICFjAuBgkqhkiG9w0BCQ4xITAfMB0GA1UdDgQWBBR6VRzktP1pzZxsGUaJivNUrtgSrzCCAUcGCSqGSIb3DQEJDDGCATgEggE0KZq9s6HEViRfplVgYkulg6XV411ZRe4U1UjfXTf1pRaygfcenGbT6RRagPtZzjuq5hHdYhqDjRzZhnbn8ZASYTgBM7qcseUq5UpS1pE08DI2jePKqatp3Pzm6a/MGSziESnREx784JlKfwKMjJl33UA8lQm9nhSeAIHyBx3c4Lf8IXdW2n3rnhbVfjpBMAxwh6lt+e5agtGXy+q/xAESUeLPfUgRYWctlLgt8Op+WTpLyBkZsVFoBvJrMt2XdM0RI32YzTRr56GXFa4VyQmY5xXwlQSPgidAP7jPkVygNcoeXvAz2ZCk3IR1Cn3mX8nMko53MlDNaMYldUQA0ug28/S7BlSlaq2CDD4Ol3swTq7C4KGTxKrI36ruYUZx7NEaQDF5V7VvqPCZ0fZoTIJuSYTQ67gwEQYKKwYBBAGDjBsCATEDEwFvMD0GCSsGAQIBDwMBATEwEy5RbVhSWTRyaEF4OE11cThkTUdrcjlxa25KZEU2VUhaRGRHYURSVFFFYndGTjViMEcGA1UdETFAEz42dnUyYnhraTc3N2l0M2NwYXl2NmZxNnZwbDRrZTNremo3Z3hpY2Z5Z201NWRoaHRwaHlmZHZ5ZC5vbmlvbjAKBggqhkjOPQQDAgNJADBGAiEAt+f1u/bchg5AZHv6NTGNoXeejTRWUhX3ioGwW6TGg84CIQCHqKNzDh2JjS/hUHx5PApAmfNnQTSf19X6LnNHQweU1g==',
'MIIDHTCCAsMCAQAwSTFHMEUGA1UEAxM+eTd5Y3ptdWdsMnRla2FtaTdzYmR6NXBmYWVtdng3YmFod3RocmR2Y2J6dzV2ZXgyY3JzcjI2cWQub25pb24wWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAATMq0l4bCmjdb0grtzpwtDVLM9E1IQpL9vrB4+lD9OBZzlrx2365jV7shVu9utas8w8fxtKoBZSnT5+32ZMFTB4oIICFjAuBgkqhkiG9w0BCQ4xITAfMB0GA1UdDgQWBBSoDQpTZdEvi1/Rr/muVXT1clyKRDCCAUcGCSqGSIb3DQEJDDGCATgEggE0BQvyvkiiXEf/PLKnsR1Ba9AhYsVO8o56bnftUnoVzBlRZgUzLJvOSroPk/EmbVz+okhMrcYNgCWHvxrAqHVVq0JRP6bi98BtCUotx6OPFHp5K5QCL60hod1uAnhKocyJG9tsoM9aS+krn/k+g4RCBjiPZ25cC7QG/UNr6wyIQ8elBho4MKm8iOp7EShSsZOV1f6xrnXYCC/zyUc85GEuycLzVImgAQvPATbdMzY4zSGnNLHxkvSUNxaR9LnEWf+i1jeqcOiXOvmdyU5Be3ZqhGKvvBg/5vyLQiCIfeapjZemnLqFHQBitglDm2xnKL6HzMyfZoAHPV7YcWYR4spU9Ju8Q8aqSeAryx7sx55eSR4GO5UQTo5DrQn6xtkwOZ/ytsOknFthF8jcA9uTAMDKA2TylCUwEQYKKwYBBAGDjBsCATEDEwFvMD0GCSsGAQIBDwMBATEwEy5RbVQxOFV2blVCa3NlTWMzU3FuZlB4cEh3TjhuekxySmVOU0xadGM4ckFGWGh6MEcGA1UdETFAEz55N3ljem11Z2wydGVrYW1pN3NiZHo1cGZhZW12eDdiYWh3dGhyZHZjYnp3NXZleDJjcnNyMjZxZC5vbmlvbjAKBggqhkjOPQQDAgNIADBFAiEAoFrAglxmk7ciD6AHQOB1qEoLu0NARcxgwmIry8oeTHwCICyXp5NJQ9Z8vReIAQNng2H2+/XjHifZEWzhoN0VkcBx',
])
const allUsers = storageService.getAllUsers()
const allUsers = await storageService.getAllUsers()

expect(allUsers).toStrictEqual([
{
Expand Down
96 changes: 77 additions & 19 deletions packages/backend/src/nest/storage/storage.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
parseCertificationRequest,
getCertFieldValue,
getReqFieldValue,
keyFromCertificate,
} from '@quiet/identity'
import type { IPFS } from 'ipfs-core'
import EventStore from 'orbit-db-eventstore'
Expand Down Expand Up @@ -287,11 +288,15 @@ export class StorageService extends EventEmitter {
this.certificatesStore.on(StorageEvents.CERTIFICATES_STORED, async payload => {
this.emit(StorageEvents.CERTIFICATES_STORED, payload)
await this.updatePeersList()
// TODO: Shouldn't we also dial new peers or at least add them
// to the peer store for the auto-dialer to handle?
})

this.certificatesRequestsStore.on(StorageEvents.CSRS_STORED, async (payload: { csrs: string[] }) => {
this.emit(StorageEvents.CSRS_STORED, payload)
await this.updatePeersList()
// TODO: Shouldn't we also dial new peers or at least add them
// to the peer store for the auto-dialer to handle?
})

this.communityMetadataStore.on(StorageEvents.COMMUNITY_METADATA_STORED, (meta: CommunityMetadata) => {
Expand All @@ -312,19 +317,35 @@ export class StorageService extends EventEmitter {
return meta
}

// TODO: Add test case around existing peers
public async updatePeersList() {
const users = this.getAllUsers()
const peers = users.map(peer => createLibp2pAddress(peer.onionAddress, peer.peerId))
console.log('updatePeersList, peers count:', peers.length)

const community = await this.localDbService.getCurrentCommunity()
if (!community) return

if (!community) {
throw new Error('Failed to update peers list - community missing')
}

// Always include existing peers. Otherwise, if CSRs or
// certificates do not replicate, then this could remove peers.
const existingPeers = community.peerList ?? []
this.logger('Existing peers count:', existingPeers.length)

const users = await this.getAllUsers()
const peers = Array.from(new Set([
// FIXME: This can result in self-dialing since we add the local
// peer address again in getSortedPeers
...existingPeers,
...users.map(user => createLibp2pAddress(user.onionAddress, user.peerId)),
]))
const sortedPeers = await this.localDbService.getSortedPeers(peers)
if (sortedPeers.length > 0) {
community.peerList = sortedPeers
await this.localDbService.setCommunity(community)

// This should never happen, but just in case
if (sortedPeers.length === 0) {
throw new Error('Failed to update peers list - no peers')
}

this.logger('Updating community peer list. Peers count:', sortedPeers.length)
community.peerList = sortedPeers
await this.localDbService.setCommunity(community)
this.emit(StorageEvents.COMMUNITY_UPDATED, community)
}

Expand Down Expand Up @@ -729,18 +750,55 @@ export class StorageService extends EventEmitter {
return result
}

public getAllUsers(): UserData[] {
const csrs = this.getAllEventLogEntries(this.certificatesRequestsStore.store)
this.logger('csrs count:', csrs.length)
const allUsers: UserData[] = []
/**
* Retrieve all users (using certificates and CSRs to determine users)
*/
// TODO: Add test case around have both certificates and CSRs
public async getAllUsers(): Promise<UserData[]> {
const csrs = await this.certificatesRequestsStore.getCsrs()
const certs = await this.certificatesStore.getCertificates()
const allUsersByKey: Record<string, UserData> = {}

this.logger(`Retrieving all users. CSRs count: ${csrs.length} Certificates count: ${certs.length}`)

for (const cert of certs) {
const parsedCert = parseCertificate(cert)
const pubKey = keyFromCertificate(parsedCert)
const onionAddress = getCertFieldValue(parsedCert, CertFieldsTypes.commonName)
const peerId = getCertFieldValue(parsedCert, CertFieldsTypes.peerId)
const username = getCertFieldValue(parsedCert, CertFieldsTypes.nickName)

// TODO: This validation should go in CertificatesStore
if (!pubKey || !onionAddress || !peerId || !username) {
this.logger.error(`Received invalid certificate. onionAddress: ${onionAddress} peerId: ${peerId} username: ${username}`)
continue
}

allUsersByKey[pubKey] = { onionAddress, peerId, username }
}

for (const csr of csrs) {
const parsedCert = parseCertificationRequest(csr)
const onionAddress = getReqFieldValue(parsedCert, CertFieldsTypes.commonName)
const peerId = getReqFieldValue(parsedCert, CertFieldsTypes.peerId)
const username = getReqFieldValue(parsedCert, CertFieldsTypes.nickName)
if (!onionAddress || !peerId || !username) continue
allUsers.push({ onionAddress, peerId, username })
const parsedCsr = parseCertificationRequest(csr)
const pubKey = keyFromCertificate(parsedCsr)
const onionAddress = getReqFieldValue(parsedCsr, CertFieldsTypes.commonName)
const peerId = getReqFieldValue(parsedCsr, CertFieldsTypes.peerId)
const username = getReqFieldValue(parsedCsr, CertFieldsTypes.nickName)

// TODO: This validation should go in CertificatesRequestsStore
if (!pubKey || !onionAddress || !peerId || !username) {
this.logger.error(`Received invalid CSR. onionAddres: ${onionAddress} peerId: ${peerId} username: ${username}`)
continue
}

if (!(pubKey in allUsersByKey)) {
allUsersByKey[pubKey] = { onionAddress, peerId, username }
}
}

const allUsers = Object.values(allUsersByKey)

this.logger(`All users count: ${allUsers.length}`)

return allUsers
}

Expand Down
8 changes: 7 additions & 1 deletion packages/desktop/src/renderer/sagas/socket/socket.saga.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,13 @@
import { io } from 'socket.io-client'
import { all, fork, takeEvery, call, put, cancel, FixedTask, select, take } from 'typed-redux-saga'
import { all, fork, takeEvery, call, put, cancel, FixedTask, select, take, delay, apply } from 'typed-redux-saga'
import { PayloadAction } from '@reduxjs/toolkit'
import { socket as stateManager, messages, connection, Socket } from '@quiet/state-manager'
import { socketActions } from './socket.slice'
import { eventChannel } from 'redux-saga'
import { displayMessageNotificationSaga } from '../notifications/notifications.saga'
import logger from '../../logger'
import { encodeSecret } from '@quiet/common'
import { SocketActionTypes } from '@quiet/types'

const log = logger('socket')

Expand All @@ -27,6 +28,7 @@ export function* startConnectionSaga(

if (!socketIOSecret) return

log('Connecting to backend')
const token = encodeSecret(socketIOSecret)
const socket = yield* call(io, `http://127.0.0.1:${dataPort}`, {
withCredentials: true,
Expand All @@ -43,6 +45,10 @@ export function* startConnectionSaga(
function* setConnectedSaga(socket: Socket): Generator {
const root = yield* fork(stateManager.useIO, socket)
const observers = yield* fork(initObservers)

console.log('Frontend is ready. Starting backend...')
yield* apply(socket, socket.emit, [SocketActionTypes.START])

// Handle suspending current connection
yield all([
takeEvery(socketActions.suspendConnection, cancelRootSaga, root),
Expand Down
1 change: 1 addition & 0 deletions packages/identity/src/extractPubKey.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import config from './config'
import { getAlgorithmParameters, Certificate, CertificationRequest, getCrypto } from 'pkijs'
import { NoCryptoEngineError } from '@quiet/types'

// FIXME: This is a duplicate of loadCertificate
export const parseCertificate = (pem: string): Certificate => {
let certificateBuffer = new ArrayBuffer(0)
certificateBuffer = stringToArrayBuffer(fromBase64(pem))
Expand Down
2 changes: 1 addition & 1 deletion packages/mobile/src/screens/Channel/Channel.screen.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ export const ChannelScreen: FC = () => {
return updatedExistingFiles
})

//User Label
// User Label

const duplicatedUsernameHandleBack = useCallback(() => {
dispatch(
Expand Down
Loading

0 comments on commit 20fa367

Please sign in to comment.