[Pkg-shadow-devel] Newuidmap works with usernames instead of uids

[Bostjan Skufca]

>> I created new branch for this one, for you to merge one single commig:
>> https://github.com/bostjan/shadow/commits/feature/subuidgid-numeric-v2
>
> My 2c:
> Do you need to include <grp.h>?

Leftovers. When I started looking into this I was under impression
that uidmap is determined by user's UID and gidmap is determined by
user's GID. It turned out getgwnam() was not needed at all, as
everything is UID-based.


> You could avoid the getpwnam_r call (and malloc, sysconf) by calling
> getpwnam. You just need to store owner_pwd.pw_uid before you enter the
> loop.
> This would also avoid the call to exit(), which IMO should be avoided in
> commonio.c (better return an error code and let the applications deal with
> the error - this can let them release a lock for example).

So obvious :)
(I copy-pasted the code strainght from getpwnam man page, that is why
exit()s were there instead of proper error handling.)

Unfortunately there is not a single instance of error handling in
lib/subordinateio.c to take as example, and I am not sure that calling
code knows how to handle errors other than "requested uidmap not
allowed", which is what "return NULL;" stands for.

Therefore ideas about how to do proper error handling are very
welcome, but I think they belong to a separate thread.


> I'm not sure regarding the guarantees you have on the size of pw_uid.
> I would recommend:
>         sprintf(owner_uid_string, "%lu", (unsigned long int)owner_pwd.pw_uid);

Sensible, fixed.


> This is rather cosmetic, but
>                         if (!range_owner_pwd) {
> could be changed to:
>                         if (NULL == range_owner_pwd) {

Future-proofing of course, fixed that too.


Commit with fixes to review:
https://github.com/bostjan/shadow/commit/d21214c9ba823edd5ac82ae2b1b03f4c219c3841

Branch to actually merge (contains single commit with everything
squashed into it):
https://github.com/bostjan/shadow/tree/feature/subuidgid-numeric-v3


@Serge:
Maybe it would be for the best if you coordinated with Christian
inclusion of this patch into upstream, as you are already "in the
upstream-patch-pushing zone" :)


b.