diff --git a/src/ServiceControl.AcceptanceTesting/OpenIdConnect/MockOidcServer.cs b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/MockOidcServer.cs index 03f5c156eb..81322cf47c 100644 --- a/src/ServiceControl.AcceptanceTesting/OpenIdConnect/MockOidcServer.cs +++ b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/MockOidcServer.cs @@ -184,9 +184,13 @@ public string GenerateToken( { var credentials = new SigningCredentials(securityKey, SecurityAlgorithms.RsaSha256); + // sub + preferred_username are required by PermissionVerbHandler for the audit log; + // defaulting them here keeps callers concise. Callers that need to test the + // missing-claim path pass an explicit additionalClaim with an empty value to override. var claims = new List { new(JwtRegisteredClaimNames.Sub, subject), + new("preferred_username", subject), new(JwtRegisteredClaimNames.Jti, Guid.NewGuid().ToString()) }; diff --git a/src/ServiceControl.Audit.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt index c73fd28a46..4a1b91b263 100644 --- a/src/ServiceControl.Audit.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt +++ b/src/ServiceControl.Audit.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt @@ -12,6 +12,8 @@ "ValidateLifetime": true, "ValidateIssuerSigningKey": true, "RequireHttpsMetadata": true, + "SubjectIdClaim": "sub", + "SubjectNameClaim": "preferred_username", "ServicePulseAuthority": null, "ServicePulseClientId": null, "ServicePulseApiScopes": null, diff --git a/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs b/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs index 82d6c3fcbc..dcf84d9637 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs @@ -6,6 +6,7 @@ namespace ServiceControl.Hosting.Auth; using Microsoft.Extensions.Hosting; using Microsoft.Extensions.Options; using ServiceControl.Infrastructure; +using ServiceControl.Infrastructure.Auth; /// /// Registers the permission-based policy authorization services: a dynamic @@ -37,6 +38,17 @@ public static void AddServiceControlAuthorization(this IHostApplicationBuilder h services.AddSingleton(sp => new PermissionPolicyProvider(sp.GetRequiredService>(), oidcSettings)); - services.AddSingleton(_ => new PermissionVerbHandler(oidcSettings.RolesClaim)); + // The provider only emits a PermissionRequirement when RBAC is enabled, so the handler is the + // only thing that evaluates one. It is registered alongside the provider (cheap singleton, never + // invoked when no requirement is produced). The handler emits an audit-log entry for every + // decision through IAuthorizationAuditLog so the platform can show, after the fact, who attempted + // what and how the system responded. The subject-id and subject-name claim names flow through + // from configuration so the handler can read them off the principal. + services.AddSingleton(); + services.AddSingleton(sp => + new PermissionVerbHandler( + sp.GetRequiredService(), + oidcSettings.SubjectIdClaim, + oidcSettings.SubjectNameClaim)); } } diff --git a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs index 68a56e29ba..65cab4842b 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs @@ -1,7 +1,9 @@ #nullable enable namespace ServiceControl.Hosting.Auth; +using System; using System.Linq; +using System.Security.Claims; using System.Threading.Tasks; using Microsoft.AspNetCore.Authorization; using ServiceControl.Infrastructure.Auth; @@ -9,34 +11,80 @@ namespace ServiceControl.Hosting.Auth; /// /// Verb-level authorization handler for . It resolves the user's /// roles and checks them against the hardcoded policy: the user must hold -/// a role (e.g. reader / writer) that grants the requested permission. +/// a role (e.g. reader / writer) that grants the requested permission. Every decision is +/// captured through for compliance. /// /// Only registered — and only reached — when OIDC is enabled. When it is disabled, /// returns an allow-all policy that carries no /// , so this handler is not needed. /// /// -public sealed class PermissionVerbHandler : AuthorizationHandler +public sealed class PermissionVerbHandler( + IAuthorizationAuditLog auditLog, + string subjectIdClaim, + string subjectNameClaim) + : AuthorizationHandler { - public PermissionVerbHandler(string rolesClaimName) - { - RoleClaimType = rolesClaimName; - } + // The per-IdP variability of the source claim is absorbed by RolesClaimsTransformation, which + // reads from the path configured in Authentication.RolesClaim and emits canonical "roles" claims. + const string RoleClaimType = "roles"; protected override Task HandleRequirementAsync( AuthorizationHandlerContext context, PermissionRequirement requirement) { - var roles = context.User.FindAll(RoleClaimType).Select(claim => claim.Value); + // Unauthenticated requests have no subject and no roles. The framework will challenge with + // 401 because the policy also includes RequireAuthenticatedUser; skipping here keeps the + // audit log restricted to identified principals. + if (context.User.Identity?.IsAuthenticated != true) + { + return Task.CompletedTask; + } + + var subjectId = RequireClaim(context.User, subjectIdClaim, "Authentication.SubjectIdClaim"); + var subjectName = RequireClaim(context.User, subjectNameClaim, "Authentication.SubjectNameClaim"); + var roles = context.User.FindAll(RoleClaimType).Select(claim => claim.Value).ToArray(); + var permission = requirement.Permission; - if (RolePermissions.IsGranted(roles, requirement.Permission)) + if (RolePermissions.IsGranted(roles, permission)) { + auditLog.Decision( + subjectId, + subjectName, + permission, + resource: null, + allowed: true, + reason: roles.Length == 0 + ? $"User holds '{permission}'" + : $"User holds '{permission}' via role(s) [{string.Join(", ", roles)}]"); + context.Succeed(requirement); + return Task.CompletedTask; } - // Otherwise leave the requirement unmet → the request is denied (403/401). + auditLog.Decision( + subjectId, + subjectName, + permission, + resource: null, + allowed: false, + reason: roles.Length == 0 + ? $"User has no roles granting '{permission}'" + : $"None of the user's role(s) [{string.Join(", ", roles)}] grants '{permission}'"); + + // Leave the requirement unmet → the framework forbids (403). return Task.CompletedTask; } - internal string RoleClaimType = "roles"; -} \ No newline at end of file + static string RequireClaim(ClaimsPrincipal user, string claimType, string settingName) + { + var value = user.FindFirst(claimType)?.Value; + if (string.IsNullOrEmpty(value)) + { + throw new InvalidOperationException( + $"Authenticated principal is missing the required '{claimType}' claim configured by {settingName}. " + + "Configure the identity provider to emit this claim, or point the setting at the claim the IdP actually emits."); + } + return value; + } +} diff --git a/src/ServiceControl.Infrastructure.Tests/Auth/AuthorizationAuditLogTests.cs b/src/ServiceControl.Infrastructure.Tests/Auth/AuthorizationAuditLogTests.cs new file mode 100644 index 0000000000..aa6a471373 --- /dev/null +++ b/src/ServiceControl.Infrastructure.Tests/Auth/AuthorizationAuditLogTests.cs @@ -0,0 +1,94 @@ +#nullable enable +namespace ServiceControl.Infrastructure.Tests.Auth; + +using System; +using Microsoft.Extensions.Logging; +using NUnit.Framework; +using ServiceControl.Infrastructure.Auth; + +[TestFixture] +public class AuthorizationAuditLogTests +{ + [Test] + public void Decision_allow_emits_one_entry_on_audit_category() + { + var provider = new RecordingLoggerProvider(); + var factory = LoggerFactory.Create(b => b.AddProvider(provider)); + var auditLog = new AuthorizationAuditLog(factory); + + auditLog.Decision("alice-sub-001", "Alice Smith", "error:messages:retry", "acme.sales", allowed: true, reason: "role:reader matched"); + + var entries = provider.EntriesFor("ServiceControl.Audit"); + Assert.That(entries, Has.Count.EqualTo(1)); + Assert.That(entries[0].Message, Does.Contain("alice-sub-001")); + Assert.That(entries[0].Message, Does.Contain("Alice Smith")); + Assert.That(entries[0].Message, Does.Contain("error:messages:retry")); + Assert.That(entries[0].Message, Does.Contain("allow")); + Assert.That(entries[0].Level, Is.EqualTo(LogLevel.Information)); + } + + [Test] + public void Decision_deny_emits_one_entry_on_audit_category() + { + var provider = new RecordingLoggerProvider(); + var factory = LoggerFactory.Create(b => b.AddProvider(provider)); + var auditLog = new AuthorizationAuditLog(factory); + + auditLog.Decision("bob-sub-002", "Bob Jones", "error:messages:retry", null, allowed: false, reason: "no matching role"); + + var entries = provider.EntriesFor("ServiceControl.Audit"); + Assert.That(entries, Has.Count.EqualTo(1)); + Assert.That(entries[0].Message, Does.Contain("bob-sub-002")); + Assert.That(entries[0].Message, Does.Contain("Bob Jones")); + Assert.That(entries[0].Message, Does.Contain("error:messages:retry")); + Assert.That(entries[0].Message, Does.Contain("deny")); + Assert.That(entries[0].Level, Is.EqualTo(LogLevel.Information)); + } + + [Test] + public void Decision_does_not_appear_on_other_categories() + { + var provider = new RecordingLoggerProvider(); + var factory = LoggerFactory.Create(b => b.AddProvider(provider)); + var auditLog = new AuthorizationAuditLog(factory); + + auditLog.Decision("carol-sub-003", "Carol White", "error:endpoints:view", null, allowed: true, reason: "role:reader matched"); + + Assert.That(provider.EntriesFor("ServiceControl.SomeOtherCategory"), Is.Empty); + } + + [Test] + public void Multiple_decisions_accumulate_in_order() + { + var provider = new RecordingLoggerProvider(); + var factory = LoggerFactory.Create(b => b.AddProvider(provider)); + var auditLog = new AuthorizationAuditLog(factory); + + auditLog.Decision("alice-sub-001", "alice", "error:messages:view", null, allowed: true, "role matched"); + auditLog.Decision("alice-sub-001", "alice", "error:messages:retry", "acme.finance", allowed: false, "out of scope"); + + var entries = provider.EntriesFor("ServiceControl.Audit"); + Assert.That(entries, Has.Count.EqualTo(2)); + Assert.That(entries[0].Message, Does.Contain("allow")); + Assert.That(entries[1].Message, Does.Contain("deny")); + } + + [TestCase(null, "Alice", "error:messages:retry", "reason")] + [TestCase("", "Alice", "error:messages:retry", "reason")] + [TestCase("alice-sub-001", null, "error:messages:retry", "reason")] + [TestCase("alice-sub-001", "", "error:messages:retry", "reason")] + [TestCase("alice-sub-001", "Alice", null, "reason")] + [TestCase("alice-sub-001", "Alice", "", "reason")] + [TestCase("alice-sub-001", "Alice", "error:messages:retry", null)] + [TestCase("alice-sub-001", "Alice", "error:messages:retry", "")] + public void Decision_throws_when_required_argument_is_null_or_empty(string? subjectId, string? subjectName, string? permission, string? reason) + { + var provider = new RecordingLoggerProvider(); + var factory = LoggerFactory.Create(b => b.AddProvider(provider)); + var auditLog = new AuthorizationAuditLog(factory); + + Assert.That( + () => auditLog.Decision(subjectId!, subjectName!, permission!, resource: null, allowed: true, reason: reason!), + Throws.InstanceOf()); + } +} diff --git a/src/ServiceControl.Infrastructure.Tests/Auth/RecordingLoggerProvider.cs b/src/ServiceControl.Infrastructure.Tests/Auth/RecordingLoggerProvider.cs new file mode 100644 index 0000000000..a753a2b8e4 --- /dev/null +++ b/src/ServiceControl.Infrastructure.Tests/Auth/RecordingLoggerProvider.cs @@ -0,0 +1,59 @@ +#nullable enable +namespace ServiceControl.Infrastructure.Tests.Auth; + +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using Microsoft.Extensions.Logging; + +/// +/// In-memory that captures log entries for test assertions. +/// Thread-safe. Use for all captured entries; +/// to filter by category. +/// +sealed class RecordingLoggerProvider : ILoggerProvider +{ + readonly ConcurrentQueue entries = new(); + + public IReadOnlyList Entries => entries.ToArray(); + + public IReadOnlyList EntriesFor(string category) => + entries.Where(e => e.Category == category).ToArray(); + + public ILogger CreateLogger(string categoryName) => + new RecordingLogger(categoryName, entries); + + public void Dispose() { } +} + +sealed record LogEntry( + string Category, + LogLevel Level, + EventId EventId, + string Message, + Exception? Exception); + +sealed class RecordingLogger(string category, ConcurrentQueue sink) : ILogger +{ + public IDisposable? BeginScope(TState state) where TState : notnull => NullScope.Instance; + + public bool IsEnabled(LogLevel logLevel) => logLevel != LogLevel.None; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + var message = formatter(state, exception); + sink.Enqueue(new LogEntry(category, logLevel, eventId, message, exception)); + } + + sealed class NullScope : IDisposable + { + public static readonly NullScope Instance = new(); + public void Dispose() { } + } +} diff --git a/src/ServiceControl.Infrastructure/Auth/AuthorizationAuditLog.cs b/src/ServiceControl.Infrastructure/Auth/AuthorizationAuditLog.cs new file mode 100644 index 0000000000..6ac35835a9 --- /dev/null +++ b/src/ServiceControl.Infrastructure/Auth/AuthorizationAuditLog.cs @@ -0,0 +1,45 @@ +#nullable enable +namespace ServiceControl.Infrastructure.Auth; + +using System; +using Microsoft.Extensions.Logging; + +/// +/// Default that emits every decision as a structured log entry on +/// the stable category ServiceControl.Audit. Sinks filter on the category, not on the type name. +/// +public sealed partial class AuthorizationAuditLog : IAuthorizationAuditLog +{ + const string AuditCategory = "ServiceControl.Audit"; + + readonly ILogger logger; + + public AuthorizationAuditLog(ILoggerFactory loggerFactory) + { + logger = loggerFactory.CreateLogger(AuditCategory); + } + + public void Decision(string subjectId, string subjectName, string permission, string? resource, bool allowed, string reason) + { + ArgumentException.ThrowIfNullOrEmpty(subjectId); + ArgumentException.ThrowIfNullOrEmpty(subjectName); + ArgumentException.ThrowIfNullOrEmpty(permission); + ArgumentException.ThrowIfNullOrEmpty(reason); + + LogDecision(logger, subjectId, subjectName, permission, resource, allowed ? "allow" : "deny", reason); + } + + // Source-generated structured log method — zero allocation on the hot path. + [LoggerMessage( + EventId = 1001, + Level = LogLevel.Information, + Message = "Authorization {Outcome}: subjectId={SubjectId} subjectName={SubjectName} permission={Permission} resource={Resource} reason={Reason}")] + static partial void LogDecision( + ILogger logger, + string subjectId, + string subjectName, + string permission, + string? resource, + string outcome, + string reason); +} diff --git a/src/ServiceControl.Infrastructure/Auth/IAuthorizationAuditLog.cs b/src/ServiceControl.Infrastructure/Auth/IAuthorizationAuditLog.cs new file mode 100644 index 0000000000..d6449673cc --- /dev/null +++ b/src/ServiceControl.Infrastructure/Auth/IAuthorizationAuditLog.cs @@ -0,0 +1,25 @@ +#nullable enable +namespace ServiceControl.Infrastructure.Auth; + +/// +/// Records every authorization allow/deny decision so the platform can demonstrate, after the fact, +/// who attempted what and how the system responded. Both allow and deny outcomes are captured — +/// denies alone are insufficient for most compliance use cases. +/// +/// Implementations write structured log entries on a stable category so sinks (Seq, OTLP, file, +/// in-memory test double, …) can filter on it without coupling to the concrete type name. +/// +/// +public interface IAuthorizationAuditLog +{ + /// + /// Records a single authorization decision. + /// + /// Stable identifier of the principal (e.g. the JWT sub claim). Must not be null or empty. + /// Human-readable display name of the principal (e.g. preferred_username). Must not be null or empty. + /// The permission that was evaluated (e.g. error:messages:retry). + /// The specific resource checked, or for verb-level checks. + /// if the decision was allow; for deny. + /// A human-readable explanation (e.g. which role granted the permission, or why nothing matched). + void Decision(string subjectId, string subjectName, string permission, string? resource, bool allowed, string reason); +} diff --git a/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs b/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs index 9c2d74158f..33f0e461a0 100644 --- a/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs +++ b/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs @@ -35,6 +35,13 @@ public OpenIdConnectSettings(SettingsRootNamespace rootNamespace, bool validateC RolesClaim = SettingsReader.Read(rootNamespace, "Authentication.RolesClaim", "roles"); RoleBasedAuthorizationEnabled = SettingsReader.Read(rootNamespace, "Authentication.RoleBasedAuthorizationEnabled", false); + // Claims that identify the principal in the authorization audit log. The handler treats both + // as required — a missing or empty value is a sign that the IdP isn't emitting the expected + // claim and the operator needs to fix the configuration, so the handler will throw rather + // than substitute a placeholder. + SubjectIdClaim = SettingsReader.Read(rootNamespace, "Authentication.SubjectIdClaim", "sub"); + SubjectNameClaim = SettingsReader.Read(rootNamespace, "Authentication.SubjectNameClaim", "preferred_username"); + // ServicePulse settings are only relevant for the primary ServiceControl instance // which serves the OIDC configuration endpoint that ServicePulse uses for login if (requireServicePulseSettings) @@ -99,6 +106,20 @@ public OpenIdConnectSettings(SettingsRootNamespace rootNamespace, bool validateC /// public bool RequireHttpsMetadata { get; } + /// + /// Claim that carries the stable subject identifier (e.g. the JWT sub claim) recorded in + /// the authorization audit log. Required — the handler throws if the configured claim is absent + /// or empty on an authenticated principal. + /// + public string SubjectIdClaim { get; } + + /// + /// Claim that carries the human-readable subject name (e.g. preferred_username) recorded + /// in the authorization audit log. Required — the handler throws if the configured claim is + /// absent or empty on an authenticated principal. + /// + public string SubjectNameClaim { get; } + /// /// Optional override for the authority URL that ServicePulse should use for authentication. /// If not specified, ServicePulse uses the main Authority value. @@ -205,8 +226,8 @@ void LogConfiguration(bool requireServicePulseSettings) var servicePulseAuthorityDisplay = requireServicePulseSettings ? (ServicePulseAuthority ?? "(not configured)") : "(n/a)"; var servicePulseApiScopesDisplay = requireServicePulseSettings ? (ServicePulseApiScopes ?? "(not configured)") : "(n/a)"; - logger.LogInformation("Authentication settings: Enabled={Enabled}, Authority={Authority}, Audience={Audience}, ValidateIssuer={ValidateIssuer}, ValidateAudience={ValidateAudience}, ValidateLifetime={ValidateLifetime}, ValidateIssuerSigningKey={ValidateIssuerSigningKey}, RequireHttpsMetadata={RequireHttpsMetadata}, RolesClaim={RolesClaim}, ServicePulseClientId={ServicePulseClientId}, ServicePulseAuthority={ServicePulseAuthority}, ServicePulseApiScopes={ServicePulseApiScopes}", - Enabled, authorityDisplay, audienceDisplay, ValidateIssuer, ValidateAudience, ValidateLifetime, ValidateIssuerSigningKey, RequireHttpsMetadata, RolesClaim, servicePulseClientIdDisplay, servicePulseAuthorityDisplay, servicePulseApiScopesDisplay); + logger.LogInformation("Authentication settings: Enabled={Enabled}, Authority={Authority}, Audience={Audience}, ValidateIssuer={ValidateIssuer}, ValidateAudience={ValidateAudience}, ValidateLifetime={ValidateLifetime}, ValidateIssuerSigningKey={ValidateIssuerSigningKey}, RequireHttpsMetadata={RequireHttpsMetadata}, RolesClaim={RolesClaim}, SubjectIdClaim={SubjectIdClaim}, SubjectNameClaim={SubjectNameClaim}, ServicePulseClientId={ServicePulseClientId}, ServicePulseAuthority={ServicePulseAuthority}, ServicePulseApiScopes={ServicePulseApiScopes}", + Enabled, authorityDisplay, audienceDisplay, ValidateIssuer, ValidateAudience, ValidateLifetime, ValidateIssuerSigningKey, RequireHttpsMetadata, RolesClaim, SubjectIdClaim, SubjectNameClaim, servicePulseClientIdDisplay, servicePulseAuthorityDisplay, servicePulseApiScopesDisplay); // Warn about potential misconfigurations var hasAuthConfig = !string.IsNullOrWhiteSpace(Authority) || !string.IsNullOrWhiteSpace(Audience); diff --git a/src/ServiceControl.Monitoring.UnitTests/ApprovalFiles/SettingsTests.PlatformSampleSettings.approved.txt b/src/ServiceControl.Monitoring.UnitTests/ApprovalFiles/SettingsTests.PlatformSampleSettings.approved.txt index fed5a2acf4..a35b4112e2 100644 --- a/src/ServiceControl.Monitoring.UnitTests/ApprovalFiles/SettingsTests.PlatformSampleSettings.approved.txt +++ b/src/ServiceControl.Monitoring.UnitTests/ApprovalFiles/SettingsTests.PlatformSampleSettings.approved.txt @@ -12,6 +12,8 @@ "ValidateLifetime": true, "ValidateIssuerSigningKey": true, "RequireHttpsMetadata": true, + "SubjectIdClaim": "sub", + "SubjectNameClaim": "preferred_username", "ServicePulseAuthority": null, "ServicePulseClientId": null, "ServicePulseApiScopes": null, diff --git a/src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt b/src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt index 72e33e6946..6e7be8475f 100644 --- a/src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt +++ b/src/ServiceControl.UnitTests/ApprovalFiles/APIApprovals.PlatformSampleSettings.approved.txt @@ -12,6 +12,8 @@ "ValidateLifetime": true, "ValidateIssuerSigningKey": true, "RequireHttpsMetadata": true, + "SubjectIdClaim": "sub", + "SubjectNameClaim": "preferred_username", "ServicePulseAuthority": null, "ServicePulseClientId": null, "ServicePulseApiScopes": null,