Skip to content

Commit 2b20cba

Browse files
bgavrilMSbgavrilMS
committed
Fix for 3000 - improve errors around credentials loading
1 parent c8ba0ce commit 2b20cba

12 files changed

Lines changed: 481 additions & 39 deletions

File tree

‎src/Microsoft.Identity.Web.Certificate/DefaultCredentialsLoader.cs‎

Lines changed: 46 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation. All rights reserved.
22
// Licensed under the MIT License.
33

4+
using System;
45
using System.Collections.Concurrent;
56
using System.Collections.Generic;
67
using System.Threading;
@@ -10,11 +11,34 @@
1011

1112
namespace Microsoft.Identity.Web
1213
{
14+
1315
/// <summary>
1416
/// Default credentials loader.
1517
/// </summary>
1618
public class DefaultCredentialsLoader : ICredentialsLoader
1719
{
20+
/// <summary>
21+
/// Logging infrastructure
22+
/// </summary>
23+
private static class Logger
24+
{
25+
public static void CredentialLoadingFailure(ILogger? logger, CredentialDescription cd, Exception? ex)
26+
{
27+
if (logger != null && logger.IsEnabled(LogLevel.Information))
28+
{
29+
__CredentialLoadingFailure(logger, cd.Id, cd.CredentialType.ToString(), cd.Skip, ex);
30+
}
31+
}
32+
33+
private static readonly Action<ILogger, string, string, bool, Exception?> __CredentialLoadingFailure =
34+
LoggerMessage.Define<string, string, bool>(
35+
LogLevel.Information,
36+
new EventId(
37+
7,
38+
nameof(CredentialLoadingFailure)),
39+
"Failed to load credential {id} from source {sourceType}. Will it be skipped in the future ? {skip}.");
40+
}
41+
1842
ILogger<DefaultCredentialsLoader>? _logger;
1943
private readonly ConcurrentDictionary<string, SemaphoreSlim> _loadingSemaphores = new ConcurrentDictionary<string, SemaphoreSlim>();
2044

@@ -32,7 +56,7 @@ public DefaultCredentialsLoader(ILogger<DefaultCredentialsLoader>? logger)
3256
{ CredentialSource.StoreWithThumbprint, new StoreWithThumbprintCertificateLoader() },
3357
{ CredentialSource.StoreWithDistinguishedName, new StoreWithDistinguishedNameCertificateLoader() },
3458
{ CredentialSource.Base64Encoded, new Base64EncodedCertificateLoader() },
35-
{ CredentialSource.SignedAssertionFromManagedIdentity, new SignedAssertionFromManagedIdentityCredentialLoader() },
59+
{ CredentialSource.SignedAssertionFromManagedIdentity, new SignedAssertionFromManagedIdentityCredentialLoader(_logger) },
3660
{ CredentialSource.SignedAssertionFilePath, new SignedAssertionFilePathCredentialsLoader(_logger) }
3761
};
3862
}
@@ -51,7 +75,8 @@ public DefaultCredentialsLoader() : this(null)
5175
public IDictionary<CredentialSource, ICredentialSourceLoader> CredentialSourceLoaders { get; }
5276

5377
/// <inheritdoc/>
54-
/// Load the credentials from the description, if needed.
78+
/// Load the credentials from the description, if needed.
79+
/// Important: Ignores SKIP flag, propagates exceptions.
5580
public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialDescription, CredentialSourceLoaderParameters? parameters = null)
5681
{
5782
_ = Throws.IfNull(credentialDescription);
@@ -69,7 +94,17 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD
6994
if (credentialDescription.CachedValue == null)
7095
{
7196
if (CredentialSourceLoaders.TryGetValue(credentialDescription.SourceType, out ICredentialSourceLoader? loader))
72-
await loader.LoadIfNeededAsync(credentialDescription, parameters);
97+
{
98+
try
99+
{
100+
await loader.LoadIfNeededAsync(credentialDescription, parameters);
101+
}
102+
catch (Exception ex)
103+
{
104+
Logger.CredentialLoadingFailure(_logger, credentialDescription, ex);
105+
throw;
106+
}
107+
}
73108
}
74109
}
75110
finally
@@ -81,11 +116,18 @@ public async Task LoadCredentialsIfNeededAsync(CredentialDescription credentialD
81116
}
82117

83118
/// <inheritdoc/>
84-
public async Task<CredentialDescription?> LoadFirstValidCredentialsAsync(IEnumerable<CredentialDescription> credentialDescriptions, CredentialSourceLoaderParameters? parameters = null)
119+
/// Loads first valid credential which is not marked as Skipped.
120+
public async Task<CredentialDescription?> LoadFirstValidCredentialsAsync(
121+
IEnumerable<CredentialDescription> credentialDescriptions,
122+
CredentialSourceLoaderParameters? parameters = null)
85123
{
86124
foreach (var credentialDescription in credentialDescriptions)
87125
{
88126
await LoadCredentialsIfNeededAsync(credentialDescription, parameters);
127+
128+
// TODO: Exceptions which occur in trying various credentials are logged and ignored.
129+
// is this really intended behavior? This is different from WithClientCredentialsAsync
130+
89131
if (!credentialDescription.Skip)
90132
{
91133
return credentialDescription;
@@ -107,6 +149,5 @@ public void ResetCredentials(IEnumerable<CredentialDescription> credentialDescri
107149
}
108150
}
109151
}
110-
111152
}
112153
}

‎src/Microsoft.Identity.Web.Certificate/SignedAssertionFilePathCredentialsLoader.cs‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,13 +36,14 @@ public async Task LoadIfNeededAsync(CredentialDescription credentialDescription,
3636
{
3737
// Given that managed identity can be not available locally, we need to try to get a
3838
// signed assertion, and if it fails, move to the next credentials
39-
_= await signedAssertion!.GetSignedAssertionAsync(null);
39+
_ = await signedAssertion!.GetSignedAssertionAsync(null);
4040
credentialDescription.CachedValue = signedAssertion;
4141
}
4242
catch (Exception)
4343
{
4444
credentialDescription.Skip = true;
45-
}
45+
throw;
46+
}
4647
}
4748
}
4849
}

‎src/Microsoft.Identity.Web.Certificate/SignedAssertionFromManagedIdentityCredentialLoader.cs‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,21 @@
44
using System.Threading;
55
using System.Threading.Tasks;
66
using Azure.Identity;
7+
using Microsoft.Extensions.Logging;
78
using Microsoft.Identity.Abstractions;
9+
using Microsoft.Identity.Client;
810

911
namespace Microsoft.Identity.Web
1012
{
1113
internal class SignedAssertionFromManagedIdentityCredentialLoader : ICredentialSourceLoader
1214
{
15+
private ILogger<DefaultCredentialsLoader>? _logger;
16+
17+
public SignedAssertionFromManagedIdentityCredentialLoader(ILogger<DefaultCredentialsLoader>? logger)
18+
{
19+
_logger = logger;
20+
}
21+
1322
public CredentialSource CredentialSource => CredentialSource.SignedAssertionFromManagedIdentity;
1423

1524
public async Task LoadIfNeededAsync(CredentialDescription credentialDescription, CredentialSourceLoaderParameters? credentialSourceLoaderParameters)
@@ -19,16 +28,19 @@ public async Task LoadIfNeededAsync(CredentialDescription credentialDescription,
1928
ManagedIdentityClientAssertion? managedIdentityClientAssertion = credentialDescription.CachedValue as ManagedIdentityClientAssertion;
2029
if (credentialDescription.CachedValue == null)
2130
{
22-
managedIdentityClientAssertion = new ManagedIdentityClientAssertion(credentialDescription.ManagedIdentityClientId, credentialDescription.TokenExchangeUrl);
31+
managedIdentityClientAssertion = new ManagedIdentityClientAssertion(
32+
credentialDescription.ManagedIdentityClientId,
33+
credentialDescription.TokenExchangeUrl,
34+
_logger);
2335
}
2436
try
2537
{
2638
// Given that managed identity can be not available locally, we need to try to get a
2739
// signed assertion, and if it fails, move to the next credentials
28-
_= await managedIdentityClientAssertion!.GetSignedAssertionAsync(null);
40+
_ = await managedIdentityClientAssertion!.GetSignedAssertionAsync(null);
2941
credentialDescription.CachedValue = managedIdentityClientAssertion;
3042
}
31-
catch (AuthenticationFailedException)
43+
catch (MsalServiceException)
3244
{
3345
credentialDescription.Skip = true;
3446
throw;

‎src/Microsoft.Identity.Web.Certificateless/ManagedIdentityClientAssertion.cs‎

Lines changed: 85 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System.Threading;
55
using System.Threading.Tasks;
6+
using Microsoft.Extensions.Logging;
67
using Microsoft.Identity.Client;
78
using Microsoft.Identity.Client.AppConfig;
89
using Microsoft.Identity.Web.Certificateless;
@@ -16,21 +17,27 @@ public class ManagedIdentityClientAssertion : ClientAssertionProviderBase
1617
{
1718
IManagedIdentityApplication _managedIdentityApplication;
1819
private readonly string _tokenExchangeUrl;
20+
private readonly ILogger? _logger;
1921

2022
/// <summary>
2123
/// See https://aka.ms/ms-id-web/certificateless.
2224
/// </summary>
2325
/// <param name="managedIdentityClientId">Optional ClientId of the Managed Identity</param>
24-
public ManagedIdentityClientAssertion(string? managedIdentityClientId)
26+
public ManagedIdentityClientAssertion(string? managedIdentityClientId) :
27+
this (managedIdentityClientId, null)
2528
{
26-
var id = ManagedIdentityId.SystemAssigned;
27-
if (!string.IsNullOrEmpty(managedIdentityClientId))
28-
{
29-
id = ManagedIdentityId.WithUserAssignedClientId(managedIdentityClientId);
30-
}
29+
30+
}
3131

32-
_managedIdentityApplication = ManagedIdentityApplicationBuilder.Create(id).Build();
33-
_tokenExchangeUrl = CertificatelessConstants.DefaultTokenExchangeUrl;
32+
/// <summary>
33+
/// See https://aka.ms/ms-id-web/certificateless.
34+
/// </summary>
35+
/// <param name="managedIdentityClientId">Optional ClientId of the Managed Identity</param>
36+
/// <param name="tokenExchangeUrl">Optional audience of the token to be requested from Managed Identity. Default value is "api://AzureADTokenExchange".
37+
/// This value is different on clouds other than Azure Public</param>
38+
public ManagedIdentityClientAssertion(string? managedIdentityClientId, string? tokenExchangeUrl) :
39+
this (managedIdentityClientId, tokenExchangeUrl, null)
40+
{
3441
}
3542

3643
/// <summary>
@@ -39,9 +46,26 @@ public ManagedIdentityClientAssertion(string? managedIdentityClientId)
3946
/// <param name="managedIdentityClientId">Optional ClientId of the Managed Identity</param>
4047
/// <param name="tokenExchangeUrl">Optional audience of the token to be requested from Managed Identity. Default value is "api://AzureADTokenExchange".
4148
/// This value is different on clouds other than Azure Public</param>
42-
public ManagedIdentityClientAssertion(string? managedIdentityClientId, string? tokenExchangeUrl) : this (managedIdentityClientId)
49+
/// <param name="logger">A logger</param>
50+
public ManagedIdentityClientAssertion(string? managedIdentityClientId, string? tokenExchangeUrl, ILogger? logger)
4351
{
4452
_tokenExchangeUrl = tokenExchangeUrl ?? CertificatelessConstants.DefaultTokenExchangeUrl;
53+
_logger = logger;
54+
55+
var id = ManagedIdentityId.SystemAssigned;
56+
if (!string.IsNullOrEmpty(managedIdentityClientId))
57+
{
58+
id = ManagedIdentityId.WithUserAssignedClientId(managedIdentityClientId);
59+
}
60+
var builder = ManagedIdentityApplicationBuilder.Create(id);
61+
if (_logger != null)
62+
{
63+
builder.WithLogging(Log, ConvertMicrosoftExtensionsLogLevelToMsal(_logger));
64+
_logger?.LogInformation($"ManagedIdentityClientAssertion with tokenExchangeUrl={_tokenExchangeUrl}");
65+
}
66+
67+
_managedIdentityApplication = builder.Build();
68+
_tokenExchangeUrl = CertificatelessConstants.DefaultTokenExchangeUrl;
4569
}
4670

4771
/// <summary>
@@ -52,11 +76,62 @@ public ManagedIdentityClientAssertion(string? managedIdentityClientId, string? t
5276
protected override async Task<ClientAssertion> GetClientAssertionAsync(AssertionRequestOptions? assertionRequestOptions)
5377
{
5478
var result = await _managedIdentityApplication
55-
.AcquireTokenForManagedIdentity(_tokenExchangeUrl)
79+
.AcquireTokenForManagedIdentity(_tokenExchangeUrl)
5680
.ExecuteAsync(assertionRequestOptions?.CancellationToken ?? CancellationToken.None)
5781
.ConfigureAwait(false);
5882

5983
return new ClientAssertion(result.AccessToken, result.ExpiresOn);
6084
}
85+
86+
private void Log(
87+
Client.LogLevel level,
88+
string message,
89+
bool containsPii)
90+
{
91+
switch (level)
92+
{
93+
case Client.LogLevel.Always:
94+
_logger.LogInformation(message);
95+
break;
96+
case Client.LogLevel.Error:
97+
_logger.LogError(message);
98+
break;
99+
case Client.LogLevel.Warning:
100+
_logger.LogWarning(message);
101+
break;
102+
case Client.LogLevel.Info:
103+
_logger.LogInformation(message);
104+
break;
105+
case Client.LogLevel.Verbose:
106+
_logger.LogDebug(message);
107+
break;
108+
}
109+
}
110+
111+
private Client.LogLevel? ConvertMicrosoftExtensionsLogLevelToMsal(ILogger logger)
112+
{
113+
if (logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Debug)
114+
|| logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Trace))
115+
{
116+
return Client.LogLevel.Verbose;
117+
}
118+
else if (logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Information))
119+
{
120+
return Client.LogLevel.Info;
121+
}
122+
else if (logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Warning))
123+
{
124+
return Client.LogLevel.Warning;
125+
}
126+
else if (logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Error)
127+
|| logger.IsEnabled(Microsoft.Extensions.Logging.LogLevel.Critical))
128+
{
129+
return Client.LogLevel.Error;
130+
}
131+
else
132+
{
133+
return null;
134+
}
135+
}
61136
}
62137
}

‎src/Microsoft.Identity.Web.TokenAcquisition/ConfidentialClientApplicationBuilderExtension.Logger.cs‎

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33

44
using System;
55
using Microsoft.Extensions.Logging;
6+
using Microsoft.Identity.Abstractions;
67

78
namespace Microsoft.Identity.Web
89
{
@@ -40,6 +41,62 @@ internal static class Logger
4041
LoggingEventId.UsingCertThumbprint,
4142
"[MsIdWeb] Using certificate Thumbprint={certThumbprint} as client credentials. ");
4243

44+
private static readonly Action<ILogger, string, string, Exception?> s_credentialAttempt =
45+
LoggerMessage.Define<string, string>(
46+
LogLevel.Information,
47+
LoggingEventId.CredentialLoadAttempt,
48+
"[MsIdWeb] Attempting to load the credential from the CredentialDescription with Id={Id} and Skip={Skip} . ");
49+
50+
private static readonly Action<ILogger, string, string, Exception?> s_credentialAttemptFailed =
51+
LoggerMessage.Define<string, string>(
52+
LogLevel.Information,
53+
LoggingEventId.CredentialLoadAttemptFailed,
54+
"[MsIdWeb] Loading the credential from CredentialDescription Id={Id} failed. Will the credential be re-attempted? - {Skip}.");
55+
56+
/// <summary>
57+
/// Logger for attempting to use a CredentialDescription with MSAL
58+
/// </summary>
59+
/// <param name="logger"></param>
60+
/// <param name="certificateDescription"></param>
61+
/// <param name="ex"></param>
62+
public static void AttemptToLoadCredentialsFailed(
63+
ILogger logger,
64+
CredentialDescription certificateDescription,
65+
Exception ex) =>
66+
s_credentialAttemptFailed(
67+
logger,
68+
certificateDescription.Id,
69+
certificateDescription.Skip.ToString(),
70+
ex);
71+
72+
/// <summary>
73+
/// Logger for attempting to use a CredentialDescription with MSAL
74+
/// </summary>
75+
/// <param name="logger"></param>
76+
/// <param name="certificateDescription"></param>
77+
public static void AttemptToLoadCredentials(
78+
ILogger logger,
79+
CredentialDescription certificateDescription) =>
80+
s_credentialAttempt(
81+
logger,
82+
certificateDescription.Id,
83+
certificateDescription.Skip.ToString(),
84+
default!);
85+
86+
/// <summary>
87+
/// Logger for attempting to use a CredentialDescription with MSAL
88+
/// </summary>
89+
/// <param name="logger"></param>
90+
/// <param name="certificateDescription"></param>
91+
public static void FailedToLoadCredentials(
92+
ILogger logger,
93+
CredentialDescription certificateDescription) =>
94+
s_credentialAttemptFailed(
95+
logger,
96+
certificateDescription.Id,
97+
certificateDescription.Skip.ToString(),
98+
default!);
99+
43100
/// <summary>
44101
/// Logger for handling information specific to ConfidentialClientApplicationBuilderExtension.
45102
/// </summary>

0 commit comments

Comments
 (0)