Revert "sub_[ug]id_{add,remove}: fix return values"

[ikerexxe]

This reverts commit a80b792.

It is possible to use another subid provider such as a FreeIPA server. This does not mean that a local user cannot have local subid ranges.

Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=2249524

Test is available at SSSD/sssd#7037

[hallyn]

In the commit message, you say:

It is possible to use another subid provider such as a FreeIPA server. This does not mean that a local user cannot have local subid ranges.

In fact, "8492dee66: subids: support nsswitch" says:

    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

The fedora bug says: "Useradd should be able to add users to /etc/passwd etc without subids" and "Userdel should not require that subids exist in the subid files in order to remove users."
Those both sound right, but sound like they point to a bug different than what you are fixing here. Am I wrong about that? (I must be misunderstanding something...)

[hallyn]

In fact, looking at useradd.c, it assumes sub_gid_add return value of 0 means error and anything else means success. So going back to returning -EOPNOTSUPP if the nss handle connection succeeds, ends up meaning that the error is ignored.

So we should first decide what behavior exactly we want, and then fix the mismatch between caller expectations and callee return values in the sub_uid_*() fns.

In the fedora case, why not update login.defs to say no subuids? I assume they do not in fact want subuids to also be allocated in /etc/subuid right?

[ikerexxe]

I considered other options, but those involved changing the prototypes for nss_init() and get_subid_nss_handle(), and this was the fastest path.

So we should first decide what behavior exactly we want, and then fix the mismatch between caller expectations and callee return values in the sub_uid_*() fns.

I agree that we should agree on the behaviour first.

IMO, if the subid provider isn't local, then useradd and userdel shouldn't fail when they trying to add/delete subids for a user. Do you agree?

This would mean changing nss_init() and get_subid_nss_handle() to return the exact reason why they failed. This way, the caller can decide whether it should fail or not.

In fact, "8492dee66: subids: support nsswitch" says:

    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

Thanks for reminding me this. I don't think we should consider any fallback. Remote users bring their own subids, and local users have their own subids locally, so no need to fix things. At least for the moment.

In the fedora case, why not update login.defs to say no subuids? I assume they do not in fact want subuids to also be allocated in /etc/subuid right?

Do we have an option in login.defs to enable subids?

By the way, did I forget any other requirement that we should take into account to solve this issue?

[alexey-tikhonov]

Hi,

So we should first decide what behavior exactly we want, and then fix the mismatch between caller expectations and callee return values in the sub_uid_*() fns.

Imo, behavior should be consistent in the following sense: a configured subid db should be used consistently in all operations (read/write/delete).

Since write/delete isn't supported by remote db "by design" (i.e. this is expected), caller of those functions should ignore EOPNOTSUPP and treat it as "no error". How to achieve this (to handle ret=0/errno=EOPNOTSUPP or ret=-EOPNOTSUPP or whatever) is an implementation detail.

[hallyn]

Since write/delete isn't supported by remote db "by design" (i.e. this is expected), caller
of those functions should ignore EOPNOTSUPP and treat it as "no error".

We're having an API definition problem: when src/useradd.c calls sub_uid_add(), right now that explictly means "ask the local /etc/subuid for a subuid range". That is, we do not have a way to ask the remote NSS specified provider for the range. We expect that to be provided out of band. Right? So in that case, the sub_uid_add() call should return EOPNOTSUPP, and the right thing to do would be to configure the local system to not provide subuids, so as to avoid that call altogether.

If we want to be requesting a subuid range from an NSS specified provider for new user, we need a way for sub_uid_add() to ask for that.

[hallyn]

Do we have an option in login.defs to enable subids?

Yes, setting SUB_UID_COUNT to 0 should disable subids.

[hallyn]

Of course, lib/subordinateio.c:add_range() returns 1 (success) if have_range(). Maybe sub_uid_add() should check the nss range.

[ikerexxe]

@hallyn I proposed the issue reporter to change SUB_*ID_COUNT to 0, and that works but only if he sets it in the prefixed directory.

I haven't used this option, so I wonder if this is the expected behaviour. --root specifically mentions that it'll use the configuration files from the chrooted directory, but --prefix doesn't (it only applies them).

[mhjacks]

Ping? I'm the original reporter (this started out as a bug for mock in Fedora) and it has taken some interesting turns as noted in the comments. My interest in this is to be able to have mock successfully create a user and group in a chroot on a host system that is using FreeIPA-provided identities (and specifically subuid/subgids)

[hallyn]

Well, I don't really know who all is using this and how. It probably wouldn't be the worst thing in the world if we always also did a local files subuid addition, even if NSS module is set. What it would mean is that when you then query the subid ranges, you'd get the NSS range which would likely be different from what was in /etc/subids. But maybe that's ok.

[hallyn]

Note that if we decide to do that, the thing to do is not revert this patch, but rather have a new patch which (a) very explictly documents in each of the 4 cases what we are doing, and then just call add_range unconditionally. E.g.:

 736 /*
 737  * sub_gid_add: add a subgid range.  Note that if an NSS module is
 738  * defined, this will set a range in local files, which will then be
 739  * ignored when querying.
 740  *
 741  * Return 1 if the range is already present or on success.  On error
 742  * return 0 and set errno appropriately.
 743  */
 744 int sub_gid_add (const char *owner, gid_t start, unsigned long count)
 745 {
 746         return add_range (&subordinate_gid_db, owner, start, count);
 747 }

Alterntatively, we could aim to implement the NSS add_range. The NSS servers would then be expected to see "hey joe already has a range, so I'll just return 1". Main concern there is privilege of the local host over the NSS server to add such a range.

[mhjacks]

For the mock case - the idea is to build packages inside the chroot. mock does not itself care about subuids inside the chroot; so it's quite possible that a different approach might be the right one here - something that would allow management of the local files inside the chroot without any reference to what the host configuration is. (The documentation would seem to indicate that this is what --root is supposed to do, but operational experience would suggest otherwise.) @praiskup is a key contributor to mock and can say more about it.

I don't see as strong of a case for an actually-used IPA-provided subid inside a chroot, but maybe a use for that exists. But in that case, I would think that there should be a first-class mechanism for handling chroots from the host-system side (this is something Pavel said we can do in mock if needed).

[ikerexxe]

Note that if we decide to do that, the thing to do is not revert this patch, but rather have a new patch which (a) very explictly documents in each of the 4 cases what we are doing, and then just call add_range unconditionally. E.g.:

I'm keeping this PR open as a point of discussion rather than anything else. I have moved the PR to draft to make this point clearer.

For the mock case - the idea is to build packages inside the chroot. mock does not itself care about subuids inside the chroot; so it's quite possible that a different approach might be the right one here - something that would allow management of the local files inside the chroot without any reference to what the host configuration is. (The documentation would seem to indicate that this is what --root is supposed to do, but operational experience would suggest otherwise.) @praiskup is a key contributor to mock and can say more about it.

This is what I'd like to understand. Maybe the issue isn't in the subid management itself, but in the way we handle the configuration files for chroot.

[mhjacks]

There might be a second question, which would be "how should deletion be handled when subids are not represented in local files" - it seems that if the subids were never there to begin with (or have been judged by system configuration to be irrelevant) that deletion should proceed. The choice otherwise is an out-of-band edit of the files, e.g. to remove the passwd/shadow lines and possibly a group entry. But we mostly avoid this question by answering the one about chroot's above.

[hallyn]

@mhjacks right, what to do with any updates in the face of NSS modules was never clear, which was why we punted by insisting that they are n/a, that is, subid assignments are simply out of band.

I would expect pretty much any subid nss server to not want to give any client permissions to create subid ranges. But so maybe it makes sense for the client (usermod) to call a nss->sub_uid_add(owner, start, count), and let the nss module check whether the range exists, return 1 if so, and return 0 (failure) if not.

[mhjacks]

@hallyn I agree it would be unlikely for centrally managed subids to be influenced by client settings. To be plain about it, I see two problems here:

1. shadow-tools seem to insist that subids are represented in local files, even when the configuration specifies that they are not being kept in local files (as a condition for successfully creating or deleting a user). I agree the situation might be a bit murky here; but in the deletion case at least, not finding a subid range should not be a reason to cause the delete to fail. (This issue becomes entirely moot to the mock case if the next issue is resolved.)

2. It is not clear that the --root option works as documented, at least in Fedora. Per the documentation, --root should edit the files under CHROOT_DIR using the chroot's configuration, but this is not happening, at least in the current Fedora release. This is specifically what is causing the issue with mock. It seems, based on strace, that even with the --root option specified, shadow-tools are still using the host (i.e. outside the chroot) config.

[ikerexxe]

As a side note to Martin's point, Fedora has reduced its downstream only patches to a minimum, and at a quick glance none of them affect chroot or subid environments. Therefore, the issues he raised are also present in this repository.

[mhjacks]

Ping?

[ikerexxe]

The discussion has been moved to #897, so I'm closing this PR.