Permit Subid storage by username or uid#1573
Merged
hallyn merged 8 commits intoshadow-maint:masterfrom Mar 30, 2026
Merged
Conversation
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
42149b0 to
ceac681
Compare
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
ceac681 to
517b0bf
Compare
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
517b0bf to
2dced79
Compare
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
3 times, most recently
from
23535c7 to
5cf0253
Compare
Collaborator
|
Just to make sure I understand the goal of this PR correctly. You'd like to store the user/group ID instead of the username/groupname in |
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
5cf0253 to
2d7e383
Compare
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
5f4d0fd to
d22e9ca
Compare
alejandro-colomar
approved these changes
Mar 13, 2026
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
d22e9ca to
8478a00
Compare
Collaborator
|
Thanks! I'll merge. |
Collaborator
|
Hmmm, there's a CI job failing. I've tried re-running it to see if it's just a spurious failure. Let's see. |
Collaborator
|
Hmmm, the testsuite failure seems consistent. Could you please have a look at the test and see why it's failing? Cc: @hallyn |
Contributor
Author
|
I'm guess that, because the queries now require the user to resolve to a real identity that the checks for user " |
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
2 times, most recently
from
060f6f9 to
c0dfadc
Compare
Contributor
Author
|
Digging a bit deeper, I'm showing results I don't understand... It appears that Here is my diff to make print outs: --- a/lib/shadow/passwd/getpw.h
+++ b/lib/shadow/passwd/getpw.h
@@ -23,6 +23,14 @@ getpw_uid_or_nam(const char *u)
{
uid_t uid;
+printf("AAA: %s\n", u);
+if (get_uid(u, &uid) == 0) {
+getpwuid(uid);
+printf("BBB: %s\n", getpwuid(uid)->pw_name);
+} else {
+printf("CCC: %s\n", getpwnam(u)->pw_name);
+}
+
return get_uid(u, &uid) == 0 ? getpwuid(uid) : getpwnam(u);
}
diff --git a/lib/subordinateio.c b/lib/subordinateio.c
index 37e5100a..d41cba21 100644
--- a/lib/subordinateio.c
+++ b/lib/subordinateio.c
@@ -151,6 +151,8 @@ is_same_user(const char *a, const char *b)
const struct passwd *pw_a;
const struct passwd *pw_b;
+printf("XXXXX: does names %s == %s?\n", a, b);
+
pw_a = getpw_uid_or_nam(a);
if (NULL == pw_a)
return false;
@@ -159,6 +161,8 @@ is_same_user(const char *a, const char *b)
if (NULL == pw_b)
return false;
+printf("YYY: does ID %s == %s?\n", pw_a->pw_name, pw_b->pw_name);
+printf("YYY: does ID %i == %i?\n", pw_a->pw_uid, pw_b->pw_uid);
return pw_a->pw_uid == pw_b->pw_uid;
}
@@ -882,6 +886,7 @@ int list_owner_ranges(const char *owner, enum subid_type id_type, struct subid_r
commonio_rewind(db);
while (NULL != (range = commonio_next(db))) {
+printf("The flag is: %s\n", is_same_user(range->owner, owner) ? "true" : "false");
if (is_same_user(range->owner, owner)) {
ranges = append_range(ranges, range, count++);
if (ranges == NULL) {And I get the following output from the test with my updated ranges: Which shows at some point before |
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
c0dfadc to
4bafb0a
Compare
Contributor
Author
|
Found the fix, the retrieved structure from This resulted in a code change to commit 2 " |
Collaborator
Ahhh, yes, now it makes sense. getpwuid(3) and getpwnam(3) reuse static memory. We should copy the contents. Thanks! I'll review the changes. |
Comment on lines
17
to
37
| change_config | ||
|
|
||
| echo -n "list foo's ranges..." | ||
| ${build_path}/src/getsubids foo > /tmp/subuidlistout | ||
| ${build_path}/src/getsubids -g foo > /tmp/subgidlistout | ||
| ${build_path}/src/getsubids nobody > /tmp/subuidlistout | ||
| ${build_path}/src/getsubids -g nobody > /tmp/subgidlistout | ||
| echo "OK" | ||
|
|
||
| echo -n "Check the subuid ranges..." | ||
| [ $(wc -l /tmp/subuidlistout | awk '{ print $1 }') -eq 2 ] | ||
| grep "0: foo 300000 10000" /tmp/subuidlistout | ||
| grep "1: foo 400000 10000" /tmp/subuidlistout | ||
| [ $(wc -l /tmp/subuidlistout | awk '{ print $1 }') -eq 3 ] | ||
| grep "0: nobody 200000 10000" /tmp/subuidlistout | ||
| grep "1: nobody 300000 10000" /tmp/subuidlistout | ||
| grep "2: nobody 400000 10000" /tmp/subuidlistout | ||
| echo "OK" | ||
|
|
||
| echo -n "Check the subgid ranges..." | ||
| [ $(wc -l /tmp/subgidlistout | awk '{ print $1 }') -eq 1 ] | ||
| grep "0: foo 200000 10000" /tmp/subgidlistout | ||
| [ $(wc -l /tmp/subgidlistout | awk '{ print $1 }') -eq 2 ] | ||
| grep "0: nobody 200000 10000" /tmp/subgidlistout | ||
| grep "1: nobody 210000 10000" /tmp/subgidlistout | ||
| echo "OK" | ||
|
|
||
| log_status "$0" "SUCCESS" |
Collaborator
There was a problem hiding this comment.
I'm not too familiar with this part. @ikerexxe , @hallyn , @stoeckmann , would you mind having a look at the last commit?
Collaborator
There was a problem hiding this comment.
I'm not really sure what the test is doing. I don't see where nobody's sub*ids come from.
Contributor
Author
There was a problem hiding this comment.
I thought I added the ranges in this commit https://github.com/jcpunk/shadow/commit/6a55ab4b603cdb710316d764460c2124e339eae8
Collaborator
There was a problem hiding this comment.
You did. I couldn't include them in the same quote, as they're in different files.
Each identifier is resolved via getpw_uid_or_nam and compared numerically. Accepts usernames, numeric UIDs, or a mix of both. Numeric resolution ensures stale entries for deleted users cannot produce false matches, and handles systems with overlapping UIDs. We do not perform a streq(3) on the passed strings themselves. In this way we ensure the user is resolvable on the system eliminating the ability to return results from deleted users with stale entries. Returns false if either identifier cannot be resolved, or if the resolved UIDs do not match. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
Replace direct owner string comparison in range_exists, get_owner_id, list_owner_ranges, and new_subid_range with is_same_user(). Previously, ownership checks required a literal match against the owner field. This created a situation where if entries were recorded by UID and by username not all would be identified. is_same_user() resolves ownership by username, UID, and overlapping entries, making range queries consistent with how ownership is determined elsewhere. Fixes: 0a7888b (2020-06-07; "Create a new libsubid") CC: Serge Hallyn <serge@hallyn.com> Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
Replace the resolution in find_range with is_same_user(). The actualy results should be the same. Previously, find_range performed its own getpwnam() based UID lookup to handle entries recorded by UID or by username. This duplicated logic centralized in is_same_user(). is_same_user() resolves ownership by username, UID, and overlapping entries, making find_range consistent with how ownership is determined elsewhere. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
They are not active within this commit, but they are fully documented Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
This adds two new options to /etc/login.defs: * SUB_UID_STORE_BY_UID * SUB_GID_STORE_BY_UID They default to 'no' but when set 'yes' the subuid/subgid entries will be written by uid rather than username. Closes: #1554 Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
There are no longer any callers for get_owner_id so it can be removed. Reviewed-by: Alejandro Colomar <alx@kernel.org> Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
Now that user resolution is required, ensure a resolvable user is used. Signed-off-by: Pat Riehecky <riehecky@fnal.gov>
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
4bafb0a to
6a55ab4
Compare
Contributor
Author
|
Just double checking that nothing is waiting on me. I was out of office and kinda lost track. |
Collaborator
hallyn
approved these changes
Mar 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR obsoletes #1570
The actual code changes are broken into smaller commits.
Closes: #1554
I'm not sure the tests are well setup or have sufficient coverage.