From 3ae802043dbb111d3339c84e8be670293cef769a Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Mon, 21 Aug 2023 21:32:53 -0300 Subject: [PATCH 01/19] first pass into transforming into handlers --- .../combiner/src/common/action.ts | 9 +- .../combiner/src/common/combine.ts | 7 +- .../combiner/src/common/controller.ts | 25 ---- .../combiner/src/common/handlers.ts | 92 +++++++++++++ .../combiner/src/common/session.ts | 1 + .../combiner/src/server.ts | 124 ++++-------------- 6 files changed, 132 insertions(+), 126 deletions(-) delete mode 100644 packages/phone-number-privacy/combiner/src/common/controller.ts create mode 100644 packages/phone-number-privacy/combiner/src/common/handlers.ts diff --git a/packages/phone-number-privacy/combiner/src/common/action.ts b/packages/phone-number-privacy/combiner/src/common/action.ts index 2bf519cb861..008d01e9b7c 100644 --- a/packages/phone-number-privacy/combiner/src/common/action.ts +++ b/packages/phone-number-privacy/combiner/src/common/action.ts @@ -1,8 +1,13 @@ -import { OdisRequest } from '@celo/phone-number-privacy-common' +import { OdisRequest, OdisResponse } from '@celo/phone-number-privacy-common' +import { Locals, Request, Response } from 'express' import { IO } from './io' import { Session } from './session' export interface Action { readonly io: IO - perform(session: Session): Promise + perform( + request: Request<{}, {}, R>, + response: Response, Locals>, + session: Session + ): Promise } diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 66350cfd32b..295f677effe 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -4,6 +4,7 @@ import { OdisResponse, WarningMessage, } from '@celo/phone-number-privacy-common' +import { Locals, Request, Response } from 'express' import { Response as FetchResponse } from 'node-fetch' import { PerformanceObserver } from 'perf_hooks' import { OdisConfig } from '../config' @@ -24,7 +25,11 @@ export abstract class CombineAction implements Action abstract combine(session: Session): void - async perform(session: Session) { + async perform( + _request: Request<{}, {}, R>, + _response: Response, Locals>, + session: Session + ) { await this.distribute(session) this.combine(session) } diff --git a/packages/phone-number-privacy/combiner/src/common/controller.ts b/packages/phone-number-privacy/combiner/src/common/controller.ts deleted file mode 100644 index 7726bebad2a..00000000000 --- a/packages/phone-number-privacy/combiner/src/common/controller.ts +++ /dev/null @@ -1,25 +0,0 @@ -import { ErrorMessage, OdisRequest, OdisResponse } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import { Action } from './action' - -export class Controller { - constructor(readonly action: Action) {} - - public async handle( - request: Request<{}, {}, unknown>, - response: Response> - ): Promise { - try { - const session = await this.action.io.init(request, response) - if (session) { - await this.action.perform(session) - } - } catch (err) { - response.locals.logger.error( - { error: err }, - `Unknown error in handler for ${this.action.io.endpoint}` - ) - this.action.io.sendFailure(ErrorMessage.UNKNOWN_ERROR, 500, response) - } - } -} diff --git a/packages/phone-number-privacy/combiner/src/common/handlers.ts b/packages/phone-number-privacy/combiner/src/common/handlers.ts new file mode 100644 index 00000000000..2fc6259219f --- /dev/null +++ b/packages/phone-number-privacy/combiner/src/common/handlers.ts @@ -0,0 +1,92 @@ +import { ErrorMessage, OdisRequest, OdisResponse } from '@celo/phone-number-privacy-common' +import Logger from 'bunyan' +import { Request, Response } from 'express' +import { performance, PerformanceObserver } from 'perf_hooks' +import { Action } from './action' + +export interface Locals { + logger: Logger +} + +export type PromiseHandler = ( + request: Request<{}, {}, R>, + res: Response, Locals> +) => Promise + +type ParentHandler = (req: Request<{}, {}, any>, res: Response) => Promise + +export function catchErrorHandler( + handler: PromiseHandler +): ParentHandler { + return async (req, res) => { + const logger: Logger = res.locals.logger + try { + await handler(req, res) + } catch (err) { + logger.error(ErrorMessage.CAUGHT_ERROR_IN_ENDPOINT_HANDLER) + logger.error(err) + if (!res.headersSent) { + logger.info('Responding with error in outer endpoint handler') + res.status(500).json({ + success: false, + error: ErrorMessage.UNKNOWN_ERROR, + }) + } else { + logger.error(ErrorMessage.ERROR_AFTER_RESPONSE_SENT) + } + } + } +} + +export function meteringHandler( + handler: PromiseHandler +): PromiseHandler { + return async (req, res) => { + const logger: Logger = res.locals.logger + + const eventLoopLagMeasurementStart = Date.now() + setTimeout(() => { + const eventLoopLag = Date.now() - eventLoopLagMeasurementStart + logger.info({ eventLoopLag }, 'Measure event loop lag') + }) + const startMark = `Begin ${req.url}` + const endMark = `End ${req.url}` + const entryName = `${req.url} latency` + + const obs = new PerformanceObserver((list) => { + const entry = list.getEntriesByName(entryName)[0] + if (entry) { + logger.info({ latency: entry }, 'e2e response latency measured') + } + }) + obs.observe({ entryTypes: ['measure'], buffered: false }) + + performance.mark(startMark) + + try { + await handler(req, res) + } finally { + performance.mark(endMark) + performance.measure(entryName, startMark, endMark) + performance.clearMarks() + obs.disconnect() + } + } +} + +export function actionHandler(action: Action): PromiseHandler { + return async (request, response) => { + try { + const session = await action.io.init(request, response) + if (session) { + await action.perform(request, response, session) + } + } catch (err) { + response.locals.logger.error( + { error: err }, + `Unknown error in handler for ${action.io.endpoint}` + ) + action.io.sendFailure(ErrorMessage.UNKNOWN_ERROR, 500, response) + } + } +} diff --git a/packages/phone-number-privacy/combiner/src/common/session.ts b/packages/phone-number-privacy/combiner/src/common/session.ts index bfa3ae24b29..26bb76f6703 100644 --- a/packages/phone-number-privacy/combiner/src/common/session.ts +++ b/packages/phone-number-privacy/combiner/src/common/session.ts @@ -52,3 +52,4 @@ export class Session { return maxErrorCode > 0 ? maxErrorCode : null } } +;`` diff --git a/packages/phone-number-privacy/combiner/src/server.ts b/packages/phone-number-privacy/combiner/src/server.ts index e5360d9eaea..0df173688b7 100644 --- a/packages/phone-number-privacy/combiner/src/server.ts +++ b/packages/phone-number-privacy/combiner/src/server.ts @@ -1,17 +1,19 @@ import { ContractKit } from '@celo/contractkit' import { CombinerEndpoint, - Endpoint, - ErrorMessage, KEY_VERSION_HEADER, loggerMiddleware, + OdisRequest, rootLogger, } from '@celo/phone-number-privacy-common' -import Logger from 'bunyan' -import express, { Request, RequestHandler, Response } from 'express' +import express, { RequestHandler } from 'express' // tslint:disable-next-line: ordered-imports -import { PerformanceObserver, performance } from 'perf_hooks' -import { Controller } from './common/controller' +import { + actionHandler, + catchErrorHandler, + meteringHandler, + PromiseHandler, +} from './common/handlers' import { CombinerConfig, getCombinerVersion } from './config' import { DomainDisableAction } from './domain/endpoints/disable/action' import { DomainDisableIO } from './domain/endpoints/disable/io' @@ -33,6 +35,7 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { logger.info('Creating combiner express server') const app = express() + // TODO get logger to show accurate serviceName // (https://github.com/celo-org/celo-monorepo/issues/9809) app.use(express.json({ limit: '0.2mb' }) as RequestHandler, loggerMiddleware(config.serviceName)) @@ -60,134 +63,59 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { const pnpThresholdStateService = new PnpThresholdStateService() - const pnpQuota = new Controller( + const pnpQuota = actionHandler( new PnpQuotaAction( config.phoneNumberPrivacy, pnpThresholdStateService, new PnpQuotaIO(config.phoneNumberPrivacy, kit) ) ) - app.post(CombinerEndpoint.PNP_QUOTA, (req, res) => - meterResponse(pnpQuota.handle.bind(pnpQuota), req, res, CombinerEndpoint.PNP_QUOTA, config) - ) - const pnpSign = new Controller( + const pnpSign = actionHandler( new PnpSignAction( config.phoneNumberPrivacy, pnpThresholdStateService, new PnpSignIO(config.phoneNumberPrivacy, kit) ) ) - app.post(CombinerEndpoint.PNP_SIGN, (req, res) => - meterResponse(pnpSign.handle.bind(pnpSign), req, res, CombinerEndpoint.PNP_SIGN, config) - ) const domainThresholdStateService = new DomainThresholdStateService(config.domains) - const domainQuota = new Controller( + const domainQuota = actionHandler( new DomainQuotaAction( config.domains, domainThresholdStateService, new DomainQuotaIO(config.domains) ) ) - app.post(CombinerEndpoint.DOMAIN_QUOTA_STATUS, (req, res) => - meterResponse( - domainQuota.handle.bind(domainQuota), - req, - res, - CombinerEndpoint.DOMAIN_QUOTA_STATUS, - config - ) - ) - const domainSign = new Controller( + + const domainSign = actionHandler( new DomainSignAction( config.domains, domainThresholdStateService, new DomainSignIO(config.domains) ) ) - app.post(CombinerEndpoint.DOMAIN_SIGN, (req, res) => - meterResponse( - domainSign.handle.bind(domainSign), - req, - res, - CombinerEndpoint.DOMAIN_SIGN, - config - ) - ) - const domainDisable = new Controller( + + const domainDisable = actionHandler( new DomainDisableAction( config.domains, domainThresholdStateService, new DomainDisableIO(config.domains) ) ) - app.post(CombinerEndpoint.DISABLE_DOMAIN, (req, res) => - meterResponse( - domainDisable.handle.bind(domainDisable), - req, - res, - CombinerEndpoint.DISABLE_DOMAIN, - config - ) - ) + + app.post(CombinerEndpoint.PNP_QUOTA, createHandler(pnpQuota)) + app.post(CombinerEndpoint.PNP_SIGN, createHandler(pnpSign)) + app.post(CombinerEndpoint.DOMAIN_QUOTA_STATUS, createHandler(domainQuota)) + app.post(CombinerEndpoint.DOMAIN_SIGN, createHandler(domainSign)) + app.post(CombinerEndpoint.DISABLE_DOMAIN, createHandler(domainDisable)) return app } -export async function meterResponse( - handler: (req: Request, res: Response) => Promise, - req: Request, - res: Response, - endpoint: Endpoint, - config: CombinerConfig -) { - if (!res.locals) { - res.locals = {} - } - const logger: Logger = loggerMiddleware(config.serviceName)(req, res) - logger.fields.endpoint = endpoint - logger.info({ req: req.body }, 'Request received') - const eventLoopLagMeasurementStart = Date.now() - setTimeout(() => { - const eventLoopLag = Date.now() - eventLoopLagMeasurementStart - logger.info({ eventLoopLag }, 'Measure event loop lag') - }) - const startMark = `Begin ${endpoint}` - const endMark = `End ${endpoint}` - const entryName = `${endpoint} latency` - - const obs = new PerformanceObserver((list) => { - const entry = list.getEntriesByName(entryName)[0] - if (entry) { - logger.info({ latency: entry }, 'e2e response latency measured') - } - }) - obs.observe({ entryTypes: ['measure'], buffered: false }) - - performance.mark(startMark) - await handler(req, res) - .then(() => { - logger.info({ res }, 'Response sent') - }) - .catch((err) => { - logger.error(ErrorMessage.CAUGHT_ERROR_IN_ENDPOINT_HANDLER) - logger.error(err) - if (!res.headersSent) { - logger.info('Responding with error in outer endpoint handler') - res.status(500).json({ - success: false, - error: ErrorMessage.UNKNOWN_ERROR, - }) - } else { - logger.error(ErrorMessage.ERROR_AFTER_RESPONSE_SENT) - } - }) - .finally(() => { - performance.mark(endMark) - performance.measure(entryName, startMark, endMark) - performance.clearMarks() - obs.disconnect() - }) +export function createHandler( + handler: PromiseHandler +): PromiseHandler { + return meteringHandler(catchErrorHandler(handler)) } From 337562fdbea3814d5cfe71956410a72d96eda1d1 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Mon, 21 Aug 2023 22:15:43 -0300 Subject: [PATCH 02/19] Extract methods out of IO class (story: IO class destruction) --- .../combiner/src/common/combine.ts | 41 +++- .../combiner/src/common/handlers.ts | 3 +- .../combiner/src/common/io.ts | 177 ++++++++++-------- .../combiner/src/common/sign.ts | 4 +- .../src/domain/endpoints/disable/action.ts | 7 +- .../src/domain/endpoints/disable/io.ts | 21 +-- .../src/domain/endpoints/quota/action.ts | 7 +- .../combiner/src/domain/endpoints/quota/io.ts | 25 +-- .../combiner/src/domain/endpoints/sign/io.ts | 34 +--- .../src/pnp/endpoints/quota/action.ts | 7 +- .../combiner/src/pnp/endpoints/quota/io.ts | 21 +-- .../combiner/src/pnp/endpoints/sign/io.ts | 30 +-- 12 files changed, 180 insertions(+), 197 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 295f677effe..9f543a4a5b9 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -9,7 +9,7 @@ import { Response as FetchResponse } from 'node-fetch' import { PerformanceObserver } from 'perf_hooks' import { OdisConfig } from '../config' import { Action } from './action' -import { IO } from './io' +import { fetchSignerResponseWithFallback, IO } from './io' import { Session } from './session' export interface Signer { @@ -18,7 +18,8 @@ export interface Signer { } export abstract class CombineAction implements Action { - readonly signers: Signer[] + private readonly signers: Signer[] + public constructor(readonly config: OdisConfig, readonly io: IO) { this.signers = JSON.parse(config.odisServices.signers) } @@ -82,10 +83,14 @@ export abstract class CombineAction implements Action obs.disconnect() } - protected async forwardToSigner(signer: Signer, session: Session): Promise { + private async forwardToSigner(signer: Signer, session: Session): Promise { let signerFetchResult: FetchResponse | undefined try { - signerFetchResult = await this.io.fetchSignerResponseWithFallback(signer, session) + signerFetchResult = await fetchSignerResponseWithFallback( + signer, + this.io.signerEndpoint, + session + ) session.logger.info({ message: 'Received signerFetchResult', signer: signer.url, @@ -108,7 +113,7 @@ export abstract class CombineAction implements Action return this.handleFetchResult(signer, session, signerFetchResult) } - protected async handleFetchResult( + private async handleFetchResult( signer: Signer, session: Session, signerFetchResult?: FetchResponse @@ -178,3 +183,29 @@ export abstract class CombineAction implements Action } } } + +// async function callSigner(logger: Logger, signer: Signer) { +// let signerFetchResult: FetchResponse | undefined +// try { +// signerFetchResult = await this.io.fetchSignerResponseWithFallback(signer, session) +// logger.info({ +// message: 'Received signerFetchResult', +// signer: signer.url, +// status: signerFetchResult.status, +// }) +// } catch (err) { +// logger.debug({ err, signer: signer.url, message: 'signer request failure' }) +// if (err instanceof Error && err.name === 'AbortError' && session.abort.signal.aborted) { +// if (session.timedOut) { +// logger.error({ signer }, ErrorMessage.TIMEOUT_FROM_SIGNER) +// } else { +// logger.info({ signer }, WarningMessage.CANCELLED_REQUEST_TO_SIGNER) +// } +// } else { +// // Logging the err & message simultaneously fails to log the message in some cases +// logger.error({ signer }, ErrorMessage.SIGNER_REQUEST_ERROR) +// logger.error({ signer, err }) +// } +// } +// return this.handleFetchResult(signer, session, signerFetchResult) +// } diff --git a/packages/phone-number-privacy/combiner/src/common/handlers.ts b/packages/phone-number-privacy/combiner/src/common/handlers.ts index 2fc6259219f..3b407e071d0 100644 --- a/packages/phone-number-privacy/combiner/src/common/handlers.ts +++ b/packages/phone-number-privacy/combiner/src/common/handlers.ts @@ -3,6 +3,7 @@ import Logger from 'bunyan' import { Request, Response } from 'express' import { performance, PerformanceObserver } from 'perf_hooks' import { Action } from './action' +import { sendFailure } from './io' export interface Locals { logger: Logger @@ -86,7 +87,7 @@ export function actionHandler(action: Action): Promise { error: err }, `Unknown error in handler for ${action.io.endpoint}` ) - action.io.sendFailure(ErrorMessage.UNKNOWN_ERROR, 500, response) + sendFailure(ErrorMessage.UNKNOWN_ERROR, 500, response) } } } diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 89545648e2f..649cb791a8b 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -2,13 +2,13 @@ import { CombinerEndpoint, ErrorMessage, ErrorType, - FailureResponse, getRequestKeyVersion, - KEY_VERSION_HEADER, KeyVersionInfo, + KEY_VERSION_HEADER, OdisRequest, OdisResponse, requestHasValidKeyVersion, + send, SignerEndpoint, SuccessResponse, WarningMessage, @@ -18,7 +18,7 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import fetch, { Response as FetchResponse } from 'node-fetch' import { performance } from 'perf_hooks' -import { OdisConfig } from '../config' +import { getCombinerVersion, OdisConfig } from '../config' import { Signer } from './combine' import { Session } from './session' @@ -44,13 +44,6 @@ export abstract class IO { abstract authenticate(request: Request<{}, {}, R>, logger?: Logger): Promise - abstract sendFailure( - error: ErrorType, - status: number, - response: Response>, - ...args: unknown[] - ): void - abstract sendSuccess( status: number, response: Response>, @@ -61,36 +54,6 @@ export abstract class IO { return this.requestSchema.is(request.body) } - getKeyVersionInfo(request: Request<{}, {}, OdisRequest>, logger: Logger): KeyVersionInfo { - // If an invalid key version is present, we don't want this function to throw but - // to instead replace the key version with the default - // If a valid but unsupported key version is present, we want this function to throw - let requestKeyVersion: number | undefined - if (requestHasValidKeyVersion(request, logger)) { - requestKeyVersion = getRequestKeyVersion(request, logger) - } - const keyVersion = requestKeyVersion ?? this.config.keys.currentVersion - const supportedVersions: KeyVersionInfo[] = JSON.parse(this.config.keys.versions) // TODO add io-ts checks for this and signer array - const filteredSupportedVersions: KeyVersionInfo[] = supportedVersions.filter( - (v) => v.keyVersion === keyVersion - ) - if (!filteredSupportedVersions.length) { - throw new Error(`key version ${keyVersion} not supported`) - } - return filteredSupportedVersions[0] - } - - requestHasSupportedKeyVersion(request: Request<{}, {}, OdisRequest>, logger: Logger): boolean { - try { - this.getKeyVersionInfo(request, logger) - return true - } catch (err) { - logger.debug('Error caught in requestHasSupportedKeyVersion') - logger.debug(err) - return false - } - } - validateSignerResponse(data: string, url: string, logger: Logger): OdisResponse { const res: unknown = JSON.parse(data) if (!this.responseSchema.is(res)) { @@ -103,36 +66,68 @@ export abstract class IO { return res } - async fetchSignerResponseWithFallback( - signer: Signer, - session: Session - ): Promise { - const start = `Start ${signer.url + this.signerEndpoint}` - const end = `End ${signer.url + this.signerEndpoint}` - performance.mark(start) + protected inputChecks( + request: Request<{}, {}, unknown>, + response: Response> + ): request is Request<{}, {}, R> { + if (!this.config.enabled) { + sendFailure(WarningMessage.API_UNAVAILABLE, 503, response) + return false + } + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return false + } + return true + } +} + +export function requestHasSupportedKeyVersion( + request: Request<{}, {}, OdisRequest>, + config: OdisConfig, + logger: Logger +): boolean { + try { + getKeyVersionInfo(request, config, logger) + return true + } catch (err) { + logger.debug('Error caught in requestHasSupportedKeyVersion') + logger.debug(err) + return false + } +} - return this.fetchSignerResponse(signer.url, session) - .catch((err) => { - session.logger.error({ url: signer.url, error: err }, `Signer failed with primary url`) - if (signer.fallbackUrl) { - session.logger.warn({ url: signer.fallbackUrl }, `Using fallback url to call signer`) - return this.fetchSignerResponse(signer.fallbackUrl, session) - } - throw err - }) - .finally(() => { - performance.mark(end) - performance.measure(signer.url, start, end) - }) +export function getKeyVersionInfo( + request: Request<{}, {}, OdisRequest>, + config: OdisConfig, + logger: Logger +): KeyVersionInfo { + // If an invalid key version is present, we don't want this function to throw but + // to instead replace the key version with the default + // If a valid but unsupported key version is present, we want this function to throw + let requestKeyVersion: number | undefined + if (requestHasValidKeyVersion(request, logger)) { + requestKeyVersion = getRequestKeyVersion(request, logger) + } + const keyVersion = requestKeyVersion ?? config.keys.currentVersion + const supportedVersions: KeyVersionInfo[] = JSON.parse(config.keys.versions) // TODO add io-ts checks for this and signer array + const filteredSupportedVersions: KeyVersionInfo[] = supportedVersions.filter( + (v) => v.keyVersion === keyVersion + ) + if (!filteredSupportedVersions.length) { + throw new Error(`key version ${keyVersion} not supported`) } + return filteredSupportedVersions[0] +} + +export async function fetchSignerResponseWithFallback( + signer: Signer, + signerEndpoint: string, + session: Session +): Promise { + const { request, abort } = session - protected async fetchSignerResponse( - signerUrl: string, - session: Session - ): Promise { - const { request, logger, abort } = session - const url = signerUrl + this.signerEndpoint - logger.debug({ url }, `Sending signer request`) + async function fetchSignerResponse(url: string): Promise { // prettier-ignore return fetch(url, { method: 'POST', @@ -151,18 +146,40 @@ export abstract class IO { }) } - protected inputChecks( - request: Request<{}, {}, unknown>, - response: Response> - ): request is Request<{}, {}, R> { - if (!this.config.enabled) { - this.sendFailure(WarningMessage.API_UNAVAILABLE, 503, response) - return false - } - if (!this.validateClientRequest(request)) { - this.sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return false - } - return true + return measureTime(signer.url + signerEndpoint, () => + fetchSignerResponse(signer.url + signerEndpoint).catch((err) => { + session.logger.error({ url: signer.url, error: err }, `Signer failed with primary url`) + if (signer.fallbackUrl) { + session.logger.warn({ url: signer.fallbackUrl }, `Using fallback url to call signer`) + return fetchSignerResponse(signer.fallbackUrl + signerEndpoint) + } else { + throw err + } + }) + ) +} +async function measureTime(name: string, fn: () => Promise): Promise { + const start = `Start ${name}` + const end = `End ${name}` + performance.mark(start) + try { + const res = await fn() + return res + } finally { + performance.mark(end) + performance.measure(name, start, end) } } + +export function sendFailure(error: ErrorType, status: number, response: Response) { + send( + response, + { + success: false, + version: getCombinerVersion(), + error, + }, + status, + response.locals.logger + ) +} diff --git a/packages/phone-number-privacy/combiner/src/common/sign.ts b/packages/phone-number-privacy/combiner/src/common/sign.ts index 3a82f0dc012..17ab7bfef6d 100644 --- a/packages/phone-number-privacy/combiner/src/common/sign.ts +++ b/packages/phone-number-privacy/combiner/src/common/sign.ts @@ -12,7 +12,7 @@ import { DomainThresholdStateService } from '../domain/services/threshold-state' import { PnpThresholdStateService } from '../pnp/services/threshold-state' import { CombineAction } from './combine' import { CryptoSession } from './crypto-session' -import { IO } from './io' +import { IO, sendFailure } from './io' // prettier-ignore export type OdisSignatureRequest = @@ -85,7 +85,7 @@ export abstract class SignAction extends Combine protected handleMissingSignatures(session: CryptoSession) { const errorCode = session.getMajorityErrorCode() ?? 500 const error = this.errorCodeToError(errorCode) - this.io.sendFailure(error, errorCode, session.response) + sendFailure(error, errorCode, session.response) } protected abstract errorCodeToError(errorCode: number): ErrorType diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index 21ab840ee9f..5d9794dee72 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -1,6 +1,6 @@ import { DisableDomainRequest, ErrorMessage } from '@celo/phone-number-privacy-common' import { CombineAction } from '../../../common/combine' -import { IO } from '../../../common/io' +import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' @@ -29,11 +29,10 @@ export class DomainDisableAction extends CombineAction { session.logger.error({ err }, 'Error combining signer disable domain status responses') } - this.io.sendFailure( + sendFailure( ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response, - session.logger + session.response ) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts index 21b8e81f501..998fefb7634 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts @@ -18,7 +18,7 @@ import { } from '@celo/phone-number-privacy-common' import { Request, Response } from 'express' import * as t from 'io-ts' -import { IO } from '../../../common/io' +import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion } from '../../../config' @@ -38,10 +38,14 @@ export class DomainDisableIO extends IO { return null } if (!(await this.authenticate(request))) { - this.sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } - return new Session(request, response, this.getKeyVersionInfo(request, response.locals.logger)) + return new Session( + request, + response, + getKeyVersionInfo(request, this.config, response.locals.logger) + ) } authenticate(request: Request<{}, {}, DisableDomainRequest>): Promise { @@ -66,15 +70,6 @@ export class DomainDisableIO extends IO { } sendFailure(error: ErrorType, status: number, response: Response) { - send( - response, - { - success: false, - version: getCombinerVersion(), - error, - }, - status, - response.locals.logger - ) + sendFailure(error, status, response) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index 4ba6032fc05..829129af791 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -1,6 +1,6 @@ import { DomainQuotaStatusRequest, ErrorMessage } from '@celo/phone-number-privacy-common' import { CombineAction } from '../../../common/combine' -import { IO } from '../../../common/io' +import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' @@ -29,11 +29,10 @@ export class DomainQuotaAction extends CombineAction { session.logger.error(err, 'Error combining signer quota status responses') } } - this.io.sendFailure( + sendFailure( ErrorMessage.THRESHOLD_DOMAIN_QUOTA_STATUS_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response, - session.logger + session.response ) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts index 3469fc2938d..26f527ef9d5 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts @@ -3,12 +3,10 @@ import { DomainQuotaStatusRequest, domainQuotaStatusRequestSchema, DomainQuotaStatusResponse, - DomainQuotaStatusResponseFailure, domainQuotaStatusResponseSchema, DomainQuotaStatusResponseSuccess, DomainSchema, DomainState, - ErrorType, getSignerEndpoint, OdisResponse, send, @@ -19,7 +17,7 @@ import { } from '@celo/phone-number-privacy-common' import { Request, Response } from 'express' import * as t from 'io-ts' -import { IO } from '../../../common/io' +import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion } from '../../../config' @@ -39,10 +37,10 @@ export class DomainQuotaIO extends IO { return null } if (!(await this.authenticate(request))) { - this.sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } - const keyVersionInfo = this.getKeyVersionInfo(request, response.locals.logger) + const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) return new Session(request, response, keyVersionInfo) } @@ -66,21 +64,4 @@ export class DomainQuotaIO extends IO { response.locals.logger ) } - - sendFailure( - error: ErrorType, - status: number, - response: Response - ) { - send( - response, - { - success: false, - version: getCombinerVersion(), - error, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts index 291564b4468..377fc0f4fae 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts @@ -3,12 +3,10 @@ import { DomainRestrictedSignatureRequest, domainRestrictedSignatureRequestSchema, DomainRestrictedSignatureResponse, - DomainRestrictedSignatureResponseFailure, domainRestrictedSignatureResponseSchema, DomainRestrictedSignatureResponseSuccess, DomainSchema, DomainState, - ErrorType, getSignerEndpoint, send, SequentialDelayDomainStateSchema, @@ -19,7 +17,12 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { DomainCryptoClient } from '../../../common/crypto-clients/domain-crypto-client' import { CryptoSession } from '../../../common/crypto-session' -import { IO } from '../../../common/io' +import { + getKeyVersionInfo, + IO, + requestHasSupportedKeyVersion, + sendFailure, +} from '../../../common/io' import { getCombinerVersion } from '../../../config' export class DomainSignIO extends IO { @@ -43,15 +46,15 @@ export class DomainSignIO extends IO { if (!super.inputChecks(request, response)) { return null } - if (!this.requestHasSupportedKeyVersion(request, response.locals.logger)) { - this.sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) + if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { + sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) return null } if (!(await this.authenticate(request))) { - this.sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } - const keyVersionInfo = this.getKeyVersionInfo(request, response.locals.logger) + const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) return new CryptoSession( request, response, @@ -85,21 +88,4 @@ export class DomainSignIO extends IO { response.locals.logger ) } - - sendFailure( - error: ErrorType, - status: number, - response: Response - ) { - send( - response, - { - success: false, - version: getCombinerVersion(), - error, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index e8f607965f1..9d18ce7c532 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -1,6 +1,6 @@ import { ErrorMessage, PnpQuotaRequest } from '@celo/phone-number-privacy-common' import { CombineAction } from '../../../common/combine' -import { IO } from '../../../common/io' +import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { OdisConfig } from '../../../config' import { PnpSignerResponseLogger } from '../../services/log-responses' @@ -32,11 +32,10 @@ export class PnpQuotaAction extends CombineAction { session.logger.error(err, 'Error combining signer quota status responses') } } - this.io.sendFailure( + sendFailure( ErrorMessage.THRESHOLD_PNP_QUOTA_STATUS_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response, - session.logger + session.response ) } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts index 26801988b19..30ba555dbc5 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts @@ -1,14 +1,12 @@ import { ContractKit } from '@celo/contractkit' import { CombinerEndpoint, - ErrorType, getSignerEndpoint, hasValidAccountParam, isBodyReasonablySized, PnpQuotaRequest, PnpQuotaRequestSchema, PnpQuotaResponse, - PnpQuotaResponseFailure, PnpQuotaResponseSchema, PnpQuotaResponseSuccess, PnpQuotaStatus, @@ -19,7 +17,7 @@ import { import Logger from 'bunyan' import { Request, Response } from 'express' import * as t from 'io-ts' -import { IO } from '../../../common/io' +import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' @@ -42,10 +40,10 @@ export class PnpQuotaIO extends IO { return null } if (!(await this.authenticate(request, response.locals.logger))) { - this.sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } - const keyVersionInfo = this.getKeyVersionInfo(request, response.locals.logger) + const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) return new Session(request, response, keyVersionInfo) } @@ -93,17 +91,4 @@ export class PnpQuotaIO extends IO { response.locals.logger ) } - - sendFailure(error: ErrorType, status: number, response: Response) { - send( - response, - { - success: false, - version: getCombinerVersion(), - error, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts index 40457004555..8b710e701f2 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts @@ -1,7 +1,6 @@ import { ContractKit } from '@celo/contractkit' import { CombinerEndpoint, - ErrorType, getSignerEndpoint, hasValidAccountParam, hasValidBlindedPhoneNumberParam, @@ -12,7 +11,6 @@ import { SignMessageRequest, SignMessageRequestSchema, SignMessageResponse, - SignMessageResponseFailure, SignMessageResponseSchema, SignMessageResponseSuccess, WarningMessage, @@ -22,7 +20,12 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' import { CryptoSession } from '../../../common/crypto-session' -import { IO } from '../../../common/io' +import { + getKeyVersionInfo, + IO, + requestHasSupportedKeyVersion, + sendFailure, +} from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' @@ -45,15 +48,15 @@ export class PnpSignIO extends IO { if (!super.inputChecks(request, response)) { return null } - if (!this.requestHasSupportedKeyVersion(request, response.locals.logger)) { - this.sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) + if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { + sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) return null } if (!(await this.authenticate(request, response.locals.logger))) { - this.sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } - const keyVersionInfo = this.getKeyVersionInfo(request, response.locals.logger) + const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) return new CryptoSession( request, response, @@ -111,17 +114,4 @@ export class PnpSignIO extends IO { response.locals.logger ) } - - sendFailure(error: ErrorType, status: number, response: Response) { - send( - response, - { - success: false, - version: getCombinerVersion(), - error, - }, - status, - response.locals.logger - ) - } } From becfd045c932d8cc2971b442e5b804e0f285cd2e Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Mon, 21 Aug 2023 22:26:42 -0300 Subject: [PATCH 03/19] io: getSignerEndpoint lifted up to base class --- .../phone-number-privacy/combiner/src/common/io.ts | 6 +++--- .../combiner/src/domain/endpoints/disable/io.ts | 10 +++++----- .../combiner/src/domain/endpoints/quota/io.ts | 10 +++++----- .../combiner/src/domain/endpoints/sign/io.ts | 9 +++++---- .../combiner/src/pnp/endpoints/quota/io.ts | 8 ++------ .../combiner/src/pnp/endpoints/sign/io.ts | 8 ++------ 6 files changed, 22 insertions(+), 29 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 649cb791a8b..1dc36cbbe63 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -3,6 +3,7 @@ import { ErrorMessage, ErrorType, getRequestKeyVersion, + getSignerEndpoint, KeyVersionInfo, KEY_VERSION_HEADER, OdisRequest, @@ -30,12 +31,11 @@ export type SignerResponse = { } export abstract class IO { - abstract readonly endpoint: CombinerEndpoint - abstract readonly signerEndpoint: SignerEndpoint + readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) abstract readonly requestSchema: t.Type abstract readonly responseSchema: t.Type, OdisResponse, unknown> - constructor(readonly config: OdisConfig) {} + constructor(readonly config: OdisConfig, readonly endpoint: CombinerEndpoint) {} abstract init( request: Request<{}, {}, unknown>, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts index 998fefb7634..7c923e1df0c 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts @@ -9,10 +9,8 @@ import { DomainSchema, DomainState, ErrorType, - getSignerEndpoint, send, SequentialDelayDomainStateSchema, - SignerEndpoint, verifyDisableDomainRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' @@ -20,16 +18,18 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' export class DomainDisableIO extends IO { - readonly endpoint: CombinerEndpoint = CombinerEndpoint.DISABLE_DOMAIN - readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) readonly requestSchema: t.Type = disableDomainRequestSchema(DomainSchema) readonly responseSchema: t.Type = disableDomainResponseSchema(SequentialDelayDomainStateSchema) + constructor(config: OdisConfig) { + super(config, CombinerEndpoint.DISABLE_DOMAIN) + } + async init( request: Request<{}, {}, unknown>, response: Response diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts index 26f527ef9d5..ba71bb90624 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts @@ -7,11 +7,9 @@ import { DomainQuotaStatusResponseSuccess, DomainSchema, DomainState, - getSignerEndpoint, OdisResponse, send, SequentialDelayDomainStateSchema, - SignerEndpoint, verifyDomainQuotaStatusRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' @@ -19,16 +17,18 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' export class DomainQuotaIO extends IO { - readonly endpoint: CombinerEndpoint = CombinerEndpoint.DOMAIN_QUOTA_STATUS - readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) readonly requestSchema: t.Type = domainQuotaStatusRequestSchema(DomainSchema) readonly responseSchema: t.Type = domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) + constructor(config: OdisConfig) { + super(config, CombinerEndpoint.DOMAIN_QUOTA_STATUS) + } + async init( request: Request<{}, {}, unknown>, response: Response> diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts index 377fc0f4fae..b6f3810d89d 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts @@ -7,7 +7,6 @@ import { DomainRestrictedSignatureResponseSuccess, DomainSchema, DomainState, - getSignerEndpoint, send, SequentialDelayDomainStateSchema, verifyDomainRestrictedSignatureRequestAuthenticity, @@ -23,11 +22,9 @@ import { requestHasSupportedKeyVersion, sendFailure, } from '../../../common/io' -import { getCombinerVersion } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' export class DomainSignIO extends IO { - readonly endpoint = CombinerEndpoint.DOMAIN_SIGN - readonly signerEndpoint = getSignerEndpoint(this.endpoint) readonly requestSchema: t.Type< DomainRestrictedSignatureRequest, DomainRestrictedSignatureRequest, @@ -39,6 +36,10 @@ export class DomainSignIO extends IO { unknown > = domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema) + constructor(config: OdisConfig) { + super(config, CombinerEndpoint.DOMAIN_SIGN) + } + async init( request: Request<{}, {}, unknown>, response: Response diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts index 30ba555dbc5..8dacf843bb4 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts @@ -1,7 +1,6 @@ import { ContractKit } from '@celo/contractkit' import { CombinerEndpoint, - getSignerEndpoint, hasValidAccountParam, isBodyReasonablySized, PnpQuotaRequest, @@ -11,7 +10,6 @@ import { PnpQuotaResponseSuccess, PnpQuotaStatus, send, - SignerEndpoint, WarningMessage, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' @@ -22,14 +20,12 @@ import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' export class PnpQuotaIO extends IO { - readonly endpoint: CombinerEndpoint = CombinerEndpoint.PNP_QUOTA - readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) readonly requestSchema: t.Type = PnpQuotaRequestSchema readonly responseSchema: t.Type = PnpQuotaResponseSchema - constructor(readonly config: OdisConfig, readonly kit: ContractKit) { - super(config) + constructor(config: OdisConfig, readonly kit: ContractKit) { + super(config, CombinerEndpoint.PNP_QUOTA) } async init( diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts index 8b710e701f2..df9ee30e12e 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts @@ -1,13 +1,11 @@ import { ContractKit } from '@celo/contractkit' import { CombinerEndpoint, - getSignerEndpoint, hasValidAccountParam, hasValidBlindedPhoneNumberParam, isBodyReasonablySized, PnpQuotaStatus, send, - SignerEndpoint, SignMessageRequest, SignMessageRequestSchema, SignMessageResponse, @@ -30,15 +28,13 @@ import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' export class PnpSignIO extends IO { - readonly endpoint: CombinerEndpoint = CombinerEndpoint.PNP_SIGN - readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) readonly requestSchema: t.Type = SignMessageRequestSchema readonly responseSchema: t.Type = SignMessageResponseSchema - constructor(readonly config: OdisConfig, readonly kit: ContractKit) { - super(config) + constructor(config: OdisConfig, readonly kit: ContractKit) { + super(config, CombinerEndpoint.PNP_SIGN) } async init( From c5c44b1fcfdfd5c2374163ad25c79fcd887819d1 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Mon, 21 Aug 2023 22:45:31 -0300 Subject: [PATCH 04/19] Remove inputChecks and authenticate from IO --- .../combiner/src/common/handlers.ts | 14 +++++++- .../combiner/src/common/io.ts | 18 ----------- .../src/domain/endpoints/disable/io.ts | 10 +++--- .../combiner/src/domain/endpoints/quota/io.ts | 9 ++---- .../combiner/src/domain/endpoints/sign/io.ts | 16 ++++------ .../combiner/src/pnp/endpoints/quota/io.ts | 31 +++++++----------- .../combiner/src/pnp/endpoints/sign/io.ts | 32 ++++++------------- .../combiner/src/server.ts | 27 +++++++++++----- 8 files changed, 67 insertions(+), 90 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/handlers.ts b/packages/phone-number-privacy/combiner/src/common/handlers.ts index 3b407e071d0..2ffdf282988 100644 --- a/packages/phone-number-privacy/combiner/src/common/handlers.ts +++ b/packages/phone-number-privacy/combiner/src/common/handlers.ts @@ -1,4 +1,9 @@ -import { ErrorMessage, OdisRequest, OdisResponse } from '@celo/phone-number-privacy-common' +import { + ErrorMessage, + OdisRequest, + OdisResponse, + WarningMessage, +} from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' import { performance, PerformanceObserver } from 'perf_hooks' @@ -91,3 +96,10 @@ export function actionHandler(action: Action): Promise } } } + +export async function disabledHandler( + _: Request<{}, {}, R>, + response: Response, Locals> +): Promise { + sendFailure(WarningMessage.API_UNAVAILABLE, 503, response) +} diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 1dc36cbbe63..6686f440167 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -12,7 +12,6 @@ import { send, SignerEndpoint, SuccessResponse, - WarningMessage, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' @@ -42,8 +41,6 @@ export abstract class IO { response: Response> ): Promise | null> - abstract authenticate(request: Request<{}, {}, R>, logger?: Logger): Promise - abstract sendSuccess( status: number, response: Response>, @@ -65,21 +62,6 @@ export abstract class IO { } return res } - - protected inputChecks( - request: Request<{}, {}, unknown>, - response: Response> - ): request is Request<{}, {}, R> { - if (!this.config.enabled) { - sendFailure(WarningMessage.API_UNAVAILABLE, 503, response) - return false - } - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return false - } - return true - } } export function requestHasSupportedKeyVersion( diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts index 7c923e1df0c..e58bb7e9a60 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts @@ -34,10 +34,12 @@ export class DomainDisableIO extends IO { request: Request<{}, {}, unknown>, response: Response ): Promise | null> { - if (!super.inputChecks(request, response)) { + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) return null } - if (!(await this.authenticate(request))) { + + if (!verifyDisableDomainRequestAuthenticity(request.body)) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } @@ -48,10 +50,6 @@ export class DomainDisableIO extends IO { ) } - authenticate(request: Request<{}, {}, DisableDomainRequest>): Promise { - return Promise.resolve(verifyDisableDomainRequestAuthenticity(request.body)) - } - sendSuccess( status: number, response: Response, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts index ba71bb90624..98b42d361de 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts @@ -33,10 +33,11 @@ export class DomainQuotaIO extends IO { request: Request<{}, {}, unknown>, response: Response> ): Promise | null> { - if (!super.inputChecks(request, response)) { + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) return null } - if (!(await this.authenticate(request))) { + if (!verifyDomainQuotaStatusRequestAuthenticity(request.body)) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } @@ -44,10 +45,6 @@ export class DomainQuotaIO extends IO { return new Session(request, response, keyVersionInfo) } - authenticate(request: Request<{}, {}, DomainQuotaStatusRequest>): Promise { - return Promise.resolve(verifyDomainQuotaStatusRequestAuthenticity(request.body)) - } - sendSuccess( status: number, response: Response, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts index b6f3810d89d..5f83e6bb75c 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts @@ -44,14 +44,19 @@ export class DomainSignIO extends IO { request: Request<{}, {}, unknown>, response: Response ): Promise | null> { - if (!super.inputChecks(request, response)) { + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) return null } if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) return null } - if (!(await this.authenticate(request))) { + + // Note that signing requests may include a nonce for replay protection that will be checked by + // the signer, but is not checked here. As a result, requests that pass the authentication check + // here may still fail when sent to the signer. + if (!verifyDomainRestrictedSignatureRequestAuthenticity(request.body)) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } @@ -64,13 +69,6 @@ export class DomainSignIO extends IO { ) } - authenticate(request: Request<{}, {}, DomainRestrictedSignatureRequest>): Promise { - // Note that signing requests may include a nonce for replay protection that will be checked by - // the signer, but is not checked here. As a result, requests that pass the authentication check - // here may still fail when sent to the signer. - return Promise.resolve(verifyDomainRestrictedSignatureRequestAuthenticity(request.body)) - } - sendSuccess( status: number, response: Response, diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts index 8dacf843bb4..99a1044bb3c 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts @@ -1,6 +1,8 @@ import { ContractKit } from '@celo/contractkit' import { + authenticateUser, CombinerEndpoint, + DataEncryptionKeyFetcher, hasValidAccountParam, isBodyReasonablySized, PnpQuotaRequest, @@ -12,7 +14,6 @@ import { send, WarningMessage, } from '@celo/phone-number-privacy-common' -import Logger from 'bunyan' import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' @@ -24,7 +25,11 @@ export class PnpQuotaIO extends IO { readonly responseSchema: t.Type = PnpQuotaResponseSchema - constructor(config: OdisConfig, readonly kit: ContractKit) { + constructor( + config: OdisConfig, + readonly kit: ContractKit, + readonly dekFetcher: DataEncryptionKeyFetcher + ) { super(config, CombinerEndpoint.PNP_QUOTA) } @@ -32,10 +37,12 @@ export class PnpQuotaIO extends IO { request: Request<{}, {}, unknown>, response: Response ): Promise | null> { - if (!super.inputChecks(request, response)) { + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) return null } - if (!(await this.authenticate(request, response.locals.logger))) { + + if (!(await authenticateUser(request, response.locals.logger, this.dekFetcher))) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } @@ -53,22 +60,6 @@ export class PnpQuotaIO extends IO { ) } - async authenticate(request: Request<{}, {}, PnpQuotaRequest>, logger: Logger): Promise { - logger.debug({ url: request.url }) // just for ts not to make a fuzz - return Promise.resolve(true) - // return authenticateUser( - // request, - // logger, - // newContractKitFetcher( - // this.kit, - // logger, - // this.config.fullNodeTimeoutMs, - // this.config.fullNodeRetryCount, - // this.config.fullNodeRetryDelayMs - // ) - // ) - } - sendSuccess( status: number, response: Response, diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts index df9ee30e12e..1749f70b210 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts @@ -1,6 +1,8 @@ import { ContractKit } from '@celo/contractkit' import { + authenticateUser, CombinerEndpoint, + DataEncryptionKeyFetcher, hasValidAccountParam, hasValidBlindedPhoneNumberParam, isBodyReasonablySized, @@ -13,7 +15,6 @@ import { SignMessageResponseSuccess, WarningMessage, } from '@celo/phone-number-privacy-common' -import Logger from 'bunyan' import { Request, Response } from 'express' import * as t from 'io-ts' import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' @@ -33,7 +34,11 @@ export class PnpSignIO extends IO { readonly responseSchema: t.Type = SignMessageResponseSchema - constructor(config: OdisConfig, readonly kit: ContractKit) { + constructor( + config: OdisConfig, + readonly kit: ContractKit, + readonly dekFetcher: DataEncryptionKeyFetcher + ) { super(config, CombinerEndpoint.PNP_SIGN) } @@ -41,14 +46,15 @@ export class PnpSignIO extends IO { request: Request<{}, {}, unknown>, response: Response ): Promise | null> { - if (!super.inputChecks(request, response)) { + if (!this.validateClientRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) return null } if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) return null } - if (!(await this.authenticate(request, response.locals.logger))) { + if (!(await authenticateUser(request, response.locals.logger, this.dekFetcher))) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return null } @@ -72,24 +78,6 @@ export class PnpSignIO extends IO { ) } - async authenticate( - _request: Request<{}, {}, SignMessageRequest>, - _logger: Logger - ): Promise { - return Promise.resolve(true) - // return authenticateUser( - // request, - // logger, - // newContractKitFetcher( - // this.kit, - // logger, - // this.config.fullNodeTimeoutMs, - // this.config.fullNodeRetryCount, - // this.config.fullNodeRetryDelayMs - // ) - // ) - } - sendSuccess( status: number, response: Response, diff --git a/packages/phone-number-privacy/combiner/src/server.ts b/packages/phone-number-privacy/combiner/src/server.ts index 0df173688b7..fbd0b9121d6 100644 --- a/packages/phone-number-privacy/combiner/src/server.ts +++ b/packages/phone-number-privacy/combiner/src/server.ts @@ -3,6 +3,7 @@ import { CombinerEndpoint, KEY_VERSION_HEADER, loggerMiddleware, + newContractKitFetcher, OdisRequest, rootLogger, } from '@celo/phone-number-privacy-common' @@ -11,6 +12,7 @@ import express, { RequestHandler } from 'express' import { actionHandler, catchErrorHandler, + disabledHandler, meteringHandler, PromiseHandler, } from './common/handlers' @@ -61,13 +63,21 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { }) }) + const dekFetcher = newContractKitFetcher( + kit, + logger, + config.phoneNumberPrivacy.fullNodeTimeoutMs, + config.phoneNumberPrivacy.fullNodeRetryCount, + config.phoneNumberPrivacy.fullNodeRetryDelayMs + ) + const pnpThresholdStateService = new PnpThresholdStateService() const pnpQuota = actionHandler( new PnpQuotaAction( config.phoneNumberPrivacy, pnpThresholdStateService, - new PnpQuotaIO(config.phoneNumberPrivacy, kit) + new PnpQuotaIO(config.phoneNumberPrivacy, kit, dekFetcher) ) ) @@ -75,7 +85,7 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { new PnpSignAction( config.phoneNumberPrivacy, pnpThresholdStateService, - new PnpSignIO(config.phoneNumberPrivacy, kit) + new PnpSignIO(config.phoneNumberPrivacy, kit, dekFetcher) ) ) @@ -105,17 +115,18 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { ) ) - app.post(CombinerEndpoint.PNP_QUOTA, createHandler(pnpQuota)) - app.post(CombinerEndpoint.PNP_SIGN, createHandler(pnpSign)) - app.post(CombinerEndpoint.DOMAIN_QUOTA_STATUS, createHandler(domainQuota)) - app.post(CombinerEndpoint.DOMAIN_SIGN, createHandler(domainSign)) - app.post(CombinerEndpoint.DISABLE_DOMAIN, createHandler(domainDisable)) + app.post(CombinerEndpoint.PNP_QUOTA, createHandler(config.phoneNumberPrivacy.enabled, pnpQuota)) + app.post(CombinerEndpoint.PNP_SIGN, createHandler(config.phoneNumberPrivacy.enabled, pnpSign)) + app.post(CombinerEndpoint.DOMAIN_QUOTA_STATUS, createHandler(config.domains.enabled, domainQuota)) + app.post(CombinerEndpoint.DOMAIN_SIGN, createHandler(config.domains.enabled, domainSign)) + app.post(CombinerEndpoint.DISABLE_DOMAIN, createHandler(config.domains.enabled, domainDisable)) return app } export function createHandler( + enabled: boolean, handler: PromiseHandler ): PromiseHandler { - return meteringHandler(catchErrorHandler(handler)) + return meteringHandler(catchErrorHandler(enabled ? handler : disabledHandler)) } From 27b84ad7898bd77c8b320eb9a5b1ff0be3ef0e44 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Tue, 22 Aug 2023 17:03:25 -0300 Subject: [PATCH 05/19] wip: kill Combiner and Signer actions --- .../combiner/src/common/combine.ts | 302 +++++++----------- .../common/crypto-clients/crypto-client.ts | 1 + .../combiner/src/common/io.ts | 34 +- .../combiner/src/common/session.ts | 6 +- .../combiner/src/common/sign.ts | 77 ----- .../src/domain/endpoints/disable/action.ts | 50 ++- .../src/domain/endpoints/disable/io.ts | 32 +- .../src/domain/endpoints/quota/action.ts | 49 ++- .../combiner/src/domain/endpoints/quota/io.ts | 27 +- .../src/domain/endpoints/sign/action.ts | 91 +++++- .../combiner/src/domain/endpoints/sign/io.ts | 31 +- .../src/pnp/endpoints/quota/action.ts | 49 ++- .../combiner/src/pnp/endpoints/quota/io.ts | 24 +- .../combiner/src/pnp/endpoints/sign/action.ts | 94 +++++- .../combiner/src/pnp/endpoints/sign/io.ts | 29 +- 15 files changed, 424 insertions(+), 472 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 9f543a4a5b9..b78a58d5d9c 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -2,14 +2,13 @@ import { ErrorMessage, OdisRequest, OdisResponse, + responseHasExpectedKeyVersion, WarningMessage, } from '@celo/phone-number-privacy-common' -import { Locals, Request, Response } from 'express' -import { Response as FetchResponse } from 'node-fetch' +import Logger from 'bunyan' +import * as t from 'io-ts' import { PerformanceObserver } from 'perf_hooks' -import { OdisConfig } from '../config' -import { Action } from './action' -import { fetchSignerResponseWithFallback, IO } from './io' +import { fetchSignerResponseWithFallback } from './io' import { Session } from './session' export interface Signer { @@ -17,195 +16,138 @@ export interface Signer { fallbackUrl?: string } -export abstract class CombineAction implements Action { - private readonly signers: Signer[] +export async function thresholdCallToSigners( + logger: Logger, + signers: Signer[], + endpoint: string, + session: Session, + keyVersion: number | null, + requestTimeoutMS: number, + responseSchema: t.Type, OdisResponse, unknown>, + processResult: (res: OdisResponse) => Promise = (_) => Promise.resolve(false) +) { + const obs = new PerformanceObserver((list) => { + // Possible race condition here: if multiple signers take exactly the same + // amount of time, the PerformanceObserver callback may be called twice with + // both entries present. Node 12 doesn't allow for entries to be deleted by name, + // and eliminating the race condition requires a more significant redesign of + // the measurement code. + // This is only used for monitoring purposes, so a rare + // duplicate latency measure for the signer should have minimal impact. + list.getEntries().forEach((entry) => { + logger.info({ latency: entry, signer: entry.name }, 'Signer response latency measured') + }) + }) + obs.observe({ entryTypes: ['measure'], buffered: false }) - public constructor(readonly config: OdisConfig, readonly io: IO) { - this.signers = JSON.parse(config.odisServices.signers) - } + const manualAbort = new AbortController() + const timeoutSignal = AbortSignal.timeout(requestTimeoutMS) + const abortSignal = (AbortSignal as any).any([manualAbort.signal, timeoutSignal]) as AbortSignal - abstract combine(session: Session): void + const failedSigners: string[] = [] + const errorCodes: Map = new Map() - async perform( - _request: Request<{}, {}, R>, - _response: Response, Locals>, - session: Session - ) { - await this.distribute(session) - this.combine(session) - } + const requiredThreshold = session.keyVersionInfo.threshold + + // Forward request to signers + // An unexpected error in handling the result for one signer should not + // block a threshold of correct responses, but should be logged. + await Promise.all( + signers.map(async (signer) => { + try { + const signerFetchResult = await fetchSignerResponseWithFallback( + signer, + endpoint, + session, + logger, + abortSignal + ) + + if (!signerFetchResult.ok) { + // Tracking failed request count via signer url prevents + // double counting the same failed request by mistake + failedSigners.push(signer.url) + errorCodes.set( + signerFetchResult.status, + (errorCodes.get(signerFetchResult.status) ?? 0) + 1 + ) - async distribute(session: Session): Promise { - const obs = new PerformanceObserver((list) => { - // Possible race condition here: if multiple signers take exactly the same - // amount of time, the PerformanceObserver callback may be called twice with - // both entries present. Node 12 doesn't allow for entries to be deleted by name, - // and eliminating the race condition requires a more significant redesign of - // the measurement code. - // This is only used for monitoring purposes, so a rare - // duplicate latency measure for the signer should have minimal impact. - list.getEntries().forEach((entry) => { + if (signers.length - failedSigners.length < requiredThreshold) { + logger.warn('Not possible to reach a threshold of signer responses. Failing fast') + manualAbort.abort() + } + return + } + + // if given key version, check that + if ( + keyVersion != null && + !responseHasExpectedKeyVersion(signerFetchResult, keyVersion, logger) + ) { + throw new Error(ErrorMessage.INVALID_KEY_VERSION_RESPONSE) + } + + const data: any = await signerFetchResult.json() session.logger.info( - { latency: entry, signer: entry.name }, - 'Signer response latency measured' + { signer, res: data, status: signerFetchResult.status }, + `received 'OK' response from signer` ) - }) - }) - obs.observe({ entryTypes: ['measure'], buffered: false }) - - const timeout = setTimeout(() => { - session.timedOut = true - session.abort.abort() - }, this.config.odisServices.timeoutMilliSeconds) - - // Forward request to signers - // An unexpected error in handling the result for one signer should not - // block a threshold of correct responses, but should be logged. - await Promise.all( - this.signers.map(async (signer) => { - try { - await this.forwardToSigner(signer, session) - } catch (err) { - session.logger.error({ - signer: signer.url, - message: 'Unexpected error caught while distributing request to signer', - err, - }) + + const odisResponse: OdisResponse = parseSchema(responseSchema, data, logger) + if (!odisResponse.success) { + logger.error( + { err: odisResponse.error, signer: signer.url }, + `Signer request to failed with 'OK' status` + ) + throw new Error(ErrorMessage.SIGNER_RESPONSE_FAILED_WITH_OK_STATUS) } - }) - ) - // TODO Resolve race condition where a session can both receive a successful - // response in time and be aborted - - clearTimeout(timeout) - // DO NOT call performance.clearMarks() as this also deletes marks used to - // measure e2e combiner latency. - obs.disconnect() - } - private async forwardToSigner(signer: Signer, session: Session): Promise { - let signerFetchResult: FetchResponse | undefined - try { - signerFetchResult = await fetchSignerResponseWithFallback( - signer, - this.io.signerEndpoint, - session - ) - session.logger.info({ - message: 'Received signerFetchResult', - signer: signer.url, - status: signerFetchResult.status, - }) - } catch (err) { - session.logger.debug({ err, signer: signer.url, message: 'signer request failure' }) - if (err instanceof Error && err.name === 'AbortError' && session.abort.signal.aborted) { - if (session.timedOut) { - session.logger.error({ signer }, ErrorMessage.TIMEOUT_FROM_SIGNER) + if (await processResult(odisResponse)) { + // we already have enough responses + manualAbort.abort() + } + } catch (err) { + if (isTimeoutError(err)) { + logger.error({ signer }, ErrorMessage.TIMEOUT_FROM_SIGNER) + } else if (isAbortError(err)) { + logger.info({ signer }, WarningMessage.CANCELLED_REQUEST_TO_SIGNER) } else { - session.logger.info({ signer }, WarningMessage.CANCELLED_REQUEST_TO_SIGNER) + // Logging the err & message simultaneously fails to log the message in some cases + logger.error({ signer }, ErrorMessage.SIGNER_REQUEST_ERROR) + logger.error({ signer, err }) + + // Tracking failed request count via signer url prevents + // double counting the same failed request by mistake + failedSigners.push(signer.url) + if (signers.length - failedSigners.length < requiredThreshold) { + logger.warn('Not possible to reach a threshold of signer responses. Failing fast') + manualAbort.abort() + } + + // TODO (mcortesi) doesn't seem we need to fail at first error + // throw err } - } else { - // Logging the err & message simultaneously fails to log the message in some cases - session.logger.error({ signer }, ErrorMessage.SIGNER_REQUEST_ERROR) - session.logger.error({ signer, err }) } - } - return this.handleFetchResult(signer, session, signerFetchResult) - } + }) + ) - private async handleFetchResult( - signer: Signer, - session: Session, - signerFetchResult?: FetchResponse - ): Promise { - if (signerFetchResult?.ok) { - try { - // Throws if response is not actually successful - await this.receiveSuccess(signerFetchResult, signer.url, session) - return - } catch (err) { - session.logger.error(err) - } - } - if (signerFetchResult) { - session.logger.info({ - message: 'Received signerFetchResult on unsuccessful signer response', - res: await signerFetchResult.text(), - status: signerFetchResult.status, - signer: signer.url, - }) - } - return this.addFailureToSession(signer, signerFetchResult?.status, session) - } + // DO NOT call performance.clearMarks() as this also deletes marks used to + // measure e2e combiner latency. + obs.disconnect() +} - protected async receiveSuccess( - signerFetchResult: FetchResponse, - url: string, - session: Session - ): Promise> { - if (!signerFetchResult.ok) { - throw new Error(`Implementation Error: receiveSuccess should only receive 'OK' responses`) - } - const { status } = signerFetchResult - const data: string = await signerFetchResult.text() - session.logger.info({ signer: url, res: data, status }, `received 'OK' response from signer`) - const signerResponse: OdisResponse = this.io.validateSignerResponse( - data, - url, - session.logger - ) - if (!signerResponse.success) { - session.logger.error( - { err: signerResponse.error, signer: url, status }, - `Signer request to ${url + this.io.signerEndpoint} failed with 'OK' status` - ) - throw new Error(ErrorMessage.SIGNER_RESPONSE_FAILED_WITH_OK_STATUS) - } - session.logger.info({ signer: url }, `Signer request successful`) - session.responses.push({ url, res: signerResponse, status }) - return signerResponse +function parseSchema(schema: t.Type, data: unknown, logger: Logger): T { + if (!schema.is(data)) { + logger.error({ data }, `Malformed schema`) + throw new Error(ErrorMessage.INVALID_SIGNER_RESPONSE) } + return data +} - private addFailureToSession(signer: Signer, errorCode: number | undefined, session: Session) { - // Tracking failed request count via signer url prevents - // double counting the same failed request by mistake - session.failedSigners.add(signer.url) - session.logger.warn( - `Received failure from ${session.failedSigners.size}/${this.signers.length} signers` - ) - if (errorCode) { - session.incrementErrorCodeCount(errorCode) - } - const { threshold } = session.keyVersionInfo - if (this.signers.length - session.failedSigners.size < threshold) { - session.logger.warn('Not possible to reach a threshold of signer responses. Failing fast') - session.abort.abort() - } - } +function isTimeoutError(err: unknown) { + return err instanceof Error && err.name === 'TimeoutError' } -// async function callSigner(logger: Logger, signer: Signer) { -// let signerFetchResult: FetchResponse | undefined -// try { -// signerFetchResult = await this.io.fetchSignerResponseWithFallback(signer, session) -// logger.info({ -// message: 'Received signerFetchResult', -// signer: signer.url, -// status: signerFetchResult.status, -// }) -// } catch (err) { -// logger.debug({ err, signer: signer.url, message: 'signer request failure' }) -// if (err instanceof Error && err.name === 'AbortError' && session.abort.signal.aborted) { -// if (session.timedOut) { -// logger.error({ signer }, ErrorMessage.TIMEOUT_FROM_SIGNER) -// } else { -// logger.info({ signer }, WarningMessage.CANCELLED_REQUEST_TO_SIGNER) -// } -// } else { -// // Logging the err & message simultaneously fails to log the message in some cases -// logger.error({ signer }, ErrorMessage.SIGNER_REQUEST_ERROR) -// logger.error({ signer, err }) -// } -// } -// return this.handleFetchResult(signer, session, signerFetchResult) -// } +function isAbortError(err: unknown) { + return err instanceof Error && err.name === 'AbortError' +} diff --git a/packages/phone-number-privacy/combiner/src/common/crypto-clients/crypto-client.ts b/packages/phone-number-privacy/combiner/src/common/crypto-clients/crypto-client.ts index c57783a3051..42658d64f86 100644 --- a/packages/phone-number-privacy/combiner/src/common/crypto-clients/crypto-client.ts +++ b/packages/phone-number-privacy/combiner/src/common/crypto-clients/crypto-client.ts @@ -15,6 +15,7 @@ export abstract class CryptoClient { /** * Returns true if the number of valid signatures is enough to perform a combination */ + // TODO (mcortesi) remove public hasSufficientSignatures(): boolean { return this.allSignaturesLength >= this.keyVersionInfo.threshold } diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 6686f440167..cc5945b8fa0 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -1,6 +1,5 @@ import { CombinerEndpoint, - ErrorMessage, ErrorType, getRequestKeyVersion, getSignerEndpoint, @@ -11,7 +10,6 @@ import { requestHasValidKeyVersion, send, SignerEndpoint, - SuccessResponse, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' @@ -32,7 +30,6 @@ export type SignerResponse = { export abstract class IO { readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) abstract readonly requestSchema: t.Type - abstract readonly responseSchema: t.Type, OdisResponse, unknown> constructor(readonly config: OdisConfig, readonly endpoint: CombinerEndpoint) {} @@ -41,27 +38,9 @@ export abstract class IO { response: Response> ): Promise | null> - abstract sendSuccess( - status: number, - response: Response>, - ...args: unknown[] - ): void - validateClientRequest(request: Request<{}, {}, unknown>): request is Request<{}, {}, R> { return this.requestSchema.is(request.body) } - - validateSignerResponse(data: string, url: string, logger: Logger): OdisResponse { - const res: unknown = JSON.parse(data) - if (!this.responseSchema.is(res)) { - logger.error( - { data, signer: url }, - `Signer request to ${url + this.signerEndpoint} returned malformed response` - ) - throw new Error(ErrorMessage.INVALID_SIGNER_RESPONSE) - } - return res - } } export function requestHasSupportedKeyVersion( @@ -105,9 +84,11 @@ export function getKeyVersionInfo( export async function fetchSignerResponseWithFallback( signer: Signer, signerEndpoint: string, - session: Session + session: Session, + logger: Logger, + abortSignal: AbortSignal ): Promise { - const { request, abort } = session + const { request } = session async function fetchSignerResponse(url: string): Promise { // prettier-ignore @@ -123,16 +104,15 @@ export async function fetchSignerResponseWithFallback( [KEY_VERSION_HEADER]: session.keyVersionInfo.keyVersion.toString() }, body: JSON.stringify(request.body), - // @ts-ignore: missing property `reason` - signal: abort.signal, + signal: abortSignal, }) } return measureTime(signer.url + signerEndpoint, () => fetchSignerResponse(signer.url + signerEndpoint).catch((err) => { - session.logger.error({ url: signer.url, error: err }, `Signer failed with primary url`) + logger.error({ url: signer.url, error: err }, `Signer failed with primary url`) if (signer.fallbackUrl) { - session.logger.warn({ url: signer.fallbackUrl }, `Using fallback url to call signer`) + logger.warn({ url: signer.fallbackUrl }, `Using fallback url to call signer`) return fetchSignerResponse(signer.fallbackUrl + signerEndpoint) } else { throw err diff --git a/packages/phone-number-privacy/combiner/src/common/session.ts b/packages/phone-number-privacy/combiner/src/common/session.ts index 26bb76f6703..ffd8603cf6f 100644 --- a/packages/phone-number-privacy/combiner/src/common/session.ts +++ b/packages/phone-number-privacy/combiner/src/common/session.ts @@ -4,15 +4,14 @@ import { OdisRequest, OdisResponse, } from '@celo/phone-number-privacy-common' -import AbortController from 'abort-controller' import Logger from 'bunyan' import { Request, Response } from 'express' import { SignerResponse } from './io' export class Session { - public timedOut: boolean = false + // public timedOut: boolean = false readonly logger: Logger - readonly abort: AbortController = new AbortController() + // readonly abort: AbortController = new AbortController() readonly failedSigners: Set = new Set() readonly errorCodes: Map = new Map() readonly responses: Array> = new Array>() @@ -52,4 +51,3 @@ export class Session { return maxErrorCode > 0 ? maxErrorCode : null } } -;`` diff --git a/packages/phone-number-privacy/combiner/src/common/sign.ts b/packages/phone-number-privacy/combiner/src/common/sign.ts index 17ab7bfef6d..86ab2a49bd4 100644 --- a/packages/phone-number-privacy/combiner/src/common/sign.ts +++ b/packages/phone-number-privacy/combiner/src/common/sign.ts @@ -1,18 +1,9 @@ import { DomainRestrictedSignatureRequest, - ErrorMessage, - ErrorType, - OdisResponse, - responseHasExpectedKeyVersion, SignMessageRequest, } from '@celo/phone-number-privacy-common' -import { Response as FetchResponse } from 'node-fetch' -import { OdisConfig } from '../config' import { DomainThresholdStateService } from '../domain/services/threshold-state' import { PnpThresholdStateService } from '../pnp/services/threshold-state' -import { CombineAction } from './combine' -import { CryptoSession } from './crypto-session' -import { IO, sendFailure } from './io' // prettier-ignore export type OdisSignatureRequest = @@ -24,71 +15,3 @@ export type ThresholdStateService = R extends Si : never | R extends DomainRestrictedSignatureRequest ? DomainThresholdStateService : never - -// tslint:disable-next-line: max-classes-per-file -export abstract class SignAction extends CombineAction { - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: ThresholdStateService, - readonly io: IO - ) { - super(config, io) - } - - // Throws if response is not actually successful - protected async receiveSuccess( - signerResponse: FetchResponse, - url: string, - session: CryptoSession - ): Promise> { - const { keyVersion } = session.keyVersionInfo - - // TODO(2.0.0, deployment) consider this while doing deployment. Signers should be updated before the combiner is - if (!responseHasExpectedKeyVersion(signerResponse, keyVersion, session.logger)) { - throw new Error(ErrorMessage.INVALID_KEY_VERSION_RESPONSE) - } - - const res = await super.receiveSuccess(signerResponse, url, session) - - if (res.success) { - const signatureAdditionStart = Date.now() - session.crypto.addSignature({ url, signature: res.signature }) - session.logger.info( - { - signer: url, - hasSufficientSignatures: session.crypto.hasSufficientSignatures(), - additionLatency: Date.now() - signatureAdditionStart, - }, - 'Added signature' - ) - // Send response immediately once we cross threshold - // BLS threshold signatures can be combined without all partial signatures - if (session.crypto.hasSufficientSignatures()) { - try { - session.crypto.combineBlindedSignatureShares( - this.parseBlindedMessage(session.request.body), - session.logger - ) - // Close outstanding requests - session.abort.abort() - } catch (err) { - // One or more signatures failed verification and were discarded. - session.logger.info('Error caught in receiveSuccess') - session.logger.info(err) - // Continue to collect signatures. - } - } - } - return res - } - - protected handleMissingSignatures(session: CryptoSession) { - const errorCode = session.getMajorityErrorCode() ?? 500 - const error = this.errorCodeToError(errorCode) - sendFailure(error, errorCode, session.response) - } - - protected abstract errorCodeToError(errorCode: number): ErrorType - - protected abstract parseBlindedMessage(req: OdisSignatureRequest): string -} diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index 5d9794dee72..053296f3557 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -1,28 +1,60 @@ -import { DisableDomainRequest, ErrorMessage } from '@celo/phone-number-privacy-common' -import { CombineAction } from '../../../common/combine' +import { + DisableDomainRequest, + disableDomainResponseSchema, + ErrorMessage, + OdisResponse, + send, + SequentialDelayDomainStateSchema, +} from '@celo/phone-number-privacy-common' +import { Request, Response } from 'express' +import { Action } from '../../../common/action' +import { Signer, thresholdCallToSigners } from '../../../common/combine' +import { Locals } from '../../../common/handlers' import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' import { DomainThresholdStateService } from '../../services/threshold-state' -export class DomainDisableAction extends CombineAction { +export class DomainDisableAction implements Action { readonly responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() - + protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) constructor( readonly config: OdisConfig, readonly thresholdStateService: DomainThresholdStateService, readonly io: IO + ) {} + + async perform( + _request: Request<{}, {}, DisableDomainRequest>, + response: Response, Locals>, + session: Session ) { - super(config, io) - } + await thresholdCallToSigners( + response.locals.logger, + this.signers, + this.io.signerEndpoint, + session, + null, + this.config.odisServices.timeoutMilliSeconds, + disableDomainResponseSchema(SequentialDelayDomainStateSchema) + ) - combine(session: Session): void { this.responseLogger.logResponseDiscrepancies(session) try { const disableDomainStatus = this.thresholdStateService.findThresholdDomainState(session) if (disableDomainStatus.disabled) { - this.io.sendSuccess(200, session.response, disableDomainStatus) + send( + response, + { + success: true, + version: getCombinerVersion(), + status: disableDomainStatus, + }, + 200, + response.locals.logger + ) + return } } catch (err) { diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts index e58bb7e9a60..a1348e6a267 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts @@ -3,14 +3,7 @@ import { DisableDomainRequest, disableDomainRequestSchema, DisableDomainResponse, - DisableDomainResponseFailure, - disableDomainResponseSchema, - DisableDomainResponseSuccess, DomainSchema, - DomainState, - ErrorType, - send, - SequentialDelayDomainStateSchema, verifyDisableDomainRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' @@ -18,13 +11,11 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion, OdisConfig } from '../../../config' +import { OdisConfig } from '../../../config' export class DomainDisableIO extends IO { readonly requestSchema: t.Type = disableDomainRequestSchema(DomainSchema) - readonly responseSchema: t.Type = - disableDomainResponseSchema(SequentialDelayDomainStateSchema) constructor(config: OdisConfig) { super(config, CombinerEndpoint.DISABLE_DOMAIN) @@ -49,25 +40,4 @@ export class DomainDisableIO extends IO { getKeyVersionInfo(request, this.config, response.locals.logger) ) } - - sendSuccess( - status: number, - response: Response, - domainState: DomainState - ) { - send( - response, - { - success: true, - version: getCombinerVersion(), - status: domainState, - }, - status, - response.locals.logger - ) - } - - sendFailure(error: ErrorType, status: number, response: Response) { - sendFailure(error, status, response) - } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index 829129af791..c10d9463fdb 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -1,29 +1,60 @@ -import { DomainQuotaStatusRequest, ErrorMessage } from '@celo/phone-number-privacy-common' -import { CombineAction } from '../../../common/combine' +import { + DomainQuotaStatusRequest, + domainQuotaStatusResponseSchema, + ErrorMessage, + OdisResponse, + send, + SequentialDelayDomainStateSchema, +} from '@celo/phone-number-privacy-common' +import { Request, Response } from 'express' +import { Action } from '../../../common/action' +import { Signer, thresholdCallToSigners } from '../../../common/combine' +import { Locals } from '../../../common/handlers' import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' import { DomainThresholdStateService } from '../../services/threshold-state' -export class DomainQuotaAction extends CombineAction { +export class DomainQuotaAction implements Action { readonly responseLogger = new DomainSignerResponseLogger() - + protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) constructor( readonly config: OdisConfig, readonly thresholdStateService: DomainThresholdStateService, readonly io: IO + ) {} + + async perform( + _request: Request<{}, {}, DomainQuotaStatusRequest>, + response: Response, Locals>, + session: Session ) { - super(config, io) - } + await thresholdCallToSigners( + response.locals.logger, + this.signers, + this.io.signerEndpoint, + session, + null, + this.config.odisServices.timeoutMilliSeconds, + domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) + ) - combine(session: Session): void { this.responseLogger.logResponseDiscrepancies(session) const { threshold } = session.keyVersionInfo if (session.responses.length >= threshold) { try { const domainQuotaStatus = this.thresholdStateService.findThresholdDomainState(session) - this.io.sendSuccess(200, session.response, domainQuotaStatus) + send( + response, + { + success: true, + version: getCombinerVersion(), + status: domainQuotaStatus, + }, + 200, + response.locals.logger + ) return } catch (err) { session.logger.error(err, 'Error combining signer quota status responses') diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts index 98b42d361de..a3654236922 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts @@ -2,14 +2,8 @@ import { CombinerEndpoint, DomainQuotaStatusRequest, domainQuotaStatusRequestSchema, - DomainQuotaStatusResponse, - domainQuotaStatusResponseSchema, - DomainQuotaStatusResponseSuccess, DomainSchema, - DomainState, OdisResponse, - send, - SequentialDelayDomainStateSchema, verifyDomainQuotaStatusRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' @@ -17,13 +11,11 @@ import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion, OdisConfig } from '../../../config' +import { OdisConfig } from '../../../config' export class DomainQuotaIO extends IO { readonly requestSchema: t.Type = domainQuotaStatusRequestSchema(DomainSchema) - readonly responseSchema: t.Type = - domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) constructor(config: OdisConfig) { super(config, CombinerEndpoint.DOMAIN_QUOTA_STATUS) @@ -44,21 +36,4 @@ export class DomainQuotaIO extends IO { const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) return new Session(request, response, keyVersionInfo) } - - sendSuccess( - status: number, - response: Response, - domainState: DomainState - ) { - send( - response, - { - success: true, - version: getCombinerVersion(), - status: domainState, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index e7f74b36d21..37c7548df3e 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -1,17 +1,85 @@ import { DomainRestrictedSignatureRequest, + domainRestrictedSignatureResponseSchema, ErrorMessage, ErrorType, + OdisResponse, + send, + SequentialDelayDomainStateSchema, WarningMessage, } from '@celo/phone-number-privacy-common' +import { Request, Response } from 'express' +import { Signer, thresholdCallToSigners } from '../../../common/combine' import { CryptoSession } from '../../../common/crypto-session' -import { SignAction } from '../../../common/sign' +import { Locals } from '../../../common/handlers' +import { IO, sendFailure } from '../../../common/io' +import { ThresholdStateService } from '../../../common/sign' +import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' -export class DomainSignAction extends SignAction { +export class DomainSignAction { readonly responseLogger = new DomainSignerResponseLogger() - combine(session: CryptoSession): void { + protected readonly signers: Signer[] + constructor( + readonly config: OdisConfig, + readonly thresholdStateService: ThresholdStateService, + readonly io: IO + ) { + this.signers = JSON.parse(config.odisServices.signers) + } + + async perform( + _request: Request<{}, {}, DomainRestrictedSignatureRequest>, + response: Response, Locals>, + session: CryptoSession + ) { + const processRequest = async ( + res: OdisResponse + ): Promise => { + session.crypto.addSignature({ url: 'TODO: remove', signature: res.signature }) + // const signatureAdditionStart = Date.now() + + // session.logger.info( + // { + // signer: url, + // hasSufficientSignatures: session.crypto.x(), + // additionLatency: Date.now() - signatureAdditionStart, + // }, + // 'Added signature' + // ) + + // Send response immediately once we cross threshold + // BLS threshold signatures can be combined without all partial signatures + if (session.crypto.hasSufficientSignatures()) { + try { + session.crypto.combineBlindedSignatureShares( + this.parseBlindedMessage(session.request.body), + session.logger + ) + // Close outstanding requests + return true + } catch (err) { + // One or more signatures failed verification and were discarded. + session.logger.info('Error caught in receiveSuccess') + session.logger.info(err) + // Continue to collect signatures. + } + } + return false + } + + await thresholdCallToSigners( + response.locals.logger, + this.signers, + this.io.signerEndpoint, + session, + session.keyVersionInfo.keyVersion, + this.config.odisServices.timeoutMilliSeconds, + domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), + processRequest + ) + this.responseLogger.logResponseDiscrepancies(session) if (session.crypto.hasSufficientSignatures()) { @@ -21,11 +89,16 @@ export class DomainSignAction extends SignAction { readonly requestSchema: t.Type< @@ -30,11 +25,6 @@ export class DomainSignIO extends IO { DomainRestrictedSignatureRequest, unknown > = domainRestrictedSignatureRequestSchema(DomainSchema) - readonly responseSchema: t.Type< - DomainRestrictedSignatureResponse, - DomainRestrictedSignatureResponse, - unknown - > = domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema) constructor(config: OdisConfig) { super(config, CombinerEndpoint.DOMAIN_SIGN) @@ -68,23 +58,4 @@ export class DomainSignIO extends IO { new DomainCryptoClient(keyVersionInfo) ) } - - sendSuccess( - status: number, - response: Response, - signature: string, - domainState: DomainState - ) { - send( - response, - { - success: true, - version: getCombinerVersion(), - signature, - status: domainState, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index 9d18ce7c532..574e7735973 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -1,23 +1,45 @@ -import { ErrorMessage, PnpQuotaRequest } from '@celo/phone-number-privacy-common' -import { CombineAction } from '../../../common/combine' +import { + ErrorMessage, + OdisResponse, + PnpQuotaRequest, + PnpQuotaResponseSchema, + send, +} from '@celo/phone-number-privacy-common' +import { Request, Response } from 'express' +import { Action } from '../../../common/action' +import { Signer, thresholdCallToSigners } from '../../../common/combine' +import { Locals } from '../../../common/handlers' import { IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' +import { getCombinerVersion, OdisConfig } from '../../../config' import { PnpSignerResponseLogger } from '../../services/log-responses' import { PnpThresholdStateService } from '../../services/threshold-state' -export class PnpQuotaAction extends CombineAction { +export class PnpQuotaAction implements Action { readonly responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() + protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) constructor( readonly config: OdisConfig, readonly thresholdStateService: PnpThresholdStateService, readonly io: IO + ) {} + + async perform( + _request: Request<{}, {}, PnpQuotaRequest>, + response: Response, Locals>, + session: Session ) { - super(config, io) - } + await thresholdCallToSigners( + response.locals.logger, + this.signers, + this.io.signerEndpoint, + session, + null, + this.config.odisServices.timeoutMilliSeconds, + PnpQuotaResponseSchema + ) - async combine(session: Session): Promise { this.responseLogger.logResponseDiscrepancies(session) this.responseLogger.logFailOpenResponses(session) @@ -26,7 +48,18 @@ export class PnpQuotaAction extends CombineAction { if (session.responses.length >= threshold) { try { const quotaStatus = this.thresholdStateService.findCombinerQuotaState(session) - this.io.sendSuccess(200, session.response, quotaStatus, session.warnings) + send( + response, + { + success: true, + version: getCombinerVersion(), + ...quotaStatus, + warnings: session.warnings, + }, + 200, + response.locals.logger + ) + return } catch (err) { session.logger.error(err, 'Error combining signer quota status responses') diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts index 99a1044bb3c..4556c46f744 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts @@ -9,16 +9,13 @@ import { PnpQuotaRequestSchema, PnpQuotaResponse, PnpQuotaResponseSchema, - PnpQuotaResponseSuccess, - PnpQuotaStatus, - send, WarningMessage, } from '@celo/phone-number-privacy-common' import { Request, Response } from 'express' import * as t from 'io-ts' import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion, OdisConfig } from '../../../config' +import { OdisConfig } from '../../../config' export class PnpQuotaIO extends IO { readonly requestSchema: t.Type = PnpQuotaRequestSchema @@ -59,23 +56,4 @@ export class PnpQuotaIO extends IO { isBodyReasonablySized(request.body) ) } - - sendSuccess( - status: number, - response: Response, - quotaStatus: PnpQuotaStatus, - warnings: string[] - ) { - send( - response, - { - success: true, - version: getCombinerVersion(), - ...quotaStatus, - warnings, - }, - status, - response.locals.logger - ) - } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index e75ddb8e727..a8999fedfc8 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -1,43 +1,113 @@ import { ErrorMessage, ErrorType, + OdisResponse, + send, SignMessageRequest, + SignMessageResponseSchema, WarningMessage, } from '@celo/phone-number-privacy-common' +import { Request, Response } from 'express' +import { Signer, thresholdCallToSigners } from '../../../common/combine' import { CryptoSession } from '../../../common/crypto-session' -import { SignAction } from '../../../common/sign' +import { Locals } from '../../../common/handlers' +import { IO, sendFailure } from '../../../common/io' +import { ThresholdStateService } from '../../../common/sign' +import { getCombinerVersion, OdisConfig } from '../../../config' import { PnpSignerResponseLogger } from '../../services/log-responses' -export class PnpSignAction extends SignAction { +export class PnpSignAction { readonly responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() - combine(session: CryptoSession): void { + protected readonly signers: Signer[] + constructor( + readonly config: OdisConfig, + readonly thresholdStateService: ThresholdStateService, + readonly io: IO + ) { + this.signers = JSON.parse(config.odisServices.signers) + } + + async perform( + request: Request<{}, {}, SignMessageRequest>, + response: Response, Locals>, + session: CryptoSession + ) { + const logger = response.locals.logger + const processRequest = async (result: OdisResponse): Promise => { + session.crypto.addSignature({ url: 'TODO: remove', signature: result.signature }) + // const signatureAdditionStart = Date.now() + + // logger.info( + // { + // signer: url, + // hasSufficientSignatures: session.crypto.x(), + // additionLatency: Date.now() - signatureAdditionStart, + // }, + // 'Added signature' + // ) + + // Send response immediately once we cross threshold + // BLS threshold signatures can be combined without all partial signatures + if (session.crypto.hasSufficientSignatures()) { + try { + session.crypto.combineBlindedSignatureShares(request.body.blindedQueryPhoneNumber, logger) + // Close outstanding requests + return true + } catch (err) { + // One or more signatures failed verification and were discarded. + logger.info('Error caught in receiveSuccess') + logger.info(err) + // Continue to collect signatures. + } + } + return false + } + + await thresholdCallToSigners( + logger, + this.signers, + this.io.signerEndpoint, + session, + session.keyVersionInfo.keyVersion, + this.config.odisServices.timeoutMilliSeconds, + SignMessageResponseSchema, + processRequest + ) + this.responseLogger.logResponseDiscrepancies(session) this.responseLogger.logFailOpenResponses(session) if (session.crypto.hasSufficientSignatures()) { try { const combinedSignature = session.crypto.combineBlindedSignatureShares( - this.parseBlindedMessage(session.request.body), - session.logger + session.request.body.blindedQueryPhoneNumber, + logger ) const quotaStatus = this.thresholdStateService.findCombinerQuotaState(session) - return this.io.sendSuccess( + return send( + response, + { + success: true, + version: getCombinerVersion(), + signature: combinedSignature, + ...quotaStatus, + warnings: session.warnings, + }, 200, - session.response, - combinedSignature, - quotaStatus, - session.warnings + logger ) } catch (error) { // May fail upon combining signatures if too many sigs are invalid // Fallback to handleMissingSignatures - session.logger.error(error) + logger.error(error) } } - this.handleMissingSignatures(session) + const errorCode = session.getMajorityErrorCode() ?? 500 + const error = this.errorCodeToError(errorCode) + sendFailure(error, errorCode, session.response) } protected parseBlindedMessage(req: SignMessageRequest): string { diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts index 1749f70b210..075dcfb4bfc 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts @@ -6,13 +6,9 @@ import { hasValidAccountParam, hasValidBlindedPhoneNumberParam, isBodyReasonablySized, - PnpQuotaStatus, - send, SignMessageRequest, SignMessageRequestSchema, SignMessageResponse, - SignMessageResponseSchema, - SignMessageResponseSuccess, WarningMessage, } from '@celo/phone-number-privacy-common' import { Request, Response } from 'express' @@ -26,13 +22,11 @@ import { sendFailure, } from '../../../common/io' import { Session } from '../../../common/session' -import { getCombinerVersion, OdisConfig } from '../../../config' +import { OdisConfig } from '../../../config' export class PnpSignIO extends IO { readonly requestSchema: t.Type = SignMessageRequestSchema - readonly responseSchema: t.Type = - SignMessageResponseSchema constructor( config: OdisConfig, @@ -77,25 +71,4 @@ export class PnpSignIO extends IO { isBodyReasonablySized(request.body) ) } - - sendSuccess( - status: number, - response: Response, - signature: string, - quotaStatus: PnpQuotaStatus, - warnings: string[] - ) { - send( - response, - { - success: true, - version: getCombinerVersion(), - signature, - ...quotaStatus, - warnings, - }, - status, - response.locals.logger - ) - } } From c46d97d705eca27fa5a221569104520aba800e35 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Tue, 22 Aug 2023 20:22:19 -0300 Subject: [PATCH 06/19] use handler function (remove IO and Action) --- .../combiner/src/common/combine.ts | 13 +- .../combiner/src/common/handlers.ts | 18 --- .../combiner/src/common/io.ts | 7 +- .../combiner/src/common/session.ts | 16 +-- .../src/domain/endpoints/disable/action.ts | 66 ++++++---- .../src/domain/endpoints/disable/io.ts | 43 ------- .../src/domain/endpoints/quota/action.ts | 63 +++++---- .../combiner/src/domain/endpoints/quota/io.ts | 39 ------ .../src/domain/endpoints/sign/action.ts | 120 ++++++++++-------- .../combiner/src/domain/endpoints/sign/io.ts | 61 --------- .../src/pnp/endpoints/quota/action.ts | 79 ++++++++---- .../combiner/src/pnp/endpoints/quota/io.ts | 59 --------- .../combiner/src/pnp/endpoints/sign/action.ts | 107 ++++++++++------ .../combiner/src/pnp/endpoints/sign/io.ts | 74 ----------- .../combiner/src/server.ts | 74 +++++------ .../test/unit/domain-response-logger.test.ts | 2 +- .../test/unit/domain-threshold-state.test.ts | 2 +- .../test/unit/pnp-response-logger.test.ts | 6 +- .../test/unit/pnp-threshold-state.test.ts | 7 +- 19 files changed, 318 insertions(+), 538 deletions(-) delete mode 100644 packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts delete mode 100644 packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts delete mode 100644 packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts delete mode 100644 packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts delete mode 100644 packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index b78a58d5d9c..6ba49f440cf 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -1,15 +1,16 @@ import { ErrorMessage, + KeyVersionInfo, OdisRequest, OdisResponse, responseHasExpectedKeyVersion, WarningMessage, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' +import { Request } from 'express' import * as t from 'io-ts' import { PerformanceObserver } from 'perf_hooks' import { fetchSignerResponseWithFallback } from './io' -import { Session } from './session' export interface Signer { url: string @@ -20,7 +21,8 @@ export async function thresholdCallToSigners( logger: Logger, signers: Signer[], endpoint: string, - session: Session, + request: Request, + keyVersionInfo: KeyVersionInfo, keyVersion: number | null, requestTimeoutMS: number, responseSchema: t.Type, OdisResponse, unknown>, @@ -47,7 +49,7 @@ export async function thresholdCallToSigners( const failedSigners: string[] = [] const errorCodes: Map = new Map() - const requiredThreshold = session.keyVersionInfo.threshold + const requiredThreshold = keyVersionInfo.threshold // Forward request to signers // An unexpected error in handling the result for one signer should not @@ -58,7 +60,8 @@ export async function thresholdCallToSigners( const signerFetchResult = await fetchSignerResponseWithFallback( signer, endpoint, - session, + keyVersionInfo.keyVersion, + request, logger, abortSignal ) @@ -88,7 +91,7 @@ export async function thresholdCallToSigners( } const data: any = await signerFetchResult.json() - session.logger.info( + logger.info( { signer, res: data, status: signerFetchResult.status }, `received 'OK' response from signer` ) diff --git a/packages/phone-number-privacy/combiner/src/common/handlers.ts b/packages/phone-number-privacy/combiner/src/common/handlers.ts index 2ffdf282988..f13846b4486 100644 --- a/packages/phone-number-privacy/combiner/src/common/handlers.ts +++ b/packages/phone-number-privacy/combiner/src/common/handlers.ts @@ -7,7 +7,6 @@ import { import Logger from 'bunyan' import { Request, Response } from 'express' import { performance, PerformanceObserver } from 'perf_hooks' -import { Action } from './action' import { sendFailure } from './io' export interface Locals { @@ -80,23 +79,6 @@ export function meteringHandler( } } -export function actionHandler(action: Action): PromiseHandler { - return async (request, response) => { - try { - const session = await action.io.init(request, response) - if (session) { - await action.perform(request, response, session) - } - } catch (err) { - response.locals.logger.error( - { error: err }, - `Unknown error in handler for ${action.io.endpoint}` - ) - sendFailure(ErrorMessage.UNKNOWN_ERROR, 500, response) - } - } -} - export async function disabledHandler( _: Request<{}, {}, R>, response: Response, Locals> diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index cc5945b8fa0..4801863664e 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -84,12 +84,11 @@ export function getKeyVersionInfo( export async function fetchSignerResponseWithFallback( signer: Signer, signerEndpoint: string, - session: Session, + keyVersion: number, + request: Request, logger: Logger, abortSignal: AbortSignal ): Promise { - const { request } = session - async function fetchSignerResponse(url: string): Promise { // prettier-ignore return fetch(url, { @@ -101,7 +100,7 @@ export async function fetchSignerResponseWithFallback( ...(request.headers.authorization ? { Authorization: request.headers.authorization } : {}), // Forward requested keyVersion if provided by client, otherwise use default keyVersion. // This will be ignored for non-signing requests. - [KEY_VERSION_HEADER]: session.keyVersionInfo.keyVersion.toString() + [KEY_VERSION_HEADER]: keyVersion.toString() }, body: JSON.stringify(request.body), signal: abortSignal, diff --git a/packages/phone-number-privacy/combiner/src/common/session.ts b/packages/phone-number-privacy/combiner/src/common/session.ts index ffd8603cf6f..42b24c245ed 100644 --- a/packages/phone-number-privacy/combiner/src/common/session.ts +++ b/packages/phone-number-privacy/combiner/src/common/session.ts @@ -5,30 +5,20 @@ import { OdisResponse, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' -import { Request, Response } from 'express' +import { Response } from 'express' import { SignerResponse } from './io' export class Session { - // public timedOut: boolean = false readonly logger: Logger - // readonly abort: AbortController = new AbortController() - readonly failedSigners: Set = new Set() + readonly errorCodes: Map = new Map() readonly responses: Array> = new Array>() readonly warnings: string[] = [] - public constructor( - readonly request: Request<{}, {}, R>, - readonly response: Response>, - readonly keyVersionInfo: KeyVersionInfo - ) { + public constructor(response: Response>, readonly keyVersionInfo: KeyVersionInfo) { this.logger = response.locals.logger } - incrementErrorCodeCount(errorCode: number) { - this.errorCodes.set(errorCode, (this.errorCodes.get(errorCode) ?? 0) + 1) - } - getMajorityErrorCode(): number | null { const uniqueErrorCount = Array.from(this.errorCodes.keys()).length if (uniqueErrorCount > 1) { diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index 053296f3557..41d8f2730e2 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -1,48 +1,59 @@ import { + CombinerEndpoint, DisableDomainRequest, + disableDomainRequestSchema, disableDomainResponseSchema, + DomainSchema, ErrorMessage, - OdisResponse, send, SequentialDelayDomainStateSchema, + verifyDisableDomainRequestAuthenticity, + WarningMessage, } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import { Action } from '../../../common/action' import { Signer, thresholdCallToSigners } from '../../../common/combine' -import { Locals } from '../../../common/handlers' -import { IO, sendFailure } from '../../../common/io' +import { PromiseHandler } from '../../../common/handlers' +import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' import { DomainThresholdStateService } from '../../services/threshold-state' -export class DomainDisableAction implements Action { - readonly responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() - protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: DomainThresholdStateService, - readonly io: IO - ) {} +export function createDisableDomainHandler( + signers: Signer[], + config: OdisConfig, + thresholdStateService: DomainThresholdStateService +): PromiseHandler { + const requestSchema = disableDomainRequestSchema(DomainSchema) + const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() + const signerEndpoint = CombinerEndpoint.DISABLE_DOMAIN + return async (request, response) => { + if (!requestSchema.is(request.body)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return + } + + if (!verifyDisableDomainRequestAuthenticity(request.body)) { + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + return + } + + const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) + const session = new Session(response, keyVersionInfo) - async perform( - _request: Request<{}, {}, DisableDomainRequest>, - response: Response, Locals>, - session: Session - ) { await thresholdCallToSigners( response.locals.logger, - this.signers, - this.io.signerEndpoint, - session, + signers, + signerEndpoint, + request, + keyVersionInfo, null, - this.config.odisServices.timeoutMilliSeconds, + config.odisServices.timeoutMilliSeconds, disableDomainResponseSchema(SequentialDelayDomainStateSchema) ) - this.responseLogger.logResponseDiscrepancies(session) + responseLogger.logResponseDiscrepancies(session) try { - const disableDomainStatus = this.thresholdStateService.findThresholdDomainState(session) + const disableDomainStatus = thresholdStateService.findThresholdDomainState(session) if (disableDomainStatus.disabled) { send( response, @@ -58,13 +69,16 @@ export class DomainDisableAction implements Action { return } } catch (err) { - session.logger.error({ err }, 'Error combining signer disable domain status responses') + response.locals.logger.error( + { err }, + 'Error combining signer disable domain status responses' + ) } sendFailure( ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response + response ) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts deleted file mode 100644 index a1348e6a267..00000000000 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/io.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { - CombinerEndpoint, - DisableDomainRequest, - disableDomainRequestSchema, - DisableDomainResponse, - DomainSchema, - verifyDisableDomainRequestAuthenticity, - WarningMessage, -} from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import * as t from 'io-ts' -import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' - -export class DomainDisableIO extends IO { - readonly requestSchema: t.Type = - disableDomainRequestSchema(DomainSchema) - - constructor(config: OdisConfig) { - super(config, CombinerEndpoint.DISABLE_DOMAIN) - } - - async init( - request: Request<{}, {}, unknown>, - response: Response - ): Promise | null> { - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return null - } - - if (!verifyDisableDomainRequestAuthenticity(request.body)) { - sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) - return null - } - return new Session( - request, - response, - getKeyVersionInfo(request, this.config, response.locals.logger) - ) - } -} diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index c10d9463fdb..cd6cba55cd7 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -1,50 +1,61 @@ import { + CombinerEndpoint, DomainQuotaStatusRequest, + domainQuotaStatusRequestSchema, domainQuotaStatusResponseSchema, + DomainSchema, ErrorMessage, - OdisResponse, send, SequentialDelayDomainStateSchema, + verifyDisableDomainRequestAuthenticity, + WarningMessage, } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import { Action } from '../../../common/action' import { Signer, thresholdCallToSigners } from '../../../common/combine' -import { Locals } from '../../../common/handlers' -import { IO, sendFailure } from '../../../common/io' +import { PromiseHandler } from '../../../common/handlers' +import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' import { DomainThresholdStateService } from '../../services/threshold-state' -export class DomainQuotaAction implements Action { - readonly responseLogger = new DomainSignerResponseLogger() - protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: DomainThresholdStateService, - readonly io: IO - ) {} +export function createDomainQuotaHandler( + signers: Signer[], + config: OdisConfig, + thresholdStateService: DomainThresholdStateService +): PromiseHandler { + const requestSchema = domainQuotaStatusRequestSchema(DomainSchema) + const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() + const signerEndpoint = CombinerEndpoint.DOMAIN_QUOTA_STATUS + return async (request, response) => { + if (!requestSchema.is(request.body)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return + } + + if (!verifyDisableDomainRequestAuthenticity(request.body)) { + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + return + } + + const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) + const session = new Session(response, keyVersionInfo) - async perform( - _request: Request<{}, {}, DomainQuotaStatusRequest>, - response: Response, Locals>, - session: Session - ) { await thresholdCallToSigners( response.locals.logger, - this.signers, - this.io.signerEndpoint, - session, + signers, + signerEndpoint, + request, + session.keyVersionInfo, null, - this.config.odisServices.timeoutMilliSeconds, + config.odisServices.timeoutMilliSeconds, domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) ) - this.responseLogger.logResponseDiscrepancies(session) + responseLogger.logResponseDiscrepancies(session) const { threshold } = session.keyVersionInfo if (session.responses.length >= threshold) { try { - const domainQuotaStatus = this.thresholdStateService.findThresholdDomainState(session) + const domainQuotaStatus = thresholdStateService.findThresholdDomainState(session) send( response, { @@ -57,13 +68,13 @@ export class DomainQuotaAction implements Action { ) return } catch (err) { - session.logger.error(err, 'Error combining signer quota status responses') + response.locals.logger.error(err, 'Error combining signer quota status responses') } } sendFailure( ErrorMessage.THRESHOLD_DOMAIN_QUOTA_STATUS_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response + response ) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts deleted file mode 100644 index a3654236922..00000000000 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/io.ts +++ /dev/null @@ -1,39 +0,0 @@ -import { - CombinerEndpoint, - DomainQuotaStatusRequest, - domainQuotaStatusRequestSchema, - DomainSchema, - OdisResponse, - verifyDomainQuotaStatusRequestAuthenticity, - WarningMessage, -} from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import * as t from 'io-ts' -import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' - -export class DomainQuotaIO extends IO { - readonly requestSchema: t.Type = - domainQuotaStatusRequestSchema(DomainSchema) - - constructor(config: OdisConfig) { - super(config, CombinerEndpoint.DOMAIN_QUOTA_STATUS) - } - - async init( - request: Request<{}, {}, unknown>, - response: Response> - ): Promise | null> { - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return null - } - if (!verifyDomainQuotaStatusRequestAuthenticity(request.body)) { - sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) - return null - } - const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) - return new Session(request, response, keyVersionInfo) - } -} diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index 37c7548df3e..12afa03510c 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -1,46 +1,70 @@ import { + CombinerEndpoint, DomainRestrictedSignatureRequest, + domainRestrictedSignatureRequestSchema, domainRestrictedSignatureResponseSchema, + DomainSchema, ErrorMessage, ErrorType, OdisResponse, send, SequentialDelayDomainStateSchema, + verifyDisableDomainRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' +import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' +import { DomainCryptoClient } from '../../../common/crypto-clients/domain-crypto-client' import { CryptoSession } from '../../../common/crypto-session' -import { Locals } from '../../../common/handlers' -import { IO, sendFailure } from '../../../common/io' -import { ThresholdStateService } from '../../../common/sign' +import { PromiseHandler } from '../../../common/handlers' +import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' import { getCombinerVersion, OdisConfig } from '../../../config' import { DomainSignerResponseLogger } from '../../services/log-responses' +import { DomainThresholdStateService } from '../../services/threshold-state' -export class DomainSignAction { - readonly responseLogger = new DomainSignerResponseLogger() +export function createDomainSignHandler( + signers: Signer[], + config: OdisConfig, + thresholdStateService: DomainThresholdStateService +): PromiseHandler { + const requestSchema = domainRestrictedSignatureRequestSchema(DomainSchema) + const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() + const signerEndpoint = CombinerEndpoint.DOMAIN_SIGN + return async (request, response) => { + if (!requestSchema.is(request.body)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return + } + if (!requestHasSupportedKeyVersion(request, config, response.locals.logger)) { + sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) + return + } - protected readonly signers: Signer[] - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: ThresholdStateService, - readonly io: IO - ) { - this.signers = JSON.parse(config.odisServices.signers) - } + // Note that signing requests may include a nonce for replay protection that will be checked by + // the signer, but is not checked here. As a result, requests that pass the authentication check + // here may still fail when sent to the signer. + if (!verifyDisableDomainRequestAuthenticity(request.body)) { + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + return + } + + const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) + const session = new CryptoSession( + request, + response, + keyVersionInfo, + new DomainCryptoClient(keyVersionInfo) + ) - async perform( - _request: Request<{}, {}, DomainRestrictedSignatureRequest>, - response: Response, Locals>, - session: CryptoSession - ) { + const logger = response.locals.logger const processRequest = async ( res: OdisResponse ): Promise => { + assert(res.success) session.crypto.addSignature({ url: 'TODO: remove', signature: res.signature }) // const signatureAdditionStart = Date.now() - // session.logger.info( + // logger.info( // { // signer: url, // hasSufficientSignatures: session.crypto.x(), @@ -53,16 +77,13 @@ export class DomainSignAction { // BLS threshold signatures can be combined without all partial signatures if (session.crypto.hasSufficientSignatures()) { try { - session.crypto.combineBlindedSignatureShares( - this.parseBlindedMessage(session.request.body), - session.logger - ) + session.crypto.combineBlindedSignatureShares(request.body.blindedMessage, logger) // Close outstanding requests return true } catch (err) { // One or more signatures failed verification and were discarded. - session.logger.info('Error caught in receiveSuccess') - session.logger.info(err) + logger.info('Error caught in receiveSuccess') + logger.info(err) // Continue to collect signatures. } } @@ -71,22 +92,23 @@ export class DomainSignAction { await thresholdCallToSigners( response.locals.logger, - this.signers, - this.io.signerEndpoint, - session, + signers, + signerEndpoint, + request, + session.keyVersionInfo, session.keyVersionInfo.keyVersion, - this.config.odisServices.timeoutMilliSeconds, + config.odisServices.timeoutMilliSeconds, domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), processRequest ) - this.responseLogger.logResponseDiscrepancies(session) + responseLogger.logResponseDiscrepancies(session) if (session.crypto.hasSufficientSignatures()) { try { const combinedSignature = session.crypto.combineBlindedSignatureShares( - this.parseBlindedMessage(session.request.body), - session.logger + request.body.blindedMessage, + logger ) return send( @@ -95,37 +117,33 @@ export class DomainSignAction { success: true, version: getCombinerVersion(), signature: combinedSignature, - status: this.thresholdStateService.findThresholdDomainState(session), + status: thresholdStateService.findThresholdDomainState(session), }, 200, response.locals.logger ) } catch (err) { // May fail upon combining signatures if too many sigs are invalid - session.logger.error('Combining signatures failed in combine') - session.logger.error(err) + logger.error('Combining signatures failed in combine') + logger.error(err) // Fallback to handleMissingSignatures } } const errorCode = session.getMajorityErrorCode() ?? 500 - const error = this.errorCodeToError(errorCode) + const error = errorCodeToError(errorCode) sendFailure(error, errorCode, session.response) } +} - protected parseBlindedMessage(req: DomainRestrictedSignatureRequest): string { - return req.blindedMessage - } - - protected errorCodeToError(errorCode: number): ErrorType { - switch (errorCode) { - case 429: - return WarningMessage.EXCEEDED_QUOTA - case 401: - // Authentication is checked in the combiner, but invalid nonces are passed through - return WarningMessage.INVALID_NONCE - default: - return ErrorMessage.NOT_ENOUGH_PARTIAL_SIGNATURES - } +function errorCodeToError(errorCode: number): ErrorType { + switch (errorCode) { + case 429: + return WarningMessage.EXCEEDED_QUOTA + case 401: + // Authentication is checked in the combiner, but invalid nonces are passed through + return WarningMessage.INVALID_NONCE + default: + return ErrorMessage.NOT_ENOUGH_PARTIAL_SIGNATURES } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts deleted file mode 100644 index 169e5ba6aeb..00000000000 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/io.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { - CombinerEndpoint, - DomainRestrictedSignatureRequest, - domainRestrictedSignatureRequestSchema, - DomainRestrictedSignatureResponse, - DomainSchema, - verifyDomainRestrictedSignatureRequestAuthenticity, - WarningMessage, -} from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import * as t from 'io-ts' -import { DomainCryptoClient } from '../../../common/crypto-clients/domain-crypto-client' -import { CryptoSession } from '../../../common/crypto-session' -import { - getKeyVersionInfo, - IO, - requestHasSupportedKeyVersion, - sendFailure, -} from '../../../common/io' -import { OdisConfig } from '../../../config' - -export class DomainSignIO extends IO { - readonly requestSchema: t.Type< - DomainRestrictedSignatureRequest, - DomainRestrictedSignatureRequest, - unknown - > = domainRestrictedSignatureRequestSchema(DomainSchema) - - constructor(config: OdisConfig) { - super(config, CombinerEndpoint.DOMAIN_SIGN) - } - - async init( - request: Request<{}, {}, unknown>, - response: Response - ): Promise | null> { - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return null - } - if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { - sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) - return null - } - - // Note that signing requests may include a nonce for replay protection that will be checked by - // the signer, but is not checked here. As a result, requests that pass the authentication check - // here may still fail when sent to the signer. - if (!verifyDomainRestrictedSignatureRequestAuthenticity(request.body)) { - sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) - return null - } - const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) - return new CryptoSession( - request, - response, - keyVersionInfo, - new DomainCryptoClient(keyVersionInfo) - ) - } -} diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index 574e7735973..a5ca8f553fc 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -1,53 +1,66 @@ import { + authenticateUser, + CombinerEndpoint, + DataEncryptionKeyFetcher, ErrorMessage, - OdisResponse, + hasValidAccountParam, + isBodyReasonablySized, PnpQuotaRequest, + PnpQuotaRequestSchema, PnpQuotaResponseSchema, send, + WarningMessage, } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import { Action } from '../../../common/action' +import { Request } from 'express' import { Signer, thresholdCallToSigners } from '../../../common/combine' -import { Locals } from '../../../common/handlers' -import { IO, sendFailure } from '../../../common/io' +import { PromiseHandler } from '../../../common/handlers' +import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' import { PnpSignerResponseLogger } from '../../services/log-responses' import { PnpThresholdStateService } from '../../services/threshold-state' -export class PnpQuotaAction implements Action { - readonly responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() - protected readonly signers: Signer[] = JSON.parse(this.config.odisServices.signers) +export function createPnpQuotaHandler( + signers: Signer[], + config: OdisConfig, + thresholdStateService: PnpThresholdStateService, + dekFetcher: DataEncryptionKeyFetcher +): PromiseHandler { + const signerEndpoint = CombinerEndpoint.PNP_QUOTA + const responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() + return async (request, response) => { + const logger = response.locals.logger + if (!validateRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return + } - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: PnpThresholdStateService, - readonly io: IO - ) {} + if (!(await authenticateUser(request, logger, dekFetcher))) { + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + return + } + const keyVersionInfo = getKeyVersionInfo(request, config, logger) + const session = new Session(response, keyVersionInfo) - async perform( - _request: Request<{}, {}, PnpQuotaRequest>, - response: Response, Locals>, - session: Session - ) { await thresholdCallToSigners( - response.locals.logger, - this.signers, - this.io.signerEndpoint, - session, + logger, + signers, + signerEndpoint, + request, + session.keyVersionInfo, null, - this.config.odisServices.timeoutMilliSeconds, + config.odisServices.timeoutMilliSeconds, PnpQuotaResponseSchema ) - this.responseLogger.logResponseDiscrepancies(session) - this.responseLogger.logFailOpenResponses(session) + responseLogger.logResponseDiscrepancies(session) + responseLogger.logFailOpenResponses(session) const { threshold } = session.keyVersionInfo if (session.responses.length >= threshold) { try { - const quotaStatus = this.thresholdStateService.findCombinerQuotaState(session) + const quotaStatus = thresholdStateService.findCombinerQuotaState(session) send( response, { @@ -57,7 +70,7 @@ export class PnpQuotaAction implements Action { warnings: session.warnings, }, 200, - response.locals.logger + logger ) return @@ -68,7 +81,17 @@ export class PnpQuotaAction implements Action { sendFailure( ErrorMessage.THRESHOLD_PNP_QUOTA_STATUS_FAILURE, session.getMajorityErrorCode() ?? 500, - session.response + response ) } } + +function validateRequest( + request: Request<{}, {}, unknown> +): request is Request<{}, {}, PnpQuotaRequest> { + return ( + PnpQuotaRequestSchema.is(request.body) && + hasValidAccountParam(request.body) && + isBodyReasonablySized(request.body) + ) +} diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts deleted file mode 100644 index 4556c46f744..00000000000 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/io.ts +++ /dev/null @@ -1,59 +0,0 @@ -import { ContractKit } from '@celo/contractkit' -import { - authenticateUser, - CombinerEndpoint, - DataEncryptionKeyFetcher, - hasValidAccountParam, - isBodyReasonablySized, - PnpQuotaRequest, - PnpQuotaRequestSchema, - PnpQuotaResponse, - PnpQuotaResponseSchema, - WarningMessage, -} from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import * as t from 'io-ts' -import { getKeyVersionInfo, IO, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' - -export class PnpQuotaIO extends IO { - readonly requestSchema: t.Type = PnpQuotaRequestSchema - readonly responseSchema: t.Type = - PnpQuotaResponseSchema - - constructor( - config: OdisConfig, - readonly kit: ContractKit, - readonly dekFetcher: DataEncryptionKeyFetcher - ) { - super(config, CombinerEndpoint.PNP_QUOTA) - } - - async init( - request: Request<{}, {}, unknown>, - response: Response - ): Promise | null> { - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return null - } - - if (!(await authenticateUser(request, response.locals.logger, this.dekFetcher))) { - sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) - return null - } - const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) - return new Session(request, response, keyVersionInfo) - } - - validateClientRequest( - request: Request<{}, {}, unknown> - ): request is Request<{}, {}, PnpQuotaRequest> { - return ( - super.validateClientRequest(request) && - hasValidAccountParam(request.body) && - isBodyReasonablySized(request.body) - ) - } -} diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index a8999fedfc8..3455e0003b5 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -1,40 +1,65 @@ import { + authenticateUser, + CombinerEndpoint, + DataEncryptionKeyFetcher, ErrorMessage, ErrorType, + hasValidAccountParam, + hasValidBlindedPhoneNumberParam, + isBodyReasonablySized, OdisResponse, send, SignMessageRequest, + SignMessageRequestSchema, SignMessageResponseSchema, WarningMessage, } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' + +import { Request } from 'express' +import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' +import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' import { CryptoSession } from '../../../common/crypto-session' -import { Locals } from '../../../common/handlers' -import { IO, sendFailure } from '../../../common/io' +import { PromiseHandler } from '../../../common/handlers' +import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' import { ThresholdStateService } from '../../../common/sign' import { getCombinerVersion, OdisConfig } from '../../../config' import { PnpSignerResponseLogger } from '../../services/log-responses' -export class PnpSignAction { - readonly responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() +export function createPnpSignHandler( + signers: Signer[], + config: OdisConfig, + thresholdStateService: ThresholdStateService, + dekFetcher: DataEncryptionKeyFetcher +): PromiseHandler { + const signerEndpoint = CombinerEndpoint.PNP_SIGN + const responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() + return async (request, response) => { + const logger = response.locals.logger + if (!validateRequest(request)) { + sendFailure(WarningMessage.INVALID_INPUT, 400, response) + return + } - protected readonly signers: Signer[] - constructor( - readonly config: OdisConfig, - readonly thresholdStateService: ThresholdStateService, - readonly io: IO - ) { - this.signers = JSON.parse(config.odisServices.signers) - } + if (!requestHasSupportedKeyVersion(request, config, response.locals.logger)) { + sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) + return + } + + if (!(await authenticateUser(request, logger, dekFetcher))) { + sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) + return + } + const keyVersionInfo = getKeyVersionInfo(request, config, logger) + const session = new CryptoSession( + request, + response, + keyVersionInfo, + new BLSCryptographyClient(keyVersionInfo) + ) - async perform( - request: Request<{}, {}, SignMessageRequest>, - response: Response, Locals>, - session: CryptoSession - ) { - const logger = response.locals.logger const processRequest = async (result: OdisResponse): Promise => { + assert(result.success) session.crypto.addSignature({ url: 'TODO: remove', signature: result.signature }) // const signatureAdditionStart = Date.now() @@ -66,17 +91,18 @@ export class PnpSignAction { await thresholdCallToSigners( logger, - this.signers, - this.io.signerEndpoint, - session, + signers, + signerEndpoint, + request, + session.keyVersionInfo, session.keyVersionInfo.keyVersion, - this.config.odisServices.timeoutMilliSeconds, + config.odisServices.timeoutMilliSeconds, SignMessageResponseSchema, processRequest ) - this.responseLogger.logResponseDiscrepancies(session) - this.responseLogger.logFailOpenResponses(session) + responseLogger.logResponseDiscrepancies(session) + responseLogger.logFailOpenResponses(session) if (session.crypto.hasSufficientSignatures()) { try { @@ -85,7 +111,7 @@ export class PnpSignAction { logger ) - const quotaStatus = this.thresholdStateService.findCombinerQuotaState(session) + const quotaStatus = thresholdStateService.findCombinerQuotaState(session) return send( response, { @@ -106,20 +132,27 @@ export class PnpSignAction { } const errorCode = session.getMajorityErrorCode() ?? 500 - const error = this.errorCodeToError(errorCode) + const error = errorCodeToError(errorCode) sendFailure(error, errorCode, session.response) } +} - protected parseBlindedMessage(req: SignMessageRequest): string { - return req.blindedQueryPhoneNumber - } +function validateRequest( + request: Request<{}, {}, unknown> +): request is Request<{}, {}, SignMessageRequest> { + return ( + SignMessageRequestSchema.is(request.body) && + hasValidAccountParam(request.body) && + hasValidBlindedPhoneNumberParam(request.body) && + isBodyReasonablySized(request.body) + ) +} - protected errorCodeToError(errorCode: number): ErrorType { - switch (errorCode) { - case 403: - return WarningMessage.EXCEEDED_QUOTA - default: - return ErrorMessage.NOT_ENOUGH_PARTIAL_SIGNATURES - } +function errorCodeToError(errorCode: number): ErrorType { + switch (errorCode) { + case 403: + return WarningMessage.EXCEEDED_QUOTA + default: + return ErrorMessage.NOT_ENOUGH_PARTIAL_SIGNATURES } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts deleted file mode 100644 index 075dcfb4bfc..00000000000 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/io.ts +++ /dev/null @@ -1,74 +0,0 @@ -import { ContractKit } from '@celo/contractkit' -import { - authenticateUser, - CombinerEndpoint, - DataEncryptionKeyFetcher, - hasValidAccountParam, - hasValidBlindedPhoneNumberParam, - isBodyReasonablySized, - SignMessageRequest, - SignMessageRequestSchema, - SignMessageResponse, - WarningMessage, -} from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import * as t from 'io-ts' -import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' -import { CryptoSession } from '../../../common/crypto-session' -import { - getKeyVersionInfo, - IO, - requestHasSupportedKeyVersion, - sendFailure, -} from '../../../common/io' -import { Session } from '../../../common/session' -import { OdisConfig } from '../../../config' - -export class PnpSignIO extends IO { - readonly requestSchema: t.Type = - SignMessageRequestSchema - - constructor( - config: OdisConfig, - readonly kit: ContractKit, - readonly dekFetcher: DataEncryptionKeyFetcher - ) { - super(config, CombinerEndpoint.PNP_SIGN) - } - - async init( - request: Request<{}, {}, unknown>, - response: Response - ): Promise | null> { - if (!this.validateClientRequest(request)) { - sendFailure(WarningMessage.INVALID_INPUT, 400, response) - return null - } - if (!requestHasSupportedKeyVersion(request, this.config, response.locals.logger)) { - sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) - return null - } - if (!(await authenticateUser(request, response.locals.logger, this.dekFetcher))) { - sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) - return null - } - const keyVersionInfo = getKeyVersionInfo(request, this.config, response.locals.logger) - return new CryptoSession( - request, - response, - keyVersionInfo, - new BLSCryptographyClient(keyVersionInfo) - ) - } - - validateClientRequest( - request: Request<{}, {}, unknown> - ): request is Request<{}, {}, SignMessageRequest> { - return ( - super.validateClientRequest(request) && - hasValidAccountParam(request.body) && - hasValidBlindedPhoneNumberParam(request.body) && - isBodyReasonablySized(request.body) - ) - } -} diff --git a/packages/phone-number-privacy/combiner/src/server.ts b/packages/phone-number-privacy/combiner/src/server.ts index fbd0b9121d6..2a6e543cc95 100644 --- a/packages/phone-number-privacy/combiner/src/server.ts +++ b/packages/phone-number-privacy/combiner/src/server.ts @@ -8,26 +8,24 @@ import { rootLogger, } from '@celo/phone-number-privacy-common' import express, { RequestHandler } from 'express' +import { Signer } from './common/combine' // tslint:disable-next-line: ordered-imports import { - actionHandler, catchErrorHandler, disabledHandler, meteringHandler, PromiseHandler, } from './common/handlers' import { CombinerConfig, getCombinerVersion } from './config' -import { DomainDisableAction } from './domain/endpoints/disable/action' -import { DomainDisableIO } from './domain/endpoints/disable/io' -import { DomainQuotaAction } from './domain/endpoints/quota/action' -import { DomainQuotaIO } from './domain/endpoints/quota/io' -import { DomainSignAction } from './domain/endpoints/sign/action' -import { DomainSignIO } from './domain/endpoints/sign/io' +import { createDisableDomainHandler } from './domain/endpoints/disable/action' +import { createDomainQuotaHandler } from './domain/endpoints/quota/action' + +import { createDomainSignHandler } from './domain/endpoints/sign/action' + import { DomainThresholdStateService } from './domain/services/threshold-state' -import { PnpQuotaAction } from './pnp/endpoints/quota/action' -import { PnpQuotaIO } from './pnp/endpoints/quota/io' -import { PnpSignAction } from './pnp/endpoints/sign/action' -import { PnpSignIO } from './pnp/endpoints/sign/io' +import { createPnpQuotaHandler } from './pnp/endpoints/quota/action' +import { createPnpSignHandler } from './pnp/endpoints/sign/action' + import { PnpThresholdStateService } from './pnp/services/threshold-state' require('events').EventEmitter.defaultMaxListeners = 15 @@ -73,46 +71,40 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { const pnpThresholdStateService = new PnpThresholdStateService() - const pnpQuota = actionHandler( - new PnpQuotaAction( - config.phoneNumberPrivacy, - pnpThresholdStateService, - new PnpQuotaIO(config.phoneNumberPrivacy, kit, dekFetcher) - ) + const pnpSigners: Signer[] = JSON.parse(config.phoneNumberPrivacy.odisServices.signers) + const pnpQuota = createPnpQuotaHandler( + pnpSigners, + config.phoneNumberPrivacy, + pnpThresholdStateService, + dekFetcher ) - const pnpSign = actionHandler( - new PnpSignAction( - config.phoneNumberPrivacy, - pnpThresholdStateService, - new PnpSignIO(config.phoneNumberPrivacy, kit, dekFetcher) - ) + const pnpSign = createPnpSignHandler( + pnpSigners, + config.phoneNumberPrivacy, + pnpThresholdStateService, + dekFetcher ) + const domainSigners: Signer[] = JSON.parse(config.domains.odisServices.signers) const domainThresholdStateService = new DomainThresholdStateService(config.domains) - const domainQuota = actionHandler( - new DomainQuotaAction( - config.domains, - domainThresholdStateService, - new DomainQuotaIO(config.domains) - ) + const domainQuota = createDomainQuotaHandler( + domainSigners, + config.domains, + domainThresholdStateService ) - const domainSign = actionHandler( - new DomainSignAction( - config.domains, - domainThresholdStateService, - new DomainSignIO(config.domains) - ) + const domainSign = createDomainSignHandler( + domainSigners, + config.domains, + domainThresholdStateService ) - const domainDisable = actionHandler( - new DomainDisableAction( - config.domains, - domainThresholdStateService, - new DomainDisableIO(config.domains) - ) + const domainDisable = createDisableDomainHandler( + domainSigners, + config.domains, + domainThresholdStateService ) app.post(CombinerEndpoint.PNP_QUOTA, createHandler(config.phoneNumberPrivacy.enabled, pnpQuota)) diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts index c97f661c38a..9f3179b9ada 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts @@ -35,7 +35,7 @@ describe('domain response logger', () => { logger: rootLogger(config.serviceName), }, } as Response - const session = new Session(mockRequest, mockResponse, keyVersionInfo) + const session = new Session(mockResponse, keyVersionInfo) responses.forEach((res) => { session.responses.push({ url, res, status: 200 }) }) diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts index 9deec258d61..4b99426244d 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts @@ -33,7 +33,7 @@ describe('domain threshold state', () => { logger: new Logger({ name: 'logger' }), }, } as Response - const session = new Session(mockRequest, mockResponse, keyVersionInfo) + const session = new Session(mockResponse, keyVersionInfo) domainStates.forEach((status) => { const res: DomainRestrictedSignatureResponseSuccess | DomainQuotaStatusResponseSuccess = { success: true, diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts index 51f17c32fc4..1cb3aabedd8 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts @@ -38,11 +38,7 @@ describe('pnp response logger', () => { logger: rootLogger(config.serviceName), }, } as Response - const session = new Session( - mockRequest, - mockResponse, - keyVersionInfo - ) + const session = new Session(mockResponse, keyVersionInfo) responses.forEach((res) => { session.responses.push({ url, res, status: 200 }) }) diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts index d42a6e6a8e6..fadeb9f630e 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts @@ -35,17 +35,12 @@ describe('pnp threshold state', () => { logger: rootLogger, }, } as Response - const session = new Session( - mockRequest, - mockResponse, - keyVersionInfo - ) + const session = new Session(mockResponse, keyVersionInfo) quotaData.forEach((q) => { const res: PnpQuotaResponseSuccess | SignMessageResponseSuccess = { success: true, version: expectedVersion, ...q, - blockNumber: testBlockNumber, } session.responses.push({ url: 'random url', res, status: 200 }) }) From 1d2460ee0ca417dee7e81ee9f8f5b55e9e05f061 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Tue, 22 Aug 2023 20:53:39 -0300 Subject: [PATCH 07/19] kill ThresholdService and ResponseLogger classes --- .../combiner/src/common/sign.ts | 8 - .../src/domain/endpoints/disable/action.ts | 12 +- .../src/domain/endpoints/quota/action.ts | 13 +- .../src/domain/endpoints/sign/action.ts | 12 +- .../src/domain/services/log-responses.ts | 90 ++++---- .../src/domain/services/threshold-state.ts | 121 +++++----- .../src/pnp/endpoints/quota/action.ts | 20 +- .../combiner/src/pnp/endpoints/sign/action.ts | 15 +- .../src/pnp/services/log-responses.ts | 216 +++++++++--------- .../src/pnp/services/threshold-state.ts | 69 +++--- .../combiner/src/server.ts | 42 +--- 11 files changed, 283 insertions(+), 335 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/sign.ts b/packages/phone-number-privacy/combiner/src/common/sign.ts index 86ab2a49bd4..d97023f51e8 100644 --- a/packages/phone-number-privacy/combiner/src/common/sign.ts +++ b/packages/phone-number-privacy/combiner/src/common/sign.ts @@ -2,16 +2,8 @@ import { DomainRestrictedSignatureRequest, SignMessageRequest, } from '@celo/phone-number-privacy-common' -import { DomainThresholdStateService } from '../domain/services/threshold-state' -import { PnpThresholdStateService } from '../pnp/services/threshold-state' // prettier-ignore export type OdisSignatureRequest = | SignMessageRequest | DomainRestrictedSignatureRequest - -export type ThresholdStateService = R extends SignMessageRequest - ? PnpThresholdStateService - : never | R extends DomainRestrictedSignatureRequest - ? DomainThresholdStateService - : never diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index 41d8f2730e2..fb96d222d09 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -15,16 +15,14 @@ import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' -import { DomainSignerResponseLogger } from '../../services/log-responses' -import { DomainThresholdStateService } from '../../services/threshold-state' +import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { findThresholdDomainState } from '../../services/threshold-state' export function createDisableDomainHandler( signers: Signer[], - config: OdisConfig, - thresholdStateService: DomainThresholdStateService + config: OdisConfig ): PromiseHandler { const requestSchema = disableDomainRequestSchema(DomainSchema) - const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() const signerEndpoint = CombinerEndpoint.DISABLE_DOMAIN return async (request, response) => { if (!requestSchema.is(request.body)) { @@ -51,9 +49,9 @@ export function createDisableDomainHandler( disableDomainResponseSchema(SequentialDelayDomainStateSchema) ) - responseLogger.logResponseDiscrepancies(session) + logDomainResponsesDiscrepancies(response.locals.logger, session.responses) try { - const disableDomainStatus = thresholdStateService.findThresholdDomainState(session) + const disableDomainStatus = findThresholdDomainState(session, signers.length) if (disableDomainStatus.disabled) { send( response, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index cd6cba55cd7..0033c1237cd 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -15,16 +15,14 @@ import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' -import { DomainSignerResponseLogger } from '../../services/log-responses' -import { DomainThresholdStateService } from '../../services/threshold-state' +import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { findThresholdDomainState } from '../../services/threshold-state' export function createDomainQuotaHandler( signers: Signer[], - config: OdisConfig, - thresholdStateService: DomainThresholdStateService + config: OdisConfig ): PromiseHandler { const requestSchema = domainQuotaStatusRequestSchema(DomainSchema) - const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() const signerEndpoint = CombinerEndpoint.DOMAIN_QUOTA_STATUS return async (request, response) => { if (!requestSchema.is(request.body)) { @@ -51,17 +49,16 @@ export function createDomainQuotaHandler( domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) ) - responseLogger.logResponseDiscrepancies(session) + logDomainResponsesDiscrepancies(response.locals.logger, session.responses) const { threshold } = session.keyVersionInfo if (session.responses.length >= threshold) { try { - const domainQuotaStatus = thresholdStateService.findThresholdDomainState(session) send( response, { success: true, version: getCombinerVersion(), - status: domainQuotaStatus, + status: findThresholdDomainState(session, signers.length), }, 200, response.locals.logger diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index 12afa03510c..02f7fc1c603 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -19,16 +19,14 @@ import { CryptoSession } from '../../../common/crypto-session' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' import { getCombinerVersion, OdisConfig } from '../../../config' -import { DomainSignerResponseLogger } from '../../services/log-responses' -import { DomainThresholdStateService } from '../../services/threshold-state' +import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { findThresholdDomainState } from '../../services/threshold-state' export function createDomainSignHandler( signers: Signer[], - config: OdisConfig, - thresholdStateService: DomainThresholdStateService + config: OdisConfig ): PromiseHandler { const requestSchema = domainRestrictedSignatureRequestSchema(DomainSchema) - const responseLogger: DomainSignerResponseLogger = new DomainSignerResponseLogger() const signerEndpoint = CombinerEndpoint.DOMAIN_SIGN return async (request, response) => { if (!requestSchema.is(request.body)) { @@ -102,7 +100,7 @@ export function createDomainSignHandler( processRequest ) - responseLogger.logResponseDiscrepancies(session) + logDomainResponsesDiscrepancies(response.locals.logger, session.responses) if (session.crypto.hasSufficientSignatures()) { try { @@ -117,7 +115,7 @@ export function createDomainSignHandler( success: true, version: getCombinerVersion(), signature: combinedSignature, - status: thresholdStateService.findThresholdDomainState(session), + status: findThresholdDomainState(session, signers.length), }, 200, response.locals.logger diff --git a/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts b/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts index 4e78834751a..ba35e6234f0 100644 --- a/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts +++ b/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts @@ -3,57 +3,53 @@ import { DomainRestrictedSignatureRequest, WarningMessage, } from '@celo/phone-number-privacy-common' -import { CryptoSession } from '../../common/crypto-session' -import { Session } from '../../common/session' +import Logger from 'bunyan' +import { SignerResponse } from '../../common/io' -export class DomainSignerResponseLogger { - logResponseDiscrepancies( - session: Session | CryptoSession - ): void { - const parsedResponses: Array<{ - signerUrl: string - values: { - version: string - counter: number - disabled: boolean - timer: number - } - }> = [] - session.responses.forEach((response) => { - if (response.res.success) { - const { version, status } = response.res - parsedResponses.push({ - signerUrl: response.url, - values: { - version, - counter: status.counter, - disabled: status.disabled, - timer: status.timer, - }, - }) - } - }) - if (parsedResponses.length === 0) { - session.logger.warn('No successful signer responses found!') - return +export function logDomainResponsesDiscrepancies( + logger: Logger, + responses: Array> +) { + const parsedResponses: Array<{ + signerUrl: string + values: { + version: string + counter: number + disabled: boolean + timer: number } - - // log all responses if we notice any discrepancies to aid with debugging - const first = JSON.stringify(parsedResponses[0].values) - for (let i = 1; i < parsedResponses.length; i++) { - if (JSON.stringify(parsedResponses[i].values) !== first) { - session.logger.warn({ parsedResponses }, WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) - break - } + }> = [] + responses.forEach((response) => { + if (response.res.success) { + const { version, status } = response.res + parsedResponses.push({ + signerUrl: response.url, + values: { + version, + counter: status.counter, + disabled: status.disabled, + timer: status.timer, + }, + }) } + }) + if (parsedResponses.length === 0) { + logger.warn('No successful signer responses found!') + return + } - // disabled - const numDisabled = parsedResponses.filter((res) => res.values.disabled).length - if (numDisabled > 0 && numDisabled < parsedResponses.length) { - session.logger.error( - { parsedResponses }, - WarningMessage.INCONSISTENT_SIGNER_DOMAIN_DISABLED_STATES - ) + // log all responses if we notice any discrepancies to aid with debugging + const first = JSON.stringify(parsedResponses[0].values) + for (let i = 1; i < parsedResponses.length; i++) { + if (JSON.stringify(parsedResponses[i].values) !== first) { + logger.warn({ parsedResponses }, WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) + break } } + + // disabled + const numDisabled = parsedResponses.filter((res) => res.values.disabled).length + if (numDisabled > 0 && numDisabled < parsedResponses.length) { + logger.error({ parsedResponses }, WarningMessage.INCONSISTENT_SIGNER_DOMAIN_DISABLED_STATES) + } } diff --git a/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts b/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts index 38cdf62e8e8..74ffb18e45d 100644 --- a/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts +++ b/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts @@ -1,78 +1,75 @@ import { DomainRequest, DomainState } from '@celo/phone-number-privacy-common' import { Session } from '../../common/session' -import { OdisConfig } from '../../config' -export class DomainThresholdStateService { - constructor(readonly config: OdisConfig) {} +export function findThresholdDomainState( + session: Session, + totalSigners: number +): DomainState { + // Get the domain status from the responses, filtering out responses that don't have the status. + const domainStates = session.responses + .map((signerResponse) => ('status' in signerResponse.res ? signerResponse.res.status : null)) + .filter((state: DomainState | null | undefined): state is DomainState => !!state) - findThresholdDomainState(session: Session): DomainState { - // Get the domain status from the responses, filtering out responses that don't have the status. - const domainStates = session.responses - .map((signerResponse) => ('status' in signerResponse.res ? signerResponse.res.status : null)) - .filter((state: DomainState | null | undefined): state is DomainState => !!state) + const { threshold } = session.keyVersionInfo - const { threshold } = session.keyVersionInfo - - // Note: when the threshold > # total signers - threshold, it's possible that we - // throw an error here when the domain is disabled. While the domain is technically disabled, - // the hope is to increase the "safety margin" of the number of signers that have - // also disabled this domain.This can be changed in the future (if we think that - // the safety margin is no longer needed) by simply checking if the domain is disabled - // before checking if the threshold of enabled responses has been met. - if (domainStates.length < threshold) { - throw new Error('Insufficient number of signer responses') - } + // Note: when the threshold > # total signers - threshold, it's possible that we + // throw an error here when the domain is disabled. While the domain is technically disabled, + // the hope is to increase the "safety margin" of the number of signers that have + // also disabled this domain.This can be changed in the future (if we think that + // the safety margin is no longer needed) by simply checking if the domain is disabled + // before checking if the threshold of enabled responses has been met. + if (domainStates.length < threshold) { + throw new Error('Insufficient number of signer responses') + } - // Check whether the domain is disabled, either by all signers or by some. - const domainStatesEnabled = domainStates.filter((ds) => !ds.disabled) - const numDisabled = domainStates.length - domainStatesEnabled.length + // Check whether the domain is disabled, either by all signers or by some. + const domainStatesEnabled = domainStates.filter((ds) => !ds.disabled) + const numDisabled = domainStates.length - domainStatesEnabled.length - const signersLength = JSON.parse(this.config.odisServices.signers).length - if (signersLength - numDisabled < threshold) { - return { timer: 0, counter: 0, disabled: true, now: 0 } - } + if (totalSigners - numDisabled < threshold) { + return { timer: 0, counter: 0, disabled: true, now: 0 } + } - // Ideally users will resubmit the request in this case. - if (domainStatesEnabled.length < threshold) { - throw new Error('Insufficient number of signer responses. Domain may be disabled') - } + // Ideally users will resubmit the request in this case. + if (domainStatesEnabled.length < threshold) { + throw new Error('Insufficient number of signer responses. Domain may be disabled') + } - // Set n to last signer index in a quorum of signers are sorted from least to most restrictive. - const n = threshold - 1 + // Set n to last signer index in a quorum of signers are sorted from least to most restrictive. + const n = threshold - 1 - const domainStatesAscendingByCounter = domainStatesEnabled.sort((a, b) => a.counter - b.counter) - const nthLeastRestrictiveByCounter = domainStatesAscendingByCounter[n] - const thresholdCounter = nthLeastRestrictiveByCounter.counter + const domainStatesAscendingByCounter = domainStatesEnabled.sort((a, b) => a.counter - b.counter) + const nthLeastRestrictiveByCounter = domainStatesAscendingByCounter[n] + const thresholdCounter = nthLeastRestrictiveByCounter.counter - // Client should submit requests with nonce === thresholdCounter + // Client should submit requests with nonce === thresholdCounter - const domainStatesWithThresholdCounter = domainStatesEnabled.filter( - (ds) => ds.counter <= thresholdCounter - ) + const domainStatesWithThresholdCounter = domainStatesEnabled.filter( + (ds) => ds.counter <= thresholdCounter + ) - const domainStatesAscendingByTimestampRestrictiveness = domainStatesWithThresholdCounter.sort( - (a, b) => a.timer - a.now - (b.timer - b.now) - /** - * Please see '@celo/phone-number-privacy-common/src/domains/sequential-delay.ts' - * and https://github.com/celo-org/celo-proposals/blob/master/CIPs/CIP-0040/sequentialDelayDomain.md - * - * For a given DomainState, it is always the case that 'now' >= 'timer'. This ordering ensures - * that we take the 'timer' and 'date' from the same DomainState while still returning a reasonable - * definition of the "nth least restrictive" values. For simplicity, we do not take into consideration - * the 'delay' until the next request will be accepted as that would require calculating this value for - * each DomainState with the checkSequentialDelayDomainState algorithm in sequential-delay.ts. - * This would add complexity because DomainStates may have different values for 'counter' that dramatically - * alter this 'delay' and we want to protect the user's quota by returning the lowest possible - * threshold 'counter'. Feel free to implement a more exact solution if you're up for a coding challenge :) - */ - ) - const nthLeastRestrictiveByTimestamps = domainStatesAscendingByTimestampRestrictiveness[n] + const domainStatesAscendingByTimestampRestrictiveness = domainStatesWithThresholdCounter.sort( + (a, b) => a.timer - a.now - (b.timer - b.now) + /** + * Please see '@celo/phone-number-privacy-common/src/domains/sequential-delay.ts' + * and https://github.com/celo-org/celo-proposals/blob/master/CIPs/CIP-0040/sequentialDelayDomain.md + * + * For a given DomainState, it is always the case that 'now' >= 'timer'. This ordering ensures + * that we take the 'timer' and 'date' from the same DomainState while still returning a reasonable + * definition of the "nth least restrictive" values. For simplicity, we do not take into consideration + * the 'delay' until the next request will be accepted as that would require calculating this value for + * each DomainState with the checkSequentialDelayDomainState algorithm in sequential-delay.ts. + * This would add complexity because DomainStates may have different values for 'counter' that dramatically + * alter this 'delay' and we want to protect the user's quota by returning the lowest possible + * threshold 'counter'. Feel free to implement a more exact solution if you're up for a coding challenge :) + */ + ) + const nthLeastRestrictiveByTimestamps = domainStatesAscendingByTimestampRestrictiveness[n] - return { - timer: nthLeastRestrictiveByTimestamps.timer, - counter: thresholdCounter, - disabled: false, - now: nthLeastRestrictiveByTimestamps.now, - } + return { + timer: nthLeastRestrictiveByTimestamps.timer, + counter: thresholdCounter, + disabled: false, + now: nthLeastRestrictiveByTimestamps.now, } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index a5ca8f553fc..897d21a28aa 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -17,17 +17,19 @@ import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' import { Session } from '../../../common/session' import { getCombinerVersion, OdisConfig } from '../../../config' -import { PnpSignerResponseLogger } from '../../services/log-responses' -import { PnpThresholdStateService } from '../../services/threshold-state' +import { + logFailOpenResponses, + logPnpSignerResponseDiscrepancies, +} from '../../services/log-responses' +import { findCombinerQuotaState } from '../../services/threshold-state' export function createPnpQuotaHandler( signers: Signer[], config: OdisConfig, - thresholdStateService: PnpThresholdStateService, dekFetcher: DataEncryptionKeyFetcher ): PromiseHandler { const signerEndpoint = CombinerEndpoint.PNP_QUOTA - const responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() + return async (request, response) => { const logger = response.locals.logger if (!validateRequest(request)) { @@ -47,20 +49,20 @@ export function createPnpQuotaHandler( signers, signerEndpoint, request, - session.keyVersionInfo, + keyVersionInfo, null, config.odisServices.timeoutMilliSeconds, PnpQuotaResponseSchema ) - responseLogger.logResponseDiscrepancies(session) - responseLogger.logFailOpenResponses(session) + session.warnings.push(...logPnpSignerResponseDiscrepancies(logger, session.responses)) + logFailOpenResponses(logger, session.responses) const { threshold } = session.keyVersionInfo if (session.responses.length >= threshold) { try { - const quotaStatus = thresholdStateService.findCombinerQuotaState(session) + const quotaStatus = findCombinerQuotaState(session) send( response, { @@ -75,7 +77,7 @@ export function createPnpQuotaHandler( return } catch (err) { - session.logger.error(err, 'Error combining signer quota status responses') + logger.error(err, 'Error combining signer quota status responses') } } sendFailure( diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index 3455e0003b5..235c7d1da9b 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -22,18 +22,19 @@ import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto import { CryptoSession } from '../../../common/crypto-session' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' -import { ThresholdStateService } from '../../../common/sign' import { getCombinerVersion, OdisConfig } from '../../../config' -import { PnpSignerResponseLogger } from '../../services/log-responses' +import { + logFailOpenResponses, + logPnpSignerResponseDiscrepancies, +} from '../../services/log-responses' +import { findCombinerQuotaState } from '../../services/threshold-state' export function createPnpSignHandler( signers: Signer[], config: OdisConfig, - thresholdStateService: ThresholdStateService, dekFetcher: DataEncryptionKeyFetcher ): PromiseHandler { const signerEndpoint = CombinerEndpoint.PNP_SIGN - const responseLogger: PnpSignerResponseLogger = new PnpSignerResponseLogger() return async (request, response) => { const logger = response.locals.logger if (!validateRequest(request)) { @@ -101,8 +102,8 @@ export function createPnpSignHandler( processRequest ) - responseLogger.logResponseDiscrepancies(session) - responseLogger.logFailOpenResponses(session) + session.warnings.push(...logPnpSignerResponseDiscrepancies(logger, session.responses)) + logFailOpenResponses(logger, session.responses) if (session.crypto.hasSufficientSignatures()) { try { @@ -111,7 +112,7 @@ export function createPnpSignHandler( logger ) - const quotaStatus = thresholdStateService.findCombinerQuotaState(session) + const quotaStatus = findCombinerQuotaState(session) return send( response, { diff --git a/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts b/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts index 3ad62ecd738..607d7404251 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts @@ -4,130 +4,128 @@ import { SignMessageRequest, WarningMessage, } from '@celo/phone-number-privacy-common' -import { Session } from '../../common/session' +import Logger from 'bunyan' +import { SignerResponse } from '../../common/io' + import { MAX_BLOCK_DISCREPANCY_THRESHOLD, MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, } from '../../config' -export class PnpSignerResponseLogger { - logResponseDiscrepancies(session: Session | Session): void { - // TODO responses should all already be successes due to CombineAction receiveSuccess - // https://github.com/celo-org/celo-monorepo/issues/9826 +export function logPnpSignerResponseDiscrepancies( + logger: Logger, + responses: Array> +): string[] { + const warnings: string[] = [] - const parsedResponses: Array<{ - signerUrl: string - values: { - version: string - performedQueryCount: number - totalQuota: number - blockNumber?: number - warnings?: string[] - } - }> = [] - session.responses.forEach((response) => { - if (response.res.success) { - const { version, performedQueryCount, totalQuota, warnings } = response.res - parsedResponses.push({ - signerUrl: response.url, - values: { version, performedQueryCount, totalQuota, warnings }, - }) - } - }) - if (parsedResponses.length === 0) { - session.logger.warn('No successful signer responses found!') - return - } + // TODO responses should all already be successes due to CombineAction receiveSuccess + // https://github.com/celo-org/celo-monorepo/issues/9826 - // log all responses if we notice any discrepancies to aid with debugging - const first = JSON.stringify(parsedResponses[0].values) - for (let i = 1; i < parsedResponses.length; i++) { - if (JSON.stringify(parsedResponses[i].values) !== first) { - session.logger.warn({ parsedResponses }, WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) - session.warnings.push(WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) - break - } + const parsedResponses: Array<{ + signerUrl: string + values: { + version: string + performedQueryCount: number + totalQuota: number + blockNumber?: number + warnings?: string[] } - - // blockNumber - parsedResponses.forEach((res) => { - if (res.values.blockNumber === undefined) { - session.logger.warn( - { signerUrl: res.signerUrl }, - 'Signer responded with undefined blockNumber' - ) - } - }) - const sortedByBlockNumber = parsedResponses - .filter((res) => !!res.values.blockNumber) - .sort((a, b) => a.values.blockNumber! - b.values.blockNumber!) - if ( - sortedByBlockNumber.length && - sortedByBlockNumber[sortedByBlockNumber.length - 1].values.blockNumber! - - sortedByBlockNumber[0].values.blockNumber! >= - MAX_BLOCK_DISCREPANCY_THRESHOLD - ) { - session.logger.error( - { sortedByBlockNumber }, - WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS - ) - session.warnings.push(WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS) + }> = [] + responses.forEach((response) => { + if (response.res.success) { + const { version, performedQueryCount, totalQuota, warnings } = response.res + parsedResponses.push({ + signerUrl: response.url, + values: { version, performedQueryCount, totalQuota, warnings }, + }) } + }) + if (parsedResponses.length === 0) { + logger.warn('No successful signer responses found!') + return warnings + } - // totalQuota - const sortedByTotalQuota = parsedResponses.sort( - (a, b) => a.values.totalQuota - b.values.totalQuota - ) - if ( - sortedByTotalQuota[sortedByTotalQuota.length - 1].values.totalQuota - - sortedByTotalQuota[0].values.totalQuota >= - MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD - ) { - session.logger.error( - { sortedByTotalQuota }, - WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS - ) - session.warnings.push(WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS) + // log all responses if we notice any discrepancies to aid with debugging + const first = JSON.stringify(parsedResponses[0].values) + for (let i = 1; i < parsedResponses.length; i++) { + if (JSON.stringify(parsedResponses[i].values) !== first) { + logger.warn({ parsedResponses }, WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) + warnings.push(WarningMessage.SIGNER_RESPONSE_DISCREPANCIES) + break } + } - // performedQueryCount - const sortedByQueryCount = parsedResponses.sort( - (a, b) => a.values.performedQueryCount - b.values.performedQueryCount - ) - if ( - sortedByQueryCount[sortedByQueryCount.length - 1].values.performedQueryCount - - sortedByQueryCount[0].values.performedQueryCount >= - MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD - ) { - session.logger.error( - { sortedByQueryCount }, - WarningMessage.INCONSISTENT_SIGNER_QUERY_MEASUREMENTS - ) - session.warnings.push(WarningMessage.INCONSISTENT_SIGNER_QUERY_MEASUREMENTS) + // blockNumber + parsedResponses.forEach((res) => { + if (res.values.blockNumber === undefined) { + logger.warn({ signerUrl: res.signerUrl }, 'Signer responded with undefined blockNumber') } + }) + const sortedByBlockNumber = parsedResponses + .filter((res) => !!res.values.blockNumber) + .sort((a, b) => a.values.blockNumber! - b.values.blockNumber!) + if ( + sortedByBlockNumber.length && + sortedByBlockNumber[sortedByBlockNumber.length - 1].values.blockNumber! - + sortedByBlockNumber[0].values.blockNumber! >= + MAX_BLOCK_DISCREPANCY_THRESHOLD + ) { + logger.error({ sortedByBlockNumber }, WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS) + warnings.push(WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS) } - logFailOpenResponses(session: Session | Session): void { - session.responses.forEach((response) => { - if (response.res.success) { - const { warnings } = response.res - if (warnings) { - warnings.forEach((warning) => { - switch (warning) { - case ErrorMessage.FAILING_OPEN: - case ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA: - case ErrorMessage.FAILURE_TO_GET_DEK: - session.logger.error( - { signerWarning: warning, service: response.url }, - WarningMessage.SIGNER_FAILED_OPEN - ) - default: - break - } - }) - } - } - }) + // totalQuota + const sortedByTotalQuota = parsedResponses.sort( + (a, b) => a.values.totalQuota - b.values.totalQuota + ) + if ( + sortedByTotalQuota[sortedByTotalQuota.length - 1].values.totalQuota - + sortedByTotalQuota[0].values.totalQuota >= + MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD + ) { + logger.error({ sortedByTotalQuota }, WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS) + warnings.push(WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS) + } + + // performedQueryCount + const sortedByQueryCount = parsedResponses.sort( + (a, b) => a.values.performedQueryCount - b.values.performedQueryCount + ) + if ( + sortedByQueryCount[sortedByQueryCount.length - 1].values.performedQueryCount - + sortedByQueryCount[0].values.performedQueryCount >= + MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD + ) { + logger.error({ sortedByQueryCount }, WarningMessage.INCONSISTENT_SIGNER_QUERY_MEASUREMENTS) + warnings.push(WarningMessage.INCONSISTENT_SIGNER_QUERY_MEASUREMENTS) } + + return warnings +} + +export function logFailOpenResponses( + logger: Logger, + responses: Array> +): void { + responses.forEach((response) => { + if (response.res.success) { + const { warnings } = response.res + if (warnings) { + warnings.forEach((warning) => { + switch (warning) { + case ErrorMessage.FAILING_OPEN: + case ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA: + case ErrorMessage.FAILURE_TO_GET_DEK: + logger.error( + { signerWarning: warning, service: response.url }, + WarningMessage.SIGNER_FAILED_OPEN + ) + default: + break + } + }) + } + } + }) } diff --git a/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts b/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts index f4ef6addc8c..0b919ee58da 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts @@ -6,43 +6,44 @@ import { } from '@celo/phone-number-privacy-common' import { Session } from '../../common/session' import { MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD } from '../../config' -export class PnpThresholdStateService { - findCombinerQuotaState(session: Session): PnpQuotaStatus { - const { threshold } = session.keyVersionInfo - const signerResponses = session.responses - .map((signerResponse) => signerResponse.res) - .filter((res) => res.success) as PnpQuotaStatus[] - const sortedResponses = signerResponses.sort( - (a, b) => b.totalQuota - b.performedQueryCount - (a.totalQuota - a.performedQueryCount) - ) - const totalQuotaAvg = - sortedResponses.map((r) => r.totalQuota).reduce((a, b) => a + b) / sortedResponses.length - const totalQuotaStDev = Math.sqrt( - sortedResponses.map((r) => (r.totalQuota - totalQuotaAvg) ** 2).reduce((a, b) => a + b) / - sortedResponses.length +export function findCombinerQuotaState( + session: Session +): PnpQuotaStatus { + const { threshold } = session.keyVersionInfo + const signerResponses = session.responses + .map((signerResponse) => signerResponse.res) + .filter((res) => res.success) as PnpQuotaStatus[] + const sortedResponses = signerResponses.sort( + (a, b) => b.totalQuota - b.performedQueryCount - (a.totalQuota - a.performedQueryCount) + ) + + const totalQuotaAvg = + sortedResponses.map((r) => r.totalQuota).reduce((a, b) => a + b) / sortedResponses.length + const totalQuotaStDev = Math.sqrt( + sortedResponses.map((r) => (r.totalQuota - totalQuotaAvg) ** 2).reduce((a, b) => a + b) / + sortedResponses.length + ) + if (totalQuotaStDev > MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD) { + // TODO(2.0.0): add alerting for this + throw new Error(WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS) + } else if (totalQuotaStDev > 0) { + session.warnings.push( + WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + + ', using threshold signer as best guess' ) - if (totalQuotaStDev > MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD) { - // TODO(2.0.0): add alerting for this - throw new Error(WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS) - } else if (totalQuotaStDev > 0) { - session.warnings.push( - WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + - ', using threshold signer as best guess' - ) - } + } - // TODO(2.0.0) currently this check is not needed, as checking for sufficient number of responses and - // filtering for successes is already done in the action. Consider adding back in based on the - // result of https://github.com/celo-org/celo-monorepo/issues/9826 - // if (signerResponses.length < threshold) { - // throw new Error('Insufficient number of successful signer responses') - // } + // TODO(2.0.0) currently this check is not needed, as checking for sufficient number of responses and + // filtering for successes is already done in the action. Consider adding back in based on the + // result of https://github.com/celo-org/celo-monorepo/issues/9826 + // if (signerResponses.length < threshold) { + // throw new Error('Insufficient number of successful signer responses') + // } - const thresholdSigner = sortedResponses[threshold - 1] - return { - performedQueryCount: thresholdSigner.performedQueryCount, - totalQuota: thresholdSigner.totalQuota, - } + const thresholdSigner = sortedResponses[threshold - 1] + return { + performedQueryCount: thresholdSigner.performedQueryCount, + totalQuota: thresholdSigner.totalQuota, } } diff --git a/packages/phone-number-privacy/combiner/src/server.ts b/packages/phone-number-privacy/combiner/src/server.ts index 2a6e543cc95..02d24483111 100644 --- a/packages/phone-number-privacy/combiner/src/server.ts +++ b/packages/phone-number-privacy/combiner/src/server.ts @@ -22,12 +22,9 @@ import { createDomainQuotaHandler } from './domain/endpoints/quota/action' import { createDomainSignHandler } from './domain/endpoints/sign/action' -import { DomainThresholdStateService } from './domain/services/threshold-state' import { createPnpQuotaHandler } from './pnp/endpoints/quota/action' import { createPnpSignHandler } from './pnp/endpoints/sign/action' -import { PnpThresholdStateService } from './pnp/services/threshold-state' - require('events').EventEmitter.defaultMaxListeners = 15 export function startCombiner(config: CombinerConfig, kit: ContractKit) { @@ -69,43 +66,14 @@ export function startCombiner(config: CombinerConfig, kit: ContractKit) { config.phoneNumberPrivacy.fullNodeRetryDelayMs ) - const pnpThresholdStateService = new PnpThresholdStateService() - const pnpSigners: Signer[] = JSON.parse(config.phoneNumberPrivacy.odisServices.signers) - const pnpQuota = createPnpQuotaHandler( - pnpSigners, - config.phoneNumberPrivacy, - pnpThresholdStateService, - dekFetcher - ) - - const pnpSign = createPnpSignHandler( - pnpSigners, - config.phoneNumberPrivacy, - pnpThresholdStateService, - dekFetcher - ) + const pnpQuota = createPnpQuotaHandler(pnpSigners, config.phoneNumberPrivacy, dekFetcher) + const pnpSign = createPnpSignHandler(pnpSigners, config.phoneNumberPrivacy, dekFetcher) const domainSigners: Signer[] = JSON.parse(config.domains.odisServices.signers) - const domainThresholdStateService = new DomainThresholdStateService(config.domains) - - const domainQuota = createDomainQuotaHandler( - domainSigners, - config.domains, - domainThresholdStateService - ) - - const domainSign = createDomainSignHandler( - domainSigners, - config.domains, - domainThresholdStateService - ) - - const domainDisable = createDisableDomainHandler( - domainSigners, - config.domains, - domainThresholdStateService - ) + const domainQuota = createDomainQuotaHandler(domainSigners, config.domains) + const domainSign = createDomainSignHandler(domainSigners, config.domains) + const domainDisable = createDisableDomainHandler(domainSigners, config.domains) app.post(CombinerEndpoint.PNP_QUOTA, createHandler(config.phoneNumberPrivacy.enabled, pnpQuota)) app.post(CombinerEndpoint.PNP_SIGN, createHandler(config.phoneNumberPrivacy.enabled, pnpSign)) From f5380f21f53696b2bfe81c8f9042092e388d5f98 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 09:01:41 -0300 Subject: [PATCH 08/19] Removes Session and CryptoSession --- .../combiner/src/common/action.ts | 13 ------ .../combiner/src/common/combine.ts | 44 +++++++++++++++---- .../combiner/src/common/crypto-session.ts | 16 ------- .../combiner/src/common/io.ts | 22 ---------- .../combiner/src/common/session.ts | 43 ------------------ .../combiner/src/common/sign.ts | 9 ---- .../src/domain/endpoints/disable/action.ts | 19 ++++---- .../src/domain/endpoints/quota/action.ts | 20 +++------ .../src/domain/endpoints/sign/action.ts | 36 +++++++-------- .../src/domain/services/threshold-state.ts | 12 ++--- .../src/pnp/endpoints/quota/action.ts | 24 ++++------ .../combiner/src/pnp/endpoints/sign/action.ts | 42 ++++++++---------- .../src/pnp/services/threshold-state.ts | 18 ++++---- 13 files changed, 110 insertions(+), 208 deletions(-) delete mode 100644 packages/phone-number-privacy/combiner/src/common/action.ts delete mode 100644 packages/phone-number-privacy/combiner/src/common/crypto-session.ts delete mode 100644 packages/phone-number-privacy/combiner/src/common/session.ts delete mode 100644 packages/phone-number-privacy/combiner/src/common/sign.ts diff --git a/packages/phone-number-privacy/combiner/src/common/action.ts b/packages/phone-number-privacy/combiner/src/common/action.ts deleted file mode 100644 index 008d01e9b7c..00000000000 --- a/packages/phone-number-privacy/combiner/src/common/action.ts +++ /dev/null @@ -1,13 +0,0 @@ -import { OdisRequest, OdisResponse } from '@celo/phone-number-privacy-common' -import { Locals, Request, Response } from 'express' -import { IO } from './io' -import { Session } from './session' - -export interface Action { - readonly io: IO - perform( - request: Request<{}, {}, R>, - response: Response, Locals>, - session: Session - ): Promise -} diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 6ba49f440cf..2e0c777ece7 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -10,7 +10,7 @@ import Logger from 'bunyan' import { Request } from 'express' import * as t from 'io-ts' import { PerformanceObserver } from 'perf_hooks' -import { fetchSignerResponseWithFallback } from './io' +import { fetchSignerResponseWithFallback, SignerResponse } from './io' export interface Signer { url: string @@ -27,7 +27,7 @@ export async function thresholdCallToSigners( requestTimeoutMS: number, responseSchema: t.Type, OdisResponse, unknown>, processResult: (res: OdisResponse) => Promise = (_) => Promise.resolve(false) -) { +): Promise<{ signerResponses: Array>; maxErrorCode?: number }> { const obs = new PerformanceObserver((list) => { // Possible race condition here: if multiple signers take exactly the same // amount of time, the PerformanceObserver callback may be called twice with @@ -46,11 +46,12 @@ export async function thresholdCallToSigners( const timeoutSignal = AbortSignal.timeout(requestTimeoutMS) const abortSignal = (AbortSignal as any).any([manualAbort.signal, timeoutSignal]) as AbortSignal - const failedSigners: string[] = [] + let errorCount = 0 const errorCodes: Map = new Map() const requiredThreshold = keyVersionInfo.threshold + const responses: Array> = [] // Forward request to signers // An unexpected error in handling the result for one signer should not // block a threshold of correct responses, but should be logged. @@ -67,15 +68,13 @@ export async function thresholdCallToSigners( ) if (!signerFetchResult.ok) { - // Tracking failed request count via signer url prevents - // double counting the same failed request by mistake - failedSigners.push(signer.url) + errorCount++ errorCodes.set( signerFetchResult.status, (errorCodes.get(signerFetchResult.status) ?? 0) + 1 ) - if (signers.length - failedSigners.length < requiredThreshold) { + if (signers.length - errorCount < requiredThreshold) { logger.warn('Not possible to reach a threshold of signer responses. Failing fast') manualAbort.abort() } @@ -105,6 +104,8 @@ export async function thresholdCallToSigners( throw new Error(ErrorMessage.SIGNER_RESPONSE_FAILED_WITH_OK_STATUS) } + responses.push({ res: odisResponse, url: signer.url }) + if (await processResult(odisResponse)) { // we already have enough responses manualAbort.abort() @@ -121,8 +122,8 @@ export async function thresholdCallToSigners( // Tracking failed request count via signer url prevents // double counting the same failed request by mistake - failedSigners.push(signer.url) - if (signers.length - failedSigners.length < requiredThreshold) { + errorCount++ + if (signers.length - errorCount < requiredThreshold) { logger.warn('Not possible to reach a threshold of signer responses. Failing fast') manualAbort.abort() } @@ -137,6 +138,17 @@ export async function thresholdCallToSigners( // DO NOT call performance.clearMarks() as this also deletes marks used to // measure e2e combiner latency. obs.disconnect() + + if (errorCodes.size > 1) { + logger.error( + { errorCodes: JSON.stringify([...errorCodes]) }, + ErrorMessage.INCONSISTENT_SIGNER_RESPONSES + ) + + return { signerResponses: responses, maxErrorCode: getMajorityErrorCode(errorCodes) } + } else { + return { signerResponses: responses } + } } function parseSchema(schema: t.Type, data: unknown, logger: Logger): T { @@ -154,3 +166,17 @@ function isTimeoutError(err: unknown) { function isAbortError(err: unknown) { return err instanceof Error && err.name === 'AbortError' } + +function getMajorityErrorCode(errorCodes: Map): number { + let maxErrorCode = -1 + let maxCount = -1 + errorCodes.forEach((count, errorCode) => { + // This gives priority to the lower status codes in the event of a tie + // because 400s are more helpful than 500s for user feedback + if (count > maxCount || (count === maxCount && errorCode < maxErrorCode)) { + maxCount = count + maxErrorCode = errorCode + } + }) + return maxErrorCode +} diff --git a/packages/phone-number-privacy/combiner/src/common/crypto-session.ts b/packages/phone-number-privacy/combiner/src/common/crypto-session.ts deleted file mode 100644 index f1a0d7d6f98..00000000000 --- a/packages/phone-number-privacy/combiner/src/common/crypto-session.ts +++ /dev/null @@ -1,16 +0,0 @@ -import { KeyVersionInfo, OdisResponse } from '@celo/phone-number-privacy-common' -import { Request, Response } from 'express' -import { CryptoClient } from './crypto-clients/crypto-client' -import { Session } from './session' -import { OdisSignatureRequest } from './sign' - -export class CryptoSession extends Session { - public constructor( - readonly request: Request<{}, {}, R>, - readonly response: Response>, - readonly keyVersionInfo: KeyVersionInfo, - readonly crypto: CryptoClient - ) { - super(request, response, keyVersionInfo) - } -} diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 4801863664e..203ceacaf51 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -1,46 +1,24 @@ import { - CombinerEndpoint, ErrorType, getRequestKeyVersion, - getSignerEndpoint, KeyVersionInfo, KEY_VERSION_HEADER, OdisRequest, OdisResponse, requestHasValidKeyVersion, send, - SignerEndpoint, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' -import * as t from 'io-ts' import fetch, { Response as FetchResponse } from 'node-fetch' import { performance } from 'perf_hooks' import { getCombinerVersion, OdisConfig } from '../config' import { Signer } from './combine' -import { Session } from './session' // tslint:disable-next-line: interface-over-type-literal export type SignerResponse = { url: string res: OdisResponse - status: number -} - -export abstract class IO { - readonly signerEndpoint: SignerEndpoint = getSignerEndpoint(this.endpoint) - abstract readonly requestSchema: t.Type - - constructor(readonly config: OdisConfig, readonly endpoint: CombinerEndpoint) {} - - abstract init( - request: Request<{}, {}, unknown>, - response: Response> - ): Promise | null> - - validateClientRequest(request: Request<{}, {}, unknown>): request is Request<{}, {}, R> { - return this.requestSchema.is(request.body) - } } export function requestHasSupportedKeyVersion( diff --git a/packages/phone-number-privacy/combiner/src/common/session.ts b/packages/phone-number-privacy/combiner/src/common/session.ts deleted file mode 100644 index 42b24c245ed..00000000000 --- a/packages/phone-number-privacy/combiner/src/common/session.ts +++ /dev/null @@ -1,43 +0,0 @@ -import { - ErrorMessage, - KeyVersionInfo, - OdisRequest, - OdisResponse, -} from '@celo/phone-number-privacy-common' -import Logger from 'bunyan' -import { Response } from 'express' -import { SignerResponse } from './io' - -export class Session { - readonly logger: Logger - - readonly errorCodes: Map = new Map() - readonly responses: Array> = new Array>() - readonly warnings: string[] = [] - - public constructor(response: Response>, readonly keyVersionInfo: KeyVersionInfo) { - this.logger = response.locals.logger - } - - getMajorityErrorCode(): number | null { - const uniqueErrorCount = Array.from(this.errorCodes.keys()).length - if (uniqueErrorCount > 1) { - this.logger.error( - { errorCodes: JSON.stringify([...this.errorCodes]) }, - ErrorMessage.INCONSISTENT_SIGNER_RESPONSES - ) - } - - let maxErrorCode = -1 - let maxCount = -1 - this.errorCodes.forEach((count, errorCode) => { - // This gives priority to the lower status codes in the event of a tie - // because 400s are more helpful than 500s for user feedback - if (count > maxCount || (count === maxCount && errorCode < maxErrorCode)) { - maxCount = count - maxErrorCode = errorCode - } - }) - return maxErrorCode > 0 ? maxErrorCode : null - } -} diff --git a/packages/phone-number-privacy/combiner/src/common/sign.ts b/packages/phone-number-privacy/combiner/src/common/sign.ts deleted file mode 100644 index d97023f51e8..00000000000 --- a/packages/phone-number-privacy/combiner/src/common/sign.ts +++ /dev/null @@ -1,9 +0,0 @@ -import { - DomainRestrictedSignatureRequest, - SignMessageRequest, -} from '@celo/phone-number-privacy-common' - -// prettier-ignore -export type OdisSignatureRequest = - | SignMessageRequest - | DomainRestrictedSignatureRequest diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index fb96d222d09..f0a92b71010 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -13,7 +13,7 @@ import { import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' + import { getCombinerVersion, OdisConfig } from '../../../config' import { logDomainResponsesDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' @@ -36,9 +36,8 @@ export function createDisableDomainHandler( } const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) - const session = new Session(response, keyVersionInfo) - await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( response.locals.logger, signers, signerEndpoint, @@ -49,9 +48,13 @@ export function createDisableDomainHandler( disableDomainResponseSchema(SequentialDelayDomainStateSchema) ) - logDomainResponsesDiscrepancies(response.locals.logger, session.responses) + logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) try { - const disableDomainStatus = findThresholdDomainState(session, signers.length) + const disableDomainStatus = findThresholdDomainState( + keyVersionInfo, + signerResponses, + signers.length + ) if (disableDomainStatus.disabled) { send( response, @@ -73,10 +76,6 @@ export function createDisableDomainHandler( ) } - sendFailure( - ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, - session.getMajorityErrorCode() ?? 500, - response - ) + sendFailure(ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, maxErrorCode ?? 500, response) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index 0033c1237cd..c696ec54ec4 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -13,7 +13,7 @@ import { import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' + import { getCombinerVersion, OdisConfig } from '../../../config' import { logDomainResponsesDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' @@ -36,29 +36,27 @@ export function createDomainQuotaHandler( } const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) - const session = new Session(response, keyVersionInfo) - await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( response.locals.logger, signers, signerEndpoint, request, - session.keyVersionInfo, + keyVersionInfo, null, config.odisServices.timeoutMilliSeconds, domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) ) - logDomainResponsesDiscrepancies(response.locals.logger, session.responses) - const { threshold } = session.keyVersionInfo - if (session.responses.length >= threshold) { + logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) + if (signerResponses.length >= keyVersionInfo.threshold) { try { send( response, { success: true, version: getCombinerVersion(), - status: findThresholdDomainState(session, signers.length), + status: findThresholdDomainState(keyVersionInfo, signerResponses, signers.length), }, 200, response.locals.logger @@ -68,10 +66,6 @@ export function createDomainQuotaHandler( response.locals.logger.error(err, 'Error combining signer quota status responses') } } - sendFailure( - ErrorMessage.THRESHOLD_DOMAIN_QUOTA_STATUS_FAILURE, - session.getMajorityErrorCode() ?? 500, - response - ) + sendFailure(ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, maxErrorCode ?? 500, response) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index 02f7fc1c603..5ae2ea4e974 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -15,9 +15,10 @@ import { import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { DomainCryptoClient } from '../../../common/crypto-clients/domain-crypto-client' -import { CryptoSession } from '../../../common/crypto-session' + import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' + import { getCombinerVersion, OdisConfig } from '../../../config' import { logDomainResponsesDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' @@ -47,25 +48,20 @@ export function createDomainSignHandler( } const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) - const session = new CryptoSession( - request, - response, - keyVersionInfo, - new DomainCryptoClient(keyVersionInfo) - ) + const crypto = new DomainCryptoClient(keyVersionInfo) const logger = response.locals.logger const processRequest = async ( res: OdisResponse ): Promise => { assert(res.success) - session.crypto.addSignature({ url: 'TODO: remove', signature: res.signature }) + crypto.addSignature({ url: 'TODO: remove', signature: res.signature }) // const signatureAdditionStart = Date.now() // logger.info( // { // signer: url, - // hasSufficientSignatures: session.crypto.x(), + // hasSufficientSignatures: crypto.x(), // additionLatency: Date.now() - signatureAdditionStart, // }, // 'Added signature' @@ -73,9 +69,9 @@ export function createDomainSignHandler( // Send response immediately once we cross threshold // BLS threshold signatures can be combined without all partial signatures - if (session.crypto.hasSufficientSignatures()) { + if (crypto.hasSufficientSignatures()) { try { - session.crypto.combineBlindedSignatureShares(request.body.blindedMessage, logger) + crypto.combineBlindedSignatureShares(request.body.blindedMessage, logger) // Close outstanding requests return true } catch (err) { @@ -88,23 +84,23 @@ export function createDomainSignHandler( return false } - await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( response.locals.logger, signers, signerEndpoint, request, - session.keyVersionInfo, - session.keyVersionInfo.keyVersion, + keyVersionInfo, + keyVersionInfo.keyVersion, config.odisServices.timeoutMilliSeconds, domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), processRequest ) - logDomainResponsesDiscrepancies(response.locals.logger, session.responses) + logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) - if (session.crypto.hasSufficientSignatures()) { + if (crypto.hasSufficientSignatures()) { try { - const combinedSignature = session.crypto.combineBlindedSignatureShares( + const combinedSignature = crypto.combineBlindedSignatureShares( request.body.blindedMessage, logger ) @@ -115,7 +111,7 @@ export function createDomainSignHandler( success: true, version: getCombinerVersion(), signature: combinedSignature, - status: findThresholdDomainState(session, signers.length), + status: findThresholdDomainState(keyVersionInfo, signerResponses, signers.length), }, 200, response.locals.logger @@ -128,9 +124,9 @@ export function createDomainSignHandler( } } - const errorCode = session.getMajorityErrorCode() ?? 500 + const errorCode = maxErrorCode ?? 500 const error = errorCodeToError(errorCode) - sendFailure(error, errorCode, session.response) + sendFailure(error, errorCode, response) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts b/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts index 74ffb18e45d..31b8a30326a 100644 --- a/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts +++ b/packages/phone-number-privacy/combiner/src/domain/services/threshold-state.ts @@ -1,17 +1,17 @@ -import { DomainRequest, DomainState } from '@celo/phone-number-privacy-common' -import { Session } from '../../common/session' +import { DomainRequest, DomainState, KeyVersionInfo } from '@celo/phone-number-privacy-common' +import { SignerResponse } from '../../common/io' export function findThresholdDomainState( - session: Session, + keyVersionInfo: KeyVersionInfo, + rawSignerResponses: Array>, totalSigners: number ): DomainState { + const { threshold } = keyVersionInfo // Get the domain status from the responses, filtering out responses that don't have the status. - const domainStates = session.responses + const domainStates = rawSignerResponses .map((signerResponse) => ('status' in signerResponse.res ? signerResponse.res.status : null)) .filter((state: DomainState | null | undefined): state is DomainState => !!state) - const { threshold } = session.keyVersionInfo - // Note: when the threshold > # total signers - threshold, it's possible that we // throw an error here when the domain is disabled. While the domain is technically disabled, // the hope is to increase the "safety margin" of the number of signers that have diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index 897d21a28aa..76ccdfb8c2a 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -15,7 +15,7 @@ import { Request } from 'express' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' -import { Session } from '../../../common/session' + import { getCombinerVersion, OdisConfig } from '../../../config' import { logFailOpenResponses, @@ -42,9 +42,8 @@ export function createPnpQuotaHandler( return } const keyVersionInfo = getKeyVersionInfo(request, config, logger) - const session = new Session(response, keyVersionInfo) - await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( logger, signers, signerEndpoint, @@ -54,22 +53,21 @@ export function createPnpQuotaHandler( config.odisServices.timeoutMilliSeconds, PnpQuotaResponseSchema ) + const warnings = logPnpSignerResponseDiscrepancies(logger, signerResponses) + logFailOpenResponses(logger, signerResponses) - session.warnings.push(...logPnpSignerResponseDiscrepancies(logger, session.responses)) - logFailOpenResponses(logger, session.responses) - - const { threshold } = session.keyVersionInfo + const { threshold } = keyVersionInfo - if (session.responses.length >= threshold) { + if (signerResponses.length >= threshold) { try { - const quotaStatus = findCombinerQuotaState(session) + const quotaStatus = findCombinerQuotaState(keyVersionInfo, signerResponses, warnings) send( response, { success: true, version: getCombinerVersion(), ...quotaStatus, - warnings: session.warnings, + warnings, }, 200, logger @@ -80,11 +78,7 @@ export function createPnpQuotaHandler( logger.error(err, 'Error combining signer quota status responses') } } - sendFailure( - ErrorMessage.THRESHOLD_PNP_QUOTA_STATUS_FAILURE, - session.getMajorityErrorCode() ?? 500, - response - ) + sendFailure(ErrorMessage.THRESHOLD_PNP_QUOTA_STATUS_FAILURE, maxErrorCode ?? 500, response) } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index 235c7d1da9b..6bc9a9df17c 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -19,9 +19,9 @@ import { Request } from 'express' import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' -import { CryptoSession } from '../../../common/crypto-session' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' + import { getCombinerVersion, OdisConfig } from '../../../config' import { logFailOpenResponses, @@ -52,22 +52,17 @@ export function createPnpSignHandler( return } const keyVersionInfo = getKeyVersionInfo(request, config, logger) - const session = new CryptoSession( - request, - response, - keyVersionInfo, - new BLSCryptographyClient(keyVersionInfo) - ) + const crypto = new BLSCryptographyClient(keyVersionInfo) const processRequest = async (result: OdisResponse): Promise => { assert(result.success) - session.crypto.addSignature({ url: 'TODO: remove', signature: result.signature }) + crypto.addSignature({ url: 'TODO: remove', signature: result.signature }) // const signatureAdditionStart = Date.now() // logger.info( // { // signer: url, - // hasSufficientSignatures: session.crypto.x(), + // hasSufficientSignatures: crypto.x(), // additionLatency: Date.now() - signatureAdditionStart, // }, // 'Added signature' @@ -75,9 +70,9 @@ export function createPnpSignHandler( // Send response immediately once we cross threshold // BLS threshold signatures can be combined without all partial signatures - if (session.crypto.hasSufficientSignatures()) { + if (crypto.hasSufficientSignatures()) { try { - session.crypto.combineBlindedSignatureShares(request.body.blindedQueryPhoneNumber, logger) + crypto.combineBlindedSignatureShares(request.body.blindedQueryPhoneNumber, logger) // Close outstanding requests return true } catch (err) { @@ -90,37 +85,36 @@ export function createPnpSignHandler( return false } - await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( logger, signers, signerEndpoint, request, - session.keyVersionInfo, - session.keyVersionInfo.keyVersion, + keyVersionInfo, + keyVersionInfo.keyVersion, config.odisServices.timeoutMilliSeconds, SignMessageResponseSchema, processRequest ) - session.warnings.push(...logPnpSignerResponseDiscrepancies(logger, session.responses)) - logFailOpenResponses(logger, session.responses) + const warnings = logPnpSignerResponseDiscrepancies(logger, signerResponses) + logFailOpenResponses(logger, signerResponses) - if (session.crypto.hasSufficientSignatures()) { + if (crypto.hasSufficientSignatures()) { try { - const combinedSignature = session.crypto.combineBlindedSignatureShares( - session.request.body.blindedQueryPhoneNumber, + const combinedSignature = crypto.combineBlindedSignatureShares( + request.body.blindedQueryPhoneNumber, logger ) - const quotaStatus = findCombinerQuotaState(session) return send( response, { success: true, version: getCombinerVersion(), signature: combinedSignature, - ...quotaStatus, - warnings: session.warnings, + ...findCombinerQuotaState(keyVersionInfo, signerResponses, warnings), + warnings, }, 200, logger @@ -132,9 +126,9 @@ export function createPnpSignHandler( } } - const errorCode = session.getMajorityErrorCode() ?? 500 + const errorCode = maxErrorCode ?? 500 const error = errorCodeToError(errorCode) - sendFailure(error, errorCode, session.response) + sendFailure(error, errorCode, response) } } diff --git a/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts b/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts index 0b919ee58da..739bc6c503c 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/services/threshold-state.ts @@ -1,17 +1,19 @@ import { - PnpQuotaRequest, + KeyVersionInfo, + OdisRequest, PnpQuotaStatus, - SignMessageRequest, WarningMessage, } from '@celo/phone-number-privacy-common' -import { Session } from '../../common/session' +import { SignerResponse } from '../../common/io' import { MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD } from '../../config' -export function findCombinerQuotaState( - session: Session +export function findCombinerQuotaState( + keyVersionInfo: KeyVersionInfo, + rawSignerResponses: Array>, + warnings: string[] ): PnpQuotaStatus { - const { threshold } = session.keyVersionInfo - const signerResponses = session.responses + const { threshold } = keyVersionInfo + const signerResponses = rawSignerResponses .map((signerResponse) => signerResponse.res) .filter((res) => res.success) as PnpQuotaStatus[] const sortedResponses = signerResponses.sort( @@ -28,7 +30,7 @@ export function findCombinerQuotaState 0) { - session.warnings.push( + warnings.push( WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + ', using threshold signer as best guess' ) From 530649c789c2068f5ea0447829e1e75a59b6c246 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 09:09:57 -0300 Subject: [PATCH 09/19] Fix typing errors --- packages/phone-number-privacy/combiner/src/common/combine.ts | 2 +- packages/phone-number-privacy/combiner/src/common/io.ts | 2 +- .../combiner/src/domain/endpoints/quota/action.ts | 4 ++-- .../combiner/src/domain/endpoints/sign/action.ts | 4 ++-- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 2e0c777ece7..26d6162cb18 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -21,7 +21,7 @@ export async function thresholdCallToSigners( logger: Logger, signers: Signer[], endpoint: string, - request: Request, + request: Request<{}, {}, R>, keyVersionInfo: KeyVersionInfo, keyVersion: number | null, requestTimeoutMS: number, diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 203ceacaf51..579d499381a 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -63,7 +63,7 @@ export async function fetchSignerResponseWithFallback( signer: Signer, signerEndpoint: string, keyVersion: number, - request: Request, + request: Request<{}, {}, R>, logger: Logger, abortSignal: AbortSignal ): Promise { diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index c696ec54ec4..2df739b7fa8 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -7,7 +7,7 @@ import { ErrorMessage, send, SequentialDelayDomainStateSchema, - verifyDisableDomainRequestAuthenticity, + verifyDomainQuotaStatusRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' import { Signer, thresholdCallToSigners } from '../../../common/combine' @@ -30,7 +30,7 @@ export function createDomainQuotaHandler( return } - if (!verifyDisableDomainRequestAuthenticity(request.body)) { + if (!verifyDomainQuotaStatusRequestAuthenticity(request.body)) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index 5ae2ea4e974..f4f99eefb59 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -9,7 +9,7 @@ import { OdisResponse, send, SequentialDelayDomainStateSchema, - verifyDisableDomainRequestAuthenticity, + verifyDomainRestrictedSignatureRequestAuthenticity, WarningMessage, } from '@celo/phone-number-privacy-common' import assert from 'node:assert' @@ -42,7 +42,7 @@ export function createDomainSignHandler( // Note that signing requests may include a nonce for replay protection that will be checked by // the signer, but is not checked here. As a result, requests that pass the authentication check // here may still fail when sent to the signer. - if (!verifyDisableDomainRequestAuthenticity(request.body)) { + if (!verifyDomainRestrictedSignatureRequestAuthenticity(request.body)) { sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return } From 58b25488bf295a75c5dfd464618560768c8a0637 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 09:45:09 -0300 Subject: [PATCH 10/19] fix tests --- .../combiner/test/integration/pnp.test.ts | 36 ++++----- .../test/unit/domain-response-logger.test.ts | 47 +++-------- .../test/unit/domain-threshold-state.test.ts | 80 ++++++++----------- 3 files changed, 64 insertions(+), 99 deletions(-) diff --git a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts index cf78f1dc0a7..8b8db3a0dec 100644 --- a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts @@ -378,7 +378,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: expectedQueryCount, totalQuota, - blockNumber: testBlockNumber, + warnings: expectedWarnings, }) }) @@ -396,7 +396,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -413,7 +413,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota, - blockNumber: testBlockNumber, + warnings: [], }) const res2 = await getCombinerQuotaResponse(req, authorization) @@ -435,7 +435,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -454,7 +454,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -474,7 +474,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota, - blockNumber: testBlockNumber, + warnings: [ WarningMessage.SIGNER_RESPONSE_DISCREPANCIES, WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + @@ -622,7 +622,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) const unblindedSig = threshold_bls.unblind( @@ -645,7 +645,7 @@ describe('pnpService', () => { signature: expectedSignatures[i - 1], performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) @@ -669,7 +669,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], } @@ -691,7 +691,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], } @@ -726,7 +726,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -743,7 +743,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -760,7 +760,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -776,7 +776,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) @@ -950,7 +950,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) const unblindedSig = threshold_bls.unblind( @@ -1067,7 +1067,7 @@ describe('pnpService', () => { version: expectedVersion, performedQueryCount: 0, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -1085,7 +1085,7 @@ describe('pnpService', () => { signature: expectedSignature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) }) @@ -1281,7 +1281,7 @@ describe('pnpService', () => { signature: res.body.signature, performedQueryCount: 1, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, + warnings: [], }) threshold_bls.unblind( diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts index 9f3179b9ada..bade7204f99 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts @@ -1,52 +1,25 @@ import { DisableDomainRequest, DomainQuotaStatusRequest, - DomainRequest, DomainRestrictedSignatureRequest, - KeyVersionInfo, OdisResponse, rootLogger, WarningMessage, } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' -import { Request, Response } from 'express' -import { Session } from '../../src/common/session' + import config from '../../src/config' -import { DomainSignerResponseLogger } from '../../src/domain/services/log-responses' +import { logDomainResponsesDiscrepancies } from '../../src/domain/services/log-responses' describe('domain response logger', () => { const url = 'test signer url' - const keyVersionInfo: KeyVersionInfo = { - keyVersion: 1, - threshold: 3, - polynomial: 'mock polynomial', - pubKey: 'mock pubKey', - } - - const getSession = (responses: OdisResponse[]) => { - const mockRequest = { - body: {}, - } as Request - - // @ts-ignore: missing some properties - const mockResponse = { - locals: { - logger: rootLogger(config.serviceName), - }, - } as Response - const session = new Session(mockResponse, keyVersionInfo) - responses.forEach((res) => { - session.responses.push({ url, res, status: 200 }) - }) - return session - } + const logger = rootLogger(config.serviceName) const version = getSignerVersion() const counter = 1 const disabled = false const timer = 10000 - const domainResponseLogger = new DomainSignerResponseLogger() const testCases: { it: string @@ -317,26 +290,28 @@ describe('domain response logger', () => { ] testCases.forEach((testCase) => { it(testCase.it, () => { - const session = getSession(testCase.responses) const logSpys = { info: { - spy: jest.spyOn(session.logger, 'info'), + spy: jest.spyOn(logger, 'info'), callCount: 0, }, debug: { - spy: jest.spyOn(session.logger, 'debug'), + spy: jest.spyOn(logger, 'debug'), callCount: 0, }, warn: { - spy: jest.spyOn(session.logger, 'warn'), + spy: jest.spyOn(logger, 'warn'), callCount: 0, }, error: { - spy: jest.spyOn(session.logger, 'error'), + spy: jest.spyOn(logger, 'error'), callCount: 0, }, } - domainResponseLogger.logResponseDiscrepancies(session) + logDomainResponsesDiscrepancies( + logger, + testCase.responses.map((res) => ({ url, res })) + ) testCase.expectedLogs.forEach((log) => { expect(logSpys[log.level].spy).toHaveBeenNthCalledWith( ++logSpys[log.level].callCount, diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts index 4b99426244d..9c13c606023 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts @@ -2,14 +2,10 @@ import { DomainQuotaStatusResponseSuccess, DomainRestrictedSignatureResponseSuccess, KeyVersionInfo, - SequentialDelayDomainState, } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' -import Logger from 'bunyan' -import { Request, Response } from 'express' -import { Session } from '../../src/common/session' -import config from '../../src/config' -import { DomainThresholdStateService } from '../../src/domain/services/threshold-state' + +import { findThresholdDomainState } from '../../src/domain/services/threshold-state' describe('domain threshold state', () => { // TODO add tests with failed signer responses, depending on @@ -22,40 +18,12 @@ describe('domain threshold state', () => { pubKey: 'mock pubKey', } - const getSession = (domainStates: SequentialDelayDomainState[]) => { - const mockRequest = { - body: {}, - } as Request - - // @ts-ignore: missing some properties - const mockResponse = { - locals: { - logger: new Logger({ name: 'logger' }), - }, - } as Response - const session = new Session(mockResponse, keyVersionInfo) - domainStates.forEach((status) => { - const res: DomainRestrictedSignatureResponseSuccess | DomainQuotaStatusResponseSuccess = { - success: true, - version: expectedVersion, - status, - } - session.responses.push({ url: 'random url', res, status: 200 }) - }) - return session - } - - const domainConfig = config.domains - domainConfig.keys.currentVersion = keyVersionInfo.keyVersion - domainConfig.keys.versions = JSON.stringify([keyVersionInfo]) - domainConfig.odisServices.signers = JSON.stringify([ - { url: 'http://localhost:3001', fallbackUrl: 'http://localhost:3001/fallback' }, - { url: 'http://localhost:3002', fallbackUrl: 'http://localhost:3002/fallback' }, - { url: 'http://localhost:3003', fallbackUrl: 'http://localhost:3003/fallback' }, - { url: 'http://localhost:4004', fallbackUrl: 'http://localhost:4004/fallback' }, - ]) + // const keyVersionInfo: KeyVersionInfo = { + // currentVersion: keyVersionInfo.keyVersion, + // versions: JSON.stringify([keyVersionInfo]), + // } - const domainThresholdStateService = new DomainThresholdStateService(domainConfig) + const totalSigners = 4 const expectedVersion = getSignerVersion() const now = Date.now() @@ -137,8 +105,15 @@ describe('domain threshold state', () => { varyingDomainStates.forEach(({ statuses, expectedCounter, expectedTimer }) => { it(`should return counter:${expectedCounter} and timer:${expectedTimer} given the domain states: ${statuses}`, () => { - const session = getSession(statuses) - const thresholdResult = domainThresholdStateService.findThresholdDomainState(session) + const responses = statuses.map((status) => { + const res: DomainRestrictedSignatureResponseSuccess | DomainQuotaStatusResponseSuccess = { + success: true, + version: expectedVersion, + status, + } + return { url: 'random url', res, status: 200 } + }) + const thresholdResult = findThresholdDomainState(keyVersionInfo, responses, totalSigners) expect(thresholdResult).toStrictEqual({ timer: expectedTimer, @@ -156,8 +131,16 @@ describe('domain threshold state', () => { { timer, counter: 2, disabled: false, now }, { timer, counter: 2, disabled: false, now }, ] - const session = getSession(statuses) - const thresholdResult = domainThresholdStateService.findThresholdDomainState(session) + + const responses = statuses.map((status) => { + const res: DomainRestrictedSignatureResponseSuccess | DomainQuotaStatusResponseSuccess = { + success: true, + version: expectedVersion, + status, + } + return { url: 'random url', res, status: 200 } + }) + const thresholdResult = findThresholdDomainState(keyVersionInfo, responses, totalSigners) expect(thresholdResult).toStrictEqual({ timer: 0, counter: 0, disabled: true, now: 0 }) }) @@ -168,9 +151,16 @@ describe('domain threshold state', () => { { timer, counter: 2, disabled: false, now }, { timer, counter: 2, disabled: false, now }, ] - const session = getSession(statuses) + const responses = statuses.map((status) => { + const res: DomainRestrictedSignatureResponseSuccess | DomainQuotaStatusResponseSuccess = { + success: true, + version: expectedVersion, + status, + } + return { url: 'random url', res, status: 200 } + }) - expect(() => domainThresholdStateService.findThresholdDomainState(session)).toThrow( + expect(() => findThresholdDomainState(keyVersionInfo, responses, totalSigners)).toThrow( 'Insufficient number of signer responses. Domain may be disabled' ) }) From 9c38953c7b378be50ef92366345b30bc4112a5cc Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 10:13:00 -0300 Subject: [PATCH 11/19] Uses httpAgent for http/s connections --- packages/phone-number-privacy/combiner/src/common/io.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 579d499381a..48cbd14e72b 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -10,11 +10,16 @@ import { } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' +import * as http from 'http' +import * as https from 'https' import fetch, { Response as FetchResponse } from 'node-fetch' import { performance } from 'perf_hooks' import { getCombinerVersion, OdisConfig } from '../config' import { Signer } from './combine' +const httpAgent = new http.Agent({ keepAlive: true }) +const httpsAgent = new https.Agent({ keepAlive: true }) + // tslint:disable-next-line: interface-over-type-literal export type SignerResponse = { url: string @@ -82,6 +87,7 @@ export async function fetchSignerResponseWithFallback( }, body: JSON.stringify(request.body), signal: abortSignal, + agent: url.startsWith("https://") ? httpsAgent : httpAgent }) } From aa88b520c52ad325340de5a5fa58b3e541e4d841 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 14:02:40 -0300 Subject: [PATCH 12/19] use shouldCheckKeyVerion boolean argument --- .../phone-number-privacy/combiner/src/common/combine.ts | 6 +++--- .../combiner/src/domain/endpoints/disable/action.ts | 1 - .../combiner/src/domain/endpoints/quota/action.ts | 1 - .../combiner/src/domain/endpoints/sign/action.ts | 2 +- .../combiner/src/pnp/endpoints/quota/action.ts | 1 - .../combiner/src/pnp/endpoints/sign/action.ts | 2 +- 6 files changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 26d6162cb18..9b2c27a3c9b 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -23,9 +23,9 @@ export async function thresholdCallToSigners( endpoint: string, request: Request<{}, {}, R>, keyVersionInfo: KeyVersionInfo, - keyVersion: number | null, requestTimeoutMS: number, responseSchema: t.Type, OdisResponse, unknown>, + shouldCheckKeyVersion: boolean = false, processResult: (res: OdisResponse) => Promise = (_) => Promise.resolve(false) ): Promise<{ signerResponses: Array>; maxErrorCode?: number }> { const obs = new PerformanceObserver((list) => { @@ -83,8 +83,8 @@ export async function thresholdCallToSigners( // if given key version, check that if ( - keyVersion != null && - !responseHasExpectedKeyVersion(signerFetchResult, keyVersion, logger) + shouldCheckKeyVersion && + !responseHasExpectedKeyVersion(signerFetchResult, keyVersionInfo.keyVersion, logger) ) { throw new Error(ErrorMessage.INVALID_KEY_VERSION_RESPONSE) } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index f0a92b71010..13f2fe97de6 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -43,7 +43,6 @@ export function createDisableDomainHandler( signerEndpoint, request, keyVersionInfo, - null, config.odisServices.timeoutMilliSeconds, disableDomainResponseSchema(SequentialDelayDomainStateSchema) ) diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index 2df739b7fa8..d414ea0499e 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -43,7 +43,6 @@ export function createDomainQuotaHandler( signerEndpoint, request, keyVersionInfo, - null, config.odisServices.timeoutMilliSeconds, domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) ) diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index f4f99eefb59..47d7fe021b1 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -90,9 +90,9 @@ export function createDomainSignHandler( signerEndpoint, request, keyVersionInfo, - keyVersionInfo.keyVersion, config.odisServices.timeoutMilliSeconds, domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), + true, processRequest ) diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index 76ccdfb8c2a..d08530df87d 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -49,7 +49,6 @@ export function createPnpQuotaHandler( signerEndpoint, request, keyVersionInfo, - null, config.odisServices.timeoutMilliSeconds, PnpQuotaResponseSchema ) diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index 6bc9a9df17c..850938180d5 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -91,9 +91,9 @@ export function createPnpSignHandler( signerEndpoint, request, keyVersionInfo, - keyVersionInfo.keyVersion, config.odisServices.timeoutMilliSeconds, SignMessageResponseSchema, + true, processRequest ) From eb76a81ba3c90dc20dd061b78f6aeefb53ef2c75 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 14:04:53 -0300 Subject: [PATCH 13/19] fix some tests --- .../test/unit/pnp-response-logger.test.ts | 235 +++--------------- .../test/unit/pnp-threshold-state.test.ts | 44 ++-- 2 files changed, 54 insertions(+), 225 deletions(-) diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts index 1cb3aabedd8..de9d5bc76de 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts @@ -8,14 +8,14 @@ import { WarningMessage, } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' -import { Request, Response } from 'express' -import { Session } from '../../src/common/session' import config, { - MAX_BLOCK_DISCREPANCY_THRESHOLD, MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, } from '../../src/config' -import { PnpSignerResponseLogger } from '../../src/pnp/services/log-responses' +import { + logFailOpenResponses, + logPnpSignerResponseDiscrepancies, +} from '../../src/pnp/services/log-responses' describe('pnp response logger', () => { const url = 'test signer url' @@ -27,31 +27,12 @@ describe('pnp response logger', () => { pubKey: 'mock pubKey', } - const getSession = (responses: OdisResponse[]) => { - const mockRequest = { - body: {}, - } as Request - - // @ts-ignore: missing some properties - const mockResponse = { - locals: { - logger: rootLogger(config.serviceName), - }, - } as Response - const session = new Session(mockResponse, keyVersionInfo) - responses.forEach((res) => { - session.responses.push({ url, res, status: 200 }) - }) - return session - } - const pnpConfig = config.phoneNumberPrivacy pnpConfig.keys.currentVersion = keyVersionInfo.keyVersion pnpConfig.keys.versions = JSON.stringify([keyVersionInfo]) - const pnpResponseLogger = new PnpSignerResponseLogger() const version = getSignerVersion() - const blockNumber = 1000000 + const totalQuota = 10 const performedQueryCount = 5 const warnings = ['warning'] @@ -77,9 +58,9 @@ describe('pnp response logger', () => { { it: 'should log correctly when all the responses are the same', responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, ], expectedLogs: [], }, @@ -91,11 +72,11 @@ describe('pnp response logger', () => { performedQueryCount, totalQuota, version: 'differentVersion', - blockNumber, + warnings, }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, ], expectedLogs: [ { @@ -105,7 +86,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version: 'differentVersion', @@ -115,7 +95,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -125,7 +104,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -143,9 +121,9 @@ describe('pnp response logger', () => { { it: 'should log correctly when there is a discrepency in performedQueryCount field', responses: [ - { success: true, performedQueryCount: 1, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount: 1, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, ], expectedLogs: [ { @@ -155,7 +133,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount: 1, totalQuota, version, @@ -165,7 +142,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -175,7 +151,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -198,11 +173,11 @@ describe('pnp response logger', () => { performedQueryCount: performedQueryCount + MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, totalQuota, version, - blockNumber, + warnings, }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, ], expectedLogs: [ { @@ -212,7 +187,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -222,7 +196,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -232,7 +205,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount: performedQueryCount + MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, totalQuota, @@ -253,7 +225,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -263,7 +234,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -273,7 +243,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount: performedQueryCount + MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, totalQuota, @@ -292,9 +261,9 @@ describe('pnp response logger', () => { { it: 'should log correctly when there is a discrepency in totalQuota field', responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota: 1, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota: 1, version, warnings }, ], expectedLogs: [ { @@ -304,7 +273,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota: 1, version, @@ -314,7 +282,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -324,7 +291,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -342,14 +308,14 @@ describe('pnp response logger', () => { { it: 'should log correctly when there is a large discrepency in totalQuota field', responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, { success: true, performedQueryCount, totalQuota: totalQuota + MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, version, - blockNumber, + warnings, }, ], @@ -361,7 +327,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -371,7 +336,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -381,7 +345,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota: totalQuota + MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, version, @@ -396,135 +359,17 @@ describe('pnp response logger', () => { }, ], }, - { - it: 'should log correctly when one signer returns an undefined blockNumber', - responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { - success: true, - performedQueryCount, - totalQuota, - version, - blockNumber: undefined, - warnings, - }, - ], - expectedLogs: [ - { - params: [ - { - parsedResponses: [ - { - signerUrl: url, - values: { - blockNumber, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - { - signerUrl: url, - values: { - blockNumber, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - { - signerUrl: url, - values: { - blockNumber: undefined, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - ], - }, - WarningMessage.SIGNER_RESPONSE_DISCREPANCIES, - ], - level: 'warn', - }, - { - params: [{ signerUrl: url }, 'Signer responded with undefined blockNumber'], - level: 'warn', - }, - ], - }, - { - it: 'should log correctly when there is a large discrepency in blockNumber field', - responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { - success: true, - performedQueryCount, - totalQuota, - version, - blockNumber: blockNumber + MAX_BLOCK_DISCREPANCY_THRESHOLD, - warnings, - }, - ], - expectedLogs: [ - { - params: [ - { - sortedByBlockNumber: [ - { - signerUrl: url, - values: { - blockNumber, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - { - signerUrl: url, - values: { - blockNumber, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - { - signerUrl: url, - values: { - blockNumber: blockNumber + MAX_BLOCK_DISCREPANCY_THRESHOLD, - performedQueryCount, - totalQuota, - version, - warnings, - }, - }, - ], - }, - WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS, - ], - level: 'error', - }, - ], - }, { it: 'should log correctly when there is a discrepency in warnings field', responses: [ - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, - { success: true, performedQueryCount, totalQuota, version, blockNumber, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, + { success: true, performedQueryCount, totalQuota, version, warnings }, { success: true, performedQueryCount, totalQuota, version, - blockNumber, + warnings: ['differentWarning'], }, ], @@ -536,7 +381,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -546,7 +390,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -556,7 +399,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -579,7 +421,7 @@ describe('pnp response logger', () => { performedQueryCount, totalQuota, version, - blockNumber, + warnings: [ErrorMessage.FAILING_OPEN], }, { @@ -587,7 +429,7 @@ describe('pnp response logger', () => { performedQueryCount, totalQuota, version, - blockNumber, + warnings: [ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA], }, { @@ -595,7 +437,7 @@ describe('pnp response logger', () => { performedQueryCount, totalQuota, version, - blockNumber, + warnings: [ErrorMessage.FAILURE_TO_GET_DEK], }, ], @@ -607,7 +449,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -617,7 +458,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -627,7 +467,6 @@ describe('pnp response logger', () => { { signerUrl: url, values: { - blockNumber, performedQueryCount, totalQuota, version, @@ -675,27 +514,29 @@ describe('pnp response logger', () => { ] testCases.forEach((testCase) => { it(testCase.it, () => { - const session = getSession(testCase.responses) + const logger = rootLogger(config.serviceName) + + const responses = testCase.responses.map((res) => ({ res, url })) const logSpys = { info: { - spy: jest.spyOn(session.logger, 'info'), + spy: jest.spyOn(logger, 'info'), callCount: 0, }, debug: { - spy: jest.spyOn(session.logger, 'debug'), + spy: jest.spyOn(logger, 'debug'), callCount: 0, }, warn: { - spy: jest.spyOn(session.logger, 'warn'), + spy: jest.spyOn(logger, 'warn'), callCount: 0, }, error: { - spy: jest.spyOn(session.logger, 'error'), + spy: jest.spyOn(logger, 'error'), callCount: 0, }, } - pnpResponseLogger.logResponseDiscrepancies(session) - pnpResponseLogger.logFailOpenResponses(session) + logPnpSignerResponseDiscrepancies(logger, responses) + logFailOpenResponses(logger, responses) testCase.expectedLogs.forEach((log) => { expect(logSpys[log.level].spy).toHaveBeenNthCalledWith( ++logSpys[log.level].callCount, diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts index fadeb9f630e..587d5c0b8a5 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts @@ -9,9 +9,8 @@ import { } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' import { Request, Response } from 'express' -import { Session } from '../../src/common/session' import config from '../../src/config' -import { PnpThresholdStateService } from '../../src/pnp/services/threshold-state' +import { findCombinerQuotaState } from '../../src/pnp/services/threshold-state' describe('pnp threshold state', () => { // TODO add tests with failed signer responses, depending on @@ -24,33 +23,9 @@ describe('pnp threshold state', () => { pubKey: 'mock pubKey', } - const getSession = (quotaData: { totalQuota: number; performedQueryCount: number }[]) => { - const mockRequest = { - body: {}, - } as Request - - // @ts-ignore: missing some properties - const mockResponse = { - locals: { - logger: rootLogger, - }, - } as Response - const session = new Session(mockResponse, keyVersionInfo) - quotaData.forEach((q) => { - const res: PnpQuotaResponseSuccess | SignMessageResponseSuccess = { - success: true, - version: expectedVersion, - ...q, - } - session.responses.push({ url: 'random url', res, status: 200 }) - }) - return session - } - const pnpConfig = config.phoneNumberPrivacy pnpConfig.keys.currentVersion = keyVersionInfo.keyVersion pnpConfig.keys.versions = JSON.stringify([keyVersionInfo]) - const pnpThresholdStateService = new PnpThresholdStateService() const expectedVersion = getSignerVersion() const testBlockNumber = 1000000 @@ -106,8 +81,21 @@ describe('pnp threshold state', () => { ] varyingQueryCount.forEach(({ signerRes, expectedQueryCount }) => { it(`should return ${expectedQueryCount} performedQueryCount given signer responses of ${signerRes}`, () => { - const session = getSession(signerRes) - const thresholdResult = pnpThresholdStateService.findCombinerQuotaState(session) + + signerRes.map((o) => ( + url: 'random url', + status: 200, + res: { + success: true, + version: expectedVersion, + ...q, + } + }) + session.responses.push({ url: 'random url', res, status: 200 }) + + + const warnings: string[] = [] + const thresholdResult = findCombinerQuotaState(keyVersionInfo, null, warnings) expect(thresholdResult).toStrictEqual({ performedQueryCount: expectedQueryCount, totalQuota, From f47f7c601e1924254ac9ddaca9094de8097ebbb1 Mon Sep 17 00:00:00 2001 From: alecps Date: Wed, 23 Aug 2023 16:05:50 -0400 Subject: [PATCH 14/19] cosmetic cleanup from PR review --- .../combiner/src/common/combine.ts | 35 +++--- .../combiner/src/common/io.ts | 5 +- .../src/domain/endpoints/disable/action.ts | 28 ++--- .../src/domain/endpoints/quota/action.ts | 25 ++--- .../src/domain/endpoints/sign/action.ts | 38 +++---- .../src/domain/services/log-responses.ts | 10 +- .../src/pnp/endpoints/quota/action.ts | 28 ++--- .../combiner/src/pnp/endpoints/sign/action.ts | 32 +++--- .../src/pnp/services/log-responses.ts | 53 +-------- .../combiner/src/server.ts | 3 - .../test/unit/domain-response-logger.test.ts | 4 +- .../test/unit/domain-threshold-state.test.ts | 6 - .../test/unit/pnp-response-logger.test.ts | 104 +----------------- 13 files changed, 107 insertions(+), 264 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index 9b2c27a3c9b..aa616c17b2f 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -4,6 +4,7 @@ import { OdisRequest, OdisResponse, responseHasExpectedKeyVersion, + SignerEndpoint, WarningMessage, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' @@ -17,15 +18,19 @@ export interface Signer { fallbackUrl?: string } +export interface ThresholdCallToSignersOptions { + signers: Signer[] + endpoint: SignerEndpoint + requestTimeoutMS: number + shouldCheckKeyVersion: boolean + keyVersionInfo: KeyVersionInfo + request: Request<{}, {}, R> + responseSchema: t.Type, OdisResponse, unknown> +} + export async function thresholdCallToSigners( logger: Logger, - signers: Signer[], - endpoint: string, - request: Request<{}, {}, R>, - keyVersionInfo: KeyVersionInfo, - requestTimeoutMS: number, - responseSchema: t.Type, OdisResponse, unknown>, - shouldCheckKeyVersion: boolean = false, + options: ThresholdCallToSignersOptions, processResult: (res: OdisResponse) => Promise = (_) => Promise.resolve(false) ): Promise<{ signerResponses: Array>; maxErrorCode?: number }> { const obs = new PerformanceObserver((list) => { @@ -42,6 +47,16 @@ export async function thresholdCallToSigners( }) obs.observe({ entryTypes: ['measure'], buffered: false }) + const { + signers, + endpoint, + requestTimeoutMS, + shouldCheckKeyVersion, + keyVersionInfo, + request, + responseSchema, + } = options + const manualAbort = new AbortController() const timeoutSignal = AbortSignal.timeout(requestTimeoutMS) const abortSignal = (AbortSignal as any).any([manualAbort.signal, timeoutSignal]) as AbortSignal @@ -81,7 +96,6 @@ export async function thresholdCallToSigners( return } - // if given key version, check that if ( shouldCheckKeyVersion && !responseHasExpectedKeyVersion(signerFetchResult, keyVersionInfo.keyVersion, logger) @@ -120,16 +134,11 @@ export async function thresholdCallToSigners( logger.error({ signer }, ErrorMessage.SIGNER_REQUEST_ERROR) logger.error({ signer, err }) - // Tracking failed request count via signer url prevents - // double counting the same failed request by mistake errorCount++ if (signers.length - errorCount < requiredThreshold) { logger.warn('Not possible to reach a threshold of signer responses. Failing fast') manualAbort.abort() } - - // TODO (mcortesi) doesn't seem we need to fail at first error - // throw err } } }) diff --git a/packages/phone-number-privacy/combiner/src/common/io.ts b/packages/phone-number-privacy/combiner/src/common/io.ts index 48cbd14e72b..60abd19cd7b 100644 --- a/packages/phone-number-privacy/combiner/src/common/io.ts +++ b/packages/phone-number-privacy/combiner/src/common/io.ts @@ -1,12 +1,13 @@ import { ErrorType, getRequestKeyVersion, - KeyVersionInfo, KEY_VERSION_HEADER, + KeyVersionInfo, OdisRequest, OdisResponse, requestHasValidKeyVersion, send, + SignerEndpoint, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { Request, Response } from 'express' @@ -66,7 +67,7 @@ export function getKeyVersionInfo( export async function fetchSignerResponseWithFallback( signer: Signer, - signerEndpoint: string, + signerEndpoint: SignerEndpoint, keyVersion: number, request: Request<{}, {}, R>, logger: Logger, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts index 13f2fe97de6..8427bdee30f 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/disable/action.ts @@ -5,6 +5,7 @@ import { disableDomainResponseSchema, DomainSchema, ErrorMessage, + getSignerEndpoint, send, SequentialDelayDomainStateSchema, verifyDisableDomainRequestAuthenticity, @@ -13,19 +14,16 @@ import { import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' - import { getCombinerVersion, OdisConfig } from '../../../config' -import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { logDomainResponseDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' export function createDisableDomainHandler( signers: Signer[], config: OdisConfig ): PromiseHandler { - const requestSchema = disableDomainRequestSchema(DomainSchema) - const signerEndpoint = CombinerEndpoint.DISABLE_DOMAIN return async (request, response) => { - if (!requestSchema.is(request.body)) { + if (!disableDomainRequestSchema(DomainSchema).is(request.body)) { sendFailure(WarningMessage.INVALID_INPUT, 400, response) return } @@ -35,19 +33,23 @@ export function createDisableDomainHandler( return } + // TODO remove? const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) - const { signerResponses, maxErrorCode } = await thresholdCallToSigners( + const { signerResponses, maxErrorCode } = await thresholdCallToSigners( response.locals.logger, - signers, - signerEndpoint, - request, - keyVersionInfo, - config.odisServices.timeoutMilliSeconds, - disableDomainResponseSchema(SequentialDelayDomainStateSchema) + { + signers, + endpoint: getSignerEndpoint(CombinerEndpoint.DISABLE_DOMAIN), + request, + keyVersionInfo, + requestTimeoutMS: config.odisServices.timeoutMilliSeconds, + responseSchema: disableDomainResponseSchema(SequentialDelayDomainStateSchema), + shouldCheckKeyVersion: false, + } ) - logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) + logDomainResponseDiscrepancies(response.locals.logger, signerResponses) try { const disableDomainStatus = findThresholdDomainState( keyVersionInfo, diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts index d414ea0499e..8d80ee871a5 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/quota/action.ts @@ -5,6 +5,7 @@ import { domainQuotaStatusResponseSchema, DomainSchema, ErrorMessage, + getSignerEndpoint, send, SequentialDelayDomainStateSchema, verifyDomainQuotaStatusRequestAuthenticity, @@ -13,19 +14,16 @@ import { import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' - import { getCombinerVersion, OdisConfig } from '../../../config' -import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { logDomainResponseDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' export function createDomainQuotaHandler( signers: Signer[], config: OdisConfig ): PromiseHandler { - const requestSchema = domainQuotaStatusRequestSchema(DomainSchema) - const signerEndpoint = CombinerEndpoint.DOMAIN_QUOTA_STATUS return async (request, response) => { - if (!requestSchema.is(request.body)) { + if (!domainQuotaStatusRequestSchema(DomainSchema).is(request.body)) { sendFailure(WarningMessage.INVALID_INPUT, 400, response) return } @@ -35,19 +33,20 @@ export function createDomainQuotaHandler( return } + // TODO remove? const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) - const { signerResponses, maxErrorCode } = await thresholdCallToSigners( - response.locals.logger, + const { signerResponses, maxErrorCode } = await thresholdCallToSigners(response.locals.logger, { signers, - signerEndpoint, + endpoint: getSignerEndpoint(CombinerEndpoint.DOMAIN_QUOTA_STATUS), request, keyVersionInfo, - config.odisServices.timeoutMilliSeconds, - domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema) - ) + requestTimeoutMS: config.odisServices.timeoutMilliSeconds, + responseSchema: domainQuotaStatusResponseSchema(SequentialDelayDomainStateSchema), + shouldCheckKeyVersion: false, + }) - logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) + logDomainResponseDiscrepancies(response.locals.logger, signerResponses) if (signerResponses.length >= keyVersionInfo.threshold) { try { send( @@ -65,6 +64,6 @@ export function createDomainQuotaHandler( response.locals.logger.error(err, 'Error combining signer quota status responses') } } - sendFailure(ErrorMessage.THRESHOLD_DISABLE_DOMAIN_FAILURE, maxErrorCode ?? 500, response) + sendFailure(ErrorMessage.THRESHOLD_DOMAIN_QUOTA_STATUS_FAILURE, maxErrorCode ?? 500, response) } } diff --git a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts index 47d7fe021b1..f58422dc5d3 100644 --- a/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/domain/endpoints/sign/action.ts @@ -6,6 +6,7 @@ import { DomainSchema, ErrorMessage, ErrorType, + getSignerEndpoint, OdisResponse, send, SequentialDelayDomainStateSchema, @@ -15,26 +16,24 @@ import { import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { DomainCryptoClient } from '../../../common/crypto-clients/domain-crypto-client' - import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' - import { getCombinerVersion, OdisConfig } from '../../../config' -import { logDomainResponsesDiscrepancies } from '../../services/log-responses' +import { logDomainResponseDiscrepancies } from '../../services/log-responses' import { findThresholdDomainState } from '../../services/threshold-state' export function createDomainSignHandler( signers: Signer[], config: OdisConfig ): PromiseHandler { - const requestSchema = domainRestrictedSignatureRequestSchema(DomainSchema) - const signerEndpoint = CombinerEndpoint.DOMAIN_SIGN return async (request, response) => { - if (!requestSchema.is(request.body)) { + const { logger } = response.locals + + if (!domainRestrictedSignatureRequestSchema(DomainSchema).is(request.body)) { sendFailure(WarningMessage.INVALID_INPUT, 400, response) return } - if (!requestHasSupportedKeyVersion(request, config, response.locals.logger)) { + if (!requestHasSupportedKeyVersion(request, config, logger)) { sendFailure(WarningMessage.INVALID_KEY_VERSION_REQUEST, 400, response) return } @@ -47,11 +46,10 @@ export function createDomainSignHandler( return } - const keyVersionInfo = getKeyVersionInfo(request, config, response.locals.logger) + const keyVersionInfo = getKeyVersionInfo(request, config, logger) const crypto = new DomainCryptoClient(keyVersionInfo) - const logger = response.locals.logger - const processRequest = async ( + const processResult = async ( res: OdisResponse ): Promise => { assert(res.success) @@ -86,17 +84,19 @@ export function createDomainSignHandler( const { signerResponses, maxErrorCode } = await thresholdCallToSigners( response.locals.logger, - signers, - signerEndpoint, - request, - keyVersionInfo, - config.odisServices.timeoutMilliSeconds, - domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), - true, - processRequest + { + signers, + endpoint: getSignerEndpoint(CombinerEndpoint.DOMAIN_SIGN), + request, + keyVersionInfo, + requestTimeoutMS: config.odisServices.timeoutMilliSeconds, + responseSchema: domainRestrictedSignatureResponseSchema(SequentialDelayDomainStateSchema), + shouldCheckKeyVersion: true, + }, + processResult ) - logDomainResponsesDiscrepancies(response.locals.logger, signerResponses) + logDomainResponseDiscrepancies(response.locals.logger, signerResponses) if (crypto.hasSufficientSignatures()) { try { diff --git a/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts b/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts index ba35e6234f0..7f4b3bebb13 100644 --- a/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts +++ b/packages/phone-number-privacy/combiner/src/domain/services/log-responses.ts @@ -1,14 +1,10 @@ -import { - DomainRequest, - DomainRestrictedSignatureRequest, - WarningMessage, -} from '@celo/phone-number-privacy-common' +import { DomainRequest, WarningMessage } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { SignerResponse } from '../../common/io' -export function logDomainResponsesDiscrepancies( +export function logDomainResponseDiscrepancies( logger: Logger, - responses: Array> + responses: Array> ) { const parsedResponses: Array<{ signerUrl: string diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts index d08530df87d..26af9325fec 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/quota/action.ts @@ -3,6 +3,7 @@ import { CombinerEndpoint, DataEncryptionKeyFetcher, ErrorMessage, + getSignerEndpoint, hasValidAccountParam, isBodyReasonablySized, PnpQuotaRequest, @@ -15,12 +16,8 @@ import { Request } from 'express' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, sendFailure } from '../../../common/io' - import { getCombinerVersion, OdisConfig } from '../../../config' -import { - logFailOpenResponses, - logPnpSignerResponseDiscrepancies, -} from '../../services/log-responses' +import { logPnpSignerResponseDiscrepancies } from '../../services/log-responses' import { findCombinerQuotaState } from '../../services/threshold-state' export function createPnpQuotaHandler( @@ -28,10 +25,9 @@ export function createPnpQuotaHandler( config: OdisConfig, dekFetcher: DataEncryptionKeyFetcher ): PromiseHandler { - const signerEndpoint = CombinerEndpoint.PNP_QUOTA - return async (request, response) => { const logger = response.locals.logger + if (!validateRequest(request)) { sendFailure(WarningMessage.INVALID_INPUT, 400, response) return @@ -41,19 +37,23 @@ export function createPnpQuotaHandler( sendFailure(WarningMessage.UNAUTHENTICATED_USER, 401, response) return } + + // TODO remove? const keyVersionInfo = getKeyVersionInfo(request, config, logger) - const { signerResponses, maxErrorCode } = await thresholdCallToSigners( - logger, + const { signerResponses, maxErrorCode } = await thresholdCallToSigners(logger, { signers, - signerEndpoint, + endpoint: getSignerEndpoint(CombinerEndpoint.PNP_QUOTA), request, keyVersionInfo, - config.odisServices.timeoutMilliSeconds, - PnpQuotaResponseSchema - ) + requestTimeoutMS: config.odisServices.timeoutMilliSeconds, + responseSchema: PnpQuotaResponseSchema, + shouldCheckKeyVersion: false, + }) const warnings = logPnpSignerResponseDiscrepancies(logger, signerResponses) - logFailOpenResponses(logger, signerResponses) + + // TODO remove? + // logFailOpenResponses(logger, signerResponses) const { threshold } = keyVersionInfo diff --git a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts index 850938180d5..23458061d87 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/endpoints/sign/action.ts @@ -4,6 +4,7 @@ import { DataEncryptionKeyFetcher, ErrorMessage, ErrorType, + getSignerEndpoint, hasValidAccountParam, hasValidBlindedPhoneNumberParam, isBodyReasonablySized, @@ -14,19 +15,14 @@ import { SignMessageResponseSchema, WarningMessage, } from '@celo/phone-number-privacy-common' - import { Request } from 'express' import assert from 'node:assert' import { Signer, thresholdCallToSigners } from '../../../common/combine' import { BLSCryptographyClient } from '../../../common/crypto-clients/bls-crypto-client' import { PromiseHandler } from '../../../common/handlers' import { getKeyVersionInfo, requestHasSupportedKeyVersion, sendFailure } from '../../../common/io' - import { getCombinerVersion, OdisConfig } from '../../../config' -import { - logFailOpenResponses, - logPnpSignerResponseDiscrepancies, -} from '../../services/log-responses' +import { logPnpSignerResponseDiscrepancies } from '../../services/log-responses' import { findCombinerQuotaState } from '../../services/threshold-state' export function createPnpSignHandler( @@ -34,7 +30,6 @@ export function createPnpSignHandler( config: OdisConfig, dekFetcher: DataEncryptionKeyFetcher ): PromiseHandler { - const signerEndpoint = CombinerEndpoint.PNP_SIGN return async (request, response) => { const logger = response.locals.logger if (!validateRequest(request)) { @@ -54,7 +49,7 @@ export function createPnpSignHandler( const keyVersionInfo = getKeyVersionInfo(request, config, logger) const crypto = new BLSCryptographyClient(keyVersionInfo) - const processRequest = async (result: OdisResponse): Promise => { + const processResult = async (result: OdisResponse): Promise => { assert(result.success) crypto.addSignature({ url: 'TODO: remove', signature: result.signature }) // const signatureAdditionStart = Date.now() @@ -77,7 +72,7 @@ export function createPnpSignHandler( return true } catch (err) { // One or more signatures failed verification and were discarded. - logger.info('Error caught in receiveSuccess') + logger.info('Error caught in processRequest') logger.info(err) // Continue to collect signatures. } @@ -87,18 +82,19 @@ export function createPnpSignHandler( const { signerResponses, maxErrorCode } = await thresholdCallToSigners( logger, - signers, - signerEndpoint, - request, - keyVersionInfo, - config.odisServices.timeoutMilliSeconds, - SignMessageResponseSchema, - true, - processRequest + { + signers, + endpoint: getSignerEndpoint(CombinerEndpoint.PNP_SIGN), + request, + keyVersionInfo, + requestTimeoutMS: config.odisServices.timeoutMilliSeconds, + responseSchema: SignMessageResponseSchema, + shouldCheckKeyVersion: true, + }, + processResult ) const warnings = logPnpSignerResponseDiscrepancies(logger, signerResponses) - logFailOpenResponses(logger, signerResponses) if (crypto.hasSufficientSignatures()) { try { diff --git a/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts b/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts index 607d7404251..89bd98c2681 100644 --- a/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts +++ b/packages/phone-number-privacy/combiner/src/pnp/services/log-responses.ts @@ -1,14 +1,11 @@ import { - ErrorMessage, PnpQuotaRequest, SignMessageRequest, WarningMessage, } from '@celo/phone-number-privacy-common' import Logger from 'bunyan' import { SignerResponse } from '../../common/io' - import { - MAX_BLOCK_DISCREPANCY_THRESHOLD, MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, } from '../../config' @@ -28,16 +25,15 @@ export function logPnpSignerResponseDiscrepancies( version: string performedQueryCount: number totalQuota: number - blockNumber?: number warnings?: string[] } }> = [] responses.forEach((response) => { if (response.res.success) { - const { version, performedQueryCount, totalQuota, warnings } = response.res + const { version, performedQueryCount, totalQuota, warnings: _warnings } = response.res parsedResponses.push({ signerUrl: response.url, - values: { version, performedQueryCount, totalQuota, warnings }, + values: { version, performedQueryCount, totalQuota, warnings: _warnings }, }) } }) @@ -56,25 +52,6 @@ export function logPnpSignerResponseDiscrepancies( } } - // blockNumber - parsedResponses.forEach((res) => { - if (res.values.blockNumber === undefined) { - logger.warn({ signerUrl: res.signerUrl }, 'Signer responded with undefined blockNumber') - } - }) - const sortedByBlockNumber = parsedResponses - .filter((res) => !!res.values.blockNumber) - .sort((a, b) => a.values.blockNumber! - b.values.blockNumber!) - if ( - sortedByBlockNumber.length && - sortedByBlockNumber[sortedByBlockNumber.length - 1].values.blockNumber! - - sortedByBlockNumber[0].values.blockNumber! >= - MAX_BLOCK_DISCREPANCY_THRESHOLD - ) { - logger.error({ sortedByBlockNumber }, WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS) - warnings.push(WarningMessage.INCONSISTENT_SIGNER_BLOCK_NUMBERS) - } - // totalQuota const sortedByTotalQuota = parsedResponses.sort( (a, b) => a.values.totalQuota - b.values.totalQuota @@ -103,29 +80,3 @@ export function logPnpSignerResponseDiscrepancies( return warnings } - -export function logFailOpenResponses( - logger: Logger, - responses: Array> -): void { - responses.forEach((response) => { - if (response.res.success) { - const { warnings } = response.res - if (warnings) { - warnings.forEach((warning) => { - switch (warning) { - case ErrorMessage.FAILING_OPEN: - case ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA: - case ErrorMessage.FAILURE_TO_GET_DEK: - logger.error( - { signerWarning: warning, service: response.url }, - WarningMessage.SIGNER_FAILED_OPEN - ) - default: - break - } - }) - } - } - }) -} diff --git a/packages/phone-number-privacy/combiner/src/server.ts b/packages/phone-number-privacy/combiner/src/server.ts index 02d24483111..cfb5a24eeed 100644 --- a/packages/phone-number-privacy/combiner/src/server.ts +++ b/packages/phone-number-privacy/combiner/src/server.ts @@ -9,7 +9,6 @@ import { } from '@celo/phone-number-privacy-common' import express, { RequestHandler } from 'express' import { Signer } from './common/combine' -// tslint:disable-next-line: ordered-imports import { catchErrorHandler, disabledHandler, @@ -19,9 +18,7 @@ import { import { CombinerConfig, getCombinerVersion } from './config' import { createDisableDomainHandler } from './domain/endpoints/disable/action' import { createDomainQuotaHandler } from './domain/endpoints/quota/action' - import { createDomainSignHandler } from './domain/endpoints/sign/action' - import { createPnpQuotaHandler } from './pnp/endpoints/quota/action' import { createPnpSignHandler } from './pnp/endpoints/sign/action' diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts index bade7204f99..7041d1c36c3 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-response-logger.test.ts @@ -9,7 +9,7 @@ import { import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' import config from '../../src/config' -import { logDomainResponsesDiscrepancies } from '../../src/domain/services/log-responses' +import { logDomainResponseDiscrepancies } from '../../src/domain/services/log-responses' describe('domain response logger', () => { const url = 'test signer url' @@ -308,7 +308,7 @@ describe('domain response logger', () => { callCount: 0, }, } - logDomainResponsesDiscrepancies( + logDomainResponseDiscrepancies( logger, testCase.responses.map((res) => ({ url, res })) ) diff --git a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts index 9c13c606023..c66c4d478fb 100644 --- a/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/domain-threshold-state.test.ts @@ -4,7 +4,6 @@ import { KeyVersionInfo, } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' - import { findThresholdDomainState } from '../../src/domain/services/threshold-state' describe('domain threshold state', () => { @@ -18,11 +17,6 @@ describe('domain threshold state', () => { pubKey: 'mock pubKey', } - // const keyVersionInfo: KeyVersionInfo = { - // currentVersion: keyVersionInfo.keyVersion, - // versions: JSON.stringify([keyVersionInfo]), - // } - const totalSigners = 4 const expectedVersion = getSignerVersion() diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts index de9d5bc76de..2daf524abad 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-response-logger.test.ts @@ -12,10 +12,7 @@ import config, { MAX_QUERY_COUNT_DISCREPANCY_THRESHOLD, MAX_TOTAL_QUOTA_DISCREPANCY_THRESHOLD, } from '../../src/config' -import { - logFailOpenResponses, - logPnpSignerResponseDiscrepancies, -} from '../../src/pnp/services/log-responses' +import { logPnpSignerResponseDiscrepancies } from '../../src/pnp/services/log-responses' describe('pnp response logger', () => { const url = 'test signer url' @@ -413,104 +410,6 @@ describe('pnp response logger', () => { }, ], }, - { - it: 'should log correctly when signers respond with fail-open warnings', - responses: [ - { - success: true, - performedQueryCount, - totalQuota, - version, - - warnings: [ErrorMessage.FAILING_OPEN], - }, - { - success: true, - performedQueryCount, - totalQuota, - version, - - warnings: [ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA], - }, - { - success: true, - performedQueryCount, - totalQuota, - version, - - warnings: [ErrorMessage.FAILURE_TO_GET_DEK], - }, - ], - expectedLogs: [ - { - params: [ - { - parsedResponses: [ - { - signerUrl: url, - values: { - performedQueryCount, - totalQuota, - version, - warnings: [ErrorMessage.FAILING_OPEN], - }, - }, - { - signerUrl: url, - values: { - performedQueryCount, - totalQuota, - version, - warnings: [ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA], - }, - }, - { - signerUrl: url, - values: { - performedQueryCount, - totalQuota, - version, - warnings: [ErrorMessage.FAILURE_TO_GET_DEK], - }, - }, - ], - }, - WarningMessage.SIGNER_RESPONSE_DISCREPANCIES, - ], - level: 'warn', - }, - { - params: [ - { - signerWarning: ErrorMessage.FAILING_OPEN, - service: url, - }, - WarningMessage.SIGNER_FAILED_OPEN, - ], - level: 'error', - }, - { - params: [ - { - signerWarning: ErrorMessage.FAILURE_TO_GET_TOTAL_QUOTA, - service: url, - }, - WarningMessage.SIGNER_FAILED_OPEN, - ], - level: 'error', - }, - { - params: [ - { - signerWarning: ErrorMessage.FAILURE_TO_GET_DEK, - service: url, - }, - WarningMessage.SIGNER_FAILED_OPEN, - ], - level: 'error', - }, - ], - }, ] testCases.forEach((testCase) => { it(testCase.it, () => { @@ -536,7 +435,6 @@ describe('pnp response logger', () => { }, } logPnpSignerResponseDiscrepancies(logger, responses) - logFailOpenResponses(logger, responses) testCase.expectedLogs.forEach((log) => { expect(logSpys[log.level].spy).toHaveBeenNthCalledWith( ++logSpys[log.level].callCount, From 5731ea010700ee2925151b42186f829722ca3c6f Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 16:05:09 -0300 Subject: [PATCH 15/19] Fixes some tests --- .../combiner/test/integration/domain.test.ts | 14 +-- .../combiner/test/integration/pnp.test.ts | 14 +-- .../test/unit/pnp-threshold-state.test.ts | 92 ++++++++++++------- .../signer/src/common/database/database.ts | 1 + 4 files changed, 75 insertions(+), 46 deletions(-) diff --git a/packages/phone-number-privacy/combiner/test/integration/domain.test.ts b/packages/phone-number-privacy/combiner/test/integration/domain.test.ts index ef8a216b812..9ca2fa81e1e 100644 --- a/packages/phone-number-privacy/combiner/test/integration/domain.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/domain.test.ts @@ -26,12 +26,9 @@ import { TestUtils, WarningMessage, } from '@celo/phone-number-privacy-common' -import { - initDatabase as initSignerDatabase, - startSigner, - SupportedDatabase, - SupportedKeystore, -} from '@celo/phone-number-privacy-signer' +import { initDatabase as initSignerDatabase } from '@celo/phone-number-privacy-signer/dist/common/database/database' +import { startSigner } from '@celo/phone-number-privacy-signer/dist/server' +import { SupportedDatabase, SupportedKeystore } from '@celo/phone-number-privacy-signer/dist/config' import { DefaultKeyName, KeyProvider, @@ -138,6 +135,11 @@ const signerConfig: SignerConfig = { fullNodeTimeoutMs: FULL_NODE_TIMEOUT_IN_MS, fullNodeRetryCount: RETRY_COUNT, fullNodeRetryDelayMs: RETRY_DELAY_IN_MS, + // TODO (alec) make SignerConfig better + shouldMockAccountService: false, + mockDek: '', + mockTotalQuota: 0, + shouldMockRequestService: false, } describe('domainService', () => { diff --git a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts index 8b8db3a0dec..9620c7d757a 100644 --- a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts @@ -19,12 +19,9 @@ import { TestUtils, WarningMessage, } from '@celo/phone-number-privacy-common' -import { - initDatabase as initSignerDatabase, - startSigner, - SupportedDatabase, - SupportedKeystore, -} from '@celo/phone-number-privacy-signer' +import { initDatabase as initSignerDatabase } from '@celo/phone-number-privacy-signer/dist/common/database/database' +import { startSigner } from '@celo/phone-number-privacy-signer/dist/server' +import { SupportedDatabase, SupportedKeystore } from '@celo/phone-number-privacy-signer/dist/config' import { DefaultKeyName, KeyProvider, @@ -148,6 +145,11 @@ const signerConfig: SignerConfig = { fullNodeTimeoutMs: FULL_NODE_TIMEOUT_IN_MS, fullNodeRetryCount: RETRY_COUNT, fullNodeRetryDelayMs: RETRY_DELAY_IN_MS, + // TODO (alec) make SignerConfig better + shouldMockAccountService: false, + mockDek: '', + mockTotalQuota: 0, + shouldMockRequestService: false, } const testBlockNumber = 1000000 diff --git a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts index 587d5c0b8a5..47b21e041c0 100644 --- a/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts +++ b/packages/phone-number-privacy/combiner/test/unit/pnp-threshold-state.test.ts @@ -1,14 +1,6 @@ -import { - KeyVersionInfo, - PnpQuotaRequest, - PnpQuotaResponseSuccess, - rootLogger, - SignMessageRequest, - SignMessageResponseSuccess, - WarningMessage, -} from '@celo/phone-number-privacy-common' +import { KeyVersionInfo, WarningMessage } from '@celo/phone-number-privacy-common' import { getSignerVersion } from '@celo/phone-number-privacy-signer/src/config' -import { Request, Response } from 'express' + import config from '../../src/config' import { findCombinerQuotaState } from '../../src/pnp/services/threshold-state' @@ -28,7 +20,6 @@ describe('pnp threshold state', () => { pnpConfig.keys.versions = JSON.stringify([keyVersionInfo]) const expectedVersion = getSignerVersion() - const testBlockNumber = 1000000 const totalQuota = 10 const performedQueryCount = 5 @@ -81,25 +72,23 @@ describe('pnp threshold state', () => { ] varyingQueryCount.forEach(({ signerRes, expectedQueryCount }) => { it(`should return ${expectedQueryCount} performedQueryCount given signer responses of ${signerRes}`, () => { - - signerRes.map((o) => ( - url: 'random url', - status: 200, - res: { - success: true, - version: expectedVersion, - ...q, + const responses = signerRes.map((o) => { + return { + url: 'random url', + status: 200, + res: { + success: true as true, + version: expectedVersion, + ...o, + }, } }) - session.responses.push({ url: 'random url', res, status: 200 }) - const warnings: string[] = [] - const thresholdResult = findCombinerQuotaState(keyVersionInfo, null, warnings) + const thresholdResult = findCombinerQuotaState(keyVersionInfo, responses, warnings) expect(thresholdResult).toStrictEqual({ performedQueryCount: expectedQueryCount, totalQuota, - blockNumber: testBlockNumber, }) }) }) @@ -138,15 +127,26 @@ describe('pnp threshold state', () => { ] varyingTotalQuota.forEach(({ signerRes, expectedTotalQuota, warning }) => { it(`should return ${expectedTotalQuota} totalQuota given signer responses of ${signerRes}`, () => { - const session = getSession(signerRes) - const thresholdResult = pnpThresholdStateService.findCombinerQuotaState(session) + const responses = signerRes.map((o) => { + return { + url: 'random url', + status: 200, + res: { + success: true as true, + version: expectedVersion, + ...o, + }, + } + }) + + const warnings: string[] = [] + const thresholdResult = findCombinerQuotaState(keyVersionInfo, responses, warnings) expect(thresholdResult).toStrictEqual({ performedQueryCount, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, }) if (warning) { - expect(session.warnings).toContain( + expect(warnings).toContain( WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + ', using threshold signer as best guess' ) @@ -180,15 +180,26 @@ describe('pnp threshold state', () => { ] varyingQuotaAndQuery.forEach(({ signerRes, expectedQueryCount, expectedTotalQuota, warning }) => { it(`should return ${expectedTotalQuota} totalQuota and ${expectedQueryCount} performedQueryCount given signer responses of ${signerRes}`, () => { - const session = getSession(signerRes) - const thresholdResult = pnpThresholdStateService.findCombinerQuotaState(session) + const responses = signerRes.map((o) => { + return { + url: 'random url', + status: 200, + res: { + success: true as true, + version: expectedVersion, + ...o, + }, + } + }) + + const warnings: string[] = [] + const thresholdResult = findCombinerQuotaState(keyVersionInfo, responses, warnings) expect(thresholdResult).toStrictEqual({ performedQueryCount: expectedQueryCount, totalQuota: expectedTotalQuota, - blockNumber: testBlockNumber, }) if (warning) { - expect(session.warnings).toContain( + expect(warnings).toContain( WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS + ', using threshold signer as best guess' ) @@ -197,13 +208,26 @@ describe('pnp threshold state', () => { }) it('should throw an error if the total quota varies too much between signers', () => { - const session = getSession([ + const signerRes = [ { performedQueryCount, totalQuota: 1 }, { performedQueryCount, totalQuota: 9 }, { performedQueryCount, totalQuota: 15 }, { performedQueryCount, totalQuota: 14 }, - ]) - expect(() => pnpThresholdStateService.findCombinerQuotaState(session)).toThrow( + ] + const responses = signerRes.map((o) => { + return { + url: 'random url', + status: 200, + res: { + success: true as true, + version: expectedVersion, + ...o, + }, + } + }) + + const warnings: string[] = [] + expect(() => findCombinerQuotaState(keyVersionInfo, responses, warnings)).toThrow( WarningMessage.INCONSISTENT_SIGNER_QUOTA_MEASUREMENTS ) }) diff --git a/packages/phone-number-privacy/signer/src/common/database/database.ts b/packages/phone-number-privacy/signer/src/common/database/database.ts index 45c86ed79c2..ef68edce9eb 100644 --- a/packages/phone-number-privacy/signer/src/common/database/database.ts +++ b/packages/phone-number-privacy/signer/src/common/database/database.ts @@ -4,6 +4,7 @@ import { DEV_MODE, SignerConfig, SupportedDatabase, VERBOSE_DB_LOGGING } from '. export async function initDatabase(config: SignerConfig, migrationsPath?: string): Promise { const logger = rootLogger(config.serviceName) + logger.info({ config: config.db }, 'Initializing database connection') const { type, host, port, user, password, database, ssl, poolMaxSize } = config.db From 93a9bf09cea7b35e74ff8cb02c3ddcdda8545229 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 17:23:42 -0300 Subject: [PATCH 16/19] fix error reporting --- .../combiner/src/common/combine.ts | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/packages/phone-number-privacy/combiner/src/common/combine.ts b/packages/phone-number-privacy/combiner/src/common/combine.ts index aa616c17b2f..d1752eaaad3 100644 --- a/packages/phone-number-privacy/combiner/src/common/combine.ts +++ b/packages/phone-number-privacy/combiner/src/common/combine.ts @@ -148,11 +148,13 @@ export async function thresholdCallToSigners( // measure e2e combiner latency. obs.disconnect() - if (errorCodes.size > 1) { - logger.error( - { errorCodes: JSON.stringify([...errorCodes]) }, - ErrorMessage.INCONSISTENT_SIGNER_RESPONSES - ) + if (errorCodes.size > 0) { + if (errorCodes.size > 1) { + logger.error( + { errorCodes: JSON.stringify([...errorCodes]) }, + ErrorMessage.INCONSISTENT_SIGNER_RESPONSES + ) + } return { signerResponses: responses, maxErrorCode: getMajorityErrorCode(errorCodes) } } else { From 480503e4c892807c8df7c87b14af012b717c6623 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 17:37:27 -0300 Subject: [PATCH 17/19] fix pnp integration test --- .../combiner/test/integration/pnp.test.ts | 26 ++++++++++++------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts index 9620c7d757a..a1082c8d022 100644 --- a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts @@ -176,6 +176,12 @@ jest.mock('@celo/contractkit', () => ({ newKit: jest.fn().mockImplementation(() => mockContractKit), })) +async function serverClose(server?: Server | HttpsServer) { + if (server) { + await new Promise((resolve) => server.close(resolve)) + } +} + describe('pnpService', () => { let keyProvider1: KeyProvider let keyProvider2: KeyProvider @@ -316,9 +322,9 @@ describe('pnpService', () => { await signerDB1?.destroy() await signerDB2?.destroy() await signerDB3?.destroy() - signer1?.close() - signer2?.close() - signer3?.close() + await serverClose(signer1) + await serverClose(signer2) + await serverClose(signer3) }) describe('when signers are operating correctly', () => { @@ -565,8 +571,8 @@ describe('pnpService', () => { it('Should respond with 500 when insufficient signer responses', async () => { await signerDB1?.destroy() await signerDB2?.destroy() - signer1?.close() - signer2?.close() + await serverClose(signer1) + await serverClose(signer2) const req = { account: ACCOUNT_ADDRESS1, @@ -1263,11 +1269,11 @@ describe('pnpService', () => { await signerDB3?.destroy() await signerDB4?.destroy() await signerDB5?.destroy() - signer1?.close() - signer2?.close() - signer3?.close() - signer4?.close() - signer5?.close() + await serverClose(signer1) + await serverClose(signer2) + await serverClose(signer3) + await serverClose(signer4) + await serverClose(signer5) }) it('Should respond with 200 on valid request', async () => { From 6eaa2b35829597b4a0a39dc6bfb36477c664ff55 Mon Sep 17 00:00:00 2001 From: alecps Date: Wed, 23 Aug 2023 16:49:15 -0400 Subject: [PATCH 18/19] bump combiner version --- packages/phone-number-privacy/combiner/package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/phone-number-privacy/combiner/package.json b/packages/phone-number-privacy/combiner/package.json index 6f8da08c6ba..61baeb902aa 100644 --- a/packages/phone-number-privacy/combiner/package.json +++ b/packages/phone-number-privacy/combiner/package.json @@ -1,6 +1,6 @@ { "name": "@celo/phone-number-privacy-combiner", - "version": "3.0.0-beta.4", + "version": "3.0.0-beta.5", "description": "Orchestrates and combines threshold signatures for use in ODIS", "author": "Celo", "license": "Apache-2.0", From d893af084443f60daefe50b041281943b829df12 Mon Sep 17 00:00:00 2001 From: Mariano Cortesi Date: Wed, 23 Aug 2023 18:58:46 -0300 Subject: [PATCH 19/19] fix integrations test --- .../combiner/test/integration/domain.test.ts | 19 ++++++++++--------- .../combiner/test/integration/pnp.test.ts | 10 ++-------- .../combiner/test/utils.ts | 8 ++++++++ 3 files changed, 20 insertions(+), 17 deletions(-) diff --git a/packages/phone-number-privacy/combiner/test/integration/domain.test.ts b/packages/phone-number-privacy/combiner/test/integration/domain.test.ts index 9ca2fa81e1e..cd0c15c3e94 100644 --- a/packages/phone-number-privacy/combiner/test/integration/domain.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/domain.test.ts @@ -39,11 +39,12 @@ import { LocalWallet } from '@celo/wallet-local' import BigNumber from 'bignumber.js' import { Server as HttpsServer } from 'https' import { Knex } from 'knex' -import { Server } from 'net' +import { Server } from 'http' import request from 'supertest' import { MockKeyProvider } from '../../../signer/dist/common/key-management/mock-key-provider' import config from '../../src/config' import { startCombiner } from '../../src/server' +import { serverClose } from '../utils' const { DOMAINS_THRESHOLD_DEV_PK_SHARE_1_V1, @@ -281,9 +282,9 @@ describe('domainService', () => { await signerDB1?.destroy() await signerDB2?.destroy() await signerDB3?.destroy() - signer1?.close() - signer2?.close() - signer3?.close() + await serverClose(signer1) + await serverClose(signer2) + await serverClose(signer3) }) describe('when signers are operating correctly', () => { @@ -1239,11 +1240,11 @@ describe('domainService', () => { await signerDB3?.destroy() await signerDB4?.destroy() await signerDB5?.destroy() - signer1?.close() - signer2?.close() - signer3?.close() - signer4?.close() - signer5?.close() + await serverClose(signer1) + await serverClose(signer2) + await serverClose(signer3) + await serverClose(signer4) + await serverClose(signer5) }) it('Should respond with 200 on valid request', async () => { diff --git a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts index a1082c8d022..e2cb3c382d3 100644 --- a/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts +++ b/packages/phone-number-privacy/combiner/test/integration/pnp.test.ts @@ -31,12 +31,12 @@ import { SignerConfig } from '@celo/phone-number-privacy-signer/dist/config' import BigNumber from 'bignumber.js' import threshold_bls from 'blind-threshold-bls' import { Server as HttpsServer } from 'https' +import { Server } from 'http' import { Knex } from 'knex' -import { Server } from 'net' import request from 'supertest' import config, { getCombinerVersion } from '../../src/config' import { startCombiner } from '../../src/server' -import { getBlindedPhoneNumber } from '../utils' +import { getBlindedPhoneNumber, serverClose } from '../utils' const { ContractRetrieval, @@ -176,12 +176,6 @@ jest.mock('@celo/contractkit', () => ({ newKit: jest.fn().mockImplementation(() => mockContractKit), })) -async function serverClose(server?: Server | HttpsServer) { - if (server) { - await new Promise((resolve) => server.close(resolve)) - } -} - describe('pnpService', () => { let keyProvider1: KeyProvider let keyProvider2: KeyProvider diff --git a/packages/phone-number-privacy/combiner/test/utils.ts b/packages/phone-number-privacy/combiner/test/utils.ts index 6dbc470c7d1..bbd2e6f21dd 100644 --- a/packages/phone-number-privacy/combiner/test/utils.ts +++ b/packages/phone-number-privacy/combiner/test/utils.ts @@ -1,6 +1,14 @@ import threshold_bls from 'blind-threshold-bls' +import { Server } from 'http' +import { Server as HttpsServer } from 'https' export function getBlindedPhoneNumber(phoneNumber: string, blindingFactor: Buffer): string { const blindedPhoneNumber = threshold_bls.blind(Buffer.from(phoneNumber), blindingFactor).message return Buffer.from(blindedPhoneNumber).toString('base64') } + +export async function serverClose(server?: Server | HttpsServer) { + if (server) { + await new Promise((resolve) => server.close(resolve)) + } +}