Heimdall sign-in: redact the app secret from client logging, expire the attempt cookie at its path, bound the return path
- The HeimdallAuthService client redacts x-heimdall-app-secret from HTTP client logging at every level; a Trace canary test pins it. - The attempt cookie's deletion carries its Path and no Max-Age (which outranks the 1970 expiry); pinned on success and refusal paths. - LocalReturnPath is at most 256 characters, else /App, so the attempt cookie stays far below the browser limit; pinned at the bound and with the heaviest-escaping path. - A set BaseUrl with a blank AppSlug is refused at Start before any Heimdall call; behavioural test. Builds untested until the verification job at the end of this batch. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
f5432f7a42
commit
d695674359
5 changed files with 136 additions and 6 deletions
|
|
@ -418,4 +418,16 @@ public static class HeimdallAttemptCookie
|
|||
Path = "/auth/heimdall",
|
||||
MaxAge = Lifetime
|
||||
};
|
||||
|
||||
/// <summary>
|
||||
/// The options a deletion carries: the attempt cookie's Path, which a cookie is matched on, and no Max-Age,
|
||||
/// which would take precedence over the expiry Delete writes and leave the emptied cookie alive.
|
||||
/// </summary>
|
||||
public static CookieOptions DeleteOptions() => new()
|
||||
{
|
||||
HttpOnly = true,
|
||||
Secure = true,
|
||||
SameSite = SameSiteMode.Lax,
|
||||
Path = "/auth/heimdall"
|
||||
};
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2,13 +2,16 @@ namespace Bifrost.Web.Features.Shared;
|
|||
|
||||
/// <summary>
|
||||
/// A sign-in return target that stays on this site. The only way to make one is <see cref="Parse"/>, which
|
||||
/// accepts a path that starts with a single slash and holds no control character, and otherwise yields
|
||||
/// <see cref="Default"/>. Callers parse once at entry and carry the typed value from there.
|
||||
/// accepts a path of at most <see cref="MaxLength"/> characters that starts with a single slash and holds no
|
||||
/// control character, and otherwise yields <see cref="Default"/>. Callers parse once at entry and carry the typed value from there.
|
||||
/// </summary>
|
||||
public sealed record LocalReturnPath
|
||||
{
|
||||
private LocalReturnPath(string value) => Value = value;
|
||||
|
||||
/// <summary>Bounds the path so the attempt cookie that carries it stays far below a browser's 4096-byte limit.</summary>
|
||||
public const int MaxLength = 256;
|
||||
|
||||
public static LocalReturnPath Default { get; } = new("/App");
|
||||
|
||||
public string Value { get; }
|
||||
|
|
@ -16,6 +19,7 @@ public sealed record LocalReturnPath
|
|||
public static LocalReturnPath Parse(string? candidate)
|
||||
{
|
||||
if (string.IsNullOrEmpty(candidate) ||
|
||||
candidate.Length > MaxLength ||
|
||||
candidate[0] != '/' ||
|
||||
candidate.Any(char.IsControl) ||
|
||||
(candidate.Length > 1 && (candidate[1] == '/' || candidate[1] == '\\')))
|
||||
|
|
|
|||
|
|
@ -125,7 +125,8 @@ builder.Services.AddScoped<ApplicationBootstrapper>();
|
|||
builder.Services.AddSingleton<StartupConfigurationValidator>();
|
||||
builder.Services.AddScoped<IAuthorizationHandler, ActiveMemberAuthorizationHandler>();
|
||||
builder.Services.AddHttpClient<HeimdallAuthService>((serviceProvider, client) =>
|
||||
HeimdallAuthService.ConfigureClient(client, serviceProvider.GetRequiredService<IOptions<HeimdallOptions>>().Value));
|
||||
HeimdallAuthService.ConfigureClient(client, serviceProvider.GetRequiredService<IOptions<HeimdallOptions>>().Value))
|
||||
.RedactLoggedHeaders(header => string.Equals(header, HeimdallAuthService.AppSecretHeader, StringComparison.OrdinalIgnoreCase));
|
||||
builder.Services.AddSingleton<HeimdallTokenVerifier>();
|
||||
builder.Services.AddHttpClient(HeimdallTokenVerifier.ClientName, (serviceProvider, client) =>
|
||||
HeimdallTokenVerifier.ConfigureClient(client, serviceProvider.GetRequiredService<IOptions<HeimdallOptions>>().Value));
|
||||
|
|
@ -537,7 +538,7 @@ app.MapPost("/auth/heimdall/complete", async (
|
|||
form?["code"].ToString(),
|
||||
cancellationToken);
|
||||
await httpContext.SignInAsync(CookieAuthenticationDefaults.AuthenticationScheme, signIn.Principal);
|
||||
httpContext.Response.Cookies.Delete(cookieName!, HeimdallAttemptCookie.Options());
|
||||
httpContext.Response.Cookies.Delete(cookieName!, HeimdallAttemptCookie.DeleteOptions());
|
||||
return Results.LocalRedirect(signIn.ReturnPath.Value);
|
||||
}
|
||||
catch (HeimdallAuthException error)
|
||||
|
|
@ -545,7 +546,7 @@ app.MapPost("/auth/heimdall/complete", async (
|
|||
logger.LogWarning("Heimdall completion refused: {Reason}", error.Message);
|
||||
if (cookieName is not null)
|
||||
{
|
||||
httpContext.Response.Cookies.Delete(cookieName, HeimdallAttemptCookie.Options());
|
||||
httpContext.Response.Cookies.Delete(cookieName, HeimdallAttemptCookie.DeleteOptions());
|
||||
}
|
||||
|
||||
return Results.Redirect(HeimdallAuthService.ErrorRedirect);
|
||||
|
|
|
|||
|
|
@ -4,10 +4,14 @@ using System.Text.Json;
|
|||
using Bifrost.Web.Domain;
|
||||
using Bifrost.Web.Features.Heimdall;
|
||||
using Bifrost.Web.Features.Membership;
|
||||
using Bifrost.Web.Features.Shared;
|
||||
using Bifrost.Web.Tests.Support;
|
||||
using Microsoft.AspNetCore.Authentication.OAuth;
|
||||
using Microsoft.AspNetCore.Hosting;
|
||||
using Microsoft.AspNetCore.Mvc.Testing;
|
||||
using Microsoft.AspNetCore.WebUtilities;
|
||||
using Microsoft.Extensions.DependencyInjection;
|
||||
using Microsoft.Extensions.Logging;
|
||||
using Microsoft.Extensions.Options;
|
||||
|
||||
namespace Bifrost.Web.Tests;
|
||||
|
|
@ -1139,4 +1143,109 @@ public sealed class HeimdallCompletionTests
|
|||
Assert.DoesNotContain(canary, logs);
|
||||
}
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task The_app_secret_reaches_Heimdall_but_is_never_logged_even_at_trace()
|
||||
{
|
||||
const string canary = "CANARY-app-secret-7c1e";
|
||||
using var host = new HeimdallHost(appSecret: canary);
|
||||
host.Heimdall.RequiredAppSecret = canary;
|
||||
using var traced = host.WithWebHostBuilder(builder => builder.ConfigureLogging(logging =>
|
||||
{
|
||||
logging.SetMinimumLevel(LogLevel.Trace);
|
||||
logging.AddFilter((string? _, LogLevel _) => true);
|
||||
logging.AddFilter("System.Net.Http", LogLevel.Trace);
|
||||
logging.AddFilter("Microsoft", LogLevel.Trace);
|
||||
}));
|
||||
using var client = traced.CreateClient(new WebApplicationFactoryClientOptions
|
||||
{
|
||||
AllowAutoRedirect = false,
|
||||
HandleCookies = false,
|
||||
BaseAddress = new Uri("https://localhost")
|
||||
});
|
||||
|
||||
using var start = await client.GetAsync("/auth/heimdall/discord?access=member&returnUrl=/App");
|
||||
|
||||
Assert.Equal(HttpStatusCode.Redirect, start.StatusCode);
|
||||
Assert.Equal(canary, Assert.Single(host.Heimdall.StartSecrets));
|
||||
var logs = string.Join("
|
||||
", host.Logs.Lines);
|
||||
Assert.Contains(HeimdallAuthService.AppSecretHeader, logs, StringComparison.OrdinalIgnoreCase);
|
||||
Assert.DoesNotContain(canary, logs);
|
||||
}
|
||||
|
||||
private static void AssertAttemptCookieExpired(HttpResponseMessage response, string nonce)
|
||||
{
|
||||
var name = HeimdallAttemptCookie.Prefix + nonce;
|
||||
var header = HeimdallHost.SetCookieHeader(response, name);
|
||||
Assert.NotNull(header);
|
||||
Assert.StartsWith(name + "=;", header);
|
||||
var attributes = header!.Split(';').Select(x => x.Trim().ToLowerInvariant()).ToArray();
|
||||
Assert.Contains("path=/auth/heimdall", attributes);
|
||||
Assert.Contains(attributes, x => x.StartsWith("expires=", StringComparison.Ordinal) && x.Contains("1970"));
|
||||
Assert.DoesNotContain(attributes, x => x.StartsWith("max-age", StringComparison.Ordinal));
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task The_attempt_cookie_is_expired_at_its_path_on_success_and_on_refusal()
|
||||
{
|
||||
using var host = new HeimdallHost();
|
||||
var accountId = HeimdallShapes.NewAccountId();
|
||||
|
||||
var signedAttempt = await host.StartAsync();
|
||||
var signedIn = await Genuine(host, signedAttempt, accountId);
|
||||
Assert.True(signedIn.SignedIn);
|
||||
AssertAttemptCookieExpired(signedIn.Response, signedAttempt.Nonce);
|
||||
|
||||
var unknownCode = await host.StartAsync();
|
||||
var gone = await host.CompleteAsync(unknownCode, "unknown-code");
|
||||
AssertRefusedRedirect(gone);
|
||||
AssertAttemptCookieExpired(gone.Response, unknownCode.Nonce);
|
||||
|
||||
var tampered = await host.StartAsync();
|
||||
var refused = await host.CompleteAsync(
|
||||
tampered.Nonce, host.CodeFor(tampered, accountId), Corrupt(tampered.Cookie!));
|
||||
AssertRefusedRedirect(refused);
|
||||
AssertAttemptCookieExpired(refused.Response, tampered.Nonce);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task Return_paths_are_bounded_so_the_attempt_cookie_stays_small()
|
||||
{
|
||||
using var host = new HeimdallHost();
|
||||
var atBound = "/" + new string('a', LocalReturnPath.MaxLength - 1);
|
||||
var overBound = atBound + "a";
|
||||
var worstCase = "/" + new string('<', LocalReturnPath.MaxLength - 1);
|
||||
|
||||
Assert.Equal(atBound, LocalReturnPath.Parse(atBound).Value);
|
||||
Assert.Equal(LocalReturnPath.Default, LocalReturnPath.Parse(overBound));
|
||||
|
||||
var kept = await host.StartAsync(returnUrl: atBound);
|
||||
var keptSignIn = await Genuine(host, kept, HeimdallShapes.NewAccountId());
|
||||
Assert.True(keptSignIn.SignedIn);
|
||||
Assert.Equal(atBound, keptSignIn.Response.Headers.Location!.OriginalString);
|
||||
|
||||
var over = await host.StartAsync(returnUrl: overBound);
|
||||
var overSignIn = await Genuine(host, over, HeimdallShapes.NewAccountId());
|
||||
Assert.True(overSignIn.SignedIn);
|
||||
Assert.Equal("/App", overSignIn.Response.Headers.Location!.OriginalString);
|
||||
|
||||
var heaviest = await host.StartAsync(returnUrl: worstCase);
|
||||
var header = HeimdallHost.SetCookieHeader(heaviest.Response, HeimdallAttemptCookie.Prefix + heaviest.Nonce)!;
|
||||
Assert.InRange(header.Split(';')[0].Length, 1, 3072);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task A_blank_app_slug_is_refused_before_Heimdall_is_called()
|
||||
{
|
||||
using var host = new HeimdallHost(appSlug: " ");
|
||||
|
||||
var refused = await host.StartAsync();
|
||||
|
||||
Assert.Equal(ErrorRedirect, refused.Response.Headers.Location!.OriginalString);
|
||||
Assert.Empty(host.Heimdall.StartBodies);
|
||||
Assert.False(refused.Response.Headers.Contains("Set-Cookie"));
|
||||
Assert.Contains("Heimdall start refused: Heimdall is not configured.", string.Join("
|
||||
", host.Logs.Lines));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -347,6 +347,7 @@ public sealed class HeimdallHost : TestWebApplicationFactory
|
|||
public const string AppSecret = "test-heimdall-app-secret";
|
||||
|
||||
private readonly string _appSecret;
|
||||
private readonly string _appSlug;
|
||||
private readonly string _baseUrl;
|
||||
private readonly string _publicBaseUrl;
|
||||
private readonly bool _discordRoles;
|
||||
|
|
@ -363,9 +364,11 @@ public sealed class HeimdallHost : TestWebApplicationFactory
|
|||
string appSecret = AppSecret,
|
||||
string baseUrl = HeimdallShapes.Issuer,
|
||||
string publicBaseUrl = "https://localhost",
|
||||
bool discordRoles = true)
|
||||
bool discordRoles = true,
|
||||
string appSlug = "bifrost")
|
||||
{
|
||||
_appSecret = appSecret;
|
||||
_appSlug = appSlug;
|
||||
_baseUrl = baseUrl;
|
||||
_publicBaseUrl = publicBaseUrl;
|
||||
_discordRoles = discordRoles;
|
||||
|
|
@ -385,6 +388,7 @@ public sealed class HeimdallHost : TestWebApplicationFactory
|
|||
{
|
||||
["Heimdall:BaseUrl"] = _baseUrl,
|
||||
["Heimdall:AppSecret"] = _appSecret,
|
||||
["Heimdall:AppSlug"] = _appSlug,
|
||||
["Heimdall:DiscordGuildId"] = "guild-1",
|
||||
["Host:PublicBaseUrl"] = _publicBaseUrl
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in a new issue