Accept NSS aliases for canonicalized user records by kosstevich · Pull Request #42452 · systemd/systemd

kosstevich · 2026-06-03T15:10:48Z

This PR fixes userdb lookups for NSS users that are resolved through an alias but returned with a canonical user name.

Some NSS providers, such as SSSD, can successfully resolve a user by an alias-like name, for example a Kerberos/AD UPN (for example [email protected]) while returning a passwd record with the canonical login name.

The original lookup name was not preserved. Later, the userdb worker checked whether the returned record matched the requested name with user_record_matches_user_name(). Since the requested name was [email protected], but the record only contained testuser, the lookup was rejected as:
io.systemd.UserDatabase.ConflictingRecordFound

This also caused pam_systemd to fail opening sessions for such users with:
pam_systemd(...:session): Failed to get user record

In my case, this broke graphical logins for Samba users logging in with a UPN on systems where SSSD canonicalizes the NSS result.

My solution preserves the requested name as a UserRecord alias when an NSS lookup by name succeeds but the returned pw_name differs from the requested name.
This allows user_record_matches_user_name() to accept the canonicalized NSS result instead of treating it as a conflicting record.
The patch does not invent new aliases. It only records the name that NSS itself already accepted and resolved successfully.

I'm not an expert on working with systemd, so I'm asking for a review of my PR.

github-actions · 2026-06-03T15:16:33Z

Claude review of PR #42452 (`650bb87`)

Must fix

Alias never added to ur->json, dropped on serialization — src/shared/user-record-nss.c:249 — the alias is written only to the C struct field; the JSON (authoritative for the varlink/multiplexer reply) is built earlier without it, so the fix has no effect on the path it targets.
Requested name stored as alias without validation — src/shared/user-record-nss.c:249 — bypasses the valid_user_group_name() check that every JSON-loaded alias undergoes, so a broken/hostile NSS module can get an invalid name accepted as a match.

Suggestions

~~Case-sensitive guard/match leaves common case-only mismatches failing~~ (dismissed) — src/shared/user-record-nss.c:248 — streq and strv_contains are byte-exact, but AD/SSSD names are usually case-insensitive, so the fix only helps the verbatim requested spelling.
Group matcher lacks user-side realm handling — src/shared/group-record.c:350 — group_record_matches_group_name() adds alias matching but, unlike user_record_matches_user_name(), has no alias@realm branch, so group lookups behave asymmetrically for realm-qualified names.
By-name lookup self-confirms the ConflictingRecordFound check — src/shared/user-record-nss.c:262 — recording the requested name as an alias during the same lookup makes the worker's user_record_matches_user_name() self-check always pass for valid by-name lookups, relaxing a defense-in-depth check (no privilege escalation: uid/gid come solely from NSS).
No user-side realm or dedup/NULL-alias test coverage — src/test/test-user-record-nss.c:49 — the user test omits the alias@realm branch (group side has nss_group_alias_realm) plus the alias==canonical and NULL-alias no-op paths.
Group perMachine table omits aliases — src/shared/group-record.c:180 — aliases is dispatched only in the top-level group_dispatch_table; the user side dispatches it in the perMachine table too, so group per-machine aliases round-trip asymmetrically.
Alias fuzzy-match branch untested — src/test/test-group-record-nss.c:55 — neither test exercises the new alias-aware user_name_fuzzy_match() branch added to group_record_match()/user_record_match().
Duplicated alias@realm block in group vs user matcher — src/shared/group-record.c:354 — the new realm-matching loop is byte-for-byte identical to the one in user_record_matches_user_name(); consider a shared helper so the two matchers can't drift.

Nits

Stylistic divergence from nss_user_record_by_uid — src/shared/user-record-nss.c:254 — by_name now uses a ur local + TAKE_PTR while the sibling still uses the (*ret)->incomplete idiom.
test-group-record-nss.c out of alphabetical order — src/test/meson.build:116 — inserted before test-glob-util.c; belongs after test-gpt.c.
GroupRecord aliases field ordering diverges from UserRecord — src/shared/group-record.h:13 — aliases is placed between group_name and realm, whereas UserRecord lists it after user_name_and_realm_auto.
sd-json.h not separated into its own include group — src/test/test-user-record-nss.c:5 — #include "sd-json.h" is glued onto the system-header block; sd-* headers should be a separate group preceded by a blank line.
sd-json.h not separated into its own include group — src/test/test-group-record-nss.c:5 — same as the user test file.
Group test omits no-op alias coverage present on user side — src/test/test-group-record-nss.c:67 — add nss_group_alias_same_as_canonical_noop and nss_group_null_alias_noop to mirror the user test's skip-policy cases.

Update for 9201f8a: this revision (now two commits: implementation 5d68278d + tests 9201f8a) keeps all previously-resolved items addressed — the two must-fixes (JSON serialization, alias validation via nss_add_valid_alias), the group realm-matching branch, the group perMachine dispatch entry, the fuzzy-match and user-side test coverage, and the meson ordering / by_uid style nits. This round's reviewers (correctness, resource-lifetime, security, alias-matching semantics, Varlink lookup-order) found no new correctness, memory-safety, JSON round-trip, or impersonation defects: the swapped name-before-uid lookup ordering preserves the documented IDL semantics (the post-lookup uid/gid + name consistency check still rejects mismatches), and uid_is_valid(p.gid)→gid_is_valid(p.gid) is a no-op type-hygiene fix. Two new minor items: the group alias@realm block now duplicates the user matcher verbatim (suggestion), and the group test lacks the alias-equals-canonical / NULL-alias no-op cases the user test has (nit). The by-name self-confirmation suggestion stays open by design (maintainer-endorsed by-name-first worker lookup). The GroupRecord field-ordering and sd-json.h include-grouping nits remain open.

Update for 650bb87: this revision (implementation e76a2358 + tests 650bb87a) resolves the remaining open items. The duplicated alias@realm loop is now factored into the shared record_name_matches_alias_realm() helper called by both the user and group matchers; the GroupRecord aliases field was moved to after group_name_and_realm_auto; both new test files now separate sd-json.h from the system-header block with a blank line; and the group test gained the nss_group_alias_same_as_canonical_noop / nss_group_null_alias_noop no-op cases. This round's reviewers (correctness, resource-lifetime, security, alias-matching semantics, Varlink lookup-order) found no new defects. The only by-name self-confirmation suggestion remains open by design (maintainer-endorsed by-name-first worker lookup).

Workflow run

github-actions · 2026-06-03T15:25:01Z

        if (r < 0)
                return r;

+        if (!streq(name, result->pw_name)) {


Claude: suggestion: Both this guard (streq(name, result->pw_name)) and the later matching in user_record_matches_user_name() (streq/strv_contains) are case-sensitive. NSS/AD/SSSD names are commonly case-insensitive, so the originally requested name frequently differs from the canonical pw_name only in case (e.g. requested [email protected] vs returned testuser). Storing the verbatim requested string does help that exact spelling, but any subsequent lookup that differs only in case from the recorded alias will still be rejected as ConflictingRecordFound. If case-insensitive providers are the motivating use case, consider whether the storage/matching should account for case rather than relying on byte-exact comparison.

Case-insensitive matching is a deeper fix, which I think is out the scope of this PR.

This change is intentionally narrow: it preserves the exact lookup name that NSS already resolved, so canonicalized NSS results continue to match the original request.

github-actions · 2026-06-04T11:03:39Z

        'test-format-util.c',
        'test-fs-util.c',
        'test-fstab-util.c',
+        'test-group-record-nss.c',


Claude: nit: test-group-record-nss.c is inserted between test-fstab-util.c and test-glob-util.c, breaking the otherwise-alphabetical ordering of this list (glob < gpt < group < gunicode). It should be placed after test-gpt.c and before test-gunicode.c. The sibling test-user-record-nss.c was added in the correct position.

github-actions · 2026-06-04T12:03:17Z

                incomplete = true;

-        r = nss_passwd_to_user_record(result, sresult, ret);
+        r = nss_passwd_to_user_record(result, sresult, name, ret);


Claude: suggestion: The NSS by-name path records the originally-requested lookup name as an alias on the returned record (and the same at the group path). Downstream, the userdb worker validates the record with user_record_matches_user_name(hr, p.name) and otherwise rejects it with ConflictingRecordFound. Because the requested name is added as an alias during the same lookup (whenever it passes valid_user_group_name(VALID_USER_RELAX) and differs from the canonical name), that self-check now always passes for any NSS-backed by-name lookup, relaxing the defense-in-depth check that previously caught NSS modules returning a record whose pw_name does not correspond to the requested name.

This is not a privilege escalation in itself — the uid/gid come solely from the NSS record, and a hostile NSS module could already return the requested name as pw_name directly. Flagging for awareness: if you want the conflict check to retain meaning for genuinely mismatched NSS results, consider scoping alias recording more narrowly than "any syntactically-valid requested name" (e.g. only the canonicalization case the PR targets).

I agree with this concern, but I am not yet sure what the right constraint should be here. Would checking that the requested name has the form <pw_name>@... be an acceptable way to scope this more tightly?

so the user_record_matches_user_name() check in userwork.c exists mostly to ensure that if people do a lookup where they specify both UID and name, that we fail if the record we found doesn't match both. because that would be weird.

now, if both UID and name are specified we currently do the lookup via UID, and then check the name afterwards. I guess this should be switched so that we do the lookup by name instead, so that for records like yours we definitely have the right alias in the record, and the test will then succeed.

github-actions · 2026-06-04T12:03:18Z

+        ASSERT_NULL(sd_json_variant_by_key(u->json, "aliases"));
+}
+
+DEFINE_TEST_MAIN(LOG_INFO);


Claude: suggestion: Two coverage gaps in the new user-side test (the production logic is correct; these are just missing cases):

The user-side alias@realm matching branch is untested. user_record_matches_user_name() gained the same realm-stripping logic as the group side, and the group test exercises it via nss_group_alias_realm, but there is no user equivalent, so a regression in the user realm branch would go undetected. Mirror nss_group_alias_realm: build a record with an alias, set u->realm, then assert user_record_matches_user_name(u, "<alias>@<realm>").

The nss_add_valid_alias() no-op branches are untested: alias_name equal to the canonical name (must not add a redundant alias nor emit an aliases JSON key) and the NULL/empty alias_name path used by the by-uid/by-gid and iterator callers.

poettering · 2026-06-04T12:57:11Z

        r = sd_json_buildo(
                        &hr->json,
                        SD_JSON_BUILD_PAIR_STRING("userName", hr->user_name),
+                        SD_JSON_BUILD_PAIR_CONDITION(!strv_isempty(hr->aliases), "aliases", SD_JSON_BUILD_STRV(hr->aliases)),


JSON_BUILD_PAIR_STRV_NON_EMPTY()

poettering · 2026-06-04T12:58:22Z

                incomplete = true;

-        r = nss_passwd_to_user_record(result, sresult, ret);
+        r = nss_passwd_to_user_record(result, sresult, NULL, ret);


we generally want in new code that "opaque" params in func calls (i.e. those which give you no hint what you are passing there) are prefixed with a param comment. i.e. for the 3rd argument here write /* name= */ NULL instead of just NULL.

poettering · 2026-06-04T12:58:33Z

        r = sd_json_buildo(
                        &g->json,
                        SD_JSON_BUILD_PAIR_STRING("groupName", g->group_name),
+                        SD_JSON_BUILD_PAIR_CONDITION(!strv_isempty(g->aliases), "aliases", SD_JSON_BUILD_STRV(g->aliases)),


poettering · 2026-06-04T12:58:41Z

                incomplete = true;

-        r = nss_group_to_group_record(result, sresult, ret);
+        r = nss_group_to_group_record(result, sresult, NULL, ret);


poettering · 2026-06-04T12:58:50Z

                        }

-                        r = nss_passwd_to_user_record(pw, r >= 0 ? &spwd : NULL, ret);
+                        r = nss_passwd_to_user_record(pw, r >= 0 ? &spwd : NULL, NULL, ret);


poettering · 2026-06-04T12:58:56Z

                        }

-                        r = nss_group_to_group_record(gr, r >= 0 ? &sgrp : NULL, ret);
+                        r = nss_group_to_group_record(gr, r >= 0 ? &sgrp : NULL, NULL, ret);


github-actions · 2026-06-04T13:28:43Z


        static const sd_json_dispatch_field group_dispatch_table[] = {
                { "groupName",      SD_JSON_VARIANT_STRING,        json_dispatch_user_group_name,  offsetof(GroupRecord, group_name),       SD_JSON_RELAX  },
+                { "aliases",        SD_JSON_VARIANT_ARRAY,         json_dispatch_user_group_list,  offsetof(GroupRecord, aliases),          SD_JSON_RELAX  },


Claude: suggestion: The new aliases field is added only to the top-level group_dispatch_table, but not to the group per_machine_dispatch_table. On the user side aliases is dispatched in both the main user_dispatch_table and the perMachine table, so a GroupRecord that carries aliases inside a perMachine block round-trips asymmetrically: the user loader honors per-machine aliases while the group loader silently drops them. The NSS path this PR targets is unaffected (it writes the top-level aliases key), but for group-vs-user symmetry of the struct/JSON mapping consider also adding the aliases entry to the group perMachine table.

github-actions · 2026-06-04T13:28:45Z

+        ASSERT_TRUE(group_record_matches_group_name(g, "[email protected]"));
+}
+
+DEFINE_TEST_MAIN(LOG_INFO);


Claude: suggestion: The new tests cover group_record_matches_group_name()/user_record_matches_user_name() and the JSON round-trip, but neither test file exercises the other new branch added by this PR: the alias-aware fuzzy match in group_record_match() and user_record_match(), which now additionally calls user_name_fuzzy_match() over ->aliases. A record carrying only an alias (no matching name or realm) looked up via a UserDBMatch with fuzzy_names is currently untested, so a regression in that branch would go undetected. This is distinct from the already-noted user-side alias@realm and nss_add_valid_alias() no-op coverage gaps.

github-actions · 2026-06-04T13:45:07Z

        bool incomplete;

        char *group_name;
+        char **aliases;


Claude: nit: For symmetry with UserRecord — where aliases is placed after user_name, realm, and user_name_and_realm_auto — consider moving this aliases field to after group_name_and_realm_auto instead of between group_name and realm (and reorder the matching strv_free() in group_record_free() accordingly). Purely cosmetic, no behavioral impact.

github-actions · 2026-06-05T15:10:25Z

+
+#include <pwd.h>
+#include <string.h>
+#include "sd-json.h"


Claude: nit: Claude: Per systemd's include conventions, sd-* public-library headers form their own group separated from the preceding system headers by a blank line (compare test-user-record.c). Here #include "sd-json.h" is glued directly onto the <pwd.h>/<string.h> block (note the file already leaves a blank line after it, before the internal headers). Add a blank line before it so the grouping reads system headers / sd-* headers / internal headers.

github-actions · 2026-06-05T15:10:26Z

+
+#include <grp.h>
+#include <string.h>
+#include "sd-json.h"


Claude: nit: Claude: Per systemd's include conventions, sd-* public-library headers form their own group separated from the preceding system headers by a blank line (compare test-user-record.c). Here #include "sd-json.h" is glued directly onto the <grp.h>/<string.h> block (note the file already leaves a blank line after it, before the internal headers). Add a blank line before it so the grouping reads system headers / sd-* headers / internal headers.

github-actions · 2026-06-05T15:53:14Z

+        const char *realm = strrchr(group_name, '@');
+        if (realm && streq_ptr(realm+1, g->realm))
+                STRV_FOREACH(a, g->aliases)
+                        if (startswith(group_name, *a) == realm)
+                                return true;


Claude: suggestion: The new alias@realm matching block here is byte-for-byte identical to the one in user_record_matches_user_name() in src/shared/user-record.c (only g->/group_name vs u->/user_name differ): same strrchr(name, '@'), realm-suffix compare, and STRV_FOREACH with startswith(name, *a) == realm. Consider factoring it into a shared helper taking (name, aliases, realm) so the user and group matchers can't drift apart as one side is later tweaked.

github-actions · 2026-06-05T15:53:15Z

+
+        ASSERT_TRUE(group_record_match(g, &match));
+        userdb_match_done(&match);
+}


Claude: nit: The user test exercises both no-op branches of nss_add_valid_alias() — nss_user_alias_same_as_canonical_noop (alias equals canonical name) and nss_user_null_alias_noop (NULL alias) — but the group test has no equivalents. Since the PR's goal is symmetric alias handling, consider adding nss_group_alias_same_as_canonical_noop and nss_group_null_alias_noop so the skip-if-empty / skip-if-equal policy is covered on the group side too.

NSS lookups may resolve an alias-like name to a canonical user or group name. For example, SSSD can resolve a userPrincipalName such as "[email protected]" while returning a passwd record whose pw_name is the canonical login name "testuser". The NSS userdb worker currently converts such records using only the canonical name. As a result, the originally requested lookup name is lost, and lookups through the varlink/multiplexer path may fail with ConflictingRecordFound even though direct NSS lookup succeeds. Preserve the originally requested lookup name as an alias when it differs from the canonical passwd/group name and passes normal alias validation. Add the alias both to the parsed record fields and to the JSON representation so serialized records remain self-consistent. Extend group record loading and matching to understand aliases too. When both a name and UID/GID are specified in a userdb lookup, prefer the by-name path so alias-aware records can satisfy the subsequent consistency checks. This allows canonicalized NSS results to match the original lookup name correctly and fixes session setup for users logging in with NSS-resolvable aliases such as UPNs.

Add separate NSS conversion tests for user and group records covering canonicalized lookup aliases, invalid alias rejection, user-side alias no-op cases, group alias@realm matching, and alias-aware fuzzy matching.

bluca added the claude-review label Jun 3, 2026

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread src/shared/user-record-nss.c Outdated

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread src/shared/user-record-nss.c Outdated

github-actions Bot reviewed Jun 3, 2026

View reviewed changes

Comment thread src/shared/user-record-nss.c Outdated

github-actions Bot added util-lib please-review PR is ready for (re-)review by a maintainer labels Jun 3, 2026

poettering reviewed Jun 3, 2026

View reviewed changes

Comment thread src/shared/user-record-nss.c Outdated

poettering added reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks and removed please-review PR is ready for (re-)review by a maintainer labels Jun 3, 2026

kosstevich force-pushed the nss_aliases_support branch from 6b840fd to 8bf7bd4 Compare June 4, 2026 10:45

github-actions Bot added build-system tests meson userdb please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 4, 2026

github-actions Bot reviewed Jun 4, 2026

View reviewed changes

Comment thread src/shared/group-record.c

github-actions Bot reviewed Jun 4, 2026

View reviewed changes

kosstevich force-pushed the nss_aliases_support branch from 8bf7bd4 to d61dda5 Compare June 4, 2026 11:25

github-actions Bot reviewed Jun 4, 2026

View reviewed changes

poettering reviewed Jun 4, 2026

View reviewed changes

poettering removed the please-review PR is ready for (re-)review by a maintainer label Jun 4, 2026

poettering added the reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks label Jun 4, 2026

kosstevich force-pushed the nss_aliases_support branch from d61dda5 to d64481a Compare June 4, 2026 13:12

github-actions Bot added please-review PR is ready for (re-)review by a maintainer and removed reviewed/needs-rework 🔨 PR has been reviewed and needs another round of reworks labels Jun 4, 2026

kosstevich force-pushed the nss_aliases_support branch from d64481a to 937a144 Compare June 4, 2026 13:28

github-actions Bot reviewed Jun 4, 2026

View reviewed changes

kosstevich marked this pull request as draft June 4, 2026 17:14

kosstevich force-pushed the nss_aliases_support branch 2 times, most recently from b7791ef to 45eb03e Compare June 5, 2026 14:56

kosstevich marked this pull request as ready for review June 5, 2026 15:06

github-actions Bot reviewed Jun 5, 2026

View reviewed changes

kosstevich force-pushed the nss_aliases_support branch from 45eb03e to 9201f8a Compare June 5, 2026 15:25

github-actions Bot reviewed Jun 5, 2026

View reviewed changes

Arseniy Kostevich added 2 commits June 9, 2026 10:52

test: add NSS alias coverage for user and group records

650bb87

Add separate NSS conversion tests for user and group records covering canonicalized lookup aliases, invalid alias rejection, user-side alias no-op cases, group alias@realm matching, and alias-aware fuzzy matching.

kosstevich force-pushed the nss_aliases_support branch from 9201f8a to 650bb87 Compare June 9, 2026 08:32

Uh oh!

Conversation

kosstevich commented Jun 3, 2026

Uh oh!

github-actions Bot commented Jun 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Claude review of PR #42452 (650bb87)

Must fix

Suggestions

Nits

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Jun 3, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 4, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot Jun 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Jun 3, 2026 •

edited

Loading

Claude review of PR #42452 (`650bb87`)