Merge fixes from 7.2.x forward to 7.3.x

.gitignore

@@ -228,4 +228,4 @@ artifacts
 
 *.Artifacts/
 
-reports
+reports

identity-server/src/Directory.Build.props

@@ -11,4 +11,4 @@
     <MinVerMinimumMajorMinor>7.0</MinVerMinimumMajorMinor>
   </PropertyGroup>
 
-</Project>
+</Project>

identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs

@@ -50,6 +50,9 @@ public class LoggingOptions
     public ICollection<string> AuthorizeRequestSensitiveValuesFilter { get; set; } =
         new HashSet<string>
         {
+            // Secrets and assertions may be passed to the authorize endpoint via PAR
+            OidcConstants.TokenRequest.ClientSecret,
+            OidcConstants.TokenRequest.ClientAssertion,
             OidcConstants.AuthorizeRequest.IdTokenHint,
             OidcConstants.AuthorizeRequest.Request
         };
@@ -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.
     /// </summary>
     /// <remarks>Please be aware that initializing this property could expose sensitive information in your logs.</remarks>
+    /// <remarks>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 <see cref="AuthorizeRequestSensitiveValuesFilter"/>
+    /// </remarks>
     public ICollection<string> PushedAuthorizationSensitiveValuesFilter { get; set; } =
         new HashSet<string>
         {
             OidcConstants.TokenRequest.ClientSecret,
             OidcConstants.TokenRequest.ClientAssertion,
+            OidcConstants.AuthorizeRequest.IdTokenHint,
             OidcConstants.AuthorizeRequest.Request
         };
 

identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/MtlsOptions.cs

@@ -23,9 +23,18 @@ public class MutualTlsOptions
 
     /// <summary>
     /// 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.
     /// </summary>
+    /// <remarks>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.
+    /// </remarks>
     public string? DomainName { get; set; }
 
     /// <summary>

identity-server/src/IdentityServer/Endpoints/Results/ProtectedResourceErrorResult.cs

@@ -58,17 +58,25 @@ public Task WriteHttpResponse(ProtectedResourceErrorResult result, HttpContext c
             errorDescription = "The access token expired";
         }
 
-        var errorString = string.Format($"error=\"{error}\"");
-        if (errorDescription.IsMissing())
+        var values = new List<string>
         {
-            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;
     }
 }

identity-server/src/IdentityServer/Hosting/MutualTlsEndpointMiddleware.cs

@@ -35,64 +35,116 @@ public MutualTlsEndpointMiddleware(RequestDelegate next, IdentityServerOptions o
         _sanitizedLogger = new SanitizedLogger<MutualTlsEndpointMiddleware>(logger);
     }
 
-    /// <inheritdoc />
-    public async Task Invoke(HttpContext context, IAuthenticationSchemeProvider schemes)
+    internal enum MtlsEndpointType
+    {
+        None,
+        SeparateDomain,
+        Subdomain,
+        PathBased
+    }
+
+    internal MtlsEndpointType DetermineMtlsEndpointType(HttpContext context, out PathString? subPath)
     {
-        if (_options.MutualTls.Enabled)
+        subPath = null;
+
+        if (!_options.MutualTls.Enabled)
         {
-            // domain-based MTLS
-            if (_options.MutualTls.DomainName.IsPresent())
+            return MtlsEndpointType.None;
+        }
+
+        if (_options.MutualTls.DomainName.IsPresent())
+        {
+            if (_options.MutualTls.DomainName.Contains('.'))
             {
-                // separate domain
-                if (_options.MutualTls.DomainName.Contains('.'))
+                var requestedHost = HostString.FromUriComponent(_options.MutualTls.DomainName);
+                // Separate domain
+                if (RequestedHostMatches(context.Request.Host, _options.MutualTls.DomainName))
                 {
-                    if (context.Request.Host.Host.Equals(_options.MutualTls.DomainName,
-                            StringComparison.OrdinalIgnoreCase))
-                    {
-                        var result = await TriggerCertificateAuthentication(context);
-                        if (!result.Succeeded)
-                        {
-                            return;
-                        }
-                    }
+                    _sanitizedLogger.LogDebug("Requiring mTLS because the request's domain matches the configured mTLS domain name.");
+                    return MtlsEndpointType.SeparateDomain;
                 }
-                // sub-domain
-                else
+            }
+            else
+            {
+                // Subdomain
+                if (context.Request.Host.Host.StartsWith(_options.MutualTls.DomainName + ".", StringComparison.OrdinalIgnoreCase))
                 {
-                    if (context.Request.Host.Host.StartsWith(_options.MutualTls.DomainName + ".", StringComparison.OrdinalIgnoreCase))
-                    {
-                        var result = await TriggerCertificateAuthentication(context);
-                        if (!result.Succeeded)
-                        {
-                            return;
-                        }
-                    }
+                    _sanitizedLogger.LogDebug("Requiring mTLS because the request's subdomain matches the configured mTLS domain name.");
+                    return MtlsEndpointType.Subdomain;
                 }
             }
-            // path based MTLS
-            else if (context.Request.Path.StartsWithSegments(ProtocolRoutePaths.MtlsPathPrefix.EnsureLeadingSlash(), out var subPath))
+
+            _sanitizedLogger.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))
+        {
+            _sanitizedLogger.LogDebug("Requiring mTLS because the request's path begins with the configured mTLS path prefix.");
+            subPath = path;
+            return MtlsEndpointType.PathBased;
+        }
+
+        return MtlsEndpointType.None;
+    }
+
+    /// <inheritdoc />
+    public async Task Invoke(HttpContext context, IAuthenticationSchemeProvider schemes)
+    {
+        var mtlsConfigurationStyle = DetermineMtlsEndpointType(context, out var subPath);
+
+        if (mtlsConfigurationStyle != MtlsEndpointType.None)
+        {
+            var result = await TriggerCertificateAuthentication(context);
+            if (!result.Succeeded)
             {
-                var result = await TriggerCertificateAuthentication(context);
+                return;
+            }
 
-                if (result.Succeeded)
-                {
-                    var path = ProtocolRoutePaths.ConnectPathPrefix + subPath.ToString().EnsureLeadingSlash();
-                    path = path.EnsureLeadingSlash();
+            // Additional processing for path-based MTLS
+            if (mtlsConfigurationStyle == MtlsEndpointType.PathBased && subPath.HasValue)
+            {
+                var path = ProtocolRoutePaths.ConnectPathPrefix + subPath.Value.ToString().EnsureLeadingSlash();
+                path = path.EnsureLeadingSlash();
 
-                    _sanitizedLogger.LogDebug("Rewriting MTLS request from: {oldPath} to: {newPath}",
-                        context.Request.Path.ToString(), path);
-                    context.Request.Path = path;
-                }
-                else
-                {
-                    return;
-                }
+                _sanitizedLogger.LogDebug("Rewriting MTLS request from: {oldPath} to: {newPath}",
+                    context.Request.Path.ToString(), path);
+                context.Request.Path = path;
             }
         }
 
         await _next(context);
     }
 
+
+    private bool RequestedHostMatches(HostString requestHost, string configuredDomain)
+    {
+        // Parse the configured domain which might contain a port
+        var configuredHostname = configuredDomain;
+        var configuredPort = 443;
+
+        var colonIndex = configuredDomain.IndexOf(':');
+        if (colonIndex >= 0)
+        {
+            configuredHostname = configuredDomain.Substring(0, colonIndex);
+            if (int.TryParse(configuredDomain.Substring(colonIndex + 1), out var 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<AuthenticateResult> TriggerCertificateAuthentication(HttpContext context)
     {
         var x509AuthResult = await context.AuthenticateAsync(_options.MutualTls.ClientCertificateAuthenticationScheme);

identity-server/src/IdentityServer/Services/Default/DefaultSessionCoordinationService.cs

@@ -140,14 +140,17 @@ public virtual async Task ProcessExpirationAsync(UserSession session)
         {
             var client = await ClientStore.FindClientByIdAsync(clientId); // i don't think we care if it's an enabled client at this point
 
-            var shouldCoordinate =
-                client.CoordinateLifetimeWithUserSession == true ||
-                (Options.Authentication.CoordinateClientLifetimesWithUserSession && client.CoordinateLifetimeWithUserSession != false);
-
-            if (shouldCoordinate)
+            if (client != null)
             {
-                // this implies they should also be contacted for backchannel logout below
-                clientsToCoordinate.Add(clientId);
+                var shouldCoordinate =
+                    client.CoordinateLifetimeWithUserSession == true ||
+                    (Options.Authentication.CoordinateClientLifetimesWithUserSession && client.CoordinateLifetimeWithUserSession != false);
+
+                if (shouldCoordinate)
+                {
+                    // this implies they should also be contacted for backchannel logout below
+                    clientsToCoordinate.Add(clientId);
+                }
             }
         }
 

identity-server/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs

@@ -15,14 +15,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;
     }
@@ -58,12 +59,32 @@ public async Task happy_path()
         authorization.State.ShouldBe(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.ShouldContain(msg => msg.Contains("\"client_secret\": \"***REDACTED***\""));
+        _mockPipeline.MockLogger.LogMessages.ShouldNotContain(msg => msg.Contains(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.ShouldBe(HttpStatusCode.NotFound);
     }
 
@@ -113,7 +134,7 @@ public async Task not_using_pushed_authorization_when_it_is_required_for_client_
     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.ShouldBe(HttpStatusCode.Created);
         parJson.ShouldNotBeNull();
 
@@ -141,7 +162,7 @@ public async Task reusing_pushed_authorization_request_uris_fails()
         // Login
         await _mockPipeline.LoginAsync("bob");
 
-        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync();
+        var (parJson, statusCode) = await _mockPipeline.PushAuthorizationRequestAsync(clientSecret: clientSecret); ;
         statusCode.ShouldBe(HttpStatusCode.Created);
         parJson.ShouldNotBeNull();
 
@@ -274,7 +295,7 @@ private void ConfigureClients() => _mockPipeline.Clients.AddRange(new Client[]
                 ClientId = "client1",
                 ClientSecrets = new []
                 {
-                     new Secret("secret".Sha256())
+                     new Secret(clientSecret.Sha256())
                 },
                 AllowedGrantTypes = GrantTypes.Implicit,
                 RequireConsent = false,