Subids: support nsswitch

[hallyn]

When starting any operation to do with subuid delegation, check
nsswitch for a module to use. If none is specified, then use
the traditional /etc/subuid and /etc/subgid files.

Currently only one module is supported, and there is no fallback
to the files on errors. Several possibilities could be considered:

1. in case of connection error, fall back to files
2. in case of unknown user, also fall back to files

etc...

When non-files nss module is used, functions to edit the range
are not supported. It may make sense to support it, but it also
may make sense to require another tool to be used.

libsubordinateio also uses the nss_ helpers. This is how for instance
lxc could easily be converted to supporting nsswitch.

Add a set of test cases, including a dummy libsubid_zzz module. This
hardcodes values such that:

'ubuntu' gets 200000 - 300000
'user1' gets 100000 - 165536
'error' emulates an nss module error
'unknown' emulates a user unknown to the nss module
'conn' emulates a connection error ot the nss module

[hallyn]

I'll address your feedback with appended patches (which I will squash in before merging) soon, but probably not today.

[alexey-tikhonov]

Hi @hallyn,

thank you for opening PR.

Please see few inline comments.

Besides that I think this PR should update libsubid/api.c as well: currently get_sub?id_ranges() and get_sub?id_owners() unconditionally call sub_?id_open(), sub_?id_close(), etc. I think this is at least redundant and might even cause issues (for example, if network source is configured and /etc/sub?id files are inaccessible for some reason).

It's even more prominent with grant/free_sub?id_range(): besides the same issue with unconditional execution of sub_?id_open/lock/unlock/close(), it operates on files directly, bypassing sub_?id_add/remove(). I think those function should return EOPNOTSUPP promptly in case nsswitch plugin configured.

Additionally, could you please clarify, if const char *owner means:

• "username",
• text representation of "UID"
• or both [should be] supported
  in subid_nss_ops and in libsubid/api.h?

[rhatdan]

Thanks @hallyn This looks good.

[hallyn]

I probably need to have configure.ac look for yacc+libtool (per my .travis.yaml update), then depending on the open thread it should be ready.

[alexey-tikhonov]

Hi @hallyn,

thanks for an update. Good progress.

In the comments to

(*find_subid_owners)(unsigned long id, uid_t **uids, enum subid_type id_type, int *count);

'owner' is described instead of 'id'.
And I still think order of arguments is weird - don't you think it makes sense to move id_type before uids? Currently input arg fits between output arguments.

And what do you think about #321 (comment) ?
Namely:

• libsubid/api.c does some files operations unconditionally
• what are the expectations towards meaning of "owner" (UID/username/both) in subid_nss_ops and in libsubid/api.h?

[hallyn]

In the comments to

(*find_subid_owners)(unsigned long id, uid_t **uids, enum subid_type id_type, int *count);

'owner' is described instead of 'id'.
And I still think order of arguments is weird - don't you think it makes sense to move id_type before uids? Currently input arg fits between output arguments.

Yes, id_type before the output value uids makes sense. Sorry - if you've mentioned this before, I don't remember it.

[hallyn]

Yeah, I can rename those. I also was going to actually comment the api.h functions :) But maybe as a separate commit since technically that's libsubid, and nss support is a core shadow feature.

[hallyn]

BTW I do notice that it's a bit inconsistent that find_subid_owners returns uids while the others take a char *username.

I'm loath to change that though - since we can have more than one name per uid, it seems sensible

[hallyn]

@ikerexxe

Straight to the point, I fear that one of the names of the header for exposing the library is not correct. I see api.h and subid.h as the header names. api.h can be a sensible name for shadow-utils repository, but once you include the file in a distribution you don't know what you are handling as there are lots of packages providing APIs. Would you mind merging those two files into subid.h? Another option would be to rename both files, something like subid_api.h and subid_data.h. This is the approach that I have taken so far in Fedora.

I've merged them into subid.h. Since you're already massaging the files in your package, can you verify that the way I have it will work for you?

If so, then I'll merge.

[alexey-tikhonov]

Thank you, @hallyn.

[hallyn]

@alexey-tikhonov @giuseppe @AkihiroSuda @ikerexxe @rhatdan

Thanks for all your helpful comments.

[ikerexxe]

@hallyn thanks to you for your work.

I've merged them into subid.h. Since you're already massaging the files in your package, can you verify that the way I have it will work for you?

Yes, that's perfectly fine.

[alexey-tikhonov]

Hi,

not sure if this is a best place, but I'd like to discuss one thing.

Currently subuid.h::get_subuid_ranges() -> list_owner_ranges() -> get_subid_nss_handle() -> nss_init() -> fprintf(stderr, ...)

That's ok for new?idmap to print to stderr, but I'm not so sure about libsubid...

@hallyn , what do you think?

[hallyn]

Hi,

not sure if this is a best place, but I'd like to discuss one thing.

Currently subuid.h::get_subuid_ranges() -> list_owner_ranges() -> get_subid_nss_handle() -> nss_init() -> fprintf(stderr, ...)

That's ok for new?idmap to print to stderr, but I'm not so sure about libsubid...

@hallyn , what do you think?

Thanks, good point. We shouldn't do that. Do you mind opening a new issue for that? If so (if you mind :) I'll open one later.

[giuseppe]

one potential issue I've found is that subid.h is not installed:

$ make install DESTDIR=$(pwd)/dest >/dev/null && find dest -name subid.h
libtool: warning: remember to run 'libtool --finish /lib'
$

[alexey-tikhonov]

Hi @hallyn ,

looking at the usage of has_any_range()/has_range() in existing code, I have following questions:

1. newusers: sub_uid_assigned() -> nsswitch::has_any_range(), so if network source is configured in nsswitch.conf then newusers consults network source instead of /etc/subid, and this doesn't make much sense, imo
   And this is the only use of has_any_range() I've found. Do we need this function in a plugin at all if only local user manipulation tools use it?

2. useradd: sub_uid_add() -> !files => -EOPNOTSUPP. Again, if network source is configured in nsswitch.conf then useradd will fail to update /etc/subuid

[alexey-tikhonov]

Btw, if we suppose that a given subid can be used by multiple users (uids), then probably usage of has_range() in user_busy.c became wrong now (subrange doesn't identify owner anymore).

[alexey-tikhonov]

Another one question about get_sub?id_ranges()/list_owner_ranges(): do we really need owner field in struct subordinate_range? Or do we expect those functions to populate this field?
Since those functions do a lookup for a specified owner, this field will clearly be the same in all ranges (and must match in arg). Feels like useless strcpy()'s...

UPD: it seems grant/ungrant rely on this field.
But doing actual implementation of list_owner_ranges(), it feels excessive to allocate every range and then strdup owner (and do a proper cleanup if any of those fails) instead of single allocation of an array of structs with 2 ints...

[hallyn]

So maybe we should have something like:

struct subordinate_id_range {
unsigned long start;
unsigned long count;
};

struct subordinate_delegated {
const char *owner;
struct subordinate_id_range range;
};

? Although simply saying that it's ok to leave the subordinate_range->owner
NULL when those functions return would be ok with me too. That might become an
issue for users who want to do several calls for different users, combine the
results, and then parse the collection. But even they may not want to do all
that freeing later.

So if you like we can add a comment in these function descriptions saying
that owner may be returned as NULL.

[hallyn]

one potential issue I've found is that subid.h is not installed:

$ make install DESTDIR=$(pwd)/dest >/dev/null && find dest -name subid.h
libtool: warning: remember to run 'libtool --finish /lib'
$

Should be fixed with #330

[hallyn]

Hi @hallyn ,

looking at the usage of has_any_range()/has_range() in existing code, I have following questions:

1. newusers: sub_uid_assigned() -> nsswitch::has_any_range(), so if network source is configured in `nsswitch.conf` then `newusers` consults network source instead of /etc/subid, and this doesn't make much sense, imo
   And this is the only use of `has_any_range()` I've found. Do we need this function in a plugin at all if only local user manipulation tools use it?

2. useradd: sub_uid_add() -> !files => -EOPNOTSUPP. Again, if network source is configured in `nsswitch.conf` then `useradd` will fail to update /etc/subuid

Would you mind creating a new issue for these as well? It'll be very hard to track conversation in this issue.

I don't mind dropping has_any_range(), I think I thought something like lxc-usernsexec could use it, but it can just as well just ask for the ranges and get back -ENOENT.

For the second, I think you're saying that useradd should ignore the failure to add subids?

[alexey-tikhonov]

1. newusers: sub_uid_assigned() -> nsswitch::has_any_range(), so if network source is configured in nsswitch.conf then newusers consults network source instead of /etc/subid, and this doesn't make much sense, imo
   And this is the only use of has_any_range() I've found. Do we need this function in a plugin at all if only local user manipulation tools use it?

2. useradd: sub_uid_add() -> !files => -EOPNOTSUPP. Again, if network source is configured in nsswitch.conf then useradd will fail to update /etc/subuid

Would you mind creating a new issue for these as well? It'll be very hard to track conversation in this issue.

Ok: #331. Just wasn't sure if you see this as an issue.

I don't mind dropping has_any_range(), I think I thought something like lxc-usernsexec could use it, but it can just as well just ask for the ranges and get back -ENOENT.

SUBID_STATUS_SUCCESS && (*count == 0), to be precise.

For the second, I think you're saying that useradd should ignore the failure to add subids?

No, I meant a little bit different thing.
What is our general approach to a case: "nsswitch has network source configured" and "local user manipulation are being done"?
I think local tools, that add/modify local users, should work with /etc/ even if "network source" is enabled in nsswitch?

[alexey-tikhonov]

struct subordinate_id_range {
unsigned long start;
unsigned long count;
};

struct subordinate_delegated {
const char *owner;
struct subordinate_id_range range;
};

? Although simply saying that it's ok to leave the subordinate_range->owner
NULL when those functions return would be ok with me too. That might become an
issue for users who want to do several calls for different users, combine the
results, and then parse the collection. But even they may not want to do all
that freeing later.

So if you like we can add a comment in these function descriptions saying
that owner may be returned as NULL.

I think my preference would be a solution with two structs (this is very similar to what I have in SSSD's plugin now).
This would allow list_owner_ranges() to work with subordinate_id_range ** (instead of subordinate_delegated ***) - single malloc instead of (1 + count * 2) malloc's...