-
Notifications
You must be signed in to change notification settings - Fork 5.1k
Handle client certificate rotation for token binding #53916
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR implements support for client certificate rotation in the Azure.Core transport layer to enable dynamic token binding scenarios. The changes allow transport instances to be updated with new certificate configurations at runtime without requiring full pipeline reconstruction.
Key Changes:
- Added
UpdateTransportmethod toHttpPipelineTransportbase class and implementations inHttpClientTransportandHttpWebRequestTransport - Introduced
ISupportsTransportCertificateUpdateinterface for policies that need to trigger transport updates - Extended
AccessTokenwith aBindingCertificateproperty for Proof of Possession (PoP) scenarios - Added comprehensive test coverage for certificate rotation scenarios (rotating from empty, rotating from existing cert)
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
sdk/core/Azure.Core/src/Pipeline/Internal/ISupportsTransportUpdate.cs |
New internal interface for policies that support transport certificate updates via event subscription |
sdk/core/Azure.Core/src/Pipeline/HttpPipelineTransport.cs |
Added virtual UpdateTransport method to base transport class with default NotSupportedException |
sdk/core/Azure.Core/src/Pipeline/HttpClientTransport.cs |
Implemented UpdateTransport with support for client and handler factories, using volatile field and Interlocked for thread-safety |
sdk/core/Azure.Core/src/Pipeline/HttpWebRequestTransport.cs |
Implemented UpdateTransport using Interlocked.Exchange for thread-safe configuration updates |
sdk/core/Azure.Core/src/Pipeline/HttpPipeline.cs |
Added subscription to ISupportsTransportCertificateUpdate events in constructors to enable transport updates |
sdk/core/Azure.Core/src/Pipeline/HttpPipelineTransportOptions.cs |
Added internal Clone method for creating copies of transport options during updates |
sdk/core/Azure.Core/src/AccessToken.cs |
Added BindingCertificate property and constructor overload to support token binding with client certificates |
sdk/core/Azure.Core/tests/TransportFunctionalTests.cs |
Added two new test methods for certificate rotation scenarios plus code formatting improvements |
sdk/core/Azure.Core/tests/PipelineTestBase.cs |
Extracted GetCertificate helper method and added second test certificate constant (Pfx2) |
sdk/core/Azure.Core/tests/HttpPipelineTests.cs |
Added test for transport update mechanism with TransportUpdatingPolicy test class |
sdk/core/Azure.Core/tests/HttpClientTransportFunctionalTest.cs |
Removed unused certCallback field |
sdk/core/Azure.Core.TestFramework/src/MockTransport.cs |
Implemented UpdateTransport to track transport updates in tests |
sdk/core/Azure.Core.TestFramework/src/Azure.Core.TestFramework.csproj |
Changed from PackageReference to ProjectReference for Azure.Core (likely temporary for testing) |
sdk/core/Azure.Core/api/*.cs |
Updated API surface files for all target frameworks with new public constructors and methods |
| for (int i = 0; i < pipeline.Length; i++) | ||
| { | ||
| // If the policy implements ISupportsTransportCertificateUpdate, we need to subscribe to its TransportUpdated event | ||
| if (pipeline[i] is ISupportsTransportCertificateUpdate transportUpdated) | ||
| { | ||
| transportUpdated.TransportOptionsChanged += options => _transport.UpdateTransport(options); | ||
| break; | ||
| } | ||
| } |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is duplicate code from the constructor above (lines 61-69). Consider extracting this logic into a private helper method to avoid code duplication.
|
|
||
| protected X509Certificate2 GetCertificate(string pfx) | ||
| { | ||
| byte[] cer = Convert.FromBase64String(Pfx); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter pfx is not used in this method. Instead, the method uses the constant Pfx from the class. This should be changed to use the pfx parameter that is passed in.
| byte[] cer = Convert.FromBase64String(Pfx); | |
| byte[] cer = Convert.FromBase64String(pfx); |
|
|
||
| var transport = GetTransport(true, options); | ||
|
|
||
| // Initially no client certificate |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment says "Initially no client certificate" but the code actually sets a client certificate in the options before creating the transport. The comment should say "Initially with first client certificate" or similar to accurately reflect what the code does.
| // Initially no client certificate | |
| // Initially with first client certificate (clientCert) |
| { | ||
| if (this == Shared) | ||
| { | ||
| throw new InvalidOperationException("Cannot update the shared HttpClientTransport instance."); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message says "Cannot update the shared HttpClientTransport instance" but this is HttpWebRequestTransport, not HttpClientTransport. The error message should be corrected to reference the correct transport type.
| throw new InvalidOperationException("Cannot update the shared HttpClientTransport instance."); | |
| throw new InvalidOperationException("Cannot update the shared HttpWebRequestTransport instance."); |
|
|
||
| private X509Certificate2 GetTestCertificate() | ||
| { | ||
| // byte[] cer = Convert.FromBase64String(Pfx); |
Copilot
AI
Nov 13, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a commented-out line // byte[] cer = Convert.FromBase64String(Pfx); followed by the same line uncommented. The commented line should be removed as it appears to be leftover from development.
| // byte[] cer = Convert.FromBase64String(Pfx); |
|
|
||
| public bool? ExpectSyncPipeline { get; set; } | ||
|
|
||
| public List<HttpPipelineTransportOptions> TransportUpdates { get; } = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this public?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is on the mock transport so that code that is testing this can observe the transport updates that have occurred.
| public void Dispose() { } | ||
| public override void Process(Azure.Core.HttpMessage message) { } | ||
| public override System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message) { throw null; } | ||
| public override void UpdateTransport(Azure.Core.Pipeline.HttpPipelineTransportOptions options) { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since the method is on transport type, should it be called simply Update?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think that makes sense.
API Change CheckAPIView identified API level changes in this PR and created the following API reviews |
Contributing to the Azure SDK
Please see our CONTRIBUTING.md if you are not familiar with contributing to this repository or have questions.
For specific information about pull request etiquette and best practices, see this section.