From 5ddf860b45a381520816e31c4b9c3f207d5a8918 Mon Sep 17 00:00:00 2001 From: Huajie Zhang Date: Thu, 11 Apr 2024 14:55:39 +0800 Subject: [PATCH] refactor: process error stack and mask potential secrets in error message in telemetry (#11290) * fix: extract method names from error stack * fix: extract method names from error stack * fix: use entropy to mask secret tokens in string * test: ut * test: ut * refactor: white list flag * fix: white list * refactor: improvement * test: ut * test: ut --- packages/api/review/teamsfx-api.api.md | 4 + packages/api/src/error.ts | 20 +++ packages/fx-core/package.json | 2 + packages/fx-core/src/common/stringUtils.ts | 134 ++++++++++++++++++ packages/fx-core/src/common/telemetry.ts | 18 ++- packages/fx-core/src/component/constants.ts | 4 +- .../middleware/addSWADeployTelemetry.ts | 2 +- .../component/driver/script/scriptDriver.ts | 3 +- .../fx-core/src/component/utils/envUtil.ts | 12 -- packages/fx-core/src/index.ts | 1 + .../fx-core/tests/common/stringUtils.test.ts | 26 ++++ .../fx-core/tests/common/telemetry.test.ts | 52 +++++++ .../tests/component/util/envUtils.test.ts | 4 +- .../test/extension/extTelemetry.test.ts | 5 +- 14 files changed, 265 insertions(+), 22 deletions(-) create mode 100644 packages/fx-core/src/common/stringUtils.ts create mode 100644 packages/fx-core/tests/common/stringUtils.test.ts create mode 100644 packages/fx-core/tests/common/telemetry.test.ts diff --git a/packages/api/review/teamsfx-api.api.md b/packages/api/review/teamsfx-api.api.md index 2c33c35224..8934e3fa20 100644 --- a/packages/api/review/teamsfx-api.api.md +++ b/packages/api/review/teamsfx-api.api.md @@ -306,6 +306,7 @@ export interface ErrorOptionBase { message?: string; // (undocumented) name?: string; + skipProcessInTelemetry?: boolean; // (undocumented) source?: string; // (undocumented) @@ -360,6 +361,7 @@ export interface FxError extends Error { categories?: string[]; innerError?: any; recommendedOperation?: string; + skipProcessInTelemetry?: boolean; source: string; timestamp: Date; // (undocumented) @@ -872,6 +874,7 @@ export class SystemError extends Error implements FxError { innerError?: any; issueLink?: string; recommendedOperation?: string; + skipProcessInTelemetry?: boolean; source: string; timestamp: Date; userData?: string; @@ -1066,6 +1069,7 @@ export class UserError extends Error implements FxError { helpLink?: string; innerError?: any; recommendedOperation?: string; + skipProcessInTelemetry?: boolean; source: string; timestamp: Date; userData?: string; diff --git a/packages/api/src/error.ts b/packages/api/src/error.ts index f188f7399f..a1202ed37a 100644 --- a/packages/api/src/error.ts +++ b/packages/api/src/error.ts @@ -24,6 +24,10 @@ export interface FxError extends Error { * e.g. "debug-in-test-tool" */ recommendedOperation?: string; + /** + * whether to skip process (such as mask secret tokens) in telemetry collection + */ + skipProcessInTelemetry?: boolean; } export interface ErrorOptionBase { source?: string; @@ -33,6 +37,10 @@ export interface ErrorOptionBase { userData?: any; displayMessage?: string; categories?: string[]; + /** + * whether to skip process (such as mask secret tokens) in telemetry collection + */ + skipProcessInTelemetry?: boolean; } export interface UserErrorOptions extends ErrorOptionBase { @@ -73,6 +81,11 @@ export class UserError extends Error implements FxError { categories?: string[]; + /** + * whether to skip process (such as mask secret tokens) in telemetry collection + */ + skipProcessInTelemetry?: boolean; + /** * recommended operation for user to fix the error * e.g. "debug-in-test-tool" @@ -124,6 +137,7 @@ export class UserError extends Error implements FxError { this.displayMessage = option.displayMessage; this.timestamp = new Date(); this.categories = option.categories; + this.skipProcessInTelemetry = option.skipProcessInTelemetry; } } @@ -159,6 +173,11 @@ export class SystemError extends Error implements FxError { categories?: string[]; + /** + * whether to skip process (such as mask secret tokens) in telemetry collection + */ + skipProcessInTelemetry?: boolean; + /** * recommended operation for user to fix the error * e.g. "debug-in-test-tool" @@ -210,5 +229,6 @@ export class SystemError extends Error implements FxError { this.displayMessage = option.displayMessage; this.timestamp = new Date(); this.categories = option.categories; + this.skipProcessInTelemetry = option.skipProcessInTelemetry; } } diff --git a/packages/fx-core/package.json b/packages/fx-core/package.json index a16f090a49..aa6d0318b7 100644 --- a/packages/fx-core/package.json +++ b/packages/fx-core/package.json @@ -62,6 +62,8 @@ "test:migration": "nyc mocha \"tests/core/middleware/migration/projectMigrationV3.test.ts\"", "test:teamsappMgr": "nyc mocha \"tests/component/driver/teamsApp/teamsappMgr.test.ts\"", "test:projcheck": "nyc mocha \"tests/common/projectTypeChecker.test.ts\"", + "test:telemetry": "nyc mocha \"tests/common/telemetry.test.ts\"", + "test:stringUtils": "nyc mocha \"tests/common/stringUtils.test.ts\"", "clean": "rm -rf build", "prebuild": "npm run gen:cli", "build": "rimraf build && npx tsc -p ./", diff --git a/packages/fx-core/src/common/stringUtils.ts b/packages/fx-core/src/common/stringUtils.ts new file mode 100644 index 0000000000..f7030eb924 --- /dev/null +++ b/packages/fx-core/src/common/stringUtils.ts @@ -0,0 +1,134 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +const MIN_ENTROPY = 4; +const SECRET_REPLACE = ""; +const USER_REPLACE = ""; + +const WHITE_LIST = [ + "user-file-path", + "publish-app,", + "X-Correlation-ID", + "innerError", + "client-request-id", +]; + +function getProbMap(str: string) { + const probMap = new Map(); + for (const char of str) { + probMap.set(char, (probMap.get(char) || 0) + 1); + } + for (const [char, freq] of probMap.entries()) { + const prob = freq / str.length; + probMap.set(char, prob); + } + return probMap; +} + +// Measure the entropy of a string in bits per symbol. +function shannonEntropy(str: string, probMap: Map) { + let sum = 0; + for (const char of str) { + const prob = probMap.get(char) || 0; + const delta = (prob * Math.log(prob)) / Math.log(2); + sum += delta; + } + return -sum; +} + +class Token { + value: string; + splitter: boolean; + entropy?: number; + constructor(value: string, splitter: boolean) { + this.value = value; + this.splitter = splitter; + } +} + +function tokenize(text: string): Token[] { + const splitterString = " '`\n\t\r\",:{}"; + const splitterChars = new Set(); + for (const char of splitterString) { + splitterChars.add(char); + } + const tokens: Token[] = []; + let currentToken = ""; + for (const char of text) { + if (splitterChars.has(char)) { + if (currentToken.length > 0) { + tokens.push(new Token(currentToken, false)); + currentToken = ""; + } + tokens.push(new Token(char, true)); + } else { + currentToken += char; + } + } + if (currentToken.length > 0) { + tokens.push(new Token(currentToken, false)); + } + return tokens; +} + +function computeShannonEntropy(token: Token) { + if (!token.splitter) { + const probMap = getProbMap(token.value); + token.entropy = shannonEntropy(token.value, probMap); + } +} + +export interface MaskSecretOptions { + threshold?: number; + whiteList?: string[]; +} + +export function maskSecret( + inputText?: string, + option = { threshold: MIN_ENTROPY, whiteList: WHITE_LIST } +): string { + if (!inputText) return ""; + // mask by secret pattern + inputText = maskByPattern(inputText); + // mask by .env.xxx.user + inputText = maskSecretValues(inputText, SECRET_REPLACE); + // mask by entropy + let output = ""; + const tokens = tokenize(inputText); + tokens.forEach((token) => { + computeShannonEntropy(token); + if ( + option.whiteList?.includes(token.value) || + token.splitter || + (token.entropy || 0) <= option.threshold + ) { + output += token.value; + } else { + output += SECRET_REPLACE; + } + }); + for (const token of tokens) { + console.log(token); + } + return output; +} + +function maskByPattern(command: string): string { + const regexU = /(-u|--username|--user) (\S+)/; + const regexP = /(-p|--password|--pwd) (\S+)/; + let output = command.replace(regexU, `$1 ${USER_REPLACE}`); + output = output.replace(regexP, `$1 ${SECRET_REPLACE}`); + return output; +} + +export function maskSecretValues(stdout: string, replace = "***"): string { + for (const key of Object.keys(process.env)) { + if (key.startsWith("SECRET_")) { + const value = process.env[key]; + if (value) { + stdout = stdout.replace(value, replace); + } + } + } + return stdout; +} diff --git a/packages/fx-core/src/common/telemetry.ts b/packages/fx-core/src/common/telemetry.ts index a86785958a..3bc5df33e8 100644 --- a/packages/fx-core/src/common/telemetry.ts +++ b/packages/fx-core/src/common/telemetry.ts @@ -7,6 +7,7 @@ import { TOOLS, globalVars } from "../core/globalVars"; import { ProjectTypeResult } from "./projectTypeChecker"; import { assign } from "lodash"; import { ProjectType } from "@microsoft/m365-spec-parser"; +import { maskSecret } from "./stringUtils"; export enum TelemetryProperty { TriggerFrom = "trigger-from", @@ -241,8 +242,10 @@ export function fillInTelemetryPropsForFxError( props[TelemetryConstants.properties.errorCode] = props[TelemetryConstants.properties.errorCode] || errorCode; props[TelemetryConstants.properties.errorType] = errorType; - // props[TelemetryConstants.properties.errorMessage] = error.message; // error-message is retired - // props[TelemetryConstants.properties.errorStack] = error.stack !== undefined ? error.stack : ""; // error stack will not append in error-message any more + props[TelemetryConstants.properties.errorMessage] = error.skipProcessInTelemetry + ? error.message + : maskSecret(error.message); + props[TelemetryConstants.properties.errorStack] = extractMethodNamesFromErrorStack(error.stack); // error stack will not append in error-message any more props[TelemetryConstants.properties.errorName] = error.name; // append global context properties @@ -290,3 +293,14 @@ export function fillinProjectTypeProperties( }; assign(props, newProps); } + +export function extractMethodNamesFromErrorStack(stack?: string): string { + if (!stack) return ""; + const methodNamesRegex = /at\s([\w.<>\[\]\s]+)\s\(/g; + let match; + const methodNames: string[] = []; + while ((match = methodNamesRegex.exec(stack)) !== null) { + methodNames.push(match[1]); + } + return methodNames.join(" | "); +} diff --git a/packages/fx-core/src/component/constants.ts b/packages/fx-core/src/component/constants.ts index d3e89e6f00..61e022b561 100644 --- a/packages/fx-core/src/component/constants.ts +++ b/packages/fx-core/src/component/constants.ts @@ -57,8 +57,8 @@ export const TelemetryConstants = { success: "success", errorCode: "error-code", errorType: "error-type", - errorMessage: "error-message", - errorStack: "error-stack", + errorMessage: "err-message", // change the error message property key + errorStack: "err-stack", // change the error stack property key timeCost: "time-cost", errorName: "error-name", // need classify, keep error name as a separate property for telemetry analysis, error name should has limited set of values innerError: "inner-error", // need classify, JSON serialized raw inner error that is caused by internal error or external call error diff --git a/packages/fx-core/src/component/driver/middleware/addSWADeployTelemetry.ts b/packages/fx-core/src/component/driver/middleware/addSWADeployTelemetry.ts index 0df36ba382..e049d4eb5b 100644 --- a/packages/fx-core/src/component/driver/middleware/addSWADeployTelemetry.ts +++ b/packages/fx-core/src/component/driver/middleware/addSWADeployTelemetry.ts @@ -11,7 +11,7 @@ import { TelemetryConstant } from "../../constant/commonConstant"; import { performance } from "perf_hooks"; import { TelemetryConstants } from "../../constants"; import { isExecutionResult } from "./addStartAndEndTelemetry"; -import { maskSecretValues } from "../../utils/envUtil"; +import { maskSecretValues } from "../../../common/stringUtils"; /** * A special telemetry middleware for SWA deployment. diff --git a/packages/fx-core/src/component/driver/script/scriptDriver.ts b/packages/fx-core/src/component/driver/script/scriptDriver.ts index 8f8a678bd2..f6ec585b61 100644 --- a/packages/fx-core/src/component/driver/script/scriptDriver.ts +++ b/packages/fx-core/src/component/driver/script/scriptDriver.ts @@ -16,10 +16,11 @@ import { ScriptExecutionError, ScriptTimeoutError } from "../../../error/script" import { TelemetryConstant } from "../../constant/commonConstant"; import { ProgressMessages } from "../../messages"; import { getSystemEncoding } from "../../utils/charsetUtils"; -import { DotenvOutput, maskSecretValues } from "../../utils/envUtil"; +import { DotenvOutput } from "../../utils/envUtil"; import { DriverContext } from "../interface/commonArgs"; import { ExecutionResult, StepDriver } from "../interface/stepDriver"; import { addStartAndEndTelemetry } from "../middleware/addStartAndEndTelemetry"; +import { maskSecretValues } from "../../../common/stringUtils"; const ACTION_NAME = "script"; diff --git a/packages/fx-core/src/component/utils/envUtil.ts b/packages/fx-core/src/component/utils/envUtil.ts index 480b84a728..0ffdcbb698 100644 --- a/packages/fx-core/src/component/utils/envUtil.ts +++ b/packages/fx-core/src/component/utils/envUtil.ts @@ -386,15 +386,3 @@ class DotenvUtil { } export const dotenvUtil = new DotenvUtil(); - -export function maskSecretValues(stdout: string): string { - for (const key of Object.keys(process.env)) { - if (key.startsWith("SECRET_")) { - const value = process.env[key]; - if (value) { - stdout = stdout.replace(value, "***"); - } - } - } - return stdout; -} diff --git a/packages/fx-core/src/index.ts b/packages/fx-core/src/index.ts index cd005cd25f..39a4b00ec5 100644 --- a/packages/fx-core/src/index.ts +++ b/packages/fx-core/src/index.ts @@ -8,6 +8,7 @@ export * from "./common/deps-checker"; export * from "./common/featureFlags"; export * from "./common/globalState"; export * from "./common/telemetry"; +export * from "./common/stringUtils"; export { jsonUtils } from "./common/jsonUtils"; export * from "./common/local"; export * from "./common/m365/constants"; diff --git a/packages/fx-core/tests/common/stringUtils.test.ts b/packages/fx-core/tests/common/stringUtils.test.ts new file mode 100644 index 0000000000..006ac36180 --- /dev/null +++ b/packages/fx-core/tests/common/stringUtils.test.ts @@ -0,0 +1,26 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { assert } from "chai"; +import "mocha"; +import sinon from "sinon"; +import { maskSecret } from "../../src/common/stringUtils"; + +describe("stringUtils", () => { + const sandbox = sinon.createSandbox(); + afterEach(async () => { + sandbox.restore(); + }); + describe("maskSecret", () => { + it("happy path", async () => { + const input = + "Bearer eyJ0eXAiOiJKV1QiLCJub25jZSI6IkZQQVpfd0ZXc2EwdFpCcGMtcXJITFBzQjd6QnJSWmpzbnFTMW"; + const output = maskSecret(input); + assert.equal(output, "Bearer "); + }); + it("input undefined", async () => { + const output = maskSecret(); + assert.equal(output, ""); + }); + }); +}); diff --git a/packages/fx-core/tests/common/telemetry.test.ts b/packages/fx-core/tests/common/telemetry.test.ts new file mode 100644 index 0000000000..ecc265d26a --- /dev/null +++ b/packages/fx-core/tests/common/telemetry.test.ts @@ -0,0 +1,52 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT license. + +import { assert } from "chai"; +import "mocha"; +import sinon from "sinon"; +import { extractMethodNamesFromErrorStack } from "../../src/common/telemetry"; + +describe("telemetry", () => { + const sandbox = sinon.createSandbox(); + + afterEach(async () => { + sandbox.restore(); + }); + describe("extractMethodNamesFromErrorStack", () => { + it("happy path", async () => { + const stack = `FetchSampleInfoError: Unable to fetch sample info + at FetchSampleInfoError.toFxError (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\error\\componentError.js:45:20) + at Object.sampleDefaultOnActionError [as onActionError] (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\generator\\generator.js:173:59) + at async Generator.generate (\\somapath\TeamsFx\\packages\\fx-core\\build\\component\\generator\\generator.js:105:21) + at async Generator.generateSample (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\generator\\generator.js:92:9) + at async Generator. (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\middleware\\actionExecutionMW.js:71:13) + at async Coordinator.create (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\coordinator\\index.js:165:25) + at async Coordinator. (\\somapath\\TeamsFx\\packages\\fx-core\\build\\component\\middleware\\actionExecutionMW.js:71:13) + at async Coordinator. (\\somapath\\TeamsFx\\packages\\fx-core\\build\\core\\globalVars.js:31:9) + at async FxCore.createSampleProject (\\somapath\\TeamsFx\\packages\\fx-core\\build\\core\\FxCore.js:102:21) + at async FxCore. (\\somapath\TeamsFx\\packages\\fx-core\\build\\component\\middleware\\questionMW.js:22:9) + at async FxCore.ErrorHandlerMW (\\somapath\\TeamsFx\\packages\\fx-core\\build\\core\\middleware\\errorHandler.js:19:9) + at async FxCore. (\\somapath\\TeamsFx\\packages\\fx-core\\build\\core\\globalVars.js:31:9)`; + const expectedOutput = [ + "FetchSampleInfoError.toFxError", + "Object.sampleDefaultOnActionError [as onActionError]", + "async Generator.generate", + "async Generator.generateSample", + "async Generator.", + "async Coordinator.create", + "async Coordinator.", + "async Coordinator.", + "async FxCore.createSampleProject", + "async FxCore.", + "async FxCore.ErrorHandlerMW", + "async FxCore.", + ]; + const output = extractMethodNamesFromErrorStack(stack); + assert.equal(output, expectedOutput.join(" | ")); + }); + it("input undefined", async () => { + const output = extractMethodNamesFromErrorStack(); + assert.equal(output, ""); + }); + }); +}); diff --git a/packages/fx-core/tests/component/util/envUtils.test.ts b/packages/fx-core/tests/component/util/envUtils.test.ts index 2c464ed329..0b5b7de9aa 100644 --- a/packages/fx-core/tests/component/util/envUtils.test.ts +++ b/packages/fx-core/tests/component/util/envUtils.test.ts @@ -4,11 +4,11 @@ * @author Siglud */ -import { maskSecretValues } from "../../../src/component/utils/envUtil"; import "mocha"; import { assert } from "chai"; +import { maskSecretValues } from "../../../src/common/stringUtils"; -describe("envUtil.maskSecretValues", () => { +describe("stringUtils.maskSecretValues", () => { afterEach(() => { delete process.env["SECRET_KEY"]; delete process.env["NON_SECRET_KEY"]; diff --git a/packages/vscode-extension/test/extension/extTelemetry.test.ts b/packages/vscode-extension/test/extension/extTelemetry.test.ts index 591ebfd044..700b375fb8 100644 --- a/packages/vscode-extension/test/extension/extTelemetry.test.ts +++ b/packages/vscode-extension/test/extension/extTelemetry.test.ts @@ -10,6 +10,7 @@ import * as fs from "fs-extra"; import * as globalVariables from "../../src/globalVariables"; import { Uri } from "vscode"; import * as globalState from "@microsoft/teamsfx-core/build/common/globalState"; +import { extractMethodNamesFromErrorStack, maskSecret } from "@microsoft/teamsfx-core"; chai.use(spies); const spy = chai.spy; @@ -151,8 +152,8 @@ describe("ExtTelemetry", () => { "settings-version": "1.0.0", "error-type": "user", "error-name": "UserTestError", - // "error-message": error.message, - // "error-stack": error.stack, + "err-message": maskSecret(error.message), + "err-stack": extractMethodNamesFromErrorStack(error.stack), "error-code": "test.UserTestError", "error-component": "", "error-method": "",