fix(#338): Whitelist allowed environment variables in Docker containers
- Add DEFAULT_ENV_WHITELIST constant with safe env vars (AGENT_ID, TASK_ID, NODE_ENV, LOG_LEVEL, TZ, MOSAIC_* vars, etc.) - Implement filterEnvVars() to separate allowed/filtered vars - Log security warning when non-whitelisted vars are filtered - Support custom whitelist via orchestrator.sandbox.envWhitelist config - Add comprehensive tests for whitelist functionality (39 tests passing) Prevents accidental leakage of secrets like API keys, database credentials, AWS secrets, etc. to Docker containers. Refs #338 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
@@ -1,7 +1,7 @@
|
|||||||
import { ConfigService } from "@nestjs/config";
|
import { ConfigService } from "@nestjs/config";
|
||||||
import { Logger } from "@nestjs/common";
|
import { Logger } from "@nestjs/common";
|
||||||
import { describe, it, expect, beforeEach, vi, afterEach } from "vitest";
|
import { describe, it, expect, beforeEach, vi, afterEach } from "vitest";
|
||||||
import { DockerSandboxService } from "./docker-sandbox.service";
|
import { DockerSandboxService, DEFAULT_ENV_WHITELIST } from "./docker-sandbox.service";
|
||||||
import Docker from "dockerode";
|
import Docker from "dockerode";
|
||||||
|
|
||||||
describe("DockerSandboxService", () => {
|
describe("DockerSandboxService", () => {
|
||||||
@@ -127,14 +127,14 @@ describe("DockerSandboxService", () => {
|
|||||||
);
|
);
|
||||||
});
|
});
|
||||||
|
|
||||||
it("should create a container with custom environment variables", async () => {
|
it("should create a container with whitelisted environment variables", async () => {
|
||||||
const agentId = "agent-123";
|
const agentId = "agent-123";
|
||||||
const taskId = "task-456";
|
const taskId = "task-456";
|
||||||
const workspacePath = "/workspace/agent-123";
|
const workspacePath = "/workspace/agent-123";
|
||||||
const options = {
|
const options = {
|
||||||
env: {
|
env: {
|
||||||
CUSTOM_VAR: "value123",
|
NODE_ENV: "production",
|
||||||
ANOTHER_VAR: "value456",
|
LOG_LEVEL: "debug",
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -145,8 +145,8 @@ describe("DockerSandboxService", () => {
|
|||||||
Env: expect.arrayContaining([
|
Env: expect.arrayContaining([
|
||||||
`AGENT_ID=${agentId}`,
|
`AGENT_ID=${agentId}`,
|
||||||
`TASK_ID=${taskId}`,
|
`TASK_ID=${taskId}`,
|
||||||
"CUSTOM_VAR=value123",
|
"NODE_ENV=production",
|
||||||
"ANOTHER_VAR=value456",
|
"LOG_LEVEL=debug",
|
||||||
]),
|
]),
|
||||||
})
|
})
|
||||||
);
|
);
|
||||||
@@ -373,4 +373,227 @@ describe("DockerSandboxService", () => {
|
|||||||
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("SECURITY WARNING"));
|
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("SECURITY WARNING"));
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("environment variable whitelist", () => {
|
||||||
|
describe("getEnvWhitelist", () => {
|
||||||
|
it("should return default whitelist when no custom whitelist is configured", () => {
|
||||||
|
const whitelist = service.getEnvWhitelist();
|
||||||
|
|
||||||
|
expect(whitelist).toEqual(DEFAULT_ENV_WHITELIST);
|
||||||
|
expect(whitelist).toContain("AGENT_ID");
|
||||||
|
expect(whitelist).toContain("TASK_ID");
|
||||||
|
expect(whitelist).toContain("NODE_ENV");
|
||||||
|
expect(whitelist).toContain("LOG_LEVEL");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should return custom whitelist when configured", () => {
|
||||||
|
const customWhitelist = ["CUSTOM_VAR_1", "CUSTOM_VAR_2"];
|
||||||
|
const customConfigService = {
|
||||||
|
get: vi.fn((key: string, defaultValue?: unknown) => {
|
||||||
|
const config: Record<string, unknown> = {
|
||||||
|
"orchestrator.docker.socketPath": "/var/run/docker.sock",
|
||||||
|
"orchestrator.sandbox.enabled": true,
|
||||||
|
"orchestrator.sandbox.defaultImage": "node:20-alpine",
|
||||||
|
"orchestrator.sandbox.defaultMemoryMB": 512,
|
||||||
|
"orchestrator.sandbox.defaultCpuLimit": 1.0,
|
||||||
|
"orchestrator.sandbox.networkMode": "bridge",
|
||||||
|
"orchestrator.sandbox.envWhitelist": customWhitelist,
|
||||||
|
};
|
||||||
|
return config[key] !== undefined ? config[key] : defaultValue;
|
||||||
|
}),
|
||||||
|
} as unknown as ConfigService;
|
||||||
|
|
||||||
|
const customService = new DockerSandboxService(customConfigService, mockDocker);
|
||||||
|
const whitelist = customService.getEnvWhitelist();
|
||||||
|
|
||||||
|
expect(whitelist).toEqual(customWhitelist);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("filterEnvVars", () => {
|
||||||
|
it("should allow whitelisted environment variables", () => {
|
||||||
|
const envVars = {
|
||||||
|
NODE_ENV: "production",
|
||||||
|
LOG_LEVEL: "debug",
|
||||||
|
TZ: "UTC",
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = service.filterEnvVars(envVars);
|
||||||
|
|
||||||
|
expect(result.allowed).toEqual({
|
||||||
|
NODE_ENV: "production",
|
||||||
|
LOG_LEVEL: "debug",
|
||||||
|
TZ: "UTC",
|
||||||
|
});
|
||||||
|
expect(result.filtered).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should filter non-whitelisted environment variables", () => {
|
||||||
|
const envVars = {
|
||||||
|
NODE_ENV: "production",
|
||||||
|
DATABASE_URL: "postgres://secret@host/db",
|
||||||
|
API_KEY: "sk-secret-key",
|
||||||
|
AWS_SECRET_ACCESS_KEY: "super-secret",
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = service.filterEnvVars(envVars);
|
||||||
|
|
||||||
|
expect(result.allowed).toEqual({
|
||||||
|
NODE_ENV: "production",
|
||||||
|
});
|
||||||
|
expect(result.filtered).toContain("DATABASE_URL");
|
||||||
|
expect(result.filtered).toContain("API_KEY");
|
||||||
|
expect(result.filtered).toContain("AWS_SECRET_ACCESS_KEY");
|
||||||
|
expect(result.filtered).toHaveLength(3);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should handle empty env vars object", () => {
|
||||||
|
const result = service.filterEnvVars({});
|
||||||
|
|
||||||
|
expect(result.allowed).toEqual({});
|
||||||
|
expect(result.filtered).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should handle all vars being filtered", () => {
|
||||||
|
const envVars = {
|
||||||
|
SECRET_KEY: "secret",
|
||||||
|
PASSWORD: "password123",
|
||||||
|
PRIVATE_TOKEN: "token",
|
||||||
|
};
|
||||||
|
|
||||||
|
const result = service.filterEnvVars(envVars);
|
||||||
|
|
||||||
|
expect(result.allowed).toEqual({});
|
||||||
|
expect(result.filtered).toEqual(["SECRET_KEY", "PASSWORD", "PRIVATE_TOKEN"]);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("createContainer with filtering", () => {
|
||||||
|
let warnSpy: ReturnType<typeof vi.spyOn>;
|
||||||
|
|
||||||
|
beforeEach(() => {
|
||||||
|
warnSpy = vi.spyOn(Logger.prototype, "warn").mockImplementation(() => undefined);
|
||||||
|
});
|
||||||
|
|
||||||
|
afterEach(() => {
|
||||||
|
warnSpy.mockRestore();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should filter non-whitelisted vars and only pass allowed vars to container", async () => {
|
||||||
|
const agentId = "agent-123";
|
||||||
|
const taskId = "task-456";
|
||||||
|
const workspacePath = "/workspace/agent-123";
|
||||||
|
const options = {
|
||||||
|
env: {
|
||||||
|
NODE_ENV: "production",
|
||||||
|
DATABASE_URL: "postgres://secret@host/db",
|
||||||
|
LOG_LEVEL: "info",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
await service.createContainer(agentId, taskId, workspacePath, options);
|
||||||
|
|
||||||
|
// Should include whitelisted vars
|
||||||
|
expect(mockDocker.createContainer).toHaveBeenCalledWith(
|
||||||
|
expect.objectContaining({
|
||||||
|
Env: expect.arrayContaining([
|
||||||
|
`AGENT_ID=${agentId}`,
|
||||||
|
`TASK_ID=${taskId}`,
|
||||||
|
"NODE_ENV=production",
|
||||||
|
"LOG_LEVEL=info",
|
||||||
|
]),
|
||||||
|
})
|
||||||
|
);
|
||||||
|
|
||||||
|
// Should NOT include filtered vars
|
||||||
|
const callArgs = (mockDocker.createContainer as ReturnType<typeof vi.fn>).mock.calls[0][0];
|
||||||
|
expect(callArgs.Env).not.toContain("DATABASE_URL=postgres://secret@host/db");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should log warning when env vars are filtered", async () => {
|
||||||
|
const agentId = "agent-123";
|
||||||
|
const taskId = "task-456";
|
||||||
|
const workspacePath = "/workspace/agent-123";
|
||||||
|
const options = {
|
||||||
|
env: {
|
||||||
|
DATABASE_URL: "postgres://secret@host/db",
|
||||||
|
API_KEY: "sk-secret",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
await service.createContainer(agentId, taskId, workspacePath, options);
|
||||||
|
|
||||||
|
expect(warnSpy).toHaveBeenCalledWith(
|
||||||
|
expect.stringContaining("SECURITY: Filtered 2 non-whitelisted env var(s)")
|
||||||
|
);
|
||||||
|
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("DATABASE_URL"));
|
||||||
|
expect(warnSpy).toHaveBeenCalledWith(expect.stringContaining("API_KEY"));
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should not log warning when all vars are whitelisted", async () => {
|
||||||
|
const agentId = "agent-123";
|
||||||
|
const taskId = "task-456";
|
||||||
|
const workspacePath = "/workspace/agent-123";
|
||||||
|
const options = {
|
||||||
|
env: {
|
||||||
|
NODE_ENV: "production",
|
||||||
|
LOG_LEVEL: "debug",
|
||||||
|
},
|
||||||
|
};
|
||||||
|
|
||||||
|
await service.createContainer(agentId, taskId, workspacePath, options);
|
||||||
|
|
||||||
|
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("SECURITY: Filtered"));
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should not log warning when no env vars are provided", async () => {
|
||||||
|
const agentId = "agent-123";
|
||||||
|
const taskId = "task-456";
|
||||||
|
const workspacePath = "/workspace/agent-123";
|
||||||
|
|
||||||
|
await service.createContainer(agentId, taskId, workspacePath);
|
||||||
|
|
||||||
|
expect(warnSpy).not.toHaveBeenCalledWith(expect.stringContaining("SECURITY: Filtered"));
|
||||||
|
});
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
describe("DEFAULT_ENV_WHITELIST", () => {
|
||||||
|
it("should contain essential agent identification vars", () => {
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("AGENT_ID");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("TASK_ID");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should contain Node.js runtime vars", () => {
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("NODE_ENV");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("NODE_OPTIONS");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should contain logging vars", () => {
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("LOG_LEVEL");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("DEBUG");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should contain locale vars", () => {
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("LANG");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("LC_ALL");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("TZ");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should contain Mosaic-specific safe vars", () => {
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("MOSAIC_WORKSPACE_ID");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("MOSAIC_PROJECT_ID");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).toContain("MOSAIC_AGENT_TYPE");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("should NOT contain sensitive var patterns", () => {
|
||||||
|
// Verify common sensitive vars are not in the whitelist
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("DATABASE_URL");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("API_KEY");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("SECRET");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("PASSWORD");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("AWS_SECRET_ACCESS_KEY");
|
||||||
|
expect(DEFAULT_ENV_WHITELIST).not.toContain("ANTHROPIC_API_KEY");
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -3,6 +3,31 @@ import { ConfigService } from "@nestjs/config";
|
|||||||
import Docker from "dockerode";
|
import Docker from "dockerode";
|
||||||
import { DockerSandboxOptions, ContainerCreateResult } from "./types/docker-sandbox.types";
|
import { DockerSandboxOptions, ContainerCreateResult } from "./types/docker-sandbox.types";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Default whitelist of allowed environment variable names/patterns for Docker containers.
|
||||||
|
* Only these variables will be passed to spawned agent containers.
|
||||||
|
* This prevents accidental leakage of secrets like API keys, database credentials, etc.
|
||||||
|
*/
|
||||||
|
export const DEFAULT_ENV_WHITELIST: readonly string[] = [
|
||||||
|
// Agent identification
|
||||||
|
"AGENT_ID",
|
||||||
|
"TASK_ID",
|
||||||
|
// Node.js runtime
|
||||||
|
"NODE_ENV",
|
||||||
|
"NODE_OPTIONS",
|
||||||
|
// Logging
|
||||||
|
"LOG_LEVEL",
|
||||||
|
"DEBUG",
|
||||||
|
// Locale
|
||||||
|
"LANG",
|
||||||
|
"LC_ALL",
|
||||||
|
"TZ",
|
||||||
|
// Application-specific safe vars
|
||||||
|
"MOSAIC_WORKSPACE_ID",
|
||||||
|
"MOSAIC_PROJECT_ID",
|
||||||
|
"MOSAIC_AGENT_TYPE",
|
||||||
|
] as const;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Service for managing Docker container isolation for agents
|
* Service for managing Docker container isolation for agents
|
||||||
* Provides secure sandboxing with resource limits and cleanup
|
* Provides secure sandboxing with resource limits and cleanup
|
||||||
@@ -16,6 +41,7 @@ export class DockerSandboxService {
|
|||||||
private readonly defaultMemoryMB: number;
|
private readonly defaultMemoryMB: number;
|
||||||
private readonly defaultCpuLimit: number;
|
private readonly defaultCpuLimit: number;
|
||||||
private readonly defaultNetworkMode: string;
|
private readonly defaultNetworkMode: string;
|
||||||
|
private readonly envWhitelist: readonly string[];
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
private readonly configService: ConfigService,
|
private readonly configService: ConfigService,
|
||||||
@@ -50,6 +76,10 @@ export class DockerSandboxService {
|
|||||||
"bridge"
|
"bridge"
|
||||||
);
|
);
|
||||||
|
|
||||||
|
// Load custom whitelist from config, or use defaults
|
||||||
|
const customWhitelist = this.configService.get<string[]>("orchestrator.sandbox.envWhitelist");
|
||||||
|
this.envWhitelist = customWhitelist ?? DEFAULT_ENV_WHITELIST;
|
||||||
|
|
||||||
this.logger.log(
|
this.logger.log(
|
||||||
`DockerSandboxService initialized (enabled: ${this.sandboxEnabled.toString()}, socket: ${socketPath})`
|
`DockerSandboxService initialized (enabled: ${this.sandboxEnabled.toString()}, socket: ${socketPath})`
|
||||||
);
|
);
|
||||||
@@ -87,13 +117,23 @@ export class DockerSandboxService {
|
|||||||
// Convert CPU limit to NanoCPUs (1.0 = 1,000,000,000 nanocpus)
|
// Convert CPU limit to NanoCPUs (1.0 = 1,000,000,000 nanocpus)
|
||||||
const nanoCpus = Math.floor(cpuLimit * 1000000000);
|
const nanoCpus = Math.floor(cpuLimit * 1000000000);
|
||||||
|
|
||||||
// Build environment variables
|
// Build environment variables with whitelist filtering
|
||||||
const env = [`AGENT_ID=${agentId}`, `TASK_ID=${taskId}`];
|
const env = [`AGENT_ID=${agentId}`, `TASK_ID=${taskId}`];
|
||||||
|
|
||||||
if (options?.env) {
|
if (options?.env) {
|
||||||
Object.entries(options.env).forEach(([key, value]) => {
|
const { allowed, filtered } = this.filterEnvVars(options.env);
|
||||||
|
|
||||||
|
// Add allowed vars
|
||||||
|
Object.entries(allowed).forEach(([key, value]) => {
|
||||||
env.push(`${key}=${value}`);
|
env.push(`${key}=${value}`);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// Log warning for filtered vars
|
||||||
|
if (filtered.length > 0) {
|
||||||
|
this.logger.warn(
|
||||||
|
`SECURITY: Filtered ${filtered.length.toString()} non-whitelisted env var(s) for agent ${agentId}: ${filtered.join(", ")}`
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Container name with timestamp to ensure uniqueness
|
// Container name with timestamp to ensure uniqueness
|
||||||
@@ -246,4 +286,44 @@ export class DockerSandboxService {
|
|||||||
isEnabled(): boolean {
|
isEnabled(): boolean {
|
||||||
return this.sandboxEnabled;
|
return this.sandboxEnabled;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Get the current environment variable whitelist
|
||||||
|
* @returns The configured whitelist of allowed env var names
|
||||||
|
*/
|
||||||
|
getEnvWhitelist(): readonly string[] {
|
||||||
|
return this.envWhitelist;
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Filter environment variables against the whitelist
|
||||||
|
* @param envVars Object of environment variables to filter
|
||||||
|
* @returns Object with allowed vars and array of filtered var names
|
||||||
|
*/
|
||||||
|
filterEnvVars(envVars: Record<string, string>): {
|
||||||
|
allowed: Record<string, string>;
|
||||||
|
filtered: string[];
|
||||||
|
} {
|
||||||
|
const allowed: Record<string, string> = {};
|
||||||
|
const filtered: string[] = [];
|
||||||
|
|
||||||
|
for (const [key, value] of Object.entries(envVars)) {
|
||||||
|
if (this.isEnvVarAllowed(key)) {
|
||||||
|
allowed[key] = value;
|
||||||
|
} else {
|
||||||
|
filtered.push(key);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
return { allowed, filtered };
|
||||||
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Check if an environment variable name is allowed by the whitelist
|
||||||
|
* @param varName Environment variable name to check
|
||||||
|
* @returns True if allowed
|
||||||
|
*/
|
||||||
|
private isEnvVarAllowed(varName: string): boolean {
|
||||||
|
return this.envWhitelist.includes(varName);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user