From 7d78a1f515ea8f02d869936b51154a7e1305105c Mon Sep 17 00:00:00 2001 From: Alejandro Rosales Date: Fri, 18 Sep 2026 16:46:56 -0600 Subject: [PATCH] Make Auth Broker authoritative for managed passwords --- docs/architecture.md | 9 +++ docs/release-0.6.8.md | 17 ++++++ docs/security.md | 3 + scripts/Deploy-AuthBroker.ps1 | 16 ++++- src/SGU.AuthBroker/BrokerEventIds.cs | 1 + .../Services/ActiveDirectorySynchronizer.cs | 60 +++++++++++++++++++ tests/AuthBrokerPasswordAuthority.Tests.ps1 | 21 +++++++ .../ActiveDirectorySynchronizerTests.cs | 28 +++++++++ 8 files changed, 154 insertions(+), 1 deletion(-) create mode 100644 docs/release-0.6.8.md create mode 100644 tests/AuthBrokerPasswordAuthority.Tests.ps1 diff --git a/docs/architecture.md b/docs/architecture.md index 64c43b0..7008d5d 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -62,6 +62,15 @@ plus six digits. It searches `BaseDn` by `sAMAccountName`, creates the user when absent, moves it to the mapped OU when required, sets `userPrincipalName`, and passes the submitted password directly to ADSI `SetPassword`. +Before an account becomes usable, the broker applies explicit deny ACEs for the +Active Directory `Change Password` extended right to SELF and Everyone. Users +beneath `OU=Usuarios-SGU` therefore cannot replace the synchronized password +from Windows, Ctrl+Alt+Delete, LDAP or another client. The broker's +administrative `SetPassword` operation uses the separate `Reset Password` right +and remains able to synchronize the current institutional credential after each +successful SGU authentication. Repeated synchronizations detect the existing +ACEs and do not duplicate them. + When the authenticated HTML exposes recognized stable IDs, the broker also updates the applicable `displayName`, `givenName`, `sn`, `mail`, `title`, `department`, `employeeType`, `employeeID`, `streetAddress`, `l`, `st`, and diff --git a/docs/release-0.6.8.md b/docs/release-0.6.8.md new file mode 100644 index 0000000..24a9d64 --- /dev/null +++ b/docs/release-0.6.8.md @@ -0,0 +1,17 @@ +# SGU Credential Provider 0.6.8 + +Esta versión declara al Auth Broker como la única autoridad de contraseñas para +todas las cuentas administradas bajo `OU=Usuarios-SGU`. + +- Cada creación, movimiento o actualización de una cuenta agrega de forma + idempotente las denegaciones SELF y Everyone para el derecho extendido de + Active Directory `Change Password`. +- El usuario no puede cambiar la contraseña desde Windows, Ctrl+Alt+Delete, + LDAP ni herramientas equivalentes. +- El broker conserva el derecho administrativo separado `Reset Password` y + continúa sincronizando la contraseña institucional exacta mediante + `SetPassword` después de una autenticación SGU válida. +- El despliegue del broker recorre todas las cuentas existentes en + `Usuarios-SGU` y corrige aquellas que todavía permiten cambios directos. + +No se modifica la contraseña institucional ni se almacena una copia adicional. diff --git a/docs/security.md b/docs/security.md index 85d136e..17a1f66 100644 --- a/docs/security.md +++ b/docs/security.md @@ -18,6 +18,9 @@ the same source list. RDP uses a separate allowlist. See `Marshal.ZeroFreeGlobalAllocUnicode`; managed references are released as soon as each request completes. - The broker uses the exact received value for both NTLM and AD `SetPassword`. +- Managed `Usuarios-SGU` accounts deny the SELF and Everyone `Change Password` + extended right. Only an administrator or the broker through the separate + `Reset Password` right can replace the AD password. - There is no HMAC password, pepper, local password cache, Supabase password, or other derived credential in this Windows path. - Neither application logs request bodies or passwords. Deployment configuration diff --git a/scripts/Deploy-AuthBroker.ps1 b/scripts/Deploy-AuthBroker.ps1 index bde2b2d..c3304b2 100644 --- a/scripts/Deploy-AuthBroker.ps1 +++ b/scripts/Deploy-AuthBroker.ps1 @@ -201,6 +201,19 @@ if ($RemoteDesktopGroupDn) { } } +$managedUsersPasswordChangeCorrected = 0 +if ($PSCmdlet.ShouldProcess($usersOuDn, 'Deny direct password changes for every managed SGU user')) { + $managedUsers = @(Get-ADUser -Filter * -SearchBase $usersOuDn -SearchScope Subtree ` + -Properties CannotChangePassword -Server $LdapHost -ErrorAction Stop) + foreach ($managedUser in $managedUsers) { + if (-not $managedUser.CannotChangePassword) { + Set-ADAccountControl -Identity $managedUser.DistinguishedName ` + -CannotChangePassword $true -Server $LdapHost -Confirm:$false + $managedUsersPasswordChangeCorrected++ + } + } +} + foreach ($file in @('SGU.AuthBroker.exe', 'SGU.AuthBroker.dll', 'appsettings.json')) { if (-not (Test-Path -LiteralPath (Join-Path $PublishPath $file))) { throw "PublishPath is missing $file." @@ -356,4 +369,5 @@ if ($PSCmdlet.ShouldProcess($installPath, 'Install the SGU Authentication Broker } Get-Service -Name $serviceName | Select-Object Name, Status, StartType, - @{ Name = 'EventLog'; Expression = { $brokerEventLogName } } + @{ Name = 'EventLog'; Expression = { $brokerEventLogName } }, + @{ Name = 'ExistingUsersPasswordChangeDenied'; Expression = { $managedUsersPasswordChangeCorrected } } diff --git a/src/SGU.AuthBroker/BrokerEventIds.cs b/src/SGU.AuthBroker/BrokerEventIds.cs index d48819c..c06eff7 100644 --- a/src/SGU.AuthBroker/BrokerEventIds.cs +++ b/src/SGU.AuthBroker/BrokerEventIds.cs @@ -22,4 +22,5 @@ internal static class BrokerEventIds internal static readonly EventId DirectoryOptionalMetadataFailure = new(1301, nameof(DirectoryOptionalMetadataFailure)); internal static readonly EventId DirectoryGroupMembershipFailure = new(1302, nameof(DirectoryGroupMembershipFailure)); internal static readonly EventId DirectoryRoleGroupMembershipAdded = new(1303, nameof(DirectoryRoleGroupMembershipAdded)); + internal static readonly EventId DirectoryPasswordChangeDenied = new(1304, nameof(DirectoryPasswordChangeDenied)); } diff --git a/src/SGU.AuthBroker/Services/ActiveDirectorySynchronizer.cs b/src/SGU.AuthBroker/Services/ActiveDirectorySynchronizer.cs index b0843d4..e26adce 100644 --- a/src/SGU.AuthBroker/Services/ActiveDirectorySynchronizer.cs +++ b/src/SGU.AuthBroker/Services/ActiveDirectorySynchronizer.cs @@ -1,5 +1,7 @@ using System.Collections.Concurrent; using System.DirectoryServices; +using System.Security.AccessControl; +using System.Security.Principal; using SGU.AuthBroker.Core.Directory; using SGU.AuthBroker.Core.Identity; using SGU.AuthBroker.Core.Profiles; @@ -15,6 +17,12 @@ public sealed class ActiveDirectorySynchronizer( private const int InfoAttributeMaximumLength = 1024; private const int AccountDisabled = 0x0002; private const int NormalAccount = 0x0200; + private static readonly Guid ChangePasswordExtendedRight = + new("AB721A53-1E2F-11D0-9819-00AA0040529B"); + private static readonly SecurityIdentifier EveryoneSid = + new(WellKnownSidType.WorldSid, null); + private static readonly SecurityIdentifier SelfSid = + new(WellKnownSidType.SelfSid, null); private static readonly AuthenticationTypes BindFlags = AuthenticationTypes.Secure | AuthenticationTypes.Signing | AuthenticationTypes.Sealing; @@ -117,6 +125,12 @@ public sealed class ActiveDirectorySynchronizer( // account without its required classification. EnsureRoleGroupMembership(user, identity); + // SGU remains the password authority. Deny the user's Change + // Password extended right before making the account usable. This + // does not deny the broker's administrative Reset Password right, + // which ADSI SetPassword uses for each successful authentication. + EnsureCannotChangePassword(user, identity.UserName); + // The exact institutional password received by the broker is passed to AD. // It is not derived, transformed, written to disk, or included in logs. user.Invoke("SetPassword", [password]); @@ -143,6 +157,52 @@ public sealed class ActiveDirectorySynchronizer( } } + private void EnsureCannotChangePassword(DirectoryEntry user, string institutionalUser) + { + user.Options!.SecurityMasks = SecurityMasks.Dacl; + ActiveDirectorySecurity security = user.ObjectSecurity; + if (!EnsureCannotChangePassword(security)) + { + return; + } + + user.ObjectSecurity = security; + user.CommitChanges(); + logger.LogInformation( + BrokerEventIds.DirectoryPasswordChangeDenied, + "Denied direct password changes for managed Active Directory user {InstitutionalUser}; SGU Auth Broker remains the password authority.", + institutionalUser); + } + + internal static bool EnsureCannotChangePassword(ActiveDirectorySecurity security) + { + bool changed = false; + foreach (SecurityIdentifier identity in new[] { EveryoneSid, SelfSid }) + { + bool exists = security + .GetAccessRules(includeExplicit: true, includeInherited: false, typeof(SecurityIdentifier)) + .OfType() + .Any(rule => + rule.AccessControlType == AccessControlType.Deny && + rule.IdentityReference.Equals(identity) && + rule.ObjectType == ChangePasswordExtendedRight && + (rule.ActiveDirectoryRights & ActiveDirectoryRights.ExtendedRight) != 0); + if (exists) + { + continue; + } + + security.AddAccessRule(new ActiveDirectoryAccessRule( + identity, + ActiveDirectoryRights.ExtendedRight, + AccessControlType.Deny, + ChangePasswordExtendedRight)); + changed = true; + } + + return changed; + } + private static void TryApplyProfile( DirectoryEntry user, UserIdentity identity, diff --git a/tests/AuthBrokerPasswordAuthority.Tests.ps1 b/tests/AuthBrokerPasswordAuthority.Tests.ps1 new file mode 100644 index 0000000..521a07e --- /dev/null +++ b/tests/AuthBrokerPasswordAuthority.Tests.ps1 @@ -0,0 +1,21 @@ +$repositoryRoot = (Resolve-Path (Join-Path $PSScriptRoot '..')).Path +$synchronizerPath = Join-Path $repositoryRoot 'src\SGU.AuthBroker\Services\ActiveDirectorySynchronizer.cs' +$deployPath = Join-Path $repositoryRoot 'scripts\Deploy-AuthBroker.ps1' +$synchronizer = Get-Content -LiteralPath $synchronizerPath -Raw +$deploy = Get-Content -LiteralPath $deployPath -Raw + +Describe 'SGU Auth Broker password authority' { + It 'denies the Change Password extended right to SELF and Everyone before SetPassword' { + $synchronizer | Should Match 'AB721A53-1E2F-11D0-9819-00AA0040529B' + $synchronizer | Should Match 'WellKnownSidType\.WorldSid' + $synchronizer | Should Match 'WellKnownSidType\.SelfSid' + $synchronizer | Should Match 'AccessControlType\.Deny' + $synchronizer.IndexOf('EnsureCannotChangePassword(user', [StringComparison]::Ordinal) | + Should BeLessThan $synchronizer.IndexOf('user.Invoke("SetPassword"', [StringComparison]::Ordinal) + } + + It 'repairs every existing account below Usuarios-SGU during broker deployment' { + $deploy | Should Match 'Get-ADUser.*-SearchBase \$usersOuDn.*-SearchScope Subtree' + $deploy | Should Match '(?s)Set-ADAccountControl.*-CannotChangePassword \$true' + } +} diff --git a/tests/SGU.AuthBroker.Tests/ActiveDirectorySynchronizerTests.cs b/tests/SGU.AuthBroker.Tests/ActiveDirectorySynchronizerTests.cs index 8c9196d..dbdc83a 100644 --- a/tests/SGU.AuthBroker.Tests/ActiveDirectorySynchronizerTests.cs +++ b/tests/SGU.AuthBroker.Tests/ActiveDirectorySynchronizerTests.cs @@ -1,3 +1,6 @@ +using System.DirectoryServices; +using System.Security.AccessControl; +using System.Security.Principal; using SGU.AuthBroker.Core.Profiles; using SGU.AuthBroker.Services; using Xunit; @@ -31,4 +34,29 @@ public sealed class ActiveDirectorySynchronizerTests Assert.Null(updated); } + + [Fact] + public void CannotChangePasswordRulesAreCompleteAndIdempotent() + { + ActiveDirectorySecurity security = new(); + + Assert.True(ActiveDirectorySynchronizer.EnsureCannotChangePassword(security)); + Assert.False(ActiveDirectorySynchronizer.EnsureCannotChangePassword(security)); + + Guid changePasswordRight = new("AB721A53-1E2F-11D0-9819-00AA0040529B"); + ActiveDirectoryAccessRule[] rules = security + .GetAccessRules(includeExplicit: true, includeInherited: false, typeof(SecurityIdentifier)) + .OfType() + .Where(rule => + rule.AccessControlType == AccessControlType.Deny && + rule.ObjectType == changePasswordRight && + (rule.ActiveDirectoryRights & ActiveDirectoryRights.ExtendedRight) != 0) + .ToArray(); + + Assert.Equal(2, rules.Length); + Assert.Contains(rules, rule => rule.IdentityReference.Equals( + new SecurityIdentifier(WellKnownSidType.WorldSid, null))); + Assert.Contains(rules, rule => rule.IdentityReference.Equals( + new SecurityIdentifier(WellKnownSidType.SelfSid, null))); + } }