Add SigningCredential loading to DefaultCredentialsLoader #3361
Conversation
|
Thanks @RojaEnnam |
| return new X509SigningCredentials(credentialDescription.Certificate, credentialDescription.Algorithm); | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Returning null if Certificate is not loaded because a null certificate is a valid state. Open to suggestions.
| { | ||
| await LoadCredentialsIfNeededAsync(credentialDescription, parameters); | ||
|
|
||
| if (credentialDescription.Certificate != null) |
There was a problem hiding this comment.
This shows creating the SigningCredentials only from the certificate. I've included the proposal to add support for loading SymmetricSecurityKey in this PR.
| StoreWithDistinguishedName = 5, | ||
|
|
||
| // New values | ||
| SymmetricKeyFromKeyVault = 6, |
There was a problem hiding this comment.
It would not be 6, which is already used.
Is it a common scenario to get symmetric keys from KeyVault? or would they rather come from somewhere else? (like the metadata document). Do we have customer requests for symmetric keys?
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?
There was a problem hiding this comment.
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
| <MicrosoftGraphVersion>4.36.0</MicrosoftGraphVersion> | ||
| <MicrosoftGraphBetaVersion>4.57.0-preview</MicrosoftGraphBetaVersion> | ||
| <MicrosoftIdentityAbstractionsVersion>9.0.0</MicrosoftIdentityAbstractionsVersion> | ||
| <MicrosoftIdentityAbstractionsVersion>9.1.0</MicrosoftIdentityAbstractionsVersion> |
| <packageSources> | ||
| <clear /> | ||
| <add key="NuGet" value="https://api.nuget.org/v3/index.json" /> | ||
| <add key="Local" value="C:\repos\Builds\abstractions" /> |
|
|
||
| /// <inheritdoc/> | ||
| public async Task<SigningCredentials?> LoadSigningCredentialsAsync( | ||
| CredentialDescription credentialDescription, |
There was a problem hiding this comment.
nit: spacing (add a tab to the params)
| Following the same pattern as CertificateDescription: | ||
|
|
||
| ```csharp | ||
| public class SymmetricKeyDescription : CredentialDescription |
There was a problem hiding this comment.
The CredentialDescription class already has the responsibility of managing different types of credentials - see the CredentialType property - it can handle Certificates, Signed Assertions (i.e. JWT tokens with a special audience to denote they are actually credentials), client secrets and decrypt keys.
Have you considered just extending CredentialDescription instead of creating a new class? What are the pros / cons?
| { | ||
| // Convert secret value to bytes and create SymmetricSecurityKey | ||
| var keyBytes = Convert.FromBase64String(secret.Value.Value); | ||
| description.CachedValue = new SymmetricSecurityKey(keyBytes); |
There was a problem hiding this comment.
The fact that CachedValue is an object is requires users to cast to the right object (string, X509Certificate2 etc.).
At least for certificates, a Certificate property also exists to avoid a cast. I suggest you do the same for symmetric keys?
There was a problem hiding this comment.
(read my comments below about extending CredentialDescription first)
| }; | ||
| } | ||
|
|
||
| public async Task<SigningCredentials?> LoadSigningCredentialsAsync( |
There was a problem hiding this comment.
I wonder if a simpler design would be to rely on the existing object model, i.e. ICredentialsLoader and to treat the SymmetricSecurityKey as another type of credential - see CredentialType enum (alongside DecryptKeys, which is also not used by MSAL).
Apart from simpelr object model, this would avoid mixing the concept of SigningCredentials in Id.Web. I believe SigningCredentials + X509 has some perf implications (i.e. loading it often is expensive, caching needs to be done).
| /// <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( |
There was a problem hiding this comment.
Not sure this is needed if CredentialDescription can alraedy return an X509Certificate2 and potentially a SymmetricKey . It becomes just a convenience method.
Description
Add SigningCredential loading to DefaultCredentialsLoader
Fixes #{3359}