From 4e44fddcc3ec19aa3bdf70434c0160f9e0bf948e Mon Sep 17 00:00:00 2001 From: Jean-Marc Prieur Date: Thu, 18 May 2023 21:20:35 -0700 Subject: [PATCH] Fixes ##2251 and the same issue for WIF for MSI and adds loggin in WIF for AKS --- .../DefaultCertificateLoader.cs | 33 +++++- .../DefaultCredentialsLoader.cs | 42 ++++++-- ...ignedAssertionFilePathCredentialsLoader.cs | 15 ++- ...tionFromManagedIdentityCredentialLoader.cs | 2 +- ...dentityForKubernetesClientAssertion.Log.cs | 101 ++++++++++++++++++ ...ureIdentityForKubernetesClientAssertion.cs | 56 ++++++++-- ...rosoft.Identity.Web.Certificateless.csproj | 1 + .../Constants.cs | 1 + .../TokenAcquisition.cs | 19 ++-- 9 files changed, 237 insertions(+), 33 deletions(-) create mode 100644 src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.Log.cs diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs index ad11c0abf..c370735b5 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCertificateLoader.cs @@ -5,6 +5,7 @@ using System.Collections.Generic; using System.Linq; using System.Security.Cryptography.X509Certificates; +using Microsoft.Extensions.Logging; using Microsoft.Identity.Abstractions; namespace Microsoft.Identity.Web @@ -26,6 +27,22 @@ namespace Microsoft.Identity.Web /// public class DefaultCertificateLoader : DefaultCredentialsLoader, ICertificateLoader { + /// + /// Constructor with a logger. + /// + /// + public DefaultCertificateLoader(ILogger? logger) : base(logger) + { + + } + + /// + /// Default constuctor. + /// + //[Obsolete("Rather use the constructor with a logger")] + public DefaultCertificateLoader() : this(null) + { + } /// /// This default is overridable at the level of the credential description (for the certificate from KeyVault). @@ -50,7 +67,7 @@ public static string? UserAssignedManagedIdentityClientId /// First certificate in the certificate description list. public static X509Certificate2? LoadFirstCertificate(IEnumerable certificateDescriptions) { - DefaultCertificateLoader defaultCertificateLoader = new(); + DefaultCertificateLoader defaultCertificateLoader = new(null); CertificateDescription? certDescription = certificateDescriptions.FirstOrDefault(c => { defaultCertificateLoader.LoadCredentialsIfNeededAsync(c).GetAwaiter().GetResult(); @@ -67,12 +84,22 @@ public static string? UserAssignedManagedIdentityClientId /// All the certificates in the certificate description list. public static IEnumerable LoadAllCertificates(IEnumerable certificateDescriptions) { - DefaultCertificateLoader defaultCertificateLoader = new(); + DefaultCertificateLoader defaultCertificateLoader = new(null); + return defaultCertificateLoader.LoadCertificates(certificateDescriptions); + } + + /// + /// Load the certificates from the certificate description list. + /// + /// + /// a collection of certificates + private IEnumerable LoadCertificates(IEnumerable certificateDescriptions) + { if (certificateDescriptions != null) { foreach (var certDescription in certificateDescriptions) { - defaultCertificateLoader.LoadCredentialsIfNeededAsync(certDescription).GetAwaiter().GetResult(); + LoadCredentialsIfNeededAsync(certDescription).GetAwaiter().GetResult(); if (certDescription.Certificate != null) { yield return certDescription.Certificate; diff --git a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs index af0de9211..4c5380fd4 100644 --- a/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs @@ -1,8 +1,10 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. +using System; using System.Collections.Generic; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; using Microsoft.Identity.Abstractions; namespace Microsoft.Identity.Web @@ -12,20 +14,40 @@ namespace Microsoft.Identity.Web /// public class DefaultCredentialsLoader : ICredentialsLoader { + ILogger? _logger; + + /// + /// Constructor with a logger + /// + /// + public DefaultCredentialsLoader(ILogger? logger) + { + _logger = logger; + CredentialSourceLoaders = new Dictionary + { + { CredentialSource.KeyVault, new KeyVaultCertificateLoader() }, + { CredentialSource.Path, new FromPathCertificateLoader() }, + { CredentialSource.StoreWithThumbprint, new StoreWithThumbprintCertificateLoader() }, + { CredentialSource.StoreWithDistinguishedName, new StoreWithDistinguishedNameCertificateLoader() }, + { CredentialSource.Base64Encoded, new Base64EncodedCertificateLoader() }, + { CredentialSource.SignedAssertionFromManagedIdentity, new SignedAssertionFromManagedIdentityCredentialLoader() }, + { CredentialSource.SignedAssertionFilePath, new SignedAssertionFilePathCredentialsLoader(_logger) } + }; + } + + /// + /// Default constructor (for backward compatibility) + /// + //[Obsolete("Rather use the constructor with a logger.")] + public DefaultCredentialsLoader() : this(null) + { + } + /// /// Dictionary of credential loaders per credential source. The application can add more to /// process additional credential sources(like dSMS). /// - public IDictionary CredentialSourceLoaders { get; } = new Dictionary - { - { CredentialSource.KeyVault, new KeyVaultCertificateLoader() }, - { CredentialSource.Path, new FromPathCertificateLoader() }, - { CredentialSource.StoreWithThumbprint, new StoreWithThumbprintCertificateLoader() }, - { CredentialSource.StoreWithDistinguishedName, new StoreWithDistinguishedNameCertificateLoader() }, - { CredentialSource.Base64Encoded, new Base64EncodedCertificateLoader() }, - { CredentialSource.SignedAssertionFromManagedIdentity, new SignedAssertionFromManagedIdentityCredentialLoader() }, - { CredentialSource.SignedAssertionFilePath, new SignedAssertionFilePathCredentialsLoader() }, - }; + public IDictionary CredentialSourceLoaders { get; } /// /// Load the credentials from the description, if needed. diff --git a/src/Microsoft.Identity.Web.Certificate/SignedAssertionFilePathCredentialsLoader.cs b/src/Microsoft.Identity.Web.Certificate/SignedAssertionFilePathCredentialsLoader.cs index 6ca2bb878..bf9a40dc7 100644 --- a/src/Microsoft.Identity.Web.Certificate/SignedAssertionFilePathCredentialsLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/SignedAssertionFilePathCredentialsLoader.cs @@ -5,11 +5,22 @@ using System.Threading; using Microsoft.Identity.Abstractions; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; namespace Microsoft.Identity.Web { internal class SignedAssertionFilePathCredentialsLoader : ICredentialSourceLoader { + ILogger? _logger; + + /// + /// Constructor + /// + /// Optional logger. + public SignedAssertionFilePathCredentialsLoader(ILogger? logger) + { + _logger = logger; + } public CredentialSource CredentialSource => CredentialSource.SignedAssertionFilePath; public async Task LoadIfNeededAsync(CredentialDescription credentialDescription, CredentialSourceLoaderParameters? credentialSourceLoaderParameters) @@ -19,17 +30,17 @@ public async Task LoadIfNeededAsync(CredentialDescription credentialDescription, AzureIdentityForKubernetesClientAssertion? signedAssertion = credentialDescription.CachedValue as AzureIdentityForKubernetesClientAssertion; if (credentialDescription.CachedValue == null) { - signedAssertion = new AzureIdentityForKubernetesClientAssertion(credentialDescription.SignedAssertionFileDiskPath); + signedAssertion = new AzureIdentityForKubernetesClientAssertion(credentialDescription.SignedAssertionFileDiskPath, _logger); } try { // Given that managed identity can be not available locally, we need to try to get a // signed assertion, and if it fails, move to the next credentials _= await signedAssertion!.GetSignedAssertion(CancellationToken.None); + credentialDescription.CachedValue = signedAssertion; } catch (Exception) { - credentialDescription.CachedValue = signedAssertion; credentialDescription.Skip = true; } } diff --git a/src/Microsoft.Identity.Web.Certificate/SignedAssertionFromManagedIdentityCredentialLoader.cs b/src/Microsoft.Identity.Web.Certificate/SignedAssertionFromManagedIdentityCredentialLoader.cs index 9a3056cef..1067e1af4 100644 --- a/src/Microsoft.Identity.Web.Certificate/SignedAssertionFromManagedIdentityCredentialLoader.cs +++ b/src/Microsoft.Identity.Web.Certificate/SignedAssertionFromManagedIdentityCredentialLoader.cs @@ -30,10 +30,10 @@ public async Task LoadIfNeededAsync(CredentialDescription credentialDescription, // Given that managed identity can be not available locally, we need to try to get a // signed assertion, and if it fails, move to the next credentials _= await managedIdentityClientAssertion!.GetSignedAssertion(CancellationToken.None); + credentialDescription.CachedValue = managedIdentityClientAssertion; } catch (AuthenticationFailedException) { - credentialDescription.CachedValue = managedIdentityClientAssertion; credentialDescription.Skip = true; throw; } diff --git a/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.Log.cs b/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.Log.cs new file mode 100644 index 000000000..5fc15ec2c --- /dev/null +++ b/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.Log.cs @@ -0,0 +1,101 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +using System; +using Microsoft.Extensions.Logging; + +namespace Microsoft.Identity.Web +{ + public partial class AzureIdentityForKubernetesClientAssertion + { + /* + // High performance logger messages (before generation). + #pragma warning disable SYSLIB1009 // Logging methods must be static + [LoggerMessage(EventId = 1, Level = LogLevel.Information, Message = "SignedAssertionFileDiskPath not provided. Falling back to the content of the AZURE_FEDERATED_TOKEN_FILE environment variable.")] + partial void SignedAssertionFileDiskPathNotProvided(ILogger logger); + + [LoggerMessage(EventId = 2, Level = LogLevel.Information, Message = "The `{environmentVariableName}` environment variable not provided.")] + partial void SignedAssertionEnvironmentVariableNotProvided(ILogger logger, string environmentVariableName); + + [LoggerMessage(EventId = 3, Level = LogLevel.Error, Message = "The environment variable AZURE_FEDERATED_TOKEN_FILE or AZURE_ACCESS_TOKEN_FILE or the 'SignedAssertionFileDiskPath' must be set to the path of the file containing the signed assertion.")] + partial void NoSignedAssertionParameterProvided(ILogger logger); + + [LoggerMessage(EventId = 4, Level = LogLevel.Error, Message = "The file `{filePath}` containing the signed assertion was not found.")] + partial void FileAssertionPathNotFound(ILogger logger, string filePath); + + [LoggerMessage(EventId = 5, Level = LogLevel.Information, Message = "Successfully read the signed assertion for `{filePath}`. Expires at {expiry}.")] + partial void SuccessFullyReadSignedAssertion(ILogger logger, string filePath, DateTime expiry); + + [LoggerMessage(EventId = 6, Level = LogLevel.Error, Message = "The file `{filePath} does not contain a valid signed assertion. {message}.")] + partial void FileDoesNotContainValidAssertion(ILogger logger, string filePath, string message); + #pragma warning restore SYSLIB1009 // Logging methods must be static + */ + + /// + /// Performant logging messages. + /// + static class Log + { + private static readonly Action __SignedAssertionFileDiskPathNotProvidedCallback = + LoggerMessage.Define(LogLevel.Information, new EventId(1, nameof(SignedAssertionFileDiskPathNotProvided)), "SignedAssertionFileDiskPath not provided. Falling back to the content of the AZURE_FEDERATED_TOKEN_FILE environment variable."); + + public static void SignedAssertionFileDiskPathNotProvided(ILogger? logger) + { + if (logger != null && logger.IsEnabled(LogLevel.Information)) + { + __SignedAssertionFileDiskPathNotProvidedCallback(logger, null); + } + } + private static readonly Action __SignedAssertionEnvironmentVariableNotProvidedCallback = + LoggerMessage.Define(LogLevel.Information, new EventId(2, nameof(SignedAssertionEnvironmentVariableNotProvided)), "The `{environmentVariableName}` environment variable not provided."); + + public static void SignedAssertionEnvironmentVariableNotProvided(ILogger? logger, string environmentVariableName) + { + if (logger != null && logger.IsEnabled(LogLevel.Information)) + { + __SignedAssertionEnvironmentVariableNotProvidedCallback(logger, environmentVariableName, null); + } + } + private static readonly Action __NoSignedAssertionParameterProvidedCallback = + LoggerMessage.Define(LogLevel.Error, new EventId(3, nameof(NoSignedAssertionParameterProvided)), "The environment variable AZURE_FEDERATED_TOKEN_FILE or AZURE_ACCESS_TOKEN_FILE or the 'SignedAssertionFileDiskPath' must be set to the path of the file containing the signed assertion."); + + public static void NoSignedAssertionParameterProvided(ILogger? logger) + { + if (logger != null && logger.IsEnabled(LogLevel.Error)) + { + __NoSignedAssertionParameterProvidedCallback(logger, null); + } + } + private static readonly Action __FileAssertionPathNotFoundCallback = + LoggerMessage.Define(LogLevel.Error, new EventId(4, nameof(FileAssertionPathNotFound)), "The file `{filePath}` containing the signed assertion was not found."); + + public static void FileAssertionPathNotFound(ILogger? logger, string filePath) + { + if (logger != null && logger.IsEnabled(LogLevel.Error)) + { + __FileAssertionPathNotFoundCallback(logger, filePath, null); + } + } + private static readonly Action __SuccessFullyReadSignedAssertionCallback = + LoggerMessage.Define(LogLevel.Information, new EventId(5, nameof(SuccessFullyReadSignedAssertion)), "Successfully read the signed assertion for `{filePath}`. Expires at {expiry}."); + + public static void SuccessFullyReadSignedAssertion(ILogger? logger, string filePath, DateTime expiry) + { + if (logger != null && logger.IsEnabled(LogLevel.Information)) + { + __SuccessFullyReadSignedAssertionCallback(logger, filePath, expiry, null); + } + } + private static readonly Action __FileDoesNotContainValidAssertionCallback = + LoggerMessage.Define(LogLevel.Error, new EventId(6, nameof(FileDoesNotContainValidAssertion)), "The file `{filePath} does not contain a valid signed assertion. {message}."); + + public static void FileDoesNotContainValidAssertion(ILogger? logger, string filePath, string message) + { + if (logger != null && logger.IsEnabled(LogLevel.Error)) + { + __FileDoesNotContainValidAssertionCallback(logger, filePath, message, null); + } + } + } + } +} diff --git a/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.cs b/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.cs index 96bb346a7..26a8fb82b 100644 --- a/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.cs +++ b/src/Microsoft.Identity.Web.Certificateless/AzureIdentityForKubernetesClientAssertion.cs @@ -5,6 +5,7 @@ using System.IO; using System.Threading; using System.Threading.Tasks; +using Microsoft.Extensions.Logging; using Microsoft.IdentityModel.JsonWebTokens; namespace Microsoft.Identity.Web @@ -14,14 +15,14 @@ namespace Microsoft.Identity.Web /// in Azure Kubernetes Services. See https://aka.ms/ms-id-web/certificateless and /// https://learn.microsoft.com/azure/aks/workload-identity-overview /// - public class AzureIdentityForKubernetesClientAssertion : ClientAssertionProviderBase + public partial class AzureIdentityForKubernetesClientAssertion : ClientAssertionProviderBase { /// /// Gets a signed assertion from Azure workload identity for kubernetes. The file path is provided /// by an environment variable ("AZURE_FEDERATED_TOKEN_FILE") /// See https://aka.ms/ms-id-web/certificateless. /// - public AzureIdentityForKubernetesClientAssertion() : this(null) + public AzureIdentityForKubernetesClientAssertion(ILogger? logger = null) : this(null, logger) { } @@ -29,14 +30,35 @@ public AzureIdentityForKubernetesClientAssertion() : this(null) /// Gets a signed assertion from a file. /// See https://aka.ms/ms-id-web/certificateless. /// - /// - public AzureIdentityForKubernetesClientAssertion(string? filePath) + /// Path to a file containing the signed assertion. + /// Logger. + public AzureIdentityForKubernetesClientAssertion(string? filePath, ILogger? logger = null) { + _logger = logger; + + if (filePath == null) + { + Log.SignedAssertionFileDiskPathNotProvided(_logger); + } + + _filePath = _filePath ?? Environment.GetEnvironmentVariable("AZURE_ACCESS_TOKEN_FILE"); + if (filePath == null) + { + Log.SignedAssertionEnvironmentVariableNotProvided(_logger, "AZURE_ACCESS_TOKEN_FILE"); + } + // See https://blog.identitydigest.com/azuread-federate-k8s/ _filePath = filePath ?? Environment.GetEnvironmentVariable("AZURE_FEDERATED_TOKEN_FILE"); + if (_filePath == null) + { + Log.SignedAssertionEnvironmentVariableNotProvided(_logger, "AZURE_FEDERATED_TOKEN_FILE"); + Log.NoSignedAssertionParameterProvided(_logger); + } } - private readonly string _filePath; + private readonly string? _filePath; + + private readonly ILogger? _logger; /// /// Get the signed assertion from a file. @@ -44,10 +66,28 @@ public AzureIdentityForKubernetesClientAssertion(string? filePath) /// The signed assertion. protected override Task GetClientAssertion(CancellationToken cancellationToken) { + if (_filePath != null && !File.Exists(_filePath)) + { + Log.FileAssertionPathNotFound(_logger, _filePath); + throw new FileNotFoundException($"The file '{_filePath}' containing the signed assertion was not found."); + + } string signedAssertion = File.ReadAllText(_filePath); - // Compute the expiry - JsonWebToken jwt = new JsonWebToken(signedAssertion); - return Task.FromResult(new ClientAssertion(signedAssertion, jwt.ValidTo)); + + // Verify that the assertion is a JWS, JWE, and computes the expiry + try + { + JsonWebToken jwt = new JsonWebToken(signedAssertion); + + Log.SuccessFullyReadSignedAssertion(_logger, _filePath!, jwt.ValidTo); + + return Task.FromResult(new ClientAssertion(signedAssertion, jwt.ValidTo)); + } + catch (ArgumentException ex) + { + Log.FileDoesNotContainValidAssertion(_logger, _filePath!, ex.Message); + throw; + } } } } diff --git a/src/Microsoft.Identity.Web.Certificateless/Microsoft.Identity.Web.Certificateless.csproj b/src/Microsoft.Identity.Web.Certificateless/Microsoft.Identity.Web.Certificateless.csproj index e9ade8f95..8bb144a0f 100644 --- a/src/Microsoft.Identity.Web.Certificateless/Microsoft.Identity.Web.Certificateless.csproj +++ b/src/Microsoft.Identity.Web.Certificateless/Microsoft.Identity.Web.Certificateless.csproj @@ -12,6 +12,7 @@ + diff --git a/src/Microsoft.Identity.Web.TokenAcquisition/Constants.cs b/src/Microsoft.Identity.Web.TokenAcquisition/Constants.cs index 0fd7c6d76..7a1dc889f 100644 --- a/src/Microsoft.Identity.Web.TokenAcquisition/Constants.cs +++ b/src/Microsoft.Identity.Web.TokenAcquisition/Constants.cs @@ -128,6 +128,7 @@ public static class Constants internal const string True = "True"; internal const string InvalidClient = "invalid_client"; internal const string InvalidKeyError = "AADSTS700027"; + internal const string SignedAssertionInvalidTimeRange = "AADSTS700024"; internal const string CiamAuthoritySuffix = ".ciamlogin.com"; internal const string TestSlice = "dc"; diff --git a/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs b/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs index d412ede3a..2d4153d2f 100644 --- a/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs +++ b/src/Microsoft.Identity.Web.TokenAcquisition/TokenAcquisition.cs @@ -167,7 +167,7 @@ public async Task AddAccountToCacheFromAuthorizationCodeAsyn result.CorrelationId, result.TokenType); } - catch (MsalServiceException exMsal) when (IsInvalidClientCertificateError(exMsal)) + catch (MsalServiceException exMsal) when (IsInvalidClientCertificateOrSignedAssertionError(exMsal)) { DefaultCertificateLoader.ResetCertificates(mergedOptions.ClientCertificates); _applicationsByAuthorityClientId[GetApplicationKey(mergedOptions)] = null; @@ -262,7 +262,7 @@ public async Task GetAuthenticationResultForUserAsync( LogAuthResult(authenticationResult); return authenticationResult; } - catch (MsalServiceException exMsal) when (IsInvalidClientCertificateError(exMsal)) + catch (MsalServiceException exMsal) when (IsInvalidClientCertificateOrSignedAssertionError(exMsal)) { DefaultCertificateLoader.ResetCertificates(mergedOptions.ClientCertificates); _applicationsByAuthorityClientId[GetApplicationKey(mergedOptions)] = null; @@ -356,13 +356,13 @@ public Task GetAuthenticationResultForAppAsync( // MSAL.net only allows .WithTenantId for AAD authorities. This makes sense as there should // not be cross tenant operations with such an authority. - if (!mergedOptions.Instance.Contains(".ciamlogin.com" + if (!mergedOptions.Instance.Contains(Constants.CiamAuthoritySuffix #if NETCOREAPP3_1_OR_GREATER , StringComparison.OrdinalIgnoreCase #endif )) - { - builder.WithTenantId(tenant); + { + builder.WithTenantId(tenant); } if (tokenAcquisitionOptions != null) @@ -414,7 +414,7 @@ public Task GetAuthenticationResultForAppAsync( { return builder.ExecuteAsync(tokenAcquisitionOptions != null ? tokenAcquisitionOptions.CancellationToken : CancellationToken.None); } - catch (MsalServiceException exMsal) when (IsInvalidClientCertificateError(exMsal)) + catch (MsalServiceException exMsal) when (IsInvalidClientCertificateOrSignedAssertionError(exMsal)) { DefaultCertificateLoader.ResetCertificates(mergedOptions.ClientCertificates); _applicationsByAuthorityClientId[GetApplicationKey(mergedOptions)] = null; @@ -540,14 +540,15 @@ public async Task RemoveAccountAsync( } } - private bool IsInvalidClientCertificateError(MsalServiceException exMsal) + private bool IsInvalidClientCertificateOrSignedAssertionError(MsalServiceException exMsal) { return !_retryClientCertificate && string.Equals(exMsal.ErrorCode, Constants.InvalidClient, StringComparison.OrdinalIgnoreCase) && #if !NETSTANDARD2_0 && !NET462 && !NET472 - exMsal.Message.Contains(Constants.InvalidKeyError, StringComparison.OrdinalIgnoreCase); + (exMsal.Message.Contains(Constants.InvalidKeyError, StringComparison.OrdinalIgnoreCase) + || exMsal.Message.Contains(Constants.SignedAssertionInvalidTimeRange, StringComparison.OrdinalIgnoreCase)); #else - exMsal.Message.Contains(Constants.InvalidKeyError); + (exMsal.Message.Contains(Constants.InvalidKeyError) || exMsal.Message.Contains(Constants.SignedAssertionInvalidTimeRange)); #endif }