diff --git a/src/ServiceControl.AcceptanceTesting/OpenIdConnect/OpenIdConnectTestConfiguration.cs b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/OpenIdConnectTestConfiguration.cs index 9e105a6983..fe330381db 100644 --- a/src/ServiceControl.AcceptanceTesting/OpenIdConnect/OpenIdConnectTestConfiguration.cs +++ b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/OpenIdConnectTestConfiguration.cs @@ -21,7 +21,7 @@ public class OpenIdConnectTestConfiguration(ServiceControlInstanceType instanceT /// public OpenIdConnectTestConfiguration WithAuthenticationEnabled() { - SetEnvironmentVariable("AUTHENTICATION_ENABLED", "true"); + SetEnvironmentVariable("AUTHENTICATION_ENABLED", bool.TrueString); return this; } @@ -30,19 +30,19 @@ public OpenIdConnectTestConfiguration WithAuthenticationEnabled() /// public OpenIdConnectTestConfiguration WithAuthenticationDisabled() { - SetEnvironmentVariable("AUTHENTICATION_ENABLED", "false"); + SetEnvironmentVariable("AUTHENTICATION_ENABLED", bool.FalseString); return this; } /// /// Enables role-based authorization. When on, controllers carrying /// [Authorize(Policy = Permissions.X)] require the caller's "roles" claim to map to a - /// role that grants the permission via RolePermissions. When off, the policy provider - /// returns allow-all policies and any authenticated request reaches the controller. + /// role that grants the permission via RolePermissions. When off, any authenticated + /// request reaches the controller. /// public OpenIdConnectTestConfiguration WithRoleBasedAuthorizationEnabled() { - SetEnvironmentVariable("AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", "true"); + SetEnvironmentVariable("AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", bool.TrueString); return this; } @@ -52,7 +52,7 @@ public OpenIdConnectTestConfiguration WithRoleBasedAuthorizationEnabled() /// public OpenIdConnectTestConfiguration WithConfigurationValidationDisabled() { - SetEnvironmentVariable("VALIDATECONFIG", "false"); + SetEnvironmentVariable("VALIDATECONFIG", bool.FalseString); return this; } diff --git a/src/ServiceControl.AcceptanceTesting/OpenIdConnect/ProtectedRoutes.cs b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/ProtectedRoutes.cs new file mode 100644 index 0000000000..425a9f63c0 --- /dev/null +++ b/src/ServiceControl.AcceptanceTesting/OpenIdConnect/ProtectedRoutes.cs @@ -0,0 +1,95 @@ +namespace ServiceControl.AcceptanceTesting.OpenIdConnect; + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Net; +using System.Net.Http; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.Routing; +using Microsoft.AspNetCore.Routing.Patterns; + +/// +/// Finds every route of a running instance that is not marked [AllowAnonymous], and sends each +/// one a request without a token. This covers every endpoint, so a new or changed authorization +/// policy cannot let anonymous callers in without a test failing. +/// +public static class ProtectedRoutes +{ + /// + /// Sends a request without a token to every protected route, and returns each route that did not + /// answer 401, as "METHOD /path → status". + /// + public static async Task> FindRoutesNotRejectingAnonymousRequests( + HttpClient client, + EndpointDataSource endpointDataSource, + CancellationToken cancellationToken = default) + { + var routes = endpointDataSource.Endpoints + .OfType() + .Where(endpoint => endpoint.Metadata.GetMetadata() is null) + .SelectMany(endpoint => (endpoint.Metadata.GetMetadata()?.HttpMethods ?? [HttpMethods.Get]) + .Select(method => (Method: method, Path: BuildPath(endpoint.RoutePattern)))) + .Distinct() + .ToList(); + + if (routes.Count == 0) + { + throw new InvalidOperationException("No protected routes found. The endpoint data source is empty or every route allows anonymous access."); + } + + var notRejected = new List(); + + foreach (var (method, path) in routes) + { + using var response = await OpenIdConnectAssertions.SendRequestWithoutAuth(client, new HttpMethod(method), path, cancellationToken); + + if (response.StatusCode != HttpStatusCode.Unauthorized) + { + notRejected.Add($"{method} {path} → {(int)response.StatusCode}"); + } + } + + return notRejected; + } + + // Fills each route parameter with a value that satisfies its constraint, so the request matches the + // route and reaches the authorization middleware instead of failing with 404. + static string BuildPath(RoutePattern pattern) + { + var segments = pattern.PathSegments.Select(segment => string.Concat(segment.Parts.Select(part => part switch + { + RoutePatternLiteralPart literal => literal.Content, + RoutePatternSeparatorPart separator => separator.Content, + RoutePatternParameterPart parameter => SampleValue(parameter), + _ => throw new NotSupportedException($"Unsupported route pattern part: {part.GetType().Name}") + }))); + + return "/" + string.Join('/', segments); + } + + static string SampleValue(RoutePatternParameterPart parameter) + { + var constraints = parameter.ParameterPolicies.Select(policy => policy.Content ?? string.Empty).ToArray(); + + if (constraints.Contains("guid", StringComparer.OrdinalIgnoreCase)) + { + return Guid.Empty.ToString(); + } + + if (constraints.Any(constraint => constraint is "int" or "long")) + { + return "1"; + } + + if (constraints.Contains("bool", StringComparer.OrdinalIgnoreCase)) + { + return bool.TrueString; + } + + return "x"; + } +} diff --git a/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs b/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs index 3795913478..a30eb6ba78 100644 --- a/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs +++ b/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs @@ -1,10 +1,13 @@ namespace ServiceControl.AcceptanceTests.Security.OpenIdConnect { + using System.Collections.Generic; using System.Net.Http; using System.Security.Claims; using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; using NServiceBus.AcceptanceTesting; using NUnit.Framework; @@ -235,6 +238,24 @@ public async Task Should_forbid_authenticated_user_lacking_required_permission() OpenIdConnectAssertions.AssertForbidden(response); } + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + class Context : ScenarioContext; } } diff --git a/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs b/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs new file mode 100644 index 0000000000..fd7ea5f96d --- /dev/null +++ b/src/ServiceControl.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs @@ -0,0 +1,90 @@ +namespace ServiceControl.AcceptanceTests.Security.OpenIdConnect +{ + using System.Collections.Generic; + using System.Net.Http; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + + /// + /// Authentication enabled, role-based authorization disabled (the default). Every route that is not + /// [AllowAnonymous] must still require a valid token. Roles are not checked, so a valid token without + /// roles is accepted. + /// + [NonParallelizable] + class When_authentication_is_enabled_without_role_based_authorization : AcceptanceTest + { + OpenIdConnectTestConfiguration configuration; + MockOidcServer mockOidcServer; + + const string TestAudience = "api://test-audience"; + + [SetUp] + public void ConfigureAuth() + { + mockOidcServer = new MockOidcServer(audience: TestAudience); + mockOidcServer.Start(); + + configuration = new OpenIdConnectTestConfiguration(ServiceControlInstanceType.Primary) + .WithConfigurationValidationDisabled() + .WithAuthenticationEnabled() + .WithAuthority(mockOidcServer.Authority) + .WithAudience(TestAudience) + .WithServicePulseClientId("test-client-id") + .WithServicePulseApiScopes("[\"api://test-audience/.default\"]") + .WithRequireHttpsMetadata(false); + } + + [TearDown] + public void CleanupAuth() + { + configuration?.Dispose(); + mockOidcServer?.Dispose(); + } + + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + + [Test] + public async Task Should_accept_token_without_roles() + { + HttpResponseMessage response = null; + + _ = await Define() + .Done(async ctx => + { + var tokenWithoutRoles = mockOidcServer.GenerateToken(); + response = await OpenIdConnectAssertions.SendRequestWithBearerToken( + HttpClient, + HttpMethod.Get, + "/api/errors", + tokenWithoutRoles); + return response != null; + }) + .Run(); + + OpenIdConnectAssertions.AssertAuthenticated(response); + } + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.AcceptanceTests/TestSupport/AcceptanceTest.cs b/src/ServiceControl.AcceptanceTests/TestSupport/AcceptanceTest.cs index a445576e0a..ba70a490bb 100644 --- a/src/ServiceControl.AcceptanceTests/TestSupport/AcceptanceTest.cs +++ b/src/ServiceControl.AcceptanceTests/TestSupport/AcceptanceTest.cs @@ -23,6 +23,7 @@ abstract class AcceptanceTest : NServiceBusAcceptanceTest, IAcceptanceTestInfras { public IDomainEvents DomainEvents => serviceControlRunnerBehavior.DomainEvents; public HttpClient HttpClient => serviceControlRunnerBehavior.HttpClient; + protected IServiceProvider ServiceProvider => serviceControlRunnerBehavior.ServiceProvider; public JsonSerializerOptions SerializerOptions => serviceControlRunnerBehavior.SerializerOptions; public Settings Settings => serviceControlRunnerBehavior.Settings; diff --git a/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs b/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs index b1c1663784..0dfe907c99 100644 --- a/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs +++ b/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs @@ -24,6 +24,7 @@ public ServiceControlComponentBehavior(ITransportIntegration transportToUse, IAc } public HttpClient HttpClient => runner.HttpClient; + public IServiceProvider ServiceProvider => runner.ServiceProvider; public JsonSerializerOptions SerializerOptions => runner.SerializerOptions; public Settings Settings => runner.Settings; public IDomainEvents DomainEvents => runner.DomainEvents; diff --git a/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs b/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs index b78c31a0da..f810c9acf0 100644 --- a/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs +++ b/src/ServiceControl.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs @@ -44,6 +44,7 @@ public ServiceControlComponentRunner(ITransportIntegration transportToUse, IAcce public override string Name { get; } = $"{nameof(ServiceControlComponentRunner)}"; public Settings Settings { get; private set; } public HttpClient HttpClient { get; private set; } + public IServiceProvider ServiceProvider { get; private set; } public JsonSerializerOptions SerializerOptions => Infrastructure.WebApi.SerializerOptions.Default; public IDomainEvents DomainEvents { get; private set; } @@ -152,6 +153,8 @@ async Task InitializeServiceControlCore(ScenarioContext context) await host.StartAsync(); DomainEvents = host.Services.GetRequiredService(); // Bring this back and look into the base address of the client + ServiceProvider = host.Services; + HttpClient = host.GetTestServer().CreateClient(); } } diff --git a/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs b/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs index a1e177076d..988c3f1513 100644 --- a/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs +++ b/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs @@ -1,10 +1,13 @@ namespace ServiceControl.Audit.AcceptanceTests.Security.OpenIdConnect { + using System.Collections.Generic; using System.Net.Http; using System.Security.Claims; using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; using NServiceBus.AcceptanceTesting; using NUnit.Framework; @@ -174,6 +177,24 @@ public async Task Should_reject_requests_with_wrong_issuer() OpenIdConnectAssertions.AssertUnauthorized(response); } + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + [Test] public async Task Should_allow_anonymous_access_to_root_endpoint() { diff --git a/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs b/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs new file mode 100644 index 0000000000..e9d25011ad --- /dev/null +++ b/src/ServiceControl.Audit.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs @@ -0,0 +1,88 @@ +namespace ServiceControl.Audit.AcceptanceTests.Security.OpenIdConnect +{ + using System.Collections.Generic; + using System.Net.Http; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + + /// + /// Authentication enabled, role-based authorization disabled (the default). Every route that is not + /// [AllowAnonymous] must still require a valid token. Roles are not checked, so a valid token without + /// roles is accepted. + /// + [NonParallelizable] + class When_authentication_is_enabled_without_role_based_authorization : AcceptanceTest + { + OpenIdConnectTestConfiguration configuration; + MockOidcServer mockOidcServer; + + const string TestAudience = "api://test-audience"; + + [SetUp] + public void ConfigureAuth() + { + mockOidcServer = new MockOidcServer(audience: TestAudience); + mockOidcServer.Start(); + + configuration = new OpenIdConnectTestConfiguration(ServiceControlInstanceType.Audit) + .WithConfigurationValidationDisabled() + .WithAuthenticationEnabled() + .WithAuthority(mockOidcServer.Authority) + .WithAudience(TestAudience) + .WithRequireHttpsMetadata(false); + } + + [TearDown] + public void CleanupAuth() + { + configuration?.Dispose(); + mockOidcServer?.Dispose(); + } + + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + + [Test] + public async Task Should_accept_token_without_roles() + { + HttpResponseMessage response = null; + + _ = await Define() + .Done(async ctx => + { + var tokenWithoutRoles = mockOidcServer.GenerateToken(); + response = await OpenIdConnectAssertions.SendRequestWithBearerToken( + HttpClient, + HttpMethod.Get, + "/api/messages", + tokenWithoutRoles); + return response != null; + }) + .Run(); + + OpenIdConnectAssertions.AssertAuthenticated(response); + } + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs b/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs index ebc000cf8b..cc93bfcee6 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionAuthorizationExtensions.cs @@ -11,12 +11,14 @@ namespace ServiceControl.Hosting.Auth; /// /// Registers the permission-based policy authorization services: a dynamic /// that resolves [Authorize(Policy = "<permission>")] -/// attributes, and — when OIDC is enabled — the that evaluates them -/// against the user's roles. +/// attributes, and the that evaluates them against the user's roles +/// when role-based authorization is enabled. /// /// The provider is registered unconditionally so the policy attributes resolve in every configuration -/// (without it, annotated endpoints fail with "AuthorizationPolicy not found"). When OIDC is disabled the -/// provider returns allow-all policies that carry no requirement, so the verb handler is not registered. +/// (without it, annotated endpoints fail with "AuthorizationPolicy not found"). When authentication is +/// disabled the provider returns allow-all policies; when authentication is enabled without role-based +/// authorization it returns policies that only require an authenticated user. Neither carries a +/// , so the verb handler is not reached in those configurations. /// Wire this into every instance that hosts annotated controllers (Error, Audit, Monitoring). /// /// @@ -36,9 +38,9 @@ public static void AddServiceControlAuthorization(this IHostApplicationBuilder h // The policy provider is registered UNCONDITIONALLY: every instance hosts controllers with // [Authorize(Policy = Permissions.X)] attributes, and without a provider that knows those // policy names ASP.NET throws "AuthorizationPolicy named '...' was not found" → 500 on every - // request to an annotated endpoint. When RBAC is disabled the provider returns allow-all - // policies (no requirement), so anonymous-to-the-policy calls pass through and the verb - // handler is unnecessary. + // request to an annotated endpoint. When RBAC is disabled the provider returns policies + // without a PermissionRequirement (allow-all when authentication is disabled, authenticated + // user only when it is enabled), so the verb handler is unnecessary. services.AddSingleton(); // The provider only emits a PermissionRequirement when RBAC is enabled, so the handler is the diff --git a/src/ServiceControl.Hosting/Auth/PermissionPolicyProvider.cs b/src/ServiceControl.Hosting/Auth/PermissionPolicyProvider.cs index 55b7caea56..329b32b6c7 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionPolicyProvider.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionPolicyProvider.cs @@ -21,9 +21,18 @@ namespace ServiceControl.Hosting.Auth; /// immutable, so the prebuilt instances are safely shared across all requests.) /// /// -/// When OIDC is enabled each permission maps to a policy carrying a -/// (evaluated by ). When OIDC is disabled the platform runs -/// unauthenticated, so every permission maps to a shared allow-all policy — no requirement, no handler. +/// The policy each permission maps to depends on both authentication settings: +/// +/// Authentication disabled: the platform runs unauthenticated, so every permission maps to a +/// shared allow-all policy (no requirement, no handler). Role-based authorization is ignored. +/// Authentication enabled, role-based authorization disabled: every permission maps to a shared +/// policy that only requires an authenticated user. +/// Authentication and role-based authorization enabled: each permission maps to a policy that +/// requires an authenticated user and carries a (evaluated by +/// ). +/// +/// Every controller action carries a permission policy, so the fallback policy never applies to them. +/// These policies must therefore enforce authentication themselves whenever it is enabled. /// Unknown policy names resolve to ; the default and fallback policies are /// delegated to the configured . /// @@ -35,23 +44,39 @@ public sealed class PermissionPolicyProvider(IOptions auth static readonly AuthorizationPolicy AllowAll = new AuthorizationPolicyBuilder().RequireAssertion(_ => true).Build(); + static readonly AuthorizationPolicy AuthenticatedOnly = + new AuthorizationPolicyBuilder().RequireAuthenticatedUser().Build(); + readonly FrozenDictionary policies = BuildPolicies(oidcSettings); static FrozenDictionary BuildPolicies(OpenIdConnectSettings oidcSettings) => Permissions.All.ToFrozenDictionary( permission => permission, - permission => oidcSettings.RoleBasedAuthorizationEnabled - ? new AuthorizationPolicyBuilder() - // RequireAuthenticatedUser() must come first so an unauthenticated request fails as - // FailedAuthentication (→ 401 challenge) rather than FailedRequirements (→ 403 - // forbid). Without it, PermissionVerbHandler is reached for anonymous callers and a - // missing-roles outcome is classified as a forbidden permission failure. - .RequireAuthenticatedUser() - .AddRequirements(new PermissionRequirement(permission)) - .Build() - : AllowAll, + permission => BuildPolicy(permission, oidcSettings), StringComparer.Ordinal); + static AuthorizationPolicy BuildPolicy(string permission, OpenIdConnectSettings oidcSettings) + { + if (!oidcSettings.Enabled) + { + return AllowAll; + } + + if (!oidcSettings.RoleBasedAuthorizationEnabled) + { + return AuthenticatedOnly; + } + + return new AuthorizationPolicyBuilder() + // RequireAuthenticatedUser() must come first so an unauthenticated request fails as + // FailedAuthentication (→ 401 challenge) rather than FailedRequirements (→ 403 + // forbid). Without it, PermissionVerbHandler is reached for anonymous callers and a + // missing-roles outcome is classified as a forbidden permission failure. + .RequireAuthenticatedUser() + .AddRequirements(new PermissionRequirement(permission)) + .Build(); + } + #pragma warning disable PS0018 // IAuthorizationPolicyProvider declares these without a CancellationToken public Task GetPolicyAsync(string policyName) => Task.FromResult(policies.GetValueOrDefault(policyName)); diff --git a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs index be007c2cfe..de99974652 100644 --- a/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs +++ b/src/ServiceControl.Hosting/Auth/PermissionVerbHandler.cs @@ -15,8 +15,8 @@ namespace ServiceControl.Hosting.Auth; /// 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 +/// Only reached when authentication and role-based authorization are both enabled. Otherwise +/// returns policies that carry no /// , so this handler is not needed. /// /// diff --git a/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs b/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs new file mode 100644 index 0000000000..c6c6aaeb59 --- /dev/null +++ b/src/ServiceControl.Infrastructure.Tests/Auth/PermissionPolicyProviderTests.cs @@ -0,0 +1,129 @@ +#nullable enable +namespace ServiceControl.Infrastructure.Tests.Auth; + +using System; +using System.Collections.Generic; +using System.Linq; +using System.Security.Claims; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.AspNetCore.Authorization; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using NUnit.Framework; +using ServiceControl.Configuration; +using ServiceControl.Hosting.Auth; +using ServiceControl.Infrastructure; +using ServiceControl.Infrastructure.Auth; + +/// +/// Evaluates every permission policy through the same registrations the instances use, for each +/// combination of Authentication.Enabled and Authentication.RoleBasedAuthorizationEnabled. +/// Every controller action carries a permission policy, so the fallback policy never applies to them: +/// the permission policies alone decide whether an anonymous caller gets in. +/// +// The settings under test are process-wide environment variables, so these cannot run beside +// each other: one test's setup overwrites what another is about to read. +[TestFixture] +[NonParallelizable] +class PermissionPolicyProviderTests +{ + [TearDown] + public void TearDown() + { + Environment.SetEnvironmentVariable("SERVICECONTROL_AUTHENTICATION_ENABLED", null); + Environment.SetEnvironmentVariable("SERVICECONTROL_AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", null); + } + + [TestCase(false)] + [TestCase(true)] + public async Task Anonymous_callers_are_denied_every_permission_when_authentication_is_enabled(bool roleBasedAuthorizationEnabled) + { + using var host = BuildHost(authenticationEnabled: true, roleBasedAuthorizationEnabled); + + Assert.That(await AllowedPermissions(host, Anonymous, TestContext.CurrentContext.CancellationToken), Is.Empty); + } + + [TestCase(false)] + [TestCase(true)] + public async Task Anonymous_callers_are_allowed_every_permission_when_authentication_is_disabled(bool roleBasedAuthorizationEnabled) + { + using var host = BuildHost(authenticationEnabled: false, roleBasedAuthorizationEnabled); + + Assert.That(await DeniedPermissions(host, Anonymous, TestContext.CurrentContext.CancellationToken), Is.Empty); + } + + [Test] + public async Task Authenticated_callers_without_a_role_are_allowed_every_permission_when_role_based_authorization_is_disabled() + { + using var host = BuildHost(authenticationEnabled: true, roleBasedAuthorizationEnabled: false); + + Assert.That(await DeniedPermissions(host, Authenticated(), TestContext.CurrentContext.CancellationToken), Is.Empty); + } + + [Test] + public async Task Authenticated_callers_without_a_role_are_denied_every_permission_when_role_based_authorization_is_enabled() + { + using var host = BuildHost(authenticationEnabled: true, roleBasedAuthorizationEnabled: true); + + Assert.That(await AllowedPermissions(host, Authenticated(), TestContext.CurrentContext.CancellationToken), Is.Empty); + } + + [Test] + public async Task Admins_are_allowed_every_permission_when_role_based_authorization_is_enabled() + { + using var host = BuildHost(authenticationEnabled: true, roleBasedAuthorizationEnabled: true); + + Assert.That(await DeniedPermissions(host, Authenticated(RolePermissions.Admin), TestContext.CurrentContext.CancellationToken), Is.Empty); + } + + static readonly ClaimsPrincipal Anonymous = new(new ClaimsIdentity()); + + static ClaimsPrincipal Authenticated(params string[] roles) => + new(new ClaimsIdentity( + [ + new Claim("sub", "alice-sub"), + new Claim("preferred_username", "Alice"), + .. roles.Select(role => new Claim(ClaimTypes.Role, role)) + ], + authenticationType: "test")); + + static IHost BuildHost(bool authenticationEnabled, bool roleBasedAuthorizationEnabled) + { + Environment.SetEnvironmentVariable("SERVICECONTROL_AUTHENTICATION_ENABLED", authenticationEnabled ? bool.TrueString : bool.FalseString); + Environment.SetEnvironmentVariable("SERVICECONTROL_AUTHENTICATION_ROLEBASEDAUTHORIZATIONENABLED", roleBasedAuthorizationEnabled ? bool.TrueString : bool.FalseString); + + var settings = new OpenIdConnectSettings(new SettingsRootNamespace("ServiceControl"), validateConfiguration: false, requireServicePulseSettings: false); + + var builder = Host.CreateEmptyApplicationBuilder(new HostApplicationBuilderSettings()); + builder.Services.AddLogging(); + builder.AddServiceControlAuthentication(settings); + builder.AddServiceControlAuthorization(settings); + return builder.Build(); + } + + static Task AllowedPermissions(IHost host, ClaimsPrincipal user, CancellationToken cancellationToken) => + PermissionsWhere(host, user, succeeded: true, cancellationToken); + + static Task DeniedPermissions(IHost host, ClaimsPrincipal user, CancellationToken cancellationToken) => + PermissionsWhere(host, user, succeeded: false, cancellationToken); + + static async Task PermissionsWhere(IHost host, ClaimsPrincipal user, bool succeeded, CancellationToken cancellationToken) + { + var authorizationService = host.Services.GetRequiredService(); + var matches = new List(); + + foreach (var permission in Permissions.All.Order()) + { + cancellationToken.ThrowIfCancellationRequested(); + + var result = await authorizationService.AuthorizeAsync(user, resource: null, permission); + if (result.Succeeded == succeeded) + { + matches.Add(permission); + } + } + + return [.. matches]; + } +} diff --git a/src/ServiceControl.Infrastructure/Auth/EffectivePermissions.cs b/src/ServiceControl.Infrastructure/Auth/EffectivePermissions.cs index 7454a6516f..b6ceffb7d5 100644 --- a/src/ServiceControl.Infrastructure/Auth/EffectivePermissions.cs +++ b/src/ServiceControl.Infrastructure/Auth/EffectivePermissions.cs @@ -9,7 +9,7 @@ namespace ServiceControl.Infrastructure.Auth; /// The set of permissions a principal effectively holds, computed per request. Mirrors the inputs the /// enforcement handler uses: when role-based authorization is enabled, the union of the permissions /// granted by the principal's claims (via ); -/// when it is disabled the platform runs allow-all, so every known permission is held. +/// when it is disabled every caller that passes authentication holds every known permission. /// public static class EffectivePermissions { diff --git a/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs b/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs index 81f6080836..18e8ce861f 100644 --- a/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs +++ b/src/ServiceControl.Infrastructure/OpenIdConnectSettings.cs @@ -303,8 +303,8 @@ void LogConfiguration(bool requireServicePulseSettings) var servicePulseApiScopesDisplay = requireServicePulseSettings ? (ServicePulseApiScopes ?? "(not configured)") : "(n/a)"; var servicePulseOfflineAccessScopeEnabledDisplay = requireServicePulseSettings ? ServicePulseOfflineAccessScopeEnabled.ToString() : "(n/a)"; - 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}, ServicePulseOfflineAccessScopeEnabled={ServicePulseOfflineAccessScopeEnabled}", - Enabled, authorityDisplay, audienceDisplay, ValidateIssuer, ValidateAudience, ValidateLifetime, ValidateIssuerSigningKey, RequireHttpsMetadata, RolesClaim, SubjectIdClaim, SubjectNameClaim, servicePulseClientIdDisplay, servicePulseAuthorityDisplay, servicePulseApiScopesDisplay, servicePulseOfflineAccessScopeEnabledDisplay); + logger.LogInformation("Authentication settings: Enabled={Enabled}, RoleBasedAuthorizationEnabled={RoleBasedAuthorizationEnabled}, 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}, ServicePulseOfflineAccessScopeEnabled={ServicePulseOfflineAccessScopeEnabled}", + Enabled, RoleBasedAuthorizationEnabled, authorityDisplay, audienceDisplay, ValidateIssuer, ValidateAudience, ValidateLifetime, ValidateIssuerSigningKey, RequireHttpsMetadata, RolesClaim, SubjectIdClaim, SubjectNameClaim, servicePulseClientIdDisplay, servicePulseAuthorityDisplay, servicePulseApiScopesDisplay, servicePulseOfflineAccessScopeEnabledDisplay); // Warn about potential misconfigurations var hasAuthConfig = !string.IsNullOrWhiteSpace(Authority) || !string.IsNullOrWhiteSpace(Audience); diff --git a/src/ServiceControl.Monitoring.AcceptanceTests/AcceptanceTest.cs b/src/ServiceControl.Monitoring.AcceptanceTests/AcceptanceTest.cs index aaf1849fd2..ce6016fbb2 100644 --- a/src/ServiceControl.Monitoring.AcceptanceTests/AcceptanceTest.cs +++ b/src/ServiceControl.Monitoring.AcceptanceTests/AcceptanceTest.cs @@ -18,6 +18,7 @@ namespace ServiceControl.Monitoring.AcceptanceTests abstract class AcceptanceTest : NServiceBusAcceptanceTest, IAcceptanceTestInfrastructureProvider { public HttpClient HttpClient => serviceControlRunnerBehavior.HttpClient; + protected IServiceProvider ServiceProvider => serviceControlRunnerBehavior.ServiceProvider; public JsonSerializerOptions SerializerOptions => serviceControlRunnerBehavior.SerializerOptions; [SetUp] diff --git a/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs b/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs index 9be7d0160f..0f9a551520 100644 --- a/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs +++ b/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled.cs @@ -1,10 +1,13 @@ namespace ServiceControl.Monitoring.AcceptanceTests.Security.OpenIdConnect { + using System.Collections.Generic; using System.Net.Http; using System.Security.Claims; using System.Threading.Tasks; using AcceptanceTesting; using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; using NServiceBus.AcceptanceTesting; using NUnit.Framework; @@ -175,6 +178,24 @@ public async Task Should_reject_requests_with_wrong_issuer() OpenIdConnectAssertions.AssertUnauthorized(response); } + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + [Test] public async Task Should_allow_anonymous_access_to_root_endpoint() { diff --git a/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs b/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs new file mode 100644 index 0000000000..a56760e78e --- /dev/null +++ b/src/ServiceControl.Monitoring.AcceptanceTests/Security/OpenIdConnect/When_authentication_is_enabled_without_role_based_authorization.cs @@ -0,0 +1,88 @@ +namespace ServiceControl.Monitoring.AcceptanceTests.Security.OpenIdConnect +{ + using System.Collections.Generic; + using System.Net.Http; + using System.Threading.Tasks; + using AcceptanceTesting; + using AcceptanceTesting.OpenIdConnect; + using Microsoft.AspNetCore.Routing; + using Microsoft.Extensions.DependencyInjection; + using NServiceBus.AcceptanceTesting; + using NUnit.Framework; + + /// + /// Authentication enabled, role-based authorization disabled (the default). Every route that is not + /// [AllowAnonymous] must still require a valid token. Roles are not checked, so a valid token without + /// roles is accepted. + /// + [NonParallelizable] + class When_authentication_is_enabled_without_role_based_authorization : AcceptanceTest + { + OpenIdConnectTestConfiguration configuration; + MockOidcServer mockOidcServer; + + const string TestAudience = "api://test-audience"; + + [SetUp] + public void ConfigureAuth() + { + mockOidcServer = new MockOidcServer(audience: TestAudience); + mockOidcServer.Start(); + + configuration = new OpenIdConnectTestConfiguration(ServiceControlInstanceType.Monitoring) + .WithConfigurationValidationDisabled() + .WithAuthenticationEnabled() + .WithAuthority(mockOidcServer.Authority) + .WithAudience(TestAudience) + .WithRequireHttpsMetadata(false); + } + + [TearDown] + public void CleanupAuth() + { + configuration?.Dispose(); + mockOidcServer?.Dispose(); + } + + [Test] + public async Task Should_reject_anonymous_requests_on_every_protected_route() + { + IReadOnlyList notRejected = null; + + _ = await Define() + .Done(async ctx => + { + notRejected = await ProtectedRoutes.FindRoutesNotRejectingAnonymousRequests( + HttpClient, + ServiceProvider.GetRequiredService()); + return true; + }) + .Run(); + + Assert.That(notRejected, Is.Empty, "Protected routes that did not answer 401 to a request without a token"); + } + + [Test] + public async Task Should_accept_token_without_roles() + { + HttpResponseMessage response = null; + + _ = await Define() + .Done(async ctx => + { + var tokenWithoutRoles = mockOidcServer.GenerateToken(); + response = await OpenIdConnectAssertions.SendRequestWithBearerToken( + HttpClient, + HttpMethod.Get, + "/monitored-endpoints", + tokenWithoutRoles); + return response != null; + }) + .Run(); + + OpenIdConnectAssertions.AssertAuthenticated(response); + } + + class Context : ScenarioContext; + } +} diff --git a/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs b/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs index b8e0c97315..5b80b2fdec 100644 --- a/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs +++ b/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentBehavior.cs @@ -19,6 +19,7 @@ public ServiceControlComponentBehavior(ITransportIntegration transportToUse, Act } public HttpClient HttpClient => runner.HttpClient; + public IServiceProvider ServiceProvider => runner.ServiceProvider; public JsonSerializerOptions SerializerOptions => runner.SerializerOptions; #pragma warning disable PS0018 // IComponentBehavior declares this without a CancellationToken diff --git a/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs b/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs index eaf799522f..238b2cda36 100644 --- a/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs +++ b/src/ServiceControl.Monitoring.AcceptanceTests/TestSupport/ServiceControlComponentRunner.cs @@ -32,6 +32,7 @@ class ServiceControlComponentRunner( { public override string Name { get; } = $"{nameof(ServiceControlComponentRunner)}"; public HttpClient HttpClient { get; private set; } + public IServiceProvider ServiceProvider { get; private set; } public JsonSerializerOptions SerializerOptions => Infrastructure.SerializerOptions.Default; public Task Initialize(RunDescriptor run, CancellationToken cancellationToken = default) => InitializeServiceControl(run.ScenarioContext, cancellationToken); @@ -129,6 +130,8 @@ async Task InitializeServiceControl(ScenarioContext context, CancellationToken c host.UseServiceControlMonitoring(settings.ForwardedHeadersSettings, settings.HttpsSettings, settings.CorsSettings); await host.StartAsync(cancellationToken); + ServiceProvider = host.Services; + HttpClient = host.Services.GetRequiredKeyedService(settings.InstanceName).CreateClient(); } } diff --git a/src/ServiceControl.Monitoring/Dockerfile b/src/ServiceControl.Monitoring/Dockerfile index 4b9fe10087..f970d8981c 100644 --- a/src/ServiceControl.Monitoring/Dockerfile +++ b/src/ServiceControl.Monitoring/Dockerfile @@ -19,7 +19,7 @@ EXPOSE 33633 COPY --from=build /deploy/Particular.ServiceControl.Monitoring /app COPY --from=build /healthcheck /healthcheck -HEALTHCHECK --start-period=10s CMD ["/healthcheck/healthcheck", "http://localhost:33633/connection"] +HEALTHCHECK --start-period=10s CMD ["/healthcheck/healthcheck", "http://localhost:33633/"] USER $APP_UID ENTRYPOINT ["/app/ServiceControl.Monitoring"] \ No newline at end of file