Permit Subid storage by username or uid#1573
Permit Subid storage by username or uid#1573jcpunk wants to merge 6 commits intoshadow-maint:masterfrom
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
lib/subordinateio.c
Outdated
| if (str2ui(&id_a, a) == -1) { | ||
| const struct passwd *pw; | ||
|
|
||
| pw = getpwnam(a); | ||
| if (NULL == pw) | ||
| return false; | ||
| id_a = pw->pw_uid; | ||
| } | ||
|
|
||
| if (str2ui(&id_b, b) == -1) { | ||
| const struct passwd *pw; | ||
|
|
||
| pw = getpwnam(b); | ||
| if (NULL == pw) | ||
| return false; | ||
| id_b = pw->pw_uid; | ||
| } |
There was a problem hiding this comment.
If we remove the streq(3) call, it might be interesting to also call getpwuid(3) if we get a number, to be 100% sure that the user exists.
We could add
const struct passwd *
getpw_uid_or_nam(const char *u)
{
uid_t uid;
return str2i(uid_t, &uid, u) == 0 ? getpwuid(uid) : getpwnam(u);
}in a new file pair lib/shadow/passwd/getpw.c (and .h).
And then call that here:
static bool
subid_owner_match(const char *a, const char *b)
{
const struct passwd *pw_a, *pw_b;
pw_a = getpw_uid_or_nam(a);
if (pw_a == NULL)
return false;
pw_b = getpw_uid_or_nam(b);
if (pw_b == NULL)
return false;
return pw_a->pw_uid == pw_b->pw_uid;
}There was a problem hiding this comment.
Is that something you'd like to see in this PR, or can it be deferred to later work?
There was a problem hiding this comment.
It's fine for later.
|
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
That is my intention. Full disclosure, I've not be very successful at running the test suite. |
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
2 times, most recently
from
618f95c to
5038d8b
Compare
| * Returns false if either identifier cannot be resolved, or if the | ||
| * resolved UIDs do not match. | ||
| */ | ||
| static bool subid_owner_match(const char *a, const char *b) |
There was a problem hiding this comment.
After tinkering with getpw_uid_or_nam(), I've been thinking that this function is really more generic than it looks.
Maybe we should call it something like is_same_user(a,b)?
It's not really constrained to subids at all. This only considers users.
There was a problem hiding this comment.
Renaming to is_same_user makes a lot of sense.
For now I'd like to keep it in this same file since there is only the one user of the behavior set if that is OK. It is a general utility function, but I'm not sure how wide to make exposure.
Let me know what you'd like and I'll try to do it.
If I rename the function commits 2 and 3 will change slightly. Those have your reviewed-by on them.
There was a problem hiding this comment.
Yeah, keeping it in this file is fine. We can move it later if/when we find more uses of it.
A simple rename would be fine.
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
5038d8b to
8a5c3a5
Compare
| if (str2ui(&owner_uid, owner) == -1) { | ||
| const struct passwd *pw; | ||
|
|
||
| pw = getpwnam(owner); | ||
| if (NULL == pw) { | ||
| errno = ENOENT; | ||
| return 0; | ||
| } | ||
| owner_uid = pw->pw_uid; | ||
| } |
There was a problem hiding this comment.
Hmmmm, this seems to be similar to the getpw_uid_or_nam() I mentioned. Maybe we should add it already, to simplify here?
There was a problem hiding this comment.
const struct passwd *pw;
pw = getpw_uid_or_nam(owner);
if (NULL == pw) {
errno = ENOENT; // I think this should actually be set within getpw_uid_or_nam().
return 0;
}
owner_uid = pw->pw_uid;There was a problem hiding this comment.
I don't have any real grip on how the lib directory is sorted...
Would that follow the behavior of getpwnam via some sort of xgetpw_uid_or_nam.c?
In that I make xgetpw_uid_or_nam.c which would look like:
/* comment */
# include headers
const struct passwd *
getpw_uid_or_nam(const char *u)
{
uid_t uid;
return str2i(uid_t, &uid, u) == 0 ? getpwuid(uid) : getpwnam(u);
}And add it to prototypes.h and Makefile.am?
Or since you wrote it would you rather put in the code yourself and have me rebase off that?
There was a problem hiding this comment.
I could write the code myself. That will make it easier for you. Thanks!
There was a problem hiding this comment.
Please pick the patch from here into your PR:
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
8a5c3a5 to
821c768
Compare
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 subid_owner_match(). 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. subid_owner_match() 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 subid_owner_match(). 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 subid_owner_match(). subid_owner_match() 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 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: shadow-maint#1554 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>
jcpunk
force-pushed
the
subid-by-username-or-uid
branch
from
821c768 to
5d87111
Compare
3 participants
This PR obsoletes #1570
The actual code changes are broken into smaller commits, but the test commit is still enormous.
Closes: #1554
I'm not sure the tests are well setup or have sufficient coverage.