From cc00a08a51ec7049e59bf2908bc17a0be4a5a943 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Fri, 25 Apr 2025 12:11:00 -0500 Subject: [PATCH 01/15] Removed nuget.config We've removed this config file from later branches, and backporting that change makes local dev simpler. --- nuget.config | 8 -------- 1 file changed, 8 deletions(-) delete mode 100644 nuget.config diff --git a/nuget.config b/nuget.config deleted file mode 100644 index 54e660f9c..000000000 --- a/nuget.config +++ /dev/null @@ -1,8 +0,0 @@ - - - - - - - - \ No newline at end of file From ba4e8b5f6080f31a65caaf8c2b81545c0bb28364 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Fri, 25 Apr 2025 12:19:21 -0500 Subject: [PATCH 02/15] Backport mock logger The mock logger was introduced in 7.1 and allows us to write tests that make assertions about what was logged. We need this in 7.0 to test the sensitive values filter. --- .../Common/IdentityServerPipeline.cs | 9 +++- .../Common/MockLogger.cs | 42 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) create mode 100644 test/IdentityServer.IntegrationTests/Common/MockLogger.cs diff --git a/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs b/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs index aca737e51..1ac3a2c2b 100644 --- a/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs +++ b/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs @@ -21,6 +21,7 @@ using Duende.IdentityServer.Services; using Duende.IdentityServer.Test; using FluentAssertions; using IdentityModel.Client; +using IdentityServer.IntegrationTests.Common; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Hosting; @@ -75,6 +76,9 @@ public class IdentityServerPipeline public MockMessageHandler BackChannelMessageHandler { get; set; } = new MockMessageHandler(); public MockMessageHandler JwtRequestMessageHandler { get; set; } = new MockMessageHandler(); + public MockLogger MockLogger { get; set; } = MockLogger.Create(); + + public event Action OnPreConfigureServices = services => { }; public event Action OnPostConfigureServices = services => { }; public event Action OnPreConfigure = app => { }; @@ -103,7 +107,10 @@ public class IdentityServerPipeline if (enableLogging) { - builder.ConfigureLogging((ctx, b) => b.AddConsole()); + // Configure logging so that the logger provider will always use our mock logger + // The MockLogger allows us to verify that particular messages were logged. + builder.ConfigureLogging((ctx, b) => + b.Services.AddSingleton(new MockLoggerProvider(MockLogger))); } Server = new TestServer(builder); diff --git a/test/IdentityServer.IntegrationTests/Common/MockLogger.cs b/test/IdentityServer.IntegrationTests/Common/MockLogger.cs new file mode 100644 index 000000000..d6fc618fb --- /dev/null +++ b/test/IdentityServer.IntegrationTests/Common/MockLogger.cs @@ -0,0 +1,42 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using System; +using System.Collections.Generic; +using Microsoft.Extensions.Logging; + +namespace IdentityServer.IntegrationTests.Common; + +public class MockLogger : ILogger +{ + public static MockLogger Create() => new MockLogger(new LoggerExternalScopeProvider()); + public MockLogger(LoggerExternalScopeProvider scopeProvider) => _scopeProvider = scopeProvider; + + public readonly List LogMessages = new(); + + + private readonly LoggerExternalScopeProvider _scopeProvider; + + + public IDisposable BeginScope(TState state) where TState : notnull => _scopeProvider.Push(state); + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log(LogLevel logLevel, EventId eventId, TState state, Exception exception, Func formatter) => LogMessages.Add(formatter(state, exception)); +} + +public class MockLogger : MockLogger, ILogger +{ + public MockLogger(LoggerExternalScopeProvider scopeProvider) : base(scopeProvider) + { + } +} + +public class MockLoggerProvider(MockLogger logger) : ILoggerProvider +{ + public void Dispose() + { + } + + public ILogger CreateLogger(string categoryName) => logger; +} From f684baa49a9de81b105d9d82cbcadbc8f2968055 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Fri, 25 Apr 2025 12:57:59 -0500 Subject: [PATCH 03/15] Harden default logging filters for PAR and Authorize endpoints PAR requests sometimes are handled by the same code path as authorize requests, so both endpoint's default sensitive values filter should be the same. --- .../Options/LoggingOptions.cs | 9 ++++- .../Authorize/PushedAuthorizationTests.cs | 33 +++++++++++++++---- 2 files changed, 35 insertions(+), 7 deletions(-) diff --git a/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs b/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs index 311042e57..a80ed6b3e 100644 --- a/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs +++ b/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs @@ -51,6 +51,9 @@ public class LoggingOptions public ICollection AuthorizeRequestSensitiveValuesFilter { get; set; } = new HashSet { + // Secrets and assertions may be passed to the authorize endpoint via PAR + OidcConstants.TokenRequest.ClientSecret, + OidcConstants.TokenRequest.ClientAssertion, OidcConstants.AuthorizeRequest.IdTokenHint }; @@ -58,11 +61,15 @@ public class LoggingOptions /// Gets or sets the collection of keys that will be used to redact sensitive values from a pushed authorization request log. /// /// Please be aware that initializing this property could expose sensitive information in your logs. + /// Note that pushed authorization parameters are eventually handled by the authorize request pipeline. + /// In most cases, changes to this collection should also be made to + /// public ICollection PushedAuthorizationSensitiveValuesFilter { get; set; } = new HashSet { OidcConstants.TokenRequest.ClientSecret, - OidcConstants.TokenRequest.ClientAssertion + OidcConstants.TokenRequest.ClientAssertion, + OidcConstants.AuthorizeRequest.IdTokenHint }; /// diff --git a/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs b/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs index 6635decc3..a5935e1a3 100644 --- a/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs +++ b/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs @@ -22,14 +22,15 @@ public class PushedAuthorizationTests { private readonly IdentityServerPipeline _mockPipeline = new(); private Client _client; - + private string clientSecret = Guid.NewGuid().ToString(); + public PushedAuthorizationTests() { ConfigureClients(); ConfigureUsers(); ConfigureScopesAndResources(); - _mockPipeline.Initialize(); + _mockPipeline.Initialize(enableLogging: true); _mockPipeline.Options.Endpoints.EnablePushedAuthorizationEndpoint = true; } @@ -68,12 +69,32 @@ public class PushedAuthorizationTests authorization.State.Should().Be(expectedState); } + [Fact] + public async Task sensitive_values_should_not_be_logged_on_bad_request_to_par_endpoint() + { + // Login + await _mockPipeline.LoginAsync("bob"); + _mockPipeline.BrowserClient.AllowAutoRedirect = false; + + // Push Authorization + var expectedCallback = _client.RedirectUris.First(); + var expectedState = "123_state"; + var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync( + clientSecret: clientSecret, + redirectUri: "bogus", // <-- Intentionally wrong, to provoke logging an error with raw request + state: expectedState + ); + + _mockPipeline.MockLogger.LogMessages.Should().ContainMatch("*\"client_secret\": \"***REDACTED***\"*"); + _mockPipeline.MockLogger.LogMessages.Should().NotContainMatch(clientSecret); + } + [Fact] public async Task using_pushed_authorization_when_it_is_globally_disabled_fails() { _mockPipeline.Options.Endpoints.EnablePushedAuthorizationEndpoint = false; - var (_, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(); + var (_, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret); statusCode.Should().Be(HttpStatusCode.NotFound); } @@ -123,7 +144,7 @@ public class PushedAuthorizationTests public async Task existing_pushed_authorization_request_uris_become_invalid_when_par_is_disabled() { // PAR is enabled when we push authorization... - var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(); + var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret); statusCode.Should().Be(HttpStatusCode.Created); parJson.Should().NotBeNull(); @@ -154,7 +175,7 @@ public class PushedAuthorizationTests // Login await _mockPipeline.LoginAsync("bob"); - var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(); + var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret);; statusCode.Should().Be(HttpStatusCode.Created); parJson.Should().NotBeNull(); @@ -298,7 +319,7 @@ public class PushedAuthorizationTests ClientId = "client1", ClientSecrets = new [] { - new Secret("secret".Sha256()) + new Secret(clientSecret.Sha256()) }, AllowedGrantTypes = GrantTypes.Implicit, RequireConsent = false, From e9a4f345e882058937c26b54c4bf11d9b6bf9ac1 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Fri, 25 Apr 2025 16:06:47 -0500 Subject: [PATCH 04/15] Copy github workflows from 7.1 to 7.0 --- .github/workflows/ci.yml | 48 -------- .github/workflows/codeql-analysis.yml | 3 +- .github/workflows/identity-server-ci.yml | 82 +++++++++++++ .github/workflows/identity-server-release.yml | 112 ++++++++++++++++++ 4 files changed, 196 insertions(+), 49 deletions(-) delete mode 100644 .github/workflows/ci.yml create mode 100644 .github/workflows/identity-server-ci.yml create mode 100644 .github/workflows/identity-server-release.yml diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml deleted file mode 100644 index f37f2fd62..000000000 --- a/.github/workflows/ci.yml +++ /dev/null @@ -1,48 +0,0 @@ -name: "CI" - -on: - push: - branches: - - main - - features/** - tags: - - '*.*.*' - pull_request: - -env: - DOTNET_NOLOGO: true - -permissions: - contents: read - -jobs: - build: - strategy: - fail-fast: false - matrix: - runs-on: [macOS-latest, ubuntu-latest, windows-latest] - name: ${{ matrix.runs-on }} - runs-on: ${{ matrix.runs-on }} - steps: - - uses: actions/checkout@v3 - with: - fetch-depth: 0 - - - name: Setup dotnet (main) - uses: actions/setup-dotnet@v4 - with: - dotnet-version: | - 8.0.100 - - - run: dotnet --info - - - if: contains(matrix.runs-on, 'macOS') || contains(matrix.runs-on, 'ubuntu') - run: ./build.sh - - if: matrix.runs-on == 'windows-latest' && github.ref != 'refs/heads/main' && !contains(github.ref, 'refs/tags/') - run: ./build.ps1 - - if: (matrix.runs-on == 'windows-latest') && (github.ref == 'refs/heads/main' || contains(github.ref, 'refs/tags/')) - env: - SignClientSecret: ${{ secrets.SIGNCLIENTSECRET }} - run: | - ./build.ps1 sign - dotnet nuget push .\artifacts\*.nupkg -s https://www.myget.org/F/duende_identityserver/api/v2/package -k ${{ secrets.MYGET }} diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 0ba0c5980..4a694c269 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -41,7 +41,8 @@ jobs: uses: actions/setup-dotnet@v4 with: dotnet-version: | - 8.0.100 + 8.0.x + 9.0.x - run: dotnet --info diff --git a/.github/workflows/identity-server-ci.yml b/.github/workflows/identity-server-ci.yml new file mode 100644 index 000000000..10752cb51 --- /dev/null +++ b/.github/workflows/identity-server-ci.yml @@ -0,0 +1,82 @@ +# This was generated by tool. Edits will be overwritten. + +name: identity-server/ci +on: + workflow_dispatch: + push: + paths: + - .github/workflows/identity-server-** + - identity-server/** + - Directory.Packages.props + pull_request: + paths: + - .github/workflows/identity-server-** + - identity-server/** + - Directory.Packages.props +env: + DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: true +jobs: + build: + name: Build + if: (github.event_name == 'pull_request' && github.event.pull_request.head.repo.full_name != github.repository) || (github.event_name == 'push') || (github.event_name == 'workflow_dispatch') + runs-on: ubuntu-latest + permissions: + actions: read + checks: write + contents: read + packages: write + defaults: + run: + shell: bash + working-directory: . + timeout-minutes: 15 + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Setup Dotnet + uses: actions/setup-dotnet@3e891b0cb619bf60e2c25674b222b8940e2c1c25 + with: + dotnet-version: |- + 6.0.x + 8.0.x + 9.0.x + - name: Build + run: dotnet build Duende.IdentityServer.sln -c Release + - name: Test + run: dotnet test Duende.IdentityServer.sln -c Release --no-build --logger "console;verbosity=normal" --logger "trx;LogFileName=Tests.trx" --collect:"XPlat Code Coverage" + - name: Test report + if: github.event == 'push' && (success() || failure()) + uses: dorny/test-reporter@31a54ee7ebcacc03a09ea97a7e5465a47b84aea5 + with: + name: Test Report + path: '**/Tests.trx' + reporter: dotnet-trx + fail-on-error: true + fail-on-empty: true + - name: Tool restore + run: dotnet tool restore + - name: Pack Duende.IdentityServer.sln + run: dotnet pack -c Release Duende.IdentityServer.sln -o artifacts + - name: Sign packages + if: github.event == 'push' + run: |- + for file in artifacts/*.nupkg; do + dotnet NuGetKeyVaultSignTool sign "$file" --file-digest sha256 --timestamp-rfc3161 http://timestamp.digicert.com --azure-key-vault-url https://duendecodesigninghsm.vault.azure.net/ --azure-key-vault-client-id 18e3de68-2556-4345-8076-a46fad79e474 --azure-key-vault-tenant-id ed3089f0-5401-4758-90eb-066124e2d907 --azure-key-vault-client-secret ${{ secrets.SignClientSecret }} --azure-key-vault-certificate NuGetPackageSigning + done + - name: Push packages to GitHub + if: github.ref == 'refs/heads/main' + run: dotnet nuget push artifacts/*.nupkg --source https://nuget.pkg.github.com/DuendeSoftware/index.json --api-key ${{ secrets.GITHUB_TOKEN }} --skip-duplicate + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + NUGET_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Upload Artifacts + if: github.ref == 'refs/heads/main' + uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 + with: + name: artifacts + path: artifacts/*.nupkg + overwrite: true + retention-days: 15 diff --git a/.github/workflows/identity-server-release.yml b/.github/workflows/identity-server-release.yml new file mode 100644 index 000000000..ed78016a0 --- /dev/null +++ b/.github/workflows/identity-server-release.yml @@ -0,0 +1,112 @@ +# This was generated by tool. Edits will be overwritten. + +name: identity-server/release +on: + workflow_dispatch: + inputs: + version: + description: 'Version in format X.Y.Z or X.Y.Z-preview.' + type: string + required: true + default: '0.0.0' + branch: + description: '(Optional) the name of the branch to release from' + type: string + required: false + default: 'main' + remove-tag-if-exists: + description: 'If set, will remove the existing tag. Use this if you have issues with the previous release action' + type: boolean + required: false + default: false +env: + DOTNET_NOLOGO: true + DOTNET_CLI_TELEMETRY_OPTOUT: true +jobs: + tag: + name: Tag and Pack + runs-on: ubuntu-latest + permissions: + contents: write + packages: write + defaults: + run: + shell: bash + working-directory: . + steps: + - name: Checkout + uses: actions/checkout@v4 + with: + fetch-depth: 0 + - name: Checkout target branch + if: github.event.inputs.branch != 'main' + run: git checkout ${{ github.event.inputs.branch }} + - name: Git Config + run: |- + git config --global user.email "github-bot@duendesoftware.com" + git config --global user.name "Duende Software GitHub Bot" + - name: Git Config + if: github.event.inputs['remove-tag-if-exists'] == 'true' + run: |- + if git rev-parse is-${{ github.event.inputs.version }} >/dev/null 2>&1; then + git tag -d is-${{ github.event.inputs.version }} + git push --delete origin is-${{ github.event.inputs.version }} + else + echo 'Tag is-${{ github.event.inputs.version }} does not exist.' + fi + - name: Git Tag + run: |- + git tag -a is-${{ github.event.inputs.version }} -m "Release v${{ github.event.inputs.version }}" + git push origin is-${{ github.event.inputs.version }} + - name: Setup Dotnet + uses: actions/setup-dotnet@3e891b0cb619bf60e2c25674b222b8940e2c1c25 + with: + dotnet-version: |- + 6.0.x + 8.0.x + 9.0.x + - name: Pack Duende.IdentityServer.sln + run: dotnet pack -c Release Duende.IdentityServer.sln -o artifacts + - name: Tool restore + run: dotnet tool restore + - name: Sign packages + run: |- + for file in artifacts/*.nupkg; do + dotnet NuGetKeyVaultSignTool sign "$file" --file-digest sha256 --timestamp-rfc3161 http://timestamp.digicert.com --azure-key-vault-url https://duendecodesigninghsm.vault.azure.net/ --azure-key-vault-client-id 18e3de68-2556-4345-8076-a46fad79e474 --azure-key-vault-tenant-id ed3089f0-5401-4758-90eb-066124e2d907 --azure-key-vault-client-secret ${{ secrets.SignClientSecret }} --azure-key-vault-certificate NuGetPackageSigning + done + - name: Push packages to GitHub + run: dotnet nuget push artifacts/*.nupkg --source https://nuget.pkg.github.com/DuendeSoftware/index.json --api-key ${{ secrets.GITHUB_TOKEN }} --skip-duplicate + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + NUGET_AUTH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Upload Artifacts + uses: actions/upload-artifact@b4b15b8c7c6ac21ea08fcf65892d2ee8f75cf882 + with: + name: artifacts + path: artifacts/*.nupkg + overwrite: true + retention-days: 15 + publish: + name: Publish to nuget.org + needs: + - tag + runs-on: ubuntu-latest + environment: + name: nuget.org + steps: + - uses: actions/download-artifact@fa0a91b85d4f404e444e00e005971372dc801d16 + with: + name: artifacts + path: artifacts + - name: Setup Dotnet + uses: actions/setup-dotnet@3e891b0cb619bf60e2c25674b222b8940e2c1c25 + with: + dotnet-version: |- + 6.0.x + 8.0.x + 9.0.x + - name: List files + run: tree + shell: bash + - name: Push packages to nuget.org + run: dotnet nuget push artifacts/*.nupkg --source https://api.nuget.org/v3/index.json --api-key ${{ secrets.NUGET_ORG_API_KEY }} --skip-duplicate From 60566e6e9f502636b73716a2161d294fc113634e Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 29 Apr 2025 10:04:19 -0500 Subject: [PATCH 05/15] Refactor mtls middleware for testability --- .../Hosting/MutualTlsEndpointMiddleware.cs | 115 ++++++++------ .../MutualTlsEndpointMiddlewareTests.cs | 148 ++++++++++++++++++ 2 files changed, 217 insertions(+), 46 deletions(-) create mode 100644 identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs diff --git a/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs b/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs index 4b8234c24..567758a31 100644 --- a/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs +++ b/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs @@ -36,62 +36,85 @@ public class MutualTlsEndpointMiddleware _logger = logger; } + internal enum MtlsEndpointType + { + None, + SeparateDomain, + Subdomain, + PathBased + } + + internal MtlsEndpointType DetermineMtlsEndpointType(HttpContext context, out PathString? subPath) + { + subPath = null; + + if (!_options.MutualTls.Enabled) + { + return MtlsEndpointType.None; + } + + if (_options.MutualTls.DomainName.IsPresent()) + { + if (_options.MutualTls.DomainName.Contains('.')) + { + // Separate domain + if (context.Request.Host.Host.Equals(_options.MutualTls.DomainName, StringComparison.OrdinalIgnoreCase)) + { + _logger.LogDebug("Requiring mTLS because the request's domain matches the configured mTLS domain name."); + return MtlsEndpointType.SeparateDomain; + } + } + else + { + // Subdomain + if (context.Request.Host.Host.StartsWith(_options.MutualTls.DomainName + ".", StringComparison.OrdinalIgnoreCase)) + { + _logger.LogDebug("Requiring mTLS because the request's subdomain matches the configured mTLS domain name."); + return MtlsEndpointType.Subdomain; + } + } + + _logger.LogDebug("Not requiring mTLS because this request's domain does not match the configured mTLS domain name."); + return MtlsEndpointType.None; + } + + // Check path-based MTLS + if (context.Request.Path.StartsWithSegments( + ProtocolRoutePaths.MtlsPathPrefix.EnsureLeadingSlash(), out var path)) + { + _logger.LogDebug("Requiring mTLS because the request's path begins with the configured mTLS path prefix."); + subPath = path; + return MtlsEndpointType.PathBased; + } + + return MtlsEndpointType.None; + } + /// public async Task Invoke(HttpContext context, IAuthenticationSchemeProvider schemes) { - if (_options.MutualTls.Enabled) + var mtlsConfigurationStyle = DetermineMtlsEndpointType(context, out var subPath); + + if (mtlsConfigurationStyle != MtlsEndpointType.None) { - // domain-based MTLS - if (_options.MutualTls.DomainName.IsPresent()) + var result = await TriggerCertificateAuthentication(context); + if (!result.Succeeded) { - // separate domain - if (_options.MutualTls.DomainName.Contains('.')) - { - if (context.Request.Host.Host.Equals(_options.MutualTls.DomainName, - StringComparison.OrdinalIgnoreCase)) - { - var result = await TriggerCertificateAuthentication(context); - if (!result.Succeeded) - { - return; - } - } - } - // sub-domain - else - { - if (context.Request.Host.Host.StartsWith(_options.MutualTls.DomainName + ".", StringComparison.OrdinalIgnoreCase)) - { - var result = await TriggerCertificateAuthentication(context); - if (!result.Succeeded) - { - return; - } - } - } + return; } - // path based MTLS - else if (context.Request.Path.StartsWithSegments(ProtocolRoutePaths.MtlsPathPrefix.EnsureLeadingSlash(), out var subPath)) + + // Additional processing for path-based MTLS + if (mtlsConfigurationStyle == MtlsEndpointType.PathBased && subPath.HasValue) { - var result = await TriggerCertificateAuthentication(context); + var path = ProtocolRoutePaths.ConnectPathPrefix + subPath.Value.ToString().EnsureLeadingSlash(); + path = path.EnsureLeadingSlash(); - if (result.Succeeded) - { - var path = ProtocolRoutePaths.ConnectPathPrefix + - subPath.ToString().EnsureLeadingSlash(); - path = path.EnsureLeadingSlash(); - - _logger.LogDebug("Rewriting MTLS request from: {oldPath} to: {newPath}", - context.Request.Path.ToString(), path); - context.Request.Path = path; - } - else - { - return; - } + _logger.LogDebug("Rewriting MTLS request from: {oldPath} to: {newPath}", + context.Request.Path.ToString(), path); + context.Request.Path = path; } } - + await _next(context); } diff --git a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs new file mode 100644 index 000000000..519646302 --- /dev/null +++ b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs @@ -0,0 +1,148 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using Duende.IdentityServer.Hosting; +using Duende.IdentityServer.Configuration; +using Microsoft.AspNetCore.Http; +using UnitTests.Common; +using System.Threading.Tasks; +using Xunit; + +public class MutualTlsEndpointMiddlewareTests +{ + private readonly IdentityServerOptions _options; + private readonly MutualTlsEndpointMiddleware _middleware; + private readonly HttpContext _httpContext; + + public MutualTlsEndpointMiddlewareTests() + { + var testLogger = TestLogger.Create(); + _options = TestIdentityServerOptions.Create(); + _middleware = new MutualTlsEndpointMiddleware( + next: (ctx) => Task.CompletedTask, + options: _options, + logger: testLogger + ); + _httpContext = new DefaultHttpContext(); + } + + [Fact] + public void mtls_endpoint_type_when_mtls_disabled_should_be_none() + { + _options.MutualTls.Enabled = false; + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); + Assert.Null(subPath); + } + + [Theory] + [InlineData("mtls.example.com")] + [InlineData("mtls.example.com:443")] + [InlineData("mtls.example.com:5001")] + public void mtls_endpoint_type_separate_domain_should_be_detected(string host) + { + // Arrange + _options.MutualTls.Enabled = true; + _options.MutualTls.DomainName = "mtls.example.com"; + _httpContext.Request.Host = new HostString(host); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain, result); + Assert.Null(subPath); + } + + [Theory] + [InlineData("example.com")] + [InlineData("example.com:443")] + [InlineData("example.com:5001")] + [InlineData("other.example.com")] + [InlineData("other.example.com:443")] + public void mtls_endpoint_type_separate_domain_should_not_match_different_domain(string host) + { + // Arrange + _options.MutualTls.Enabled = true; + _options.MutualTls.DomainName = "mtls.example.com"; + _httpContext.Request.Host = new HostString(host); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); + Assert.Null(subPath); + } + + [Theory] + [InlineData("mtls.example.com")] + [InlineData("mtls.example.com:443")] + [InlineData("mtls.example.com:5001")] + public void mtls_endpoint_type_subdomain_should_be_detected(string host) + { + // Arrange + _options.MutualTls.Enabled = true; + _options.MutualTls.DomainName = "mtls"; + _httpContext.Request.Host = new HostString(host); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.Subdomain, result); + Assert.Null(subPath); + } + + [Theory] + [InlineData("api.example.com")] + [InlineData("api.example.com:443")] + [InlineData("example.com")] + [InlineData("example.com:5001")] + public void mtls_endpoint_type_subdomain_should_not_match_different_subdomain(string host) + { + // Arrange + _options.MutualTls.Enabled = true; + _options.MutualTls.DomainName = "mtls"; + _httpContext.Request.Host = new HostString(host); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); + Assert.Null(subPath); + } + + [Fact] + public void mtls_endpoint_type_path_based_should_be_detected() + { + // Arrange + _options.MutualTls.Enabled = true; + _httpContext.Request.Path = new PathString("/connect/mtls/token"); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.PathBased, result); + Assert.Equal("/token", subPath!.Value); + } + + [Fact] + public void mtls_endpoint_type_should_be_none_when_enabled_but_no_matching_configuration() + { + // Arrange + _options.MutualTls.Enabled = true; + _options.MutualTls.DomainName = "mtls.example.com"; + _httpContext.Request.Host = new HostString("regular.example.com"); + _httpContext.Request.Path = new PathString("/connect/token"); + + // Act + var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); + + // Assert + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); + Assert.Null(subPath); + } +} \ No newline at end of file From 41c47c8f22da4358de2b20a5479a43488bb0a4c3 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 29 Apr 2025 10:49:05 -0500 Subject: [PATCH 06/15] Add failing test cases for mtls port number --- .../Options/MtlsOptions.cs | 13 +++- .../MutualTlsEndpointMiddlewareTests.cs | 74 +++++++++++-------- 2 files changed, 53 insertions(+), 34 deletions(-) diff --git a/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/MtlsOptions.cs b/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/MtlsOptions.cs index 72faeb89a..e6971e714 100644 --- a/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/MtlsOptions.cs +++ b/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/MtlsOptions.cs @@ -23,9 +23,18 @@ public class MutualTlsOptions /// /// Specifies a separate domain to run the MTLS endpoints on. - /// If the string does not contain any dots, a subdomain is assumed - e.g. main domain: identityserver.local, MTLS domain: mtls.identityserver.local - /// If the string contains dots, a completely separate domain is assumend, e.g. main domain: identity.app.com, MTLS domain: mtls.app.com. In this case you must set a static issuer name on the options. /// + /// If the string does not contain any dots, it is treated as a + /// subdomain. For example, if the non-mTLS endpoints are hosted at + /// example.com, configuring this option with the value "mtls" means that + /// mtls is required for requests to mtls.example.com. + /// + /// If the string contains dots, it is treated as a complete domain. + /// mTLS will be required for requests whose host name matches the + /// configured domain name completely, including the port number. + /// This allows for separate domains for the mTLS and non-mTLS endpoints. + /// For example, identity.example.com and mtls.example.com. + /// public string? DomainName { get; set; } /// diff --git a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs index 519646302..94c2d5b92 100644 --- a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs +++ b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs @@ -27,7 +27,7 @@ public class MutualTlsEndpointMiddlewareTests } [Fact] - public void mtls_endpoint_type_when_mtls_disabled_should_be_none() + internal void mtls_endpoint_type_when_mtls_disabled_should_be_none() { _options.MutualTls.Enabled = false; var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); @@ -36,55 +36,65 @@ public class MutualTlsEndpointMiddlewareTests } [Theory] - [InlineData("mtls.example.com")] - [InlineData("mtls.example.com:443")] - [InlineData("mtls.example.com:5001")] - public void mtls_endpoint_type_separate_domain_should_be_detected(string host) + [InlineData("mtls.example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mtls.example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mtls.example.com:5001", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("mtls.example.com", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mtls.example.com:443", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mtls.example.com:5001", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("mtls.example.com", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("mtls.example.com:443", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("mtls.example.com:5001", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + internal void mtls_endpoint_type_separate_domain_should_be_detected(string requestedHost, string configuredDomainName, MutualTlsEndpointMiddleware.MtlsEndpointType expectedType) { // Arrange _options.MutualTls.Enabled = true; - _options.MutualTls.DomainName = "mtls.example.com"; - _httpContext.Request.Host = new HostString(host); + _options.MutualTls.DomainName = configuredDomainName; + _httpContext.Request.Host = new HostString(requestedHost); // Act var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); // Assert - Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain, result); + Assert.Equal(expectedType, result); Assert.Null(subPath); } [Theory] - [InlineData("example.com")] - [InlineData("example.com:443")] - [InlineData("example.com:5001")] - [InlineData("other.example.com")] - [InlineData("other.example.com:443")] - public void mtls_endpoint_type_separate_domain_should_not_match_different_domain(string host) + [InlineData("example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("example.com:5001", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("other.example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("other.example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("example.com", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("example.com:443", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("other.example.com", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + [InlineData("other.example.com:5001", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] + internal void mtls_endpoint_type_separate_domain_should_not_match_different_domain(string requestedHost, string configuredDomainName, MutualTlsEndpointMiddleware.MtlsEndpointType expectedType) { // Arrange _options.MutualTls.Enabled = true; - _options.MutualTls.DomainName = "mtls.example.com"; - _httpContext.Request.Host = new HostString(host); + _options.MutualTls.DomainName = configuredDomainName; + _httpContext.Request.Host = new HostString(requestedHost); // Act var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); // Assert - Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); + Assert.Equal(expectedType, result); Assert.Null(subPath); } [Theory] - [InlineData("mtls.example.com")] - [InlineData("mtls.example.com:443")] - [InlineData("mtls.example.com:5001")] - public void mtls_endpoint_type_subdomain_should_be_detected(string host) + [InlineData("mtls.example.com", "mtls")] + [InlineData("mtls.example.com:443", "mtls")] + [InlineData("mtls.example.com:5001", "mtls")] + internal void mtls_endpoint_type_subdomain_should_be_detected(string requestedHost, string configuredDomainName) { // Arrange _options.MutualTls.Enabled = true; - _options.MutualTls.DomainName = "mtls"; - _httpContext.Request.Host = new HostString(host); + _options.MutualTls.DomainName = configuredDomainName; + _httpContext.Request.Host = new HostString(requestedHost); // Act var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); @@ -95,16 +105,16 @@ public class MutualTlsEndpointMiddlewareTests } [Theory] - [InlineData("api.example.com")] - [InlineData("api.example.com:443")] - [InlineData("example.com")] - [InlineData("example.com:5001")] - public void mtls_endpoint_type_subdomain_should_not_match_different_subdomain(string host) + [InlineData("api.example.com", "mtls")] + [InlineData("api.example.com:443", "mtls")] + [InlineData("example.com", "mtls")] + [InlineData("example.com:5001", "mtls")] + internal void mtls_endpoint_type_subdomain_should_not_match_different_subdomain(string requestedHost, string configuredDomainName) { // Arrange _options.MutualTls.Enabled = true; - _options.MutualTls.DomainName = "mtls"; - _httpContext.Request.Host = new HostString(host); + _options.MutualTls.DomainName = configuredDomainName; + _httpContext.Request.Host = new HostString(requestedHost); // Act var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); @@ -115,7 +125,7 @@ public class MutualTlsEndpointMiddlewareTests } [Fact] - public void mtls_endpoint_type_path_based_should_be_detected() + internal void mtls_endpoint_type_path_based_should_be_detected() { // Arrange _options.MutualTls.Enabled = true; @@ -130,7 +140,7 @@ public class MutualTlsEndpointMiddlewareTests } [Fact] - public void mtls_endpoint_type_should_be_none_when_enabled_but_no_matching_configuration() + internal void mtls_endpoint_type_should_be_none_when_enabled_but_no_matching_configuration() { // Arrange _options.MutualTls.Enabled = true; From c7aa99875c8cee3d266b508cb3c5aa99a612dcab Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 29 Apr 2025 11:22:15 -0500 Subject: [PATCH 07/15] Respect port number in mTLS middleware --- .../Hosting/MutualTlsEndpointMiddleware.cs | 31 ++++++++++++++++++- .../MutualTlsEndpointMiddlewareTests.cs | 2 ++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs b/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs index 567758a31..f6c626c09 100644 --- a/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs +++ b/identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs @@ -8,6 +8,7 @@ using Duende.IdentityServer.Configuration; using Duende.IdentityServer.Extensions; using Microsoft.AspNetCore.Authentication; using Microsoft.AspNetCore.Http; +using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Logging; using static Duende.IdentityServer.IdentityServerConstants; @@ -57,8 +58,9 @@ public class MutualTlsEndpointMiddleware { if (_options.MutualTls.DomainName.Contains('.')) { + var requestedHost = HostString.FromUriComponent(_options.MutualTls.DomainName); // Separate domain - if (context.Request.Host.Host.Equals(_options.MutualTls.DomainName, StringComparison.OrdinalIgnoreCase)) + if (RequestedHostMatches(context.Request.Host, _options.MutualTls.DomainName)) { _logger.LogDebug("Requiring mTLS because the request's domain matches the configured mTLS domain name."); return MtlsEndpointType.SeparateDomain; @@ -118,6 +120,33 @@ public class MutualTlsEndpointMiddleware await _next(context); } + + private bool RequestedHostMatches(HostString requestHost, string configuredDomain) + { + // Parse the configured domain which might contain a port + string configuredHostname = configuredDomain; + int configuredPort = 443; + + int colonIndex = configuredDomain.IndexOf(':'); + if (colonIndex >= 0) + { + configuredHostname = configuredDomain.Substring(0, colonIndex); + if (int.TryParse(configuredDomain.Substring(colonIndex + 1), out int port)) + { + configuredPort = port; + } + } + + // Compare hostnames (case-insensitive) + if (!string.Equals(requestHost.Host, configuredHostname, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + var requestPort = requestHost.Port ?? 443; + return requestPort == configuredPort; + } + private async Task TriggerCertificateAuthentication(HttpContext context) { var x509AuthResult = diff --git a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs index 94c2d5b92..e558d1f36 100644 --- a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs +++ b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs @@ -8,6 +8,8 @@ using UnitTests.Common; using System.Threading.Tasks; using Xunit; +namespace UnitTests.Hosting; + public class MutualTlsEndpointMiddlewareTests { private readonly IdentityServerOptions _options; From eb55e7b1ce546d670322e50bc48e515447f9f02e Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 29 Apr 2025 11:41:30 -0500 Subject: [PATCH 08/15] Add test cases for mTLS case insensitivity --- .../MutualTlsEndpointMiddlewareTests.cs | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs index e558d1f36..11bb00bfc 100644 --- a/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs +++ b/identity-server/test/IdentityServer.UnitTests/Hosting/MutualTlsEndpointMiddlewareTests.cs @@ -39,6 +39,8 @@ public class MutualTlsEndpointMiddlewareTests [Theory] [InlineData("mtls.example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mTLS.example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] + [InlineData("mtls.example.com", "mTLS.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] [InlineData("mtls.example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] [InlineData("mtls.example.com:5001", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] [InlineData("mtls.example.com", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.SeparateDomain)] @@ -63,16 +65,16 @@ public class MutualTlsEndpointMiddlewareTests } [Theory] - [InlineData("example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("example.com:5001", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("other.example.com", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("other.example.com:443", "mtls.example.com", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("example.com", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("example.com:443", "mtls.example.com:443", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("other.example.com", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - [InlineData("other.example.com:5001", "mtls.example.com:5001", MutualTlsEndpointMiddleware.MtlsEndpointType.None)] - internal void mtls_endpoint_type_separate_domain_should_not_match_different_domain(string requestedHost, string configuredDomainName, MutualTlsEndpointMiddleware.MtlsEndpointType expectedType) + [InlineData("example.com", "mtls.example.com")] + [InlineData("example.com:443", "mtls.example.com")] + [InlineData("example.com:5001", "mtls.example.com")] + [InlineData("other.example.com", "mtls.example.com")] + [InlineData("other.example.com:443", "mtls.example.com")] + [InlineData("example.com", "mtls.example.com:443")] + [InlineData("example.com:443", "mtls.example.com:443")] + [InlineData("other.example.com", "mtls.example.com:5001")] + [InlineData("other.example.com:5001", "mtls.example.com:5001")] + internal void mtls_endpoint_type_separate_domain_should_not_match_different_domain(string requestedHost, string configuredDomainName) { // Arrange _options.MutualTls.Enabled = true; @@ -83,12 +85,14 @@ public class MutualTlsEndpointMiddlewareTests var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); // Assert - Assert.Equal(expectedType, result); + Assert.Equal(MutualTlsEndpointMiddleware.MtlsEndpointType.None, result); Assert.Null(subPath); } [Theory] [InlineData("mtls.example.com", "mtls")] + [InlineData("mtls.example.com", "mTLS")] + [InlineData("mTLS.example.com", "mtls")] [InlineData("mtls.example.com:443", "mtls")] [InlineData("mtls.example.com:5001", "mtls")] internal void mtls_endpoint_type_subdomain_should_be_detected(string requestedHost, string configuredDomainName) @@ -126,12 +130,14 @@ public class MutualTlsEndpointMiddlewareTests Assert.Null(subPath); } - [Fact] - internal void mtls_endpoint_type_path_based_should_be_detected() + [Theory] + [InlineData("/connect/mtls/token")] + [InlineData("/connect/mTLS/token")] + internal void mtls_endpoint_type_path_based_should_be_detected(string requestedPath) { // Arrange _options.MutualTls.Enabled = true; - _httpContext.Request.Path = new PathString("/connect/mtls/token"); + _httpContext.Request.Path = new PathString(requestedPath); // Act var result = _middleware.DetermineMtlsEndpointType(_httpContext, out var subPath); From 57c4c074756fb6867567f22b9adeb2453618396e Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 6 May 2025 16:05:03 -0500 Subject: [PATCH 09/15] Update minver config - Autoincrement patch instead of minor. Major and minor version numbers are explicitly controlled by our release process. - Add missing is- tag prefix config --- Directory.Build.props | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Directory.Build.props b/Directory.Build.props index 5f186aedd..f968bd5ab 100644 --- a/Directory.Build.props +++ b/Directory.Build.props @@ -28,7 +28,8 @@ - minor + patch + is- True \ No newline at end of file From 9f1fa1aa4954f7d0f5913682bcf0ef0a4eccd575 Mon Sep 17 00:00:00 2001 From: khalidabuhakmeh Date: Tue, 6 May 2025 14:17:19 -0400 Subject: [PATCH 10/15] Add unit tests and refactor ProtectedResourceErrorResult Introduce tests for ProtectedResourceErrorResult to ensure WWW-Authenticate header formatting is correct. Refactor header construction values into a single string. --- .../Results/ProtectedResourceErrorResult.cs | 23 ++++--- .../ProtectedResourceErrorResultTests.cs | 63 +++++++++++++++++++ 2 files changed, 79 insertions(+), 7 deletions(-) create mode 100644 identity-server/test/IdentityServer.UnitTests/Endpoints/Results/ProtectedResourceErrorResultTests.cs diff --git a/identity-server/src/IdentityServer/Endpoints/Results/ProtectedResourceErrorResult.cs b/identity-server/src/IdentityServer/Endpoints/Results/ProtectedResourceErrorResult.cs index ba73ef94f..f0504e8d5 100644 --- a/identity-server/src/IdentityServer/Endpoints/Results/ProtectedResourceErrorResult.cs +++ b/identity-server/src/IdentityServer/Endpoints/Results/ProtectedResourceErrorResult.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.Primitives; using Microsoft.AspNetCore.Http; using Microsoft.Net.Http.Headers; using Duende.IdentityModel; +using System.Collections.Generic; namespace Duende.IdentityServer.Endpoints.Results; @@ -59,16 +60,24 @@ internal class ProtectedResourceErrorHttpWriter : IHttpResponseWriter { - context.Response.Headers.Append(HeaderNames.WWWAuthenticate, new StringValues(new[] { "Bearer realm=\"IdentityServer\"", errorString })); - } - else + """ + Bearer realm="IdentityServer" + """, + $""" + error="{error}" + """ + }; + + if (!errorDescription.IsMissing()) { - var errorDescriptionString = string.Format($"error_description=\"{errorDescription}\""); - context.Response.Headers.Append(HeaderNames.WWWAuthenticate, new StringValues(new[] { "Bearer realm=\"IdentityServer\"", errorString, errorDescriptionString })); + values.Add($""" + error_description="{errorDescription}" + """); } + + context.Response.Headers.Append(HeaderNames.WWWAuthenticate, string.Join(",", values)); return Task.CompletedTask; } diff --git a/identity-server/test/IdentityServer.UnitTests/Endpoints/Results/ProtectedResourceErrorResultTests.cs b/identity-server/test/IdentityServer.UnitTests/Endpoints/Results/ProtectedResourceErrorResultTests.cs new file mode 100644 index 000000000..a39398722 --- /dev/null +++ b/identity-server/test/IdentityServer.UnitTests/Endpoints/Results/ProtectedResourceErrorResultTests.cs @@ -0,0 +1,63 @@ +// Copyright (c) Duende Software. All rights reserved. +// See LICENSE in the project root for license information. + +using Duende.IdentityServer.Endpoints.Results; +using FluentAssertions; +using Microsoft.AspNetCore.Http; +using Microsoft.Net.Http.Headers; +using Xunit; + +namespace UnitTests.Endpoints.Results; + +public class ProtectedResourceErrorResultTests +{ + private readonly ProtectedResourceErrorHttpWriter writer = new(); + + [Fact] + public void WwwAuthenticate_header_with_error_and_description_should_be_a_single_line() + { + var context = new DefaultHttpContext(); + + writer.WriteHttpResponse( + new ProtectedResourceErrorResult("oops", "big oops"), + context + ); + + var wwwAuthHeader = context.Response.Headers[HeaderNames.WWWAuthenticate].ToString(); + wwwAuthHeader.Should().BeEquivalentTo( + """ + Bearer realm="IdentityServer",error="oops",error_description="big oops" + """); + } + + [Fact] + public void WwwAuthenticate_header_with_error_should_be_a_single_line() + { + var context = new DefaultHttpContext(); + + writer.WriteHttpResponse( + new ProtectedResourceErrorResult("oops"), + context + ); + + var wwwAuthHeader = context.Response.Headers[HeaderNames.WWWAuthenticate].ToString(); + wwwAuthHeader.Should().BeEquivalentTo( + """ + Bearer realm="IdentityServer",error="oops" + """); + } + + [Fact] + public void WwwAuthenticate_header_should_always_be_a_single_string_value() + { + var context = new DefaultHttpContext(); + + writer.WriteHttpResponse( + new ProtectedResourceErrorResult("oops", "big oops"), + context + ); + + var wwwAuthHeader = context.Response.Headers[HeaderNames.WWWAuthenticate]; + wwwAuthHeader.Count.Should().Be(1); + } +} \ No newline at end of file From 8d233567fb24a37e79c071bbb00c530a968822cd Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 6 May 2025 20:36:52 -0500 Subject: [PATCH 11/15] Add vscode settings to .gitignore --- .gitignore | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .gitignore diff --git a/.gitignore b/.gitignore new file mode 100644 index 000000000..b04fff14a --- /dev/null +++ b/.gitignore @@ -0,0 +1,2 @@ +# Visual Studio Code workspace options +.vscode/settings.json From a960bf47e175acf1012000c98f916778c3807694 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Tue, 6 May 2025 20:38:29 -0500 Subject: [PATCH 12/15] Add Shoudly in identity-server test projects This is only adding the dependency, not updating all the tests. This gives us an easy way to write tests that will merge cleanly into main. --- identity-server/Directory.Build.targets | 1 + .../Configuration.IntegrationTests.csproj | 1 + .../EntityFramework.IntegrationTests.csproj | 1 + .../EntityFramework.Storage.IntegrationTests.csproj | 5 +++-- .../EntityFramework.Storage.UnitTests.csproj | 1 + .../IdentityServer.IntegrationTests.csproj | 1 + .../IdentityServer.UnitTests/IdentityServer.UnitTests.csproj | 1 + 7 files changed, 9 insertions(+), 2 deletions(-) diff --git a/identity-server/Directory.Build.targets b/identity-server/Directory.Build.targets index 7b3436797..a1f512d4c 100644 --- a/identity-server/Directory.Build.targets +++ b/identity-server/Directory.Build.targets @@ -27,6 +27,7 @@ + diff --git a/identity-server/test/Configuration.IntegrationTests/Configuration.IntegrationTests.csproj b/identity-server/test/Configuration.IntegrationTests/Configuration.IntegrationTests.csproj index 2f225ae68..89291c4a0 100644 --- a/identity-server/test/Configuration.IntegrationTests/Configuration.IntegrationTests.csproj +++ b/identity-server/test/Configuration.IntegrationTests/Configuration.IntegrationTests.csproj @@ -23,6 +23,7 @@ all + diff --git a/identity-server/test/EntityFramework.IntegrationTests/EntityFramework.IntegrationTests.csproj b/identity-server/test/EntityFramework.IntegrationTests/EntityFramework.IntegrationTests.csproj index 90ab0a162..7d232874c 100644 --- a/identity-server/test/EntityFramework.IntegrationTests/EntityFramework.IntegrationTests.csproj +++ b/identity-server/test/EntityFramework.IntegrationTests/EntityFramework.IntegrationTests.csproj @@ -9,6 +9,7 @@ + diff --git a/identity-server/test/EntityFramework.Storage.IntegrationTests/EntityFramework.Storage.IntegrationTests.csproj b/identity-server/test/EntityFramework.Storage.IntegrationTests/EntityFramework.Storage.IntegrationTests.csproj index 2eaec819d..75e1c0799 100644 --- a/identity-server/test/EntityFramework.Storage.IntegrationTests/EntityFramework.Storage.IntegrationTests.csproj +++ b/identity-server/test/EntityFramework.Storage.IntegrationTests/EntityFramework.Storage.IntegrationTests.csproj @@ -13,6 +13,7 @@ + @@ -21,7 +22,7 @@