-
Notifications
You must be signed in to change notification settings - Fork 254
Add SigningCredential loading to DefaultCredentialsLoader #3361
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: master
Are you sure you want to change the base?
Changes from all commits
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 |
|---|---|---|
|
|
@@ -3,5 +3,6 @@ | |
| <packageSources> | ||
| <clear /> | ||
| <add key="NuGet" value="https://api.nuget.org/v3/index.json" /> | ||
| <add key="Local" value="C:\repos\Builds\abstractions" /> | ||
|
Collaborator
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. No longer needed |
||
| </packageSources> | ||
| </configuration> | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,220 @@ | ||
| # Adding Support for Symmetric Keys in Microsoft.Identity.Web | ||
|
|
||
| ## Overview | ||
| This proposal outlines the addition of symmetric key support for signing credentials in Microsoft.Identity.Web, allowing keys to be loaded from Key Vault or Base64 encoded strings while maintaining clean abstractions. | ||
|
|
||
| ## Requirements | ||
| 1. Support symmetric keys from: | ||
| - Azure Key Vault secrets | ||
| - Base64 encoded strings | ||
| 2. Avoid circular dependencies with Microsoft.IdentityModel | ||
| 3. Follow existing patterns in the codebase | ||
| 4. Maintain backward compatibility | ||
|
|
||
| ## Developer Experience | ||
| The implementation provides a straightforward and type-safe approach to working with symmetric keys while maintaining clean separation of concerns: | ||
|
|
||
| ### Key Management | ||
| When working with symmetric keys, developers can utilize two primary sources: | ||
|
|
||
| 1. **Azure Key Vault Integration** | ||
| ```csharp | ||
| var credentials = new CredentialDescription | ||
| { | ||
| SourceType = CredentialSource.SymmetricKeyFromKeyVault, | ||
| KeyVaultUrl = "https://your-vault.vault.azure.net", | ||
| KeyVaultSecretName = "your-secret-name" | ||
| }; | ||
| ``` | ||
|
|
||
| 2. **Direct Base64 Encoded Keys** | ||
| ```csharp | ||
| var credentials = new CredentialDescription | ||
| { | ||
| SourceType = CredentialSource.SymmetricKeyBase64Encoded, | ||
| Base64EncodedValue = "your-base64-encoded-key" | ||
| }; | ||
| ``` | ||
|
|
||
| ### Implementation Details | ||
| - The DefaultCredentialLoader automatically selects the appropriate loader based on the SourceType | ||
| - Key material is loaded and converted to a SymmetricSecurityKey | ||
| - The security key is stored in the CachedValue property of CredentialDescription | ||
| - This design maintains independence from Microsoft.IdentityModel types in the abstractions layer | ||
| - The implementation follows the same pattern as certificate handling for consistency | ||
|
|
||
| ## Design | ||
|
|
||
| ### 1. New CredentialSource Values(Abstractions Layer) | ||
| ```csharp | ||
| public enum CredentialSource | ||
| { | ||
| // Existing values | ||
| Certificate = 0, | ||
| KeyVault = 1, | ||
| Base64Encoded = 2, | ||
| Path = 3, | ||
| StoreWithThumbprint = 4, | ||
| StoreWithDistinguishedName = 5, | ||
|
|
||
| // New values | ||
| SymmetricKeyFromKeyVault = 6, | ||
|
Collaborator
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. It would not be 6, which is already used. Would it be a thing to expose the symmetric key as base64encoded? In that case should the Algorithm be used? or is it already in the key?
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. I will update the right number in the implementation, this is created by CLINE and it knew only the IdWeb files. For ServerNoncePolicy the credentials can also be Symmetric, today we have this supported in SAL. I am not aware of other places the key can come from, but today in SAL they are being expected to come from either KeyVault or base64encoded string. Algorithm is still used while creating the SigningCredentials from the key see here |
||
| SymmetricKeyBase64Encoded = 7 | ||
| } | ||
| ``` | ||
|
|
||
| ### 2. SymmetricKeyDescription Class(IdWeb Layer) | ||
| Following the same pattern as CertificateDescription: | ||
|
|
||
| ```csharp | ||
| public class SymmetricKeyDescription : CredentialDescription | ||
|
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. The Have you considered just extending |
||
| { | ||
| public static SymmetricKeyDescription FromKeyVault(string keyVaultUrl, string secretName) | ||
| { | ||
| return new SymmetricKeyDescription | ||
| { | ||
| SourceType = CredentialSource.SymmetricKeyFromKeyVault, | ||
| KeyVaultUrl = keyVaultUrl, | ||
| KeyVaultSecretName = secretName | ||
| }; | ||
| } | ||
|
|
||
| public static SymmetricKeyDescription FromBase64Encoded(string base64EncodedValue) | ||
| { | ||
| return new SymmetricKeyDescription | ||
| { | ||
| SourceType = CredentialSource.SymmetricKeyBase64Encoded, | ||
| Base64EncodedValue = base64EncodedValue | ||
| }; | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### 3. New Loader Classes(IdWeb Layer) | ||
| Internal implementation in Microsoft.Identity.Web: | ||
|
|
||
| ```csharp | ||
| internal class KeyVaultSymmetricKeyLoader : ICredentialSourceLoader | ||
| { | ||
| private readonly SecretClient _secretClient; | ||
|
|
||
| public KeyVaultSymmetricKeyLoader(SecretClient secretClient) | ||
| { | ||
| _secretClient = secretClient ?? throw new ArgumentNullException(nameof(secretClient)); | ||
| } | ||
|
|
||
| public async Task LoadIfNeededAsync(CredentialDescription description, CredentialSourceLoaderParameters? parameters) | ||
| { | ||
| _ = Throws.IfNull(description); | ||
|
|
||
| if (description.CachedValue != null) | ||
| return; | ||
|
|
||
| if (string.IsNullOrEmpty(description.KeyVaultUrl)) | ||
| throw new ArgumentException("KeyVaultUrl is required for KeyVault source"); | ||
|
|
||
| if (string.IsNullOrEmpty(description.KeyVaultSecretName)) | ||
| throw new ArgumentException("KeyVaultSecretName is required for KeyVault source"); | ||
|
|
||
| // Load secret from Key Vault | ||
| var secret = await _secretClient.GetSecretAsync(description.KeyVaultSecretName).ConfigureAwait(false); | ||
| if (secret?.Value == null) | ||
| throw new InvalidOperationException($"Secret {description.KeyVaultSecretName} not found in Key Vault"); | ||
|
|
||
| try | ||
| { | ||
| // Convert secret value to bytes and create SymmetricSecurityKey | ||
| var keyBytes = Convert.FromBase64String(secret.Value.Value); | ||
| description.CachedValue = new SymmetricSecurityKey(keyBytes); | ||
|
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. The fact that At least for certificates, a
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. (read my comments below about extending CredentialDescription first) |
||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new InvalidOperationException($"Failed to create symmetric key from Key Vault secret: {ex.Message}", ex); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| internal class Base64EncodedSymmetricKeyLoader : ICredentialSourceLoader | ||
| { | ||
| public async Task LoadIfNeededAsync(CredentialDescription description, CredentialSourceLoaderParameters? parameters) | ||
| { | ||
| _ = Throws.IfNull(description); | ||
|
|
||
| if (description.CachedValue != null) | ||
| return; | ||
|
|
||
| if (string.IsNullOrEmpty(description.Base64EncodedValue)) | ||
| throw new ArgumentException("Base64EncodedValue is required for Base64Encoded source"); | ||
|
|
||
| try | ||
| { | ||
| // Convert Base64 string to bytes and create SymmetricSecurityKey | ||
| var keyBytes = Convert.FromBase64String(description.Base64EncodedValue); | ||
| description.CachedValue = new SymmetricSecurityKey(keyBytes); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| throw new FormatException("Invalid Base64 string for symmetric key", ex); | ||
| } | ||
|
|
||
| await Task.CompletedTask.ConfigureAwait(false); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### 4. DefaultCredentialsLoader Changes(IdWeb Layer) | ||
| Update the loader to handle both certificate and symmetric key scenarios: | ||
|
|
||
| ```csharp | ||
| public partial class DefaultCredentialsLoader : ICredentialsLoader, ISigningCredentialsLoader | ||
| { | ||
| public DefaultCredentialsLoader(ILogger<DefaultCredentialsLoader>? logger) | ||
| { | ||
| _logger = logger ?? new NullLogger<DefaultCredentialsLoader>(); | ||
|
|
||
| CredentialSourceLoaders = new Dictionary<CredentialSource, ICredentialSourceLoader> | ||
| { | ||
| // Existing certificate loaders | ||
| { CredentialSource.KeyVault, new KeyVaultCertificateLoader() }, | ||
| { CredentialSource.Path, new FromPathCertificateLoader() }, | ||
| { CredentialSource.StoreWithThumbprint, new StoreWithThumbprintCertificateLoader() }, | ||
| { CredentialSource.StoreWithDistinguishedName, new StoreWithDistinguishedNameCertificateLoader() }, | ||
| { CredentialSource.Base64Encoded, new Base64EncodedCertificateLoader() }, | ||
|
|
||
| // New symmetric key loaders | ||
| { CredentialSource.SymmetricKeyFromKeyVault, new KeyVaultSymmetricKeyLoader(_secretClient) }, | ||
| { CredentialSource.SymmetricKeyBase64Encoded, new Base64EncodedSymmetricKeyLoader() } | ||
| }; | ||
| } | ||
|
|
||
| public async Task<SigningCredentials?> LoadSigningCredentialsAsync( | ||
|
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. I wonder if a simpler design would be to rely on the existing object model, i.e. Apart from simpelr object model, this would avoid mixing the concept of |
||
| CredentialDescription credentialDescription, | ||
| CredentialSourceLoaderParameters? parameters = null) | ||
| { | ||
| _ = Throws.IfNull(credentialDescription); | ||
|
|
||
| try | ||
| { | ||
| await LoadCredentialsIfNeededAsync(credentialDescription, parameters); | ||
|
|
||
| if (credentialDescription.Certificate != null) | ||
| { | ||
| return new X509SigningCredentials( | ||
| credentialDescription.Certificate, | ||
| credentialDescription.Algorithm); | ||
| } | ||
| else if (credentialDescription.CachedValue is SymmetricSecurityKey key) | ||
| { | ||
| return new SigningCredentials(key, credentialDescription.Algorithm); | ||
| } | ||
|
|
||
| return null; | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.CredentialLoadingFailure(_logger, credentialDescription, ex); | ||
| throw; | ||
| } | ||
| } | ||
| } | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,13 +9,14 @@ | |
| using Microsoft.Extensions.Logging; | ||
| using Microsoft.Extensions.Logging.Abstractions; | ||
| using Microsoft.Identity.Abstractions; | ||
| using Microsoft.IdentityModel.Tokens; | ||
|
|
||
| namespace Microsoft.Identity.Web | ||
| { | ||
| /// <summary> | ||
| /// Default credentials loader. | ||
| /// </summary> | ||
| public partial class DefaultCredentialsLoader : ICredentialsLoader | ||
| public partial class DefaultCredentialsLoader : ICredentialsLoader, ISigningCredentialsLoader | ||
| { | ||
| private readonly ILogger<DefaultCredentialsLoader> _logger; | ||
| private readonly ConcurrentDictionary<string, SemaphoreSlim> _loadingSemaphores = new ConcurrentDictionary<string, SemaphoreSlim>(); | ||
|
|
@@ -48,13 +49,13 @@ public DefaultCredentialsLoader() : this(null) | |
| } | ||
|
|
||
| /// <summary> | ||
| /// Dictionary of credential loaders per credential source. The application can add more to | ||
| /// Dictionary of credential loaders per credential source. The application can add more to | ||
| /// process additional credential sources(like dSMS). | ||
| /// </summary> | ||
| public IDictionary<CredentialSource, ICredentialSourceLoader> CredentialSourceLoaders { get; } | ||
|
|
||
| /// <inheritdoc/> | ||
| /// Load the credentials from the description, if needed. | ||
| /// Load the credentials from the description, if needed. | ||
| /// Important: Ignores SKIP flag, propagates exceptions. | ||
| public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialDescription, CredentialSourceLoaderParameters? parameters = null) | ||
| { | ||
|
|
@@ -99,9 +100,9 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD | |
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| /// Loads first valid credential which is not marked as Skipped. | ||
| /// Loads first valid credential which is not marked as Skipped. | ||
| public async Task<CredentialDescription?> LoadFirstValidCredentialsAsync( | ||
| IEnumerable<CredentialDescription> credentialDescriptions, | ||
| IEnumerable<CredentialDescription> credentialDescriptions, | ||
| CredentialSourceLoaderParameters? parameters = null) | ||
| { | ||
| foreach (var credentialDescription in credentialDescriptions) | ||
|
|
@@ -116,6 +117,31 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD | |
| return null; | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public async Task<SigningCredentials?> LoadSigningCredentialsAsync( | ||
| CredentialDescription credentialDescription, | ||
|
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. nit: spacing (add a tab to the params) |
||
| CredentialSourceLoaderParameters? parameters = null) | ||
| { | ||
| _ = Throws.IfNull(credentialDescription); | ||
|
|
||
| try | ||
| { | ||
| await LoadCredentialsIfNeededAsync(credentialDescription, parameters); | ||
|
|
||
| if (credentialDescription.Certificate != null) | ||
|
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 shows creating the SigningCredentials only from the certificate. I've included the proposal to add support for loading SymmetricSecurityKey in this PR. |
||
| { | ||
| return new X509SigningCredentials(credentialDescription.Certificate, credentialDescription.Algorithm); | ||
| } | ||
|
|
||
| return null; | ||
|
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. Returning null if Certificate is not loaded because a null certificate is a valid state. Open to suggestions. |
||
| } | ||
| catch (Exception ex) | ||
| { | ||
| Logger.CredentialLoadingFailure(_logger, credentialDescription, ex); | ||
| throw; | ||
| } | ||
| } | ||
|
|
||
| /// <inheritdoc/> | ||
| public void ResetCredentials(IEnumerable<CredentialDescription> credentialDescriptions) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,25 @@ | ||
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License. | ||
|
|
||
| using System.Threading.Tasks; | ||
| using Microsoft.Identity.Abstractions; | ||
| using Microsoft.IdentityModel.Tokens; | ||
|
|
||
| namespace Microsoft.Identity.Web | ||
| { | ||
| /// <summary> | ||
| /// Interface for loading signing credentials. | ||
| /// </summary> | ||
| public interface ISigningCredentialsLoader | ||
| { | ||
| /// <summary> | ||
| /// Loads SigningCredentials from the credential description. | ||
| /// </summary> | ||
| /// <param name="credentialDescription">Credential description.</param> | ||
| /// <param name="parameters">Optional parameters for loading credentials.</param> | ||
| /// <returns>SigningCredentials if successful, null otherwise.</returns> | ||
| Task<SigningCredentials?> LoadSigningCredentialsAsync( | ||
|
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. Not sure this is needed if |
||
| CredentialDescription credentialDescription, | ||
| CredentialSourceLoaderParameters? parameters = null); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| Microsoft.Identity.Web.DefaultCredentialsLoader.LoadSigningCredentialsAsync(Microsoft.Identity.Abstractions.CredentialDescription! credentialDescription, Microsoft.Identity.Abstractions.CredentialSourceLoaderParameters? parameters = null) -> System.Threading.Tasks.Task<Microsoft.IdentityModel.Tokens.SigningCredentials?>! | ||
| Microsoft.Identity.Web.ISigningCredentialsLoader | ||
| Microsoft.Identity.Web.ISigningCredentialsLoader.LoadSigningCredentialsAsync(Microsoft.Identity.Abstractions.CredentialDescription! credentialDescription, Microsoft.Identity.Abstractions.CredentialSourceLoaderParameters? parameters = null) -> System.Threading.Tasks.Task<Microsoft.IdentityModel.Tokens.SigningCredentials?>! |
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.
It's not published, @RojaEnnam FYI