diff --git a/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs b/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs index dffa77b3d..34dd2d683 100644 --- a/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs +++ b/identity-server/src/IdentityServer/Configuration/DependencyInjection/Options/LoggingOptions.cs @@ -52,6 +52,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 }; @@ -59,11 +62,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/identity-server/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs b/identity-server/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs index fe91e6843..f0462bdfa 100644 --- a/identity-server/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs +++ b/identity-server/test/IdentityServer.IntegrationTests/Common/IdentityServerPipeline.cs @@ -392,7 +392,7 @@ public string CreateAuthorizeUrl( } public async Task<(JsonDocument, HttpStatusCode)> PushAuthorizationRequestAsync( Dictionary parameters) - { + { var httpResponse = await BackChannelClient.PostAsync(ParEndpoint, new FormUrlEncodedContent(parameters)); var statusCode = httpResponse.StatusCode; @@ -423,9 +423,9 @@ public string CreateAuthorizeUrl( { "state", state } }; - if(extra != null) + if (extra != null) { - foreach(var (key, value) in extra) + foreach (var (key, value) in extra) { parameters[key] = value; } @@ -501,7 +501,7 @@ protected override async Task SendAsync(HttpRequestMessage } } -public class MockExternalAuthenticationHandler : +public class MockExternalAuthenticationHandler : IAuthenticationHandler, IAuthenticationSignInHandler, IAuthenticationRequestHandler @@ -509,7 +509,7 @@ public class MockExternalAuthenticationHandler : private readonly IHttpContextAccessor _httpContextAccessor; private HttpContext HttpContext => _httpContextAccessor.HttpContext; - public Func> OnFederatedSignout = + public Func> OnFederatedSignout = async context => { await context.SignOutAsync(); diff --git a/identity-server/test/IdentityServer.IntegrationTests/Common/MockLogger.cs b/identity-server/test/IdentityServer.IntegrationTests/Common/MockLogger.cs index 83468898a..640aca03b 100644 --- a/identity-server/test/IdentityServer.IntegrationTests/Common/MockLogger.cs +++ b/identity-server/test/IdentityServer.IntegrationTests/Common/MockLogger.cs @@ -16,11 +16,11 @@ public MockLogger(LoggerExternalScopeProvider 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; diff --git a/identity-server/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs b/identity-server/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs index c4bc4bc04..49ca46577 100644 --- a/identity-server/test/IdentityServer.IntegrationTests/Endpoints/Authorize/PushedAuthorizationTests.cs +++ b/identity-server/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; } @@ -65,12 +66,32 @@ public async Task happy_path() 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); } @@ -120,7 +141,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.Should().Be(HttpStatusCode.Created); parJson.Should().NotBeNull(); @@ -148,7 +169,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.Should().Be(HttpStatusCode.Created); parJson.Should().NotBeNull(); @@ -286,7 +307,7 @@ private void ConfigureClients() ClientId = "client1", ClientSecrets = new [] { - new Secret("secret".Sha256()) + new Secret(clientSecret.Sha256()) }, AllowedGrantTypes = GrantTypes.Implicit, RequireConsent = false,