From 259c1747167a996d4b89cf4d24cd00d5ac05a16c Mon Sep 17 00:00:00 2001 From: Joel Mut <62260472+sw-joelmut@users.noreply.github.com> Date: Tue, 28 May 2024 12:26:11 -0300 Subject: [PATCH] fix: Add Audience for Certificate auth to work with Skills (#6794) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Fix Certificate auth in Skills * Ensure thread safety for CertificateServiceClientCredentialsFactory --------- Co-authored-by: Andrés Robinet --- .../CertificateAppCredentials.cs | 19 +++++++++- ...tificateServiceClientCredentialsFactory.cs | 37 +++++++++++++------ ...ateServiceClientCredentialsFactoryTests.cs | 24 +++++++++++- 3 files changed, 65 insertions(+), 15 deletions(-) diff --git a/libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs b/libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs index c6b6c86295..584dfc5e1c 100644 --- a/libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs +++ b/libraries/Microsoft.Bot.Connector/Authentication/CertificateAppCredentials.cs @@ -48,7 +48,7 @@ public CertificateAppCredentials(CertificateAppCredentialsOptions options) /// Optional to be used when acquiring tokens. /// Optional to gather telemetry data while acquiring and managing credentials. public CertificateAppCredentials(X509Certificate2 clientCertificate, string appId, string channelAuthTenant = null, HttpClient customHttpClient = null, ILogger logger = null) - : this(clientCertificate, false, appId, channelAuthTenant, customHttpClient, logger) + : this(clientCertificate, appId, channelAuthTenant, string.Empty, false, customHttpClient, logger) { } @@ -62,7 +62,22 @@ public CertificateAppCredentials(X509Certificate2 clientCertificate, string appI /// Optional to be used when acquiring tokens. /// Optional to gather telemetry data while acquiring and managing credentials. public CertificateAppCredentials(X509Certificate2 clientCertificate, bool sendX5c, string appId, string channelAuthTenant = null, HttpClient customHttpClient = null, ILogger logger = null) - : base(channelAuthTenant, customHttpClient, logger) + : this(clientCertificate, appId, channelAuthTenant, string.Empty, sendX5c, customHttpClient, logger) + { + } + + /// + /// Initializes a new instance of the class. + /// + /// Client certificate to be presented for authentication. + /// Microsoft application Id related to the certifiacte. + /// Optional. The oauth token tenant. + /// Optional. The scope for the token. + /// Optional. This parameter, if true, enables application developers to achieve easy certificates roll-over in Azure AD: setting this parameter to true will send the public certificate to Azure AD along with the token request, so that Azure AD can use it to validate the subject name based on a trusted issuer policy. + /// Optional to be used when acquiring tokens. + /// Optional to gather telemetry data while acquiring and managing credentials. + public CertificateAppCredentials(X509Certificate2 clientCertificate, string appId, string channelAuthTenant = null, string oAuthScope = null, bool sendX5c = false, HttpClient customHttpClient = null, ILogger logger = null) + : base(channelAuthTenant, customHttpClient, logger, oAuthScope: oAuthScope) { if (clientCertificate == null) { diff --git a/libraries/Microsoft.Bot.Connector/Authentication/CertificateServiceClientCredentialsFactory.cs b/libraries/Microsoft.Bot.Connector/Authentication/CertificateServiceClientCredentialsFactory.cs index 5243ec00df..b8defbc1dc 100644 --- a/libraries/Microsoft.Bot.Connector/Authentication/CertificateServiceClientCredentialsFactory.cs +++ b/libraries/Microsoft.Bot.Connector/Authentication/CertificateServiceClientCredentialsFactory.cs @@ -2,6 +2,7 @@ // Licensed under the MIT License. using System; +using System.Collections.Concurrent; using System.Net.Http; using System.Security.Cryptography.X509Certificates; using System.Threading; @@ -16,8 +17,13 @@ namespace Microsoft.Bot.Connector.Authentication /// public class CertificateServiceClientCredentialsFactory : ServiceClientCredentialsFactory { - private readonly CertificateAppCredentials _certificateAppCredentials; + private readonly X509Certificate2 _certificate; private readonly string _appId; + private readonly string _tenantId; + private readonly bool _sendX5c; + private readonly HttpClient _httpClient; + private readonly ILogger _logger; + private readonly ConcurrentDictionary _certificateAppCredentialsByAudience = new ConcurrentDictionary(); /// /// Initializes a new instance of the class. @@ -44,16 +50,12 @@ public CertificateServiceClientCredentialsFactory( throw new ArgumentNullException(nameof(appId)); } + _certificate = certificate ?? throw new ArgumentNullException(nameof(certificate)); _appId = appId; - - // Instance must be reused otherwise it will cause throttling on AAD. - _certificateAppCredentials = new CertificateAppCredentials( - certificate ?? throw new ArgumentNullException(nameof(certificate)), - sendX5c, - appId, - tenantId, - httpClient, - logger); + _tenantId = tenantId; + _sendX5c = sendX5c; + _httpClient = httpClient; + _logger = logger; } /// @@ -78,7 +80,20 @@ public override Task CreateCredentialsAsync( throw new InvalidOperationException("Invalid Managed ID."); } - return Task.FromResult(_certificateAppCredentials); + // Instance must be reused per audience, otherwise it will cause throttling on AAD. + var certificateAppCredentials = _certificateAppCredentialsByAudience.GetOrAdd(audience, (audience) => + { + return new CertificateAppCredentials( + _certificate, + _appId, + _tenantId, + audience, + _sendX5c, + _httpClient, + _logger); + }); + + return Task.FromResult(certificateAppCredentials); } } } diff --git a/tests/Microsoft.Bot.Connector.Tests/Authentication/CertificateServiceClientCredentialsFactoryTests.cs b/tests/Microsoft.Bot.Connector.Tests/Authentication/CertificateServiceClientCredentialsFactoryTests.cs index d76a54bd3b..e72b68227b 100644 --- a/tests/Microsoft.Bot.Connector.Tests/Authentication/CertificateServiceClientCredentialsFactoryTests.cs +++ b/tests/Microsoft.Bot.Connector.Tests/Authentication/CertificateServiceClientCredentialsFactoryTests.cs @@ -17,6 +17,7 @@ public class CertificateServiceClientCredentialsFactoryTests private const string TestAppId = nameof(TestAppId); private const string TestTenantId = nameof(TestTenantId); private const string TestAudience = nameof(TestAudience); + private const string LoginEndpoint = "https://login.microsoftonline.com"; private readonly Mock logger = new Mock(); private readonly Mock certificate = new Mock(); @@ -68,19 +69,38 @@ public async void CanCreateCredentials() var factory = new CertificateServiceClientCredentialsFactory(certificate.Object, TestAppId); var credentials = await factory.CreateCredentialsAsync( - TestAppId, TestAudience, "https://login.microsoftonline.com", true, CancellationToken.None); + TestAppId, TestAudience, LoginEndpoint, true, CancellationToken.None); Assert.NotNull(credentials); Assert.IsType(credentials); } + [Fact] + public async void ShouldCreateUniqueCredentialsByAudience() + { + var factory = new CertificateServiceClientCredentialsFactory(certificate.Object, TestAppId); + + var credentials1 = await factory.CreateCredentialsAsync( + TestAppId, string.Empty, LoginEndpoint, true, CancellationToken.None); + var credentials2 = await factory.CreateCredentialsAsync( + TestAppId, TestAudience, LoginEndpoint, true, CancellationToken.None); + var credentials3 = await factory.CreateCredentialsAsync( + TestAppId, Guid.NewGuid().ToString(), LoginEndpoint, true, CancellationToken.None); + var credentials4 = await factory.CreateCredentialsAsync( + TestAppId, string.Empty, LoginEndpoint, true, CancellationToken.None); + + Assert.NotEqual(credentials1, credentials2); + Assert.NotEqual(credentials1, credentials3); + Assert.Equal(credentials1, credentials4); + } + [Fact] public void CannotCreateCredentialsWithInvalidAppId() { var factory = new CertificateServiceClientCredentialsFactory(certificate.Object, TestAppId); Assert.ThrowsAsync(() => factory.CreateCredentialsAsync( - "InvalidAppId", TestAudience, "https://login.microsoftonline.com", true, CancellationToken.None)); + "InvalidAppId", TestAudience, LoginEndpoint, true, CancellationToken.None)); } } }