From dece9d8da72bd0f1fdae3b975148bf5757bb650f Mon Sep 17 00:00:00 2001 From: David Pine Date: Tue, 11 Aug 2026 14:39:42 -0500 Subject: [PATCH 1/8] Redact owning resource's own secret env var in `describe` `aspire describe --format json` redacted a generated secret parameter (such as the password created by `AddPostgres`) when it flowed into a dependent resource, but still emitted the value in plaintext via the owning resource's own environment variable (e.g. `POSTGRES_PASSWORD`). The redaction added in #18089 only enumerated top-level ParameterResource instances in the model, so generated parameters that are referenced by a resource but never added to the model were absent from the redaction set. Mirror ParameterProcessor's dependent-parameter discovery (GetResourceDependenciesAsync) so the redaction set matches the set of secret values that can flow into any resource's environment. The secret parameter instance set is discovered once and cached per connection; only the resolved values are re-read per snapshot. Fixes #19241 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AuxiliaryBackchannelRpcTarget.cs | 104 +++++++++++++++--- .../AuxiliaryBackchannelRpcTargetTests.cs | 53 +++++++++ 2 files changed, 139 insertions(+), 18 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index d162a794ef9..49b66b817f8 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -33,6 +33,12 @@ internal sealed class AuxiliaryBackchannelRpcTarget( { private static readonly TimeSpan s_mcpDiscoveryTimeout = TimeSpan.FromSeconds(5); + // The set of secret parameter instances reachable from the model is fixed once the application is + // running, so it is discovered once and cached for the lifetime of the connection. Only the + // parameters' resolved values change over time, and those are re-read on every snapshot. See + // GetSecretParametersAsync. + private IReadOnlyList? _secretParameters; + #region V2 API Methods /// @@ -975,7 +981,7 @@ private async Task> GetResourceSnapshotsAsync(bool resour // This is a point-in-time batch, so the set of resolved secret values is identical for // every resource. Compute it once here rather than once per resource. - var secretParameterValues = GetResolvedSecretParameterValues(); + var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); // Get current state for each resource directly using TryGetCurrentState foreach (var resource in appModel.Resources) @@ -1025,9 +1031,11 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( await foreach (var resourceEvent in resourceEvents.WithCancellation(cancellationToken).ConfigureAwait(false)) { // Recompute the resolved secret values for every event. Secrets can be resolved between - // events (e.g. interactive parameter entry after the watch starts), so caching the set - // once outside the loop would let a value that becomes secret later bypass redaction. - var secretParameterValues = GetResolvedSecretParameterValues(); + // events (e.g. interactive parameter entry after the watch starts), so reading the values + // once outside the loop would let a value that becomes secret later bypass redaction. Only + // the resolved values are re-read here; the set of secret parameter instances is cached by + // GetSecretParametersAsync. + var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); if (snapshot is not null) { @@ -1211,26 +1219,16 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( => value is not null && secretParameterValues.Contains(value) ? null : value; /// - /// Collects the resolved values of secret parameters in the application model so they can - /// be redacted from data sent to clients. Only values that have already been resolved are + /// Collects the resolved values of secret parameters reachable from the application model so they + /// can be redacted from data sent to clients. Only values that have already been resolved are /// returned; this never blocks waiting for interactive parameter resolution. /// - private HashSet GetResolvedSecretParameterValues() + private async Task> GetResolvedSecretParameterValuesAsync(CancellationToken cancellationToken) { var secretValues = new HashSet(StringComparer.Ordinal); - if (serviceProvider.GetService() is not { } appModel) - { - return secretValues; - } - - foreach (var parameter in appModel.Resources.OfType()) + foreach (var parameter in await GetSecretParametersAsync(cancellationToken).ConfigureAwait(false)) { - if (!parameter.Secret) - { - continue; - } - if (parameter.WaitForValueTcs is { } waitForValueTcs) { // Run mode: peek at the resolved value without waiting for resolution. @@ -1259,6 +1257,76 @@ private HashSet GetResolvedSecretParameterValues() return secretValues; } + /// + /// Gets the set of secret instances reachable from the application + /// model, including parameters that are only referenced by another resource rather than registered + /// as a top-level resource. + /// + /// + /// Enumerating only appModel.Resources.OfType<ParameterResource>() misses generated + /// parameters such as the password created by AddPostgres("pg"), 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. POSTGRES_PASSWORD) leak the secret in plaintext even though the same value + /// was redacted for dependent resources (https://github.com/microsoft/aspire/issues/19241). Discovery + /// mirrors — the component that actually resolves these + /// parameters — so the redaction set matches the set of secret values that can flow into a resource. + /// The result is cached because the model's topology is fixed once the application is running; only the + /// parameters' resolved values change over time and those are re-read by + /// on every snapshot. + /// + private async Task> GetSecretParametersAsync(CancellationToken cancellationToken) + { + if (_secretParameters is { } cached) + { + return cached; + } + + if (serviceProvider.GetService() is not { } appModel) + { + return _secretParameters = []; + } + + var executionContext = serviceProvider.GetRequiredService(); + + // Keyed by name to de-duplicate a parameter that is both a top-level resource and referenced elsewhere. + var secretParameters = new Dictionary(StringComparer.Ordinal); + + foreach (var parameter in appModel.Resources.OfType()) + { + if (parameter.Secret) + { + secretParameters[parameter.Name] = parameter; + } + } + + foreach (var resource in appModel.Resources) + { + IReadOnlySet dependencies; + try + { + // Dependency discovery runs environment/argument callbacks to find referenced resources. + // A misbehaving callback must not break describe, so failures are logged and skipped; any + // top-level secret parameters were already collected above. + dependencies = await resource.GetResourceDependenciesAsync(executionContext, ResourceDependencyDiscoveryMode.Recursive, cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) + { + logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name); + continue; + } + + foreach (var parameter in dependencies.OfType()) + { + if (parameter.Secret) + { + secretParameters[parameter.Name] = parameter; + } + } + } + + return _secretParameters = [.. secretParameters.Values]; + } + private static ResourceSnapshotCommandArgument CreateCommandArgument(InteractionInput input) { return new ResourceSnapshotCommandArgument diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index 3c85f4a7948..ddb9e9e4246 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -375,6 +375,55 @@ await notificationService.PublishUpdateAsync(custom.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RedactsSecretParameterReferencedByOwningResourceEnvironment() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Mimic AddPostgres: a generated secret password parameter that is referenced by the owning + // resource's own environment but is never registered as a top-level resource in the model. Prior + // to the fix for https://github.com/microsoft/aspire/issues/19241 the redaction set only contained + // top-level ParameterResources, so the owning resource leaked this value in plaintext even though + // the same value was redacted when it flowed into a dependent resource. + var passwordParameter = new ParameterResource("pg-password", _ => "generated-s3cr3t", secret: true); + var owner = builder.AddResource(new CustomResourceWithEnvironment("pg")) + .WithEnvironment(context => context.EnvironmentVariables["POSTGRES_PASSWORD"] = passwordParameter); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + // Simulate the generated parameter having been resolved to its runtime value. + passwordParameter.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + passwordParameter.WaitForValueTcs.SetResult("generated-s3cr3t"); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("POSTGRES_PASSWORD", "generated-s3cr3t", true) + ] + }).DefaultTimeout(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var result = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + + var snapshot = Assert.Single(result, r => r.Name == "pg"); + var password = Assert.Single(snapshot.EnvironmentVariables, e => e.Name == "POSTGRES_PASSWORD"); + + // The owning resource's own environment variable must be redacted even though the secret parameter + // is only referenced by the resource and is not a top-level resource in the model. + Assert.Null(password.Value); + + await app.StopAsync().DefaultTimeout(); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotBlockOnUnresolvedSecretParameter() { @@ -933,6 +982,10 @@ private sealed class CustomResource(string name) : Resource(name) { } + private sealed class CustomResourceWithEnvironment(string name) : Resource(name), IResourceWithEnvironment + { + } + // Synthesise per-replica terminal layouts directly rather than going through the public // WithTerminal() path so the test stays focused on backchannel snapshot stamping and doesn't // depend on real DCP terminal-host provisioning. Mirrors DashboardServiceDataTerminalTests. From f0532616cc3c621fef0adc360264f0ee73643c12 Mon Sep 17 00:00:00 2001 From: Mitch Denny Date: Wed, 12 Aug 2026 11:21:04 +1000 Subject: [PATCH 2/8] Preserve same-named secrets during redaction Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 65e57977-4415-4e2f-b01a-3e01b9746dbe --- .../AuxiliaryBackchannelRpcTarget.cs | 11 ++--- .../AuxiliaryBackchannelRpcTargetTests.cs | 44 +++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index 49b66b817f8..6a6dc60e1f3 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -1288,14 +1288,15 @@ private async Task> GetSecretParametersAsync(Ca var executionContext = serviceProvider.GetRequiredService(); - // Keyed by name to de-duplicate a parameter that is both a top-level resource and referenced elsewhere. - var secretParameters = new Dictionary(StringComparer.Ordinal); + // Parameter resources referenced by annotations are not registered in the model, so they are not + // subject to its unique-name constraint. Collect by reference to preserve distinct same-named secrets. + var secretParameters = new HashSet(ReferenceEqualityComparer.Instance); foreach (var parameter in appModel.Resources.OfType()) { if (parameter.Secret) { - secretParameters[parameter.Name] = parameter; + secretParameters.Add(parameter); } } @@ -1319,12 +1320,12 @@ private async Task> GetSecretParametersAsync(Ca { if (parameter.Secret) { - secretParameters[parameter.Name] = parameter; + secretParameters.Add(parameter); } } } - return _secretParameters = [.. secretParameters.Values]; + return _secretParameters = [.. secretParameters]; } private static ResourceSnapshotCommandArgument CreateCommandArgument(InteractionInput input) diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index ddb9e9e4246..d8a445c8efa 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -424,6 +424,50 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RedactsDistinctSecretParametersWithSameName() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + var topLevelParameter = builder.AddParameter("shared-name", "top-level-secret", secret: true); + var referencedParameter = new ParameterResource("shared-name", _ => "referenced-secret", secret: true); + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["REFERENCED_SECRET"] = referencedParameter); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + topLevelParameter.Resource.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + topLevelParameter.Resource.WaitForValueTcs.SetResult("top-level-secret"); + referencedParameter.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + referencedParameter.WaitForValueTcs.SetResult("referenced-secret"); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("TOP_LEVEL_SECRET", "top-level-secret", true), + new EnvironmentVariableSnapshot("REFERENCED_SECRET", "referenced-secret", true) + ] + }).DefaultTimeout(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var result = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + + var snapshot = Assert.Single(result, r => r.Name == "owner"); + Assert.Null(Assert.Single(snapshot.EnvironmentVariables, e => e.Name == "TOP_LEVEL_SECRET").Value); + Assert.Null(Assert.Single(snapshot.EnvironmentVariables, e => e.Name == "REFERENCED_SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotBlockOnUnresolvedSecretParameter() { From 786648301745eef64740e1f44073b4213163c74b Mon Sep 17 00:00:00 2001 From: David Pine Date: Wed, 12 Aug 2026 16:04:42 -0500 Subject: [PATCH 3/8] Recompute secret redaction set per snapshot and fail closed on discovery errors Addresses review feedback on the describe secret-redaction fix (#19241): - Remove the connection-lifetime _secretParameters cache. DCP clears and re-evaluates a resource's environment/argument callbacks on restart (see DcpExecutor.ForgetCachedCallbackResults), so the set of secret parameters a resource references can change across a restart. A set cached for the connection lifetime could omit a newly referenced secret and leak it in plaintext, so the set is now recomputed on every snapshot. Discovery reads the execution-cached callback results (CacheAnnotationCallbackResults) so it observes the same referenced resources that produced the running resource without re-invoking stateful callbacks, keeping the per-call cost low. - Fail closed when dependency discovery throws. The previous broad catch logged and continued with a partial redaction set, which could emit a snapshot with an under-redacted environment. Discovery now propagates non-cancellation exceptions so no snapshot is produced from an incomplete set; cancellation is intentionally excluded so it surfaces as cancellation rather than a leak. Adds regression tests covering restart-safety (recompute), fail-closed propagation, and cancellation surfacing. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 38fd195d-a216-483e-8323-e29217122209 --- .../AuxiliaryBackchannelRpcTarget.cs | 65 +++++---- .../AuxiliaryBackchannelRpcTargetTests.cs | 138 ++++++++++++++++++ 2 files changed, 176 insertions(+), 27 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index 6a6dc60e1f3..edaebacd006 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -33,12 +33,6 @@ internal sealed class AuxiliaryBackchannelRpcTarget( { private static readonly TimeSpan s_mcpDiscoveryTimeout = TimeSpan.FromSeconds(5); - // The set of secret parameter instances reachable from the model is fixed once the application is - // running, so it is discovered once and cached for the lifetime of the connection. Only the - // parameters' resolved values change over time, and those are re-read on every snapshot. See - // GetSecretParametersAsync. - private IReadOnlyList? _secretParameters; - #region V2 API Methods /// @@ -1030,11 +1024,13 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( await foreach (var resourceEvent in resourceEvents.WithCancellation(cancellationToken).ConfigureAwait(false)) { - // Recompute the resolved secret values for every event. Secrets can be resolved between - // events (e.g. interactive parameter entry after the watch starts), so reading the values - // once outside the loop would let a value that becomes secret later bypass redaction. Only - // the resolved values are re-read here; the set of secret parameter instances is cached by - // GetSecretParametersAsync. + // Recompute the secret set for every event. Two things can change while a watch is open, and + // both must be reflected or a secret can be emitted in plaintext: + // 1. A parameter's value can be resolved after the watch starts (e.g. interactive entry). + // 2. The set of secret parameters a resource references can change across a restart — DCP + // clears and re-evaluates environment/argument callbacks on restart (see + // DcpExecutor.ForgetCachedCallbackResults), so a value cached for the connection lifetime + // could omit a newly referenced secret. var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); if (snapshot is not null) @@ -1270,24 +1266,38 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel /// was redacted for dependent resources (https://github.com/microsoft/aspire/issues/19241). Discovery /// mirrors — the component that actually resolves these /// parameters — so the redaction set matches the set of secret values that can flow into a resource. - /// The result is cached because the model's topology is fixed once the application is running; only the - /// parameters' resolved values change over time and those are re-read by - /// on every snapshot. + /// + /// The set is recomputed on every call rather than cached for the connection lifetime. DCP clears and + /// re-evaluates a resource's environment and argument callbacks when it restarts (see + /// DcpExecutor.ForgetCachedCallbackResults), so a restart can change which secret parameters a + /// resource references; a set cached from an earlier snapshot could then omit a newly referenced secret + /// and leak it in plaintext. Discovery reads the execution-cached callback results + /// () so it observes the + /// same referenced resources that produced the running resource instead of re-invoking stateful + /// callbacks, which also keeps the per-call cost low. Discovery failures fail closed: rather than return + /// an incomplete redaction set, the exception propagates so no snapshot is emitted with an + /// under-redacted environment. + /// /// private async Task> GetSecretParametersAsync(CancellationToken cancellationToken) { - if (_secretParameters is { } cached) - { - return cached; - } - if (serviceProvider.GetService() is not { } appModel) { - return _secretParameters = []; + return []; } var executionContext = serviceProvider.GetRequiredService(); + // Read the callback results cached during execution rather than re-invoking the callbacks. This + // observes the same referenced resources that produced the running resource, does not re-run + // stateful callbacks (which could report a different set of parameters), and reflects the values + // DCP re-evaluates on restart. Mirrors ContainerCreator.GetHostDependenciesAsync. + var discoveryOptions = new ResourceDependencyDiscoveryOptions + { + DiscoveryMode = ResourceDependencyDiscoveryMode.Recursive, + CacheAnnotationCallbackResults = true + }; + // Parameter resources referenced by annotations are not registered in the model, so they are not // subject to its unique-name constraint. Collect by reference to preserve distinct same-named secrets. var secretParameters = new HashSet(ReferenceEqualityComparer.Instance); @@ -1305,15 +1315,16 @@ private async Task> GetSecretParametersAsync(Ca IReadOnlySet dependencies; try { - // Dependency discovery runs environment/argument callbacks to find referenced resources. - // A misbehaving callback must not break describe, so failures are logged and skipped; any - // top-level secret parameters were already collected above. - dependencies = await resource.GetResourceDependenciesAsync(executionContext, ResourceDependencyDiscoveryMode.Recursive, cancellationToken).ConfigureAwait(false); + dependencies = await resource.GetResourceDependenciesAsync(executionContext, discoveryOptions, cancellationToken).ConfigureAwait(false); } - catch (Exception ex) + catch (Exception ex) when (ex is not OperationCanceledException) { + // Fail closed at this confidentiality boundary. If a resource's secret dependencies cannot + // be determined, the redaction set is incomplete and a snapshot built from it could expose a + // secret in plaintext, so propagate instead of continuing with a partial set. Cancellation + // is intentionally not caught here so it surfaces as cancellation rather than a leak. logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name); - continue; + throw; } foreach (var parameter in dependencies.OfType()) @@ -1325,7 +1336,7 @@ private async Task> GetSecretParametersAsync(Ca } } - return _secretParameters = [.. secretParameters]; + return [.. secretParameters]; } private static ResourceSnapshotCommandArgument CreateCommandArgument(InteractionInput input) diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index d8a445c8efa..501f6a4e2ce 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -468,6 +468,144 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RecomputesSecretParameterSetPerCall_ForRestartSafety() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // The set of secret parameters a resource references is not fixed for the connection lifetime. DCP + // clears and re-evaluates a resource's environment/argument callbacks when it restarts (see + // DcpExecutor.ForgetCachedCallbackResults), so a restart can make a resource reference a secret it + // did not reference before. A redaction set cached from an earlier snapshot would then omit the + // newly referenced secret and emit it in plaintext. This test simulates a restart between two + // snapshot calls on the same target — flipping the secret the callback references and clearing the + // cached callback result exactly as a restart does — and asserts the second call redacts the newly + // referenced secret. + var secretA = new ParameterResource("secret-a", _ => "value-a", secret: true); + secretA.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secretA.WaitForValueTcs.SetResult("value-a"); + + var secretB = new ParameterResource("secret-b", _ => "value-b", secret: true); + secretB.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secretB.WaitForValueTcs.SetResult("value-b"); + + // The environment callback references whichever secret this local points at when it is evaluated. + // The callback is registered once, before the host is built, so the resource's annotation collection + // is never mutated on a live host — mutating annotations after StartAsync races with background + // annotation evaluation and makes the test flaky. The restart is simulated below by flipping this + // local and clearing the callback's cached result, not by adding another annotation. + var referencedSecret = secretA; + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = referencedSecret); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + // The same target instance is reused across both calls so a connection-lifetime cache, if present, + // would persist between them; the test only distinguishes the fix because the set is recomputed. + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var firstResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var firstSnapshot = Assert.Single(firstResult, r => r.Name == "owner"); + Assert.Null(Assert.Single(firstSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + // Simulate a restart: the resource now references a different secret, and the previously cached + // callback result is cleared exactly as DcpExecutor.ForgetCachedCallbackResults does on restart. + referencedSecret = secretB; + var environmentCallback = owner.Resource.Annotations.OfType().Single(); + environmentCallback.AsCallbackAnnotation().ForgetCachedResult(); + + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-b", true) + ] + }).DefaultTimeout(); + + var secondResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var secondSnapshot = Assert.Single(secondResult, r => r.Name == "owner"); + + // After the restart the redaction set must reflect the newly referenced secret. A set cached from the + // first call would still contain only secretA and would leak secretB's value in plaintext. + Assert.Null(Assert.Single(secondSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + + [Fact] + public async Task GetResourceSnapshotsAsync_FailsClosed_WhenDependencyDiscoveryThrows() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Computing the redaction set is a confidentiality boundary: if a resource's secret dependencies + // cannot be determined, the set is incomplete and a snapshot built from it could leak a secret. + // Discovery must fail closed (propagate) instead of swallowing the failure and emitting snapshots. + // The callback is registered before Build and the host is never started, so the throw originates in + // dependency discovery (GetResourceSnapshotsAsync resolves the secret set before reading any + // snapshot) rather than during application start. Typed as Action to bind the void-returning overload. + Action throwingCallback = + _ => throw new InvalidOperationException("dependency discovery failure"); + builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(throwingCallback); + + using var app = builder.Build(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + await Assert.ThrowsAsync( + async () => await target.GetResourceSnapshotsAsync().DefaultTimeout()); + } + + [Fact] + public async Task GetResourceSnapshotsAsync_DoesNotSwallowCancellation_DuringDependencyDiscovery() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + // Simulate a nested operation being cancelled while discovery evaluates the callback. The previous + // broad catch treated cancellation like any other failure and continued with an incomplete set; the + // fix only excludes OperationCanceledException, so cancellation must surface rather than be swallowed. + // As above, the callback is registered before Build and the host is never started so the cancellation + // originates in dependency discovery, which runs before any snapshot is read. + Action cancelingCallback = + _ => cts.Token.ThrowIfCancellationRequested(); + builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(cancelingCallback); + + using var app = builder.Build(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + await Assert.ThrowsAnyAsync( + async () => await target.GetResourceSnapshotsAsync().DefaultTimeout()); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotBlockOnUnresolvedSecretParameter() { From 0f2b41723f9a9288b5019c6855d076d06ea528e7 Mon Sep 17 00:00:00 2001 From: David Pine Date: Thu, 13 Aug 2026 08:12:32 -0500 Subject: [PATCH 4/8] Discover secret parameters with a single multi-root dependency walk Address PR review feedback: computing the redaction set ran recursive dependency discovery independently for every resource in the model, so the initial WatchAsync stream (one event per resource) performed up to R graph traversals per event and did quadratic-or-worse work. Replace the per-resource loop with a single call to the multi-root ResourceExtensions.GetDependenciesAsync(appModel.Resources, ...) overload, which shares one visited set across all roots so each resource's (execution-cached) callbacks are read at most once. The fail-closed catch (excluding OperationCanceledException) and recompute-per-snapshot behavior are preserved. Also trim the verbose GetSecretParametersAsync remarks to the enduring restart and fail-closed rationale. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- .../AuxiliaryBackchannelRpcTarget.cs | 70 ++++++++----------- 1 file changed, 31 insertions(+), 39 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index edaebacd006..171ad4ff787 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -1259,25 +1259,16 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel /// as a top-level resource. /// /// - /// Enumerating only appModel.Resources.OfType<ParameterResource>() misses generated - /// parameters such as the password created by AddPostgres("pg"), 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. POSTGRES_PASSWORD) leak the secret in plaintext even though the same value - /// was redacted for dependent resources (https://github.com/microsoft/aspire/issues/19241). Discovery - /// mirrors — the component that actually resolves these - /// parameters — so the redaction set matches the set of secret values that can flow into a resource. - /// - /// The set is recomputed on every call rather than cached for the connection lifetime. DCP clears and - /// re-evaluates a resource's environment and argument callbacks when it restarts (see - /// DcpExecutor.ForgetCachedCallbackResults), so a restart can change which secret parameters a - /// resource references; a set cached from an earlier snapshot could then omit a newly referenced secret - /// and leak it in plaintext. Discovery reads the execution-cached callback results - /// () so it observes the - /// same referenced resources that produced the running resource instead of re-invoking stateful - /// callbacks, which also keeps the per-call cost low. Discovery failures fail closed: rather than return - /// an incomplete redaction set, the exception propagates so no snapshot is emitted with an - /// under-redacted environment. - /// + /// The set includes generated parameters (such as the password created by AddPostgres) that are + /// referenced by a resource but never registered in the model, which enumerating + /// appModel.Resources.OfType<ParameterResource>() alone would miss and leak in plaintext + /// (https://github.com/microsoft/aspire/issues/19241). It is recomputed per call rather than cached + /// because DCP re-evaluates a resource's environment and argument callbacks on restart (see + /// DcpExecutor.ForgetCachedCallbackResults), so a restart can change which secrets a resource + /// references and a cached set could omit a newly referenced one. Discovery reads the execution-cached + /// callback results () and + /// fails closed: if dependencies cannot be determined the exception propagates rather than emitting a + /// snapshot from an incomplete redaction set. /// private async Task> GetSecretParametersAsync(CancellationToken cancellationToken) { @@ -1310,29 +1301,30 @@ private async Task> GetSecretParametersAsync(Ca } } - foreach (var resource in appModel.Resources) + // 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. + IReadOnlySet dependencies; + try { - IReadOnlySet dependencies; - try - { - dependencies = await resource.GetResourceDependenciesAsync(executionContext, discoveryOptions, cancellationToken).ConfigureAwait(false); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - // Fail closed at this confidentiality boundary. If a resource's secret dependencies cannot - // be determined, the redaction set is incomplete and a snapshot built from it could expose a - // secret in plaintext, so propagate instead of continuing with a partial set. Cancellation - // is intentionally not caught here so it surfaces as cancellation rather than a leak. - logger.LogDebug(ex, "Failed to compute dependencies for resource {ResourceName} while collecting secret parameters for redaction.", resource.Name); - throw; - } + dependencies = await ResourceExtensions.GetDependenciesAsync(appModel.Resources, executionContext, discoveryOptions, cancellationToken).ConfigureAwait(false); + } + catch (Exception ex) when (ex is not OperationCanceledException) + { + // Fail closed at this confidentiality boundary. If secret dependencies cannot be determined, the + // redaction set is incomplete and a snapshot built from it could expose a secret in plaintext, so + // propagate instead of continuing with a partial set. Cancellation is intentionally not caught + // here so it surfaces as cancellation rather than a leak. + logger.LogDebug(ex, "Failed to compute resource dependencies while collecting secret parameters for redaction."); + throw; + } - foreach (var parameter in dependencies.OfType()) + foreach (var parameter in dependencies.OfType()) + { + if (parameter.Secret) { - if (parameter.Secret) - { - secretParameters.Add(parameter); - } + secretParameters.Add(parameter); } } From 93ba27cd59efb63a1ae96d3efa0e12cca580ad49 Mon Sep 17 00:00:00 2001 From: David Pine Date: Thu, 13 Aug 2026 09:15:21 -0500 Subject: [PATCH 5/8] Make describe secret discovery peek-only and accumulate the redaction set Reworks the #19241 fix per PR review so `aspire describe` never invokes a resource callback while discovering secret parameters, and never shrinks its redaction set across a restart. - Add internal ICallbackResourceAnnotation.TryGetCachedResult so discovery can read an already-cached callback result without executing the callback, and implement it on all three annotations. - Add internal ResourceDependencyDiscoveryOptions.PeekCachedCallbackResultsOnly and honor it in the env/args/launch-tool gatherers, which only read cached results that completed successfully and never invoke a callback. - Switch AuxiliaryBackchannelRpcTarget secret discovery to peek-only and accumulate discovered secret parameters add-only for the connection lifetime, so a lagging snapshot from a prior restart incarnation stays redacted. Peek-only avoids racing DCP's cache lifecycle: invoking a callback from describe could cache a canceled/faulted task (bound to the client's token) that DCP would then reuse on the resource's execution path. A running resource only appears in a snapshot after DCP has already resolved and cached its values, so peeking still observes every secret a snapshot could expose. Tests: prime the callback cache the way DCP does before discovery; add a stale-value-after-restart regression test; replace the invoke-based fail-closed and cancellation tests with peek-proof tests that assert discovery neither invokes nor caches a callback and leaves it evaluable; add a discovery-primitive peek-only test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6410f72-f73a-4be8-b069-7cc8e08a547a --- .../CommandLineArgsCallbackAnnotation.cs | 9 + .../EnvironmentCallbackAnnotation.cs | 9 + .../ICallbackResourceAnnotation.cs | 19 ++ .../LaunchToolArgsCallbackAnnotation.cs | 9 + .../ResourceDependencyDiscoveryOptions.cs | 14 ++ .../ApplicationModel/ResourceExtensions.cs | 87 +++++-- .../AuxiliaryBackchannelRpcTarget.cs | 71 ++++-- .../AuxiliaryBackchannelRpcTargetTests.cs | 225 +++++++++++++++--- .../ResourceDependencyTests.cs | 39 +++ 9 files changed, 401 insertions(+), 81 deletions(-) diff --git a/src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs b/src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs index cd927e8f944..6641aa4362d 100644 --- a/src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs +++ b/src/Aspire.Hosting/ApplicationModel/CommandLineArgsCallbackAnnotation.cs @@ -70,6 +70,15 @@ void IArgCallbackAnnotation.ForgetCachedResult() } } + bool IArgCallbackAnnotation.TryGetCachedResult(out Task>? result) + { + lock(_lock) + { + result = _callbackTask; + return result is not null; + } + } + private async Task> ExecuteCallbackAsync(CommandLineArgsCallbackContext context) { await Callback(context).ConfigureAwait(false); diff --git a/src/Aspire.Hosting/ApplicationModel/EnvironmentCallbackAnnotation.cs b/src/Aspire.Hosting/ApplicationModel/EnvironmentCallbackAnnotation.cs index 410f0282ef8..336d5ee214b 100644 --- a/src/Aspire.Hosting/ApplicationModel/EnvironmentCallbackAnnotation.cs +++ b/src/Aspire.Hosting/ApplicationModel/EnvironmentCallbackAnnotation.cs @@ -103,6 +103,15 @@ void IEnvCallbackAnnotation.ForgetCachedResult() } } + bool IEnvCallbackAnnotation.TryGetCachedResult(out Task>? result) + { + lock(_lock) + { + result = _callbackTask; + return result is not null; + } + } + private async Task> ExecuteCallbackAsync(EnvironmentCallbackContext context) { await Callback(context).ConfigureAwait(false); diff --git a/src/Aspire.Hosting/ApplicationModel/ICallbackResourceAnnotation.cs b/src/Aspire.Hosting/ApplicationModel/ICallbackResourceAnnotation.cs index 767cf003cc4..6113df76b0a 100644 --- a/src/Aspire.Hosting/ApplicationModel/ICallbackResourceAnnotation.cs +++ b/src/Aspire.Hosting/ApplicationModel/ICallbackResourceAnnotation.cs @@ -19,6 +19,25 @@ internal interface ICallbackResourceAnnotation /// The cached result of the callback evaluation. Task EvaluateOnceAsync(TContext context); + /// + /// Peeks at the already-cached callback result without ever executing the callback. + /// + /// + /// When this method returns , the task previously produced by + /// ; otherwise . + /// + /// + /// when a cached result exists; otherwise . + /// + /// + /// This is a read-only peek: unlike it never invokes the callback and never + /// populates the cache. It exists so that read-only consumers (such as aspire describe observing live + /// resource snapshots) can inspect values that DCP has already resolved without racing DCP's own + /// cache lifecycle. Invoking the callback from such a consumer would run it with the consumer's cancellation + /// token and could cache a canceled or faulted task that DCP would later reuse on the resource's execution path. + /// + bool TryGetCachedResult(out Task? result); + /// /// Clears the cached result so that the next call to will re-execute the callback. /// diff --git a/src/Aspire.Hosting/ApplicationModel/LaunchToolArgsCallbackAnnotation.cs b/src/Aspire.Hosting/ApplicationModel/LaunchToolArgsCallbackAnnotation.cs index 0947196c2a1..f9c99dddc54 100644 --- a/src/Aspire.Hosting/ApplicationModel/LaunchToolArgsCallbackAnnotation.cs +++ b/src/Aspire.Hosting/ApplicationModel/LaunchToolArgsCallbackAnnotation.cs @@ -107,6 +107,15 @@ void IArgCallbackAnnotation.ForgetCachedResult() } } + bool IArgCallbackAnnotation.TryGetCachedResult(out Task>? result) + { + lock (_lock) + { + result = _callbackTask; + return result is not null; + } + } + private async Task> ExecuteCallbackAsync(CommandLineArgsCallbackContext context) { await Callback(context).ConfigureAwait(false); diff --git a/src/Aspire.Hosting/ApplicationModel/ResourceDependencyDiscoveryOptions.cs b/src/Aspire.Hosting/ApplicationModel/ResourceDependencyDiscoveryOptions.cs index 3f7e752a67c..8881b126937 100644 --- a/src/Aspire.Hosting/ApplicationModel/ResourceDependencyDiscoveryOptions.cs +++ b/src/Aspire.Hosting/ApplicationModel/ResourceDependencyDiscoveryOptions.cs @@ -19,4 +19,18 @@ public sealed class ResourceDependencyDiscoveryOptions /// on subsequent evaluations of the same annotation, rather than re-evaluating the callback each time. /// public bool CacheAnnotationCallbackResults { get; init; } + + /// + /// When true, discovery reads only callback results that have already been cached by a prior + /// call and never invokes a + /// callback itself. Annotations without a completed cached result are skipped. + /// + /// + /// This is for read-only consumers (such as aspire describe inspecting live resource snapshots) that + /// must not run resource callbacks. Invoking a callback from such a consumer would execute it with the + /// consumer's cancellation token and could poison DCP's cache with a canceled or faulted task that DCP later + /// reuses on the resource's execution path. When this is set, has + /// no effect because nothing is executed or newly cached. + /// + internal bool PeekCachedCallbackResultsOnly { get; init; } } diff --git a/src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs b/src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs index 949ac242dd8..1d801ba5891 100644 --- a/src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs +++ b/src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs @@ -288,6 +288,7 @@ internal static async ValueTask> GatherArgumentValuesWithoutCaching executionContext, logger, cacheAnnotationCallbackResult: false, + peekCachedResultOnly: false, cancellationToken).ConfigureAwait(false); args.InsertRange(0, launchToolArgs); @@ -299,6 +300,7 @@ private static async ValueTask> GatherLaunchToolArgumentValuesAsyn DistributedApplicationExecutionContext executionContext, ILogger logger, bool cacheAnnotationCallbackResult, + bool peekCachedResultOnly, CancellationToken cancellationToken) { // Launch tool arguments run against an isolated list and do not apply to containers, matching @@ -309,6 +311,15 @@ private static async ValueTask> GatherLaunchToolArgumentValuesAsyn return []; } + if (peekCachedResultOnly) + { + // Read-only discovery: never invoke the callback. Only surface a result DCP has already resolved + // and cached; skip anything still in flight, faulted, or canceled. + return annotation.AsCallbackAnnotation().TryGetCachedResult(out var cachedTask) && cachedTask!.IsCompletedSuccessfully + ? cachedTask.Result + : []; + } + var context = new CommandLineArgsCallbackContext([], resource, cancellationToken) { Logger = logger, @@ -1630,52 +1641,83 @@ private static async Task> GatherRawEnvironmentAndArgumentValuesAsy // Gather environment variable values if (resource.TryGetEnvironmentVariables(out var envAnnotations)) { - var envVars = new Dictionary(); - var context = new EnvironmentCallbackContext(executionContext, resource, envVars, cancellationToken: cancellationToken); - - if (options.CacheAnnotationCallbackResults) + if (options.PeekCachedCallbackResultsOnly) { + // Read-only discovery: never invoke a callback. Only harvest values DCP has already resolved + // and cached via EvaluateOnceAsync. Skip in-flight/faulted/canceled tasks so we never block + // describe on an unresolved value nor observe a poisoned task. foreach (var ann in envAnnotations) { - var resultingVars = await ann.AsCallbackAnnotation().EvaluateOnceAsync(context).ConfigureAwait(false); - rawValues.AddRange(resultingVars.Values); + if (ann.AsCallbackAnnotation().TryGetCachedResult(out var cachedTask) && + cachedTask!.IsCompletedSuccessfully) + { + rawValues.AddRange(cachedTask.Result.Values); + } } - } else { - foreach (var ann in envAnnotations) + var envVars = new Dictionary(); + var context = new EnvironmentCallbackContext(executionContext, resource, envVars, cancellationToken: cancellationToken); + + if (options.CacheAnnotationCallbackResults) { - await ann.Callback(context).ConfigureAwait(false); + foreach (var ann in envAnnotations) + { + var resultingVars = await ann.AsCallbackAnnotation().EvaluateOnceAsync(context).ConfigureAwait(false); + rawValues.AddRange(resultingVars.Values); + } + + } + else + { + foreach (var ann in envAnnotations) + { + await ann.Callback(context).ConfigureAwait(false); + } + rawValues.AddRange(envVars.Values); } - rawValues.AddRange(envVars.Values); } } // Gather command-line argument values if (resource.TryGetAnnotationsOfType(out var argAnnotations)) { - var args = new List(); - var context = new CommandLineArgsCallbackContext(args, resource, cancellationToken) - { - ExecutionContext = executionContext - }; - - if (options.CacheAnnotationCallbackResults) + if (options.PeekCachedCallbackResultsOnly) { foreach (var ann in argAnnotations) { - var resultingArgs = await ann.AsCallbackAnnotation().EvaluateOnceAsync(context).ConfigureAwait(false); - rawValues.AddRange(resultingArgs); + if (ann.AsCallbackAnnotation().TryGetCachedResult(out var cachedTask) && + cachedTask!.IsCompletedSuccessfully) + { + rawValues.AddRange(cachedTask.Result); + } } } else { - foreach (var ann in argAnnotations) + var args = new List(); + var context = new CommandLineArgsCallbackContext(args, resource, cancellationToken) + { + ExecutionContext = executionContext + }; + + if (options.CacheAnnotationCallbackResults) + { + foreach (var ann in argAnnotations) + { + var resultingArgs = await ann.AsCallbackAnnotation().EvaluateOnceAsync(context).ConfigureAwait(false); + rawValues.AddRange(resultingArgs); + } + } + else { - await ann.Callback(context).ConfigureAwait(false); + foreach (var ann in argAnnotations) + { + await ann.Callback(context).ConfigureAwait(false); + } + rawValues.AddRange(args); } - rawValues.AddRange(args); } } @@ -1684,6 +1726,7 @@ private static async Task> GatherRawEnvironmentAndArgumentValuesAsy executionContext, NullLogger.Instance, options.CacheAnnotationCallbackResults, + options.PeekCachedCallbackResultsOnly, cancellationToken).ConfigureAwait(false); rawValues.AddRange(launchToolArgs); diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index 171ad4ff787..0b883f8b020 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -33,6 +33,17 @@ internal sealed class AuxiliaryBackchannelRpcTarget( { private static readonly TimeSpan s_mcpDiscoveryTimeout = TimeSpan.FromSeconds(5); + // Add-only accumulator of the secret parameters discovered over this connection's lifetime, guarded by + // _secretParametersLock. A backchannel connection lives for the duration of a single describe/watch (see + // AuxiliaryBackchannelService.HandleClientConnectionAsync), so this spans exactly that read session. It only + // ever grows: when DCP restarts a resource it forgets and re-evaluates the resource's callbacks + // (DcpExecutor.ForgetCachedCallbackResults), which can swap which secret a resource references. A + // still-in-flight snapshot from the prior incarnation can carry the old secret value, so we must keep + // redacting every secret we have ever observed rather than only the current pass's set, otherwise the old + // value would be published in plaintext (https://github.com/microsoft/aspire/issues/19241). + private readonly HashSet _accumulatedSecretParameters = new(ReferenceEqualityComparer.Instance); + private readonly object _secretParametersLock = new(); + #region V2 API Methods /// @@ -1029,8 +1040,10 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( // 1. A parameter's value can be resolved after the watch starts (e.g. interactive entry). // 2. The set of secret parameters a resource references can change across a restart — DCP // clears and re-evaluates environment/argument callbacks on restart (see - // DcpExecutor.ForgetCachedCallbackResults), so a value cached for the connection lifetime - // could omit a newly referenced secret. + // DcpExecutor.ForgetCachedCallbackResults). Peek-only discovery re-runs here and the redaction + // set only ever grows (see GetSecretParametersAsync), so a newly referenced secret is picked up + // while a secret referenced by a prior incarnation — which a lagging snapshot may still carry — + // stays redacted. var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); if (snapshot is not null) @@ -1262,31 +1275,43 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel /// The set includes generated parameters (such as the password created by AddPostgres) that are /// referenced by a resource but never registered in the model, which enumerating /// appModel.Resources.OfType<ParameterResource>() alone would miss and leak in plaintext - /// (https://github.com/microsoft/aspire/issues/19241). It is recomputed per call rather than cached - /// because DCP re-evaluates a resource's environment and argument callbacks on restart (see - /// DcpExecutor.ForgetCachedCallbackResults), so a restart can change which secrets a resource - /// references and a cached set could omit a newly referenced one. Discovery reads the execution-cached - /// callback results () and - /// fails closed: if dependencies cannot be determined the exception propagates rather than emitting a - /// snapshot from an incomplete redaction set. + /// (https://github.com/microsoft/aspire/issues/19241). + /// + /// Discovery is peek-only: it reads callback results that DCP already resolved and cached while + /// starting the resource () and + /// never invokes a callback itself. This matters because aspire describe observes live resource + /// snapshots concurrently with DCP's own cache lifecycle: DCP forgets and re-evaluates a resource's callbacks + /// on restart (see DcpExecutor.ForgetCachedCallbackResults). Invoking a callback from here would run it + /// with the client's cancellation token and could cache a canceled or faulted task that DCP would then reuse on + /// the resource's execution path. A running resource can only appear in a snapshot after DCP has resolved and + /// cached its values, so peeking still observes every secret that a snapshot could expose. + /// + /// + /// The discovered set is merged into a per-connection, add-only accumulator: it only ever grows for the life + /// of this describe/watch. A restart can change which secret a resource references, and a still-in-flight + /// snapshot from the prior incarnation can carry the previous value, so the redaction set must never shrink or + /// that value would be emitted in plaintext. + /// /// private async Task> GetSecretParametersAsync(CancellationToken cancellationToken) { if (serviceProvider.GetService() is not { } appModel) { - return []; + lock (_secretParametersLock) + { + return [.. _accumulatedSecretParameters]; + } } var executionContext = serviceProvider.GetRequiredService(); - // Read the callback results cached during execution rather than re-invoking the callbacks. This - // observes the same referenced resources that produced the running resource, does not re-run - // stateful callbacks (which could report a different set of parameters), and reflects the values - // DCP re-evaluates on restart. Mirrors ContainerCreator.GetHostDependenciesAsync. + // Peek at the callback results DCP cached while starting each resource; never invoke a callback. This + // observes the same referenced resources that produced the running snapshot without racing DCP's cache + // lifecycle or running stateful callbacks with the client's cancellation token. var discoveryOptions = new ResourceDependencyDiscoveryOptions { DiscoveryMode = ResourceDependencyDiscoveryMode.Recursive, - CacheAnnotationCallbackResults = true + PeekCachedCallbackResultsOnly = true }; // Parameter resources referenced by annotations are not registered in the model, so they are not @@ -1312,10 +1337,10 @@ private async Task> GetSecretParametersAsync(Ca } catch (Exception ex) when (ex is not OperationCanceledException) { - // Fail closed at this confidentiality boundary. If secret dependencies cannot be determined, the - // redaction set is incomplete and a snapshot built from it could expose a secret in plaintext, so - // propagate instead of continuing with a partial set. Cancellation is intentionally not caught - // here so it surfaces as cancellation rather than a leak. + // Fail closed at this confidentiality boundary. Peek-only discovery does not invoke callbacks, so a + // failure here is unexpected; if it does happen the redaction set is incomplete and a snapshot built + // from it could expose a secret in plaintext, so propagate instead of continuing with a partial set. + // Cancellation is intentionally not caught here so it surfaces as cancellation rather than a leak. logger.LogDebug(ex, "Failed to compute resource dependencies while collecting secret parameters for redaction."); throw; } @@ -1328,7 +1353,13 @@ private async Task> GetSecretParametersAsync(Ca } } - return [.. secretParameters]; + // Merge this pass's discoveries into the add-only accumulator and return everything seen so far, so a + // secret referenced by an earlier incarnation stays redacted even after a restart re-points the resource. + lock (_secretParametersLock) + { + _accumulatedSecretParameters.UnionWith(secretParameters); + return [.. _accumulatedSecretParameters]; + } } private static ResourceSnapshotCommandArgument CreateCommandArgument(InteractionInput input) diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index 501f6a4e2ce..a4beb201f92 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -396,6 +396,9 @@ public async Task GetResourceSnapshotsAsync_RedactsSecretParameterReferencedByOw passwordParameter.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); passwordParameter.WaitForValueTcs.SetResult("generated-s3cr3t"); + // Cache the env callback the way DCP does on start, so peek-only discovery can observe the reference. + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + var notificationService = app.Services.GetRequiredService(); await notificationService.PublishUpdateAsync(owner.Resource, s => s with { @@ -442,6 +445,9 @@ public async Task GetResourceSnapshotsAsync_RedactsDistinctSecretParametersWithS referencedParameter.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); referencedParameter.WaitForValueTcs.SetResult("referenced-secret"); + // Cache the env callback the way DCP does on start, so peek-only discovery can observe the reference. + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + var notificationService = app.Services.GetRequiredService(); await notificationService.PublishUpdateAsync(owner.Resource, s => s with { @@ -469,18 +475,18 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with } [Fact] - public async Task GetResourceSnapshotsAsync_RecomputesSecretParameterSetPerCall_ForRestartSafety() + public async Task GetResourceSnapshotsAsync_RedactsNewlyReferencedSecret_AfterRestartRepointsResource() { using var builder = TestDistributedApplicationBuilder.Create(outputHelper); // The set of secret parameters a resource references is not fixed for the connection lifetime. DCP // clears and re-evaluates a resource's environment/argument callbacks when it restarts (see // DcpExecutor.ForgetCachedCallbackResults), so a restart can make a resource reference a secret it - // did not reference before. A redaction set cached from an earlier snapshot would then omit the - // newly referenced secret and emit it in plaintext. This test simulates a restart between two - // snapshot calls on the same target — flipping the secret the callback references and clearing the - // cached callback result exactly as a restart does — and asserts the second call redacts the newly - // referenced secret. + // did not reference before. Discovery is peek-only (it reads the callback results DCP cached on start + // and never invokes a callback), so this test primes the cache like DCP does, then simulates a restart + // between two snapshot calls on the same target — flipping the referenced secret, clearing the cached + // callback result, and re-priming exactly as a restart does — and asserts the second call redacts the + // newly referenced secret. var secretA = new ParameterResource("secret-a", _ => "value-a", secret: true); secretA.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); secretA.WaitForValueTcs.SetResult("value-a"); @@ -493,7 +499,7 @@ public async Task GetResourceSnapshotsAsync_RecomputesSecretParameterSetPerCall_ // The callback is registered once, before the host is built, so the resource's annotation collection // is never mutated on a live host — mutating annotations after StartAsync races with background // annotation evaluation and makes the test flaky. The restart is simulated below by flipping this - // local and clearing the callback's cached result, not by adding another annotation. + // local, clearing the callback's cached result, and re-priming, not by adding another annotation. var referencedSecret = secretA; var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = referencedSecret); @@ -501,6 +507,9 @@ public async Task GetResourceSnapshotsAsync_RecomputesSecretParameterSetPerCall_ using var app = builder.Build(); await app.StartAsync().DefaultTimeout(); + // Cache the callback result (referencing secretA) the way DCP does when it first starts the resource. + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + var notificationService = app.Services.GetRequiredService(); await notificationService.PublishUpdateAsync(owner.Resource, s => s with { @@ -511,8 +520,8 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with ] }).DefaultTimeout(); - // The same target instance is reused across both calls so a connection-lifetime cache, if present, - // would persist between them; the test only distinguishes the fix because the set is recomputed. + // The same target instance is reused across both calls so the add-only accumulator persists between + // them, exactly as it would over the lifetime of a single describe/watch connection. var target = new AuxiliaryBackchannelRpcTarget( NullLogger.Instance, app.Services.GetRequiredService(), @@ -523,11 +532,13 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with var firstSnapshot = Assert.Single(firstResult, r => r.Name == "owner"); Assert.Null(Assert.Single(firstSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); - // Simulate a restart: the resource now references a different secret, and the previously cached - // callback result is cleared exactly as DcpExecutor.ForgetCachedCallbackResults does on restart. + // Simulate a restart: the resource now references a different secret. Clear the previously cached + // callback result exactly as DcpExecutor.ForgetCachedCallbackResults does, then re-prime so the cache + // holds the new reference (secretB) as it would after DCP re-evaluates on restart. referencedSecret = secretB; var environmentCallback = owner.Resource.Annotations.OfType().Single(); environmentCallback.AsCallbackAnnotation().ForgetCachedResult(); + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); await notificationService.PublishUpdateAsync(owner.Resource, s => s with { @@ -541,27 +552,104 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with var secondResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); var secondSnapshot = Assert.Single(secondResult, r => r.Name == "owner"); - // After the restart the redaction set must reflect the newly referenced secret. A set cached from the - // first call would still contain only secretA and would leak secretB's value in plaintext. + // After the restart the redaction set must reflect the newly referenced secret. Peek-only discovery on + // the second call observes secretB (freshly cached), so value-b is redacted. + Assert.Null(Assert.Single(secondSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + + [Fact] + public async Task GetResourceSnapshotsAsync_RetainsPreviouslyReferencedSecret_AfterRestartRepointsResource() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Regression test for the leak adamint identified in review of the #19241 fix: during a restart, a + // still-in-flight snapshot from the prior incarnation can carry the OLD secret value while the resource's + // callback has already been re-pointed at a new secret. If discovery only redacted the current pass's + // set, the stale old value would be emitted in plaintext. The per-connection redaction set is add-only, + // so a secret observed on an earlier pass stays redacted even after the resource stops referencing it. + var secretA = new ParameterResource("secret-a", _ => "value-a", secret: true); + secretA.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secretA.WaitForValueTcs.SetResult("value-a"); + + var secretB = new ParameterResource("secret-b", _ => "value-b", secret: true); + secretB.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secretB.WaitForValueTcs.SetResult("value-b"); + + var referencedSecret = secretA; + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = referencedSecret); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + // First pass observes secretA and adds it to the connection's redaction set. + var firstResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var firstSnapshot = Assert.Single(firstResult, r => r.Name == "owner"); + Assert.Null(Assert.Single(firstSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + // Restart re-points the resource at secretB, but a lagging snapshot from the old incarnation still + // carries value-a. Re-point and re-prime the cache so discovery would, on its own, only find secretB. + referencedSecret = secretB; + var environmentCallback = owner.Resource.Annotations.OfType().Single(); + environmentCallback.AsCallbackAnnotation().ForgetCachedResult(); + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Stopping", KnownResourceStateStyles.Info), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + var secondResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var secondSnapshot = Assert.Single(secondResult, r => r.Name == "owner"); + + // The stale value-a must still be redacted. Without add-only accumulation, discovery on this pass finds + // only secretB and would publish value-a in plaintext — the exact leak the reviewer called out. Assert.Null(Assert.Single(secondSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); await app.StopAsync().DefaultTimeout(); } [Fact] - public async Task GetResourceSnapshotsAsync_FailsClosed_WhenDependencyDiscoveryThrows() + public async Task GetResourceSnapshotsAsync_DoesNotInvokeUncachedResourceCallback_AndLeavesItEvaluable() { using var builder = TestDistributedApplicationBuilder.Create(outputHelper); - // Computing the redaction set is a confidentiality boundary: if a resource's secret dependencies - // cannot be determined, the set is incomplete and a snapshot built from it could leak a secret. - // Discovery must fail closed (propagate) instead of swallowing the failure and emitting snapshots. - // The callback is registered before Build and the host is never started, so the throw originates in - // dependency discovery (GetResourceSnapshotsAsync resolves the secret set before reading any - // snapshot) rather than during application start. Typed as Action to bind the void-returning overload. - Action throwingCallback = - _ => throw new InvalidOperationException("dependency discovery failure"); - builder.AddResource(new CustomResourceWithEnvironment("owner")) + // Peek-only discovery must never invoke a resource callback: it reads only results DCP already cached. + // A callback that has not been cached yet is simply skipped, so describe succeeds without running it, and + // the callback is left untouched — no cached result, still evaluable exactly as authored. This replaces + // the previous "fail closed when the callback throws during discovery" test: discovery no longer invokes + // callbacks, so a throwing callback can never be reached (and thus never poisoned) by describe. + var invocations = 0; + Action throwingCallback = _ => + { + invocations++; + throw new InvalidOperationException("resource callbacks must not be invoked by describe discovery"); + }; + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) .WithEnvironment(throwingCallback); using var app = builder.Build(); @@ -572,27 +660,45 @@ public async Task GetResourceSnapshotsAsync_FailsClosed_WhenDependencyDiscoveryT app.Services.GetRequiredService(), app.Services); + // Describe succeeds because the uncached, throwing callback is peeked (found absent) rather than invoked. + _ = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + Assert.Equal(0, invocations); + + // Discovery left the cache pristine: nothing was cached, so DCP can still evaluate the callback and it + // behaves exactly as authored (invoked once, throwing its own exception). + var annotation = owner.Resource.Annotations.OfType().Single(); + Assert.False(annotation.AsCallbackAnnotation().TryGetCachedResult(out var cached)); + Assert.Null(cached); + + var executionContext = app.Services.GetRequiredService(); await Assert.ThrowsAsync( - async () => await target.GetResourceSnapshotsAsync().DefaultTimeout()); + async () => await annotation.AsCallbackAnnotation() + .EvaluateOnceAsync(new EnvironmentCallbackContext(executionContext, owner.Resource, new Dictionary())) + .DefaultTimeout()); + Assert.Equal(1, invocations); } [Fact] - public async Task GetResourceSnapshotsAsync_DoesNotSwallowCancellation_DuringDependencyDiscovery() + public async Task GetResourceSnapshotsAsync_DoesNotInvokeOrPoisonCallbackCache_WhenDescribeIsCanceled() { using var builder = TestDistributedApplicationBuilder.Create(outputHelper); - using var cts = new CancellationTokenSource(); - cts.Cancel(); - - // Simulate a nested operation being cancelled while discovery evaluates the callback. The previous - // broad catch treated cancellation like any other failure and continued with an incomplete set; the - // fix only excludes OperationCanceledException, so cancellation must surface rather than be swallowed. - // As above, the callback is registered before Build and the host is never started so the cancellation - // originates in dependency discovery, which runs before any snapshot is read. - Action cancelingCallback = - _ => cts.Token.ThrowIfCancellationRequested(); - builder.AddResource(new CustomResourceWithEnvironment("owner")) - .WithEnvironment(cancelingCallback); + // The precise leak adamint identified in review: previously discovery invoked resource callbacks, so a + // describe/watch cancelled while an unresolved parameter callback awaited the client's cancellation token + // cached a *cancelled* task that DCP then reused, failing the resource until the next restart. Peek-only + // discovery never invokes a callback, so even a cancelled describe cannot run it nor cache a cancelled + // task, and the annotation stays fully evaluable for DCP afterward. + var invocations = 0; + Action cancelAwareCallback = context => + { + invocations++; + // If discovery had invoked this under the cancelled describe token (old behaviour), the throw would + // have been cached as a cancelled/faulted task and reused by DCP. + context.CancellationToken.ThrowIfCancellationRequested(); + context.EnvironmentVariables["SECRET"] = "resolved"; + }; + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(cancelAwareCallback); using var app = builder.Build(); @@ -602,8 +708,34 @@ public async Task GetResourceSnapshotsAsync_DoesNotSwallowCancellation_DuringDep app.Services.GetRequiredService(), app.Services); - await Assert.ThrowsAnyAsync( - async () => await target.GetResourceSnapshotsAsync().DefaultTimeout()); + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + try + { + await target.GetResourceSnapshotsAsync(cts.Token).DefaultTimeout(); + } + catch (OperationCanceledException) + { + // Cancellation surfacing is acceptable; the guarantee under test is that the callback was neither + // invoked nor cached, regardless of whether the cancelled describe returns or throws. + } + + var annotation = owner.Resource.Annotations.OfType().Single(); + Assert.Equal(0, invocations); + Assert.False(annotation.AsCallbackAnnotation().TryGetCachedResult(out var cached)); + Assert.Null(cached); + + // The annotation is still evaluable: DCP evaluates it to a successful, cached result. The earlier + // cancelled describe left nothing poisoned behind, so the resource is not stuck failing until a restart. + var executionContext = app.Services.GetRequiredService(); + var evaluated = await annotation.AsCallbackAnnotation() + .EvaluateOnceAsync(new EnvironmentCallbackContext(executionContext, owner.Resource, new Dictionary())) + .DefaultTimeout(); + Assert.Equal(1, invocations); + Assert.True(annotation.AsCallbackAnnotation().TryGetCachedResult(out var cachedAfter)); + Assert.True(cachedAfter!.IsCompletedSuccessfully); + Assert.Equal("resolved", evaluated["SECRET"]); } [Fact] @@ -779,6 +911,21 @@ private static bool HasEnvironmentVariable(ResourceSnapshot snapshot, string nam private static string? GetEnvironmentVariableValue(ResourceSnapshot snapshot, string name) => snapshot.EnvironmentVariables.Single(e => e.Name == name).Value; + // Mirror what DCP does when it starts a resource: evaluate the resource's environment callbacks once so the + // result is cached. Peek-only secret discovery (used by describe/watch) never invokes callbacks itself — it + // only reads results DCP already cached — so a test must prime that cache the same way a real run would before + // a secret referenced through a callback can be discovered and redacted. + private static async Task PrimeEnvironmentCallbackCacheAsync(IResource resource, IServiceProvider services) + { + var executionContext = services.GetRequiredService(); + foreach (var annotation in resource.Annotations.OfType()) + { + await annotation.AsCallbackAnnotation() + .EvaluateOnceAsync(new EnvironmentCallbackContext(executionContext, resource, new Dictionary())) + .ConfigureAwait(false); + } + } + [Fact] public async Task WaitForResourceAsync_ReturnsFailureWhenResourceHasErrorStateStyle() { diff --git a/tests/Aspire.Hosting.Tests/ResourceDependencyTests.cs b/tests/Aspire.Hosting.Tests/ResourceDependencyTests.cs index a1eb2b32c2d..db787e8b029 100644 --- a/tests/Aspire.Hosting.Tests/ResourceDependencyTests.cs +++ b/tests/Aspire.Hosting.Tests/ResourceDependencyTests.cs @@ -234,6 +234,45 @@ public async Task ParameterInLaunchToolArgsIsIncluded(bool cacheAnnotationCallba Assert.Collection(dependencies, dependency => Assert.Same(param.Resource, dependency)); } + [Fact] + public async Task PeekCachedCallbackResultsOnly_OnlySeesCachedResultsAndNeverInvokesCallback() + { + using var builder = TestDistributedApplicationBuilder.Create(); + + var param = builder.AddParameter("config"); + var invocations = 0; + var executable = builder.AddExecutable("app", "myapp", ".") + .WithEnvironment(context => + { + invocations++; + context.EnvironmentVariables["CONFIG"] = param.Resource; + }); + + var executionContext = new DistributedApplicationExecutionContext(DistributedApplicationOperation.Run); + var peekOptions = new ResourceDependencyDiscoveryOptions + { + DiscoveryMode = ResourceDependencyDiscoveryMode.DirectOnly, + PeekCachedCallbackResultsOnly = true + }; + + // Nothing has evaluated/cached the callback yet, so peek-only discovery must not invoke it and must not + // discover the referenced parameter — it only reads results that were already cached. + var beforePriming = await executable.Resource.GetResourceDependenciesAsync(executionContext, peekOptions); + Assert.Equal(0, invocations); + Assert.Empty(beforePriming); + + // Mirror what DCP does when it starts the resource: evaluate the callback once so its result is cached. + var annotation = executable.Resource.Annotations.OfType().Single(); + await annotation.AsCallbackAnnotation().EvaluateOnceAsync( + new EnvironmentCallbackContext(executionContext, executable.Resource, new Dictionary())); + Assert.Equal(1, invocations); + + // Peek-only discovery now surfaces the cached reference without invoking the callback a second time. + var afterPriming = await executable.Resource.GetResourceDependenciesAsync(executionContext, peekOptions); + Assert.Equal(1, invocations); + Assert.Contains(param.Resource, afterPriming); + } + [Fact] public async Task OnlyLastLaunchToolArgsAnnotationContributesDependencies() { From b43bfddfdd9a8a7d3290de1fe8fc9c27a020de0d Mon Sep 17 00:00:00 2001 From: David Pine Date: Thu, 13 Aug 2026 09:36:29 -0500 Subject: [PATCH 6/8] Accumulate resolved secret values add-only, not just parameter objects Addresses PR review: retaining the discovered secret ParameterResource objects is not enough to keep a prior secret redacted. The runtime "Set parameter" path replaces a parameter's already-completed WaitForValueTcs with a new value (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 secret string, which would then be emitted in plaintext. Add a per-connection, add-only HashSet of resolved secret values guarded by the existing lock. GetResolvedSecretParameterValuesAsync now unions each pass's resolved values into it and returns the accumulation, so once a secret string has been observed it stays redacted for the life of the describe/watch even after the owning parameter's value is reassigned. Test: RetainsPreviousSecretValue_AfterParameterValueIsReplaced references one secret parameter, redacts value-a, replaces the parameter's WaitForValueTcs with value-b while a lagging snapshot still carries value-a, and asserts value-a stays redacted. It fails when only parameter objects (not values) are accumulated. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: e6410f72-f73a-4be8-b069-7cc8e08a547a --- .../AuxiliaryBackchannelRpcTarget.cs | 55 +++++++++----- .../AuxiliaryBackchannelRpcTargetTests.cs | 71 +++++++++++++++++++ 2 files changed, 110 insertions(+), 16 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index 0b883f8b020..83640ddd7f3 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -33,15 +33,23 @@ internal sealed class AuxiliaryBackchannelRpcTarget( { private static readonly TimeSpan s_mcpDiscoveryTimeout = TimeSpan.FromSeconds(5); - // Add-only accumulator of the secret parameters discovered over this connection's lifetime, guarded by + // Add-only accumulators of the secrets discovered over this connection's lifetime, both guarded by // _secretParametersLock. A backchannel connection lives for the duration of a single describe/watch (see - // AuxiliaryBackchannelService.HandleClientConnectionAsync), so this spans exactly that read session. It only - // ever grows: when DCP restarts a resource it forgets and re-evaluates the resource's callbacks - // (DcpExecutor.ForgetCachedCallbackResults), which can swap which secret a resource references. A - // still-in-flight snapshot from the prior incarnation can carry the old secret value, so we must keep - // redacting every secret we have ever observed rather than only the current pass's set, otherwise the old - // value would be published in plaintext (https://github.com/microsoft/aspire/issues/19241). + // AuxiliaryBackchannelService.HandleClientConnectionAsync), so these span exactly that read session. + // + // _accumulatedSecretParameters only ever grows: when DCP restarts a resource it forgets and re-evaluates the + // resource's callbacks (DcpExecutor.ForgetCachedCallbackResults), which can swap which secret a resource + // references. A still-in-flight snapshot from the prior incarnation can carry the old secret, so we keep + // trying to resolve every secret parameter we have ever observed rather than only the current pass's set. + // + // _accumulatedSecretValues also only ever grows, and is required in addition to the parameter set because a + // parameter's resolved value can be replaced in place: the runtime "Set parameter" path swaps a completed + // ParameterResource.WaitForValueTcs for 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 we must keep redacting every secret string we have ever resolved or + // that old value would be emitted in plaintext (https://github.com/microsoft/aspire/issues/19241). private readonly HashSet _accumulatedSecretParameters = new(ReferenceEqualityComparer.Instance); + private readonly HashSet _accumulatedSecretValues = new(StringComparer.Ordinal); private readonly object _secretParametersLock = new(); #region V2 API Methods @@ -1035,8 +1043,8 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( await foreach (var resourceEvent in resourceEvents.WithCancellation(cancellationToken).ConfigureAwait(false)) { - // Recompute the secret set for every event. Two things can change while a watch is open, and - // both must be reflected or a secret can be emitted in plaintext: + // Recompute the secret set for every event. Three things can change while a watch is open, and all + // must be reflected or a secret can be emitted in plaintext: // 1. A parameter's value can be resolved after the watch starts (e.g. interactive entry). // 2. The set of secret parameters a resource references can change across a restart — DCP // clears and re-evaluates environment/argument callbacks on restart (see @@ -1044,6 +1052,9 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( // set only ever grows (see GetSecretParametersAsync), so a newly referenced secret is picked up // while a secret referenced by a prior incarnation — which a lagging snapshot may still carry — // stays redacted. + // 3. A parameter's resolved value can be replaced in place (the runtime "Set parameter" path), so + // resolved secret strings are also accumulated add-only (see GetResolvedSecretParameterValuesAsync) + // to keep redacting a prior value that a still-current snapshot may carry. var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); if (snapshot is not null) @@ -1228,13 +1239,17 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( => value is not null && secretParameterValues.Contains(value) ? null : value; /// - /// Collects the resolved values of secret parameters reachable from the application model so they - /// can be redacted from data sent to clients. Only values that have already been resolved are - /// returned; this never blocks waiting for interactive parameter resolution. + /// Collects the resolved values of secret parameters reachable from the application model so they can be + /// redacted from data sent to clients. Only values that have already been resolved are included; this never + /// blocks waiting for interactive parameter resolution. Resolved values are accumulated add-only for the + /// connection lifetime, so a value a parameter has since been reassigned away from stays redacted while an + /// older snapshot can still carry it. /// private async Task> GetResolvedSecretParameterValuesAsync(CancellationToken cancellationToken) { - var secretValues = new HashSet(StringComparer.Ordinal); + // Resolve the current value of each accumulated secret parameter (peek-only; never blocks on interactive + // resolution), then merge into the connection's add-only value set below. + var resolvedThisPass = new List(); foreach (var parameter in await GetSecretParametersAsync(cancellationToken).ConfigureAwait(false)) { @@ -1244,7 +1259,7 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel if (waitForValueTcs.Task is { IsCompletedSuccessfully: true } valueTask && valueTask.Result is { Length: > 0 } value) { - secretValues.Add(value); + resolvedThisPass.Add(value); } } else @@ -1253,7 +1268,7 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel { if (parameter.ValueInternal is { Length: > 0 } value) { - secretValues.Add(value); + resolvedThisPass.Add(value); } } catch @@ -1263,7 +1278,15 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel } } - return secretValues; + // Accumulate this pass's resolved secret strings add-only and return everything seen so far. A parameter's + // value can be replaced in place (the runtime "Set parameter" path swaps its completed WaitForValueTcs), + // so re-resolving a retained parameter later yields only the new value; keeping every value we have ever + // resolved ensures a still-current snapshot carrying the previous value is still redacted. + lock (_secretParametersLock) + { + _accumulatedSecretValues.UnionWith(resolvedThisPass); + return [.. _accumulatedSecretValues]; + } } /// diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index a4beb201f92..4c7b468fe73 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -633,6 +633,77 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RetainsPreviousSecretValue_AfterParameterValueIsReplaced() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Regression test for the leak the reviewer identified: the runtime "Set parameter" path replaces a + // parameter's already-completed WaitForValueTcs with a new value (ParameterProcessor.SetParameterValue), + // so re-resolving the SAME retained parameter object later yields only the new value. An already-published + // or still-current snapshot can still carry the previous secret value, so redaction accumulates resolved + // secret STRINGS add-only — retaining the parameter object alone is not enough because its value has been + // overwritten in place. The owner keeps referencing the same parameter throughout; only its value changes. + var secret = new ParameterResource("secret", _ => "value-a", secret: true); + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secret.WaitForValueTcs.SetResult("value-a"); + + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = secret); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + // The same target instance is reused across both calls so the add-only accumulator persists between them, + // exactly as it would over the lifetime of a single describe/watch connection. + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + // First pass observes value-a and adds it to the connection's redaction set. + var firstResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var firstSnapshot = Assert.Single(firstResult, r => r.Name == "owner"); + Assert.Null(Assert.Single(firstSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + // The runtime replaces the parameter's resolved value with value-b (as SetParameterValue does by swapping + // the completed WaitForValueTcs), but a lagging snapshot still carries value-a. Re-resolving the same + // parameter object now yields only value-b. + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secret.WaitForValueTcs.SetResult("value-b"); + + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + var secondResult = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + var secondSnapshot = Assert.Single(secondResult, r => r.Name == "owner"); + + // value-a must still be redacted. Without add-only value accumulation, re-resolving the parameter yields + // only value-b, so the stale value-a would be emitted in plaintext — the exact leak the reviewer called out. + Assert.Null(Assert.Single(secondSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotInvokeUncachedResourceCallback_AndLeavesItEvaluable() { From 7f64ebbf87b346c5a7decde229873a0c423ab1cb Mon Sep 17 00:00:00 2001 From: David Pine Date: Thu, 13 Aug 2026 10:43:54 -0500 Subject: [PATCH 7/8] Scope describe secret redaction history to the AppHost 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 --- .../AuxiliaryBackchannelRpcTarget.cs | 59 ++++---------- .../Backchannel/SecretRedactionHistory.cs | 77 +++++++++++++++++++ .../DistributedApplicationBuilder.cs | 3 + .../AuxiliaryBackchannelRpcTargetTests.cs | 76 ++++++++++++++++++ 4 files changed, 172 insertions(+), 43 deletions(-) create mode 100644 src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index 83640ddd7f3..b8de3d55321 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -33,25 +33,6 @@ internal sealed class AuxiliaryBackchannelRpcTarget( { private static readonly TimeSpan s_mcpDiscoveryTimeout = TimeSpan.FromSeconds(5); - // Add-only accumulators of the secrets discovered over this connection's lifetime, both guarded by - // _secretParametersLock. A backchannel connection lives for the duration of a single describe/watch (see - // AuxiliaryBackchannelService.HandleClientConnectionAsync), so these span exactly that read session. - // - // _accumulatedSecretParameters only ever grows: when DCP restarts a resource it forgets and re-evaluates the - // resource's callbacks (DcpExecutor.ForgetCachedCallbackResults), which can swap which secret a resource - // references. A still-in-flight snapshot from the prior incarnation can carry the old secret, so we keep - // trying to resolve every secret parameter we have ever observed rather than only the current pass's set. - // - // _accumulatedSecretValues also only ever grows, and is required in addition to the parameter set because a - // parameter's resolved value can be replaced in place: the runtime "Set parameter" path swaps a completed - // ParameterResource.WaitForValueTcs for 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 we must keep redacting every secret string we have ever resolved or - // that old value would be emitted in plaintext (https://github.com/microsoft/aspire/issues/19241). - private readonly HashSet _accumulatedSecretParameters = new(ReferenceEqualityComparer.Instance); - private readonly HashSet _accumulatedSecretValues = new(StringComparer.Ordinal); - private readonly object _secretParametersLock = new(); - #region V2 API Methods /// @@ -1241,14 +1222,14 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( /// /// Collects the resolved values of secret parameters reachable from the application model so they can be /// redacted from data sent to clients. Only values that have already been resolved are included; this never - /// blocks waiting for interactive parameter resolution. Resolved values are accumulated add-only for the - /// connection lifetime, so a value a parameter has since been reassigned away from stays redacted while an - /// older snapshot can still carry it. + /// blocks waiting for interactive parameter resolution. Resolved values are accumulated add-only in the + /// AppHost-scoped , so a value a parameter has since been reassigned away + /// from stays redacted while an older snapshot can still carry it. /// private async Task> GetResolvedSecretParameterValuesAsync(CancellationToken cancellationToken) { // Resolve the current value of each accumulated secret parameter (peek-only; never blocks on interactive - // resolution), then merge into the connection's add-only value set below. + // resolution), then merge into the AppHost-scoped add-only value history below. var resolvedThisPass = new List(); foreach (var parameter in await GetSecretParametersAsync(cancellationToken).ConfigureAwait(false)) @@ -1282,11 +1263,7 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel // value can be replaced in place (the runtime "Set parameter" path swaps its completed WaitForValueTcs), // so re-resolving a retained parameter later yields only the new value; keeping every value we have ever // resolved ensures a still-current snapshot carrying the previous value is still redacted. - lock (_secretParametersLock) - { - _accumulatedSecretValues.UnionWith(resolvedThisPass); - return [.. _accumulatedSecretValues]; - } + return serviceProvider.GetRequiredService().AddValuesAndSnapshot(resolvedThisPass); } /// @@ -1310,20 +1287,20 @@ private async Task> GetResolvedSecretParameterValuesAsync(Cancel /// cached its values, so peeking still observes every secret that a snapshot could expose. /// /// - /// The discovered set is merged into a per-connection, add-only accumulator: it only ever grows for the life - /// of this describe/watch. A restart can change which secret a resource references, and a still-in-flight - /// snapshot from the prior incarnation can carry the previous value, so the redaction set must never shrink or - /// that value would be emitted in plaintext. + /// The discovered set is merged into the AppHost-scoped, add-only : it only + /// ever grows for the life of the AppHost and is shared across connections. A restart can change which secret a + /// resource references, and a still-in-flight snapshot from the prior incarnation can carry the previous value, + /// so the redaction set must never shrink or that value would be emitted in plaintext. /// /// private async Task> GetSecretParametersAsync(CancellationToken cancellationToken) { + var history = serviceProvider.GetRequiredService(); + if (serviceProvider.GetService() is not { } appModel) { - lock (_secretParametersLock) - { - return [.. _accumulatedSecretParameters]; - } + // No model resolved yet; return the secrets accumulated so far without adding any. + return history.AddParametersAndSnapshot([]); } var executionContext = serviceProvider.GetRequiredService(); @@ -1376,13 +1353,9 @@ private async Task> GetSecretParametersAsync(Ca } } - // Merge this pass's discoveries into the add-only accumulator and return everything seen so far, so a - // secret referenced by an earlier incarnation stays redacted even after a restart re-points the resource. - lock (_secretParametersLock) - { - _accumulatedSecretParameters.UnionWith(secretParameters); - return [.. _accumulatedSecretParameters]; - } + // Merge this pass's discoveries into the add-only history and return everything seen so far, so a secret + // referenced by an earlier incarnation stays redacted even after a restart re-points the resource. + return history.AddParametersAndSnapshot(secretParameters); } private static ResourceSnapshotCommandArgument CreateCommandArgument(InteractionInput input) diff --git a/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs b/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs new file mode 100644 index 00000000000..06812d19d22 --- /dev/null +++ b/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs @@ -0,0 +1,77 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +using Aspire.Hosting.ApplicationModel; + +namespace Aspire.Hosting.Backchannel; + +/// +/// AppHost-scoped, add-only history of the secret parameters and resolved secret values that any +/// aspire describe/watch backchannel connection has ever observed, so they stay redacted from data +/// sent to clients (https://github.com/microsoft/aspire/issues/19241). +/// +/// +/// Registered as a singleton for the lifetime of the AppHost and shared by every +/// (one is created per connection). The redaction set must outlive an +/// individual connection: a resource's secret can change while the app runs, and a snapshot carrying an older value +/// can be emitted to a client that connected after the change. If each connection tracked history on its +/// own, a freshly connected client's target would start empty and leak that older value, so history is accumulated +/// once per AppHost instead. +/// +/// Both sets only ever grow: +/// +/// +/// +/// The parameter set grows because when DCP restarts a resource it forgets and re-evaluates the resource's callbacks +/// (DcpExecutor.ForgetCachedCallbackResults), which can swap which secret a resource references. A +/// still-in-flight snapshot from the prior incarnation can carry the old secret, so we keep trying to resolve every +/// secret parameter ever observed rather than only the current pass's set. +/// +/// +/// The value set grows because a parameter's resolved value can be replaced in place: the runtime "Set parameter" +/// path swaps a completed for 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 we must keep +/// redacting every secret string ever resolved or that old value would be emitted in plaintext. +/// +/// +/// +/// This narrows but does not fully close a cold-start residual: a value assigned and then reassigned before any +/// connection ever observed it is not in the history. In practice the always-on dashboard/CLI watch keeps a +/// connection open from startup, so the history is populated continuously. +/// +/// +internal sealed class SecretRedactionHistory +{ + // Collected by reference: parameter resources referenced by annotations are not registered in the model and so + // are not subject to its unique-name constraint, and distinct same-named secrets must be preserved. + private readonly HashSet _parameters = new(ReferenceEqualityComparer.Instance); + private readonly HashSet _values = new(StringComparer.Ordinal); + private readonly object _lock = new(); + + /// + /// Merges into the history and returns a snapshot of every secret parameter seen so + /// far. Pass an empty sequence to read the current snapshot without adding. + /// + public IReadOnlyList AddParametersAndSnapshot(IEnumerable parameters) + { + lock (_lock) + { + _parameters.UnionWith(parameters); + return [.. _parameters]; + } + } + + /// + /// Merges into the history and returns a fresh snapshot of every resolved secret value + /// seen so far, for membership testing while redacting. + /// + public HashSet AddValuesAndSnapshot(IEnumerable values) + { + lock (_lock) + { + _values.UnionWith(values); + return new HashSet(_values, StringComparer.Ordinal); + } + } +} diff --git a/src/Aspire.Hosting/DistributedApplicationBuilder.cs b/src/Aspire.Hosting/DistributedApplicationBuilder.cs index 20f7fad0aa8..57c6abfdc3f 100644 --- a/src/Aspire.Hosting/DistributedApplicationBuilder.cs +++ b/src/Aspire.Hosting/DistributedApplicationBuilder.cs @@ -425,6 +425,9 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options) _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddHostedService(sp => sp.GetRequiredService()); + // Shared by every per-connection AuxiliaryBackchannelRpcTarget so the describe/watch secret redaction set + // outlives an individual connection (https://github.com/microsoft/aspire/issues/19241). + _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddSingleton(); diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index 4c7b468fe73..b3c380a0794 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -704,6 +704,82 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RetainsPreviousSecretValue_AcrossSeparateConnections() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Regression test for the AppHost-scope gap the reviewer identified: every backchannel connection gets its + // own AuxiliaryBackchannelRpcTarget (AuxiliaryBackchannelService.HandleClientConnectionAsync). If the + // redaction history lived on the target, a client that connected AFTER a secret's value was replaced would + // start with an empty set and leak the previous value carried by a lagging snapshot. The history is + // AppHost-scoped (the SecretRedactionHistory singleton) and shared by every target, so a value one + // connection observed stays redacted for a later, independently constructed connection. + var secret = new ParameterResource("secret", _ => "value-a", secret: true); + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secret.WaitForValueTcs.SetResult("value-a"); + + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = secret); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + // First connection observes value-a and records it in the shared, AppHost-scoped history. + var firstConnection = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var firstResult = await firstConnection.GetResourceSnapshotsAsync().DefaultTimeout(); + var firstSnapshot = Assert.Single(firstResult, r => r.Name == "owner"); + Assert.Null(Assert.Single(firstSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + // The runtime replaces the parameter's resolved value with value-b (as SetParameterValue does by swapping + // the completed WaitForValueTcs), but a lagging snapshot still carries value-a. + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + secret.WaitForValueTcs.SetResult("value-b"); + + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + // A second, independently constructed connection that never observed value-a itself must still redact it, + // because the redaction history is shared across connections for the life of the AppHost. + var secondConnection = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var secondResult = await secondConnection.GetResourceSnapshotsAsync().DefaultTimeout(); + var secondSnapshot = Assert.Single(secondResult, r => r.Name == "owner"); + + // value-a must still be redacted. With per-connection history the fresh connection would start empty and + // resolve only value-b, so the stale value-a would be emitted in plaintext. + Assert.Null(Assert.Single(secondSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotInvokeUncachedResourceCallback_AndLeavesItEvaluable() { From e0eb89c7941204a80fb1397ef422e8c5f4260319 Mon Sep 17 00:00:00 2001 From: David Pine Date: Fri, 14 Aug 2026 10:36:41 -0500 Subject: [PATCH 8/8] Bind describe/watch secret redaction to each snapshot and record at assignment 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> --- .../AuxiliaryBackchannelRpcTarget.cs | 47 +++-- .../Backchannel/SecretRedactionHistory.cs | 21 ++- .../DistributedApplicationBuilder.cs | 11 +- .../Orchestrator/ParameterProcessor.cs | 26 +++ .../AuxiliaryBackchannelRpcTargetTests.cs | 164 ++++++++++++++++-- .../Orchestrator/ParameterProcessorTests.cs | 57 +++++- 6 files changed, 281 insertions(+), 45 deletions(-) diff --git a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs index b8de3d55321..8ec3a73ac31 100644 --- a/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs +++ b/src/Aspire.Hosting/Backchannel/AuxiliaryBackchannelRpcTarget.cs @@ -973,10 +973,6 @@ private async Task> GetResourceSnapshotsAsync(bool resour var notificationService = serviceProvider.GetRequiredService(); var results = new List(); - // This is a point-in-time batch, so the set of resolved secret values is identical for - // every resource. Compute it once here rather than once per resource. - var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); - // Get current state for each resource directly using TryGetCurrentState foreach (var resource in appModel.Resources) { @@ -992,7 +988,12 @@ async Task AddResult(string resourceName) { if (notificationService.TryGetCurrentState(resourceName, out var resourceEvent)) { - var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); + // The secret redaction set is resolved per snapshot inside CreateResourceSnapshotFromEventAsync, + // not once for the whole batch. Building each snapshot can await MCP tool discovery for up to + // s_mcpDiscoveryTimeout, and a parameter can resolve during that window, so a set computed once up + // front could miss a secret that a later resource's snapshot already carries and leak it in + // plaintext. Resolving per snapshot keeps redaction bound to each snapshot. + var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, cancellationToken).ConfigureAwait(false); if (snapshot is not null) { results.Add(snapshot); @@ -1024,20 +1025,10 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( await foreach (var resourceEvent in resourceEvents.WithCancellation(cancellationToken).ConfigureAwait(false)) { - // Recompute the secret set for every event. Three things can change while a watch is open, and all - // must be reflected or a secret can be emitted in plaintext: - // 1. A parameter's value can be resolved after the watch starts (e.g. interactive entry). - // 2. The set of secret parameters a resource references can change across a restart — DCP - // clears and re-evaluates environment/argument callbacks on restart (see - // DcpExecutor.ForgetCachedCallbackResults). Peek-only discovery re-runs here and the redaction - // set only ever grows (see GetSecretParametersAsync), so a newly referenced secret is picked up - // while a secret referenced by a prior incarnation — which a lagging snapshot may still carry — - // stays redacted. - // 3. A parameter's resolved value can be replaced in place (the runtime "Set parameter" path), so - // resolved secret strings are also accumulated add-only (see GetResolvedSecretParameterValuesAsync) - // to keep redacting a prior value that a still-current snapshot may carry. - var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); - var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, secretParameterValues, cancellationToken).ConfigureAwait(false); + // The secret redaction set is resolved per event inside CreateResourceSnapshotFromEventAsync, so a + // secret resolved (or replaced, or newly referenced after a resource restart) while the watch is open + // is reflected on later events rather than fixed to when the watch started. + var snapshot = await CreateResourceSnapshotFromEventAsync(resourceEvent, resourcePropertiesAsJson, cancellationToken).ConfigureAwait(false); if (snapshot is not null) { yield return snapshot; @@ -1048,7 +1039,6 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( private async Task CreateResourceSnapshotFromEventAsync( ResourceEvent resourceEvent, bool resourcePropertiesAsJson, - HashSet secretParameterValues, CancellationToken cancellationToken) { var resource = resourceEvent.Resource; @@ -1121,11 +1111,17 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( }) .ToArray(); + // Resolve the secret redaction set for THIS snapshot, after the MCP discovery await above. The snapshot's + // environment values were captured before that await, so any secret they carry was already resolved by the + // time we get here. Resolving now (rather than once for a whole describe batch) keeps redaction bound to + // the snapshot even though building each snapshot can block on MCP discovery for up to s_mcpDiscoveryTimeout, + // during which another parameter can resolve. The redaction history is add-only and AppHost-scoped (see + // GetResolvedSecretParameterValuesAsync), so resolving per snapshot only ever grows the set and also keeps + // redacting a secret whose value has since been replaced or whose owning resource has restarted. + var secretParameterValues = await GetResolvedSecretParameterValuesAsync(cancellationToken).ConfigureAwait(false); + // Build environment variables. Values that match a secret parameter's value are // redacted so secrets don't leak through clients (e.g. aspire describe --format json). - // The secret values are computed by the caller: once per batch for the one-shot - // GetResourceSnapshotsAsync, but per event for the streaming WatchResourceSnapshotsAsync - // so that secrets resolved mid-stream are still redacted. var environmentVariables = snapshot.EnvironmentVariables .Select(e => new ResourceSnapshotEnvironmentVariable { @@ -1223,8 +1219,9 @@ private async IAsyncEnumerable WatchResourceSnapshotsAsync( /// Collects the resolved values of secret parameters reachable from the application model so they can be /// redacted from data sent to clients. Only values that have already been resolved are included; this never /// blocks waiting for interactive parameter resolution. Resolved values are accumulated add-only in the - /// AppHost-scoped , so a value a parameter has since been reassigned away - /// from stays redacted while an older snapshot can still carry it. + /// AppHost-scoped (which ParameterProcessor also populates at + /// assignment time), so a value a parameter has since been reassigned away from stays redacted while an older + /// snapshot can still carry it. /// private async Task> GetResolvedSecretParameterValuesAsync(CancellationToken cancellationToken) { diff --git a/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs b/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs index 06812d19d22..cf27883948a 100644 --- a/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs +++ b/src/Aspire.Hosting/Backchannel/SecretRedactionHistory.cs @@ -36,9 +36,11 @@ namespace Aspire.Hosting.Backchannel; /// /// /// -/// This narrows but does not fully close a cold-start residual: a value assigned and then reassigned before any -/// connection ever observed it is not in the history. In practice the always-on dashboard/CLI watch keeps a -/// connection open from startup, so the history is populated continuously. +/// A backchannel connection only observes a secret value once it is open, so a value assigned and then reassigned +/// before the first connection would otherwise be absent from the history and leak from a lagging snapshot. To close +/// that cold-start residual, ParameterProcessor also records secret values into this history at the moment it +/// assigns or replaces them (see its SecretRedactionHistory wiring), independent of any connection. Values +/// that never flow through the parameter processor still rely on connection-time observation. /// /// internal sealed class SecretRedactionHistory @@ -74,4 +76,17 @@ public HashSet AddValuesAndSnapshot(IEnumerable values) return new HashSet(_values, StringComparer.Ordinal); } } + + /// + /// Merges into the history without allocating a snapshot. Used to record secret values + /// as they are assigned (see ParameterProcessor), where the caller only writes and never needs to read the + /// set back. + /// + public void AddValues(IEnumerable values) + { + lock (_lock) + { + _values.UnionWith(values); + } + } } diff --git a/src/Aspire.Hosting/DistributedApplicationBuilder.cs b/src/Aspire.Hosting/DistributedApplicationBuilder.cs index 57c6abfdc3f..9d4af0aad9f 100644 --- a/src/Aspire.Hosting/DistributedApplicationBuilder.cs +++ b/src/Aspire.Hosting/DistributedApplicationBuilder.cs @@ -382,7 +382,16 @@ public DistributedApplicationBuilder(DistributedApplicationOptions options) _innerBuilder.Services.TryAddSingleton(); _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddSingleton(sp => sp.GetRequiredService()); - _innerBuilder.Services.AddSingleton(); + _innerBuilder.Services.AddSingleton(static sp => + { + var parameterProcessor = ActivatorUtilities.CreateInstance(sp); + // Wire the AppHost-scoped redaction history after construction (not through the public constructor) so + // the processor records resolved secret values as they are assigned/replaced. This populates the + // describe/watch redaction set from startup, independent of any backchannel connection, while keeping + // ParameterProcessor's public constructor unchanged (https://github.com/microsoft/aspire/issues/19241). + parameterProcessor.SecretRedactionHistory = sp.GetRequiredService(); + return parameterProcessor; + }); _innerBuilder.Services.AddSingleton(Eventing); _innerBuilder.Services.AddSingleton(); _innerBuilder.Services.AddHealthChecks(); diff --git a/src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs b/src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs index 6671aa9ce39..34907d1411d 100644 --- a/src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs +++ b/src/Aspire.Hosting/Orchestrator/ParameterProcessor.cs @@ -6,6 +6,7 @@ using System.Globalization; using Aspire.Hosting.ApplicationModel; +using Aspire.Hosting.Backchannel; using Aspire.Hosting.Pipelines; using Aspire.Hosting.Resources; using Microsoft.Extensions.Logging; @@ -33,6 +34,15 @@ public sealed class ParameterProcessor( private CancellationTokenSource? _allParametersResolvedCts; private Task? _parameterResolutionTask; + /// + /// AppHost-scoped history that records resolved secret parameter values so aspire describe/watch + /// can redact them. Assigned by DI after construction (see DistributedApplicationBuilder) rather than + /// injected through the public constructor, to keep the public API surface unchanged for backporting. Null when + /// the processor is created outside the AppHost container (e.g. in unit tests), in which case recording is a + /// no-op. + /// + internal SecretRedactionHistory? SecretRedactionHistory { get; set; } + /// /// Initializes parameter resources and handles unresolved parameters if interaction service is available. /// @@ -154,6 +164,7 @@ private async Task ProcessParameterAsync(ParameterResource parameterResource) await UpdateParameterStateAsync(parameterResource, value, KnownResourceStates.Running).ConfigureAwait(false); parameterResource.WaitForValueTcs?.TrySetResult(value); + RecordSecretValueForRedaction(parameterResource, value); } catch (Exception ex) { @@ -494,6 +505,7 @@ private async Task ApplyParameterValueAsync(ParameterResource parameterResource, } parameterResource.WaitForValueTcs?.TrySetResult(inputValue); + RecordSecretValueForRedaction(parameterResource, inputValue); await UpdateParameterStateAsync(parameterResource, inputValue, KnownResourceStates.Running).ConfigureAwait(false); @@ -518,6 +530,20 @@ private async Task ApplyParameterValueAsync(ParameterResource parameterResource, } } + // Record a resolved secret value into the AppHost-scoped redaction history at the moment it is assigned or + // replaced, so `aspire describe`/`watch` redacts it even before any backchannel connection has peeked it. The + // describe path only observes a secret once a connection is open; a value assigned (and possibly replaced) before + // the first connection would otherwise be absent from the history and leak from a lagging snapshot + // (https://github.com/microsoft/aspire/issues/19241). No-op for non-secret parameters, empty values, or when no + // history is wired (e.g. unit tests that construct the processor directly). + private void RecordSecretValueForRedaction(ParameterResource parameterResource, string? value) + { + if (parameterResource.Secret && value is { Length: > 0 } && SecretRedactionHistory is { } history) + { + history.AddValues([value]); + } + } + // Internal for testing purposes - allows passing specific parameters to test. internal async Task HandleUnresolvedParametersAsync(IList unresolvedParameters, CancellationToken cancellationToken) { diff --git a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs index b3c380a0794..124ab1c2ea1 100644 --- a/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs +++ b/tests/Aspire.Hosting.Tests/Backchannel/AuxiliaryBackchannelRpcTargetTests.cs @@ -780,6 +780,127 @@ await notificationService.PublishUpdateAsync(owner.Resource, s => s with await app.StopAsync().DefaultTimeout(); } + [Fact] + public async Task GetResourceSnapshotsAsync_RedactsSecretResolvedDuringAnotherResourceMcpDiscoveryWindow() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Regression test for the per-snapshot concern the reviewer identified: building a resource's snapshot can + // await MCP tool discovery for up to s_mcpDiscoveryTimeout, and a parameter can resolve during that window. + // If the redaction set is computed once for the whole describe batch (before the resource loop), a value + // resolved mid-loop is missing from it, so a LATER resource whose snapshot already carries that value leaks + // it in plaintext. Resolving the redaction set per snapshot (after the MCP await) closes the window. + var secret = new ParameterResource("mcp-window-secret", _ => "unused", secret: true); + + // The first resource exposes an MCP endpoint. Its resolver is awaited while its snapshot is built, and here + // it deterministically resolves the secret — standing in for a parameter that happens to resolve during the + // real (up to 5s) MCP discovery window. Returning null skips the network TryListToolsAsync call. + var mcpResource = builder.AddResource(new CustomResourceWithEndpoints("mcp")) + .WithAnnotation(new McpServerEndpointAnnotation((resource, cancellationToken) => + { + secret.WaitForValueTcs!.TrySetResult("leaked-during-mcp"); + return Task.FromResult(null); + })); + + // The second resource owns an environment variable carrying the secret's resolved value. It is registered + // AFTER the MCP resource so its snapshot is built after the MCP resolver has run. + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = secret); + + using var app = builder.Build(); + await app.StartAsync().DefaultTimeout(); + + // Start the secret UNRESOLVED; the MCP resolver completes it mid-loop while the "owner" snapshot is still to + // be built. (A fresh uncompleted TCS also discards any startup resolution of the referenced parameter.) + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(mcpResource.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success) + }).DefaultTimeout(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "leaked-during-mcp", true) + ] + }).DefaultTimeout(); + + var target = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var result = await target.GetResourceSnapshotsAsync().DefaultTimeout(); + + var ownerSnapshot = Assert.Single(result, r => r.Name == "owner"); + + // The owner's snapshot is built after the MCP resource resolved the secret. A per-snapshot redaction set + // observes the now-resolved value and redacts it; a batch-wide set computed before the loop would not. + Assert.Null(Assert.Single(ownerSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + + [Fact] + public async Task GetResourceSnapshotsAsync_RedactsSecretReplacedBeforeFirstConnection() + { + using var builder = TestDistributedApplicationBuilder.Create(outputHelper); + + // Regression test for the cold-start residual the reviewer identified: a backchannel connection only observes + // a secret once it is open. A value assigned at startup and then replaced before the FIRST connection would, + // with connection-time observation alone, be absent from the redaction history — a fresh connection peeking + // the current value would find only the replacement and leak the original from a lagging snapshot. The + // parameter processor records secret values at assignment time (see its SecretRedactionHistory wiring), so + // the original value is in the history from startup, independent of any connection. + var coldSecret = builder.AddParameter("cold-secret", "value-a", secret: true); + var owner = builder.AddResource(new CustomResourceWithEnvironment("owner")) + .WithEnvironment(context => context.EnvironmentVariables["SECRET"] = coldSecret.Resource); + + using var app = builder.Build(); + + // Startup resolves the parameter to value-a; assignment-time recording adds value-a to the AppHost-scoped + // redaction history before any backchannel connection exists. + await app.StartAsync().DefaultTimeout(); + + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); + + // The value is replaced with value-b in place (as the runtime "Set parameter" path does). Peek-only discovery + // on a first-ever connection would now resolve only value-b, so value-a can only stay redacted via the + // assignment-time record captured at startup. + coldSecret.Resource.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + coldSecret.Resource.WaitForValueTcs.SetResult("value-b"); + + var notificationService = app.Services.GetRequiredService(); + await notificationService.PublishUpdateAsync(owner.Resource, s => s with + { + State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), + EnvironmentVariables = + [ + new EnvironmentVariableSnapshot("SECRET", "value-a", true) + ] + }).DefaultTimeout(); + + // A brand-new connection that never observed value-a must still redact it. + var firstConnection = new AuxiliaryBackchannelRpcTarget( + NullLogger.Instance, + app.Services.GetRequiredService(), + app.Services.GetRequiredService(), + app.Services); + + var result = await firstConnection.GetResourceSnapshotsAsync().DefaultTimeout(); + var ownerSnapshot = Assert.Single(result, r => r.Name == "owner"); + + Assert.Null(Assert.Single(ownerSnapshot.EnvironmentVariables, e => e.Name == "SECRET").Value); + + await app.StopAsync().DefaultTimeout(); + } + [Fact] public async Task GetResourceSnapshotsAsync_DoesNotInvokeUncachedResourceCallback_AndLeavesItEvaluable() { @@ -890,19 +1011,26 @@ public async Task GetResourceSnapshotsAsync_DoesNotBlockOnUnresolvedSecretParame { using var builder = TestDistributedApplicationBuilder.Create(outputHelper); - var secret = builder.AddParameter("dbpassword", "s3cr3t-value", secret: true); - var custom = builder.AddResource(new CustomResource("myresource")); + // A secret parameter that is only referenced by a resource (never registered as a top-level parameter) so it + // is not resolved — and therefore not recorded for redaction — during run-mode startup. This lets the test + // exercise a genuinely unresolved secret at describe time. + var secret = new ParameterResource("dbpassword", _ => "s3cr3t-value", secret: true); + var owner = builder.AddResource(new CustomResourceWithEnvironment("myresource")) + .WithEnvironment(context => context.EnvironmentVariables["DB_PASSWORD"] = secret); using var app = builder.Build(); await app.StartAsync().DefaultTimeout(); - // Simulate a secret parameter whose value has not been resolved yet by replacing its completion - // source with one that never completes. GetResolvedSecretParameterValues must peek (not await) the - // task, so an unresolved secret cannot block the snapshot call. - secret.Resource.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + // Leave the secret unresolved: its completion source never completes. GetResolvedSecretParameterValues must + // peek (not await) the task, so an unresolved secret cannot block the snapshot call. + secret.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + // Cache the env callback the way DCP does on start, so peek-only discovery can observe (and then skip) the + // unresolved secret reference. + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); var notificationService = app.Services.GetRequiredService(); - await notificationService.PublishUpdateAsync(custom.Resource, s => s with + await notificationService.PublishUpdateAsync(owner.Resource, s => s with { State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), EnvironmentVariables = @@ -924,7 +1052,7 @@ await notificationService.PublishUpdateAsync(custom.Resource, s => s with var snapshot = Assert.Single(result, r => r.Name == "myresource"); var dbPassword = Assert.Single(snapshot.EnvironmentVariables, e => e.Name == "DB_PASSWORD"); - // Because the secret value was not resolved, it is not part of the redaction set: the unresolved + // Because the secret value was never resolved, it is not part of the redaction set: the unresolved // secret is skipped rather than awaited. Once resolved it is redacted (see the streaming test). Assert.Equal("s3cr3t-value", dbPassword.Value); @@ -936,15 +1064,21 @@ public async Task WatchResourceSnapshotsAsync_RedactsSecretResolvedAfterWatchSta { using var builder = TestDistributedApplicationBuilder.Create(outputHelper); - var secret = builder.AddParameter("dbpassword", "s3cr3t-value", secret: true); - var custom = builder.AddResource(new CustomResource("myresource")); + // A secret parameter referenced by the resource (not a top-level parameter), so it is not resolved or + // recorded for redaction during run-mode startup and the watch can begin with it genuinely unresolved. + var secret = new ParameterResource("dbpassword", _ => "s3cr3t-value", secret: true); + var owner = builder.AddResource(new CustomResourceWithEnvironment("myresource")) + .WithEnvironment(context => context.EnvironmentVariables["DB_PASSWORD"] = secret); using var app = builder.Build(); await app.StartAsync().DefaultTimeout(); // Begin with the secret unresolved so the watch starts before the value is known. var waitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); - secret.Resource.WaitForValueTcs = waitForValueTcs; + secret.WaitForValueTcs = waitForValueTcs; + + // Cache the env callback the way DCP does on start, so peek-only discovery can observe the secret reference. + await PrimeEnvironmentCallbackCacheAsync(owner.Resource, app.Services).DefaultTimeout(); var notificationService = app.Services.GetRequiredService(); @@ -959,7 +1093,7 @@ public async Task WatchResourceSnapshotsAsync_RedactsSecretResolvedAfterWatchSta try { // Phase 1: secret unresolved. The env var value is not redacted because the secret value is unknown. - await notificationService.PublishUpdateAsync(custom.Resource, s => s with + await notificationService.PublishUpdateAsync(owner.Resource, s => s with { State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), EnvironmentVariables = @@ -975,7 +1109,7 @@ await notificationService.PublishUpdateAsync(custom.Resource, s => s with // Resolve the secret mid-stream, then push a new event whose env var now matches the secret value. waitForValueTcs.SetResult("s3cr3t-value"); - await notificationService.PublishUpdateAsync(custom.Resource, s => s with + await notificationService.PublishUpdateAsync(owner.Resource, s => s with { State = new ResourceStateSnapshot("Running", KnownResourceStateStyles.Success), EnvironmentVariables = @@ -1462,6 +1596,10 @@ private sealed class CustomResourceWithEnvironment(string name) : Resource(name) { } + private sealed class CustomResourceWithEndpoints(string name) : Resource(name), IResourceWithEndpoints + { + } + // Synthesise per-replica terminal layouts directly rather than going through the public // WithTerminal() path so the test stays focused on backchannel snapshot stamping and doesn't // depend on real DCP terminal-host provisioning. Mirrors DashboardServiceDataTerminalTests. diff --git a/tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs b/tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs index 3049f21331e..762e4cc647b 100644 --- a/tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs +++ b/tests/Aspire.Hosting.Tests/Orchestrator/ParameterProcessorTests.cs @@ -4,6 +4,7 @@ using System.Globalization; using System.Text.Json.Nodes; using Aspire.Dashboard.Model; +using Aspire.Hosting.Backchannel; using Aspire.Hosting.Pipelines; using Aspire.Hosting.Pipelines.Internal; using Aspire.Hosting.Resources; @@ -1174,6 +1175,50 @@ public async Task SetParameterAsync_WithUserInput_UpdatesParameterValueAndSavedS Assert.Equal("newValue", savedValueNode?.GetValue()); } + [Fact] + public async Task InitializeParametersAsync_RecordsResolvedSecretValues_ForRedaction() + { + // Assignment-time recording (https://github.com/microsoft/aspire/issues/19241): the processor records a + // secret's resolved value into the AppHost-scoped redaction history the moment it assigns it, so + // aspire describe/watch can redact it even before any backchannel connection has observed the value. + var redactionHistory = new SecretRedactionHistory(); + var parameterProcessor = CreateParameterProcessor(secretRedactionHistory: redactionHistory); + + var secretParam = CreateParameterResource("db-password", "s3cr3t-value", secret: true); + var nonSecretParam = CreateParameterResource("region", "public-value", secret: false); + + await parameterProcessor.InitializeParametersAsync([secretParam, nonSecretParam], waitForResolution: true).DefaultTimeout(); + + // Only the secret value is recorded; the non-secret value must never enter the redaction set (asserting a + // single element proves "public-value" was not recorded). + var recorded = redactionHistory.AddValuesAndSnapshot([]); + Assert.Equal("s3cr3t-value", Assert.Single(recorded)); + } + + [Fact] + public async Task SetParameterCoreAsync_RecordsReplacedSecretValue_ForRedaction() + { + // The runtime "Set parameter" path replaces a secret's resolved value in place. Both the original and the + // replacement value must stay redactable, because a still-current or lagging snapshot can carry the previous + // value. Recording at assignment time accumulates both add-only, independent of any backchannel connection. + var redactionHistory = new SecretRedactionHistory(); + var parameterProcessor = CreateParameterProcessor(secretRedactionHistory: redactionHistory); + + var secretParam = CreateParameterResource("db-password", "initial-secret", secret: true); + + await parameterProcessor.InitializeParametersAsync([secretParam], waitForResolution: true).DefaultTimeout(); + + // Recreate the completed TCS the way the runtime does before replacing a resolved value. + secretParam.WaitForValueTcs = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); + + var result = await parameterProcessor.SetParameterCoreAsync(secretParam, CreateSetParameterArguments("replacement-secret"), CancellationToken.None).DefaultTimeout(); + Assert.True(result.Success); + + var recorded = redactionHistory.AddValuesAndSnapshot([]); + Assert.Contains("initial-secret", recorded); + Assert.Contains("replacement-secret", recorded); + } + private static InteractionInputCollection CreateSetParameterArguments(string? value, string? saveToUserSecrets = null) { return new InteractionInputCollection([ @@ -1212,7 +1257,8 @@ private static ParameterProcessor CreateParameterProcessor( bool disableDashboard = true, DistributedApplicationExecutionContext? executionContext = null, IDeploymentStateManager? deploymentStateManager = null, - IUserSecretsManager? userSecretsManager = null) + IUserSecretsManager? userSecretsManager = null, + SecretRedactionHistory? secretRedactionHistory = null) { return new ParameterProcessor( notificationService ?? ResourceNotificationServiceTestHelpers.Create(), @@ -1221,8 +1267,13 @@ private static ParameterProcessor CreateParameterProcessor( logger ?? new NullLogger(), executionContext ?? new DistributedApplicationExecutionContext(DistributedApplicationOperation.Run), deploymentStateManager ?? new MockDeploymentStateManager(), - userSecretsManager ?? UserSecrets.NoopUserSecretsManager.Instance - ); + userSecretsManager ?? UserSecrets.NoopUserSecretsManager.Instance) + { + // Mirror the DI wiring in DistributedApplicationBuilder so tests can observe assignment-time secret + // recording. Left null by default (property is a no-op then), matching a processor created outside the + // AppHost container. + SecretRedactionHistory = secretRedactionHistory + }; } private static InteractionService CreateInteractionService(bool disableDashboard = false)