-
Notifications
You must be signed in to change notification settings - Fork 378
Add HybridCache for IMDSv2 Attestation Token Caching #5539
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: rginsburg/msiv2_feature_branch
Are you sure you want to change the base?
Add HybridCache for IMDSv2 Attestation Token Caching #5539
Conversation
/// For persistent implementations, cross-process synchronization should be considered. | ||
/// Hybrid implementations should synchronize between memory and persistent storage when possible. | ||
/// </remarks> | ||
internal interface IHybridCache |
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.
I am not sure about the name, as it conflicts with https://learn.microsoft.com/en-us/aspnet/core/performance/caching/hybrid?view=aspnetcore-9.0
/// <exception cref="OperationCanceledException"> | ||
/// Thrown when the operation is canceled via the cancellation token. | ||
/// </exception> | ||
Task RemoveAsync(long key, CancellationToken ct); |
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.
nit: I believe it's customary to return Task to indicate if an entry was actually removed.
/// <exception cref="OperationCanceledException"> | ||
/// Thrown when the operation is canceled via the cancellation token. | ||
/// </exception> | ||
Task SetAsync(long key, string token, DateTimeOffset expiresOnUtc, CancellationToken ct); |
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 don't you set a AttestationTokenResponse ?
/// | ||
/// This entry structure is used for both in-memory and file cache storage. | ||
/// </remarks> | ||
private class CacheEntry |
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 do you need this object when you have AttestationTokenResponse ?
rsaCng.Key.Handle != null && | ||
!rsaCng.Key.Handle.IsInvalid) | ||
{ | ||
return rsaCng.Key.Handle.DangerousGetHandle().ToInt64(); |
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.
I think you should use a key thumbprint instead. When I asked copilot it said that "The handle does not represent the key’s content, just its memory address in the current process."
A key thumbprint (e.g., SHA-256 hash of the public key) would be a stable, cryptographically unique identifier for the key material, regardless of process or machine. This is a standard approach for caching or identifying cryptographic keys.
rsaCng.Key.Export(CngKeyBlobFormat.GenericPublicBlob))
Hash the blob (e.g., using SHA-256)
Use the hash as the cache key.
Thoughts?
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 can also put the thumbprint in the ManagedIdentityKeyInfo (via the ctor) so as to not compute it everytime.
|
||
// Create named mutex for cross-process synchronization | ||
// Using a deterministic name based on cache file path to ensure same mutex across processes | ||
_mutexName = $"Global\\MSAL_AttestationCache_{_cacheFilePath.GetHashCode():X8}"; |
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.
not sure if this will work, as different processes may compute different mutex names?
|
||
_requestContext.Logger.Verbose(() => $"[ImdsV2] GetAttestationJwtAsync called for cache key: {cacheKey}"); | ||
|
||
var cache = new HybridCache(_requestContext.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.
HybridCache implements IDisposable but is not disposed? should this be wrapped in using for short‑lived use?
} | ||
catch (Exception ex) | ||
{ | ||
_logger.Warning($"[HybridCache] Error loading file cache from {_cacheFilePath}: {ex.Message}. Created a new file."); |
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.
but the new file isn't created here?
|
||
_logger.Verbose(() => $"[HybridCache] Loading cache from file: {_cacheFilePath}"); | ||
|
||
var json = await Task.Run(() => File.ReadAllText(_cacheFilePath)).ConfigureAwait(false); |
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 not use ReadAllTextAsync?
|
||
// Write to temp file first, then move to avoid corruption during write | ||
var tempFile = _cacheFilePath + ".tmp"; | ||
await Task.Run(() => File.WriteAllText(tempFile, json)).ConfigureAwait(false); |
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.
WriteAllTextAsync?
{ | ||
if (!string.IsNullOrEmpty(memoryEntry.Token) && now + s_expirySkew < memoryEntry.ExpiresOnUtc) | ||
{ | ||
_logger.Info(() => $"[HybridCache] Cache hit in memory for key: {key}"); |
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.
verbose?
|
||
// Token expired in memory, remove it | ||
s_memoryCache.TryRemove(keyString, out _); | ||
_logger.Info(() => $"[HybridCache] Expired token removed from memory cache for key: {key}"); |
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.
verbose?
throw new ArgumentNullException(nameof(token)); | ||
} | ||
|
||
_logger.Info(() => $"[HybridCache] SetAsync called for key: {key}, expires: {expiresOnUtc}"); |
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.
verbose?
throw new ObjectDisposedException(nameof(HybridCache)); | ||
} | ||
|
||
_logger.Info(() => $"[HybridCache] RemoveAsync called for key: {key}"); |
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.
verbose?
return Task.CompletedTask; | ||
}, ct).ConfigureAwait(false); | ||
|
||
_logger.Info(() => $"[HybridCache] Remove operation completed for key: {key}"); |
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.
verbose?
{ | ||
cache.Remove(key); | ||
} | ||
_logger.Info(() => $"[HybridCache] Cleaned up {keysToRemove.Count} expired entries from file cache"); |
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.
verbose?
|
||
_logger.Info(() => $"[HybridCache] Initializing cache with directory: {cacheDirectory}"); | ||
|
||
Directory.CreateDirectory(cacheDirectory); |
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 this operation fail?
Summary
Implements a new hybrid caching strategy for IMDSv2 attestation tokens with both in-memory and file-based persistence.
Changes
IHybridCache.cs
- Interface for hybrid cache operationsHybridCache.cs
- Hybrid cache implementation with:ImdsV2ManagedIdentitySource.cs
- Updated to use HybridCache to cache MAA tokensBenefits
TODO: Testing