diff --git a/ZB.MOM.WW.Auth/README.md b/ZB.MOM.WW.Auth/README.md index a7457f2..ca2d5cf 100644 --- a/ZB.MOM.WW.Auth/README.md +++ b/ZB.MOM.WW.Auth/README.md @@ -41,6 +41,11 @@ All five packages are versioned **lockstep** from `Directory.Build.props`. The c - **`{0}` is now RFC 4514-escaped in a DN template**, so a typed name such as `x,ou=Admins` cannot rewrite the bind DN. - **Behaviour change for consumers:** AD with a down-level or UPN template is unaffected for accounts whose bound identity and searched entry agree (AD has supported Who-am-I since Windows Server 2003). A UPN template now refuses accounts whose UPN prefix differs from `sAMAccountName` instead of risking the wrong account. A **non-AD directory** in `User` mode needs either Who-am-I support or a DN template that yields the entry's DN *exactly as the search returns it* — for GLAuth that is `cn={0},ou=,ou=users,`, not `cn={0},ou=,`. ServiceAccount mode is unchanged. - The validator's `UserBindTemplate` message now leads with `EXAMPLE\{0}`. No `Abstractions` contract change (XML docs only). +- **AD forests (scadaproj#50).** `sAMAccountName` is unique per *domain*, so with a search base that spans a forest (a Global Catalog) a matching `sAMAccountName` alone could be another domain's account of the same name. For AD's `u:DOMAIN\name` answer the entry must now also be proven to be that domain's account: + - where the directory computes `msDS-PrincipalName` for the entry (a base read; AD's `DOMAIN\name`, derived from `objectSid`), it must equal the Who-am-I answer, domain included; + - otherwise the name must have exactly one holder under `SearchBase`, and it must be the entry found — no extra search is made when `UserNameAttribute` is `sAMAccountName` and the user typed the bound name, because the user search already proved that. Two holders → `GroupLookupFailed`. + - A single-domain AD (BTDB Manager) is unaffected. Not yet live-verified against AD — the fake connection covers the paths; GLAuth has no Who-am-I. +- **Canonical username from a multi-valued attribute (Auth-009).** `LdapAuthResult.Username` is now the `UserNameAttribute` value that equals the typed name ignoring case — `cn: [John Smith, jsmith]` signs `jsmith` in as `jsmith`, not whichever value the server lists first. The first value is used only when none matches. Both bind modes. ### 0.4.1 — LDAP review fixes diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs index 0dd2239..e57d6d0 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/ILdapConnection.cs @@ -46,6 +46,14 @@ internal interface ILdapConnection : IDisposable /// Executes a subtree search and returns all matching entries. IReadOnlyList Search(string searchBase, string filter, IReadOnlyList attributes); + /// + /// Reads one entry by DN (a base-scope search) and returns the requested attributes the server + /// sent, or when the entry does not exist. Throws LdapException on + /// any other failure. Used for constructed attributes such as AD's msDS-PrincipalName, + /// which a server may compute for a base read but not for a subtree search. + /// + LdapSearchEntry? Read(string dn, IReadOnlyList attributes); + /// /// RFC 4532 "Who am I?": the authorization identity the directory associates with the current /// bind, as the server sends it (dn:<dn>, u:DOMAIN\name, or empty for an diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs index cf4e9ef..b5f4515 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/Internal/NovellLdapConnection.cs @@ -102,6 +102,27 @@ internal sealed class NovellLdapConnection : ILdapConnection return ReadEntries(results); } + /// + public LdapSearchEntry? Read(string dn, IReadOnlyList attributes) + { + var results = _conn.Search( + dn, + LdapConnection.ScopeBase, + "(objectClass=*)", + attributes.ToArray(), + typesOnly: false); + + try + { + var entries = ReadEntries(results); + return entries.Count == 0 ? null : entries[0]; + } + catch (LdapException ex) when (ex.ResultCode == LdapException.NoSuchObject) + { + return null; + } + } + /// public string WhoAmI() { diff --git a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs index 35f9e17..ad0d0ed 100644 --- a/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs +++ b/ZB.MOM.WW.Auth/src/ZB.MOM.WW.Auth.Ldap/LdapAuthService.cs @@ -369,7 +369,7 @@ public sealed class LdapAuthService : ILdapAuthService if (entries.Count != 1) return LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed); - if (!IsBoundAccount(entries[0], bound)) + if (!IsBoundAccount(conn, entries[0], bound, username)) { _logger?.LogWarning( "LDAP user bind authenticated {BoundIdentity} but the entry found for the typed name is " + @@ -384,13 +384,18 @@ public sealed class LdapAuthService : ILdapAuthService /// The AD attribute that an RFC 4532 u:DOMAIN\name authorization identity names. private const string SamAccountNameAttribute = "sAMAccountName"; + /// AD's constructed attribute holding a principal's DOMAIN\name, derived from its objectSid. + private const string PrincipalNameAttribute = "msDS-PrincipalName"; + /// /// The account a user bind authenticated: either its DN, or (AD's Who-am-I form) its - /// sAMAccountName. Exactly one member is set. + /// sAMAccountName plus, when the answer carried one, its NetBIOS . + /// Exactly one of / is set. /// - private sealed record BoundIdentity(string? Dn, string? AccountName) + private sealed record BoundIdentity(string? Dn, string? AccountName, string? Domain = null) { - public override string ToString() => Dn is not null ? $"dn:{Dn}" : $"u:{AccountName}"; + public override string ToString() => + Dn is not null ? $"dn:{Dn}" : Domain is null ? $"u:{AccountName}" : $@"u:{Domain}\{AccountName}"; } /// A template is a DN template when it contains an attribute assertion (=); UPN and down-level forms never do. @@ -424,11 +429,15 @@ public sealed class LdapAuthService : ILdapAuthService if (authzId.StartsWith("u:", StringComparison.OrdinalIgnoreCase) && authzId.Length > 2) { var account = authzId[2..].Trim(); + string? domain = null; var slash = account.LastIndexOf('\\'); if (slash >= 0) + { + domain = account[..slash].Trim(); account = account[(slash + 1)..]; + } if (account.Length > 0) - return new BoundIdentity(null, account); + return new BoundIdentity(null, account, string.IsNullOrEmpty(domain) ? null : domain); } return dnTemplate ? new BoundIdentity(bindName, null) : null; @@ -437,9 +446,11 @@ public sealed class LdapAuthService : ILdapAuthService /// /// only when is the account that bound: its DN /// equals the bound DN (RFC 4514 distinguishedNameMatch, case- and space-insensitive), or - /// its sAMAccountName equals the Who-am-I account name (case-insensitive, as AD compares it). + /// — for AD's u:DOMAIN\name answer — it is provably that domain's account of that name + /// (see ). /// - private static bool IsBoundAccount(LdapSearchEntry entry, BoundIdentity bound) + private bool IsBoundAccount( + ZB.MOM.WW.Auth.Ldap.Internal.ILdapConnection conn, LdapSearchEntry entry, BoundIdentity bound, string username) { if (bound.Dn is not null) { @@ -454,9 +465,97 @@ public sealed class LdapAuthService : ILdapAuthService } } - return entry.Attributes.TryGetValue(SamAccountNameAttribute, out var values) - && values.Count == 1 - && string.Equals(values[0], bound.AccountName, StringComparison.OrdinalIgnoreCase); + return IsBoundAdAccount(conn, entry, bound, username); + } + + /// + /// The u:DOMAIN\name case. A matching sAMAccountName is necessary but not enough: + /// sAMAccountName is unique per DOMAIN, so a search base that spans a forest (a Global + /// Catalog) can find another domain's account of the same name and hand back its groups + /// (scadaproj#50). In order of strength: + /// + /// Where the directory offers AD's constructed msDS-PrincipalName for the entry — + /// its DOMAIN\name, computed from the entry's objectSid — it must equal the + /// Who-am-I answer, domain included. That is the decisive, SID-anchored check. (The bound + /// account's own objectSid cannot be compared directly: Who-am-I does not return it.) + /// Otherwise the name must have exactly ONE holder under , + /// and that holder's DN must be the entry's: two holders mean which one bound cannot be proven, + /// so the sign-in fails closed. The user search already proves this when it was itself + /// (sAMAccountName=<the bound name>) and found one entry, so no second search is + /// made then. Residual: a search base that covers another domain's holder but NOT the bound + /// account's own domain is not detectable without msDS-PrincipalName. + /// + /// + private bool IsBoundAdAccount( + ZB.MOM.WW.Auth.Ldap.Internal.ILdapConnection conn, LdapSearchEntry entry, BoundIdentity bound, string username) + { + if (!entry.Attributes.TryGetValue(SamAccountNameAttribute, out var values) + || values.Count != 1 + || !string.Equals(values[0], bound.AccountName, StringComparison.OrdinalIgnoreCase)) + { + return false; + } + + if (bound.Domain is not null && ReadPrincipalName(conn, entry.Dn) is { } principal) + return string.Equals(principal, $@"{bound.Domain}\{bound.AccountName}", StringComparison.OrdinalIgnoreCase); + + var searchedOnTheBoundName = + string.Equals(_options.UserNameAttribute, SamAccountNameAttribute, StringComparison.OrdinalIgnoreCase) + && string.Equals(username, bound.AccountName, StringComparison.OrdinalIgnoreCase); + if (searchedOnTheBoundName) + return true; + + IReadOnlyList holders; + try + { + holders = conn.Search( + _options.SearchBase, + $"({SamAccountNameAttribute}={LdapEscaping.Filter(bound.AccountName!)})", + new[] { SamAccountNameAttribute }); + } + catch (LdapException) + { + // Same bucket as the user search: the directory could not answer, which is system-side. + throw new StageFailure(LdapAuthFailure.ServiceAccountBindFailed, "search"); + } + + if (holders.Count != 1) + return false; + + try + { + return LdapDn.Equals(holders[0].Dn, entry.Dn); + } + catch (Exception) + { + return false; + } + } + + /// + /// The entry's msDS-PrincipalName when the directory computes one in DOMAIN\name + /// form; when it is absent, unreadable, or a SID string (AD's form for a + /// principal it cannot name) — all of which mean "not available", never "mismatch". + /// + private static string? ReadPrincipalName(ZB.MOM.WW.Auth.Ldap.Internal.ILdapConnection conn, string dn) + { + try + { + var read = conn.Read(dn, new[] { PrincipalNameAttribute }); + if (read is not null + && read.Attributes.TryGetValue(PrincipalNameAttribute, out var values) + && values.Count == 1 + && values[0].Contains('\\', StringComparison.Ordinal)) + { + return values[0].Trim(); + } + } + catch (LdapException) + { + // Not offered, or not readable by this account: fall back to the uniqueness check. + } + + return null; } /// Step 7: search for the user entry; a search fault is system-side. diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs index d7a26fb..b1b3425 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/FakeLdapConnection.cs @@ -19,6 +19,9 @@ internal sealed class FakeLdapConnection : ILdapConnection private int? _firstBindResultCode; private bool _throwOnSearch; private string? _whoAmI; + private string? _principalName; + private bool _throwOnRead; + private readonly Dictionary> _filterResults = new(StringComparer.Ordinal); // ---- observation ----- @@ -123,6 +126,39 @@ internal sealed class FakeLdapConnection : ILdapConnection return this; } + /// + /// Scripts AD's constructed msDS-PrincipalName (DOMAIN\name) that a base + /// of any DN returns. Unscripted, returns the entry without it. + /// + public FakeLdapConnection WithPrincipalName(string value) + { + _principalName = value; + return this; + } + + /// Throw from . + public FakeLdapConnection ThrowOnRead() + { + _throwOnRead = true; + return this; + } + + /// + /// Scripts the entries a with exactly returns, each + /// carrying the given sAMAccountName. Other filters still return the + /// entries. + /// + public FakeLdapConnection WithSearchResult(string filter, params (string Dn, string Sam)[] entries) + { + _filterResults[filter] = entries + .Select(e => new LdapSearchEntry(e.Dn, new Dictionary>(StringComparer.OrdinalIgnoreCase) + { + ["sAMAccountName"] = new[] { e.Sam }, + })) + .ToList(); + return this; + } + /// Throw from (unhealthy directory). public FakeLdapConnection ThrowOnSearch() { @@ -205,7 +241,19 @@ internal sealed class FakeLdapConnection : ILdapConnection if (_throwOnSearch) throw new Novell.Directory.Ldap.LdapException( "Search failed", Novell.Directory.Ldap.LdapException.Unavailable, searchBase); - return _scriptedEntries.AsReadOnly(); + return _filterResults.TryGetValue(filter, out var scripted) ? scripted : _scriptedEntries.AsReadOnly(); + } + + public LdapSearchEntry? Read(string dn, IReadOnlyList attributes) + { + Calls.Add($"read:{dn}"); + if (_throwOnRead) + throw new Novell.Directory.Ldap.LdapException( + "Read failed", Novell.Directory.Ldap.LdapException.InsufficientAccessRights, dn); + var attrs = new Dictionary>(StringComparer.OrdinalIgnoreCase); + if (_principalName is not null) + attrs["msDS-PrincipalName"] = new[] { _principalName }; + return new LdapSearchEntry(dn, attrs); } public string WhoAmI() diff --git a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs index b74c739..e1f85dc 100644 --- a/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs +++ b/ZB.MOM.WW.Auth/tests/ZB.MOM.WW.Auth.Ldap.Tests/LdapAuthServiceUserBindTests.cs @@ -288,6 +288,95 @@ public class LdapAuthServiceUserBindTests Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); } + // ---- scadaproj#50: an AD forest — the same sAMAccountName in two domains ---------------------- + + // A Global Catalog search base spans every domain in the forest. CORP\jsmith binds; the search + // (here on mail, so the filter is not sAMAccountName) finds OTHER\jsmith, whose sAMAccountName is + // also "jsmith". 0.4.4 compared sAMAccountName alone and returned OTHER\jsmith's groups. + private const string OtherDn = "CN=J Smith,OU=u,DC=other,DC=forest"; + private const string CorpDn = "CN=John Smith,OU=u,DC=corp,DC=forest"; + + private static FakeLdapConnection ForestSearchFinding(string dn) => new FakeLdapConnection() + .WithWhoAmI(@"u:CORP\jsmith") + .WithUserEntry(dn, new[] { "CN=UG_Admins,OU=g,DC=other,DC=forest" }, "J Smith", + new Dictionary { ["sAMAccountName"] = new[] { "jsmith" }, ["mail"] = new[] { "jsmith" } }); + + private static LdapOptions ForestOpts() => Opts() with { UserNameAttribute = "mail", SearchBase = "DC=forest" }; + + [Fact] + public async Task Forest_PrincipalNameInAnotherDomain_FailsClosed() + { + var fake = ForestSearchFinding(OtherDn).WithPrincipalName(@"OTHER\jsmith"); + + var result = await Login(new LdapAuthService(ForestOpts(), new SequenceLdapConnectionFactory(fake)), "jsmith"); + + Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); + Assert.Empty(result.Groups); + Assert.Contains("read:" + OtherDn, fake.Calls); + } + + [Fact] + public async Task Forest_PrincipalNameInTheBoundDomain_Succeeds() + { + var fake = ForestSearchFinding(CorpDn).WithPrincipalName(@"corp\JSMITH"); + + var result = await Login(new LdapAuthService(ForestOpts(), new SequenceLdapConnectionFactory(fake)), "jsmith"); + + Assert.True(result.Succeeded); + } + + [Fact] + public async Task Forest_NoPrincipalName_AndTheNameIsHeldInTwoDomains_FailsClosed() + { + // The directory offers no msDS-PrincipalName, so which jsmith bound cannot be proven; with two + // holders of the name in scope the sign-in is refused rather than guessed. + var fake = ForestSearchFinding(OtherDn) + .WithSearchResult("(sAMAccountName=jsmith)", (CorpDn, "jsmith"), (OtherDn, "jsmith")); + + var result = await Login(new LdapAuthService(ForestOpts(), new SequenceLdapConnectionFactory(fake)), "jsmith"); + + Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); + } + + [Fact] + public async Task Forest_NoPrincipalName_AndTheReadFails_StillFallsBackToTheUniquenessCheck() + { + var fake = ForestSearchFinding(OtherDn).ThrowOnRead() + .WithSearchResult("(sAMAccountName=jsmith)", (CorpDn, "jsmith"), (OtherDn, "jsmith")); + + var result = await Login(new LdapAuthService(ForestOpts(), new SequenceLdapConnectionFactory(fake)), "jsmith"); + + Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); + } + + [Fact] + public async Task SingleDomain_NoPrincipalName_UniqueName_Succeeds() + { + // Compatibility: a single-domain directory without msDS-PrincipalName still signs in — the + // name has exactly one holder in scope, and it is the entry found. + var fake = ForestSearchFinding(CorpDn).WithSearchResult("(sAMAccountName=jsmith)", (CorpDn, "jsmith")); + + var result = await Login(new LdapAuthService(ForestOpts(), new SequenceLdapConnectionFactory(fake)), "jsmith"); + + Assert.True(result.Succeeded); + } + + [Fact] + public async Task SamAccountNameSearch_ForTheBoundName_NeedsNoSecondSearch() + { + // When the user search itself was (sAMAccountName=) and found exactly one + // entry, the name is already proven unique in scope. + var fake = new FakeLdapConnection() + .WithWhoAmI(@"u:CORP\jdoe") + .WithUserEntry(JaneDn, new[] { "CN=UG_A,OU=g,DC=x" }, "Jane Doe", + new Dictionary { ["sAMAccountName"] = new[] { "jdoe" } }); + + var result = await Login(new LdapAuthService(Opts(), new SequenceLdapConnectionFactory(fake)), "jdoe"); + + Assert.True(result.Succeeded); + Assert.Single(fake.Calls, c => c.StartsWith("search:", StringComparison.Ordinal)); + } + [Fact] public async Task WhoAmIDn_NamingAnotherEntry_FailsClosed() {