dotnet / dotnet/aspnetcore

Certificate validation cache key is not stable across application event callbacks

Open
#69,273 1 comment 0 reactions 0 assignees View on GitHub
area-auth
Dominant language
C#
Stars
38.4k
Forks
10.9k
Avg merge
2d 10h
Merged PRs (30d)
281

Description

## Summary

`CertificateValidationCache` derives the scheme component of its cache key from mutable per-request state that is read at call time. `CertificateAuthenticationHandler` establishes that state before reading the cache but does not re-establish it before writing, and between those two points it awaits application-supplied event callbacks. The key used to store a result is therefore not guaranteed to identify the scheme that produced it.

## What is wrong

The invariant introduced by #66673 is that a result stored in the certificate validation cache is keyed by the authentication scheme that produced it, so one scheme's cached result is never returned for another scheme's lookup. That invariant is established on the read path but not on the write path.

* The scheme name reaches the cache out of band, through the single-slot per-request dictionary entry `HttpContext.Items[CertificateAuthenticationHandler.CertificateSchemeCacheKeyItem]`, rather than as a parameter. `ICertificateValidationCache.Get` and `Put` accept only `(HttpContext, X509Certificate2)`.
* `CertificateValidationCache.ComputeKey` reads that slot at call time, so the key is late-bound on both the read and the write.
* `CertificateAuthenticationHandler.HandleAuthenticateAsync` writes the slot, reads the cache, then awaits `Events.CertificateValidated` and — on the non-exception failure path — `Events.AuthenticationFailed`, before writing the cache. The scheme is never captured into a local and never re-asserted, and no `try`/`finally` restores the slot.
* Handler instances, and the `AuthenticationHandler._authenticateTask` idempotence guard, are scoped per (request × scheme name). The `Items` slot is scoped per request and shared by every certificate scheme. A per-scheme guard cannot protect a per-request single-slot channel, so any code that runs a second certificate scheme's handler inside those callbacks leaves the slot holding a different scheme name for the remainder of the request.

Stated as a property: the key derivation is stable across the read but not across the write, so the store operation can associate a result with a scheme other than the one that produced it.

Two secondary observations worth addressing in the same work item:

* `AddCertificateCache` registers `ICertificateValidationCache` as an application-wide singleton. Neither `CertificateAuthenticationOptions` nor `CertificateValidationCacheOptions` (configured unnamed) offers a per-scheme opt-out, so an application that registers two certificate schemes necessarily shares one key space. There is no supported configuration that avoids it.
* The `ICertificateValidationCache` contract exposes no scheme parameter. A third-party implementation either depends on an `internal const` it cannot reference, or keys on the certificate alone — which does not satisfy the isolation invariant at all. The #66673 fix lives in the in-box implementation rather than in the contract.

## Why it matters (defense in depth)

* Correctness, independent of any actor: a scheme can observe a cached result it did not produce, skipping its own chain build, revocation check, and `CertificateValidated` callback. Claims minted by one scheme's callback can surface under another scheme's identity.
* `AuthenticateResult.Clone()` preserves `Ticket.AuthenticationScheme`, and nothing downstream compares it against the requested scheme, so the inconsistency is neither detected nor corrected later in the pipeline. `ClaimsIdentity.AuthenticationType` is the same constant for every certificate scheme, so it cannot distinguish them either.
* The write is unconditional for success, failure, and no-result outcomes, so a misattributed entry can suppress a scheme's own validation in either direction, for the cache lifetime.
* Hardening value: this closes the remaining gap in the scheme-isolation boundary that #66673 set out to establish, and makes that boundary hold regardless of what application code does inside the event callbacks.

## Affected code

* `src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs:16` — `CertificateSchemeCacheKeyItem`, the out-of-band channel; `internal const`, so applications cannot participate in maintaining it.
* `src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs:70-78` — sole write of the slot, immediately followed by `Get`. Note the write also occurs on the cache-hit return path.
* `src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs:80-90` — the two awaited application callbacks that sit between the write and the read-back.
* `src/Security/Authentication/Certificate/src/CertificateAuthenticationHandler.cs:92` — `Put`, whose key is recomputed from the slot.
* `src/Security/Authentication/Certificate/src/CertificateValidationCache.cs:40-48` — `Get`.
* `src/Security/Authentication/Certificate/src/CertificateValidationCache.cs:56-75` — `Put`, unconditional for all result kinds.
* `src/Security/Authentication/Certificate/src/CertificateValidationCache.cs:77-85` — `ComputeKey`, the late-bound read of the slot.
* `src/Security/Authentication/Certificate/src/ICertificateValidationCache.cs` — contract with no scheme parameter.
* `src/Security/Authentication/test/CertificateTests.cs:938-1060` — `VerifyCacheIsIsolatedAcrossSchemes` and `VerifyCacheNoOpsWithoutSchemeInHttpContextItems`; both exercise sequential requests only, so the re-entrant path is uncovered.

## Recommended fix

Capture `Scheme.Name` into a local before any `await`, and re-establish the slot immediately before `Put` so the key is derived from state the handler controls rather than from state application code may have changed. This is a minimal, non-breaking change confined to one method.

```csharp
protected override async Task HandleAuthenticateAsync()
{
// You only get client certificates over HTTPS
if (!Context.Request.IsHttps)
{
Logger.NotHttps();
return AuthenticateResult.NoResult();
}

try
{
var clientCertificate = await Context.Connection.GetClientCertificateAsync();

// This should never be the case, as cert authentication happens long before ASP.NET kicks in.
if (clientCertificate == null)
{
Logger.NoCertificate();
return AuthenticateResult.NoResult();
}

// Event callbacks may run another scheme's handler, which overwrites the shared marker.
var schemeName = Scheme.Name;

if (_cache != null)
{
Context.Items[CertificateSchemeCacheKeyItem] = schemeName;
var cacheHit = _cache.Get(Context, clientCertificate);
if (cacheHit != null)
{
return cacheHit;
}
}

var result = await ValidateCertificateAsync(clientCertificate);

// Invoke the failed handler if validation failed, before updating the cache
if (result.Failure != null)
{
var authenticationFailedContext = await HandleFailureAsync(result.Failure);
if (authenticationFailedContext.Result != null)
{
result = authenticationFailedContext.Result;
}
}

if (_cache != null)
{
Context.Items[CertificateSchemeCacheKeyItem] = schemeName;
_cache.Put(Context, clientCertificate, result);
}

return result;
}
catch (Exception ex)
{
var authenticationFailedContext = await HandleFailureAsync(ex);
if (authenticationFailedContext.Result != null)
{
return authenticationFailedContext.Result;
}

throw;
}
}
```

The `catch` path needs no change: it has two terminal exits and no fall-through to `Put`, so no cache write occurs when an exception propagates.

### Alternatives considered

* **Add a scheme parameter to `ICertificateValidationCache`.** Structurally correct — it removes the out-of-band channel entirely and lets third-party implementations honour the invariant. Rejected for servicing because `ICertificateValidationCache` is public, so this is a breaking change. Worth considering for `main` as a follow-up, optionally as a default interface method that forwards to the existing overload.
* **`try`/`finally` around the callbacks to restore the slot.** Equivalent for this defect and additionally leaves the slot consistent for the rest of the pipeline, but broader in scope than the property being fixed, and the slot has no defined meaning outside the handler.
* **Move the channel to `AsyncLocal`.** Heavier, and the value is genuinely request-scoped, so it would trade one implicit channel for another without removing the late binding.
* **Document the constraint instead of fixing it.** Not viable: the slot is an `internal const` on an `internal sealed` type, so applications cannot save or restore it, and no per-scheme cache opt-out exists. The framework is the only component able to maintain the invariant.

### Compatibility, migration, versioning

* No public API change; `ICertificateValidationCache`, `CertificateValidationCache`, `CertificateAuthenticationOptions`, and `CertificateValidationCacheOptions` are untouched.
* No cache format or key format change, so no invalidation or migration is required. Existing entries remain valid.
* Behaviour changes only where the slot would previously have been observed in a mutated state; all single-scheme and sequential multi-scheme behaviour is unchanged.
* Applies wherever #66673 shipped: `main` plus the servicing branches carrying the scheme-keyed cache. Branches predating #66673 key on the certificate alone and need that change first.
* Consider adding remarks to `ICertificateValidationCache` stating that implementations must treat the certificate alone as an insufficient key when more than one certificate scheme is registered.

## Acceptance criteria

* [ ] A result produced by a given scheme is retrievable only under that scheme's key, regardless of what application code does inside `CertificateValidated` or `AuthenticationFailed`.
* [ ] Regression test: a scheme whose `CertificateValidated` callback triggers authentication for a second certificate scheme registered against the same cache. Assert the outer scheme's result is stored under the outer scheme's key, and that a later lookup by the second scheme does not observe it.
* [ ] Equivalent coverage for the `AuthenticationFailed` callback on the non-exception failure path.
* [ ] Test asserting no cache write occurs when an exception propagates out of a callback.
* [ ] Existing `VerifyCacheIsIsolatedAcrossSchemes` and `VerifyCacheNoOpsWithoutSchemeInHttpContextItems` continue to pass unchanged.
* [ ] Decision recorded on whether `ICertificateValidationCache` gains a scheme parameter in `main`, and whether the interface documentation is updated for third-party implementers.

Contributor guide

Open the contributing guide

Research direction

Start with CertificateAuthenticationHandler.cs, especially HandleAuthenticateAsync and the cache calls around the awaited callbacks, then read CertificateValidationCache.cs and ICertificateValidationCache.cs. Run the existing VerifyCacheIsIsolatedAcrossSchemes and VerifyCacheNoOpsWithoutSchemeInHttpContextItems tests in CertificateTests.cs before adding re-entrant callback coverage. Done means callback-driven nested authentication cannot cause a result to be stored under the wrong scheme, including failure and exception paths.

Written by the indexing model from the issue text.

Assessment

Tech stack
csharp
Domain
authentication, backend-api-design, security
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
72/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.