Skip to content

feat(mcp): refresh Azure identity for long-running tasks - #2097

Open
jamesadevine wants to merge 29 commits into
mainfrom
feat/cross-org-repository-writes
Open

feat(mcp): refresh Azure identity for long-running tasks#2097
jamesadevine wants to merge 29 commits into
mainfrom
feat/cross-org-repository-writes

Conversation

@jamesadevine

@jamesadevine jamesadevine commented Sep 3, 2026

Copy link
Copy Markdown
Collaborator

Summary

Add first-class, renewable Azure workload-identity authentication for user-defined containerized stdio MCP servers. Authors name an ARM workload-identity service connection; the compiler owns assertion acquisition, renewal, projection into the MCP container, and lifecycle cleanup.

This addresses the lifetime gap in #1964: an assertion staged once at job start can expire before an agent makes its first Azure tool call, or before a later access-token acquisition. Authors should not need to build their own credential-refresh daemon to use an Azure Identity-enabled MCP server.

Closes #1964.

Authoring

For an image containing an Azure Identity-enabled stdio MCP server:

mcp-servers:
  azure-tool:
    container: your-registry/azure-mcp:tag
    azure-auth:
      service-connection: my-arm-workload-identity-connection
      # Optional; this is the default:
      mount-path: /var/run/ado-aw/azure

The compiler supplies AZURE_CLIENT_ID, AZURE_TENANT_ID, and AZURE_FEDERATED_TOKEN_FILE to the target MCP container. Authors do not stage assertions or supply the job credential themselves.

The projected token is a federated assertion, not an Azure resource access token. The MCP server's Azure Identity credential implementation remains responsible for consuming it and exchanging it with Entra for the access token required by the requested resource.

Runtime flow

Trusted AzureCLI@3 setup
  initial assertion + job OIDC request material
                  |
            one-shot FIFO
                  v
Trusted refresh sidecar ----> Azure DevOps job OIDC endpoint
                  |
       atomic assertion-file replacement
                  v
Read-only token-directory mount in the intended stdio MCP
                  |
       Azure Identity / Entra exchange
                  v
          Azure resource access
  1. A typed AzureCLI@3 task obtains the initial assertion and workload-identity service-connection metadata. Bootstrap remains in one authenticated task because those values are scoped to its process.
  2. The task streams the initial assertion, System.AccessToken, System.OidcRequestUri, and service-connection ID to the trusted sidecar. The job credential is not put in Docker arguments or container environment variables.
  3. The sidecar publishes the initial assertion before signaling readiness. It schedules renewal at JWT expiry minus 60 seconds and atomically replaces the token file.
  4. MCPG mounts the directory, not a single token-file inode, read-only into the configured MCP container. An already-running consumer therefore sees subsequent replacements.
  5. Transient renewal failures are retried with capped exponential backoff while the current assertion remains valid. Requests support timeout and shutdown cancellation. If no valid assertion remains, the refresher reports an unhealthy state and exits; this is not a claim that it synchronously terminates the agent.
  6. Always-running cleanup stops the per-server refresher and removes its scoped host directory.

Renewal is bounded by the running job's ability to request new assertions; this does not extend the lifetime of the underlying job credential.

Credential custody and isolation

Material Intended holders / exposure
System.AccessToken Trusted AzureCLI setup and refresher memory; not the AWF agent, MCPG, or target MCP container
Federated assertion Refresher and its token file; only the token directory is projected into the intended MCP
Client/tenant IDs Explicit runtime bindings into the target MCP; compiler-owned intermediate variables are excluded from Agent and Detection environment passthrough
Sidecar bootstrap/status files Private per-server directory; not part of the target MCP's token-only mount

The host auth root and per-server directory remain 0700. Only token.d is 0755, with assertion files 0644, so the intended MCP can run under a different non-root UID. Unrelated host UIDs cannot traverse the private parent directories.

Permissions are not the agent-isolation boundary: they do not protect against host root or another process with the runner's UID. The assertion directory must remain outside AWF's exposed filesystem paths, including runner /tmp, the mounted workspace, and their chroot aliases. The new real-AWF regression exercises that separation.

Structural safeguards

  • Compiler-owned WIF and ado-proxy launches use a separate typed DockerRun model, lowered into registered shell fragments. The existing Docker@2 task model remains specific to the Azure Pipelines task rather than being overloaded with unsupported runtime-container operations.
  • Mount and tmpfs destinations use ContainerAbsolutePath; tmpfs options reject ambiguous colon delimiters. Shell quoting alone is not treated as protection against Docker's own mount-spec parsing.
  • The invocation model rejects duplicate singleton settings/mount destinations and known credential variables in arguments.
  • Shell fragment metadata declares consumed variables so registry-wide shell linting retains visibility into generated fragments.
  • Typed task environments and explicit MCP pipeline-variable sources replace ambient wiring. Internal client/tenant placeholders are resolved in MCPG configuration at runtime.
  • Unsupported HTTP/non-container configurations, reserved Azure environment overrides, overlapping mounts, and raw Docker environment/mount flags (including --env-file) are rejected. Bootstrap also rejects missing WIF metadata and failed readiness.

Why not reuse MCPG's GitHub OIDC authentication?

MCPG's auth.type: github-oidc is an HTTP-backend feature. Its provider caches tokens per audience and refreshes on demand when an outgoing request needs a token near expiry, then injects that token into an HTTP authorization header.

This feature serves a different consumer: a stdio MCP process that uses an Azure federated-token file. Reusing the existing upstream feature would require both an Azure issuer implementation and token-file delivery support; changing the request endpoint alone would not address that transport boundary.

We use the same 60-second renewal margin, but a background sidecar rather than request-time HTTP injection. The sidecar also keeps the ADO job credential outside MCPG.

Comparison reference: github/gh-aw-mcpg v0.4.1, internal/oidc/provider.go, internal/mcp/http_transport_client.go, and the HTTP-only auth contract in docs/CONFIGURATION.md.

Validation and remaining evidence gap

Layer Coverage
Rust compiler and typed IR Path/delimiter rejection, generated AzureCLI/stdio mount wiring, credential placement, Agent/Detection exclusions, and provider-auth coexistence
Generated shell Registry-wide shellcheck, generated-shell guard, and shellcheck of compiled pipeline bodies
Refresher unit tests Expiry scheduling/fallback, readiness ordering, atomic publication, retries, invalid responses, timeout/shutdown cancellation, and redacted diagnostics
Credential-free Linux Docker regression Compiler-generated setup and mount plus the real bundled refresher with a fake clock/provider; one persistent different-UID consumer observes rotation, cannot write or read private sibling files, and an unrelated UID is denied through the host path
Real pinned-AWF regression Compiled mount/environment settings with the AI command replaced by a probe; verifies normal and /host paths, a workspace symlink, and internal identity-variable exclusion

The Docker and AWF regressions run in the existing ado-script Linux CI job. No new ADO smoke lane, service connection, app registration, or personal credential is needed. The handwritten runtime fixture is ESM (.mjs), not a committed generated bundle.

At head 74079737, both the Rust workflow and the ado-script workflow passed, including the two credential-free integration regressions.

Still unverified: live Azure issuance and Entra exchange after the genuine initial assertion expires. The manual pipeline in tests/azure-wif-refresh-e2e/azure-pipelines.yml is included for that purpose. It requires an authorized ARM WIF service connection, waits past the original assertion's actual expiry, checks replacement/expiry advancement, and performs a fresh direct Entra exchange rather than relying on an Azure CLI token cache.

The credential-free tests do not substitute for that result. The AWF probe also omits MCPG network attachment because it has no MCPG service; it is evidence for filesystem/environment isolation, not a complete credentialed MCP session.

Targeted commands

cargo test --bin ado-aw azure_auth
cargo test --bin ado-aw container_invocation
cargo test --test compiler_tests test_mcpg_container_azure_auth_emits_refresher_and_rotating_token_mount
cargo test --test generated_shell_guard --test bash_lint_tests
cargo clippy --bin ado-aw --tests -- -D warnings

cd scripts/ado-script
npx vitest run src/azure-wif-refresh
npm run typecheck
npm run build:azure-wif-refresh

# Linux, with the compiler already built and Docker available:
ADO_AW_TEST_DOCKER=1 ADO_AW_TEST_AWF=1 \
  npx vitest run -c vitest.config.smoke.ts test/azure-wif-isolation.test.ts

Scope

Azure assertion refresh is opt-in via azure-auth. This PR does not add HTTP MCP Azure authentication, renew the first-party ado-proxy's read credential, manage Azure resource access tokens on behalf of MCP servers, or claim live federation support has already been proven for every Azure Identity SDK/server combination.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Refresh cached Azure DevOps clients when short-lived tokens rotate and add direct coverage for default auth, PR repository selectors, and branch/tag E2E scenario behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Verify a failed required cross-organization target is retained while later repository preparation continues.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Use a closed token-variable enum, restrict repository identity folding to ASCII, update proxy credential docs, and complete failure-path coverage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Pin the complete repos entry emitted alongside expanded write permissions in executor E2E sources.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Assert repository target names with more than project/repository segments fail closed.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3c8e5f36-958b-48ea-99cd-d7c14b5eb8da
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: d6d08ba7-33cd-4c10-8c9e-69f19c8e3132
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

@jamesadevine

Copy link
Copy Markdown
Collaborator Author

/review

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeScript review — scripts/ado-script/src/azure-wif-refresh/

Reviewed the new sidecar script (773 lines) plus its test suite. ts-critic ran in the background but returned no findings; my own pass agrees there is nothing merge-blocking here.

Highlights that make this solid:

  • systemAccessToken/initialIdToken never reach a log line or thrown Error — all report(...) calls use static, sanitized strings.
  • Network calls go through requestWithTimeout with an AbortSignal race plus a hard timeout, and retries back off correctly against the assertion's actual expiry.
  • writeAtomic uses a same-directory temp file + rename, with cleanup of the handle and temp file on every failure path.
  • parseMaterial allow-lists fields and validates every value's shape (including a GUID regex for serviceConnectionId) before anything touches the state machine.

One minor, non-blocking nit worth a look before merge: in onData (readOneJsonDocument), Buffer.byteLength(buffer) is recomputed against the entire accumulated buffer on every chunk to enforce MAX_MATERIAL_BYTES, which is O(n2) for many small chunks. Given input is bounded to 1MB from a trusted host, this is a performance nit rather than a defect — track a running byte count instead if this ever gets exercised with high chunk counts.

No inline comments needed.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 40.9 AIC · ⌖ 10.5 AIC · ⊞ 11.6K
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security review — no regressions found

Reviewed the new Azure workload-identity refresh sidecar (azure-wif-refresh.js), its Rust wiring (agentic_pipeline.rs, mcpg.rs, common.rs, ado_bundle.rs, azure_cli.rs, types.rs, secure.rs), and doc updates.

This is a well-defended addition to the credential-isolation model, not a weakening of it:

  • System.AccessToken and the initial idToken reach the sidecar only via a one-shot, mode-600 FIFO (mkfifo -m 600), never as a container env var, CLI argument, or file — confirmed in both the generated bash (START_AZURE_WIF_REFRESH) and the TypeScript parseMaterial/writeAtomic paths.
  • The refresher container is hardened (--cap-drop ALL, --security-opt no-new-privileges, --read-only, non-root --user, --pids-limit 64) and only the token-only subdirectory is mounted read-only into the target MCP container — the sidecar's own status/ready/material channels stay outside that mount.
  • New ContainerAbsolutePath validator (secure.rs) rejects traversal, : mount separators, shell metacharacters, and even ##vso[ injection in mount-path — good defense-in-depth for a value ultimately used in a Docker mount flag.
  • common.rs validation blocks user args/env/mounts from colliding with or overriding the compiler-owned AZURE_CLIENT_ID/AZURE_TENANT_ID/AZURE_FEDERATED_TOKEN_FILE env vars or the auth mount path, and azure-auth is correctly restricted to containerized stdio servers only (rejected for safeoutputs, extension-owned servers, and HTTP servers).
  • mcpg.rs's new bind_internal_pipeline_variable/required_names machinery keeps the compiler-internal client/tenant-id variables out of user-writable MCPG_ENV_NAMES while still enforcing they're non-empty and not an unresolved ADO macro ($() before MCPG starts — a good fail-closed check against silent misconfiguration.
  • The sidecar's report() logging only emits ErrorCategory strings, never token/material content, and errorCategory() derives categories from HTTP status/error codes without echoing response bodies.
  • Cleanup (STOP_AZURE_WIF_REFRESH) runs unconditionally (Condition::Always) and removes the private auth directory even on partial-startup failure.

No injection, token-scope, or path-traversal issues found in this diff. Nice attention to the /tmp chroot-exposure trap called out in the docs update — the credential directory correctly stays under a private subpath of Agent.TempDirectory, distinct from the shared runner /tmp that AWF exposes to the agent.

🔒 Security review by PR Security Reviewer · auto · 67.1 AIC · ⌖ 2.03 AIC · ⊞ 11.7K
Comment /review to run again

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall the test coverage for this PR is strong — the mcpg.rs rejection paths (HTTP servers, env/mount collisions, unsafe mount paths) and the runRefresher state-machine tests are thorough and behavior-focused rather than implementation-mirroring.

Three gaps worth closing before merge:

  1. readOneJsonDocument (chunked stdin JSON parser with brace-depth/string-escape tracking) has zero direct test coverage.
  2. errorCategory classifies 7 categories but only throttled/invalid-response/generic-server paths are exercised indirectly through runRefreshertimeout, client, network, and unknown are untested.
  3. The new ContainerAbsolutePath validator's security-critical checks (##vso[ injection guard, control characters \0/\n/\r) aren't asserted in container_absolute_path_rules, only path-traversal and colon cases are.

None of these are blocking — the core refresh/rotation logic is well tested — but they're the highest-value additions if there's time before merge.

🧪 Test quality analysis by Test Quality Sentinel · auto · 67.8 AIC · ⌖ 2.47 AIC · ⊞ 11.3K
Comment /review to run again

@github-actions github-actions Bot mentioned this pull request Sep 3, 2026

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rust review — no merge-blocking defects found

Reviewed the Rust portions of this PR (src/compile/ado_bundle.rs, agentic_pipeline.rs, common.rs, extensions/ado_script.rs, extensions/mod.rs, ir/tasks/azure_cli.rs, mcpg.rs, types.rs, secure.rs, tests/compiler_tests.rs). Note: the rust-critic sub-agent I launched did not receive the diff content due to a shell-substitution issue on my side, so this review reflects only my own manual pass — flagging for transparency rather than treating it as a clean bill from two independent reviewers.

Positives:

  • McpgLaunchEnvironment::bindings is a BTreeMap<McpgEnvName, _>, so names()/required_names() iteration order is deterministic — important since it feeds generated YAML env lists.
  • The ContainerAbsolutePath validator correctly rejects traversal, :/$/backtick, and empty/./.. segments before the path is ever used to build a Docker mount destination.
  • Reserved-env and mount-collision checks in validate_stdio_mcp correctly bail before azure-auth credentials could be shadowed or a container mount could collide with the token mount.
  • SYSTEM_ACCESSTOKEN is injected via EnvValue::secret("System.AccessToken") rather than a literal, keeping it out of the rendered YAML/log surface, consistent with the project's credential-isolation model.

One non-blocking hardening note:
The Docker-flag detector in validate_stdio_mcp (common.rs ~L3269-3277) matches only unbundled short/long flags (-e, --env, -v, --volume, --mount, --volumes-from, and their =value forms) plus the -eX/-vX bundled-value case. It does not reject a bundled multi-short-flag form such as -itve where -e/-v is not the first character. I could not confirm whether the downstream runtime.extra_args consumer parses args in a way that would treat this as equivalent to -e/-v (most likely it passes them straight through to docker run's own arg vector, which would not be vulnerable), so this is flagged only for defense-in-depth review since it's a security-relevant guard, not because I have a working bypass.

Nothing else in the diff rises to merge-blocking: error handling uses anyhow with context consistently, no new unwrap/expect on user-reachable paths, and the new typed AzureCliV3/ContainerAbsolutePath/McpgLaunchEnvironment additions all follow existing patterns in this codebase.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 98.1 AIC · ⌖ 12.7 AIC · ⊞ 11.6K
Comment /review to run again

Address review feedback by adding a fluent TaskStep environment builder while keeping Azure WIF startup atomic inside its authenticated AzureCLI task.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI lite review requested due to automatic review settings September 8, 2026 08:57

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

There are correctness issues in the new E2E pipeline script (top-level await) and in the MCPG/azure-auth runtime substitution path that can break auth at runtime.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 3 High severity · 1 Medium severity · 1 Low severity

New issues introduced by this change (2)
Severity Finding
High severity src/​compile/​agentic_pipeline.rsmcpg-config.json uses ${VAR} placeholders for Azure client/tenant IDs (from azure-auth), but…
High severity tests/​azure-wif-refresh-e2e/​azure-pipelines.yml — This inline node -e script uses top-level await, which will fail under Node's default CommonJS…
Pre-existing issues (3)
Severity Finding
High severity src/​compile/​common.rs — When azure-auth is enabled, mcp-servers.&lt;name&gt;.args is supposed to reject Docker env/mount… View comment
Medium severity scripts/​ado-script/​src/​azure-wif-refresh/​index.ts — The fallback &quot;code&quot; in error ? &quot;filesystem&quot; : errorCategory(error) will misclassify common… View comment
Low severity tests/​compiler_tests.rs — This assertion is brittle about YAML quoting (&#39;true&#39; vs true). Since the exact quoting is an… View comment
Suppressed comments (1)

scripts/ado-script/src/azure-wif-refresh/index.ts:739

  • The fatal-error handler forces any error with a code property into the filesystem category, which will misclassify network/undici errors like ENOTFOUND/ETIMEDOUT (they also have code). Prefer errorCategory(error) and only fall back to filesystem when the category is still unknown.
    const category =
      error && typeof error === "object" && "code" in error
        ? "filesystem"
        : errorCategory(error);

Comment thread src/compile/agentic_pipeline.rs Outdated
Comment thread tests/azure-wif-refresh-e2e/azure-pipelines.yml Outdated
Block Docker env-file overrides, substitute internal MCPG identity placeholders, preserve network error categories, make the E2E exchange script module-compatible, and remove a quoting-sensitive assertion.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944
Copilot AI review requested due to automatic review settings September 8, 2026 09:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The new container mount-path validator is currently less strict than its stated security intent, and the refresher step relies on implicit SYSTEM_OIDCREQUESTURI propagation instead of explicitly mapping it into the task env.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity src/​secure.rsContainerAbsolutePath claims to reject control characters and shell metacharacters, but the…
Issues resolved since last review (5)
Severity Finding
High severity tests/​azure-wif-refresh-e2e/​azure-pipelines.yml — This inline node -e script uses top-level await, which will fail under Node's default CommonJS… View resolved comment
High severity src/​compile/​agentic_pipeline.rsmcpg-config.json uses ${VAR} placeholders for Azure client/tenant IDs (from azure-auth), but… View resolved comment
Low severity tests/​compiler_tests.rs — This assertion is brittle about YAML quoting (&#39;true&#39; vs true). Since the exact quoting is an… View resolved comment
Medium severity scripts/​ado-script/​src/​azure-wif-refresh/​index.ts — The fallback &quot;code&quot; in error ? &quot;filesystem&quot; : errorCategory(error) will misclassify common… View resolved comment
High severity src/​compile/​common.rs — When azure-auth is enabled, mcp-servers.&lt;name&gt;.args is supposed to reject Docker env/mount… View resolved comment
Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/compile/agentic_pipeline.rs:5148

  • The Azure WIF refresher script requires SYSTEM_OIDCREQUESTURI, but the generated AzureCLI@3 step only explicitly maps System.AccessToken. Relying on implicit env propagation for SYSTEM_OIDCREQUESTURI is brittle across agent pools/tasks; map $(System.OidcRequestUri) into the task env explicitly so the feature fails deterministically based on pipeline settings, not ambient step env.

Comment thread src/secure.rs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944
Copilot AI review requested due to automatic review settings September 8, 2026 10:02

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It introduces a new credential-refresh sidecar and MCPG launch-time substitution path that should receive final human security/ops review despite no specific defects found in this pass.

Review tier: Lite
Findings: 1 High severity

Pre-existing issues (1)
Severity Finding
High severity src/​secure.rsContainerAbsolutePath claims to reject control characters and shell metacharacters, but the… View comment

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944
Copilot AI review requested due to automatic review settings September 8, 2026 10:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The current implementation makes the rotated federated assertion potentially world-readable on the build host (directory mode 0755), and the new typed docker invocation IR should explicitly reject : in mount/tmpfs fields to prevent ambiguous argument construction.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity

New issues introduced by this change (1)
Severity Finding
High severity src/​compile/​agentic_pipeline.rstoken.d is chmodded to 0755, which makes the federated assertion file potentially world-readable…
Issues resolved since last review (1)
Severity Finding
High severity src/​secure.rsContainerAbsolutePath claims to reject control characters and shell metacharacters, but the… View resolved comment
Suppressed comments (3)

Previously missed (3) — in code that hasn't changed since the last review.

src/compile/container_invocation.rs:167

  • DockerMount destinations are interpolated into a source:dest:mode string. Because validate_literal() does not reject :, a destination containing : would produce an ambiguous mount spec (and could become an injection footgun for future callers). Explicitly reject : in mount destinations.
    src/compile/container_invocation.rs:196
  • DockerTmpfs renders as dest:options. Because validate_literal() allows :, a : in either field would break the generated --tmpfs argument. Reject : explicitly to keep the typed invocation IR robust for future uses.
    tests/azure-wif-refresh-e2e/azure-pipelines.yml:75
  • token.d is chmodded to 0755, which makes the refreshed federated assertion file potentially readable by other users/processes on the build host. Since the test itself reads the file as the current user, token.d can be 0700 to keep the assertion private without affecting the scenario.

Comment thread src/compile/agentic_pipeline.rs
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings September 8, 2026 12:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

It introduces at least one confirmed security hardening gap (mount-destination : injection) and one functional robustness issue (token dir permissions vs non-root MCP containers), plus a documentation guarantee that the pipeline does not currently enforce.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review tier: Lite
Findings: 1 High severity · 1 Medium severity

New issues introduced by this change (2)
Severity Finding
High severity src/​compile/​container_invocation.rs — Docker mount destinations are interpolated into the -v source:dest:mode spec; if destination
Medium severity src/​compile/​agentic_pipeline.rs — The azure-auth token directory is chmod 700. If the target MCP container runs as a non-root user…
Issues resolved since last review (1)
Severity Finding
High severity src/​compile/​agentic_pipeline.rstoken.d is chmodded to 0755, which makes the federated assertion file potentially world-readable… View resolved comment
Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

docs/network.md:154

  • This section claims the AWF agent receives no identity environment variables, but the refresher currently publishes client/tenant IDs via ##vso[task.setvariable], and the agent is launched with --env-all, so these variables will be present in the agent environment in practice. Either exclude these internal vars from AWF (--exclude-env) or adjust the documentation to avoid making a guarantee that the pipeline does not enforce.

Comment thread src/compile/container_invocation.rs Outdated
Comment thread src/compile/agentic_pipeline.rs
Address review feedback on Docker mount delimiters, non-root assertion readers, and internal identity passthrough. Add credential-free Docker and pinned AWF regressions to existing CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944
Copilot AI review requested due to automatic review settings September 8, 2026 13:05
Keep AWF home writes inside the disposable regression fixture and restore root-owned files without following symlinks before cleanup.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The change introduces a new credential-bearing runtime sidecar and modifies multiple security boundaries (AWF exclusions, MCPG launch/env substitution, container lifecycle), warranting final human review despite strong test coverage.

Review tier: Lite
Findings: None

Issues resolved since last review (2)
Severity Finding
Medium severity src/​compile/​agentic_pipeline.rs — The azure-auth token directory is chmod 700. If the target MCP container runs as a non-root user… View resolved comment
High severity src/​compile/​container_invocation.rs — Docker mount destinations are interpolated into the -v source:dest:mode spec; if destinationView resolved comment

Copilot AI review requested due to automatic review settings September 8, 2026 13:12

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

It introduces new security-sensitive credential-custody and long-lived sidecar/container orchestration paths that warrant final human security review.

Review tier: Lite
Findings: None

Replace the CommonJS fixture with an mjs module using static imports and update the Docker harness paths.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 09ad274e-3601-48b8-9dde-979d4a6ef944
Copilot AI review requested due to automatic review settings September 8, 2026 13:28

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The new typed container-invocation literal validator allows $( and known secret-name substrings, which undermines the “secrets must not reach generated YAML/argv” custody goal and should fail closed.

Review tier: Lite
Findings: None

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/compile/container_invocation.rs:126

  • ShellWord::literal / with_literal currently accept $( and credential-like substrings, which can be expanded by Azure DevOps macro substitution before Bash runs. That creates a footgun where a future compiler-owned container invocation could accidentally embed $(System.AccessToken) (or similar) into the generated YAML / process args. Since this module is intended for credential-custody-sensitive invocations, the literal validator should fail closed on $( and on known secret names (similar to Binding::text).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[agent-issue]: Gateway-managed, auto-refreshed Azure federated token file for user-defined MCP servers

2 participants