fix(auth): prove the AD domain of a u:DOMAIN\name user bind

Auth-007's sAMAccountName match assumed the search base covers one AD
domain; across a forest (Global Catalog) it could accept another domain's
same-named account and return its groups. For a u:DOMAIN\name Who-am-I
answer the entry must now also be that domain's account: AD's
msDS-PrincipalName (objectSid-derived DOMAIN\name, read by a base search
through the new ILdapConnection.Read) must equal the answer where offered;
otherwise the name must have exactly one holder under SearchBase and it must
be the entry found. Three forest cases fail pre-fix; single-domain and
sAMAccountName-search compat cases pass both ways. README notes this and
Auth-009.

(scadaproj#50)

Claude-Session: https://claude.ai/code/session_01GT67AEHd2Acv1YPwGfXEoV
This commit is contained in:
Joseph Doherty
2026-11-09 15:18:46 -05:00
parent 9ec0da1b2c
commit b8d3fa2ba1
6 changed files with 281 additions and 11 deletions
+5
View File
@@ -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. - **`{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=<group>,ou=users,<base>`, not `cn={0},ou=<group>,<base>`. ServiceAccount mode is unchanged. - **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=<group>,ou=users,<base>`, not `cn={0},ou=<group>,<base>`. ServiceAccount mode is unchanged.
- The validator's `UserBindTemplate` message now leads with `EXAMPLE\{0}`. No `Abstractions` contract change (XML docs only). - 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 ### 0.4.1 — LDAP review fixes
@@ -46,6 +46,14 @@ internal interface ILdapConnection : IDisposable
/// <summary>Executes a subtree search and returns all matching entries.</summary> /// <summary>Executes a subtree search and returns all matching entries.</summary>
IReadOnlyList<LdapSearchEntry> Search(string searchBase, string filter, IReadOnlyList<string> attributes); IReadOnlyList<LdapSearchEntry> Search(string searchBase, string filter, IReadOnlyList<string> attributes);
/// <summary>
/// Reads one entry by DN (a base-scope search) and returns the requested attributes the server
/// sent, or <see langword="null"/> when the entry does not exist. Throws <c>LdapException</c> on
/// any other failure. Used for constructed attributes such as AD's <c>msDS-PrincipalName</c>,
/// which a server may compute for a base read but not for a subtree search.
/// </summary>
LdapSearchEntry? Read(string dn, IReadOnlyList<string> attributes);
/// <summary> /// <summary>
/// RFC 4532 "Who am I?": the authorization identity the directory associates with the current /// RFC 4532 "Who am I?": the authorization identity the directory associates with the current
/// bind, as the server sends it (<c>dn:&lt;dn&gt;</c>, <c>u:DOMAIN\name</c>, or empty for an /// bind, as the server sends it (<c>dn:&lt;dn&gt;</c>, <c>u:DOMAIN\name</c>, or empty for an
@@ -102,6 +102,27 @@ internal sealed class NovellLdapConnection : ILdapConnection
return ReadEntries(results); return ReadEntries(results);
} }
/// <inheritdoc/>
public LdapSearchEntry? Read(string dn, IReadOnlyList<string> 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;
}
}
/// <inheritdoc/> /// <inheritdoc/>
public string WhoAmI() public string WhoAmI()
{ {
@@ -369,7 +369,7 @@ public sealed class LdapAuthService : ILdapAuthService
if (entries.Count != 1) if (entries.Count != 1)
return LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed); return LdapAuthResult.Fail(LdapAuthFailure.GroupLookupFailed);
if (!IsBoundAccount(entries[0], bound)) if (!IsBoundAccount(conn, entries[0], bound, username))
{ {
_logger?.LogWarning( _logger?.LogWarning(
"LDAP user bind authenticated {BoundIdentity} but the entry found for the typed name is " + "LDAP user bind authenticated {BoundIdentity} but the entry found for the typed name is " +
@@ -384,13 +384,18 @@ public sealed class LdapAuthService : ILdapAuthService
/// <summary>The AD attribute that an RFC 4532 <c>u:DOMAIN\name</c> authorization identity names.</summary> /// <summary>The AD attribute that an RFC 4532 <c>u:DOMAIN\name</c> authorization identity names.</summary>
private const string SamAccountNameAttribute = "sAMAccountName"; private const string SamAccountNameAttribute = "sAMAccountName";
/// <summary>AD's constructed attribute holding a principal's <c>DOMAIN\name</c>, derived from its <c>objectSid</c>.</summary>
private const string PrincipalNameAttribute = "msDS-PrincipalName";
/// <summary> /// <summary>
/// The account a user bind authenticated: either its DN, or (AD's Who-am-I form) its /// The account a user bind authenticated: either its DN, or (AD's Who-am-I form) its
/// <c>sAMAccountName</c>. Exactly one member is set. /// <c>sAMAccountName</c> plus, when the answer carried one, its NetBIOS <see cref="Domain"/>.
/// Exactly one of <see cref="Dn"/> / <see cref="AccountName"/> is set.
/// </summary> /// </summary>
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}";
} }
/// <summary>A template is a DN template when it contains an attribute assertion (<c>=</c>); UPN and down-level forms never do.</summary> /// <summary>A template is a DN template when it contains an attribute assertion (<c>=</c>); UPN and down-level forms never do.</summary>
@@ -424,11 +429,15 @@ public sealed class LdapAuthService : ILdapAuthService
if (authzId.StartsWith("u:", StringComparison.OrdinalIgnoreCase) && authzId.Length > 2) if (authzId.StartsWith("u:", StringComparison.OrdinalIgnoreCase) && authzId.Length > 2)
{ {
var account = authzId[2..].Trim(); var account = authzId[2..].Trim();
string? domain = null;
var slash = account.LastIndexOf('\\'); var slash = account.LastIndexOf('\\');
if (slash >= 0) if (slash >= 0)
{
domain = account[..slash].Trim();
account = account[(slash + 1)..]; account = account[(slash + 1)..];
}
if (account.Length > 0) 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; return dnTemplate ? new BoundIdentity(bindName, null) : null;
@@ -437,9 +446,11 @@ public sealed class LdapAuthService : ILdapAuthService
/// <summary> /// <summary>
/// <see langword="true"/> only when <paramref name="entry"/> is the account that bound: its DN /// <see langword="true"/> only when <paramref name="entry"/> is the account that bound: its DN
/// equals the bound DN (RFC 4514 <c>distinguishedNameMatch</c>, case- and space-insensitive), or /// equals the bound DN (RFC 4514 <c>distinguishedNameMatch</c>, case- and space-insensitive), or
/// its <c>sAMAccountName</c> equals the Who-am-I account name (case-insensitive, as AD compares it). /// — for AD's <c>u:DOMAIN\name</c> answer — it is provably that domain's account of that name
/// (see <see cref="IsBoundAdAccount"/>).
/// </summary> /// </summary>
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) if (bound.Dn is not null)
{ {
@@ -454,9 +465,97 @@ public sealed class LdapAuthService : ILdapAuthService
} }
} }
return entry.Attributes.TryGetValue(SamAccountNameAttribute, out var values) return IsBoundAdAccount(conn, entry, bound, username);
&& values.Count == 1 }
&& string.Equals(values[0], bound.AccountName, StringComparison.OrdinalIgnoreCase);
/// <summary>
/// The <c>u:DOMAIN\name</c> case. A matching <c>sAMAccountName</c> is necessary but not enough:
/// <c>sAMAccountName</c> 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:
/// <list type="number">
/// <item>Where the directory offers AD's constructed <c>msDS-PrincipalName</c> for the entry —
/// its <c>DOMAIN\name</c>, computed from the entry's <c>objectSid</c> — it must equal the
/// Who-am-I answer, domain included. That is the decisive, SID-anchored check. (The bound
/// account's own <c>objectSid</c> cannot be compared directly: Who-am-I does not return it.)</item>
/// <item>Otherwise the name must have exactly ONE holder under <see cref="LdapOptions.SearchBase"/>,
/// 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
/// <c>(sAMAccountName=&lt;the bound name&gt;)</c> 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 <c>msDS-PrincipalName</c>.</item>
/// </list>
/// </summary>
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<LdapSearchEntry> 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;
}
}
/// <summary>
/// The entry's <c>msDS-PrincipalName</c> when the directory computes one in <c>DOMAIN\name</c>
/// form; <see langword="null"/> 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".
/// </summary>
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;
} }
/// <summary>Step 7: search for the user entry; a search fault is system-side.</summary> /// <summary>Step 7: search for the user entry; a search fault is system-side.</summary>
@@ -19,6 +19,9 @@ internal sealed class FakeLdapConnection : ILdapConnection
private int? _firstBindResultCode; private int? _firstBindResultCode;
private bool _throwOnSearch; private bool _throwOnSearch;
private string? _whoAmI; private string? _whoAmI;
private string? _principalName;
private bool _throwOnRead;
private readonly Dictionary<string, List<LdapSearchEntry>> _filterResults = new(StringComparer.Ordinal);
// ---- observation ----- // ---- observation -----
@@ -123,6 +126,39 @@ internal sealed class FakeLdapConnection : ILdapConnection
return this; return this;
} }
/// <summary>
/// Scripts AD's constructed <c>msDS-PrincipalName</c> (<c>DOMAIN\name</c>) that a base
/// <see cref="Read"/> of any DN returns. Unscripted, <see cref="Read"/> returns the entry without it.
/// </summary>
public FakeLdapConnection WithPrincipalName(string value)
{
_principalName = value;
return this;
}
/// <summary>Throw <see cref="Novell.Directory.Ldap.LdapException"/> from <see cref="Read"/>.</summary>
public FakeLdapConnection ThrowOnRead()
{
_throwOnRead = true;
return this;
}
/// <summary>
/// Scripts the entries a <see cref="Search"/> with exactly <paramref name="filter"/> returns, each
/// carrying the given <c>sAMAccountName</c>. Other filters still return the
/// <see cref="WithUserEntry"/> entries.
/// </summary>
public FakeLdapConnection WithSearchResult(string filter, params (string Dn, string Sam)[] entries)
{
_filterResults[filter] = entries
.Select(e => new LdapSearchEntry(e.Dn, new Dictionary<string, IReadOnlyList<string>>(StringComparer.OrdinalIgnoreCase)
{
["sAMAccountName"] = new[] { e.Sam },
}))
.ToList();
return this;
}
/// <summary>Throw <see cref="Novell.Directory.Ldap.LdapException"/> from <see cref="Search"/> (unhealthy directory).</summary> /// <summary>Throw <see cref="Novell.Directory.Ldap.LdapException"/> from <see cref="Search"/> (unhealthy directory).</summary>
public FakeLdapConnection ThrowOnSearch() public FakeLdapConnection ThrowOnSearch()
{ {
@@ -205,7 +241,19 @@ internal sealed class FakeLdapConnection : ILdapConnection
if (_throwOnSearch) if (_throwOnSearch)
throw new Novell.Directory.Ldap.LdapException( throw new Novell.Directory.Ldap.LdapException(
"Search failed", Novell.Directory.Ldap.LdapException.Unavailable, searchBase); "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<string> 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<string, IReadOnlyList<string>>(StringComparer.OrdinalIgnoreCase);
if (_principalName is not null)
attrs["msDS-PrincipalName"] = new[] { _principalName };
return new LdapSearchEntry(dn, attrs);
} }
public string WhoAmI() public string WhoAmI()
@@ -288,6 +288,95 @@ public class LdapAuthServiceUserBindTests
Assert.Equal(LdapAuthFailure.GroupLookupFailed, result.Failure); 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<string, string[]> { ["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=<the bound name>) 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<string, string[]> { ["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] [Fact]
public async Task WhoAmIDn_NamingAnotherEntry_FailsClosed() public async Task WhoAmIDn_NamingAnotherEntry_FailsClosed()
{ {