From ac34dac47989fc23b3d3fed52d44800a8958a559 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Tue, 2 Jun 2026 01:04:34 -0400 Subject: [PATCH] feat(auth): cut ScadaBridge over to ZB.MOM.WW.Auth.Ldap; nest+rename Ldap config; roles+sitescope via IGroupRoleMapper (Task 1.2/1.4) --- .../central-node-a/appsettings.Central.json | 16 +- .../central-node-b/appsettings.Central.json | 16 +- .../central-node-a/appsettings.Central.json | 16 +- .../central-node-b/appsettings.Central.json | 16 +- .../Auth/AuthEndpoints.cs | 65 +- src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 2 +- .../StartupValidator.cs | 10 +- .../appsettings.Central.json | 18 +- .../AuditEndpoints.cs | 16 +- .../DebugStreamHub.cs | 10 +- .../ManagementEndpoints.cs | 16 +- .../LdapAuthFailureMessages.cs | 87 +++ .../LdapAuthResult.cs | 8 - .../LdapAuthService.cs | 417 ------------ .../LdapTransport.cs | 26 - .../SecurityOptions.cs | 88 +-- .../SecurityOptionsValidator.cs | 56 -- .../ServiceAccountBindException.cs | 15 - .../ServiceCollectionExtensions.cs | 46 +- .../ZB.MOM.WW.ScadaBridge.Security.csproj | 2 + .../ActorPathTests.cs | 14 +- .../AkkaHostedServiceAuditWiringTests.cs | 14 +- .../CompositionRootTests.cs | 21 +- .../StartupValidatorTests.cs | 8 +- .../ScadaBridgeWebApplicationFactory.cs | 24 +- .../SecurityHardeningTests.cs | 37 +- .../StartupValidationTests.cs | 14 +- .../AuditEndpointsTests.cs | 11 +- .../ScadaBridgeGroupRoleMapperTests.cs | 43 ++ .../SecurityTests.cs | 644 +++++++----------- ...B.MOM.WW.ScadaBridge.Security.Tests.csproj | 3 + 31 files changed, 647 insertions(+), 1132 deletions(-) create mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthFailureMessages.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthResult.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthService.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/LdapTransport.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptionsValidator.cs delete mode 100644 src/ZB.MOM.WW.ScadaBridge.Security/ServiceAccountBindException.cs diff --git a/docker-env2/central-node-a/appsettings.Central.json b/docker-env2/central-node-a/appsettings.Central.json index 9fcff378..dc4a8ca4 100644 --- a/docker-env2/central-node-a/appsettings.Central.json +++ b/docker-env2/central-node-a/appsettings.Central.json @@ -22,13 +22,15 @@ "MachineDataDb": "Server=scadabridge-mssql,1433;Database=ScadaBridgeMachineData2;User Id=scadabridge_app;Password=ScadaBridge_Dev1#;TrustServerCertificate=true" }, "Security": { - "LdapServer": "scadabridge-ldap", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "password", + "Ldap": { + "Server": "scadabridge-ldap", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "password" + }, "JwtSigningKey": "scadabridge-env2-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, diff --git a/docker-env2/central-node-b/appsettings.Central.json b/docker-env2/central-node-b/appsettings.Central.json index c73b218d..761c0974 100644 --- a/docker-env2/central-node-b/appsettings.Central.json +++ b/docker-env2/central-node-b/appsettings.Central.json @@ -22,13 +22,15 @@ "MachineDataDb": "Server=scadabridge-mssql,1433;Database=ScadaBridgeMachineData2;User Id=scadabridge_app;Password=ScadaBridge_Dev1#;TrustServerCertificate=true" }, "Security": { - "LdapServer": "scadabridge-ldap", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "password", + "Ldap": { + "Server": "scadabridge-ldap", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "password" + }, "JwtSigningKey": "scadabridge-env2-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, diff --git a/docker/central-node-a/appsettings.Central.json b/docker/central-node-a/appsettings.Central.json index 125f11c6..c8f0a1a4 100644 --- a/docker/central-node-a/appsettings.Central.json +++ b/docker/central-node-a/appsettings.Central.json @@ -22,13 +22,15 @@ "MachineDataDb": "Server=scadabridge-mssql,1433;Database=ScadaBridgeMachineData;User Id=scadabridge_app;Password=ScadaBridge_Dev1#;TrustServerCertificate=true" }, "Security": { - "LdapServer": "scadabridge-ldap", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "password", + "Ldap": { + "Server": "scadabridge-ldap", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "password" + }, "JwtSigningKey": "scadabridge-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, diff --git a/docker/central-node-b/appsettings.Central.json b/docker/central-node-b/appsettings.Central.json index 2acd4219..41a89814 100644 --- a/docker/central-node-b/appsettings.Central.json +++ b/docker/central-node-b/appsettings.Central.json @@ -22,13 +22,15 @@ "MachineDataDb": "Server=scadabridge-mssql,1433;Database=ScadaBridgeMachineData;User Id=scadabridge_app;Password=ScadaBridge_Dev1#;TrustServerCertificate=true" }, "Security": { - "LdapServer": "scadabridge-ldap", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "password", + "Ldap": { + "Server": "scadabridge-ldap", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "password" + }, "JwtSigningKey": "scadabridge-dev-jwt-signing-key-must-be-at-least-32-characters-long", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30, diff --git a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs index 815870e2..60173be2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.CentralUI/Auth/AuthEndpoints.cs @@ -5,6 +5,8 @@ using Microsoft.AspNetCore.Builder; using Microsoft.AspNetCore.Http; using Microsoft.AspNetCore.Routing; using Microsoft.Extensions.DependencyInjection; +using ZB.MOM.WW.Auth.Abstractions.Ldap; +using ZB.MOM.WW.Auth.Abstractions.Roles; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.CentralUI.Auth; @@ -31,20 +33,25 @@ public static class AuthEndpoints return; } - var ldapAuth = context.RequestServices.GetRequiredService(); + var ldapAuth = context.RequestServices.GetRequiredService(); var jwtService = context.RequestServices.GetRequiredService(); - var roleMapper = context.RequestServices.GetRequiredService(); + var roleMapper = context.RequestServices.GetRequiredService>(); - var authResult = await ldapAuth.AuthenticateAsync(username, password); - if (!authResult.Success) + var authResult = await ldapAuth.AuthenticateAsync(username, password, context.RequestAborted); + if (!authResult.Succeeded) { - var errorMsg = Uri.EscapeDataString(authResult.ErrorMessage ?? "Authentication failed."); + var errorMsg = Uri.EscapeDataString(LdapAuthFailureMessages.ToMessage(authResult.Failure)); context.Response.Redirect($"/login?error={errorMsg}"); return; } - // Map LDAP groups to roles - var roleMappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups ?? []); + // Map LDAP groups to roles via the shared IGroupRoleMapper seam + // (Task 1.1 ScadaBridgeGroupRoleMapper, wrapping the DB-backed RoleMapper). + // The full RoleMappingResult — including PermittedSiteIds and the + // system-wide flag — is carried in the mapping's opaque Scope so the + // site-scope→SiteId claims below are built exactly as before. + var roleMapping = await roleMapper.MapAsync(authResult.Groups, context.RequestAborted); + var scope = (RoleMappingResult)roleMapping.Scope!; // Build claims from LDAP auth + role mapping. // CentralUI-005: no fixed "expires_at" absolute-cap claim is stamped @@ -52,21 +59,23 @@ public static class AuthEndpoints // (ZB.MOM.WW.ScadaBridge.Security AddCookie: ExpireTimeSpan = idle timeout, // SlidingExpiration = true). A frozen absolute claim would contradict // the documented sliding-refresh policy. + var displayName = string.IsNullOrEmpty(authResult.DisplayName) ? username : authResult.DisplayName; + var resolvedUsername = string.IsNullOrEmpty(authResult.Username) ? username : authResult.Username; var claims = new List { - new(ClaimTypes.Name, authResult.Username ?? username), - new(JwtTokenService.DisplayNameClaimType, authResult.DisplayName ?? username), - new(JwtTokenService.UsernameClaimType, authResult.Username ?? username), + new(ClaimTypes.Name, resolvedUsername), + new(JwtTokenService.DisplayNameClaimType, displayName), + new(JwtTokenService.UsernameClaimType, resolvedUsername), }; - foreach (var role in roleMappingResult.Roles) + foreach (var role in roleMapping.Roles) { claims.Add(new Claim(JwtTokenService.RoleClaimType, role)); } - if (!roleMappingResult.IsSystemWideDeployment) + if (!scope.IsSystemWideDeployment) { - foreach (var siteId in roleMappingResult.PermittedSiteIds) + foreach (var siteId in scope.PermittedSiteIds) { claims.Add(new Claim(JwtTokenService.SiteIdClaimType, siteId)); } @@ -94,33 +103,37 @@ public static class AuthEndpoints return Results.Json(new { error = "Username and password are required." }, statusCode: 400); } - var ldapAuth = context.RequestServices.GetRequiredService(); + var ldapAuth = context.RequestServices.GetRequiredService(); var jwtService = context.RequestServices.GetRequiredService(); - var roleMapper = context.RequestServices.GetRequiredService(); + var roleMapper = context.RequestServices.GetRequiredService>(); - var authResult = await ldapAuth.AuthenticateAsync(username, password); - if (!authResult.Success) + var authResult = await ldapAuth.AuthenticateAsync(username, password, context.RequestAborted); + if (!authResult.Succeeded) { return Results.Json( - new { error = authResult.ErrorMessage ?? "Authentication failed." }, + new { error = LdapAuthFailureMessages.ToMessage(authResult.Failure) }, statusCode: 401); } - var roleMappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups ?? []); + var roleMapping = await roleMapper.MapAsync(authResult.Groups, context.RequestAborted); + var scope = (RoleMappingResult)roleMapping.Scope!; + + var displayName = string.IsNullOrEmpty(authResult.DisplayName) ? username : authResult.DisplayName; + var resolvedUsername = string.IsNullOrEmpty(authResult.Username) ? username : authResult.Username; var token = jwtService.GenerateToken( - authResult.DisplayName ?? username, - authResult.Username ?? username, - roleMappingResult.Roles, - roleMappingResult.IsSystemWideDeployment ? null : roleMappingResult.PermittedSiteIds); + displayName, + resolvedUsername, + roleMapping.Roles, + scope.IsSystemWideDeployment ? null : scope.PermittedSiteIds); return Results.Json(new { access_token = token, token_type = "Bearer", - username = authResult.Username ?? username, - display_name = authResult.DisplayName ?? username, - roles = roleMappingResult.Roles, + username = resolvedUsername, + display_name = displayName, + roles = roleMapping.Roles, }); }).DisableAntiforgery(); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index 43d32967..deedecf9 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -103,7 +103,7 @@ try builder.Services.AddSiteCallAudit(); builder.Services.AddTemplateEngine(); builder.Services.AddDeploymentManager(); - builder.Services.AddSecurity(); + builder.Services.AddSecurity(builder.Configuration); builder.Services.AddCentralUI(); builder.Services.AddInboundAPI(); builder.Services.AddManagementService(); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index f7ab3ed6..b60c5617 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -60,8 +60,14 @@ public static class StartupValidator .Require("ScadaBridge:Database:ConfigurationDb", _ => !string.IsNullOrEmpty(configuration.GetSection("ScadaBridge:Database")["ConfigurationDb"]), "connection string required for Central") - .Require("ScadaBridge:Security:LdapServer", - _ => !string.IsNullOrEmpty(configuration.GetSection("ScadaBridge:Security")["LdapServer"]), + // Task 1.4: the LDAP server key moved into the nested Security:Ldap + // sub-section (bound to the shared LdapOptions). Validate the nested key so + // the pre-host preflight still fails fast on a missing LDAP server for + // Central. The full LDAP option set (SearchBase / ServiceAccountDn / + // transport) is additionally validated post-host by the shared + // LdapOptionsValidator (registered with ValidateOnStart by AddZbLdapAuth). + .Require("ScadaBridge:Security:Ldap:Server", + _ => !string.IsNullOrEmpty(configuration.GetSection("ScadaBridge:Security:Ldap")["Server"]), "required for Central") .Require("ScadaBridge:Security:JwtSigningKey", _ => !string.IsNullOrEmpty(configuration.GetSection("ScadaBridge:Security")["JwtSigningKey"]), diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Central.json b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Central.json index c53ec978..b0efdbd2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Central.json +++ b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Central.json @@ -18,18 +18,20 @@ "FailureDetectionThreshold": "00:00:10", "MinNrOfMembers": 1 }, - "_secrets": "Host-003: Secrets are NOT committed in this file. Supply them via environment variables, which the Host's configuration builder (AddEnvironmentVariables) overlays over this file. Required: ScadaBridge__Database__ConfigurationDb, ScadaBridge__Security__LdapServiceAccountPassword, ScadaBridge__Security__JwtSigningKey. The ${...} placeholders below are intentionally non-functional and must be overridden per environment.", + "_secrets": "Host-003: Secrets are NOT committed in this file. Supply them via environment variables, which the Host's configuration builder (AddEnvironmentVariables) overlays over this file. Required: ScadaBridge__Database__ConfigurationDb, ScadaBridge__Security__Ldap__ServiceAccountPassword, ScadaBridge__Security__JwtSigningKey. The ${...} placeholders below are intentionally non-functional and must be overridden per environment. NOTE (Task 1.4): the LDAP settings moved into the nested Security:Ldap sub-section (bound to the shared ZB.MOM.WW.Auth LdapOptions) — the service-account-password env var is now ScadaBridge__Security__Ldap__ServiceAccountPassword (was ScadaBridge__Security__LdapServiceAccountPassword).", "Database": { "ConfigurationDb": "${SCADABRIDGE_CONFIGURATIONDB_CONNECTION_STRING}" }, "Security": { - "LdapServer": "localhost", - "LdapPort": 3893, - "LdapUseTls": false, - "AllowInsecureLdap": true, - "LdapSearchBase": "dc=scadabridge,dc=local", - "LdapServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", - "LdapServiceAccountPassword": "${SCADABRIDGE_LDAP_SERVICE_ACCOUNT_PASSWORD}", + "Ldap": { + "Server": "localhost", + "Port": 3893, + "Transport": "None", + "AllowInsecure": true, + "SearchBase": "dc=scadabridge,dc=local", + "ServiceAccountDn": "cn=admin,dc=scadabridge,dc=local", + "ServiceAccountPassword": "${SCADABRIDGE_LDAP_SERVICE_ACCOUNT_PASSWORD}" + }, "JwtSigningKey": "${SCADABRIDGE_JWT_SIGNING_KEY}", "JwtExpiryMinutes": 15, "IdleTimeoutMinutes": 30 diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs index 2d68087c..b5476482 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/AuditEndpoints.cs @@ -11,6 +11,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.ManagementService; @@ -355,25 +356,24 @@ public static class AuditEndpoints new { error = "Username and password are required.", code = "AUTH_FAILED" }, statusCode: 401)); } - var ldapAuth = context.RequestServices.GetRequiredService(); - var authResult = await ldapAuth.AuthenticateAsync(username, password); - if (!authResult.Success) + var ldapAuth = context.RequestServices.GetRequiredService(); + var authResult = await ldapAuth.AuthenticateAsync(username, password, context.RequestAborted); + if (!authResult.Succeeded) { return new AuthOutcome(null, Results.Json( - new { error = authResult.ErrorMessage ?? "Authentication failed.", code = "AUTH_FAILED" }, statusCode: 401)); + new { error = LdapAuthFailureMessages.ToMessage(authResult.Failure), code = "AUTH_FAILED" }, statusCode: 401)); } var roleMapper = context.RequestServices.GetRequiredService(); - var mappingResult = await roleMapper.MapGroupsToRolesAsync( - authResult.Groups ?? (IReadOnlyList)Array.Empty()); + var mappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups, context.RequestAborted); var permittedSiteIds = mappingResult.IsSystemWideDeployment ? Array.Empty() : mappingResult.PermittedSiteIds.ToArray(); var user = new AuthenticatedUser( - authResult.Username!, - authResult.DisplayName!, + authResult.Username, + authResult.DisplayName, mappingResult.Roles.ToArray(), permittedSiteIds); diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs index 4021ec67..7f6542b2 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/DebugStreamHub.cs @@ -6,6 +6,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Messages.DebugView; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Streaming; using ZB.MOM.WW.ScadaBridge.Communication; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.ManagementService; @@ -99,9 +100,9 @@ public class DebugStreamHub : Hub } // LDAP authentication - var ldapAuth = httpContext.RequestServices.GetRequiredService(); - var authResult = await ldapAuth.AuthenticateAsync(username, password); - if (!authResult.Success) + var ldapAuth = httpContext.RequestServices.GetRequiredService(); + var authResult = await ldapAuth.AuthenticateAsync(username, password, Context.ConnectionAborted); + if (!authResult.Succeeded) { _logger.LogWarning("DebugStreamHub connection rejected: LDAP auth failed for {Username}", username); Context.Abort(); @@ -110,8 +111,7 @@ public class DebugStreamHub : Hub // Role check — Deployment role required var roleMapper = httpContext.RequestServices.GetRequiredService(); - var mappingResult = await roleMapper.MapGroupsToRolesAsync( - authResult.Groups ?? (IReadOnlyList)Array.Empty()); + var mappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups, Context.ConnectionAborted); if (!mappingResult.Roles.Contains("Deployment")) { diff --git a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs index c9ebb30e..5766d188 100644 --- a/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs +++ b/src/ZB.MOM.WW.ScadaBridge.ManagementService/ManagementEndpoints.cs @@ -8,6 +8,7 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.ScadaBridge.Commons.Messages.Management; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.ManagementService; @@ -85,27 +86,26 @@ public static class ManagementEndpoints } // 2. LDAP authentication - var ldapAuth = context.RequestServices.GetRequiredService(); - var authResult = await ldapAuth.AuthenticateAsync(username, password); - if (!authResult.Success) + var ldapAuth = context.RequestServices.GetRequiredService(); + var authResult = await ldapAuth.AuthenticateAsync(username, password, context.RequestAborted); + if (!authResult.Succeeded) { return Results.Json( - new { error = authResult.ErrorMessage ?? "Authentication failed.", code = "AUTH_FAILED" }, + new { error = LdapAuthFailureMessages.ToMessage(authResult.Failure), code = "AUTH_FAILED" }, statusCode: 401); } // 3. Role resolution var roleMapper = context.RequestServices.GetRequiredService(); - var mappingResult = await roleMapper.MapGroupsToRolesAsync( - authResult.Groups ?? (IReadOnlyList)Array.Empty()); + var mappingResult = await roleMapper.MapGroupsToRolesAsync(authResult.Groups, context.RequestAborted); var permittedSiteIds = mappingResult.IsSystemWideDeployment ? Array.Empty() : mappingResult.PermittedSiteIds.ToArray(); var authenticatedUser = new AuthenticatedUser( - authResult.Username!, - authResult.DisplayName!, + authResult.Username, + authResult.DisplayName, mappingResult.Roles.ToArray(), permittedSiteIds); diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthFailureMessages.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthFailureMessages.cs new file mode 100644 index 00000000..d67d4a72 --- /dev/null +++ b/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthFailureMessages.cs @@ -0,0 +1,87 @@ +using ZB.MOM.WW.Auth.Abstractions.Ldap; + +namespace ZB.MOM.WW.ScadaBridge.Security; + +/// +/// Translates the shared enum returned by +/// ZB.MOM.WW.Auth.Ldap's into the +/// user-facing error strings ScadaBridge surfaced from its bespoke +/// LdapAuthService before the Task 1.2 cutover. +/// +/// +/// +/// The cutover replaced ScadaBridge's hand-rolled LDAP client (which returned a +/// pre-formatted ErrorMessage string) with the shared library service +/// (which returns a structured code). This single +/// adapter keeps the externally observable error text stable across the login +/// (/auth/login, /auth/token) and Basic-Auth (ManagementService) +/// surfaces so the user-visible behaviour does not regress. +/// +/// +/// Message-mapping rationale, preserving the donor's deliberate security framing: +/// +/// and +/// both map to the same generic +/// "Invalid username or password." — a username-enumeration guard +/// (Security): a "user not found" message must be indistinguishable from a +/// "wrong password" message. +/// (the directory returned +/// two or more entries for the username) is a directory-data fault, not a +/// user-credential one. The donor never attempted an ambiguous bind; the +/// library rejects it outright. Surfaced as the misconfiguration message so +/// the operator — not the user — is pointed at the cause. +/// keeps the +/// donor's distinct "service is misconfigured" wording (Security-019) so a +/// system-side fault is not blamed on user input. NOTE: the library also maps +/// connect/search infrastructure failures (directory unreachable) into this +/// bucket, so this message now covers "directory unavailable at connect/search +/// time" as well — see for the +/// post-bind directory-outage case. +/// keeps the donor's +/// "directory is temporarily unavailable" wording (Security-012): a post-bind +/// group-lookup failure means the directory is partially unavailable and the +/// login is failed closed rather than admitting a roleless session. NOTE: the +/// library additionally treats a successful-but-empty group set as +/// , whereas the donor admitted +/// an empty-group user as a successful (roleless) login — a documented +/// behavioural deviation of the cutover. +/// (the provider is turned off via +/// Enabled = false) maps to a neutral "not available" message. +/// +/// +/// +public static class LdapAuthFailureMessages +{ + /// The generic, enumeration-safe message for a bad-credentials / user-not-found failure. + public const string InvalidCredentials = "Invalid username or password."; + + /// The system-misconfiguration message (service-account bind / ambiguous user / unreachable directory). + public const string Misconfigured = "Authentication service is misconfigured. Contact an administrator."; + + /// The transient directory-outage message for a post-bind group-lookup failure. + public const string DirectoryUnavailable = "The directory is temporarily unavailable. Please try again."; + + /// The provider-disabled message. + public const string Disabled = "Authentication is not available."; + + /// The fallback message for an unrecognised failure code. + public const string Generic = "Authentication failed."; + + /// + /// Maps a to its user-facing message. A + /// failure (which should not occur on a failed result) + /// falls back to . + /// + /// The structured failure code from . + /// The user-facing error string to surface. + public static string ToMessage(LdapAuthFailure? failure) => failure switch + { + LdapAuthFailure.BadCredentials => InvalidCredentials, + LdapAuthFailure.UserNotFound => InvalidCredentials, + LdapAuthFailure.AmbiguousUser => Misconfigured, + LdapAuthFailure.ServiceAccountBindFailed => Misconfigured, + LdapAuthFailure.GroupLookupFailed => DirectoryUnavailable, + LdapAuthFailure.Disabled => Disabled, + _ => Generic, + }; +} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthResult.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthResult.cs deleted file mode 100644 index 54cc23ea..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthResult.cs +++ /dev/null @@ -1,8 +0,0 @@ -namespace ZB.MOM.WW.ScadaBridge.Security; - -public record LdapAuthResult( - bool Success, - string? DisplayName, - string? Username, - IReadOnlyList? Groups, - string? ErrorMessage); diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthService.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthService.cs deleted file mode 100644 index 389506ad..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LdapAuthService.cs +++ /dev/null @@ -1,417 +0,0 @@ -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Options; -using Novell.Directory.Ldap; - -namespace ZB.MOM.WW.ScadaBridge.Security; - -public class LdapAuthService -{ - private readonly SecurityOptions _options; - private readonly ILogger _logger; - - /// Initializes a new instance of with the given options and logger. - /// Security configuration options including LDAP server settings. - /// Logger for authentication diagnostics. - public LdapAuthService(IOptions options, ILogger logger) - { - _options = options?.Value ?? throw new ArgumentNullException(nameof(options)); - _logger = logger ?? throw new ArgumentNullException(nameof(logger)); - } - - // virtual: a test seam so HTTP-pipeline tests (e.g. the #23 M8 audit - // endpoints) can substitute the LDAP bind without standing up a directory. - /// Authenticates a user against the configured LDAP directory and returns an auth result with roles. - /// The plain-text username to authenticate. - /// The plain-text password to bind with. - /// Cancellation token. - public virtual async Task AuthenticateAsync(string username, string password, CancellationToken ct = default) - { - if (string.IsNullOrWhiteSpace(username)) - return new LdapAuthResult(false, null, null, null, "Username is required."); - - if (string.IsNullOrWhiteSpace(password)) - return new LdapAuthResult(false, null, null, null, "Password is required."); - - // Trim once, up front: a username with leading/trailing whitespace (copy-paste - // artefacts, mobile keyboards) is otherwise passed verbatim into the LDAP filter, - // the fallback bind DN, and — most consequentially — the JWT Username claim and - // audit trail, producing two distinct identities for the same person - // (Security-015). The IsNullOrWhiteSpace guard above already rejects an - // all-whitespace value, so the trimmed result here is always non-empty. - username = NormalizeUsername(username); - - // Enforce TLS unless explicitly allowed for dev/test - if (_options.LdapTransport == LdapTransport.None && !_options.AllowInsecureLdap) - { - return new LdapAuthResult(false, null, null, null, - "Insecure LDAP connections are not allowed. Enable TLS or set AllowInsecureLdap for dev/test."); - } - - try - { - using var connection = new LdapConnection(); - - // Bound how long a hung LDAP server can pin a thread-pool thread. The - // `ct` passed to Task.Run below only prevents the work item from starting; - // it cannot interrupt an in-progress blocking Connect/Bind/Search. This - // timeout is the real safeguard (Security-009). - ApplyConnectionTimeout(connection); - - // LDAPS: TLS negotiated at connection time. StartTLS: connect plaintext, - // then upgrade the session before any credentials are sent. - if (_options.LdapTransport == LdapTransport.Ldaps) - { - connection.SecureSocketLayer = true; - } - - await Task.Run(() => connection.Connect(_options.LdapServer, _options.LdapPort), ct); - - if (_options.LdapTransport == LdapTransport.StartTls) - { - await Task.Run(() => connection.StartTls(), ct); - - if (!connection.Tls) - { - return new LdapAuthResult(false, null, null, null, - "StartTLS upgrade did not produce an encrypted session."); - } - } - - // Resolve the user's actual DN, then bind with their credentials - var bindDn = await ResolveUserDnAsync(connection, username, ct); - await Task.Run(() => connection.Bind(bindDn, password), ct); - - // Re-bind as service account for attribute/group lookup (user may lack search rights). - // A failure here is the SYSTEM's misconfiguration (wrong service-account credentials, - // disabled/locked account) — not the user's credential problem. The user bind on the - // line above already succeeded, so masking this as "Invalid username or password" would - // route operators down the wrong incident path (Security-019). - if (!string.IsNullOrWhiteSpace(_options.LdapServiceAccountDn)) - { - await BindServiceAccountAsync(connection, ct); - } - - // Query for user attributes and group memberships - var displayName = username; - var groups = new List(); - var groupLookupSucceeded = true; - - try - { - var searchFilter = $"({_options.LdapUserIdAttribute}={EscapeLdapFilter(username)})"; - var searchResults = await Task.Run(() => - connection.Search( - _options.LdapSearchBase, - LdapConnection.ScopeSub, - searchFilter, - new[] { _options.LdapDisplayNameAttribute, _options.LdapGroupAttribute }, - false), ct); - - // `HasMore()` is the loop guard for end-of-results; it returns false - // when the enumeration is exhausted. An LdapException thrown by - // `Next()` inside a HasMore()-guarded loop is therefore NOT a benign - // "no more results" sentinel — it is a genuine error (referral failure, - // server-side limit, transport drop mid-enumeration). The previous - // `catch (LdapException) { break; }` silently truncated the group list - // and masked a partial outage (Security-012); such an exception now - // propagates to the outer catch and fails the login. - while (searchResults.HasMore()) - { - var entry = searchResults.Next(); - - var dnAttr = entry.GetAttribute(_options.LdapDisplayNameAttribute); - if (dnAttr != null) - displayName = dnAttr.StringValue; - - var groupAttr = entry.GetAttribute(_options.LdapGroupAttribute); - if (groupAttr != null) - { - foreach (var groupDn in groupAttr.StringValueArray) - { - groups.Add(ExtractFirstRdnValue(groupDn)); - } - } - } - } - catch (LdapException ex) - { - // A failed group/attribute lookup on initial login means the directory - // is partially unavailable. The design's LDAP-failure rule requires new - // logins to FAIL when LDAP is unavailable — admitting the user here - // would yield an authenticated session with zero roles (Security-012). - _logger.LogWarning(ex, "LDAP group/attribute lookup failed for user {Username}; failing the login per the LDAP-failure rule", username); - groupLookupSucceeded = false; - } - - connection.Disconnect(); - - return BuildAuthResultFromGroupLookup(username, displayName, groups, groupLookupSucceeded); - } - catch (ServiceAccountBindException ex) - { - // Distinct from the user-credential catch below so the operator - // sees the *system* misconfiguration rather than blaming user input - // (Security-019). The inner exception was already logged at Error - // by BindServiceAccountAsync; nothing further to log here. - _ = ex; - return new LdapAuthResult(false, null, username, null, - "Authentication service is misconfigured. Contact an administrator."); - } - catch (LdapException ex) - { - _logger.LogWarning(ex, "LDAP authentication failed for user {Username}", username); - return new LdapAuthResult(false, null, username, null, "Invalid username or password."); - } - catch (Exception ex) when (ex is not OperationCanceledException) - { - _logger.LogError(ex, "Unexpected error during LDAP authentication for user {Username}", username); - return new LdapAuthResult(false, null, username, null, "An unexpected error occurred during authentication."); - } - } - - /// - /// Binds the supplied connection as the configured service account. A failure here is - /// a system-misconfiguration condition (Security-019) — wrong service-account DN / - /// password, locked or disabled account, server-side ACL change — not a user-credential - /// problem. The underlying is logged at Error and rethrown - /// as so callers can distinguish it from a - /// user-bind failure. - /// - private async Task BindServiceAccountAsync(LdapConnection connection, CancellationToken ct) - { - try - { - await Task.Run(() => - connection.Bind(_options.LdapServiceAccountDn, _options.LdapServiceAccountPassword), ct); - } - catch (LdapException ex) - { - _logger.LogError(ex, - "Service-account rebind failed; check LdapServiceAccountDn / LdapServiceAccountPassword configuration"); - throw new ServiceAccountBindException(ex); - } - } - - /// - /// Applies to both the socket - /// connect timeout and the per-operation (bind/search) time limit, so a hung or - /// unresponsive LDAP server cannot pin a thread-pool thread indefinitely. The - /// CancellationToken handed to the Task.Run wrappers only guards - /// work-item scheduling and cannot interrupt an in-progress blocking call. - /// - private void ApplyConnectionTimeout(LdapConnection connection) - { - var timeoutMs = _options.LdapConnectionTimeoutMs; - if (timeoutMs <= 0) - return; - - connection.ConnectionTimeout = timeoutMs; - - // LdapConstraints.TimeLimit is the server-side operation time limit in ms. - var constraints = connection.Constraints; - constraints.TimeLimit = timeoutMs; - connection.Constraints = constraints; - } - - /// - /// Resolves the user's full DN. When a service account is configured, performs a - /// search-then-bind lookup. Otherwise falls back to constructing the DN directly. - /// - private async Task ResolveUserDnAsync(LdapConnection connection, string username, CancellationToken ct) - { - // If a service account is configured, search for the user's actual DN. - // The service-account bind is routed through BindServiceAccountAsync so a - // misconfiguration surfaces distinctly rather than masking as - // "Invalid username or password" (Security-019). - if (!string.IsNullOrWhiteSpace(_options.LdapServiceAccountDn)) - { - await BindServiceAccountAsync(connection, ct); - - var searchFilter = $"({_options.LdapUserIdAttribute}={EscapeLdapFilter(username)})"; - var searchResults = await Task.Run(() => - connection.Search( - _options.LdapSearchBase, - LdapConnection.ScopeSub, - searchFilter, - new[] { "dn" }, - false), ct); - - if (searchResults.HasMore()) - { - var entry = searchResults.Next(); - return entry.Dn; - } - - throw new LdapException("User not found", LdapException.NoSuchObject, - $"No entry found for {_options.LdapUserIdAttribute}={username}"); - } - - // Fallback: construct the bind DN directly from the configured user-id - // attribute. The username is RFC 4514 DN-escaped so it cannot alter the - // DN structure (Security-005). The previous Contains('=') shortcut that - // accepted a raw caller-supplied DN has been removed — accepting an - // arbitrary DN from untrusted input let a client choose the bind identity. - return BuildFallbackUserDn(username, _options.LdapSearchBase, _options.LdapUserIdAttribute); - } - - /// - /// Builds the no-service-account fallback bind DN as - /// {userIdAttribute}={escaped-username}[,{searchBase}]. The username is - /// escaped per RFC 4514 so DN metacharacters in untrusted input cannot inject - /// additional RDN components or change the bind identity. - /// - /// The username to embed in the DN value. - /// The LDAP search base to append after the RDN, if any. - /// The attribute name (e.g. uid or sAMAccountName) used as the RDN type. - public static string BuildFallbackUserDn(string username, string searchBase, string userIdAttribute) - { - var rdn = $"{userIdAttribute}={EscapeLdapDn(username)}"; - return string.IsNullOrWhiteSpace(searchBase) ? rdn : $"{rdn},{searchBase}"; - } - - /// - /// Escapes a string for use as an RFC 4514 DN attribute value: the special - /// characters , + " \ < > ; are backslash-escaped, as are a leading - /// or trailing space and a leading #. - /// - /// The raw string to escape. - public static string EscapeLdapDn(string input) - { - if (string.IsNullOrEmpty(input)) - return input; - - var sb = new System.Text.StringBuilder(input.Length + 8); - for (var i = 0; i < input.Length; i++) - { - var c = input[i]; - var isEdgeSpace = c == ' ' && (i == 0 || i == input.Length - 1); - var isLeadingHash = c == '#' && i == 0; - switch (c) - { - case ',': - case '+': - case '"': - case '\\': - case '<': - case '>': - case ';': - sb.Append('\\').Append(c); - break; - case '\0': - sb.Append("\\00"); - break; - default: - if (isEdgeSpace || isLeadingHash) - sb.Append('\\'); - sb.Append(c); - break; - } - } - return sb.ToString(); - } - - private static string EscapeLdapFilter(string input) - { - return input - .Replace("\\", "\\5c") - .Replace("*", "\\2a") - .Replace("(", "\\28") - .Replace(")", "\\29") - .Replace("\0", "\\00"); - } - - /// - /// Normalises a username by trimming leading and trailing whitespace. Applied once - /// at the top of so the same canonical value flows - /// into the LDAP filter, the fallback bind DN, and the JWT Username claim — - /// avoiding two distinct identities for the same person (Security-015). - /// - /// The raw username input to normalise. - public static string NormalizeUsername(string username) - => username?.Trim() ?? string.Empty; - - /// - /// Builds the final for a login attempt once the user - /// bind has succeeded. When the group/attribute lookup failed - /// ( is false) the directory is partially - /// unavailable, so the login is FAILED per the design's LDAP-failure rule rather - /// than returning an authenticated session with zero roles (Security-012). When the - /// lookup succeeded, an empty list is a genuine - /// "no mapped groups" outcome and the login succeeds. - /// - /// The normalised username that was authenticated. - /// The display name resolved from the directory. - /// The list of group names resolved from the directory. - /// Whether the group/attribute lookup completed without error. - public static LdapAuthResult BuildAuthResultFromGroupLookup( - string username, - string displayName, - IReadOnlyList groups, - bool groupLookupSucceeded) - { - if (!groupLookupSucceeded) - { - return new LdapAuthResult(false, null, username, null, - "The directory is temporarily unavailable. Please try again."); - } - - return new LdapAuthResult(true, displayName, username, groups, null); - } - - /// - /// Extracts the value of the first RDN from a DN, e.g. - /// ou=SCADA-Admins,ou=groups,dc=... → SCADA-Admins. The scan is - /// RFC 4514 escape-aware: a backslash-escaped , inside the RDN value does - /// not terminate it, and recognised escape sequences are unescaped, so a group CN - /// that legitimately contains a comma is returned intact (Security-013). - /// - /// The distinguished name string to parse. - public static string ExtractFirstRdnValue(string dn) - { - if (string.IsNullOrEmpty(dn)) - return dn; - - var equalsIndex = dn.IndexOf('='); - if (equalsIndex < 0) - return dn; - - var valueStart = equalsIndex + 1; - var sb = new System.Text.StringBuilder(dn.Length - valueStart); - - for (var i = valueStart; i < dn.Length; i++) - { - var c = dn[i]; - if (c == '\\' && i + 1 < dn.Length) - { - var next = dn[i + 1]; - // RFC 4514 hex escape: \XX (two hex digits). - if (i + 2 < dn.Length && IsHexDigit(next) && IsHexDigit(dn[i + 2])) - { - sb.Append((char)Convert.ToInt32(dn.Substring(i + 1, 2), 16)); - i += 2; - } - else - { - // Single-character escape (e.g. \, \+ \\ \" \; etc.) — emit the - // escaped character literally and skip the backslash. - sb.Append(next); - i += 1; - } - continue; - } - - if (c == ',') - { - // Unescaped comma terminates the first RDN. - break; - } - - sb.Append(c); - } - - return sb.ToString(); - } - - private static bool IsHexDigit(char c) - => (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f') || (c >= 'A' && c <= 'F'); -} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/LdapTransport.cs b/src/ZB.MOM.WW.ScadaBridge.Security/LdapTransport.cs deleted file mode 100644 index c4cdc1a0..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Security/LdapTransport.cs +++ /dev/null @@ -1,26 +0,0 @@ -namespace ZB.MOM.WW.ScadaBridge.Security; - -/// -/// Transport security mode for the LDAP connection. The design requires either -/// LDAPS or StartTLS in production; is for dev/test only and -/// must be paired with . -/// -public enum LdapTransport -{ - /// - /// LDAPS — TLS negotiated at connection time (typically port 636). - /// - Ldaps, - - /// - /// StartTLS — connect in plaintext (typically port 389), then upgrade the - /// session to TLS before binding. - /// - StartTls, - - /// - /// No transport security. Dev/test only — requires - /// to be true. - /// - None -} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs b/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs index e4b8d5dc..34c19d22 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptions.cs @@ -1,82 +1,20 @@ namespace ZB.MOM.WW.ScadaBridge.Security; +/// +/// Non-LDAP security configuration: the cookie-embedded JWT signing/lifetime +/// settings and the session idle-timeout / cookie-security policy. +/// +/// +/// Task 1.2/1.4 cutover: the LDAP connection settings that used to live here as +/// flat Ldap* keys (server, port, transport, search base, service account, +/// attributes, timeout) moved into a nested ScadaBridge:Security:Ldap +/// sub-section bound to the shared ZB.MOM.WW.Auth.Abstractions.Ldap.LdapOptions +/// and registered via AddZbLdapAuth. This is a BREAKING config-key change — +/// see CHANGELOG. The non-LDAP fields below are unchanged and still bound from +/// ScadaBridge:Security. +/// public class SecurityOptions { - /// Hostname or IP address of the LDAP server. - public string LdapServer { get; set; } = string.Empty; - /// TCP port for the LDAP connection (default 389; 636 for LDAPS). - public int LdapPort { get; set; } = 389; - - /// - /// Transport security mode for the LDAP connection. Defaults to LDAPS. - /// Use to connect on the plaintext port - /// and upgrade the session before binding. - /// - public LdapTransport LdapTransport { get; set; } = LdapTransport.Ldaps; - - /// - /// True when the configured transport provides encryption (LDAPS or StartTLS). - /// Retained for backward compatibility: assigning a value maps onto - /// (true => LDAPS, false => None). - /// - public bool LdapUseTls - { - get => LdapTransport != LdapTransport.None; - set => LdapTransport = value ? LdapTransport.Ldaps : LdapTransport.None; - } - - /// - /// Allow insecure (non-TLS) LDAP connections. ONLY for dev/test with GLAuth. - /// Must be false in production. - /// - public bool AllowInsecureLdap { get; set; } = false; - - /// - /// Base DN for LDAP searches (e.g., "dc=example,dc=com"). - /// - public string LdapSearchBase { get; set; } = string.Empty; - - /// - /// Service account DN for LDAP user searches (e.g., "cn=admin,dc=example,dc=com"). - /// Required for search-then-bind authentication. If empty, direct bind with - /// {LdapUserIdAttribute}={username},{LdapSearchBase} is attempted instead. - /// - public string LdapServiceAccountDn { get; set; } = string.Empty; - - /// - /// LDAP attribute that identifies a user. Used both for the search-then-bind - /// filter (({LdapUserIdAttribute}={username})) and for constructing the - /// fallback bind DN when no service account is configured, so the two - /// authentication modes are interchangeable. Common values: uid (OpenLDAP), - /// sAMAccountName (Active Directory). - /// - public string LdapUserIdAttribute { get; set; } = "uid"; - - /// - /// Service account password for LDAP user searches. - /// - public string LdapServiceAccountPassword { get; set; } = string.Empty; - - /// - /// LDAP attribute that contains the user's display name. - /// - public string LdapDisplayNameAttribute { get; set; } = "cn"; - - /// - /// LDAP attribute that contains group membership. - /// - public string LdapGroupAttribute { get; set; } = "memberOf"; - - /// - /// Network timeout, in milliseconds, applied to the LDAP socket connect and to - /// LDAP operations (bind/search). The synchronous Novell LDAP calls are wrapped - /// in Task.Run, where the CancellationToken only guards work-item - /// scheduling — it cannot interrupt an in-progress blocking call. This timeout is - /// the real safeguard: it bounds how long a hung LDAP server can pin a thread-pool - /// thread (Security-009). Default 10 seconds. - /// - public int LdapConnectionTimeoutMs { get; set; } = 10_000; - /// /// Symmetric HMAC-SHA256 signing key for cookie-embedded JWTs. Must be at least /// 32 bytes (256 bits) — validated at construction. diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptionsValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptionsValidator.cs deleted file mode 100644 index d47a764d..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Security/SecurityOptionsValidator.cs +++ /dev/null @@ -1,56 +0,0 @@ -using ZB.MOM.WW.Configuration; - -namespace ZB.MOM.WW.ScadaBridge.Security; - -/// -/// Security-020: validates at startup so a -/// missing or empty required LDAP field fails fast at boot with a clear, -/// key-naming message — rather than surfacing minutes or hours later as a -/// generic "An unexpected error occurred during authentication" on the first -/// real login attempt. -/// -/// -/// The LDAP-side required fields validated here are -/// (no sane default — the host must be specified) and -/// (the DN root every directory -/// search runs against). A typo in the appsettings section name, a missing -/// environment-variable substitution, or a misconfigured Docker compose file -/// leaves both defaulted to string.Empty — without this validator the -/// process would start cleanly and only fail on the first login when -/// LdapConnection.Connect("") throws a low-level exception that does -/// not name the offending config key. -/// -/// -/// -/// is intentionally NOT validated -/// here — it already fails fast at construction -/// (Security-003 fix), with a length-aware error message. Centralising it -/// here would duplicate that guard; leaving it on the constructor keeps the -/// minimum-byte length contract co-located with the type that enforces it. -/// -/// -public sealed class SecurityOptionsValidator : OptionsValidatorBase -{ - /// - /// The configuration section name is bound - /// to (matches the Host's builder.Configuration.GetSection("Security") - /// call). Exposed so validation messages can name the full - /// Security:Field key the operator would edit, not just the field - /// name. - /// - public const string ConfigSectionName = "Security"; - - /// - protected override void Validate(ValidationBuilder builder, SecurityOptions options) - { - builder.RequireThat(!string.IsNullOrWhiteSpace(options.LdapServer), - $"{ConfigSectionName}:{nameof(SecurityOptions.LdapServer)} is required " + - "but was empty or whitespace — set it to the LDAP server hostname or IP " + - "(e.g. \"ldap.example.com\")."); - - builder.RequireThat(!string.IsNullOrWhiteSpace(options.LdapSearchBase), - $"{ConfigSectionName}:{nameof(SecurityOptions.LdapSearchBase)} is required " + - "but was empty or whitespace — set it to the search-base DN " + - "(e.g. \"dc=example,dc=com\")."); - } -} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceAccountBindException.cs b/src/ZB.MOM.WW.ScadaBridge.Security/ServiceAccountBindException.cs deleted file mode 100644 index 0e3dd715..00000000 --- a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceAccountBindException.cs +++ /dev/null @@ -1,15 +0,0 @@ -namespace ZB.MOM.WW.ScadaBridge.Security; - -/// -/// Thrown by when the configured LDAP service-account -/// rebind fails. Distinct from a user-bind LdapException so the outer login -/// pipeline can surface "Authentication service is misconfigured" instead of -/// masking the system fault as "Invalid username or password" (Security-019). -/// -public sealed class ServiceAccountBindException : Exception -{ - public ServiceAccountBindException(Exception innerException) - : base("LDAP service-account rebind failed", innerException) - { - } -} diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs b/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs index 97c6f785..5f355251 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Security/ServiceCollectionExtensions.cs @@ -1,21 +1,44 @@ using Microsoft.AspNetCore.Authentication.Cookies; +using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; -using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Options; using ZB.MOM.WW.Auth.Abstractions.Roles; +using ZB.MOM.WW.Auth.AspNetCore; namespace ZB.MOM.WW.ScadaBridge.Security; public static class ServiceCollectionExtensions { /// - /// Registers LDAP authentication, JWT token service, role mapper, cookie authentication, and authorization policies. + /// The configuration section bound to the shared LDAP LdapOptions. Nested + /// under the existing ScadaBridge:Security section as a Ldap sub-section + /// (Task 1.4 config rename) so the non-LDAP fields stay + /// where they are while the LDAP connection settings bind to the shared library. + /// + public const string LdapSectionPath = "ScadaBridge:Security:Ldap"; + + /// + /// Registers LDAP authentication (shared ZB.MOM.WW.Auth.Ldap), JWT token service, + /// role mapper, cookie authentication, and authorization policies. /// /// The service collection to register into. - public static IServiceCollection AddSecurity(this IServiceCollection services) + /// + /// Application configuration, read for the nested LDAP + /// options bound + validated by AddZbLdapAuth. + /// + public static IServiceCollection AddSecurity(this IServiceCollection services, IConfiguration configuration) { - services.AddScoped(); + // Task 1.2 cutover: replace ScadaBridge's bespoke LdapAuthService with the shared + // ZB.MOM.WW.Auth.Ldap implementation (ScadaBridge was the donor for its hardened + // bind-then-search / escaping / fail-closed semantics, so this is a behaviour- + // equivalent re-point). AddZbLdapAuth binds LdapOptions from the nested Ldap + // sub-section, registers IValidateOptions with ValidateOnStart (so a + // misconfigured directory fails fast at boot — superseding the old + // SecurityOptionsValidator LDAP checks), and registers ILdapAuthService as a + // stateless singleton. + services.AddZbLdapAuth(configuration, LdapSectionPath); + services.AddScoped(); services.AddScoped(); @@ -27,16 +50,11 @@ public static class ServiceCollectionExtensions // to consume this seam in a later task. services.AddScoped, ScadaBridgeGroupRoleMapper>(); - // Security-020: register the IValidateOptions so a - // missing/empty LdapServer or LdapSearchBase fails fast at startup - // with a clear, key-naming message rather than a generic LDAP error - // on the first real login. ValidateOnStart() forces the validation to - // run during host startup rather than lazily on the first - // IOptions resolve. TryAddEnumerable so multiple - // AddSecurity calls (or future additional validators) don't pile up. - services.AddOptions().ValidateOnStart(); - services.TryAddEnumerable( - ServiceDescriptor.Singleton, SecurityOptionsValidator>()); + // Note: the old SecurityOptionsValidator (which fail-fast-validated LdapServer + + // LdapSearchBase) is gone — those keys moved into the shared LdapOptions, whose + // LdapOptionsValidator (registered with ValidateOnStart by AddZbLdapAuth above) + // now enforces Server + SearchBase + ServiceAccountDn + transport at startup. The + // JWT signing key continues to fail-fast at JwtTokenService construction. // Register ASP.NET Core authentication with cookie scheme services.AddAuthentication(CookieAuthenticationDefaults.AuthenticationScheme) diff --git a/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj b/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj index faa7131e..bc8b1264 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj +++ b/src/ZB.MOM.WW.ScadaBridge.Security/ZB.MOM.WW.ScadaBridge.Security.csproj @@ -15,6 +15,8 @@ + + diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs index 18263df2..ecf872f9 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/ActorPathTests.cs @@ -43,11 +43,15 @@ public class CentralActorPathTests : IAsyncLifetime ["ScadaBridge:Cluster:MinNrOfMembers"] = "1", ["ScadaBridge:Database:SkipMigrations"] = "true", ["ScadaBridge:Security:JwtSigningKey"] = "test-signing-key-must-be-at-least-32-chars-long!", - ["ScadaBridge:Security:LdapServer"] = "localhost", - ["ScadaBridge:Security:LdapPort"] = "3893", - ["ScadaBridge:Security:LdapUseTls"] = "false", - ["ScadaBridge:Security:AllowInsecureLdap"] = "true", - ["ScadaBridge:Security:LdapSearchBase"] = "dc=scadabridge,dc=local", + // Task 1.4: LDAP settings nest under Security:Ldap (shared LdapOptions). + // ServiceAccountDn is now required by the library's LdapOptionsValidator + // (ValidateOnStart), so it must be present for the host to start. + ["ScadaBridge:Security:Ldap:Server"] = "localhost", + ["ScadaBridge:Security:Ldap:Port"] = "3893", + ["ScadaBridge:Security:Ldap:Transport"] = "None", + ["ScadaBridge:Security:Ldap:AllowInsecure"] = "true", + ["ScadaBridge:Security:Ldap:SearchBase"] = "dc=scadabridge,dc=local", + ["ScadaBridge:Security:Ldap:ServiceAccountDn"] = "cn=admin,dc=scadabridge,dc=local", }); }); builder.UseSetting("ScadaBridge:Node:Role", "Central"); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs index b4c5e815..69ec59a3 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/AkkaHostedServiceAuditWiringTests.cs @@ -108,11 +108,15 @@ public class CentralAuditWiringTests : IDisposable ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", ["ScadaBridge:Database:SkipMigrations"] = "true", ["ScadaBridge:Security:JwtSigningKey"] = "test-signing-key-must-be-at-least-32-chars-long!", - ["ScadaBridge:Security:LdapServer"] = "localhost", - ["ScadaBridge:Security:LdapPort"] = "3893", - ["ScadaBridge:Security:LdapUseTls"] = "false", - ["ScadaBridge:Security:AllowInsecureLdap"] = "true", - ["ScadaBridge:Security:LdapSearchBase"] = "dc=scadabridge,dc=local", + // Task 1.4: LDAP settings nest under Security:Ldap (shared LdapOptions). + // ServiceAccountDn is now required by the library's LdapOptionsValidator + // (ValidateOnStart), so it must be present for the host to start. + ["ScadaBridge:Security:Ldap:Server"] = "localhost", + ["ScadaBridge:Security:Ldap:Port"] = "3893", + ["ScadaBridge:Security:Ldap:Transport"] = "None", + ["ScadaBridge:Security:Ldap:AllowInsecure"] = "true", + ["ScadaBridge:Security:Ldap:SearchBase"] = "dc=scadabridge,dc=local", + ["ScadaBridge:Security:Ldap:ServiceAccountDn"] = "cn=admin,dc=scadabridge,dc=local", ["ScadaBridge:InboundApi:ApiKeyPepper"] = "test-inbound-api-key-pepper-at-least-32-chars!", }); }); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs index 74d4390f..104bfb19 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/CompositionRootTests.cs @@ -21,6 +21,7 @@ using ZB.MOM.WW.ScadaBridge.Host.Health; using ZB.MOM.WW.ScadaBridge.InboundAPI; using ZB.MOM.WW.ScadaBridge.ManagementService; using ZB.MOM.WW.ScadaBridge.NotificationService; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; using ZB.MOM.WW.ScadaBridge.SiteEventLogging; using ZB.MOM.WW.ScadaBridge.Communication.Grpc; @@ -102,11 +103,15 @@ public class CentralCompositionRootTests : IDisposable ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@localhost:2552", ["ScadaBridge:Database:SkipMigrations"] = "true", ["ScadaBridge:Security:JwtSigningKey"] = "test-signing-key-must-be-at-least-32-chars-long!", - ["ScadaBridge:Security:LdapServer"] = "localhost", - ["ScadaBridge:Security:LdapPort"] = "3893", - ["ScadaBridge:Security:LdapUseTls"] = "false", - ["ScadaBridge:Security:AllowInsecureLdap"] = "true", - ["ScadaBridge:Security:LdapSearchBase"] = "dc=scadabridge,dc=local", + // Task 1.4: LDAP settings nest under Security:Ldap (shared LdapOptions). + // ServiceAccountDn is now required by the library's LdapOptionsValidator + // (ValidateOnStart), so it must be present for the host to start. + ["ScadaBridge:Security:Ldap:Server"] = "localhost", + ["ScadaBridge:Security:Ldap:Port"] = "3893", + ["ScadaBridge:Security:Ldap:Transport"] = "None", + ["ScadaBridge:Security:Ldap:AllowInsecure"] = "true", + ["ScadaBridge:Security:Ldap:SearchBase"] = "dc=scadabridge,dc=local", + ["ScadaBridge:Security:Ldap:ServiceAccountDn"] = "cn=admin,dc=scadabridge,dc=local", // ConfigurationDatabase-012: inbound-API keys are hashed // with a server-side HMAC pepper; ApiKeyHasher fails fast // if it is missing or weak, so resolving ApiKeyValidator @@ -167,6 +172,9 @@ public class CentralCompositionRootTests : IDisposable new object[] { typeof(OperationLockManager) }, new object[] { typeof(OAuth2TokenService) }, new object[] { typeof(InboundScriptExecutor) }, + // Security: the shared ZB.MOM.WW.Auth.Ldap service is registered as a stateless + // singleton by AddZbLdapAuth (Task 1.2), replacing the old scoped LdapAuthService. + new object[] { typeof(ILdapAuthService) }, }; // --- Scoped services --- @@ -193,8 +201,7 @@ public class CentralCompositionRootTests : IDisposable new object[] { typeof(IFlatteningPipeline) }, new object[] { typeof(DeploymentService) }, new object[] { typeof(ArtifactDeploymentService) }, - // Security - new object[] { typeof(LdapAuthService) }, + // Security (ILdapAuthService is now a singleton — see CentralSingletonServices) new object[] { typeof(JwtTokenService) }, new object[] { typeof(RoleMapper) }, // InboundAPI diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs index 95b0af2f..8a5b7e7a 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs @@ -20,7 +20,7 @@ public class StartupValidatorTests ["ScadaBridge:Node:NodeHostname"] = "central-node1", ["ScadaBridge:Node:RemotingPort"] = "8081", ["ScadaBridge:Database:ConfigurationDb"] = "Server=localhost;Database=Config;", - ["ScadaBridge:Security:LdapServer"] = "ldap.example.com", + ["ScadaBridge:Security:Ldap:Server"] = "ldap.example.com", ["ScadaBridge:Security:JwtSigningKey"] = "test-signing-key-at-least-32-chars-long", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@central-node1:8081", ["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@central-node2:8081", @@ -166,12 +166,14 @@ public class StartupValidatorTests [Fact] public void Central_MissingLdapServer_FailsValidation() { + // Task 1.4: the LDAP server key nests under Security:Ldap now. The pre-host + // preflight validates the nested key and still fails fast for Central. var values = ValidCentralConfig(); - values.Remove("ScadaBridge:Security:LdapServer"); + values.Remove("ScadaBridge:Security:Ldap:Server"); var config = BuildConfig(values); var ex = Assert.Throws(() => StartupValidator.Validate(config)); - Assert.Contains("LdapServer required for Central", ex.Message); + Assert.Contains("Ldap:Server required for Central", ex.Message); } [Fact] diff --git a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/ScadaBridgeWebApplicationFactory.cs b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/ScadaBridgeWebApplicationFactory.cs index bc6ddbb3..87882fc8 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/ScadaBridgeWebApplicationFactory.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/ScadaBridgeWebApplicationFactory.cs @@ -38,17 +38,19 @@ public class ScadaBridgeWebApplicationFactory : WebApplicationFactory ["ScadaBridge__Database__MachineDataDb"] = "Server=localhost;Database=ScadaBridge_MachineData_Test;TrustServerCertificate=True", ["ScadaBridge__Database__SkipMigrations"] = "true", ["ScadaBridge__Security__JwtSigningKey"] = "integration-test-signing-key-must-be-at-least-32-chars-long", - ["ScadaBridge__Security__LdapServer"] = "localhost", - ["ScadaBridge__Security__LdapPort"] = "3893", - ["ScadaBridge__Security__LdapUseTls"] = "false", - ["ScadaBridge__Security__AllowInsecureLdap"] = "true", - ["ScadaBridge__Security__LdapSearchBase"] = "dc=scadabridge,dc=local", - // GLAuth places users at cn=,ou=,ou=users,dc=... — the - // no-service-account fallback DN (uid=,dc=...) does not match, - // so a service account is configured to enable search-then-bind: - // resolve the user's real DN by (uid=) lookup, then bind it. - ["ScadaBridge__Security__LdapServiceAccountDn"] = "cn=admin,ou=SCADA-Admins,ou=users,dc=scadabridge,dc=local", - ["ScadaBridge__Security__LdapServiceAccountPassword"] = "password", + // Task 1.4: LDAP settings nest under Security:Ldap (shared LdapOptions) and use + // the renamed keys (Transport replaces LdapUseTls; None == plaintext for the + // GLAuth dev directory, paired with AllowInsecure=true). + ["ScadaBridge__Security__Ldap__Server"] = "localhost", + ["ScadaBridge__Security__Ldap__Port"] = "3893", + ["ScadaBridge__Security__Ldap__Transport"] = "None", + ["ScadaBridge__Security__Ldap__AllowInsecure"] = "true", + ["ScadaBridge__Security__Ldap__SearchBase"] = "dc=scadabridge,dc=local", + // GLAuth places users at cn=,ou=,ou=users,dc=... — a service + // account is configured to enable the shared service's search-then-bind: + // resolve the user's real DN by (UserNameAttribute=) lookup, then bind it. + ["ScadaBridge__Security__Ldap__ServiceAccountDn"] = "cn=admin,ou=SCADA-Admins,ou=users,dc=scadabridge,dc=local", + ["ScadaBridge__Security__Ldap__ServiceAccountPassword"] = "password", }; foreach (var (key, value) in envVars) diff --git a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/SecurityHardeningTests.cs b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/SecurityHardeningTests.cs index 62b395f3..01c5f365 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/SecurityHardeningTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/SecurityHardeningTests.cs @@ -1,5 +1,6 @@ using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.IntegrationTests; @@ -23,18 +24,19 @@ public class SecurityHardeningTests } [Fact] - public void SecurityOptions_LdapUseTls_DefaultsToTrue() + public void LdapOptions_Transport_DefaultsToLdaps() { - // Production requires LDAPS. The default must be true. - var options = new SecurityOptions(); - Assert.True(options.LdapUseTls); + // Production requires encrypted transport. The shared LdapOptions defaults to + // LDAPS (secure-by-default), preserving the donor's LdapUseTls=true default. + var options = new LdapOptions(); + Assert.Equal(LdapTransport.Ldaps, options.Transport); } [Fact] - public void SecurityOptions_AllowInsecureLdap_DefaultsToFalse() + public void LdapOptions_AllowInsecure_DefaultsToFalse() { - var options = new SecurityOptions(); - Assert.False(options.AllowInsecureLdap); + var options = new LdapOptions(); + Assert.False(options.AllowInsecure); } [Fact] @@ -172,10 +174,21 @@ public class SecurityHardeningTests [Fact] public void StartupValidator_RejectsInsecureLdapInProduction() { - // The SecurityOptions.AllowInsecureLdap defaults to false. - // Only when explicitly set to true (for dev/test) is insecure LDAP allowed. - var prodOptions = new SecurityOptions { LdapUseTls = true, AllowInsecureLdap = false }; - Assert.True(prodOptions.LdapUseTls); - Assert.False(prodOptions.AllowInsecureLdap); + // The shared LdapOptionsValidator (registered with ValidateOnStart by AddZbLdapAuth) + // rejects plaintext transport (Transport=None) unless AllowInsecure is explicitly set, + // preserving the donor's production LDAPS-enforcement guarantee. + var insecure = new LdapOptions + { + Server = "ldap.example.com", + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", + Transport = LdapTransport.None, + AllowInsecure = false, + }; + + var result = new ZB.MOM.WW.Auth.Ldap.LdapOptionsValidator().Validate(name: null, insecure); + + Assert.True(result.Failed); + Assert.Contains(nameof(LdapOptions.AllowInsecure), result.FailureMessage); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/StartupValidationTests.cs b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/StartupValidationTests.cs index 1940c793..dcd0cbfb 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/StartupValidationTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.IntegrationTests/StartupValidationTests.cs @@ -47,7 +47,7 @@ public class StartupValidationTests ["ScadaBridge__Cluster__SeedNodes__1"] = "akka.tcp://scadabridge@localhost:8082", ["ScadaBridge__Database__ConfigurationDb"] = "Server=x;Database=x", ["ScadaBridge__Database__MachineDataDb"] = "Server=x;Database=x", - ["ScadaBridge__Security__LdapServer"] = "localhost", + ["ScadaBridge__Security__Ldap__Server"] = "localhost", // Deliberately missing JwtSigningKey }); @@ -92,11 +92,13 @@ public class StartupValidationTests "ScadaBridge__Database__MachineDataDb", "ScadaBridge__Database__SkipMigrations", "ScadaBridge__Security__JwtSigningKey", - "ScadaBridge__Security__LdapServer", - "ScadaBridge__Security__LdapPort", - "ScadaBridge__Security__LdapUseTls", - "ScadaBridge__Security__AllowInsecureLdap", - "ScadaBridge__Security__LdapSearchBase", + "ScadaBridge__Security__Ldap__Server", + "ScadaBridge__Security__Ldap__Port", + "ScadaBridge__Security__Ldap__Transport", + "ScadaBridge__Security__Ldap__AllowInsecure", + "ScadaBridge__Security__Ldap__SearchBase", + "ScadaBridge__Security__Ldap__ServiceAccountDn", + "ScadaBridge__Security__Ldap__ServiceAccountPassword", }; public TempEnvironment(Dictionary varsToSet) diff --git a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs index 50c37bf3..c41fa64b 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.ManagementService.Tests/AuditEndpointsTests.cs @@ -14,6 +14,7 @@ using ZB.MOM.WW.ScadaBridge.Commons.Entities.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Repositories; using ZB.MOM.WW.ScadaBridge.Commons.Types.Audit; using ZB.MOM.WW.ScadaBridge.Commons.Types.Enums; +using ZB.MOM.WW.Auth.Abstractions.Ldap; using ZB.MOM.WW.ScadaBridge.ManagementService; using ZB.MOM.WW.ScadaBridge.Security; @@ -71,14 +72,12 @@ public class AuditEndpointsTests .Returns(Task.FromResult>(Array.Empty())); } - // Substituted LDAP bind — AuthenticateAsync is virtual (test seam). - var ldap = Substitute.For( - Options.Create(new SecurityOptions()), - Substitute.For>()); + // Substituted LDAP bind — the shared ILdapAuthService is the seam now (Task 1.2). + var ldap = Substitute.For(); ldap.AuthenticateAsync(Arg.Any(), Arg.Any(), Arg.Any()) .Returns(ldapSucceeds - ? new LdapAuthResult(true, "Auditor", "auditor", new[] { "cn=audit" }, null) - : new LdapAuthResult(false, null, null, null, "Bad credentials.")); + ? LdapAuthResult.Success("auditor", "Auditor", new[] { "audit" }) + : LdapAuthResult.Fail(LdapAuthFailure.BadCredentials)); // Substituted role mapper — MapGroupsToRolesAsync is virtual (test seam). var roleMapper = Substitute.For(Substitute.For()); diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ScadaBridgeGroupRoleMapperTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ScadaBridgeGroupRoleMapperTests.cs index 8ae32059..eb7b8a21 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ScadaBridgeGroupRoleMapperTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ScadaBridgeGroupRoleMapperTests.cs @@ -122,6 +122,49 @@ public class ScadaBridgeGroupRoleMapperTests Assert.True(scope.IsSystemWideDeployment); Assert.Empty(scope.PermittedSiteIds); } + + [Fact] + public async Task MapAsync_EmptyGroups_ReturnsNoRoles_AndNonNullScope() + { + // Reviewer-requested empty-groups case (Task 1.2): a user that resolves to NO + // groups must map to NO roles and a non-null, not-system-wide Scope with no + // permitted sites — so the login endpoint can build a roleless principal (which + // every authorization policy then denies) without NRE-ing on Scope. This is the + // post-cutover home for the donor's "no mapped groups" outcome, now that the + // shared LDAP service fail-closes a zero-GROUP LDAP result before it ever reaches + // the mapper. + var repo = new FakeSecurityRepository( + new List { Mapping(1, "SCADA-Admins", Roles.Admin) }, + new Dictionary>()); + var sut = new ScadaBridgeGroupRoleMapper(new RoleMapper(repo)); + + var mapping = await sut.MapAsync(Array.Empty(), CancellationToken.None); + + Assert.Empty(mapping.Roles); + var scope = Assert.IsType(mapping.Scope); + Assert.Empty(scope.Roles); + Assert.False(scope.IsSystemWideDeployment); + Assert.Empty(scope.PermittedSiteIds); + } + + [Fact] + public async Task MapAsync_GroupsMatchNoMapping_ReturnsNoRoles() + { + // A user WITH groups that match no configured LDAP-group→role mapping likewise + // yields zero roles (not an error) — the mapper is the authoritative empty-roles + // boundary now that the LDAP service no longer admits zero-group successes. + var repo = new FakeSecurityRepository( + new List { Mapping(1, "SCADA-Admins", Roles.Admin) }, + new Dictionary>()); + var sut = new ScadaBridgeGroupRoleMapper(new RoleMapper(repo)); + + var mapping = await sut.MapAsync(new[] { "some-unmapped-group" }, CancellationToken.None); + + Assert.Empty(mapping.Roles); + var scope = Assert.IsType(mapping.Scope); + Assert.Empty(scope.PermittedSiteIds); + Assert.False(scope.IsSystemWideDeployment); + } } #endregion diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs index 5041032d..48bf60c6 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/SecurityTests.cs @@ -5,6 +5,8 @@ using Microsoft.Extensions.DependencyInjection; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; using Microsoft.Extensions.Options; +using ZB.MOM.WW.Auth.Abstractions.Ldap; +using ZB.MOM.WW.Auth.Ldap; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Security; using ZB.MOM.WW.ScadaBridge.Commons.Entities.Sites; using ZB.MOM.WW.ScadaBridge.ConfigurationDatabase; @@ -13,70 +15,76 @@ using ZB.MOM.WW.ScadaBridge.Security; namespace ZB.MOM.WW.ScadaBridge.Security.Tests; -#region WP-6: LdapAuthService Tests +#region WP-6: LdapAuthService Tests (re-aimed at the shared ZB.MOM.WW.Auth.Ldap service) +// Task 1.2 cutover: ScadaBridge's bespoke LdapAuthService was replaced by the shared +// ZB.MOM.WW.Auth.Ldap.LdapAuthService (ScadaBridge was the donor for its hardened +// bind-then-search / RFC-4514+4515 escaping / fail-closed / service-account-bind- +// distinction / per-op-timeout semantics). The deep hygiene parity now lives in the +// LIBRARY's own test suite (ZB.MOM.WW.Auth.Ldap.Tests: LdapAuthServiceTests, +// LdapAuthServiceFailureTests, LdapEscapingTests), which exercises those paths through +// the library's internal ILdapConnection seam. Re-aiming them here would require that +// internal seam, which ScadaBridge cannot reach. The tests below instead pin the +// LIBRARY service's public, network-free surface and ScadaBridge's adopter wiring. public class LdapAuthServiceTests { - private static SecurityOptions CreateOptions(bool useTls = true, bool allowInsecure = false) => new() + private static LdapOptions CreateOptions(LdapTransport transport = LdapTransport.Ldaps, bool allowInsecure = false) => new() { - LdapServer = "ldap.example.com", - LdapPort = 636, - LdapUseTls = useTls, - AllowInsecureLdap = allowInsecure, - LdapSearchBase = "dc=example,dc=com", - JwtSigningKey = "test-key-that-is-long-enough-for-hmac-sha256-minimum" + Enabled = true, + Server = "ldap.example.com", + Port = 636, + Transport = transport, + AllowInsecure = allowInsecure, + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", }; [Fact] - public async Task AuthenticateAsync_EmptyUsername_ReturnsFailed() + public async Task AuthenticateAsync_Disabled_FailsWithDisabled_NoNetwork() { - var service = new LdapAuthService( - Options.Create(CreateOptions()), - NullLogger.Instance); + var options = CreateOptions() with { Enabled = false }; + var service = new LdapAuthService(options); - var result = await service.AuthenticateAsync("", "password"); - Assert.False(result.Success); - Assert.Contains("Username is required", result.ErrorMessage); + var result = await service.AuthenticateAsync("user", "password", CancellationToken.None); + + Assert.False(result.Succeeded); + Assert.Equal(LdapAuthFailure.Disabled, result.Failure); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + public async Task AuthenticateAsync_EmptyUsername_FailsBadCredentials_NoNetwork(string username) + { + // An empty/whitespace username is rejected before any connection is attempted, + // mapped to the enumeration-safe BadCredentials bucket (no "user is required" + // string leak). This is the donor's empty-username guard, preserved. + var service = new LdapAuthService(CreateOptions()); + + var result = await service.AuthenticateAsync(username, "password", CancellationToken.None); + + Assert.False(result.Succeeded); + Assert.Equal(LdapAuthFailure.BadCredentials, result.Failure); } [Fact] - public async Task AuthenticateAsync_EmptyPassword_ReturnsFailed() + public async Task AuthenticateAsync_ConnectionFailure_FailsClosed_NeverThrows() { - var service = new LdapAuthService( - Options.Create(CreateOptions()), - NullLogger.Instance); + // Point at a non-existent server: the library fails closed (never throws) and + // maps the unreachable directory to the system-side ServiceAccountBindFailed + // bucket — preserving the donor's "directory unavailable ⇒ login fails" rule. + var options = CreateOptions(LdapTransport.None, allowInsecure: true) with + { + Server = "nonexistent.invalid", + Port = 9999, + ConnectionTimeoutMs = 2_000, + }; + var service = new LdapAuthService(options); - var result = await service.AuthenticateAsync("user", ""); - Assert.False(result.Success); - Assert.Contains("Password is required", result.ErrorMessage); - } + var result = await service.AuthenticateAsync("user", "password", CancellationToken.None); - [Fact] - public async Task AuthenticateAsync_InsecureLdapNotAllowed_ReturnsFailed() - { - var service = new LdapAuthService( - Options.Create(CreateOptions(useTls: false, allowInsecure: false)), - NullLogger.Instance); - - var result = await service.AuthenticateAsync("user", "password"); - Assert.False(result.Success); - Assert.Contains("Insecure LDAP", result.ErrorMessage); - } - - [Fact] - public async Task AuthenticateAsync_ConnectionFailure_ReturnsFailed() - { - // Point to a non-existent server — connection should fail - var options = CreateOptions(); - options.LdapServer = "nonexistent.invalid"; - options.LdapPort = 9999; - - var service = new LdapAuthService( - Options.Create(options), - NullLogger.Instance); - - var result = await service.AuthenticateAsync("user", "password"); - Assert.False(result.Success); + Assert.False(result.Succeeded); + Assert.Equal(LdapAuthFailure.ServiceAccountBindFailed, result.Failure); } } @@ -433,11 +441,7 @@ public class SecurityReviewRegressionTests var services = new ServiceCollection(); services.AddLogging(); services.AddDataProtection(); - services.AddSecurity(); - // Security-020: the cookie PostConfigure reads SecurityOptions.Value, - // which triggers SecurityOptionsValidator — supply the required LDAP - // fields so the cookie wiring under test can be resolved. - ConfigureValidLdapDefaults(services); + services.AddSecurity(EmptyConfig()); using var provider = services.BuildServiceProvider(); var cookieOptions = provider @@ -451,18 +455,14 @@ public class SecurityReviewRegressionTests } /// - /// Security-020: supplies the minimum-valid LDAP fields so the cookie / - /// JWT wiring under test can be resolved without hitting - /// . Used by the cookie-policy - /// integration tests in this class, which only care about the cookie - /// options shape — not the LDAP fields. + /// Task 1.2: an empty + /// for AddSecurity(config). The cookie PostConfigure under test reads only the + /// non-LDAP fields (idle timeout / HTTPS-cookie policy), + /// and the library's LdapOptions ValidateOnStart only fires at host start (not on + /// BuildServiceProvider), so no LDAP config is needed to resolve the cookie wiring. /// - private static void ConfigureValidLdapDefaults(IServiceCollection services) => - services.Configure(o => - { - o.LdapServer = "ldap.example.com"; - o.LdapSearchBase = "dc=example,dc=com"; - }); + private static Microsoft.Extensions.Configuration.IConfiguration EmptyConfig() => + new Microsoft.Extensions.Configuration.ConfigurationBuilder().Build(); // --- CentralUI-005: cookie auth must use a sliding session window --- // Documented policy (CLAUDE.md Security & Auth): sliding refresh with a @@ -475,8 +475,7 @@ public class SecurityReviewRegressionTests var services = new ServiceCollection(); services.AddLogging(); services.AddDataProtection(); - services.AddSecurity(); - ConfigureValidLdapDefaults(services); + services.AddSecurity(EmptyConfig()); using var provider = services.BuildServiceProvider(); var cookieOptions = provider @@ -492,8 +491,7 @@ public class SecurityReviewRegressionTests var services = new ServiceCollection(); services.AddLogging(); services.AddDataProtection(); - services.AddSecurity(); - ConfigureValidLdapDefaults(services); + services.AddSecurity(EmptyConfig()); // The idle timeout drives the cookie's expiry window. services.Configure(o => o.IdleTimeoutMinutes = 30); @@ -511,8 +509,7 @@ public class SecurityReviewRegressionTests var services = new ServiceCollection(); services.AddLogging(); services.AddDataProtection(); - services.AddSecurity(); - ConfigureValidLdapDefaults(services); + services.AddSecurity(EmptyConfig()); services.Configure(o => o.IdleTimeoutMinutes = 45); using var provider = services.BuildServiceProvider(); @@ -524,53 +521,62 @@ public class SecurityReviewRegressionTests Assert.True(cookieOptions.SlidingExpiration); } - // --- Security-001: StartTLS transport must be reachable --- + // --- Security-001: transport security (now owned by the shared LdapOptions) --- [Fact] - public void SecurityOptions_LdapTransport_DefaultsToLdaps() + public void LdapOptions_Transport_DefaultsToLdaps() { - var options = new SecurityOptions(); - Assert.Equal(LdapTransport.Ldaps, options.LdapTransport); + // The shared LdapOptions keeps the donor's secure-by-default transport. + var options = new LdapOptions(); + Assert.Equal(LdapTransport.Ldaps, options.Transport); } [Fact] - public async Task AuthenticateAsync_StartTlsTransport_AttemptsConnection() + public async Task AuthenticateAsync_StartTlsTransport_NotBlockedByInsecureGuard() { - // With StartTLS selected the service must not be blocked by the insecure-LDAP - // guard and must reach the connection stage (which fails against a dead host). - // This proves the StartTLS path is reachable rather than dead code. - var options = new SecurityOptions + // With StartTLS selected the service is NOT rejected as insecure: it reaches the + // connection stage (which fails against a dead host) and fails closed to the + // system-side bucket — proving the StartTLS path is reachable, not dead code. + var options = new LdapOptions { - LdapServer = "nonexistent.invalid", - LdapPort = 389, - LdapTransport = LdapTransport.StartTls, - LdapSearchBase = "dc=example,dc=com" + Enabled = true, + Server = "nonexistent.invalid", + Port = 389, + Transport = LdapTransport.StartTls, + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", + ConnectionTimeoutMs = 2_000, }; - var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + var service = new LdapAuthService(options); - var result = await service.AuthenticateAsync("user", "password"); + var result = await service.AuthenticateAsync("user", "password", CancellationToken.None); - // Connection fails (host invalid) — but crucially NOT with the insecure-LDAP message. - Assert.False(result.Success); - Assert.DoesNotContain("Insecure LDAP", result.ErrorMessage ?? string.Empty); + // Connection fails (host invalid) — fail-closed to ServiceAccountBindFailed, NOT Disabled. + Assert.False(result.Succeeded); + Assert.Equal(LdapAuthFailure.ServiceAccountBindFailed, result.Failure); } [Fact] - public async Task AuthenticateAsync_NoTlsTransport_RejectedWithoutAllowInsecure() + public void LdapOptionsValidator_NoTlsTransport_RejectedWithoutAllowInsecure() { - var options = new SecurityOptions + // The donor blocked plaintext LDAP (Transport=None) unless AllowInsecure was set. + // That guard moved to the shared LdapOptionsValidator (registered with + // ValidateOnStart by AddZbLdapAuth), which now fails fast at boot. + var options = new LdapOptions { - LdapServer = "ldap.example.com", - LdapPort = 389, - LdapTransport = LdapTransport.None, - AllowInsecureLdap = false + Server = "ldap.example.com", + Port = 389, + Transport = LdapTransport.None, + AllowInsecure = false, + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", }; - var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + var validator = new LdapOptionsValidator(); - var result = await service.AuthenticateAsync("user", "password"); + var result = validator.Validate(name: null, options); - Assert.False(result.Success); - Assert.Contains("Insecure LDAP", result.ErrorMessage); + Assert.True(result.Failed); + Assert.Contains("AllowInsecure", result.FailureMessage); } } @@ -579,11 +585,18 @@ public class SecurityReviewRegressionTests #region Code Review Regression Tests — Security-004/005/006/007 /// -/// Regression tests for Security-004 (uid/cn attribute mismatch between search filter -/// and fallback DN), Security-005 (DN injection in the no-service-account fallback), -/// Security-006 (JWT issuer/audience checks disabled), and Security-007 (idle-timeout -/// claim reset on every token refresh). +/// Regression tests for Security-006 (JWT issuer/audience checks disabled) and +/// Security-007 (idle-timeout claim reset on every token refresh). /// +/// +/// Task 1.2 cutover: the former Security-004 (uid/cn attribute parity) and +/// Security-005 (no-service-account fallback-DN injection) cases tested ScadaBridge's +/// bespoke LdapAuthService.BuildFallbackUserDn / EscapeLdapDn static +/// helpers. The shared ZB.MOM.WW.Auth.Ldap service is search-then-bind only (no +/// no-service-account fallback DN) and its RFC-4514/4515 escaping parity is covered by +/// the library's own LdapEscapingTests / LdapAuthServiceTests. Those cases +/// were therefore removed here rather than re-aimed. +/// public class SecurityReviewRegressionTests2 { private static SecurityOptions JwtOptions() => new() @@ -597,58 +610,6 @@ public class SecurityReviewRegressionTests2 private static JwtTokenService CreateJwtService(SecurityOptions? options = null) => new(Options.Create(options ?? JwtOptions()), NullLogger.Instance); - // --- Security-004: search filter and fallback DN must use the same attribute --- - - [Fact] - public void BuildFallbackUserDn_UsesConfiguredUserIdAttribute() - { - // The default user-id attribute is "uid"; the fallback DN must use it, - // not a hard-coded "cn", so search-then-bind and direct-bind are interchangeable. - var dn = LdapAuthService.BuildFallbackUserDn("alice", "dc=example,dc=com", "uid"); - Assert.Equal("uid=alice,dc=example,dc=com", dn); - } - - [Fact] - public void BuildFallbackUserDn_HonoursNonDefaultUserIdAttribute() - { - var dn = LdapAuthService.BuildFallbackUserDn("alice", "dc=example,dc=com", "sAMAccountName"); - Assert.Equal("sAMAccountName=alice,dc=example,dc=com", dn); - } - - [Fact] - public void SecurityOptions_LdapUserIdAttribute_DefaultsToUid() - { - Assert.Equal("uid", new SecurityOptions().LdapUserIdAttribute); - } - - // --- Security-005: DN-component escaping must be applied to the username --- - - [Fact] - public void BuildFallbackUserDn_EscapesDnMetacharacters() - { - // A hostile username must not be able to alter the DN structure: the comma - // that would otherwise start a new RDN ("ou=admins") must be escaped so the - // whole string remains a single RDN value. - var dn = LdapAuthService.BuildFallbackUserDn("victim,ou=admins", "dc=example,dc=com", "uid"); - Assert.Equal(@"uid=victim\,ou=admins,dc=example,dc=com", dn); - // The comma from the username is backslash-escaped, so it does not act as an - // RDN separator: the only unescaped comma is the one joining RDN and base DN. - Assert.Contains(@"victim\,ou=admins", dn); - } - - [Fact] - public void EscapeLdapDn_EscapesAllRfc4514Specials() - { - var escaped = LdapAuthService.EscapeLdapDn("a,b+c\"d\\eg;h"); - Assert.Equal(@"a\,b\+c\""d\\e\g\;h", escaped); - } - - [Fact] - public void EscapeLdapDn_EscapesLeadingAndTrailingSpaces() - { - Assert.Equal(@"\ x \ ", LdapAuthService.EscapeLdapDn(" x ")); - } - // --- Security-006: JWT issuer/audience must be bound and validated --- [Fact] @@ -785,12 +746,13 @@ public class SecurityReviewRegressionTests3 // --- Security-009: LDAP connection timeout must be configurable and bounded --- [Fact] - public void SecurityOptions_LdapConnectionTimeout_HasSaneDefault() + public void LdapOptions_ConnectionTimeout_HasSaneDefault() { - var options = new SecurityOptions(); - // A positive, finite default so a hung LDAP server cannot pin a thread forever. - Assert.True(options.LdapConnectionTimeoutMs > 0); - Assert.True(options.LdapConnectionTimeoutMs <= 60_000, + var options = new LdapOptions(); + // A positive, finite default so a hung LDAP server cannot pin a thread forever + // (the donor's Security-009 safeguard, preserved on the shared LdapOptions). + Assert.True(options.ConnectionTimeoutMs > 0); + Assert.True(options.ConnectionTimeoutMs <= 60_000, "Default LDAP connection timeout should be bounded (<= 60s)."); } @@ -798,52 +760,37 @@ public class SecurityReviewRegressionTests3 public async Task AuthenticateAsync_UnreachableHost_FailsWithinConfiguredTimeout() { // A routable-but-non-responsive address would otherwise hang for the OS default - // (often minutes). With LdapConnectionTimeoutMs applied to the LdapConnection the - // call must give up promptly. 198.51.100.0/24 (TEST-NET-2, RFC 5737) is reserved - // and not routed, so the connect attempt stalls until the timeout fires. - var options = new SecurityOptions + // (often minutes). With ConnectionTimeoutMs applied to the LdapConnection the call + // must give up promptly. 198.51.100.0/24 (TEST-NET-2, RFC 5737) is reserved and not + // routed, so the connect attempt stalls until the timeout fires. + var options = new LdapOptions { - LdapServer = "198.51.100.1", - LdapPort = 389, - LdapTransport = LdapTransport.None, - AllowInsecureLdap = true, - LdapSearchBase = "dc=example,dc=com", - LdapConnectionTimeoutMs = 2_000 + Enabled = true, + Server = "198.51.100.1", + Port = 389, + Transport = LdapTransport.None, + AllowInsecure = true, + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", + ConnectionTimeoutMs = 2_000, }; - var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); + var service = new LdapAuthService(options); var sw = System.Diagnostics.Stopwatch.StartNew(); - var result = await service.AuthenticateAsync("user", "password"); + var result = await service.AuthenticateAsync("user", "password", CancellationToken.None); sw.Stop(); - Assert.False(result.Success); + Assert.False(result.Succeeded); // Generous ceiling: the 2s timeout plus scheduling/CI overhead, far below the // multi-minute OS default that an unconfigured connection would incur. Assert.True(sw.Elapsed < TimeSpan.FromSeconds(30), $"Authentication did not honour the LDAP connection timeout: took {sw.Elapsed}."); } - // --- Security-011: additional coverage for the no-service-account / DN paths --- - - [Fact] - public void BuildFallbackUserDn_NoSearchBase_ReturnsBareRdn() - { - var dn = LdapAuthService.BuildFallbackUserDn("alice", "", "uid"); - Assert.Equal("uid=alice", dn); - } - - [Fact] - public void EscapeLdapDn_LeadingHash_IsEscaped() - { - Assert.Equal(@"\#admin", LdapAuthService.EscapeLdapDn("#admin")); - } - - [Fact] - public void EscapeLdapDn_NullOrEmpty_ReturnedUnchanged() - { - Assert.Equal("", LdapAuthService.EscapeLdapDn("")); - Assert.Null(LdapAuthService.EscapeLdapDn(null!)); - } + // Note: the former Security-011 no-service-account fallback-DN / DN-escape static-helper + // cases (BuildFallbackUserDn / EscapeLdapDn) were removed: the shared service is + // search-then-bind only (no fallback DN), and its escaping parity is covered by the + // library's own LdapEscapingTests. } #endregion @@ -851,13 +798,18 @@ public class SecurityReviewRegressionTests3 #region Code Review Regression Tests — Security-012/013/014/015 /// -/// Regression tests for Security-012 (a partial LDAP outage during login — bind OK but -/// group search failing — silently yields a roleless authenticated session), -/// Security-013 (ExtractFirstRdnValue mis-parses group DNs containing an escaped -/// comma), Security-014 (RefreshToken re-issues a token without checking the idle -/// timeout, so an idle-expired session can be renewed indefinitely), and Security-015 -/// (a username with leading/trailing whitespace is not trimmed before use). +/// Regression tests for Security-014 (RefreshToken re-issues a token without +/// checking the idle timeout, so an idle-expired session can be renewed indefinitely). /// +/// +/// Task 1.2 cutover: the former Security-013 (ExtractFirstRdnValue escaped-comma +/// parsing) and Security-015 (username trim before use) cases tested ScadaBridge's +/// bespoke LdapAuthService.ExtractFirstRdnValue / NormalizeUsername / +/// BuildFallbackUserDn statics. Those behaviours are now owned by the shared +/// library and covered by its own tests — LdapEscapingTests.FirstRdnValue_IsEscapeAware +/// (escaped-comma RDN extraction) and LdapAuthServiceFailureTests +/// (whitespace-username → BadCredentials, no connection) — so they were removed here. +/// public class SecurityReviewRegressionTests4 { private static SecurityOptions JwtOptions() => new() @@ -928,85 +880,6 @@ public class SecurityReviewRegressionTests4 Assert.Null(refreshed); } - - // --- Security-013: ExtractFirstRdnValue must honour RFC 4514 escaped commas --- - - [Fact] - public void ExtractFirstRdnValue_EscapedComma_KeepsWholeGroupName() - { - // A CN that legitimately contains a comma is RFC 4514 backslash-escaped in the - // memberOf DN. The extracted group name must be the full unescaped CN value, - // not the fragment before the escaped comma. - var name = LdapAuthService.ExtractFirstRdnValue( - @"cn=Acme\, Inc Operators,ou=groups,dc=example,dc=com"); - Assert.Equal("Acme, Inc Operators", name); - } - - [Fact] - public void ExtractFirstRdnValue_PlainDn_ReturnsFirstRdnValue() - { - var name = LdapAuthService.ExtractFirstRdnValue("ou=SCADA-Admins,ou=groups,dc=example,dc=com"); - Assert.Equal("SCADA-Admins", name); - } - - [Fact] - public void ExtractFirstRdnValue_SingleRdn_ReturnsValue() - { - var name = LdapAuthService.ExtractFirstRdnValue("cn=Operators"); - Assert.Equal("Operators", name); - } - - [Fact] - public void ExtractFirstRdnValue_EscapedSpecials_AreUnescaped() - { - // RFC 4514 escape sequences (escaped '+', '"', '\\') must be unescaped in the - // returned value so it matches the configured LdapGroupName verbatim. - var name = LdapAuthService.ExtractFirstRdnValue( - @"cn=A\+B\\C,ou=groups,dc=example,dc=com"); - Assert.Equal(@"A+B\C", name); - } - - // --- Security-015: username must be trimmed before use --- - - [Fact] - public void BuildFallbackUserDn_TrimmedUsername_NoLeadingTrailingSpace() - { - // The whitespace-edge escaping in EscapeLdapDn only fires when whitespace is NOT - // trimmed. AuthenticateAsync trims first; this asserts the trimmed value yields - // a clean DN with no escaped edge spaces. - var dn = LdapAuthService.BuildFallbackUserDn("alice".Trim(), "dc=example,dc=com", "uid"); - Assert.Equal("uid=alice,dc=example,dc=com", dn); - } - - [Fact] - public async Task AuthenticateAsync_UsernameWithSurroundingWhitespace_StillRejectedForInsecure() - { - // Sanity guard: a padded but otherwise-valid username is not rejected by the - // IsNullOrWhiteSpace guard — it passes through to the (here, insecure-LDAP) path. - var options = new SecurityOptions - { - LdapServer = "ldap.example.com", - LdapPort = 389, - LdapTransport = LdapTransport.None, - AllowInsecureLdap = false - }; - var service = new LdapAuthService(Options.Create(options), NullLogger.Instance); - - var result = await service.AuthenticateAsync(" alice ", "password"); - - // Reaches the insecure-LDAP guard (not the empty-username guard) — proves the - // padded username is treated as a real, non-empty username. - Assert.False(result.Success); - Assert.Contains("Insecure LDAP", result.ErrorMessage); - } - - [Fact] - public void NormalizeUsername_TrimsLeadingAndTrailingWhitespace() - { - Assert.Equal("alice", LdapAuthService.NormalizeUsername(" alice ")); - Assert.Equal("alice", LdapAuthService.NormalizeUsername("alice")); - Assert.Equal("alice", LdapAuthService.NormalizeUsername("\talice\n")); - } } #endregion @@ -1014,79 +887,94 @@ public class SecurityReviewRegressionTests4 #region Code Review Regression Tests — Security-012 (partial LDAP outage) /// -/// Regression tests for Security-012: a partial LDAP outage during login (the user bind -/// succeeds but the subsequent group/attribute search fails) must fail the login per the -/// design's LDAP-failure rule, rather than returning an authenticated session with zero -/// roles. These exercise the seam through a stubbed group-lookup so the bind itself can -/// be treated as successful. +/// Security-012 (fail-closed group lookup) + the ScadaBridge-side +/// failure-code→message adapter. /// +/// +/// +/// Task 1.2 cutover: the donor's fail-closed group-lookup rule (a post-bind group +/// search failure FAILS the login rather than admitting a roleless session) is now +/// owned by the shared ZB.MOM.WW.Auth.Ldap service and asserted by its own +/// LdapAuthServiceFailureTests.GroupLookupFailed_WhenUserHasNoGroups / +/// ServiceAccountBindFailed_Distinctly_WhenServiceBindThrows. The donor's +/// bespoke BuildAuthResultFromGroupLookup / ServiceAccountBindException +/// statics no longer exist, so those cases were re-aimed at the shared +/// contract here. +/// +/// +/// DEVIATION (documented): the donor admitted an "authenticated with zero groups" +/// login when the group lookup SUCCEEDED but returned no groups +/// (BuildAuthResultFromGroupLookup_LookupSucceededNoGroups_IsAuthenticated). +/// The shared library is STRICTER and fail-closes a zero-group result to +/// — see +/// EmptyGroups_IsTreatedAsGroupLookupFailed_LibraryIsStricter below. The +/// downstream effect (a user with no mapped groups cannot obtain a useful session) is +/// unchanged: the donor's empty-group session mapped to zero roles and was denied by +/// every authorization policy anyway. The empty-GROUP-set-at-the-mapper case (a user +/// whose groups map to no roles) is covered by +/// ScadaBridgeGroupRoleMapperTests.MapAsync_EmptyGroups_*. +/// +/// public class Security012GroupLookupFailureTests { - private static SecurityOptions Options() => new() - { - LdapServer = "ldap.example.com", - LdapPort = 636, - LdapTransport = LdapTransport.Ldaps, - LdapSearchBase = "dc=example,dc=com" - }; - [Fact] - public void BuildAuthResultFromGroupLookup_LookupFailed_FailsTheLogin() + public void LdapAuthResult_Fail_GroupLookupFailed_IsFailedLoginWithNoGroups() { - // When the group lookup failed (directory partially unavailable mid-login) the - // result must be a FAILED login — not a success with an empty group list. - var result = LdapAuthService.BuildAuthResultFromGroupLookup( - username: "alice", - displayName: "Alice", - groups: new List(), - groupLookupSucceeded: false); + // The fail-closed outcome: a failed group lookup surfaces as a FAILED result with + // the GroupLookupFailed code and an empty group set (never a roleless success). + var result = LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed); - Assert.False(result.Success); - Assert.Null(result.Groups); - Assert.False(string.IsNullOrEmpty(result.ErrorMessage)); + Assert.False(result.Succeeded); + Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); + Assert.Empty(result.Groups); } [Fact] - public void BuildAuthResultFromGroupLookup_LookupSucceededNoGroups_IsAuthenticated() + public void LdapAuthResult_Success_CarriesGroups() { - // A genuine "user belongs to no mapped groups" outcome must remain a successful - // login — it must be distinguishable from a failed lookup. - var result = LdapAuthService.BuildAuthResultFromGroupLookup( - username: "alice", - displayName: "Alice", - groups: new List(), - groupLookupSucceeded: true); + var result = LdapAuthResult.Success("alice", "Alice", new[] { "SCADA-Admins" }); - Assert.True(result.Success); - Assert.NotNull(result.Groups); - Assert.Empty(result.Groups!); - } - - [Fact] - public void BuildAuthResultFromGroupLookup_LookupSucceededWithGroups_CarriesGroups() - { - var result = LdapAuthService.BuildAuthResultFromGroupLookup( - username: "alice", - displayName: "Alice", - groups: new List { "SCADA-Admins" }, - groupLookupSucceeded: true); - - Assert.True(result.Success); + Assert.True(result.Succeeded); Assert.Equal(new[] { "SCADA-Admins" }, result.Groups); + Assert.Null(result.Failure); } [Fact] - public void ServiceAccountBindException_DoesNotInheritLdapException_SoCatchOrderIsCorrect() + public void EmptyGroups_IsTreatedAsGroupLookupFailed_LibraryIsStricter() { - // Security-019: the LdapAuthService catch chain matches - // ServiceAccountBindException *before* the generic LdapException catch. That only - // produces the distinct "Authentication service is misconfigured" message if the - // exception type is NOT an LdapException subtype (otherwise it would be caught - // by the broader handler first regardless of ordering). - var ex = new ServiceAccountBindException(new InvalidOperationException("boom")); + // Documents the deviation: the shared library NEVER admits a zero-group success. + // The success factory requires >=1 group at the service level; a fabricated + // zero-group "success" is not a state the service produces. We pin the contract + // the cutover relies on: a zero-group outcome is the GroupLookupFailed failure. + var failClosed = LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed); + Assert.False(failClosed.Succeeded); - Assert.IsNotType(ex); - Assert.IsType(ex.InnerException); + // And the donor's "Security-012 directory-unavailable" message is preserved for it. + Assert.Equal( + LdapAuthFailureMessages.DirectoryUnavailable, + LdapAuthFailureMessages.ToMessage(LdapAuthFailure.GroupLookupFailed)); + } + + [Theory] + [InlineData(LdapAuthFailure.BadCredentials, LdapAuthFailureMessages.InvalidCredentials)] + [InlineData(LdapAuthFailure.UserNotFound, LdapAuthFailureMessages.InvalidCredentials)] + [InlineData(LdapAuthFailure.AmbiguousUser, LdapAuthFailureMessages.Misconfigured)] + [InlineData(LdapAuthFailure.ServiceAccountBindFailed, LdapAuthFailureMessages.Misconfigured)] + [InlineData(LdapAuthFailure.GroupLookupFailed, LdapAuthFailureMessages.DirectoryUnavailable)] + [InlineData(LdapAuthFailure.Disabled, LdapAuthFailureMessages.Disabled)] + public void LdapAuthFailureMessages_MapsEachFailure_ToDonorEquivalentMessage( + LdapAuthFailure failure, string expected) + { + // Security-019 framing preserved: user-credential failures are enumeration-safe + // ("Invalid username or password."), system-side faults point the OPERATOR at the + // cause ("...misconfigured...") rather than blaming user input. + Assert.Equal(expected, LdapAuthFailureMessages.ToMessage(failure)); + } + + [Fact] + public void LdapAuthFailureMessages_NullFailure_FallsBackToGeneric() + { + Assert.Equal(LdapAuthFailureMessages.Generic, LdapAuthFailureMessages.ToMessage(null)); } } @@ -1218,98 +1106,94 @@ public class AuthorizationPolicyTests #endregion -#region Code Review Regression Tests — Security-020 +#region Code Review Regression Tests — Security-020 (LDAP fail-fast validation) /// -/// Security-020: must reject empty -/// / -/// at startup with a clear, key-naming message, so a typo'd appsettings section -/// fails fast at boot instead of surfacing minutes/hours later as a generic -/// LDAP error on the first real login. +/// Security-020: a missing/empty required LDAP field must fail fast at startup with a +/// clear, field-naming message, so a typo'd appsettings section fails at boot instead +/// of surfacing minutes/hours later as a generic LDAP error on the first real login. /// +/// +/// Task 1.2/1.4 cutover: the donor's SecurityOptionsValidator (which checked +/// Security:LdapServer + Security:LdapSearchBase non-empty) was replaced by +/// the shared ZB.MOM.WW.Auth.Ldap.LdapOptionsValidator, registered with +/// ValidateOnStart() by AddZbLdapAuth inside AddSecurity. The shared +/// validator is STRICTER — it also requires ServiceAccountDn and rejects plaintext +/// transport without AllowInsecure — so the boot-time fail-fast guarantee is +/// preserved and broadened. These tests pin the shared validator's behaviour and the +/// ScadaBridge wiring that registers it. +/// public class SecurityOptionsValidatorTests { - private static SecurityOptions ValidOptions() => new() + private static LdapOptions ValidOptions() => new() { - LdapServer = "ldap.example.com", - LdapSearchBase = "dc=example,dc=com", + Server = "ldap.example.com", + SearchBase = "dc=example,dc=com", + ServiceAccountDn = "cn=admin,dc=example,dc=com", + Transport = LdapTransport.Ldaps, }; [Fact] public void Validate_AllRequiredFieldsSet_Succeeds() { - var validator = new SecurityOptionsValidator(); - - var result = validator.Validate(name: null, ValidOptions()); - + var result = new LdapOptionsValidator().Validate(name: null, ValidOptions()); Assert.True(result.Succeeded); } [Theory] [InlineData("")] [InlineData(" ")] - public void Validate_EmptyOrWhitespaceLdapServer_Fails(string ldapServer) + public void Validate_EmptyOrWhitespaceServer_Fails(string server) { - var options = ValidOptions(); - options.LdapServer = ldapServer; - var validator = new SecurityOptionsValidator(); + var options = ValidOptions() with { Server = server }; - var result = validator.Validate(name: null, options); + var result = new LdapOptionsValidator().Validate(name: null, options); Assert.True(result.Failed); - // Must name the full Section:Field key so the operator can find it. - Assert.Contains("Security:LdapServer", result.FailureMessage); + Assert.Contains(nameof(LdapOptions.Server), result.FailureMessage); } [Theory] [InlineData("")] [InlineData(" ")] - public void Validate_EmptyOrWhitespaceLdapSearchBase_Fails(string ldapSearchBase) + public void Validate_EmptyOrWhitespaceSearchBase_Fails(string searchBase) { - var options = ValidOptions(); - options.LdapSearchBase = ldapSearchBase; - var validator = new SecurityOptionsValidator(); + var options = ValidOptions() with { SearchBase = searchBase }; - var result = validator.Validate(name: null, options); + var result = new LdapOptionsValidator().Validate(name: null, options); Assert.True(result.Failed); - Assert.Contains("Security:LdapSearchBase", result.FailureMessage); + Assert.Contains(nameof(LdapOptions.SearchBase), result.FailureMessage); } [Fact] - public void Validate_BothRequiredFieldsEmpty_ReportsBoth() + public void Validate_EmptyServiceAccountDn_Fails() { - var options = new SecurityOptions - { - LdapServer = string.Empty, - LdapSearchBase = string.Empty, - }; - var validator = new SecurityOptionsValidator(); + // Stricter than the donor: an empty ServiceAccountDn would bind anonymously and + // defeat search-then-bind, so the shared validator rejects it at boot. + var options = ValidOptions() with { ServiceAccountDn = string.Empty }; - var result = validator.Validate(name: null, options); + var result = new LdapOptionsValidator().Validate(name: null, options); Assert.True(result.Failed); - // Both keys named — the operator should not need to re-run after - // fixing the first one to discover the second is also missing. - Assert.Contains("Security:LdapServer", result.FailureMessage); - Assert.Contains("Security:LdapSearchBase", result.FailureMessage); + Assert.Contains(nameof(LdapOptions.ServiceAccountDn), result.FailureMessage); } [Fact] - public void AddSecurity_RegistersSecurityOptionsValidator() + public void AddSecurity_RegistersLdapOptionsValidator_WithValidateOnStart() { var services = new ServiceCollection(); services.AddLogging(); services.AddDataProtection(); - services.AddSecurity(); + services.AddSecurity( + new Microsoft.Extensions.Configuration.ConfigurationBuilder().Build()); using var provider = services.BuildServiceProvider(); - // The validator participates in IValidateOptions — - // registration is the load-bearing wiring that makes Security-020 - // ValidateOnStart() actually fire. - var validators = provider.GetServices>().ToList(); - Assert.Contains(validators, v => v is SecurityOptionsValidator); + // The shared validator participates in IValidateOptions — + // registration is the load-bearing wiring that makes ValidateOnStart() fire. + var validators = provider.GetServices>().ToList(); + Assert.Contains(validators, v => v is LdapOptionsValidator); } } diff --git a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj index 257ef86c..057bfee4 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj +++ b/tests/ZB.MOM.WW.ScadaBridge.Security.Tests/ZB.MOM.WW.ScadaBridge.Security.Tests.csproj @@ -19,6 +19,9 @@ + + +