diff --git a/server/ldap_auth.py b/server/ldap_auth.py index 11c14c0..f12789c 100644 --- a/server/ldap_auth.py +++ b/server/ldap_auth.py @@ -117,6 +117,7 @@ BAD_CREDENTIALS = "bad_credentials" NOT_IN_GROUP = "not_in_group" NO_DIRECTORY_ENTRY = "no_directory_entry" GROUP_NOT_FOUND = "group_not_found" +GROUP_CHECK_FAILED = "group_check_failed" # AD returns these as `data ` inside an error-49 message. Kept for the log # only: telling an unauthenticated caller "your password expired" confirms the @@ -156,7 +157,8 @@ class LdapResult: """True when the failure is ours, not the user's. With no local password fallback (D13) these must be logged as errors and surfaced to an operator — otherwise a broken deploy is indistinguishable from a forgotten password.""" - return self.reason in (UNCONFIGURED, UNREACHABLE, UNTRUSTED, GROUP_NOT_FOUND) + return self.reason in (UNCONFIGURED, UNREACHABLE, UNTRUSTED, GROUP_NOT_FOUND, + GROUP_CHECK_FAILED) def base_dn(domain: Optional[str] = None) -> str: @@ -268,17 +270,57 @@ def _resolve_group_dn(conn, group: str) -> Optional[str]: def member_of(conn, sam: str, group: str) -> bool: - """Nested-group-aware membership test for an already-bound connection.""" + """Is `sam` in `group`, counting nested membership? + + THE RETURN VALUE OF conn.search() IS NOT OPTIONAL READING. The connection is + built with raise_exceptions=False, so a search that FAILS returns False and + leaves conn.entries empty — which is byte-for-byte indistinguishable from "no + match" if you only look at conn.entries. An earlier version of this function did + exactly that, and every failure of the extensible-match filter presented to the + user as "you are not in the group" while they plainly were. + + Two searches, in order, and the second one exists to catch the first being wrong: + + 1. AD's transitive matching rule (LDAP_MATCHING_RULE_IN_CHAIN). This is the + correct query — it walks nested groups, which plain memberOf does not. + 2. If that matches nothing, a plain memberOf equality check for DIRECT + membership. + + If (2) matches after (1) did not, the person IS a member and is let in — refusing + a real member is the worse error — but it is logged as a WARNING, because it means + the transitive rule is returning nothing and NESTED membership is silently not + working on this connection. That needs a human; it must not pass unnoticed. + """ dn = _resolve_group_dn(conn, group) if not dn: log.error("required group %r does not resolve in %s — refusing the sign-in. " "This is a configuration fault, not a bad password.", group, base_dn()) raise LookupError(GROUP_NOT_FOUND) - filt = (f"(&(sAMAccountName={escape_filter_chars(sam)})" - f"(memberOf:{NESTED_MEMBER_RULE}:={escape_filter_chars(dn)}))") - conn.search(base_dn(), filt, search_scope=SUBTREE, - attributes=["sAMAccountName"], size_limit=1) - return bool(conn.entries) + + esc_sam, esc_dn = escape_filter_chars(sam), escape_filter_chars(dn) + + def _search(filt: str, label: str): + ok = conn.search(base_dn(), filt, search_scope=SUBTREE, + attributes=["sAMAccountName"], size_limit=1) + if not ok: + log.error("the %s membership search FAILED (not 'no match') for %r: %s | " + "filter=%s", label, sam, conn.result, filt) + raise LookupError(GROUP_CHECK_FAILED) + return bool(conn.entries) + + if _search(f"(&(sAMAccountName={esc_sam})(memberOf:{NESTED_MEMBER_RULE}:={esc_dn}))", + "nested"): + return True + + if _search(f"(&(sAMAccountName={esc_sam})(memberOf={esc_dn}))", "direct"): + log.warning( + "%r IS a direct member of %r, but AD's transitive matching rule " + "(%s) returned nothing for them. Allowing the sign-in — refusing a real " + "member is worse — but NESTED group membership is not working on this " + "connection and needs investigating.", sam, dn, NESTED_MEMBER_RULE) + return True + + return False def verify(username: str, password: str, required_group: Optional[str] = None) -> LdapResult: @@ -354,8 +396,9 @@ def verify(username: str, password: str, required_group: Optional[str] = None) - log.warning("bind succeeded for %r but the account is NOT in %r", sam, group) return LdapResult(False, NOT_IN_GROUP, f"not in {group}") - except LookupError: - return LdapResult(False, GROUP_NOT_FOUND, f"group {group!r} not found") + except LookupError as exc: + reason = str(exc) or GROUP_NOT_FOUND + return LdapResult(False, reason, f"group {group!r}: {reason}") return LdapResult( True, OK,