-
Notifications
You must be signed in to change notification settings - Fork 379
IMDSv2 mTLS PoP: add best‑effort persisted binding‑cert cache (Windows CurrentUser\My) layered over in‑memory cache; per‑alias mutex; tests #5566
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
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/FriendlyNameCodec.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/FriendlyNameCodec.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/FriendlyNameCodec.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MsiCertificateFriendlyNameEncoder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/FriendlyNameCodec.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/MsiCertificateFriendlyNameEncoder.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/ImdsV2ManagedIdentitySource.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/InterprocessLock.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/InterprocessLock.cs
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/InterprocessLock.cs
Show resolved
Hide resolved
| /// - No throws; persistence must not block token acquisition. | ||
| /// - Windows-only; FriendlyName semantics are undefined elsewhere. | ||
| /// </summary> | ||
| internal static class PersistentCertificateStore |
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.
Are you overusing static? Everything seems static in this design. Does this not need mocking? And a logger?
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.
we don't need mocking here, but made few changes.
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/PersistentCertificateStore.cs
Outdated
Show resolved
Hide resolved
src/client/Microsoft.Identity.Client/ManagedIdentity/V2/StorePruner.cs
Outdated
Show resolved
Hide resolved
| PersistentCertificateCacheFactory.Create(requestContext.Logger); | ||
|
|
||
| _mtlsCache = new MtlsBindingCache(s_mtlsCertificateCache, persisted); | ||
| } |
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.
Can we use dependency injection instead of putting this in the constructor? This should enhance testability.
| { | ||
| internal interface IMtlsBindingCache | ||
| { | ||
| Task<Tuple<X509Certificate2, string /*endpoint*/, string /*clientId*/>> GetOrCreateAsync( |
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.
Can you create a named tuple?
internal class MtlsBindingInfo
{
public X509Certificate2 Certificate { get; set; }
public string Endpoint { get; set; }
public string ClientId { get; set; }
}
| try | ||
| { | ||
| // Create or open existing | ||
| using var m = new Mutex(initiallyOwned: false, name); |
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.
what if an exception occurs before the using block exposes?
| } | ||
| catch (AbandonedMutexException) | ||
| { | ||
| entered = true; // prior holder crashed |
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.
can you add a log here?
|
|
||
| if (!entered) | ||
| { | ||
| logVerbose?.Invoke($"[PersistentCert] Skip persist (lock busy '{name}')."); |
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.
should we log duration waited?
| if (string.IsNullOrEmpty(cn)) | ||
| return false; | ||
|
|
||
| // Unconditional CNs |
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 there a difference in string comparison between conditional and unconditional CNs? (Ordinal vs. OrdinalIgnoreCase)
| @@ -0,0 +1,92 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
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.
can you add additional tests:
- Edge cases:
- Invalid or null alias values to ensure proper exception handling or fallback.
- Very long alias strings to check for buffer overflows or system limitations.
- Non-Windows platforms
- Multiple concurrent attempts
- Exception handling within action: Add tests where the action delegate throws an exception—confirm lock is released and exception bubbles as expected.
| { Assert.Inconclusive("Windows-only"); return; } | ||
|
|
||
| var aliasRaw = " my-alias "; | ||
| var g = InterprocessLock.GetMutexNameForAlias(aliasRaw, preferGlobal: true); |
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.
please use global/local for readability
| @@ -0,0 +1,553 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
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.
can you add additional tests:
- for non-windows behavior, validate intended no-op/error/alternative behavior
- Add tests to validate behavior when mutex acquisition fails due to errors besides contention (e.g., invalid alias, OS failures), or when the action throws an exception—does the lock get released?
- Test that repeated calls for the same alias succeed serially—i.e., after the first lock is released, the second can acquire.
- Test boundary and edge cases for aliases: empty string, extremely long name, special characters, unicode, etc.
| @@ -0,0 +1,553 @@ | |||
| // Copyright (c) Microsoft Corporation. All rights reserved. | |||
| // Licensed under the MIT License. | |||
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.
Use private helper methods to reduce repeated code for Windows checks or lock calls
Fixes - Adds MSI v2 cert to the store (persistent cache)
Changes proposed in this request
The IMDSv2 mTLS PoP flow currently relies only on a process‑local cache. That means new processes (or app restarts) must re‑mint the binding certificate even when a valid one already exists on the machine. This PR layers a best‑effort, cross‑process persisted cache on top of the existing in‑memory cache to reduce reminting, while keeping token acquisition robust (never blocked by persistence).
Testing
unit testing
Performance impact
none
Documentation