Add SpiffeHttpHandler with auto-rotating mTLS certificate support by vurhanau · Pull Request #439 · vurhanau/csharp-spiffe

vurhanau · 2026-03-23T00:50:14Z

Summary

Add SpiffeHttpHandler: an HttpMessageHandler that presents the current X.509 SVID for mTLS and automatically swaps its inner SocketsHttpHandler whenever the X509Source receives a rotated certificate
Add Updated event to Source (base class) and BundleSource so consumers can react to credential rotation
Fix TaskCompletionSource construction to use TaskCreationOptions.RunContinuationsAsynchronously across Source and BundleSource
Update samples (ASP.NET Core mTLS, gRPC mTLS docker + local) to use the new SpiffeHttpHandler instead of manual SocketsHttpHandler + SslOptions wiring
Add unit tests for SpiffeHttpHandler covering rotation, disposal, and null-argument guards

Test plan

Build passes (make build)
Tests pass (make test)
Manually verified mTLS sample works with SpiffeHttpHandler

🤖 Generated with Claude Code

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR introduces an auto-rotating HttpMessageHandler for SPIFFE mTLS clients and adds update notifications to Workload API sources so consumers can react to SVID/bundle rotation.

Changes:

Added SpiffeHttpHandler that rebuilds/swaps its inner SocketsHttpHandler when the backing X509Source updates.
Added Updated events to Source and BundleSource, and switched related TaskCompletionSource instances to RunContinuationsAsynchronously.
Updated README and sample clients to use SpiffeHttpHandler instead of manually wiring SocketsHttpHandler.SslOptions.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/Spiffe.Tests/Ssl/TestSpiffeHttpHandler.cs	Adds unit tests for `Updated` and `SpiffeHttpHandler` behaviors.
src/Spiffe/WorkloadApi/Source.cs	Adds `Updated` event and makes TCS continuations async.
src/Spiffe/WorkloadApi/BundleSource.cs	Adds `Updated` event, makes TCS continuations async, raises `Updated` on bundle updates.
src/Spiffe/Ssl/SpiffeHttpHandler.cs	New rotating handler that swaps inner HTTP handler on `X509Source` updates.
samples/local/Spiffe.Sample.Grpc.Mtls/Client/Program.cs	Switches gRPC client to `SpiffeHttpHandler`.
samples/docker/Spiffe.Sample.Grpc.Mtls/Client/Program.cs	Switches gRPC client to `SpiffeHttpHandler`.
samples/docker/Spiffe.Sample.AspNetCore.Mtls/Client/Program.cs	Switches HttpClient creation to `SpiffeHttpHandler`.
README.md	Updates mTLS dialing snippet and documents handler rotation behavior.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-24T00:05:21Z

src/Spiffe/WorkloadApi/BundleSource.cs

        }

        _initializedX509.TrySetResult(true);
+        Updated?.Invoke();
    }


Updated?.Invoke() is executed inline as part of the update path. A throwing subscriber will propagate back through Watcher.OnUpdate, which can be caught as a watch error and trigger reconnect/backoff, disrupting updates. Consider shielding the update path from subscriber exceptions (catch/log per subscriber or similar).

Left as-is intentionally. SpiffeHttpHandler.Refresh() (the only subscriber) does not throw, and adding a silent catch or per-subscriber iteration would hide bugs in future subscribers without a logger to surface them. If a subscriber throws it is a bug in that subscriber — propagating the exception makes it visible rather than swallowing it silently.

Copilot · 2026-03-24T00:05:21Z

src/Spiffe/WorkloadApi/BundleSource.cs

        }

        _initializedJwt.TrySetResult(true);
+        Updated?.Invoke();
    }


Same as above: invoking Updated inline means a subscriber exception can break the watch update flow (and trigger reconnect/backoff). It’s safer if SetJwtBundles can’t be taken down by consumer event handlers.

Same reasoning as above — left as-is intentionally.

Copilot · 2026-03-24T00:05:21Z

src/Spiffe/Ssl/SpiffeHttpHandler.cs

+    public SpiffeHttpHandler(X509Source source, IAuthorizer authorizer, TimeSpan? drainDelay = null)
+    {
+        _source = source ?? throw new ArgumentNullException(nameof(source));
+        _authorizer = authorizer ?? throw new ArgumentNullException(nameof(authorizer));
+        _drainDelay = drainDelay ?? TimeSpan.FromSeconds(30);
+        _inner = CreateInvoker();
+        _source.Updated += Refresh;
+    }


drainDelay is accepted as-is; negative values (other than Timeout.InfiniteTimeSpan) will cause Task.Delay(_drainDelay) to throw during Refresh(), potentially on a Workload API update callback thread. Consider validating/clamping drainDelay in the constructor and throwing an ArgumentOutOfRangeException early.

Fixed in 5b9ef5c. Constructor now validates drainDelay is in [0, 10 minutes] and throws ArgumentOutOfRangeException eagerly. 10 minutes chosen as the upper bound based on common web server and proxy drain/timeout defaults.

Copilot · 2026-03-24T00:05:21Z

src/Spiffe/Ssl/SpiffeHttpHandler.cs

+    private void Refresh()
+    {
+        if (_disposed)
+        {
+            return;
+        }
+
+        HttpMessageInvoker old = Interlocked.Exchange(ref _inner, CreateInvoker());
+        _ = Task.Delay(_drainDelay).ContinueWith(_ => old.Dispose(), TaskScheduler.Default);
+    }


Refresh() can race with Dispose(): it checks _disposed and then creates/swaps _inner. If disposal happens after the check but before/while exchanging, Refresh() can install a new HttpMessageInvoker after disposal, leaking sockets/handlers because the instance is already disposed and will never dispose the newly created invoker. Consider synchronizing Dispose/Refresh (e.g., a lock) or using an interlocked disposed state plus a double-check that disposes the newly created invoker if disposal won the race.

Fixed in 5b9ef5c. Added _lock to synchronize Refresh() and Dispose(). CreateInvoker() runs outside the lock (it is the expensive part); the lock only guards the install-or-discard decision. If Dispose() wins the lock after Refresh() already created a new invoker, that invoker is disposed immediately inside the lock before returning.

Copilot · 2026-03-24T00:05:21Z

tests/Spiffe.Tests/Ssl/TestSpiffeHttpHandler.cs

+    [Fact]
+    public void TestHandlerRefreshesOnSourceUpdate()
+    {
+        using X509Source source = MakeInitializedSource();
+        int refreshCount = 0;
+
+        // Intercept Updated to count refreshes triggered by SpiffeHttpHandler's subscription
+        // We add our own listener alongside SpiffeHttpHandler's internal listener.
+        using SpiffeHttpHandler handler = new(source, Authorizers.AuthorizeAny());
+        source.Updated += () => refreshCount++;
+
+        // Trigger two more updates
+        source.SetX509Context(MakeContext());
+        source.SetX509Context(MakeContext());
+
+        // Our counter saw 2 updates; SpiffeHttpHandler saw the same and refreshed its inner handler
+        refreshCount.Should().Be(2);
+    }


TestHandlerRefreshesOnSourceUpdate only asserts that X509Source.Updated fired; it doesn’t verify that SpiffeHttpHandler actually swapped/rotated its inner handler (the behavior this test name implies). To make this test meaningful, consider adding an observable seam (e.g., internal accessor/injected factory/callback) so the test can assert that the inner handler/invoker instance changes after an update.

Fixed in 5b9ef5c. Added internal CurrentInvoker accessor and the test now asserts the actual invoker instance changes after each update.

Copilot · 2026-03-24T00:05:22Z

tests/Spiffe.Tests/Ssl/TestSpiffeHttpHandler.cs

+    [Fact]
+    public void TestDisposeUnsubscribesFromUpdated()
+    {
+        using X509Source source = MakeInitializedSource();
+        SpiffeHttpHandler handler = new(source, Authorizers.AuthorizeAny());
+        handler.Dispose();
+
+        // After disposal, firing Updated must not throw (handler already unsubscribed)
+        Action trigger = () => source.SetX509Context(MakeContext());
+        trigger.Should().NotThrow();
+    }


TestDisposeUnsubscribesFromUpdated currently only asserts that triggering an update doesn’t throw after disposal. Since Refresh() is already resilient and would likely not throw even if still subscribed, this doesn’t actually validate that the handler unsubscribed. Consider asserting unsubscription explicitly (e.g., via an observable seam/instrumentation that records whether Refresh ran after disposal).

Fixed in 5b9ef5c. Test now captures CurrentInvoker before disposal and asserts it is unchanged after firing an update, proving Refresh() was not called.

Copilot · 2026-03-24T00:05:22Z

src/Spiffe/WorkloadApi/Source.cs

+        protected virtual void Initialized()
+        {
+            _initialized.TrySetResult(true);
+            Updated?.Invoke();
+        }


Updated?.Invoke() runs subscriber code inline. If any subscriber throws, that exception will bubble out of Initialized() into the Workload API watch loop (via Watcher.OnUpdate), potentially causing reconnect/backoff loops and disrupting future updates. Consider invoking subscribers defensively (e.g., iterate invocation list and catch/log per handler, or otherwise ensure subscriber exceptions don’t break source update processing).

Left as-is intentionally. SpiffeHttpHandler.Refresh() (the only subscriber) does not throw, and adding a silent catch or per-subscriber iteration would hide bugs in future subscribers without a logger to surface them. If a subscriber throws it is a bug in that subscriber — propagating the exception makes it visible rather than swallowing it silently.

…mprove tests - Validate drainDelay in [0, 10 minutes] range in constructor - Add _lock to synchronize Refresh() and Dispose(), preventing a race where a newly created HttpMessageInvoker could be leaked if disposal won after the _disposed check in Refresh() - Add internal CurrentInvoker accessor for testing - TestHandlerRefreshesOnSourceUpdate: assert inner invoker instance changes - TestDisposeUnsubscribesFromUpdated: assert inner invoker unchanged after disposal - Add TestInvalidDrainDelayThrows Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

… published package yet) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Add SpiffeHttpHandler with auto-rotating mTLS certificate support

f718cf8

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

vurhanau requested a review from Copilot March 24, 2026 00:01

Copilot started reviewing on behalf of vurhanau March 24, 2026 00:02 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

vurhanau and others added 2 commits March 28, 2026 14:41

Revert docker samples to SocketsHttpHandler (SpiffeHttpHandler not in…

f57b153

… published package yet) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Conversation

vurhanau commented Mar 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

Uh oh!

vurhanau Mar 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vurhanau commented Mar 23, 2026 •

edited

Loading