From 2a71d2822ec1f0e9847dd203eb7c1cdb53499742 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Thu, 14 Dec 2023 10:55:29 -0600 Subject: [PATCH 1/3] Hex encode consent keys --- .../Stores/Default/DefaultGrantStore.cs | 6 ++++- .../Stores/Default/DefaultUserConsentStore.cs | 23 +++++++++++++++---- 2 files changed, 24 insertions(+), 5 deletions(-) diff --git a/src/IdentityServer/Stores/Default/DefaultGrantStore.cs b/src/IdentityServer/Stores/Default/DefaultGrantStore.cs index e5fa28ea8..127282e0a 100644 --- a/src/IdentityServer/Stores/Default/DefaultGrantStore.cs +++ b/src/IdentityServer/Stores/Default/DefaultGrantStore.cs @@ -72,7 +72,11 @@ protected DefaultGrantStore(string grantType, } private const string KeySeparator = ":"; - const string HexEncodingFormatSuffix = "-1"; + + /// + /// The suffix added to keys to indicate that hex encoding should be used. + /// + protected const string HexEncodingFormatSuffix = "-1"; /// /// Creates a handle. diff --git a/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs b/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs index ca72e56a9..5dab9f5ec 100644 --- a/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs +++ b/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs @@ -31,9 +31,15 @@ public DefaultUserConsentStore( { } - private string GetConsentKey(string subjectId, string clientId) + private string GetConsentKey(string subjectId, string clientId, bool useHexEncoding = true) { - return clientId + "|" + subjectId; + if(useHexEncoding) + { + return $"{clientId}|{subjectId}{HexEncodingFormatSuffix}"; + } else + { + return $"{clientId}|{subjectId}"; + } } /// @@ -55,12 +61,21 @@ public Task StoreUserConsentAsync(Consent consent) /// The subject identifier. /// The client identifier. /// - public Task GetUserConsentAsync(string subjectId, string clientId) + public async Task GetUserConsentAsync(string subjectId, string clientId) { using var activity = Tracing.StoreActivitySource.StartActivity("DefaultUserConsentStore.GetUserConsent"); var key = GetConsentKey(subjectId, clientId); - return GetItemAsync(key); + var consent = await GetItemAsync(key); + if(consent == null) + { + var legacyKey = GetConsentKey(subjectId, clientId, useHexEncoding: false); + consent = await GetItemAsync(legacyKey); + await StoreUserConsentAsync(consent); // Write back the consent record to update its key + await RemoveItemAsync(legacyKey); + } + + return consent; } /// From 663a7986ccfdb3a784d9210cc4572932a41c002c Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Thu, 14 Dec 2023 11:11:08 -0600 Subject: [PATCH 2/3] Fix failing test --- .../Stores/Default/DefaultUserConsentStore.cs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs b/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs index 5dab9f5ec..bb98ef06b 100644 --- a/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs +++ b/src/IdentityServer/Stores/Default/DefaultUserConsentStore.cs @@ -71,8 +71,11 @@ public async Task GetUserConsentAsync(string subjectId, string clientId { var legacyKey = GetConsentKey(subjectId, clientId, useHexEncoding: false); consent = await GetItemAsync(legacyKey); - await StoreUserConsentAsync(consent); // Write back the consent record to update its key - await RemoveItemAsync(legacyKey); + if(consent != null) + { + await StoreUserConsentAsync(consent); // Write back the consent record to update its key + await RemoveItemAsync(legacyKey); + } } return consent; From dc09f374764d5e3ef6589bdba063c040e851a8b9 Mon Sep 17 00:00:00 2001 From: Joe DeCock Date: Thu, 14 Dec 2023 16:56:02 -0600 Subject: [PATCH 3/3] Added integration test for consent key migration Shows that consents stored with the legacy key still work, and that they get migrated to the hex encoded keys automatically. --- .../Endpoints/Authorize/ConsentTests.cs | 81 +++++++++++++++++++ 1 file changed, 81 insertions(+) diff --git a/test/IdentityServer.IntegrationTests/Endpoints/Authorize/ConsentTests.cs b/test/IdentityServer.IntegrationTests/Endpoints/Authorize/ConsentTests.cs index 8933fd109..dfbb1532f 100644 --- a/test/IdentityServer.IntegrationTests/Endpoints/Authorize/ConsentTests.cs +++ b/test/IdentityServer.IntegrationTests/Endpoints/Authorize/ConsentTests.cs @@ -6,16 +6,21 @@ using System.Collections.Generic; using System.Net; using System.Security.Claims; +using System.Security.Cryptography; +using System.Text; +using System.Text.Json; using System.Threading.Tasks; using Duende.IdentityServer; using Duende.IdentityServer.Models; using Duende.IdentityServer.Stores; using Duende.IdentityServer.Stores.Default; +using Duende.IdentityServer.Stores.Serialization; using Duende.IdentityServer.Test; using FluentAssertions; using IntegrationTests.Common; using Microsoft.Extensions.DependencyInjection; using Xunit; +using static Duende.IdentityServer.Models.IdentityResources; namespace IntegrationTests.Endpoints.Authorize; @@ -359,4 +364,80 @@ public async Task consent_response_of_unmet_authentication_requirements_should_r authorization.Error.Should().Be("unmet_authentication_requirements"); authorization.ErrorDescription.Should().Be("some description"); } + + [Fact] + [Trait("Category", Category)] + public async Task legacy_consents_should_apply_and_be_migrated_to_hex_encoding() + { + var clientId = "client2"; + var subjectId = "bob"; + + // Create and serialize a consent record + _mockPipeline.Options.PersistentGrants.DataProtectData = false; + var serializer = _mockPipeline.Resolve(); + var serialized = serializer.Serialize(new Consent + { + ClientId = clientId, + SubjectId = subjectId, + CreationTime = DateTime.UtcNow, + Scopes = new List { "openid" } + }); + + // Store the consent using the legacy key format + var persistedGrantStore = _mockPipeline.Resolve(); + var legacyKey = $"{clientId}|{subjectId}:{IdentityServerConstants.PersistedGrantTypes.UserConsent}".Sha256(); + var legacyConsent = new PersistedGrant + { + Key = legacyKey, + Type = IdentityServerConstants.PersistedGrantTypes.UserConsent, + ClientId = clientId, + SubjectId = subjectId, + SessionId = Guid.NewGuid().ToString(), + Description = null, + CreationTime = DateTime.UtcNow, + Expiration = null, + ConsumedTime = null, + Data = serialized + }; + await persistedGrantStore.StoreAsync(legacyConsent); + + // Create a session cookie + await _mockPipeline.LoginAsync("bob"); + + // Start a challenge + var url = _mockPipeline.CreateAuthorizeUrl( + clientId: "client2", + responseType: "id_token", + scope: "openid", + redirectUri: "https://client2/callback", + state: "123_state", + nonce: "123_nonce" + ); + _mockPipeline.BrowserClient.AllowAutoRedirect = false; + var response = await _mockPipeline.BrowserClient.GetAsync(url); + + // The existing legacy consent should apply - user isn't show consent screen + response.StatusCode.Should().Be(HttpStatusCode.Redirect); + response.Headers.Location.ToString().Should().StartWith("https://client2/callback"); + _mockPipeline.ConsentWasCalled.Should().BeFalse(); + + // The legacy consent should be migrated to use a new key... + + // Old key shouldn't find anything + var grant = await persistedGrantStore.GetAsync(legacyKey); + grant.Should().BeNull(); + + // New key should + var hexEncodedKeyNoHash = $"{clientId}|{subjectId}-1:{IdentityServerConstants.PersistedGrantTypes.UserConsent}"; + using (var sha = SHA256.Create()) + { + var bytes = Encoding.UTF8.GetBytes(hexEncodedKeyNoHash); + var hash = sha.ComputeHash(bytes); + var hexEncodedKey = BitConverter.ToString(hash).Replace("-", ""); + grant = await persistedGrantStore.GetAsync(hexEncodedKey); + grant.Should().NotBeNull(); + grant.ClientId.Should().Be(clientId); + grant.SubjectId.Should().Be(subjectId); + } + } }