From 2da61b13de0f0fb0081671c43f94b5c4e65c6b53 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 13 Dec 2023 13:52:04 -0600 Subject: [PATCH 1/4] Add failing test Demonstrates that the back channel logout service will create tokens with a bad issuer if it is missing from the logout request --- .../DefaultBackChannelLogoutServiceTests.cs | 72 +++++++++++++++++++ 1 file changed, 72 insertions(+) create mode 100644 test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs diff --git a/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs b/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs new file mode 100644 index 000000000..f8228fd36 --- /dev/null +++ b/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs @@ -0,0 +1,72 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + + +using System.Collections.Generic; +using System.Text.Json; +using System.Threading.Tasks; +using Duende.IdentityServer; +using Duende.IdentityServer.Configuration; +using Duende.IdentityServer.Services; +using FluentAssertions; +using IdentityModel; +using Microsoft.AspNetCore.Authentication; +using Microsoft.Extensions.Logging; +using Microsoft.IdentityModel.Tokens; +using UnitTests.Common; +using UnitTests.Services.Default.KeyManagement; +using UnitTests.Validation.Setup; +using Xunit; + +namespace UnitTests.Services.Default; + +public class DefaultBackChannelLogoutServiceTests +{ + private class ServiceTestHarness : DefaultBackChannelLogoutService + { + public ServiceTestHarness(ISystemClock clock, + IdentityServerTools tools, + ILogoutNotificationService logoutNotificationService, + IBackChannelLogoutHttpClient backChannelLogoutHttpClient, + ILogger logger) + : base(clock, tools, logoutNotificationService, backChannelLogoutHttpClient, logger) + { + } + + // CreateTokenAsync is protected, so we use this wrapper to exercise it in our tests + public async Task ExerciseCreateTokenAsync(BackChannelLogoutRequest request) + { + return await CreateTokenAsync(request); + } + } + + [Fact] + public async Task CreateTokenAsync_Should_Set_Issuer_Correctly() + { + var expected = "https://identity.example.com"; + + var mockKeyMaterialService = new MockKeyMaterialService(); + var signingKey = new SigningCredentials(CryptoHelper.CreateRsaSecurityKey(), CryptoHelper.GetRsaSigningAlgorithmValue(IdentityServerConstants.RsaSigningAlgorithm.RS256)); + mockKeyMaterialService.SigningCredentials.Add(signingKey); + + var tokenCreation = new DefaultTokenCreationService(new MockClock(), mockKeyMaterialService, TestIdentityServerOptions.Create(), TestLogger.Create()); + + var tools = new IdentityServerTools( + null, // service provider is unused + new TestIssuerNameService(expected), + tokenCreation, + new MockClock() + ); + + var subject = new ServiceTestHarness(null, tools, null, null, null); + var rawToken = await subject.ExerciseCreateTokenAsync(new BackChannelLogoutRequest + { + ClientId = "test_client", + SubjectId = "test_sub", + }); + + + var payload = JsonSerializer.Deserialize>(Base64Url.Decode(rawToken.Split('.')[1])); + payload["iss"].GetString().Should().Be(expected); + } +} \ No newline at end of file From c7dd81b5bdecd91b74da3bd0aa1d571ecb377f6f Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 13 Dec 2023 13:52:29 -0600 Subject: [PATCH 2/4] Fix xmldoc typo, add a todo --- src/IdentityServer/IdentityServerTools.cs | 2 +- .../Services/Default/DefaultBackChannelLogoutService.cs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/IdentityServer/IdentityServerTools.cs b/src/IdentityServer/IdentityServerTools.cs index 15d69edeb..c9ca8dc68 100644 --- a/src/IdentityServer/IdentityServerTools.cs +++ b/src/IdentityServer/IdentityServerTools.cs @@ -19,7 +19,7 @@ namespace Duende.IdentityServer; /// public class IdentityServerTools { - internal readonly IServiceProvider ServiceProvider; + internal readonly IServiceProvider ServiceProvider; // TODO - consider removing this, as it is not used. internal readonly IIssuerNameService IssuerNameService; private readonly ITokenCreationService _tokenCreation; private readonly ISystemClock _clock; diff --git a/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs b/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs index 241786d79..dbd9e4ad6 100644 --- a/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs +++ b/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs @@ -30,7 +30,7 @@ public class DefaultBackChannelLogoutService : IBackChannelLogoutService protected ISystemClock Clock { get; } /// - /// The IdentityServerTools used to create and the JWT. + /// The IdentityServerTools used to create the JWT. /// protected IdentityServerTools Tools { get; } From 2f575dcb2f4cebcb4371257ea1aa93c0893af60a Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 13 Dec 2023 14:03:11 -0600 Subject: [PATCH 3/4] Fix logout token iss claim when issuer is missing --- src/IdentityServer/IdentityServerTools.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/IdentityServer/IdentityServerTools.cs b/src/IdentityServer/IdentityServerTools.cs index c9ca8dc68..02ed1e09b 100644 --- a/src/IdentityServer/IdentityServerTools.cs +++ b/src/IdentityServer/IdentityServerTools.cs @@ -48,22 +48,23 @@ public IdentityServerTools(IServiceProvider serviceProvider, IIssuerNameService /// claims public virtual async Task IssueJwtAsync(int lifetime, IEnumerable claims) { + var tokenType = OidcConstants.TokenTypes.AccessToken; var issuer = await IssuerNameService.GetCurrentAsync(); - return await IssueJwtAsync(lifetime, issuer, claims); + return await IssueJwtAsync(lifetime, issuer, tokenType, claims); } /// /// Issues a JWT. /// /// The lifetime. - /// The issuer. + /// The token type. /// The claims. /// /// claims - public virtual Task IssueJwtAsync(int lifetime, string issuer, IEnumerable claims) + public virtual async Task IssueJwtAsync(int lifetime, string tokenType, IEnumerable claims) { - var tokenType = OidcConstants.TokenTypes.AccessToken; - return IssueJwtAsync(lifetime, issuer, tokenType, claims); + var issuer = await IssuerNameService.GetCurrentAsync(); + return await IssueJwtAsync(lifetime, issuer, tokenType, claims); } /// From ea5617b437b0c9ace1a1868f69fe4ad0e06ec329 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Wed, 13 Dec 2023 21:01:55 -0600 Subject: [PATCH 4/4] Rework fix for logout token issuer bug Moved the fix from IdentityServerTools into the back channel logout service to avoid breaking changes in IdentityServerTools. While technically this is a breaking change in the back channel service too, this makes the impact much smaller, and more likely to be caught by the compiler. The previous implementation changed the semantics of a string parameter in a way that could have been surprising, vs this way adds a new constructor parameter in a way that will be totally obvious and caught by the compiler. --- src/IdentityServer/IdentityServerTools.cs | 11 +++++----- .../DefaultBackChannelLogoutService.cs | 11 +++++++++- .../DefaultBackChannelLogoutServiceTests.cs | 20 +++++++++++-------- 3 files changed, 27 insertions(+), 15 deletions(-) diff --git a/src/IdentityServer/IdentityServerTools.cs b/src/IdentityServer/IdentityServerTools.cs index 02ed1e09b..c9ca8dc68 100644 --- a/src/IdentityServer/IdentityServerTools.cs +++ b/src/IdentityServer/IdentityServerTools.cs @@ -48,23 +48,22 @@ public IdentityServerTools(IServiceProvider serviceProvider, IIssuerNameService /// claims public virtual async Task IssueJwtAsync(int lifetime, IEnumerable claims) { - var tokenType = OidcConstants.TokenTypes.AccessToken; var issuer = await IssuerNameService.GetCurrentAsync(); - return await IssueJwtAsync(lifetime, issuer, tokenType, claims); + return await IssueJwtAsync(lifetime, issuer, claims); } /// /// Issues a JWT. /// /// The lifetime. - /// The token type. + /// The issuer. /// The claims. /// /// claims - public virtual async Task IssueJwtAsync(int lifetime, string tokenType, IEnumerable claims) + public virtual Task IssueJwtAsync(int lifetime, string issuer, IEnumerable claims) { - var issuer = await IssuerNameService.GetCurrentAsync(); - return await IssueJwtAsync(lifetime, issuer, tokenType, claims); + var tokenType = OidcConstants.TokenTypes.AccessToken; + return IssueJwtAsync(lifetime, issuer, tokenType, claims); } /// diff --git a/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs b/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs index dbd9e4ad6..54b8d5e94 100644 --- a/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs +++ b/src/IdentityServer/Services/Default/DefaultBackChannelLogoutService.cs @@ -49,6 +49,11 @@ public class DefaultBackChannelLogoutService : IBackChannelLogoutService /// protected ILogger Logger { get; } + /// + /// Ths issuer name service. + /// + protected IIssuerNameService IssuerNameService { get; } + /// /// Constructor. /// @@ -56,12 +61,14 @@ public class DefaultBackChannelLogoutService : IBackChannelLogoutService /// /// /// + /// /// public DefaultBackChannelLogoutService( ISystemClock clock, IdentityServerTools tools, ILogoutNotificationService logoutNotificationService, IBackChannelLogoutHttpClient backChannelLogoutHttpClient, + IIssuerNameService issuerNameService, ILogger logger) { Clock = clock; @@ -69,6 +76,7 @@ public DefaultBackChannelLogoutService( LogoutNotificationService = logoutNotificationService; HttpClient = backChannelLogoutHttpClient; Logger = logger; + IssuerNameService = issuerNameService; } /// @@ -150,7 +158,8 @@ protected virtual async Task CreateTokenAsync(BackChannelLogoutRequest r return await Tools.IssueJwtAsync(DefaultLogoutTokenLifetime, request.Issuer, IdentityServerConstants.TokenTypes.LogoutToken, claims); } - return await Tools.IssueJwtAsync(DefaultLogoutTokenLifetime, IdentityServerConstants.TokenTypes.LogoutToken, claims); + var issuer = await IssuerNameService.GetCurrentAsync(); + return await Tools.IssueJwtAsync(DefaultLogoutTokenLifetime, issuer, IdentityServerConstants.TokenTypes.LogoutToken, claims); } /// diff --git a/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs b/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs index f8228fd36..a864ce618 100644 --- a/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs +++ b/test/IdentityServer.UnitTests/Services/Default/DefaultBackChannelLogoutServiceTests.cs @@ -24,15 +24,18 @@ public class DefaultBackChannelLogoutServiceTests { private class ServiceTestHarness : DefaultBackChannelLogoutService { - public ServiceTestHarness(ISystemClock clock, - IdentityServerTools tools, - ILogoutNotificationService logoutNotificationService, - IBackChannelLogoutHttpClient backChannelLogoutHttpClient, - ILogger logger) - : base(clock, tools, logoutNotificationService, backChannelLogoutHttpClient, logger) + public ServiceTestHarness( + ISystemClock clock, + IdentityServerTools tools, + ILogoutNotificationService logoutNotificationService, + IBackChannelLogoutHttpClient backChannelLogoutHttpClient, + IIssuerNameService issuerNameService, + ILogger logger) + : base(clock, tools, logoutNotificationService, backChannelLogoutHttpClient, issuerNameService, logger) { } + // CreateTokenAsync is protected, so we use this wrapper to exercise it in our tests public async Task ExerciseCreateTokenAsync(BackChannelLogoutRequest request) { @@ -51,14 +54,15 @@ public async Task CreateTokenAsync_Should_Set_Issuer_Correctly() var tokenCreation = new DefaultTokenCreationService(new MockClock(), mockKeyMaterialService, TestIdentityServerOptions.Create(), TestLogger.Create()); + var issuerNameService = new TestIssuerNameService(expected); var tools = new IdentityServerTools( null, // service provider is unused - new TestIssuerNameService(expected), + issuerNameService, tokenCreation, new MockClock() ); - var subject = new ServiceTestHarness(null, tools, null, null, null); + var subject = new ServiceTestHarness(null, tools, null, null, issuerNameService, null); var rawToken = await subject.ExerciseCreateTokenAsync(new BackChannelLogoutRequest { ClientId = "test_client",