-
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?
Changes from all commits
6d28cf0
e6e0db9
523ed95
77964b3
fc99dd6
1793f3f
1c0d769
60070f6
0b99d3c
d3b2c7b
e9bf39d
29450fd
c21a9c6
b786f3c
1a938c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,7 +4,8 @@ | |||||
| <IncludeOperationsSharedSource>true</IncludeOperationsSharedSource> | ||||||
| </PropertyGroup> | ||||||
| <ItemGroup> | ||||||
| <PackageReference Include="Azure.Core" /> | ||||||
| <!-- <PackageReference Include="Azure.Core" /> --> | ||||||
| <ProjectReference Include="..\..\..\..\sdk\core\Azure.Core\src\Azure.Core.csproj" /> | ||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I think we can simplify a bit. Probably want to throw an issue in for the "change back to a package ref" as well. |
||||||
| <PackageReference Include="Azure.Identity" /> | ||||||
| <PackageReference Include="Newtonsoft.Json" /> | ||||||
| <PackageReference Include="NUnit" /> | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,6 +21,8 @@ public class MockTransport : HttpPipelineTransport | |
|
|
||
| public bool? ExpectSyncPipeline { get; set; } | ||
|
|
||
| public List<HttpPipelineTransportOptions> TransportUpdates { get; } = []; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why is this public?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 MockTransport() | ||
| { | ||
| RequestGate = new AsyncGate<MockRequest, MockResponse>(); | ||
|
|
@@ -38,7 +40,7 @@ public MockTransport(params MockResponse[] responses) | |
| }; | ||
| } | ||
|
|
||
| public MockTransport(Func<MockRequest, MockResponse> responseFactory): this(req => responseFactory((MockRequest)req.Request)) | ||
| public MockTransport(Func<MockRequest, MockResponse> responseFactory) : this(req => responseFactory((MockRequest)req.Request)) | ||
| { | ||
| } | ||
|
|
||
|
|
@@ -72,6 +74,11 @@ public override async ValueTask ProcessAsync(HttpMessage message) | |
| await ProcessCore(message); | ||
| } | ||
|
|
||
| public override void Update(HttpPipelineTransportOptions options) | ||
| { | ||
| TransportUpdates.Add(options); | ||
| } | ||
|
|
||
| private async Task ProcessCore(HttpMessage message) | ||
| { | ||
| if (!(message.Request is MockRequest request)) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -306,6 +306,8 @@ public partial struct AccessToken | |
| public AccessToken(string accessToken, System.DateTimeOffset expiresOn) { throw null; } | ||
| public AccessToken(string accessToken, System.DateTimeOffset expiresOn, System.DateTimeOffset? refreshOn) { throw null; } | ||
| public AccessToken(string accessToken, System.DateTimeOffset expiresOn, System.DateTimeOffset? refreshOn, string tokenType) { throw null; } | ||
| public AccessToken(string accessToken, System.DateTimeOffset expiresOn, System.DateTimeOffset? refreshOn, string tokenType, System.Security.Cryptography.X509Certificates.X509Certificate2 bindingCertificate) { throw null; } | ||
| public System.Security.Cryptography.X509Certificates.X509Certificate2? BindingCertificate { get { throw null; } set { } } | ||
| public System.DateTimeOffset ExpiresOn { get { throw null; } } | ||
| public System.DateTimeOffset? RefreshOn { get { throw null; } } | ||
| public string Token { get { throw null; } } | ||
|
|
@@ -1042,12 +1044,15 @@ public partial class HttpClientTransport : Azure.Core.Pipeline.HttpPipelineTrans | |
| { | ||
| public static readonly Azure.Core.Pipeline.HttpClientTransport Shared; | ||
| public HttpClientTransport() { } | ||
| public HttpClientTransport(System.Func<Azure.Core.Pipeline.HttpPipelineTransportOptions, System.Net.Http.HttpClient> clientFactory) { } | ||
| public HttpClientTransport(System.Func<Azure.Core.Pipeline.HttpPipelineTransportOptions, System.Net.Http.HttpMessageHandler> handlerFactory) { } | ||
| public HttpClientTransport(System.Net.Http.HttpClient client) { } | ||
| public HttpClientTransport(System.Net.Http.HttpMessageHandler messageHandler) { } | ||
| public sealed override Azure.Core.Request CreateRequest() { throw null; } | ||
| 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) { } | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I think that makes sense. |
||
| } | ||
| public partial class HttpPipeline | ||
| { | ||
|
|
@@ -1103,6 +1108,7 @@ protected HttpPipelineTransport() { } | |
| public abstract Azure.Core.Request CreateRequest(); | ||
| public abstract void Process(Azure.Core.HttpMessage message); | ||
| public abstract System.Threading.Tasks.ValueTask ProcessAsync(Azure.Core.HttpMessage message); | ||
| public virtual void UpdateTransport(Azure.Core.Pipeline.HttpPipelineTransportOptions options) { } | ||
| } | ||
| public partial class HttpPipelineTransportOptions | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ | |
|
|
||
| using System; | ||
| using System.ClientModel.Primitives; | ||
| using System.Security.Cryptography.X509Certificates; | ||
|
|
||
| namespace Azure.Core | ||
| { | ||
|
|
@@ -52,6 +53,23 @@ public AccessToken(string accessToken, DateTimeOffset expiresOn, DateTimeOffset? | |
| TokenType = tokenType; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Creates a new instance of <see cref="AccessToken"/> using the provided <paramref name="accessToken"/> and <paramref name="expiresOn"/>. | ||
| /// </summary> | ||
| /// <param name="accessToken">The access token value.</param> | ||
| /// <param name="expiresOn">The access token expiry date.</param> | ||
| /// <param name="refreshOn">Specifies the time when the cached token should be proactively refreshed.</param> | ||
| /// <param name="tokenType">The access token type.</param> | ||
| /// <param name="bindingCertificate">The binding certificate for the access token.</param> | ||
| public AccessToken(string accessToken, DateTimeOffset expiresOn, DateTimeOffset? refreshOn, string tokenType, X509Certificate2 bindingCertificate) | ||
| { | ||
| Token = accessToken; | ||
| ExpiresOn = expiresOn; | ||
| RefreshOn = refreshOn; | ||
| TokenType = tokenType; | ||
| BindingCertificate = bindingCertificate; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Get the access token value. | ||
| /// </summary> | ||
|
|
@@ -72,6 +90,12 @@ public AccessToken(string accessToken, DateTimeOffset expiresOn, DateTimeOffset? | |
| /// </summary> | ||
| public string TokenType { get; } | ||
|
|
||
| /// <summary> | ||
| /// Gets or sets the binding certificate for the access token. | ||
| /// This is used when authenticating via Proof of Possession (PoP). | ||
christothes marked this conversation as resolved.
Show resolved
Hide resolved
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May be a good idea to throw a |
||
| /// </summary> | ||
| public X509Certificate2? BindingCertificate { get; set; } | ||
|
|
||
| /// <inheritdoc /> | ||
| public override bool Equals(object? obj) | ||
| { | ||
|
|
||
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 PackageReference to Azure.Core is commented out and replaced with a ProjectReference. This appears to be a temporary change for testing purposes. Ensure this is intentional and should be included in the PR, or if it should be reverted before merging.