Redact owning resource's own secret env var in describe - #19248
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 19248Or
iex "& { $(irm https://raw.githubusercontent.com/microsoft/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 19248" |
github-actions
Bot
added
the
needs-area-label
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Fixes secret leakage from owning resources in backchannel snapshots.
Changes:
- Discovers secret parameters through recursive resource dependencies.
- Caches parameter instances while refreshing resolved values.
- Adds regression coverage for generated owning-resource secrets.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs |
Expands secret discovery and redaction. |
tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs |
Tests owning-resource environment redaction. |
Suppressed comments (1)
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1315
- This makes secret redaction fail open. If a callback succeeded while producing the resource environment but throws during this later discovery pass, its non-top-level secret parameters are omitted and the partial result is cached, so subsequent snapshots can expose those values in plaintext. Cancellation is swallowed the same way. At this confidentiality boundary, propagate discovery failures (and cancellation) rather than continuing with an incomplete redaction set, or conservatively redact the affected resource's environment.
catch (Exception ex)
{
logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name);
continue;
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
PR Testing Report — #19248PR Information
Artifact Version Verification
Changes Analyzed
Scenario — exact repro from #19241var pg = builder.AddPostgres("pg"); // auto-generates secret param pg-password (referenced, not in model)
pg.AddDatabase("appdb");
builder.AddContainer("consumer", "nginx")
.WithEnvironment("PG_CONN", pg.Resource.ConnectionStringExpression);
Results✅ PR fix works — owning resource's own env var is redacted
|
| Scenario | Status |
|---|---|
Owning pg.POSTGRES_PASSWORD redacted (batch) |
✅ Passed |
Owning pg.POSTGRES_PASSWORD redacted (--follow stream) |
✅ Passed |
resource.connectionString redacted |
✅ Passed |
Container has real non-empty secret (proves null = redaction) |
✅ Confirmed |
| CLI/hive version == PR head | ✅ Verified |
Dependent consumer.PG_CONN plaintext |
Overall Result
✅ PR VERIFIED for its stated scope — the owning resource's own generated-secret env var is now redacted in aspire describe (batch + follow), end-to-end with a real AddPostgres container.
PG_CONN), which this PR neither introduces nor claims to fix, but which is relevant to #19241's overall goal and worth a follow-up.
|
Yes!!! Thank you for doing this |
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Requesting changes because secret discovery can still fail open. The existing restart-cache thread and the callback re-evaluation issue below both need to be resolved before this is safe to merge.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Review details
Suppressed comments (2)
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1316
- This failure path is fail-open for secret redaction. If runtime environment evaluation already produced a hidden parameter value but dependency discovery later throws (for example, because this call re-invokes a stateful callback),
continueomits that parameter and line 1328 permanently caches the incomplete set; subsequent snapshots can then return the secret in plaintext. Redaction must either propagate the discovery failure or conservatively suppress environment values for the affected resource rather than caching a partial result.
catch (Exception ex)
{
logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name);
continue;
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1313
- This catch also intercepts
OperationCanceledExceptionraised from the supplied cancellation token, so a canceled snapshot/watch request can continue traversing resources instead of terminating. Exclude cancellation exceptions from the defensive callback handling.
catch (Exception ex)
- Files reviewed: 2/2 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
Retrying the failed CI jobs for this pull request from the CI run attempt. The rerun is being tracked in the rerun attempt. |
Mitch Denny (mitchdenny)
left a comment
Mitch Denny (mitchdenny)
left a comment
There was a problem hiding this comment.
The same-named secret redaction gap is fixed and the updated check suite is green.
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
Requesting changes again because both redaction threads remain unresolved on d460159. The same-name fix and test are covered by the current checks, but describe can still leak secrets on the first discovery pass or after a resource restart.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Suppressed comments (3)
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1337
- Every watch event reaches this loop, while
ResourceNotificationService.WatchAsyncemits one initial event per resource and every subsequent update. Running recursive discovery independently for every model resource makes each event perform up toRgraph traversals; the initial stream can therefore do quadratic-or-worse work and backlog its unbounded notification channel. Use the existing multi-rootResourceExtensions.GetDependenciesAsync(appModel.Resources, ...)overload once per secret-set refresh (top-level parameters are already collected above), or cache this set with restart-aware invalidation.
foreach (var resource in appModel.Resources)
{
IReadOnlySet<IResource> dependencies;
try
{
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1327
- The PR description says dependency-discovery exceptions are logged and skipped so a bad callback does not break
describe, but this rethrows, and the added failure test codifies that opposite behavior. Either update the PR description to document the intentional fail-closed availability tradeoff or implement the stated per-resource recovery without returning an under-redacted snapshot.
logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name);
throw;
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1281
- This private helper's remarks narrate the old implementation and issue history across 20 lines. Internal Aspire members should have concise documentation; keep only the enduring restart and fail-closed rationale rather than documenting the branch's evolution.
/// <remarks>
/// Enumerating only <c>appModel.Resources.OfType<ParameterResource>()</c> misses generated
/// parameters such as the password created by <c>AddPostgres("pg")</c>, which is referenced by the
/// owning resource but never added to the model. That gap let the owning resource's own environment
/// variable (e.g. <c>POSTGRES_PASSWORD</c>) leak the secret in plaintext even though the same value
Move the add-only secret parameter and value accumulators off the per-connection AuxiliaryBackchannelRpcTarget and into a new AppHost-scoped SecretRedactionHistory singleton shared by every connection. Each backchannel connection gets its own RPC target, so a client that connects after a secret's value is replaced would otherwise start with an empty redaction set and leak the previous value carried by a lagging snapshot. Sharing the history across connections for the life of the AppHost keeps every secret value ever observed redacted for later, independent connections. Adds a regression test proving a second, freshly constructed target redacts a value that only the first target ever observed. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6410f72-f73a-4be8-b069-7cc8e08a547a
b761b87 to
7f64ebb
Compare
|
Retargeted this PR from Also addressed the last open thread: the add-only redaction history is now AppHost-scoped via a |
Stale
Adam Ratzman (adamint)
left a comment
Adam Ratzman (adamint)
left a comment
There was a problem hiding this comment.
The intended owning-resource case works end-to-end through a live AppHost and real aspire describe, including restart/repoint and a new client. Two fail-open paths still remain, though: one-shot describe samples the redaction set before per-resource async work, and the history is observation-based so the first backchannel connection after A→B can miss a lagging A value. I left the batch race inline and followed up in the existing cold-history thread. Requesting changes because either path can still emit a plaintext secret.
…ssignment time Addresses review feedback on #19248 (two remaining concerns): 1. Resolve the secret redaction set per snapshot inside CreateResourceSnapshotFromEventAsync (after the MCP discovery await) instead of once per describe batch. Building a snapshot can block on MCP tool discovery for up to the discovery timeout, during which a parameter can resolve; a set computed once up front could miss a secret a later resource's snapshot already carries and leak it. 2. Record resolved secret values into the AppHost-scoped SecretRedactionHistory the moment ParameterProcessor assigns or replaces them, closing the cold-start residual where a value assigned (and possibly replaced) before the first backchannel connection was absent from the history and could leak from a lagging snapshot. Wired via an internal settable property + DI factory so the public ParameterProcessor constructor stays unchanged (backport-safe). Adds regression tests covering a secret resolved during another resource's MCP-discovery window, a secret replaced before the first connection, and assignment-time recording in ParameterProcessor. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Tests selector (audit mode)The full test matrix and all jobs still run in audit mode. The tests and jobs below are what selective CI would run under enforcement. 48 / 100 test projects · 4 jobs, from 13 changed files. Selected test projects (48 / 100)
Selected jobs (4)
How these were chosen — grouped by what changed
🔧 show 43
🧪 📦 affected project 🧪 🧪 Job reasons
Selection computed for commit |
Stale
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.
Suppressed comments (1)
src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs:1329
- This still performs a whole-model recursive dependency walk once per snapshot.
ResourceNotificationService.WatchAsyncemits one initial event per resource (ResourceNotificationService.cs:773-788), and the one-shot path also builds one snapshot per resource, so both startup paths now do Θ(N²) callback/dependency scans; every subsequent resource update is Θ(N). Avoid rediscovering the full model on every snapshot—for example, keep a generation-aware discovered-parameter cache that is invalidated when DCP forgets callback results, while continuing to snapshot the add-only resolved-value history per event.
// Compute the transitive dependency closure of every resource in a single multi-root walk. It shares
// one visited set across all roots, so each resource's (execution-cached) callbacks are read at most
// once. Discovering per resource instead would repeat the traversal for every resource and make the
// initial WatchAsync stream — which emits one event per resource — do quadratic work.
|
/backport release/13.5 |
Documents changes from microsoft/aspire#19248
|
Pull request created: #1499
|
|
📝 Documentation has been drafted in microsoft/aspire.dev#1499 targeting Added an
Note This draft PR needs human review before merging. |
Description
aspire describe --format jsonredacted a generated secret parameter (such as the password created byAddPostgres) when it flowed into a dependent resource, but still emitted the value in plaintext via the owning resource's own environment variable (for examplePOSTGRES_PASSWORD).The redaction added in #18089 only enumerated top-level
ParameterResourceinstances in the application model. Generated parameters created by helpers such asAddPostgres,AddRedis, andAddSqlServer(viaCreateDefaultPasswordParameter->CreateGeneratedParameter) are referenced by their owning resource but are never added to the model, so they were absent from the redaction set and leaked in plaintext.This change mirrors
ParameterProcessor's dependent-parameter discovery (a recursive dependency walk) so the redaction set matches the full set of secret values that can flow into any resource's environment — including the owning resource's own env vars. Discovery runs as a single multi-root walk over all resources (one shared visited set, so each resource is read at most once) and is peek-only: it reads only the callback results DCP has already cached and never invokes a callback. A running resource only appears in a snapshot after DCP has resolved and cached its environment/argument values, so peeking observes every secret a snapshot can expose, while never repopulating DCP's shared annotation cache or binding a canceled/faulted task to the describe client's cancellation token (which DCP would otherwise reuse on the resource's own execution path).The redaction set is accumulated add-only rather than recomputed per event, and is scoped to the AppHost (a
SecretRedactionHistorysingleton shared by every backchannel connection) rather than to a single connection. Eachdescribe/watchclient gets its own RPC target; scoping the history per connection would let a client that connects after a value changes start with an empty set and leak the previous value carried by a lagging snapshot. The history accumulates at two levels:ParameterResourceobjects). A restart can change which secret a resource references (DCP forgets and re-evaluates callbacks), and a laggingStopping/Startingsnapshot from the prior incarnation can still carry the previous value, so a secret parameter observed on an earlier pass keeps being re-resolved.WaitForValueTcsfor a new one (ParameterProcessor.SetParameterValue), so re-resolving a retained parameter later yields only the new value. An already-published or still-current snapshot can still carry the previous value, so every secret string ever resolved stays in the redaction set.This narrows but does not fully close a cold-start residual (a value assigned and reassigned before any connection ever observed it); in practice the always-on dashboard/CLI watch keeps a connection open from startup, so the history is populated continuously.
Peek-only discovery is implemented with internal-only API (no public API surface added):
ICallbackResourceAnnotation.TryGetCachedResultreads an already-cached callback result under the annotation lock, andResourceDependencyDiscoveryOptions.PeekCachedCallbackResultsOnlymakes the env/args/launch-tool gatherers read only results that completed successfully.Regression tests:
GetResourceSnapshotsAsync_RedactsSecretParameterReferencedByOwningResourceEnvironmentreferences a secret parameter that is not a top-level model resource and asserts the owning resource's own environment variable is redacted.RetainsPreviouslyReferencedSecret_AfterRestartRepointsResourcereproduces the restart generation-skew — it flips the referenced secret A->B, re-primes the cache the wayForgetCachedCallbackResults+ re-evaluation does, then publishes the still-present oldvalue-aand asserts it stays redacted (fails without the accumulator).RetainsPreviousSecretValue_AfterParameterValueIsReplacedcovers the runtime "Set parameter" path: it redactsvalue-a, swaps the same parameter'sWaitForValueTcstovalue-bwhile a lagging snapshot still carriesvalue-a, and assertsvalue-astays redacted (fails when only parameter objects, not resolved values, are accumulated).RetainsPreviousSecretValue_AcrossSeparateConnectionsproves the AppHost-scoped history: it observesvalue-aon one RPC target, replaces the value withvalue-b, then drives a second, independently constructed target (as a newly connected client would) and assertsvalue-astays redacted (fails when the history is per-connection; verified via a negative check).RedactsNewlyReferencedSecret_AfterRestartRepointsResourcecovers picking up the newly referenced secret after a restart.DoesNotInvokeUncachedResourceCallback_AndLeavesItEvaluableandDoesNotInvokeOrPoisonCallbackCache_WhenDescribeIsCanceledprove discovery never invokes or caches a callback (even under cancellation) and leaves it evaluable afterward.PeekCachedCallbackResultsOnly_OnlySeesCachedResultsAndNeverInvokesCallbackcovers the discovery primitive directly.This targets
main; a selective backport torelease/13.5will follow once merged.Fixes #19241
Checklist
<remarks />and<code />elements on your triple slash comments?