Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/security/log-redaction-policy.md
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ them, so renaming one silently breaks every rule built against it.
| `Auth.AccountLockedOut` | The specific failed attempt that crosses the configured lockout threshold (#128) and transitions the account from unlocked to locked. Fires **once** per lockout episode — a later attempt against an already-locked account re-fires `LoginFailed` but not this. | `UserId`, `ClientIp`. |
| `Auth.RefreshTokenReplayDetected` | A presented refresh token is found already revoked/rotated *and* the #176 grace-replacement check rules out a benign concurrent retry — a genuine reuse of a dead token. | `UserId`, `ClientIp`. |
| `Auth.RefreshRevocationFailed` | The app's own attempt to revoke a refresh token (or a whole token family, following a replay) throws instead of completing — the safety action meant to lock a suspected attacker out failed to run. Covers the whole attempt, including the prerequisite lookup of the token's owner: a failure anywhere in it means the revoke did not happen. Emitted **once** per failed attempt. | `UserId` (when known — a failure in the owner lookup itself has no user id to report, and says so rather than staying silent). |
| `Auth.RateLimitRejected` | The per-IP fixed-window limiter rejects a request against the login or refresh policy with 429. Deliberately excludes the client-errors policy (#217) — that budget guards log-pipeline volume, not a credential. | `ClientIp`, `Path`. |
| `Auth.RateLimitRejected` | The fixed-window limiter rejects a request with 429 under the login, refresh, `oauth-authorize`, `oauth-token`, `oauth-register` or `oauth-api` policy (#1164). Deliberately excludes the client-errors policy (#217) — that budget guards log-pipeline volume, not a credential. | `Policy`, `ClientIp`, `Path` (no query string). `oauth-api` is keyed per bearer, but the event carries the client IP, never the token. |

### Operational (non-credential) security events

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,35 +69,27 @@ public static CluckworkRateLimitingRegistration AddCluckworkRateLimiting(
.ToString(CultureInfo.InvariantCulture);
}

// #273 codex review (P1c) — a stable, alertable event for the AUTH
// POLICY only (RateLimitingOptions.LoginPolicyName /
// RefreshPolicyName): a 429 there is a brute-force/credential-
// stuffing signal a deployment backend should be able to page on.
// The client-errors policy (#217) guards log-pipeline VOLUME, not
// a credential, so its rejections deliberately stay plain 429s
// with no security event — see SecurityEvents.RateLimitRejected.
// #273 codex review (P1c), #1164 — a stable, alertable event for every
// policy guarding a credential or an OAuth surface: a 429 there is a
// brute-force or flooding signal a deployment backend should page on. The
// client-errors policy (#217) guards log-pipeline VOLUME, not a credential,
// so its rejections stay plain 429s — see SecurityEvents.RateLimitRejected.
//
// Keyed on the endpoint's ATTACHED POLICY (via the
// EnableRateLimitingAttribute metadata RequireRateLimiting sets),
// not a hardcoded list of literal paths: AuthEndpoints attaches
// LoginPolicyName to /auth/login AND /auth/step-up AND
// /auth/change-password (all three verify a credential and must
// share the brute-force budget), and the earlier path-list version
// here only recognized /auth/login and /auth/refresh — a rejection
// on step-up or change-password was silently invisible. Matching
// the policy name means any FUTURE route that opts into the login
// or refresh policy is covered automatically, with no second edit
// required here.
// Keyed on the endpoint's ATTACHED POLICY (the EnableRateLimitingAttribute
// metadata RequireRateLimiting sets), never on paths: a path list missed
// step-up and change-password, and a login/refresh allow-list missed the
// four OAuth policies (#1164). Path carries no query string; ClientIp is
// logged, never the oauth-api partition key, which hashes a bearer.
var policyName = context.HttpContext.GetEndpoint()?.Metadata
.GetMetadata<EnableRateLimitingAttribute>()?.PolicyName;
if (policyName == RateLimitingOptions.LoginPolicyName
|| policyName == RateLimitingOptions.RefreshPolicyName)
if (policyName is not null && policyName != RateLimitingOptions.ClientErrorsPolicyName)
{
var rejectionLogger = context.HttpContext.RequestServices
.GetRequiredService<ILoggerFactory>()
.CreateLogger("Cluckwork.Api.Security.RateLimiting");
rejectionLogger.LogWarning("{SecurityEvent} client={ClientIp} path={Path}",
rejectionLogger.LogWarning("{SecurityEvent} policy={Policy} client={ClientIp} path={Path}",
SecurityEvents.RateLimitRejected,
policyName,
RateLimitKey.ForClient(context.HttpContext.Connection.RemoteIpAddress),
context.HttpContext.Request.Path.Value);
}
Expand Down
10 changes: 5 additions & 5 deletions src/Cluckwork.Application/Common/SecurityEvents.cs
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ public static class SecurityEvents
// separately.
public const string RefreshRevocationFailed = "Auth.RefreshRevocationFailed";

// Fires when the per-IP fixed-window limiter rejects a request against the
// login or refresh policy (RateLimitingOptions.LoginPolicyName /
// RefreshPolicyName) with 429. Deliberately excludes the client-errors
// policy (#217) — that budget guards log-pipeline volume, not a
// credential, so its rejections carry no security-event line.
// Fires when the fixed-window limiter rejects a request with 429 under any
// policy except client-errors: login, refresh and the four OAuth policies
// (#1164). Deliberately excludes the client-errors policy (#217) — that
// budget guards log-pipeline volume, not a credential, so its rejections
// carry no security-event line.
public const string RateLimitRejected = "Auth.RateLimitRejected";

// Fires when a shared-state (Redis) operation throws and the caller
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ namespace Cluckwork.Infrastructure.RateLimiting;
//
// OnRejected is null on purpose: the single global RateLimiterOptions.OnRejected handler in
// CluckworkRateLimitingServiceCollectionExtensions owns the 429 body, the Retry-After header,
// and the auth-only SecurityEvents.RateLimitRejected event. A per-policy OnRejected here
// and the SecurityEvents.RateLimitRejected event. A per-policy OnRejected here
// would double-handle or split that logic.
public sealed class DistributedFixedWindowPolicy : IRateLimiterPolicy<string>
{
Expand All @@ -37,7 +37,7 @@ public DistributedFixedWindowPolicy(
}

// Null on purpose: the single global RateLimiterOptions.OnRejected handler owns the 429
// body, the Retry-After header, and the auth-only RateLimitRejected security event. That the
// body, the Retry-After header, and the RateLimitRejected security event. That the
// framework falls back to the global handler when a policy's OnRejected is null is guarded by
// DistributedRateLimiterWiringTests.No_attacker_supplied_dimension_in_key_or_log (it asserts
// the event fires); a runtime change to that behaviour fails there by name, not silently.
Expand Down
87 changes: 85 additions & 2 deletions tests/Cluckwork.Api.IntegrationTests/AuthRateLimitLoggingTests.cs
Original file line number Diff line number Diff line change
@@ -1,10 +1,15 @@
using Cluckwork.Api.Modules.Access.Auth;
using Cluckwork.Api.Modules.Access.OAuth;
using Cluckwork.Api.RateLimiting;
using System.Collections.Concurrent;
using System.Net;
using System.Net.Http.Headers;
using Cluckwork.Api.IntegrationTests.Infrastructure;
using Cluckwork.Application.Common;
using Microsoft.AspNetCore.Builder;
using Microsoft.AspNetCore.Hosting;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Routing;
using Microsoft.AspNetCore.TestHost;
using Microsoft.Extensions.DependencyInjection;
using Serilog.Core;
Expand Down Expand Up @@ -36,13 +41,34 @@ protected override void ConfigureWebHost(IWebHostBuilder builder)
builder.UseSetting("RateLimiting:ClientErrors:PermitLimit", LoginLimit.ToString());
builder.UseSetting("RateLimiting:ClientErrors:WindowSeconds", "86400");
builder.UseSetting("RateLimiting:TrustedProxies:0", $"{TrustedProxy}/32");
// #1164 — one permit per OAuth policy, so the second request is the rejection.
foreach (var policy in new[] { "OAuthAuthorize", "OAuthToken", "OAuthRegister", "OAuthApi" })
{
builder.UseSetting($"RateLimiting:{policy}:PermitLimit", "1");
builder.UseSetting($"RateLimiting:{policy}:WindowSeconds", "86400");
}
builder.ConfigureTestServices(services =>
{
services.AddSingleton<IStartupFilter, FakeRemoteIpStartupFilter>();
services.AddSingleton<IStartupFilter, OAuthApiProbe>();
services.AddSingleton<ILogEventSink>(Sink);
});
}

// No real endpoint accepts OAuth tokens yet; the limiter runs before authentication,
// so an unauthenticated call still spends the oauth-api permit.
private sealed class OAuthApiProbe : IStartupFilter
{
public Action<IApplicationBuilder> Configure(Action<IApplicationBuilder> next) => app =>
{
next(app);
var endpoints = (IEndpointRouteBuilder)app.Properties["__EndpointRouteBuilder"]!;
endpoints.MapGet(OAuthApiProbePath, () => Results.Ok()).AcceptOAuthTokens("cw1164.read");
};
}

public const string OAuthApiProbePath = "/test/oauth/rate-limited";

public sealed class CollectingSink : ILogEventSink
{
public ConcurrentQueue<LogEvent> Events { get; } = new();
Expand All @@ -61,9 +87,11 @@ public sealed class AuthRateLimitLoggingTests(AuthRateLimitLoggingFactory factor
private IReadOnlyList<LogEvent> EventsFor(string securityEvent) =>
[.. factory.Sink.Events.Where(e => ScalarOf(e, "SecurityEvent") == securityEvent)];

private HttpClient ProxiedClient(string clientIp)
private HttpClient ProxiedClient(string clientIp, Uri? baseAddress = null)
{
var client = factory.CreateClient();
var client = baseAddress is null
? factory.CreateClient()
: factory.CreateClient(new() { BaseAddress = baseAddress, AllowAutoRedirect = false });
client.DefaultRequestHeaders.Add("X-Test-Remote", AuthRateLimitLoggingFactory.TrustedProxy);
client.DefaultRequestHeaders.Add("X-Forwarded-For", clientIp);
return client;
Expand Down Expand Up @@ -129,6 +157,61 @@ public async Task StepUp_rate_limit_rejection_emits_RateLimitRejected_because_it
Assert.Contains("step-up", ScalarOf(rejected, "Path"));
}

// #1164 — every OAuth policy emits the event with its policy name. Each request
// carries a secret where that endpoint takes one (query string, form body, bearer);
// none may reach the event.
private const string Secret = "cw1164-secret-sentinel";

[Theory]
[InlineData(RateLimitingOptions.OAuthAuthorizePolicyName, "203.0.113.211")]
[InlineData(RateLimitingOptions.OAuthTokenPolicyName, "203.0.113.212")]
[InlineData(RateLimitingOptions.OAuthRegisterPolicyName, "203.0.113.213")]
[InlineData(RateLimitingOptions.OAuthApiPolicyName, "203.0.113.214")]
public async Task OAuth_rate_limit_rejection_emits_RateLimitRejected_with_policy_ip_and_path(
string policy, string clientIp)
{
factory.Sink.Events.Clear();
var client = ProxiedClient(clientIp, new Uri("https://localhost"));
// Unique per run: oauth-api is keyed per bearer, and the factory outlives one test.
client.DefaultRequestHeaders.Authorization =
new AuthenticationHeaderValue("Bearer", $"{Secret}-{Guid.NewGuid():N}");
var (path, send) = OAuthRequest(policy, client);

using var allowed = await send();
Assert.NotEqual(HttpStatusCode.TooManyRequests, allowed.StatusCode);
Assert.Empty(EventsFor(SecurityEvents.RateLimitRejected));

using var limited = await send();
Assert.Equal(HttpStatusCode.TooManyRequests, limited.StatusCode);
Assert.NotNull(limited.Headers.RetryAfter);
var rejected = Assert.Single(EventsFor(SecurityEvents.RateLimitRejected));
Assert.Equal(LogEventLevel.Warning, rejected.Level);
Assert.Equal(policy, ScalarOf(rejected, "Policy"));
Assert.Equal(clientIp, ScalarOf(rejected, "ClientIp"));
Assert.Equal(path, ScalarOf(rejected, "Path"));
Assert.DoesNotContain(Secret, rejected.RenderMessage());
}

private static (string Path, Func<Task<HttpResponseMessage>> Send) OAuthRequest(string policy, HttpClient client) =>
policy switch
{
RateLimitingOptions.OAuthAuthorizePolicyName => ("/api/v1/oauth/authorize",
() => client.GetAsync($"/api/v1/oauth/authorize?client_id=cw1164&code_challenge={Secret}")),
RateLimitingOptions.OAuthTokenPolicyName => ("/api/v1/oauth/token",
() => client.PostAsync("/api/v1/oauth/token", new FormUrlEncodedContent(new Dictionary<string, string>
{
["grant_type"] = "authorization_code",
["client_id"] = "cw1164",
["code"] = Secret,
["code_verifier"] = Secret,
}))),
RateLimitingOptions.OAuthRegisterPolicyName => ("/api/v1/oauth/register",
() => client.PostAsJsonAsync("/api/v1/oauth/register", new { client_name = Secret })),
RateLimitingOptions.OAuthApiPolicyName => (AuthRateLimitLoggingFactory.OAuthApiProbePath,
() => client.GetAsync(AuthRateLimitLoggingFactory.OAuthApiProbePath)),
_ => throw new ArgumentOutOfRangeException(nameof(policy), policy, null),
};

// Scope guard — proves the event is NOT over-fired for the non-auth policy
// sharing the same OnRejected delegate.
[Fact]
Expand Down