Skip to content

Commit d4d128b

Browse files
authored
Support runtime secret-backed OpenAI endpoint override in AWF api-proxy (#6656)
* Initial plan * feat: support secret-backed OpenAI endpoint override * fix: address review feedback for secret-backed OpenAI endpoint override Three security issues identified in code review are fixed: 1. Sensitive hostname not leaked in logs/audit (Issue 1): - Add sensitiveAllowedDomains to NetworkOptions and propagate through config - resolveApiTargetsToAllowedDomains routes secret-derived entries to sensitiveAllowedDomains instead of allowedDomains when the array is provided - Squid config generation combines both arrays so egress still works - sensitiveAllowedDomains added to SENSITIVE_CONFIG_KEYS so it is excluded from the debug log and awf-resolved-config.json audit artifact 2. OPENAI_ENDPOINT_OVERRIDE excluded from agent environment (Issue 2): - Added to excluded-vars.ts when enableApiProxy is true so the sidecar endpoint URL never reaches the untrusted agent container 3. Allowlist resolution parity with sidecar routing (Issue 3): - resolveApiTargetsToAllowedDomains accepts new openaiEndpointOverride param - preflight.ts resolves OPENAI_ENDPOINT_OVERRIDE from all config sources (additionalEnv > envFile > process.env) matching getConfigEnvValue semantics - Ensures that --env / --env-file supplied overrides reach the Squid ACL --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent 11b3c19 commit d4d128b

18 files changed

Lines changed: 276 additions & 32 deletions

docs/api-proxy-sidecar.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -318,7 +318,7 @@ If the key is present only in `secrets.*` but not exported into the step's `env:
318318

319319
| Flag | Default | Description |
320320
|------|---------|-------------|
321-
| `--openai-api-target <host>` | `api.openai.com` | Custom upstream for OpenAI API requests (e.g. Azure OpenAI or an internal LLM router). Can also be set via `OPENAI_API_TARGET` env var. |
321+
| `--openai-api-target <host>` | `api.openai.com` | Custom upstream for OpenAI API requests (e.g. Azure OpenAI or an internal LLM router). Can also be set via `OPENAI_API_TARGET` env var (or `OPENAI_ENDPOINT_OVERRIDE` for runtime secret-backed endpoint injection). |
322322
| `--anthropic-api-target <host>` | `api.anthropic.com` | Custom upstream for Anthropic API requests (e.g. an internal Claude router). Can also be set via `ANTHROPIC_API_TARGET` env var. |
323323
| `--copilot-api-target <host>` | auto-derived | Custom upstream for GitHub Copilot API requests (useful for GHES). Can also be set via `COPILOT_API_TARGET` env var. |
324324
| `--vertex-api-target <host>` | `aiplatform.googleapis.com` | Custom upstream for Vertex API requests. Can also be set via `VERTEX_API_TARGET` env var. |

src/api-proxy-config-domains.test.ts

Lines changed: 54 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,60 @@ describe('resolveApiTargetsToAllowedDomains', () => {
4747
expect(domains).toContain('https://env.openai.com');
4848
});
4949

50+
it('should read OPENAI_ENDPOINT_OVERRIDE from env when OPENAI_API_TARGET is not set (backward compat: no sensitiveAllowedDomains)', () => {
51+
const domains: string[] = [];
52+
const env = { OPENAI_ENDPOINT_OVERRIDE: 'https://secret.openai.internal/path' };
53+
resolveApiTargetsToAllowedDomains({}, domains, env);
54+
// Backward compat: when sensitiveAllowedDomains is not supplied, falls back to allowedDomains
55+
expect(domains).toContain('https://secret.openai.internal');
56+
});
57+
58+
it('should route OPENAI_ENDPOINT_OVERRIDE host to sensitiveAllowedDomains when the array is provided', () => {
59+
const domains: string[] = [];
60+
const sensitive: string[] = [];
61+
const env = { OPENAI_ENDPOINT_OVERRIDE: 'https://secret.openai.internal/path' };
62+
resolveApiTargetsToAllowedDomains({}, domains, env, () => {}, sensitive);
63+
expect(domains).not.toContain('https://secret.openai.internal');
64+
expect(sensitive).toContain('https://secret.openai.internal');
65+
});
66+
67+
it('should use pre-resolved openaiEndpointOverride param over env fallback', () => {
68+
const domains: string[] = [];
69+
const sensitive: string[] = [];
70+
const env = { OPENAI_ENDPOINT_OVERRIDE: 'https://env-fallback.openai.internal' };
71+
resolveApiTargetsToAllowedDomains({}, domains, env, () => {}, sensitive, 'https://additional-env.openai.internal');
72+
expect(sensitive).toContain('https://additional-env.openai.internal');
73+
expect(sensitive).not.toContain('https://env-fallback.openai.internal');
74+
});
75+
76+
it('should use openaiEndpointOverride param even when env has no OPENAI_ENDPOINT_OVERRIDE', () => {
77+
const domains: string[] = [];
78+
const sensitive: string[] = [];
79+
resolveApiTargetsToAllowedDomains({}, domains, {}, () => {}, sensitive, 'https://additionalenv-only.openai.internal');
80+
expect(sensitive).toContain('https://additionalenv-only.openai.internal');
81+
expect(domains).not.toContain('https://additionalenv-only.openai.internal');
82+
});
83+
84+
it('should not include OPENAI_ENDPOINT_OVERRIDE host value in debug logs', () => {
85+
const domains: string[] = [];
86+
const debugMessages: string[] = [];
87+
const env = { OPENAI_ENDPOINT_OVERRIDE: 'https://secret.openai.internal/path' };
88+
resolveApiTargetsToAllowedDomains({}, domains, env, (msg) => debugMessages.push(msg));
89+
expect(debugMessages.some(msg => msg.includes('secret.openai.internal'))).toBe(false);
90+
expect(debugMessages).toContain('Auto-added OpenAI endpoint override host to allowed domains');
91+
});
92+
93+
it('should prefer OPENAI_API_TARGET over OPENAI_ENDPOINT_OVERRIDE', () => {
94+
const domains: string[] = [];
95+
const env = {
96+
OPENAI_API_TARGET: 'env.openai.com',
97+
OPENAI_ENDPOINT_OVERRIDE: 'secret.openai.internal',
98+
};
99+
resolveApiTargetsToAllowedDomains({}, domains, env);
100+
expect(domains).toContain('https://env.openai.com');
101+
expect(domains).not.toContain('https://secret.openai.internal');
102+
});
103+
50104
it('should read ANTHROPIC_API_TARGET from env when flag not set', () => {
51105
const domains: string[] = [];
52106
const env = { ANTHROPIC_API_TARGET: 'env.anthropic.com' };

src/api-proxy-config-domains.ts

Lines changed: 55 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -105,6 +105,13 @@ function extractGhesDomainsFromEngineApiTarget(
105105
* @param allowedDomains - The current list of allowed domains (mutated in place)
106106
* @param env - Environment variables (defaults to process.env)
107107
* @param debug - Optional debug logging function
108+
* @param sensitiveAllowedDomains - Optional separate array for sensitive (secret-derived) domains.
109+
* When provided, domains derived from `OPENAI_ENDPOINT_OVERRIDE` are pushed here instead of
110+
* into `allowedDomains` so they are never logged or included in audit artifacts.
111+
* @param openaiEndpointOverride - Optional pre-resolved value of OPENAI_ENDPOINT_OVERRIDE,
112+
* already read from all config sources (additionalEnv, envFile, process.env). When provided,
113+
* it takes precedence over `env['OPENAI_ENDPOINT_OVERRIDE']` so that overrides supplied via
114+
* `--env` or `--env-file` are honoured by allowlist expansion as well as sidecar routing.
108115
* @returns The updated allowedDomains array (same reference, mutated)
109116
*/
110117
export function resolveApiTargetsToAllowedDomains(
@@ -117,38 +124,47 @@ export function resolveApiTargetsToAllowedDomains(
117124
},
118125
allowedDomains: string[],
119126
env: Record<string, string | undefined> = process.env,
120-
debug: (msg: string) => void = () => {}
127+
debug: (msg: string) => void = () => {},
128+
sensitiveAllowedDomains?: string[],
129+
openaiEndpointOverride?: string,
121130
): string[] {
122-
const apiTargets: string[] = [];
131+
const apiTargets: Array<{ value: string; sensitive?: boolean }> = [];
123132

124133
if (options.copilotApiTarget) {
125-
apiTargets.push(options.copilotApiTarget);
134+
apiTargets.push({ value: options.copilotApiTarget });
126135
} else if (env['COPILOT_API_TARGET']) {
127-
apiTargets.push(env['COPILOT_API_TARGET']);
136+
apiTargets.push({ value: env['COPILOT_API_TARGET'] });
128137
}
129138

130139
if (options.openaiApiTarget) {
131-
apiTargets.push(options.openaiApiTarget);
140+
apiTargets.push({ value: options.openaiApiTarget });
132141
} else if (env['OPENAI_API_TARGET']) {
133-
apiTargets.push(env['OPENAI_API_TARGET']);
142+
apiTargets.push({ value: env['OPENAI_API_TARGET'] });
143+
} else {
144+
// Prefer the pre-resolved override (from additionalEnv / envFile / process.env),
145+
// falling back to the raw env lookup for backward-compat callers that don't pass it.
146+
const endpointOverride = openaiEndpointOverride ?? env['OPENAI_ENDPOINT_OVERRIDE'];
147+
if (endpointOverride) {
148+
apiTargets.push({ value: endpointOverride, sensitive: true });
149+
}
134150
}
135151

136152
if (options.anthropicApiTarget) {
137-
apiTargets.push(options.anthropicApiTarget);
153+
apiTargets.push({ value: options.anthropicApiTarget });
138154
} else if (env['ANTHROPIC_API_TARGET']) {
139-
apiTargets.push(env['ANTHROPIC_API_TARGET']);
155+
apiTargets.push({ value: env['ANTHROPIC_API_TARGET'] });
140156
}
141157

142158
if (options.geminiApiTarget) {
143-
apiTargets.push(options.geminiApiTarget);
159+
apiTargets.push({ value: options.geminiApiTarget });
144160
} else if (env['GEMINI_API_TARGET']) {
145-
apiTargets.push(env['GEMINI_API_TARGET']);
161+
apiTargets.push({ value: env['GEMINI_API_TARGET'] });
146162
}
147163

148164
if (options.vertexApiTarget) {
149-
apiTargets.push(options.vertexApiTarget);
165+
apiTargets.push({ value: options.vertexApiTarget });
150166
} else if (env['VERTEX_API_TARGET']) {
151-
apiTargets.push(env['VERTEX_API_TARGET']);
167+
apiTargets.push({ value: env['VERTEX_API_TARGET'] });
152168
}
153169

154170
// Auto-populate GHEC domains when GITHUB_SERVER_URL points to a *.ghe.com tenant
@@ -176,10 +192,12 @@ export function resolveApiTargetsToAllowedDomains(
176192
// Merge API target values into the allowedDomains list so that later checks/logs about
177193
// "no allowed domains" see the final, expanded allowlist.
178194
// API targets may be provided as full URLs; only the hostname is relevant to Squid allowlisting.
195+
type NormalizedApiTarget = { hostname: string; scheme: 'http' | 'https'; sensitive?: boolean };
179196
const normalizedApiTargets = apiTargets
180-
.map((t) => (typeof t === 'string' ? t.trim() : ''))
181-
.filter((t) => t.length > 0)
182-
.map((raw) => {
197+
.map(({ value, sensitive }) => ({ value: (typeof value === 'string' ? value.trim() : ''), sensitive }))
198+
.filter(({ value }) => value.length > 0)
199+
.flatMap(({ value, sensitive }): NormalizedApiTarget[] => {
200+
const raw = value;
183201
const hasScheme = /^[a-zA-Z][a-zA-Z\d+\-.]*:\/\//.test(raw);
184202
const candidate = hasScheme ? raw : `https://${raw}`;
185203

@@ -189,22 +207,35 @@ export function resolveApiTargetsToAllowedDomains(
189207
} catch {
190208
// Let domain-validation surface a clear error later.
191209
}
192-
if (!hostname) return null;
210+
if (!hostname) return [];
193211

194212
const scheme: 'http' | 'https' = /^http:\/\//i.test(raw) ? 'http' : 'https';
195-
return { hostname, scheme } as const;
196-
})
197-
.filter((t): t is { hostname: string; scheme: 'http' | 'https' } => t !== null);
213+
return [{ hostname, scheme, sensitive }];
214+
});
198215

199216
if (normalizedApiTargets.length > 0) {
200-
for (const { hostname, scheme } of normalizedApiTargets) {
217+
for (const { hostname, scheme, sensitive } of normalizedApiTargets) {
201218
const urlEntry = `${scheme}://${hostname}`;
202-
if (!allowedDomains.includes(urlEntry)) {
203-
allowedDomains.push(urlEntry);
204-
debug(`Automatically added API target to allowlist: ${urlEntry}`);
219+
// Route sensitive (secret-derived) entries to sensitiveAllowedDomains when the
220+
// caller supplies that array so the value never appears in allowedDomains (which
221+
// is logged and written to the audit artifact). Fall back to allowedDomains for
222+
// callers that don't supply the array (backward compatibility).
223+
const targetList = (sensitive && sensitiveAllowedDomains) ? sensitiveAllowedDomains : allowedDomains;
224+
if (!targetList.includes(urlEntry)) {
225+
targetList.push(urlEntry);
226+
if (!sensitive) {
227+
debug(`Automatically added API target to allowlist: ${urlEntry}`);
228+
}
205229
}
206230
}
207-
debug(`Auto-added API target hostnames to allowed domains: ${normalizedApiTargets.map(t => t.hostname).join(', ')}`);
231+
232+
const nonSensitiveHosts = normalizedApiTargets.filter(t => !t.sensitive).map(t => t.hostname);
233+
if (nonSensitiveHosts.length > 0) {
234+
debug(`Auto-added API target hostnames to allowed domains: ${nonSensitiveHosts.join(', ')}`);
235+
}
236+
if (normalizedApiTargets.some(t => t.sensitive)) {
237+
debug('Auto-added OpenAI endpoint override host to allowed domains');
238+
}
208239
}
209240

210241
return allowedDomains;

src/commands/build-config.ts

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@ interface BuildConfigInputs {
4444
agentCommand: string;
4545
logLevel: LogLevel;
4646
allowedDomains: string[];
47+
sensitiveAllowedDomains?: string[];
4748
blockedDomains: string[];
4849
localhostDetected: boolean;
4950
additionalEnv: Record<string, string>;
@@ -81,6 +82,7 @@ export function buildConfig(inputs: BuildConfigInputs): WrapperConfig {
8182
agentCommand,
8283
logLevel,
8384
allowedDomains,
85+
sensitiveAllowedDomains = [],
8486
blockedDomains,
8587
localhostDetected,
8688
additionalEnv,
@@ -116,6 +118,7 @@ export function buildConfig(inputs: BuildConfigInputs): WrapperConfig {
116118

117119
return {
118120
allowedDomains,
121+
sensitiveAllowedDomains: sensitiveAllowedDomains.length > 0 ? sensitiveAllowedDomains : undefined,
119122
blockedDomains: blockedDomains.length > 0 ? blockedDomains : undefined,
120123
agentCommand,
121124
logLevel,

src/commands/main-action.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,8 @@ const SENSITIVE_CONFIG_KEYS = new Set([
4848
'geminiApiKey',
4949
'googleApiKey',
5050
'githubToken',
51+
// Secret-derived allowlist entries must not appear in logs or the audit artifact.
52+
'sensitiveAllowedDomains',
5153
]);
5254

5355
function redactConfigForLogging(config: WrapperConfig): Record<string, unknown> {

src/commands/preflight.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ jest.mock('../domain-validation');
1818
jest.mock('../option-parsers');
1919
jest.mock('../copilot-api-resolver');
2020
jest.mock('../api-proxy-config');
21+
jest.mock('../github-env', () => ({
22+
readEnvFile: jest.fn().mockReturnValue({}),
23+
}));
2124

2225
import { logger } from '../logger';
2326
import * as configFile from '../config-file';
@@ -29,6 +32,7 @@ import * as domainValidation from '../domain-validation';
2932
import * as optionParsers from '../option-parsers';
3033
import * as copilotResolver from '../copilot-api-resolver';
3134
import * as apiProxyConfig from '../api-proxy-config';
35+
import * as githubEnv from '../github-env';
3236

3337
const mockedLogger = logger as jest.Mocked<typeof logger>;
3438
const mockedConfigFile = configFile as jest.Mocked<typeof configFile>;
@@ -40,6 +44,7 @@ const mockedDomainValidation = domainValidation as jest.Mocked<typeof domainVali
4044
const mockedOptionParsers = optionParsers as jest.Mocked<typeof optionParsers>;
4145
const mockedCopilotResolver = copilotResolver as jest.Mocked<typeof copilotResolver>;
4246
const mockedApiProxyConfig = apiProxyConfig as jest.Mocked<typeof apiProxyConfig>;
47+
const mockedGithubEnv = githubEnv as jest.Mocked<typeof githubEnv>;
4348

4449
describe('applyConfigFilePrecedence', () => {
4550
let processExitSpy: jest.SpyInstance;
@@ -137,6 +142,7 @@ describe('resolveAllowedDomains', () => {
137142
copilotApiBasePath: undefined,
138143
});
139144
mockedApiProxyConfig.resolveApiTargetsToAllowedDomains.mockReturnValue([]);
145+
mockedGithubEnv.readEnvFile.mockReturnValue({});
140146
});
141147

142148
afterEach(() => {
@@ -370,6 +376,51 @@ describe('resolveAllowedDomains', () => {
370376
});
371377
expect(result.allowedDomains).toContain('awmg-cli-proxy');
372378
});
379+
380+
it('always returns sensitiveAllowedDomains in the result', () => {
381+
const result = resolveAllowedDomains({});
382+
expect(result).toHaveProperty('sensitiveAllowedDomains');
383+
expect(Array.isArray(result.sensitiveAllowedDomains)).toBe(true);
384+
});
385+
386+
it('passes OPENAI_ENDPOINT_OVERRIDE from additionalEnv to resolveApiTargetsToAllowedDomains', () => {
387+
resolveAllowedDomains({
388+
additionalEnv: { OPENAI_ENDPOINT_OVERRIDE: 'https://additional-env.example.com' },
389+
});
390+
expect(mockedApiProxyConfig.resolveApiTargetsToAllowedDomains).toHaveBeenCalledWith(
391+
expect.any(Object),
392+
expect.any(Array),
393+
expect.any(Object),
394+
expect.any(Function),
395+
expect.any(Array),
396+
'https://additional-env.example.com',
397+
);
398+
});
399+
400+
it('passes OPENAI_ENDPOINT_OVERRIDE from envFile to resolveApiTargetsToAllowedDomains', () => {
401+
mockedGithubEnv.readEnvFile.mockReturnValue({ OPENAI_ENDPOINT_OVERRIDE: 'https://envfile.example.com' });
402+
resolveAllowedDomains({ envFile: '/path/to/.env' });
403+
expect(mockedApiProxyConfig.resolveApiTargetsToAllowedDomains).toHaveBeenCalledWith(
404+
expect.any(Object),
405+
expect.any(Array),
406+
expect.any(Object),
407+
expect.any(Function),
408+
expect.any(Array),
409+
'https://envfile.example.com',
410+
);
411+
});
412+
413+
it('passes undefined openaiEndpointOverride when not set in any source', () => {
414+
resolveAllowedDomains({});
415+
expect(mockedApiProxyConfig.resolveApiTargetsToAllowedDomains).toHaveBeenCalledWith(
416+
expect.any(Object),
417+
expect.any(Array),
418+
expect.any(Object),
419+
expect.any(Function),
420+
expect.any(Array),
421+
undefined,
422+
);
423+
});
373424
});
374425

375426
describe('parseDomainOptions', () => {

src/commands/preflight.ts

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { processLocalhostKeyword } from '../option-parsers';
99
import { resolveCopilotApiRouting } from '../copilot-api-resolver';
1010
import { resolveApiTargetsToAllowedDomains } from '../api-proxy-config';
1111
import { resolveTopologyPeerHosts } from '../topology-peers';
12+
import { readEnvFile } from '../github-env';
1213

1314
/**
1415
* Resolves the Commander option-value source for a given option name.
@@ -21,6 +22,7 @@ type OptionSourceResolver = (optionName: string) => string | undefined;
2122
*/
2223
interface AllowedDomainsResult {
2324
allowedDomains: string[];
25+
sensitiveAllowedDomains: string[];
2426
localhostResult: ReturnType<typeof processLocalhostKeyword>;
2527
resolvedCopilotApiTarget: string | undefined;
2628
resolvedCopilotApiBasePath: string | undefined;
@@ -162,9 +164,21 @@ export function resolveAllowedDomains(options: Record<string, unknown>): Allowed
162164
process.env
163165
);
164166

165-
// Automatically add API target values to allowlist when specified
166-
// This ensures that when engine.api-target is set in GitHub Agentic Workflows,
167-
// the target domain is automatically accessible through the firewall
167+
// Resolve OPENAI_ENDPOINT_OVERRIDE from all config sources so that values
168+
// supplied via --env or --env-file reach the allowlist (not just process.env).
169+
// Priority matches getConfigEnvValue: additionalEnv > envFile > process.env.
170+
const additionalEnv = options.additionalEnv as Record<string, string> | undefined;
171+
const envFilePath = options.envFile as string | undefined;
172+
const openaiEndpointOverride: string | undefined = (
173+
additionalEnv?.['OPENAI_ENDPOINT_OVERRIDE']
174+
?? (envFilePath ? readEnvFile(envFilePath)['OPENAI_ENDPOINT_OVERRIDE'] : undefined)
175+
?? process.env['OPENAI_ENDPOINT_OVERRIDE']
176+
) || undefined;
177+
178+
// Automatically add API target values to allowlist when specified.
179+
// Secret-derived entries (OPENAI_ENDPOINT_OVERRIDE) go into sensitiveAllowedDomains
180+
// so they are never logged or included in the audit config artifact.
181+
const sensitiveAllowedDomains: string[] = [];
168182
resolveApiTargetsToAllowedDomains(
169183
{
170184
copilotApiTarget: resolvedCopilotApiTarget,
@@ -174,7 +188,9 @@ export function resolveAllowedDomains(options: Record<string, unknown>): Allowed
174188
},
175189
allowedDomains,
176190
process.env,
177-
logger.debug.bind(logger)
191+
logger.debug.bind(logger),
192+
sensitiveAllowedDomains,
193+
openaiEndpointOverride,
178194
);
179195

180196
// In network-isolation (topology) mode, automatically add trusted topology
@@ -203,7 +219,7 @@ export function resolveAllowedDomains(options: Record<string, unknown>): Allowed
203219

204220
validateAllowedDomains(allowedDomains);
205221

206-
return { allowedDomains, localhostResult, resolvedCopilotApiTarget, resolvedCopilotApiBasePath };
222+
return { allowedDomains, sensitiveAllowedDomains, localhostResult, resolvedCopilotApiTarget, resolvedCopilotApiBasePath };
207223
}
208224

209225
/**

0 commit comments

Comments
 (0)