From 01ca585bf1f95bf04071d4260b19d14fed605f50 Mon Sep 17 00:00:00 2001 From: Sander Bruens Date: Fri, 19 Apr 2024 15:30:04 -0400 Subject: [PATCH] feat(metrics_server): do not invalidate legacy report types (#1532) * Check if `countries` is an array. * Use `VALID_REPORT` in `postConnectionMetrics` test. * Make realistic legacy reports and silently drop them during validation. * Add more explicit support for legacy report types. * Make diff smaller. * Update `@typescript-eslint/eslint-plugin` dependency. * Revert some changes. * Revert "Update `@typescript-eslint/eslint-plugin` dependency." This reverts commit a2f3edff096bdd78066e9f2d4f6996cef258ad95. * Update test variables to improve readability. * Add back in `userId` tests. * Remove check that at least 1 of `userId` or `countries` is set. * Remove unneeded test. --- src/metrics_server/connection_metrics.spec.ts | 115 ++++++++++-------- src/metrics_server/connection_metrics.ts | 68 +++++++---- src/metrics_server/model.ts | 8 +- 3 files changed, 117 insertions(+), 74 deletions(-) diff --git a/src/metrics_server/connection_metrics.spec.ts b/src/metrics_server/connection_metrics.spec.ts index 2e89cb57..385b0088 100644 --- a/src/metrics_server/connection_metrics.spec.ts +++ b/src/metrics_server/connection_metrics.spec.ts @@ -32,14 +32,22 @@ const VALID_USER_REPORT2: HourlyUserConnectionMetricsReport = { }; /* - * A user report to test legacy fields to ensure backwards compatibility with - * older servers that may still send reports like this. + * Legacy access key user reports to ensure backwards compatibility with servers not + * synced past https://github.com/Jigsaw-Code/outline-server/pull/1529). */ -const LEGACY_USER_REPORT = { +const LEGACY_PER_KEY_USER_REPORT: HourlyUserConnectionMetricsReport = { userId: 'foo', + bytesTransferred: 123, +}; + +/* + * Legacy multiple countries user reports to ensure backwards compatibility with servers + * not synced past https://github.com/Jigsaw-Code/outline-server/pull/1242. + */ +const LEGACY_PER_LOCATION_USER_REPORT: HourlyUserConnectionMetricsReport = { + userId: 'foobar', countries: ['US', 'UK'], bytesTransferred: 123, - tunnelTimeSec: 789, }; const VALID_REPORT: HourlyConnectionMetricsReport = { @@ -49,10 +57,17 @@ const VALID_REPORT: HourlyConnectionMetricsReport = { userReports: [ structuredClone(VALID_USER_REPORT), structuredClone(VALID_USER_REPORT2), - structuredClone(LEGACY_USER_REPORT), + structuredClone(LEGACY_PER_LOCATION_USER_REPORT), ], }; +const LEGACY_REPORT: HourlyConnectionMetricsReport = { + serverId: 'legacy-id', + startUtcMs: 3, + endUtcMs: 4, + userReports: [structuredClone(LEGACY_PER_KEY_USER_REPORT)], +}; + class FakeConnectionsTable implements InsertableTable { public rows: ConnectionRow[] | undefined; @@ -62,54 +77,46 @@ class FakeConnectionsTable implements InsertableTable { } describe('postConnectionMetrics', () => { - it('correctly inserts feature metrics rows', async () => { + it('correctly inserts connection metrics rows', async () => { const table = new FakeConnectionsTable(); - const userReports = [ - { - countries: ['UK'], - bytesTransferred: 123, - tunnelTimeSec: 987, - }, - { - countries: ['EC'], - bytesTransferred: 456, - tunnelTimeSec: 654, - }, - { - countries: ['BR'], - bytesTransferred: 789, - }, - ]; - const report = {serverId: 'id', startUtcMs: 1, endUtcMs: 2, userReports}; - await postConnectionMetrics(table, report); + + await postConnectionMetrics(table, VALID_REPORT); + const rows: ConnectionRow[] = [ { - serverId: report.serverId, - startTimestamp: new Date(report.startUtcMs).toISOString(), - endTimestamp: new Date(report.endUtcMs).toISOString(), - bytesTransferred: userReports[0].bytesTransferred, - tunnelTimeSec: userReports[0].tunnelTimeSec, - countries: userReports[0].countries, + serverId: VALID_REPORT.serverId, + startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(), + endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(), + bytesTransferred: VALID_USER_REPORT.bytesTransferred, + tunnelTimeSec: VALID_USER_REPORT.tunnelTimeSec, + countries: VALID_USER_REPORT.countries, }, { - serverId: report.serverId, - startTimestamp: new Date(report.startUtcMs).toISOString(), - endTimestamp: new Date(report.endUtcMs).toISOString(), - bytesTransferred: userReports[1].bytesTransferred, - tunnelTimeSec: userReports[1].tunnelTimeSec, - countries: userReports[1].countries, + serverId: VALID_REPORT.serverId, + startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(), + endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(), + bytesTransferred: VALID_USER_REPORT2.bytesTransferred, + tunnelTimeSec: VALID_USER_REPORT2.tunnelTimeSec, + countries: VALID_USER_REPORT2.countries, }, { - serverId: report.serverId, - startTimestamp: new Date(report.startUtcMs).toISOString(), - endTimestamp: new Date(report.endUtcMs).toISOString(), - bytesTransferred: userReports[2].bytesTransferred, - tunnelTimeSec: undefined, - countries: userReports[2].countries, + serverId: VALID_REPORT.serverId, + startTimestamp: new Date(VALID_REPORT.startUtcMs).toISOString(), + endTimestamp: new Date(VALID_REPORT.endUtcMs).toISOString(), + bytesTransferred: LEGACY_PER_LOCATION_USER_REPORT.bytesTransferred, + tunnelTimeSec: LEGACY_PER_LOCATION_USER_REPORT.tunnelTimeSec, + countries: LEGACY_PER_LOCATION_USER_REPORT.countries, }, ]; expect(table.rows).toEqual(rows); }); + it('correctly drops legacy connection metrics', async () => { + const table = new FakeConnectionsTable(); + + await postConnectionMetrics(table, LEGACY_REPORT); + + expect(table.rows).toEqual([]); + }); }); describe('isValidConnectionMetricsReport', () => { @@ -117,6 +124,10 @@ describe('isValidConnectionMetricsReport', () => { const report = structuredClone(VALID_REPORT); expect(isValidConnectionMetricsReport(report)).toBeTrue(); }); + it('returns true for legacy report', () => { + const report = structuredClone(LEGACY_REPORT); + expect(isValidConnectionMetricsReport(report)).toBeTrue(); + }); it('returns false for missing report', () => { expect(isValidConnectionMetricsReport(undefined)).toBeFalse(); }); @@ -166,14 +177,6 @@ describe('isValidConnectionMetricsReport', () => { delete report['endUtcMs']; expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for missing user report field `countries`', () => { - const report = structuredClone(VALID_REPORT); - const userReport: Partial = - structuredClone(VALID_USER_REPORT); - delete userReport['countries']; - report.userReports[0] = userReport as HourlyUserConnectionMetricsReport; - expect(isValidConnectionMetricsReport(report)).toBeFalse(); - }); it('returns false for missing user report field `bytesTransferred`', () => { const report = structuredClone(VALID_REPORT); const userReport: Partial = @@ -202,7 +205,17 @@ describe('isValidConnectionMetricsReport', () => { report.endUtcMs = '100' as unknown as number; expect(isValidConnectionMetricsReport(report)).toBeFalse(); }); - it('returns false for `countries` field type that is not a string', () => { + it('returns false for `userId` field type that is not a string', () => { + const report = structuredClone(VALID_REPORT); + report.userReports[0].userId = 1234 as unknown as string; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `countries` field type that is not an array', () => { + const report = structuredClone(VALID_REPORT); + report.userReports[0].countries = 'US' as unknown as string[]; + expect(isValidConnectionMetricsReport(report)).toBeFalse(); + }); + it('returns false for `countries` arry items that are not strings', () => { const report = structuredClone(VALID_REPORT); report.userReports[0].countries = [1, 2, 3] as unknown as string[]; expect(isValidConnectionMetricsReport(report)).toBeFalse(); diff --git a/src/metrics_server/connection_metrics.ts b/src/metrics_server/connection_metrics.ts index eee6cdaa..2a611b09 100644 --- a/src/metrics_server/connection_metrics.ts +++ b/src/metrics_server/connection_metrics.ts @@ -14,7 +14,13 @@ import {Table} from '@google-cloud/bigquery'; import {InsertableTable} from './infrastructure/table'; -import {HourlyConnectionMetricsReport} from './model'; +import { + HourlyConnectionMetricsReport, + HourlyUserConnectionMetricsReport, + HourlyUserConnectionMetricsReportByLocation, +} from './model'; + +const TERABYTE = Math.pow(2, 40); export interface ConnectionRow { serverId: string; @@ -45,18 +51,29 @@ function getConnectionRowsFromReport(report: HourlyConnectionMetricsReport): Con const endTimestampStr = new Date(report.endUtcMs).toISOString(); const rows = []; for (const userReport of report.userReports) { - rows.push({ - serverId: report.serverId, - startTimestamp: startTimestampStr, - endTimestamp: endTimestampStr, - bytesTransferred: userReport.bytesTransferred, - tunnelTimeSec: userReport.tunnelTimeSec || undefined, - countries: userReport.countries || [], - }); + // User reports come in 2 flavors: "per location" and "per key". We no longer store the + // "per key" reports. + if (isPerLocationUserReport(userReport)) { + rows.push({ + serverId: report.serverId, + startTimestamp: startTimestampStr, + endTimestamp: endTimestampStr, + bytesTransferred: userReport.bytesTransferred, + tunnelTimeSec: userReport.tunnelTimeSec || undefined, + countries: userReport.countries, + }); + } } return rows; } +function isPerLocationUserReport( + // eslint-disable-next-line @typescript-eslint/no-explicit-any + userReport: HourlyUserConnectionMetricsReport +): userReport is HourlyUserConnectionMetricsReportByLocation { + return 'countries' in userReport; +} + // Returns true iff testObject contains a valid HourlyConnectionMetricsReport. export function isValidConnectionMetricsReport( // eslint-disable-next-line @typescript-eslint/no-explicit-any @@ -93,14 +110,21 @@ export function isValidConnectionMetricsReport( return false; } - const MIN_BYTES_TRANSFERRED = 0; - const MAX_BYTES_TRANSFERRED = 1 * Math.pow(2, 40); // 1 TB. for (const userReport of testObject.userReports) { - // Check that `bytesTransferred` is a number between min and max transfer limits + // Check that `userId` is a string. + if (userReport.userId && typeof userReport.userId !== 'string') { + return false; + } + + // We used to set a limit of 1TB per access key, then per location. We later + // realized that a server may use a single key, or all the traffic may come + // from a single location. + // However, as we report hourly, it's unlikely we hit 1TB, so we keep the + // check for now to try and prevent malicious reports. if ( typeof userReport.bytesTransferred !== 'number' || - userReport.bytesTransferred < MIN_BYTES_TRANSFERRED || - userReport.bytesTransferred > MAX_BYTES_TRANSFERRED + userReport.bytesTransferred < 0 || + userReport.bytesTransferred > TERABYTE ) { return false; } @@ -112,16 +136,16 @@ export function isValidConnectionMetricsReport( return false; } - // We require at least 1 country to be set - const countries = userReport.countries ?? []; - if (countries.length === 0) { - return false; - } - // Check that all `countries` are strings. - for (const country of countries) { - if (typeof country !== 'string') { + // Check that `countries` is an array of strings. + if (userReport.countries) { + if (!Array.isArray(userReport.countries)) { return false; } + for (const country of userReport.countries) { + if (typeof country !== 'string') { + return false; + } + } } } diff --git a/src/metrics_server/model.ts b/src/metrics_server/model.ts index 8f127fe6..93a50d73 100644 --- a/src/metrics_server/model.ts +++ b/src/metrics_server/model.ts @@ -22,11 +22,17 @@ export interface HourlyConnectionMetricsReport { } export interface HourlyUserConnectionMetricsReport { - countries: string[]; + userId?: string; + countries?: string[]; bytesTransferred: number; tunnelTimeSec?: number; } +export interface HourlyUserConnectionMetricsReportByLocation + extends Omit { + countries: string[]; +} + export interface DailyFeatureMetricsReport { serverId: string; serverVersion: string;