Mask secrets when writing logs

src/spec-common/injectHeadless.ts

@@ -7,7 +7,6 @@ import * as path from 'path';
 import * as fs from 'fs';
 import { StringDecoder } from 'string_decoder';
 import * as crypto from 'crypto';
-import * as jsonc from 'jsonc-parser';
 
 import { ContainerError, toErrorText, toWarningText } from './errors';
 import { launch, ShellServer } from './shellServer';
@@ -66,7 +65,7 @@ export interface ResolverParameters {
 	containerSessionDataFolder?: string;
 	skipPersistingCustomizationsFromFeatures: boolean;
 	omitConfigRemotEnvFromMetadata?: boolean;
-	secretsFile?: string;
+	secretsP?: Promise<Record<string, string>>;
 }
 
 export interface LifecycleHook {
@@ -325,9 +324,9 @@ export async function setupInContainer(params: ResolverParameters, containerProp
 	const computeRemoteEnv = params.computeExtensionHostEnv || params.lifecycleHook.enabled;
 	const updatedConfig = containerSubstitute(params.cliHost.platform, config.configFilePath, containerProperties.env, config);
 	const remoteEnv = computeRemoteEnv ? probeRemoteEnv(params, containerProperties, updatedConfig) : Promise.resolve({});
-	const secrets = readSecretsFromFile(params);
+	const secretsP = params.secretsP || Promise.resolve({});
 	if (params.lifecycleHook.enabled) {
-		await runLifecycleHooks(params, lifecycleCommandOriginMap, containerProperties, updatedConfig, remoteEnv, secrets, false);
+		await runLifecycleHooks(params, lifecycleCommandOriginMap, containerProperties, updatedConfig, remoteEnv, secretsP, false);
 	}
 	return {
 		remoteEnv: params.computeExtensionHostEnv ? await remoteEnv : {},
@@ -343,25 +342,6 @@ export function probeRemoteEnv(params: ResolverParameters, containerProperties:
 		} as Record<string, string>));
 }
 
-export async function readSecretsFromFile(params: { output: Log; secretsFile?: string; cliHost: CLIHost }) {
-	const { secretsFile, cliHost } = params;
-	if (!secretsFile) {
-		return {};
-	}
-
-	try {
-		const fileBuff = await cliHost.readFile(secretsFile);
-		return jsonc.parse(fileBuff.toString()) as Record<string, string>;
-	}
-	catch (e) {
-		params.output.write(`Failed to read/parse secrets from file '${secretsFile}'`, LogLevel.Error);
-		throw new ContainerError({
-			description: 'Failed to read/parse secrets',
-			originalError: e
-		});
-	}
-}
-
 export async function runLifecycleHooks(params: ResolverParameters, lifecycleHooksInstallMap: LifecycleHooksInstallMap, containerProperties: ContainerProperties, config: CommonMergedDevContainerConfig, remoteEnv: Promise<Record<string, string>>, secrets: Promise<Record<string, string>>, stopForPersonalization: boolean): Promise<'skipNonBlocking' | 'prebuild' | 'stopForPersonalization' | 'done'> {
 	const skipNonBlocking = params.lifecycleHook.skipNonBlocking;
 	const waitFor = config.waitFor || defaultWaitFor;

src/spec-node/devContainers.ts

@@ -12,7 +12,7 @@ import { createNullLifecycleHook, finishBackgroundTasks, ResolverParameters, Use
 import { getCLIHost, loadNativeModule } from '../spec-common/commonUtils';
 import { resolve } from './configContainer';
 import { URI } from 'vscode-uri';
-import { LogLevel, LogDimensions, toErrorText, createCombinedLog, createTerminalLog, Log, makeLog, LogFormat, createJSONLog, createPlainLog } from '../spec-utils/log';
+import { LogLevel, LogDimensions, toErrorText, createCombinedLog, createTerminalLog, Log, makeLog, LogFormat, createJSONLog, createPlainLog, LogHandler, replaceAllLog } from '../spec-utils/log';
 import { dockerComposeCLIConfig } from './dockerCompose';
 import { Mount } from '../spec-configuration/containerFeaturesConfiguration';
 import { getPackageConfig, PackageConfiguration } from '../spec-utils/product';
@@ -63,9 +63,9 @@ export interface ProvisionOptions {
 		installCommand?: string;
 		targetPath?: string;
 	};
-	secretsFile?: string;
 	experimentalLockfile?: boolean;
 	experimentalFrozenLockfile?: boolean;
+	secretsP?: Promise<Record<string, string>>;
 }
 
 export async function launch(options: ProvisionOptions, providedIdLabels: string[] | undefined, disposables: (() => Promise<unknown> | undefined)[]) {
@@ -93,15 +93,15 @@ export async function launch(options: ProvisionOptions, providedIdLabels: string
 }
 
 export async function createDockerParams(options: ProvisionOptions, disposables: (() => Promise<unknown> | undefined)[]): Promise<DockerResolverParameters> {
-	const { persistedFolder, additionalMounts, updateRemoteUserUIDDefault, containerDataFolder, containerSystemDataFolder, workspaceMountConsistency, mountWorkspaceGitRoot, remoteEnv, secretsFile, experimentalLockfile, experimentalFrozenLockfile } = options;
+	const { persistedFolder, additionalMounts, updateRemoteUserUIDDefault, containerDataFolder, containerSystemDataFolder, workspaceMountConsistency, mountWorkspaceGitRoot, remoteEnv, experimentalLockfile, experimentalFrozenLockfile, omitLoggerHeader, secretsP } = options;
 	let parsedAuthority: DevContainerAuthority | undefined;
 	if (options.workspaceFolder) {
 		parsedAuthority = { hostPath: options.workspaceFolder } as DevContainerAuthority;
 	}
 	const extensionPath = path.join(__dirname, '..', '..');
 	const sessionStart = new Date();
 	const pkg = getPackageConfig();
-	const output = createLog(options, pkg, sessionStart, disposables, options.omitLoggerHeader);
+	const output = createLog(options, pkg, sessionStart, disposables, omitLoggerHeader, secretsP ? await secretsP : undefined);
 
 	const appRoot = undefined;
 	const cwd = options.workspaceFolder || process.cwd();
@@ -134,7 +134,7 @@ export async function createDockerParams(options: ProvisionOptions, disposables:
 		backgroundTasks: [],
 		persistedFolder: persistedFolder || await getCacheFolder(cliHost), // Fallback to tmp folder, even though that isn't 'persistent'
 		remoteEnv,
-		secretsFile,
+		secretsP,
 		buildxPlatform: options.buildxPlatform,
 		buildxPush: options.buildxPush,
 		buildxOutput: options.buildxOutput,
@@ -199,24 +199,34 @@ export interface LogOptions {
 	onDidChangeTerminalDimensions?: Event<LogDimensions>;
 }
 
-export function createLog(options: LogOptions, pkg: PackageConfiguration, sessionStart: Date, disposables: (() => Promise<unknown> | undefined)[], omitHeader?: boolean) {
+export function createLog(options: LogOptions, pkg: PackageConfiguration, sessionStart: Date, disposables: (() => Promise<unknown> | undefined)[], omitHeader?: boolean, secrets?: Record<string, string>) {
 	const header = omitHeader ? undefined : `${pkg.name} ${pkg.version}. Node.js ${process.version}. ${os.platform()} ${os.release()} ${os.arch()}.`;
-	const output = createLogFrom(options, sessionStart, header);
+	const output = createLogFrom(options, sessionStart, header, secrets);
 	output.dimensions = options.terminalDimensions;
 	output.onDidChangeDimensions = options.onDidChangeTerminalDimensions;
 	disposables.push(() => output.join());
 	return output;
 }
 
-function createLogFrom({ log: write, logLevel, logFormat }: LogOptions, sessionStart: Date, header: string | undefined = undefined): Log & { join(): Promise<void> } {
+function createLogFrom({ log: write, logLevel, logFormat }: LogOptions, sessionStart: Date, header: string | undefined = undefined, secrets?: Record<string, string>): Log & { join(): Promise<void> } {
 	const handler = logFormat === 'json' ? createJSONLog(write, () => logLevel, sessionStart) :
 		process.stdout.isTTY ? createTerminalLog(write, () => logLevel, sessionStart) :
-		createPlainLog(write, () => logLevel);
+			createPlainLog(write, () => logLevel);
 	const log = {
-		...makeLog(createCombinedLog([handler], header)),
+		...makeLog(createCombinedLog([maskSecrets(handler, secrets)], header)),
 		join: async () => {
 			// TODO: wait for write() to finish.
 		},
 	};
 	return log;
 }
+
+function maskSecrets(handler: LogHandler, secrets?: Record<string, string>): LogHandler {
+	if (secrets) {
+		const mask = '********';
+		const secretValues = Object.values(secrets);
+		return replaceAllLog(handler, secretValues, mask);
+	}
+
+	return handler;
+}

src/spec-node/devContainersSpecCLI.ts

@@ -13,15 +13,15 @@ import { SubstitutedConfig, createContainerProperties, createFeaturesTempFolder,
 import { URI } from 'vscode-uri';
 import { ContainerError } from '../spec-common/errors';
 import { Log, LogDimensions, LogLevel, makeLog, mapLogLevel } from '../spec-utils/log';
-import { probeRemoteEnv, runLifecycleHooks, runRemoteCommand, UserEnvProbe, setupInContainer, readSecretsFromFile } from '../spec-common/injectHeadless';
+import { probeRemoteEnv, runLifecycleHooks, runRemoteCommand, UserEnvProbe, setupInContainer } from '../spec-common/injectHeadless';
 import { extendImage } from './containerFeatures';
 import { DockerCLIParameters, dockerPtyCLI, inspectContainer } from '../spec-shutdown/dockerUtils';
 import { buildAndExtendDockerCompose, dockerComposeCLIConfig, getDefaultImageName, getProjectName, readDockerComposeConfig, readVersionPrefix } from './dockerCompose';
 import { DevContainerConfig, DevContainerFromDockerComposeConfig, DevContainerFromDockerfileConfig, getDockerComposeFilePaths } from '../spec-configuration/configuration';
 import { workspaceFromPath } from '../spec-utils/workspaces';
 import { readDevContainerConfigFile } from './configContainer';
 import { getDefaultDevContainerConfigPath, getDevContainerConfigPathIn, uriToFsPath } from '../spec-configuration/configurationCommonUtils';
-import { getCLIHost } from '../spec-common/cliHost';
+import { CLIHost, getCLIHost } from '../spec-common/cliHost';
 import { loadNativeModule, processSignals } from '../spec-common/commonUtils';
 import { FeaturesConfig, generateFeaturesConfig, getContainerFeaturesFolder } from '../spec-configuration/containerFeaturesConfiguration';
 import { featuresTestOptions, featuresTestHandler } from './featuresCLI/test';
@@ -200,6 +200,11 @@ async function provision({
 	const addCacheFroms = addCacheFrom ? (Array.isArray(addCacheFrom) ? addCacheFrom as string[] : [addCacheFrom]) : [];
 	const additionalFeatures = additionalFeaturesJson ? jsonc.parse(additionalFeaturesJson) as Record<string, string | boolean | Record<string, string | boolean>> : {};
 	const providedIdLabels = idLabel ? Array.isArray(idLabel) ? idLabel as string[] : [idLabel] : undefined;
+
+	const cwd = workspaceFolder || process.cwd();
+	const cliHost = await getCLIHost(cwd, loadNativeModule);
+	const secretsP = readSecretsFromFile({ secretsFile, cliHost });
+
 	const options: ProvisionOptions = {
 		dockerPath,
 		dockerComposePath,
@@ -238,7 +243,7 @@ async function provision({
 		},
 		updateRemoteUserUIDDefault,
 		remoteEnv: envListToObj(addRemoteEnvs),
-		secretsFile,
+		secretsP,
 		additionalCacheFroms: addCacheFroms,
 		useBuildKit: buildkit,
 		buildxPlatform: undefined,
@@ -251,7 +256,7 @@ async function provision({
 		skipPersistingCustomizationsFromFeatures: false,
 		omitConfigRemotEnvFromMetadata: omitConfigRemotEnvFromMetadata,
 		experimentalLockfile,
-		experimentalFrozenLockfile
+		experimentalFrozenLockfile,
 	};
 
 	const result = await doProvision(options, providedIdLabels);
@@ -776,6 +781,11 @@ async function doRunUserCommands({
 		const addRemoteEnvs = addRemoteEnv ? (Array.isArray(addRemoteEnv) ? addRemoteEnv as string[] : [addRemoteEnv]) : [];
 		const configFile = configParam ? URI.file(path.resolve(process.cwd(), configParam)) : undefined;
 		const overrideConfigFile = overrideConfig ? URI.file(path.resolve(process.cwd(), overrideConfig)) : undefined;
+
+		const cwd = workspaceFolder || process.cwd();
+		const cliHost = await getCLIHost(cwd, loadNativeModule);
+		const secretsP = readSecretsFromFile({ secretsFile, cliHost });
+
 		const params = await createDockerParams({
 			dockerPath,
 			dockerComposePath,
@@ -814,11 +824,11 @@ async function doRunUserCommands({
 				targetPath: dotfilesTargetPath,
 			},
 			containerSessionDataFolder,
-			secretsFile
+			secretsP,
 		}, disposables);
 
 		const { common } = params;
-		const { cliHost, output } = common;
+		const { output } = common;
 		const workspace = workspaceFolder ? workspaceFromPath(cliHost.path, workspaceFolder) : undefined;
 		const configPath = configFile ? configFile : workspace
 			? (await getDevContainerConfigPathIn(cliHost, workspace.configFolderPath)
@@ -848,7 +858,6 @@ async function doRunUserCommands({
 		const containerProperties = await createContainerProperties(params, container.Id, configs?.workspaceConfig.workspaceFolder, mergedConfig.remoteUser);
 		const updatedConfig = containerSubstitute(cliHost.platform, config.config.configFilePath, containerProperties.env, mergedConfig);
 		const remoteEnvP = probeRemoteEnv(common, containerProperties, updatedConfig);
-		const secretsP = readSecretsFromFile(common);
 		const result = await runLifecycleHooks(common, lifecycleCommandOriginMapFromMetadata(imageMetadata), containerProperties, updatedConfig, remoteEnvP, secretsP, stopForPersonalization);
 		return {
 			outcome: 'success' as 'success',
@@ -1230,3 +1239,31 @@ function createStdoutResizeEmitter(disposables: (() => Promise<unknown> | void)[
 	disposables.push(() => emitter.dispose());
 	return emitter.event;
 }
+
+async function readSecretsFromFile(params: { output?: Log; secretsFile?: string; cliHost: CLIHost }) {
+	const { secretsFile, cliHost, output } = params;
+	if (!secretsFile) {
+		return {};
+	}
+
+	try {
+		const fileBuff = await cliHost.readFile(secretsFile);
+		const parseErrors: jsonc.ParseError[] = [];
+		const secrets = jsonc.parse(fileBuff.toString(), parseErrors) as Record<string, string>;
+		if (parseErrors.length) {
+			throw new Error('Invalid json data');
+		}
+
+		return secrets;
+	}
+	catch (e) {
+		if (output) {
+			output.write(`Failed to read/parse secrets from file '${secretsFile}'`, LogLevel.Error);
+		}
+
+		throw new ContainerError({
+			description: 'Failed to read/parse secrets',
+			originalError: e
+		});
+	}
+}

src/spec-utils/log.ts

@@ -296,3 +296,30 @@ export function toWarningText(str: string) {
 		.map(line => `${line}`)
 		.join('\r\n') + '\r\n';
 }
+
+export function replaceAllLog(origin: LogHandler, values: string[], replacement: string): LogHandler {
+	values = values
+		.map(v => v.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'))
+		.filter(v => v.length);
+	if (!values.length) {
+		return origin;
+	}
+	const r = new RegExp(values.join('|'), 'g');
+	return {
+		event: e => {
+			if ('text' in e) {
+				origin.event({
+					...e,
+					text: e.text.replace(r, replacement),
+				});
+			} else if (e.type === 'progress' && e.stepDetail) {
+				origin.event({
+					...e,
+					stepDetail: e.stepDetail.replace(r, replacement),
+				});
+			} else {
+				origin.event(e);
+			}
+		}
+	};
+}

src/test/cli.up.test.ts

@@ -51,10 +51,11 @@ describe('Dev Containers CLI', function () {
 			await shellExec(`rm -f ${testFolder}/*.testMarker`, undefined, undefined, true);
 			const secrets = {
 				'SECRET1': 'SecretValue1',
+				'MASK_IT': 'container',
 			};
 			await shellExec(`printf '${JSON.stringify(secrets)}' > ${testFolder}/test-secrets-temp.json`, undefined, undefined, true);
 
-			const res = await shellExec(`${cli} up --workspace-folder ${__dirname}/configs/image-with-git-feature --dotfiles-repository https://github.com/codspace/test-dotfiles --secrets-file ${testFolder}/test-secrets-temp.json`);
+			const res = await shellExec(`${cli} up --workspace-folder ${__dirname}/configs/image-with-git-feature --dotfiles-repository https://github.com/codspace/test-dotfiles --secrets-file ${testFolder}/test-secrets-temp.json --log-level trace --log-format json`);
 			const response = JSON.parse(res.stdout);
 			assert.equal(response.outcome, 'success');
 			containerId = response.containerId;
@@ -69,6 +70,12 @@ describe('Dev Containers CLI', function () {
 			assert.match(stdout, /SECRET1=SecretValue1/);
 			assert.match(stdout, /TEST_REMOTE_ENV=Value 1/);
 
+			// assert secret masking
+			// We log the message `Starting container` from CLI. Since the word `container` is specified as a secret here, that should get masked
+			const logs = res.stderr;
+			assert.match(logs, /Starting \*\*\*\*\*\*\*\*/);
+			assert.doesNotMatch(logs, /Starting container/);
+
 			await shellExec(`docker rm -f ${containerId}`);
 		});
 

src/test/container-features/lifecycleHooks.test.ts

@@ -202,9 +202,10 @@ describe('Feature lifecycle hooks', function () {
 				await shellExec(`rm -f ${testFolder}/*.testMarker`, undefined, undefined, true);
 				const secrets = {
 					'SECRET1': 'SecretValue1',
+					'MASK_IT': 'cycle',
 				};
 				await shellExec(`printf '${JSON.stringify(secrets)}' > ${testFolder}/test-secrets-temp.json`, undefined, undefined, true);
-				containerId = (await devContainerUp(cli, testFolder, { 'logLevel': 'trace', extraArgs: '--skip-post-create' })).containerId;
+				containerId = (await devContainerUp(cli, testFolder, { 'logLevel': 'trace', extraArgs: `--secrets-file ${testFolder}/test-secrets-temp.json --skip-post-create` })).containerId;
 				await shellExec(`rm -f ${testFolder}/*.testMarker`, undefined, undefined, true);
 			});
 
@@ -262,6 +263,12 @@ describe('Feature lifecycle hooks', function () {
 						assert.strictEqual(catResp.error, null);
 						assert.match(catResp.stdout, /SECRET1=SecretValue1/);
 					}
+
+					// assert secret masking
+					// We log the string `LifecycleCommandExecutionMap: ...` from CLI. Since the word `cycle` is specified as a secret here, that should get masked
+					const logs = res.stderr;
+					assert.match(logs, /Life\*\*\*\*\*\*\*\*CommandExecutionMap: /);
+					assert.notMatch(logs, /LifecycleCommandExecutionMap: /);
 				}
 			});
 		});