From 6a93eb7900071d8fe3cfb77bebb97ea6e5bdbabf Mon Sep 17 00:00:00 2001 From: Mohammad Mustakim Ali Date: Thu, 13 Aug 2020 10:59:32 +0100 Subject: [PATCH] Introduce LoggingOptions.AuthorizeRequestSensitiveValuesFilter We take some sensitive parameters with the "AuthorizeRequest" endpoint with some ICustomAuthorizeRequestValidator implementation to validate them. We don't want those sensitive parameters be logged when the validation fails. This commit introduces `AuthorizeRequestSensitiveValuesFilter` similar to `TokenRequestSensitiveValuesFilter`. This will allow us customize which parameter gets scrubbed. --- .../DependencyInjection/Options/LoggingOptions.cs | 9 +++++++++ .../src/Endpoints/AuthorizeCallbackEndpoint.cs | 4 +++- src/IdentityServer4/src/Endpoints/AuthorizeEndpoint.cs | 4 +++- .../src/Endpoints/AuthorizeEndpointBase.cs | 8 ++++++-- .../src/Logging/Models/AuthorizeRequestValidationLog.cs | 4 ++-- .../src/Validation/Default/AuthorizeRequestValidator.cs | 6 +++--- .../Authorize/AuthorizeCallbackEndpointTests.cs | 4 ++++ .../Endpoints/Authorize/AuthorizeEndpointBaseTests.cs | 7 ++++++- .../Endpoints/Authorize/AuthorizeEndpointTests.cs | 4 ++++ 9 files changed, 40 insertions(+), 10 deletions(-) diff --git a/src/IdentityServer4/src/Configuration/DependencyInjection/Options/LoggingOptions.cs b/src/IdentityServer4/src/Configuration/DependencyInjection/Options/LoggingOptions.cs index 7c4d7f184e..ad6c02be90 100644 --- a/src/IdentityServer4/src/Configuration/DependencyInjection/Options/LoggingOptions.cs +++ b/src/IdentityServer4/src/Configuration/DependencyInjection/Options/LoggingOptions.cs @@ -24,5 +24,14 @@ public class LoggingOptions OidcConstants.TokenRequest.RefreshToken, OidcConstants.TokenRequest.DeviceCode }; + + /// + /// + /// + public ICollection AuthorizeRequestSensitiveValuesFilter { get; set; } = + new HashSet + { + OidcConstants.AuthorizeRequest.IdTokenHint + }; } } \ No newline at end of file diff --git a/src/IdentityServer4/src/Endpoints/AuthorizeCallbackEndpoint.cs b/src/IdentityServer4/src/Endpoints/AuthorizeCallbackEndpoint.cs index f377b2dfce..baf4e603fd 100644 --- a/src/IdentityServer4/src/Endpoints/AuthorizeCallbackEndpoint.cs +++ b/src/IdentityServer4/src/Endpoints/AuthorizeCallbackEndpoint.cs @@ -4,6 +4,7 @@ using System.Collections.Specialized; using System.Net; using System.Threading.Tasks; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints.Results; using IdentityServer4.Extensions; using IdentityServer4.Hosting; @@ -25,13 +26,14 @@ internal class AuthorizeCallbackEndpoint : AuthorizeEndpointBase public AuthorizeCallbackEndpoint( IEventService events, ILogger logger, + IdentityServerOptions options, IAuthorizeRequestValidator validator, IAuthorizeInteractionResponseGenerator interactionGenerator, IAuthorizeResponseGenerator authorizeResponseGenerator, IUserSession userSession, IConsentMessageStore consentResponseStore, IAuthorizationParametersMessageStore authorizationParametersMessageStore = null) - : base(events, logger, validator, interactionGenerator, authorizeResponseGenerator, userSession) + : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession) { _consentResponseStore = consentResponseStore; _authorizationParametersMessageStore = authorizationParametersMessageStore; diff --git a/src/IdentityServer4/src/Endpoints/AuthorizeEndpoint.cs b/src/IdentityServer4/src/Endpoints/AuthorizeEndpoint.cs index df7aea2a26..e01aa3fe8c 100644 --- a/src/IdentityServer4/src/Endpoints/AuthorizeEndpoint.cs +++ b/src/IdentityServer4/src/Endpoints/AuthorizeEndpoint.cs @@ -4,6 +4,7 @@ using System.Collections.Specialized; using System.Net; using System.Threading.Tasks; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints.Results; using IdentityServer4.Extensions; using IdentityServer4.Hosting; @@ -20,11 +21,12 @@ internal class AuthorizeEndpoint : AuthorizeEndpointBase public AuthorizeEndpoint( IEventService events, ILogger logger, + IdentityServerOptions options, IAuthorizeRequestValidator validator, IAuthorizeInteractionResponseGenerator interactionGenerator, IAuthorizeResponseGenerator authorizeResponseGenerator, IUserSession userSession) - : base(events, logger, validator, interactionGenerator, authorizeResponseGenerator, userSession) + : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession) { } diff --git a/src/IdentityServer4/src/Endpoints/AuthorizeEndpointBase.cs b/src/IdentityServer4/src/Endpoints/AuthorizeEndpointBase.cs index 00633f03ca..dad027c38e 100644 --- a/src/IdentityServer4/src/Endpoints/AuthorizeEndpointBase.cs +++ b/src/IdentityServer4/src/Endpoints/AuthorizeEndpointBase.cs @@ -6,6 +6,7 @@ using System.Security.Claims; using System.Threading.Tasks; using IdentityModel; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints.Results; using IdentityServer4.Events; using IdentityServer4.Extensions; @@ -25,6 +26,7 @@ internal abstract class AuthorizeEndpointBase : IEndpointHandler private readonly IAuthorizeResponseGenerator _authorizeResponseGenerator; private readonly IEventService _events; + private readonly IdentityServerOptions _options; private readonly IAuthorizeInteractionResponseGenerator _interactionGenerator; @@ -33,12 +35,14 @@ internal abstract class AuthorizeEndpointBase : IEndpointHandler protected AuthorizeEndpointBase( IEventService events, ILogger logger, + IdentityServerOptions options, IAuthorizeRequestValidator validator, IAuthorizeInteractionResponseGenerator interactionGenerator, IAuthorizeResponseGenerator authorizeResponseGenerator, IUserSession userSession) { _events = events; + _options = options; Logger = logger; _validator = validator; _interactionGenerator = interactionGenerator; @@ -119,7 +123,7 @@ protected async Task CreateErrorResultAsync( if (request != null) { - var details = new AuthorizeRequestValidationLog(request); + var details = new AuthorizeRequestValidationLog(request, _options.Logging.AuthorizeRequestSensitiveValuesFilter); Logger.LogInformation("{@validationDetails}", details); } @@ -137,7 +141,7 @@ protected async Task CreateErrorResultAsync( private void LogRequest(ValidatedAuthorizeRequest request) { - var details = new AuthorizeRequestValidationLog(request); + var details = new AuthorizeRequestValidationLog(request, _options.Logging.AuthorizeRequestSensitiveValuesFilter); Logger.LogDebug(nameof(ValidatedAuthorizeRequest) + Environment.NewLine + "{@validationDetails}", details); } diff --git a/src/IdentityServer4/src/Logging/Models/AuthorizeRequestValidationLog.cs b/src/IdentityServer4/src/Logging/Models/AuthorizeRequestValidationLog.cs index e3e76146da..9988762c7a 100644 --- a/src/IdentityServer4/src/Logging/Models/AuthorizeRequestValidationLog.cs +++ b/src/IdentityServer4/src/Logging/Models/AuthorizeRequestValidationLog.cs @@ -35,9 +35,9 @@ internal class AuthorizeRequestValidationLog public Dictionary Raw { get; set; } - public AuthorizeRequestValidationLog(ValidatedAuthorizeRequest request) + public AuthorizeRequestValidationLog(ValidatedAuthorizeRequest request, IEnumerable sensitiveValuesFilter) { - Raw = request.Raw.ToScrubbedDictionary(OidcConstants.AuthorizeRequest.IdTokenHint); + Raw = request.Raw.ToScrubbedDictionary(sensitiveValuesFilter.ToArray()); if (request.Client != null) { diff --git a/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs b/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs index 38d855b76a..6fa4bb7212 100644 --- a/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs +++ b/src/IdentityServer4/src/Validation/Default/AuthorizeRequestValidator.cs @@ -835,14 +835,14 @@ private AuthorizeRequestValidationResult Valid(ValidatedAuthorizeRequest request private void LogError(string message, ValidatedAuthorizeRequest request) { - var requestDetails = new AuthorizeRequestValidationLog(request); + var requestDetails = new AuthorizeRequestValidationLog(request, _options.Logging.AuthorizeRequestSensitiveValuesFilter); _logger.LogError(message + "\n{@requestDetails}", requestDetails); } private void LogError(string message, string detail, ValidatedAuthorizeRequest request) { - var requestDetails = new AuthorizeRequestValidationLog(request); + var requestDetails = new AuthorizeRequestValidationLog(request, _options.Logging.AuthorizeRequestSensitiveValuesFilter); _logger.LogError(message + ": {detail}\n{@requestDetails}", detail, requestDetails); } } -} \ No newline at end of file +} diff --git a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeCallbackEndpointTests.cs b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeCallbackEndpointTests.cs index 38e451e3bf..b609e3c46e 100644 --- a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeCallbackEndpointTests.cs +++ b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeCallbackEndpointTests.cs @@ -7,6 +7,7 @@ using FluentAssertions; using IdentityServer.UnitTests.Common; using IdentityServer4; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints; using IdentityServer4.Endpoints.Results; using IdentityServer4.Extensions; @@ -28,6 +29,8 @@ public class AuthorizeCallbackEndpointTests private ILogger _fakeLogger = TestLogger.Create(); + private IdentityServerOptions _options = new IdentityServerOptions(); + private MockConsentMessageStore _mockUserConsentResponseMessageStore = new MockConsentMessageStore(); private MockUserSession _mockUserSession = new MockUserSession(); @@ -225,6 +228,7 @@ internal void Init() _subject = new AuthorizeCallbackEndpoint( _fakeEventService, _fakeLogger, + _options, _stubAuthorizeRequestValidator, _stubInteractionGenerator, _stubAuthorizeResponseGenerator, diff --git a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointBaseTests.cs b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointBaseTests.cs index 6aabde78e1..f6cdfbb2d2 100644 --- a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointBaseTests.cs +++ b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointBaseTests.cs @@ -7,6 +7,7 @@ using FluentAssertions; using IdentityServer.UnitTests.Common; using IdentityServer4; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints; using IdentityServer4.Endpoints.Results; using IdentityServer4.Hosting; @@ -30,6 +31,8 @@ public class AuthorizeEndpointBaseTests private ILogger _fakeLogger = TestLogger.Create(); + private IdentityServerOptions _options = new IdentityServerOptions(); + private MockUserSession _mockUserSession = new MockUserSession(); private NameValueCollection _params = new NameValueCollection(); @@ -180,6 +183,7 @@ internal void Init() _subject = new TestAuthorizeEndpoint( _fakeEventService, _fakeLogger, + _options, _stubAuthorizeRequestValidator, _stubInteractionGenerator, _stubAuthorizeResponseGenerator, @@ -191,11 +195,12 @@ internal class TestAuthorizeEndpoint : AuthorizeEndpointBase public TestAuthorizeEndpoint( IEventService events, ILogger logger, + IdentityServerOptions options, IAuthorizeRequestValidator validator, IAuthorizeInteractionResponseGenerator interactionGenerator, IAuthorizeResponseGenerator authorizeResponseGenerator, IUserSession userSession) - : base(events, logger, validator, interactionGenerator, authorizeResponseGenerator, userSession) + : base(events, logger, options, validator, interactionGenerator, authorizeResponseGenerator, userSession) { } diff --git a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointTests.cs b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointTests.cs index 5c19a15d60..6b9d4b9a53 100644 --- a/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointTests.cs +++ b/src/IdentityServer4/test/IdentityServer.UnitTests/Endpoints/Authorize/AuthorizeEndpointTests.cs @@ -8,6 +8,7 @@ using FluentAssertions; using IdentityServer.UnitTests.Common; using IdentityServer4; +using IdentityServer4.Configuration; using IdentityServer4.Endpoints; using IdentityServer4.Endpoints.Results; using IdentityServer4.Models; @@ -28,6 +29,8 @@ public class AuthorizeEndpointTests private ILogger _fakeLogger = TestLogger.Create(); + private IdentityServerOptions _options = new IdentityServerOptions(); + private MockUserSession _mockUserSession = new MockUserSession(); private NameValueCollection _params = new NameValueCollection(); @@ -100,6 +103,7 @@ internal void Init() _subject = new AuthorizeEndpoint( _fakeEventService, _fakeLogger, + _options, _stubAuthorizeRequestValidator, _stubInteractionGenerator, _stubAuthorizeResponseGenerator,