SSSD / sssd

#3629 Implement sss_nss_getsidbyuid and sss_nss_etsidbygid for situations where customers define UID == GID

Closed: Fixed 5 years ago Opened 6 years ago by realrichardsharpe.

I said, on the mailing list:

We have a customer who seems to do what RHEL does with respect to users
and groups.

That is:

User A has a SID of S-1-5-21-x-y-z-1234
They also have a primary group SID of S-1-5-21-x-y-x-1235 (usually adjacent).
They are also using RFC2307bis and they have the UID property set
to 21234 and the GID property set to 21234.

Now, when the Samba idmap_sss.c calls sss_nss_getsidbyid looking for
the SID associated with the GID it often gets back the SID associated
with the UID, probably because we just asked for the SID associated
with the UID.
=============

Sumit responded with:

So it looks like sss_nss_getsidbyuid() and sss_nss_getsidbygid() should
be added to the interface.
=============

These are needed for situations like the above unless sss_nss_getsidbyid can return all SIDs found for that ID, ie both UID- and GID-relating SIDs.

lslebodn commented 6 years ago

There is a documentation comment for sss_nss_getsidbyid

/**
 * @brief Find SID by a POSIX UID or GID
 *
 * @param[in] id       POSIX UID or GID
 * @param[out] sid     String representation of the SID of the requested user
 *                     or group, must be freed by the caller
 * @param[out] type    Type of the object related to the given ID
 *
 * @return
 *  - see #sss_nss_getsidbyname
 */

and

/**
 * Object types
 */
enum sss_id_type {
    SSS_ID_TYPE_NOT_SPECIFIED = 0,
    SSS_ID_TYPE_UID,
    SSS_ID_TYPE_GID,
    SSS_ID_TYPE_BOTH /* used for user or magic private groups */
};

IMHO if client application expect GID and will get type SSS_ID_TYPE_BOTH then it should think that it is GID and not UID. But maybe I misunderstood something.

simo commented 6 years ago

Lukas,
type is an output, not an input.

This call is fully flawed, because you never know what kind of ID you passed in input, so you do not really know what you should look for. In this case the code should look for the SID associate with a specific UID but it is bringing back a different SID associated with a GID == broken.
This call should be replaced by a new one where you can specify if you are passing in a UID or a GID.

Edited 6 years ago by simo

lslebodn commented 6 years ago

Lukas,
type is an output, not an input.

I know that it is an output.

This call is fully flawed, because you never know what kind of ID you passed in input, so you do not really know what you should look for. In this case the code should look for the SID associate with a specific UID but it is bringing back a different SID associated with a GID == broken.

Could you explain the assumption why do you think that you never know what kind of ID you passed to input?

With standard glibc interface you would pass ID to either getpwuid with assumption that ID is UID or getgrgid with assumption that it is GID.
So in the beginning you already know what kind of ID you have.

Another thing which came to my mind. Correct me if I am wrong but you cannot have the same SID for user and for group. Therefore we cannot have GID = UID with id_mapping. We can only achieve it with MPG (magic private group) but in such case SID for group does not exist because primary group is virtual; so we return "virtual SID" for group.

This call should be replaced by a new one where you can specify if you are passing in a UID or a GID.

It should not be difficult but I still miss something because I cannot see any issue with using of sss_nss_getsidbyid in winbind idmap plugin provided by sssd. But reporter was probably talking about different idmap file idmap_sss.c

https://pagure.io/SSSD/sssd/blob/master/f/src/lib/winbind_idmap_sss/winbind_idmap_sss.c#_88

Edited 6 years ago by lslebodn

realrichardsharpe commented 6 years ago

@simo Yeah, that would work for us as well.

I may have to try to implement this anyway by myself, because I understand some of the code now.

simo commented 6 years ago

On Fri, 2018-02-02 at 17:05 +0000, Lukas Slebodnik wrote:

This call is fully flawed, because you never know what kind of ID
you passed in input, so you do not really know what you should look
for. In this case the code should look for the SID associate with a
specific UID but it is bringing back a different SID associated
with a GID == broken.

Could you explain the assumption why do you think that you never
know what kind of ID you passed to input?

Because there is no way to tell the function what you are asking for,
plain and simple.
You can decide if the answer is what you wanted, but that is not the
same thing.

With standard glibc interface you would pass ID to either getpwuid
with assumption that ID is UID or getgrgid with assumption that it
is GID.

It is not an "assumption", it is the function's contract, the functions
in these case are fully "id-namespaced", not so with sssd's function.

So in the beginning you already know what kind of ID you have.

You (the user) know, but the function doesn't, so it picks "at random",
it has no other way.

Another thing which came to my mind. Correct me if I am wrong but you
cannot have the same SID for user and for group.

A SID is a unique identifier, it may represent a user or a group, or
something else entirely. but that doesn't really matter, you are
passing in an unspecified "ID", not a SID.

Therefore we cannot have GID = UID with id_mapping.

This is a completely faulty assumption.
In AD you can assign arbitrary posix values to user's UIDs and group's
GIDs via RFC2307bis attributes, so you can definitely have a User SID
mapped to a UID that is numerically equal to a GID assigned to a Group
SID.

realrichardsharpe commented 6 years ago

Here is a possible patch for the problem.

I will likely be testing this later today or tomorrow.

sbose commented 6 years ago

Thank you for the patch.

I guess in src/responder/nss/nss_cmd.c instead of using CACHE_REQ_OBJECT_BY_ID it might be better to use CACHE_REQ_USER_BY_ID and CACHE_REQ_GROUP_BY_ID respectively to get the right type of object.

Btw, if you prefer you can open a pull-request against https://github.com/SSSD/sssd directly on github.

realrichardsharpe commented 6 years ago

After making the appropriate changes and enabling debug, this is what I am seeing:

(Mon Feb  5 15:11:19 2018) [sssd[nss]] [sysdb_search_object_attr] (0x0020): Search with filter [(|(&(objectclass=user)(uidNumber=30017))(&(objectclass=group)(gidNumber=30017)))] returned more than one object.
(Mon Feb  5 15:11:19 2018) [sssd[nss]] [sysdb_search_object_attr] (0x0040): Error: 22 (Invalid argument)
(Mon Feb  5 15:11:19 2018) [sssd[nss]] [cache_req_search_cache] (0x0020): CR #46: Unable to lookup [ID:30017@lin.ad.test] in cache [22]: Invalid argument
(Mon Feb  5 15:11:19 2018) [sssd[nss]] [sss_dp_req_destructor] (0x0400): Deleting request: [0x5583a76dec00:7:30017@ad.test]
(Mon Feb  5 15:11:19 2018) [sssd[nss]] [cache_req_done] (0x0400): CR #46: Finished: Error 22: Invalid argument
(Mon Feb  5 15:11:19 2018) [sssd[nss]] [nss_protocol_done] (0x4000): Sending reply: error [22]: Invalid argument

sbose commented 6 years ago

After making the appropriate changes and enabling debug, this is what I am seeing:
(Mon Feb 5 15:11:19 2018) [sssd[nss]] [sysdb_search_object_attr] (0x0020): Search with filter [(|(&(objectclass=user)(uidNumber=30017))(&(objectclass=group)(gidNumber=30017)))] returned more than one object.

This looks like the byid search filter. Did you try one of your now byuid() or bygid() requests or the old byid() one?

Did you use CACHE_REQ_USER_BY_ID and CACHE_REQ_GROUP_BY_ID as suggested earlier?

(Mon Feb 5 15:11:19 2018) [sssd[nss]] [sysdb_search_object_attr] (0x0040): Error: 22 (Invalid argument)
(Mon Feb 5 15:11:19 2018) [sssd[nss]] [cache_req_search_cache] (0x0020): CR #46: Unable to lookup [ID:30017@lin.ad.test] in cache [22]: Invalid argument
(Mon Feb 5 15:11:19 2018) [sssd[nss]] [sss_dp_req_destructor] (0x0400): Deleting request: [0x5583a76dec00:7:30017@ad.test]
(Mon Feb 5 15:11:19 2018) [sssd[nss]] [cache_req_done] (0x0400): CR #46: Finished: Error 22: Invalid argument
(Mon Feb 5 15:11:19 2018) [sssd[nss]] [nss_protocol_done] (0x4000): Sending reply: error [22]: Invalid argument