tsai
/
abp
mirror of https://github.com/abpframework/abp.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity
Browse Source

Refactor client_id cookie cleanup into an options extension 

- move OnRefreshingPrincipal logic to AbpOpenIddictSecurityStampValidatorOptionsExtensions

- add unit tests for removal and callback composition order
pull/25711/head
maliming 3 months ago
parent
f9493d6dec
commit
30119d7d90
No known key found for this signature in database GPG Key ID: A646B9CB645ECEA4
5 changed files with 205 additions and 178 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 3
      modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs
  2. 58
      modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs
  3. 48
      modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs
  4. 154
      modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs
  5. 120
      modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs

3
modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Properties/AssemblyInfo.cs
View File

@ -1,3 +0,0 @@
using System.Runtime.CompilerServices;
[assembly: InternalsVisibleTo("Volo.Abp.OpenIddict.AspNetCore.Tests")]

58
modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictAspNetCoreModule.cs
View File

@ -1,5 +1,4 @@
using System.Linq;
using Microsoft.AspNetCore.Identity;
using Microsoft.AspNetCore.Identity;
using Microsoft.AspNetCore.Mvc.Razor;
using Microsoft.Extensions.DependencyInjection;
using OpenIddict.Abstractions;
@ -42,63 +41,12 @@ public class AbpOpenIddictAspNetCoreModule : AbpModule
options.ViewLocationFormats.Add("/Volo/Abp/OpenIddict/Views/{1}/{0}.cshtml");
});
ConfigureSecurityStampValidator(context.Services);
}
/// <summary>
/// The <see cref="OpenIddictClaimsPrincipalContributor"/> adds the ambient authorization
/// request's <c>client_id</c> claim to every principal built by
/// <c>SignInManager.CreateUserPrincipalAsync</c>. That is correct for principals that OpenIddict
/// signs into tokens, but the same method is also used by the cookie security-stamp validator to
/// rebuild and re-issue the interactive authentication cookie. When the cookie happens to be
/// refreshed during a <c>/connect/authorize</c> request, the <c>client_id</c> of the OAuth client
/// being authorized leaks into the cookie and corrupts <c>ICurrentClient.Id</c> (and therefore
/// audit-log client attribution) for every later cookie-authenticated request in that browser.
///
/// The contributor cannot tell whether the principal it contributes to is destined for a token or
/// for the cookie, so the claim is stripped here, at the only point where the cookie is actually
/// re-written: the security-stamp <c>OnRefreshingPrincipal</c> callback. This never runs for token
/// issuance (which signs into the OpenIddict scheme, not the cookie), so the token path is left
/// untouched. The removal is chained after any previously registered callback (e.g. ABP Identity's
/// <c>SecurityStampValidatorCallback.UpdatePrincipal</c>), so it also self-heals cookies that were
/// already corrupted before this fix.
/// </summary>
internal static void ConfigureSecurityStampValidator(IServiceCollection services)
{
services.Configure<SecurityStampValidatorOptions>(options =>
Configure<SecurityStampValidatorOptions>(options =>
{
var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal;
options.OnRefreshingPrincipal = async context =>
{
if (previousOnRefreshingPrincipal != null)
{
await previousOnRefreshingPrincipal(context);
}
// Runs after any previously registered callback (e.g. ABP Identity's
// SecurityStampValidatorCallback.UpdatePrincipal), so an already-corrupted
// cookie that re-introduces client_id during the refresh still gets cleaned.
RemoveClientIdClaimsFromRefreshedPrincipal(context);
};
options.RemoveClientIdClaim();
});
}
internal static void RemoveClientIdClaimsFromRefreshedPrincipal(SecurityStampRefreshingPrincipalContext context)
{
if (context.NewPrincipal == null)
{
return;
}
foreach (var identity in context.NewPrincipal.Identities)
{
foreach (var clientIdClaim in identity.FindAll(AbpClaimTypes.ClientId).ToArray())
{
identity.RemoveClaim(clientIdClaim);
}
}
}
private void AddOpenIddictServer(IServiceCollection services)
{
var builderOptions = services.ExecutePreConfiguredActions<AbpOpenIddictAspNetCoreOptions>();

48
modules/openiddict/src/Volo.Abp.OpenIddict.AspNetCore/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions.cs
View File

@ -0,0 +1,48 @@
using System.Linq;
using Microsoft.AspNetCore.Identity;
using Volo.Abp.Security.Claims;
namespace Volo.Abp.OpenIddict;
public static class AbpOpenIddictSecurityStampValidatorOptionsExtensions
{
public static SecurityStampValidatorOptions RemoveClientIdClaim(this SecurityStampValidatorOptions options)
{
// OpenIddictClaimsPrincipalContributor stamps the ambient /connect/authorize request's client_id
// onto every principal built by CreateUserPrincipalAsync. That is meant for the access_token, but the
// cookie security-stamp validator rebuilds the interactive cookie through the same method, so a refresh
// that lands on /connect/authorize leaks client_id into the cookie and corrupts ICurrentClient.
// OnRefreshingPrincipal is the only place the cookie is re-written and never runs for token issuance.
var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal;
options.OnRefreshingPrincipal = async context =>
{
// Run the previous callback first: ABP Identity's UpdatePrincipal copies claims that are on the
// current cookie but not on the new principal forward, re-introducing client_id from an
// already-corrupted cookie. Removing it afterwards lets such a cookie self-heal on its next refresh.
if (previousOnRefreshingPrincipal != null)
{
await previousOnRefreshingPrincipal.Invoke(context);
}
RemoveClientIdClaimsFromPrincipal(context);
};
return options;
}
private static void RemoveClientIdClaimsFromPrincipal(SecurityStampRefreshingPrincipalContext context)
{
if (context.NewPrincipal == null)
{
return;
}
foreach (var identity in context.NewPrincipal.Identities)
{
foreach (var claim in identity.FindAll(AbpClaimTypes.ClientId).ToArray())
{
identity.RemoveClaim(claim);
}
}
}
}

154
modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests.cs
View File

@ -0,0 +1,154 @@
using System;
using System.Linq;
using System.Security.Claims;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Identity;
using Shouldly;
using Volo.Abp.Security.Claims;
using Xunit;
namespace Volo.Abp.OpenIddict;
public class AbpOpenIddictSecurityStampValidatorOptionsExtensions_Tests
{
[Fact]
public async Task Should_Remove_ClientId_From_Refreshed_Principal()
{
var context = new SecurityStampRefreshingPrincipalContext
{
NewPrincipal = CreateCookiePrincipal(
new Claim(AbpClaimTypes.UserId, "user-1"),
new Claim(AbpClaimTypes.ClientId, "MyClient"))
};
await RefreshPrincipalAsync(context);
context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1");
}
[Fact]
public async Task Should_Not_Touch_A_Principal_Without_ClientId()
{
var context = new SecurityStampRefreshingPrincipalContext
{
NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1"))
};
await RefreshPrincipalAsync(context);
context.NewPrincipal.Claims.Count().ShouldBe(1);
context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1");
}
[Fact]
public async Task Should_Remove_Every_ClientId_Claim_From_Every_Identity()
{
var principal = new ClaimsPrincipal();
principal.AddIdentity(new ClaimsIdentity(
new[] { new Claim(AbpClaimTypes.UserId, "user-1"), new Claim(AbpClaimTypes.ClientId, "Client-A") },
IdentityConstants.ApplicationScheme));
principal.AddIdentity(new ClaimsIdentity(
new[] { new Claim(AbpClaimTypes.ClientId, "Client-B"), new Claim(AbpClaimTypes.ClientId, "Client-C") },
IdentityConstants.ApplicationScheme));
var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = principal };
await RefreshPrincipalAsync(context);
context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
context.NewPrincipal.FindFirst(AbpClaimTypes.UserId).Value.ShouldBe("user-1");
}
[Fact]
public async Task Should_Not_Throw_When_New_Principal_Is_Null()
{
var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = null };
await Should.NotThrowAsync(() => RefreshPrincipalAsync(context));
}
[Fact]
public async Task Should_Remove_ClientId_After_Running_The_Previously_Registered_Callback()
{
// The real module order: ABP Identity's callback is registered first, the removal after it. Identity's
// SecurityStampValidatorCallback.UpdatePrincipal copies client_id from an already-corrupted cookie onto
// the refreshed principal; the removal still strips it, so the cookie self-heals on its next refresh.
var previousCallbackRan = false;
Task PreviousCallback(SecurityStampRefreshingPrincipalContext context)
{
previousCallbackRan = true;
CopyClientIdForward(context);
return Task.CompletedTask;
}
var refreshingContext = CreateCorruptedCookieRefreshContext();
await RefreshPrincipalAsync(refreshingContext, PreviousCallback);
previousCallbackRan.ShouldBeTrue();
refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
}
[Fact]
public async Task Should_Remove_ClientId_When_A_Callback_Is_Registered_After_It()
{
// The reverse order: the removal is registered first and an Identity-style callback (which runs its own
// copy-forward before invoking the previous callback) is registered after it. Because the two wrappers
// chain in opposite directions, the removal still runs last, so the order the modules load does not matter.
var options = new SecurityStampValidatorOptions();
options.RemoveClientIdClaim();
var previousOnRefreshingPrincipal = options.OnRefreshingPrincipal;
options.OnRefreshingPrincipal = async context =>
{
CopyClientIdForward(context);
if (previousOnRefreshingPrincipal != null)
{
await previousOnRefreshingPrincipal.Invoke(context);
}
};
var refreshingContext = CreateCorruptedCookieRefreshContext();
await options.OnRefreshingPrincipal(refreshingContext);
refreshingContext.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
}
private static async Task RefreshPrincipalAsync(
SecurityStampRefreshingPrincipalContext context,
Func<SecurityStampRefreshingPrincipalContext, Task> previousCallback = null)
{
var options = new SecurityStampValidatorOptions { OnRefreshingPrincipal = previousCallback };
options.RemoveClientIdClaim().ShouldBeSameAs(options);
await options.OnRefreshingPrincipal(context);
}
private static void CopyClientIdForward(SecurityStampRefreshingPrincipalContext context)
{
var clientId = context.CurrentPrincipal.FindFirst(AbpClaimTypes.ClientId);
if (clientId != null)
{
context.NewPrincipal.Identities.First().AddClaim(clientId);
}
}
private static SecurityStampRefreshingPrincipalContext CreateCorruptedCookieRefreshContext()
{
return new SecurityStampRefreshingPrincipalContext
{
CurrentPrincipal = CreateCookiePrincipal(
new Claim(AbpClaimTypes.UserId, "user-1"),
new Claim(AbpClaimTypes.ClientId, "MyClient")),
NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1"))
};
}
private static ClaimsPrincipal CreateCookiePrincipal(params Claim[] claims)
{
return new ClaimsPrincipal(new ClaimsIdentity(claims, IdentityConstants.ApplicationScheme));
}
}

120
modules/openiddict/test/Volo.Abp.OpenIddict.AspNetCore.Tests/Volo/Abp/OpenIddict/OpenIddictCookieClientIdLeak_Tests.cs
View File

@ -1,120 +0,0 @@
using System.Linq;
using System.Security.Claims;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Identity;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.Options;
using Shouldly;
using Volo.Abp.Security.Claims;
using Xunit;
namespace Volo.Abp.OpenIddict;
/// <summary>
/// Tests for the fix that stops the OAuth <c>client_id</c> of a <c>/connect/authorize</c> request
/// from leaking into the interactive authentication cookie.
///
/// Background: when the cookie's security stamp happens to be refreshed during a
/// <c>/connect/authorize</c> request, <c>OpenIddictClaimsPrincipalContributor</c> stamps the ambient
/// request's <c>client_id</c> onto the principal that is written back to the cookie. From then on
/// <c>ICurrentClient.Id</c> resolves to that client for every later cookie-authenticated request
/// (corrupting audit-log client attribution). The fix strips <c>client_id</c> from the principal at
/// the security-stamp <c>OnRefreshingPrincipal</c> callback, which only runs when the cookie is
/// re-issued and never for OpenIddict token issuance.
/// </summary>
public class OpenIddictCookieClientIdLeak_Tests
{
[Fact]
public void Should_Remove_ClientId_From_Refreshed_Cookie_Principal()
{
// A cookie principal that was wrongly stamped with client_id while being rebuilt
// by the security-stamp validator during /connect/authorize.
var context = new SecurityStampRefreshingPrincipalContext
{
CurrentPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1")),
NewPrincipal = CreateCookiePrincipal(
new Claim(AbpClaimTypes.UserId, "user-1"),
new Claim(AbpClaimTypes.ClientId, "MyClient"))
};
AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context);
context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
// unrelated claims are preserved
context.NewPrincipal.FindFirst(AbpClaimTypes.UserId)!.Value.ShouldBe("user-1");
}
[Fact]
public void Should_Not_Touch_A_Principal_That_Has_No_ClientId()
{
var context = new SecurityStampRefreshingPrincipalContext
{
NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1"))
};
AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context);
context.NewPrincipal.Claims.Count().ShouldBe(1);
context.NewPrincipal.FindFirst(AbpClaimTypes.UserId)!.Value.ShouldBe("user-1");
}
[Fact]
public void Should_Not_Throw_When_New_Principal_Is_Null()
{
var context = new SecurityStampRefreshingPrincipalContext { NewPrincipal = null };
Should.NotThrow(() => AbpOpenIddictAspNetCoreModule.RemoveClientIdClaimsFromRefreshedPrincipal(context));
}
[Fact]
public async Task Registered_Callback_Should_Strip_ClientId_After_Running_The_Previous_Callback()
{
// Reproduces the real composition order. A previously registered callback - e.g. ABP
// Identity's SecurityStampValidatorCallback.UpdatePrincipal - re-introduces client_id from
// an already-corrupted cookie onto the refreshed principal. The fix is chained AFTER it, so
// the claim is still removed and the cookie self-heals on its next refresh.
var services = new ServiceCollection();
services.AddOptions();
var previousCallbackRan = false;
services.Configure<SecurityStampValidatorOptions>(options =>
{
options.OnRefreshingPrincipal = context =>
{
previousCallbackRan = true;
var currentClientId = context.CurrentPrincipal!.FindFirst(AbpClaimTypes.ClientId);
if (currentClientId != null)
{
context.NewPrincipal!.Identities.First().AddClaim(currentClientId);
}
return Task.CompletedTask;
};
});
AbpOpenIddictAspNetCoreModule.ConfigureSecurityStampValidator(services);
var options = services
.BuildServiceProvider()
.GetRequiredService<IOptions<SecurityStampValidatorOptions>>()
.Value;
var context = new SecurityStampRefreshingPrincipalContext
{
CurrentPrincipal = CreateCookiePrincipal(
new Claim(AbpClaimTypes.UserId, "user-1"),
new Claim(AbpClaimTypes.ClientId, "MyClient")),
NewPrincipal = CreateCookiePrincipal(new Claim(AbpClaimTypes.UserId, "user-1"))
};
await options.OnRefreshingPrincipal!(context);
previousCallbackRan.ShouldBeTrue();
context.NewPrincipal.FindAll(AbpClaimTypes.ClientId).ShouldBeEmpty();
}
private static ClaimsPrincipal CreateCookiePrincipal(params Claim[] claims)
{
return new ClaimsPrincipal(new ClaimsIdentity(claims, IdentityConstants.ApplicationScheme));
}
}
Write Preview
Loading…
Cancel
Save