feat: add keychain to redact known sensitive information from loggers

This sets up a Keychain, a holder of known secrets. When a new secret
is created it should be registered in the keychain so loggers can
redact them and avoid leaking secrets into our logs.

Author: kmruiz

src/common/config.ts

@@ -3,6 +3,8 @@ import os from "os";
 import argv from "yargs-parser";
 import type { CliOptions, ConnectionInfo } from "@mongosh/arg-parser";
 import { generateConnectionInfoFromCliArgs } from "@mongosh/arg-parser";
+import { Keychain } from "./keychain.js";
+import type { Secret } from "./keychain.js";
 
 // From: https://github.com/mongodb-js/mongosh/blob/main/packages/cli-repl/src/arg-parser.ts
 const OPTIONS = {
@@ -316,6 +318,29 @@ function commaSeparatedToArray<T extends string[]>(str: string | string[] | unde
     return str as T;
 }
 
+export function registerKnownSecretsInRootKeychain(userConfig: UserConfig): void {
+    const keychain = Keychain.root;
+
+    const maybeRegister = (value: string | undefined, kind: Secret["kind"]) => {
+        if (value) {
+            keychain.register(value, kind);
+        }
+    };
+
+    maybeRegister(userConfig.apiClientId, "user");
+    maybeRegister(userConfig.apiClientSecret, "password");
+    maybeRegister(userConfig.awsAccessKeyId, "password");
+    maybeRegister(userConfig.awsIamSessionToken, "password");
+    maybeRegister(userConfig.awsSecretAccessKey, "password");
+    maybeRegister(userConfig.awsSessionToken, "password");
+    maybeRegister(userConfig.password, "password");
+    maybeRegister(userConfig.tlsCAFile, "url");
+    maybeRegister(userConfig.tlsCRLFile, "url");
+    maybeRegister(userConfig.tlsCertificateKeyFile, "url");
+    maybeRegister(userConfig.tlsCertificateKeyFilePassword, "password");
+    maybeRegister(userConfig.username, "user");
+}
+
 export function setupUserConfig({
     cli,
     env,
@@ -369,6 +394,7 @@ export function setupUserConfig({
         }
     }
 
+    registerKnownSecretsInRootKeychain(userConfig);
     return userConfig;
 }
 

src/common/keychain.ts

@@ -0,0 +1,38 @@
+export type Secret = {
+    value: string;
+    kind: string;
+};
+
+/**
+ * This class holds the secrets of a single server. Ideally, we might want to have a keychain
+ * per session, but right now the loggers are set up by server and are not aware of the concept
+ * of session and this would require a bigger refactor.
+ *
+ * Whenever we identify or create a secret (for example, Atlas login, CLI arguments...) we
+ * should register them in the root Keychain (`Keychain.root.register`) or preferably
+ * on the session keychain if available `this.session.keychain`.
+ **/
+export class Keychain {
+    private secrets: Secret[];
+    private static rootKeychain: Keychain = new Keychain();
+
+    private constructor() {
+        this.secrets = [];
+    }
+
+    static get root(): Keychain {
+        return Keychain.rootKeychain;
+    }
+
+    register(value: Secret["value"], kind: Secret["kind"]): void {
+        this.secrets.push({ value, kind });
+    }
+
+    clearAllSecrets(): void {
+        this.secrets = [];
+    }
+
+    get allSecrets(): Secret[] {
+        return [...this.secrets];
+    }
+}

src/common/logger.ts

@@ -5,6 +5,7 @@ import redact from "mongodb-redact";
 import type { LoggingMessageNotification } from "@modelcontextprotocol/sdk/types.js";
 import { EventEmitter } from "events";
 import type { Server } from "../lib.js";
+import type { Keychain } from "./keychain.js";
 
 export type LogLevel = LoggingMessageNotification["params"]["level"];
 
@@ -83,6 +84,10 @@ type DefaultEventMap = [never];
 export abstract class LoggerBase<T extends EventMap<T> = DefaultEventMap> extends EventEmitter<T> {
     private readonly defaultUnredactedLogger: LoggerType = "mcp";
 
+    constructor(private readonly keychain: Keychain | undefined) {
+        super();
+    }
+
     public log(level: LogLevel, payload: LogPayload): void {
         // If no explicit value is supplied for unredacted loggers, default to "mcp"
         const noRedaction = payload.noRedaction !== undefined ? payload.noRedaction : this.defaultUnredactedLogger;
@@ -121,7 +126,7 @@ export abstract class LoggerBase<T extends EventMap<T> = DefaultEventMap> extend
             return message;
         }
 
-        return redact(message);
+        return redact(message, this.keychain?.allSecrets ?? []);
     }
 
     public info(payload: LogPayload): void {
@@ -179,6 +184,10 @@ export abstract class LoggerBase<T extends EventMap<T> = DefaultEventMap> extend
 export class ConsoleLogger extends LoggerBase {
     protected readonly type: LoggerType = "console";
 
+    public constructor(keychain: Keychain) {
+        super(keychain);
+    }
+
     protected logCore(level: LogLevel, payload: LogPayload): void {
         const { id, context, message } = payload;
         console.error(
@@ -200,8 +209,8 @@ export class DiskLogger extends LoggerBase<{ initialized: [] }> {
     private bufferedMessages: { level: LogLevel; payload: LogPayload }[] = [];
     private logWriter?: MongoLogWriter;
 
-    public constructor(logPath: string, onError: (error: Error) => void) {
-        super();
+    public constructor(logPath: string, onError: (error: Error) => void, keychain: Keychain) {
+        super(keychain);
 
         void this.initialize(logPath, onError);
     }
@@ -261,8 +270,11 @@ export class McpLogger extends LoggerBase {
         "emergency",
     ] as const;
 
-    public constructor(private readonly server: Server) {
-        super();
+    public constructor(
+        private readonly server: Server,
+        keychain: Keychain
+    ) {
+        super(keychain);
     }
 
     protected readonly type: LoggerType = "mcp";
@@ -294,7 +306,9 @@ export class CompositeLogger extends LoggerBase {
     private readonly attributes: Record<string, string> = {};
 
     constructor(...loggers: LoggerBase[]) {
-        super();
+        // composite logger does not redact, only the actual delegates do the work
+        // so we don't need the Keychain here
+        super(undefined);
 
         this.loggers = loggers;
     }

src/common/session.ts

@@ -15,6 +15,7 @@ import type {
 import type { NodeDriverServiceProvider } from "@mongosh/service-provider-node-driver";
 import { ErrorCodes, MongoDBError } from "./errors.js";
 import type { ExportsManager } from "./exportsManager.js";
+import { Keychain } from "./keychain.js";
 
 export interface SessionOptions {
     apiBaseUrl: string;
@@ -23,6 +24,7 @@ export interface SessionOptions {
     logger: CompositeLogger;
     exportsManager: ExportsManager;
     connectionManager: ConnectionManager;
+    keychain: Keychain;
 }
 
 export type SessionEvents = {
@@ -37,6 +39,8 @@ export class Session extends EventEmitter<SessionEvents> {
     readonly exportsManager: ExportsManager;
     readonly connectionManager: ConnectionManager;
     readonly apiClient: ApiClient;
+    readonly keychain: Keychain;
+
     mcpClient?: {
         name?: string;
         version?: string;
@@ -52,9 +56,11 @@ export class Session extends EventEmitter<SessionEvents> {
         logger,
         connectionManager,
         exportsManager,
+        keychain,
     }: SessionOptions) {
         super();
 
+        this.keychain = keychain;
         this.logger = logger;
         const credentials: ApiClientCredentials | undefined =
             apiClientId && apiClientSecret

src/index.ts

@@ -42,6 +42,7 @@ import { packageInfo } from "./common/packageInfo.js";
 import { StdioRunner } from "./transports/stdio.js";
 import { StreamableHttpRunner } from "./transports/streamableHttp.js";
 import { systemCA } from "@mongodb-js/devtools-proxy-support";
+import { Keychain } from "./common/keychain.js";
 
 async function main(): Promise<void> {
     systemCA().catch(() => undefined); // load system CA asynchronously as in mongosh
@@ -114,7 +115,7 @@ main().catch((error: unknown) => {
     // At this point, we may be in a very broken state, so we can't rely on the logger
     // being functional. Instead, create a brand new ConsoleLogger and log the error
     // to the console.
-    const logger = new ConsoleLogger();
+    const logger = new ConsoleLogger(Keychain.root);
     logger.emergency({
         id: LogId.serverStartFailure,
         context: "server",

src/tools/atlas/connect/connectCluster.ts

@@ -110,6 +110,9 @@ export class ConnectClusterTool extends AtlasToolBase {
         cn.password = password;
         cn.searchParams.set("authSource", "admin");
 
+        this.session.keychain.register(username, "user");
+        this.session.keychain.register(password, "password");
+
         return { connectionString: cn.toString(), atlas: connectedAtlasCluster };
     }
 

src/tools/atlas/create/createDBUser.ts

@@ -78,6 +78,11 @@ export class CreateDBUserTool extends AtlasToolBase {
             body: input,
         });
 
+        this.session.keychain.register(username, "user");
+        if (password) {
+            this.session.keychain.register(password, "password");
+        }
+
         return {
             content: [
                 {

src/transports/base.ts

@@ -9,6 +9,7 @@ import { CompositeLogger, ConsoleLogger, DiskLogger, McpLogger } from "../common
 import { ExportsManager } from "../common/exportsManager.js";
 import { DeviceId } from "../helpers/deviceId.js";
 import { type ConnectionManagerFactoryFn } from "../common/connectionManager.js";
+import { Keychain } from "../common/keychain.js";
 
 export abstract class TransportRunnerBase {
     public logger: LoggerBase;
@@ -21,16 +22,20 @@ export abstract class TransportRunnerBase {
     ) {
         const loggers: LoggerBase[] = [...additionalLoggers];
         if (this.userConfig.loggers.includes("stderr")) {
-            loggers.push(new ConsoleLogger());
+            loggers.push(new ConsoleLogger(Keychain.root));
         }
 
         if (this.userConfig.loggers.includes("disk")) {
             loggers.push(
-                new DiskLogger(this.userConfig.logPath, (err) => {
-                    // If the disk logger fails to initialize, we log the error to stderr and exit
-                    console.error("Error initializing disk logger:", err);
-                    process.exit(1);
-                })
+                new DiskLogger(
+                    this.userConfig.logPath,
+                    (err) => {
+                        // If the disk logger fails to initialize, we log the error to stderr and exit
+                        console.error("Error initializing disk logger:", err);
+                        process.exit(1);
+                    },
+                    Keychain.root
+                )
             );
         }
 
@@ -59,6 +64,7 @@ export abstract class TransportRunnerBase {
             logger,
             exportsManager,
             connectionManager,
+            keychain: Keychain.root,
         });
 
         const telemetry = Telemetry.create(session, this.userConfig, this.deviceId);
@@ -73,7 +79,7 @@ export abstract class TransportRunnerBase {
         // We need to create the MCP logger after the server is constructed
         // because it needs the server instance
         if (this.userConfig.loggers.includes("mcp")) {
-            logger.addLogger(new McpLogger(result));
+            logger.addLogger(new McpLogger(result, Keychain.root));
         }
 
         return result;

tests/integration/tools/atlas/dbUsers.test.ts

@@ -74,6 +74,16 @@ describeWithAtlas("db users", (integration) => {
                 expect(elements[0]?.text).toContain("created successfully");
                 expect(elements[0]?.text).toContain(userName);
                 expect(elements[0]?.text).not.toContain("testpassword");
+                expect(integration.mcpServer().session.keychain.allSecrets).to.deep.equal([
+                    {
+                        value: userName,
+                        kind: "user",
+                    },
+                    {
+                        value: "testpassword",
+                        kind: "password",
+                    },
+                ]);
             });
 
             it("should create a database user with generated password", async () => {
@@ -83,6 +93,21 @@ describeWithAtlas("db users", (integration) => {
                 expect(elements[0]?.text).toContain("created successfully");
                 expect(elements[0]?.text).toContain(userName);
                 expect(elements[0]?.text).toContain("with password: `");
+
+                const passwordStart = elements[0]?.text.lastIndexOf(":") ?? -1;
+                const passwordEnd = elements[0]?.text.length ?? 1 - 1;
+
+                const password = elements[0]?.text.substring(passwordStart).substring(0, passwordEnd).trim();
+                expect(integration.mcpServer().session.keychain.allSecrets).to.deep.equal([
+                    {
+                        value: userName,
+                        kind: "user",
+                    },
+                    {
+                        value: password,
+                        kind: "password",
+                    },
+                ]);
             });
 
             it("should add current IP to access list when creating a database user", async () => {

tests/unit/common/config.test.ts

@@ -1,7 +1,14 @@
-import { describe, it, expect, vi, beforeEach } from "vitest";
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
 import type { UserConfig } from "../../../src/common/config.js";
-import { setupUserConfig, defaultUserConfig, warnAboutDeprecatedCliArgs } from "../../../src/common/config.js";
+import {
+    setupUserConfig,
+    defaultUserConfig,
+    warnAboutDeprecatedCliArgs,
+    registerKnownSecretsInRootKeychain,
+} from "../../../src/common/config.js";
 import type { CliOptions } from "@mongosh/arg-parser";
+import { Keychain } from "../../../src/common/keychain.js";
+import type { Secret } from "../../../src/common/keychain.js";
 
 describe("config", () => {
     describe("env var parsing", () => {
@@ -641,4 +648,40 @@ describe("Deprecated CLI arguments", () => {
             });
         });
     }
+
+    describe("keychain management", () => {
+        type TestCase = { readonly cliArg: keyof UserConfig; secretKind: Secret["kind"] };
+        const testCases = [
+            { cliArg: "apiClientId", secretKind: "user" },
+            { cliArg: "apiClientSecret", secretKind: "password" },
+            { cliArg: "awsAccessKeyId", secretKind: "password" },
+            { cliArg: "awsIamSessionToken", secretKind: "password" },
+            { cliArg: "awsSecretAccessKey", secretKind: "password" },
+            { cliArg: "awsSessionToken", secretKind: "password" },
+            { cliArg: "password", secretKind: "password" },
+            { cliArg: "tlsCAFile", secretKind: "url" },
+            { cliArg: "tlsCRLFile", secretKind: "url" },
+            { cliArg: "tlsCertificateKeyFile", secretKind: "url" },
+            { cliArg: "tlsCertificateKeyFilePassword", secretKind: "password" },
+            { cliArg: "username", secretKind: "user" },
+        ] as TestCase[];
+        let keychain: Keychain;
+
+        beforeEach(() => {
+            keychain = Keychain.root;
+            keychain.clearAllSecrets();
+        });
+
+        afterEach(() => {
+            keychain.clearAllSecrets();
+        });
+
+        for (const { cliArg, secretKind } of testCases) {
+            it(`should register ${cliArg} as a secret of kind ${secretKind} in the root keychain`, () => {
+                registerKnownSecretsInRootKeychain({ [cliArg]: cliArg });
+
+                expect(keychain.allSecrets).to.deep.equal([{ value: cliArg, kind: secretKind }]);
+            });
+        }
+    });
 });