From 8969c43dc2d8d0800c2f0b509d078378db855622 Mon Sep 17 00:00:00 2001

From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina@redhat.com>

Date: Tue, 23 Jun 2020 12:05:08 +0200

Subject: [PATCH] files: allow root membership

There are two use cases that do not work with files provider:

1. User has primary GID 0:

This is fine by itself since SSSD does not store this user in cache and it is

handled only by `nss_files` so the user (`tuser`) is returned correctly. The

problem is when you try to resolve group that the user is member of. In this

case that the membership is missing the group (but only if the user was

previously resolved and thus stored in negative cache).

```

tuser:x:1001:0::/home/tuser:/bin/bash

tuser:x:1001:tuser

// tuser@files is ghost member of the group so it is returned because it is not in negative cache

$ getent group tuser

tuser:x:1001:tuser

// expire memcache

// tuser@files is ghost member but not returned because it is in negative cache

$ id tuser // returned from nss_files

uid=1001(tuser) gid=0(root) groups=0(root),1001(tuser)

[pbrezina /dev/shm/sssd]$ getent group tuser

tuser:x:1001:

```

**2. root is member of other group**

The root member is missing from the membership since it was filtered out by

negative cache.

```

tuser:x:1001:root

$ id root

uid=0(root) gid=0(root) groups=0(root),1001(tuser)

[pbrezina /dev/shm/sssd]$ getent group tuser

tuser:x:1001:

```

In files provider, only the users that we do not want to managed are stored

as ghost member, therefore we can let nss_files handle group that has ghost

members.

Tests are changed as well to work with this behavior. Users are added when

required and ghost are expected to return ENOENT.

Resolves:

https://github.com/SSSD/sssd/issues/5170

Reviewed-by: Sumit Bose <sbose@redhat.com>

---

 src/responder/nss/nss_protocol_grent.c | 18 +++++++

 src/tests/intg/files_ops.py            | 13 +++++

 src/tests/intg/test_files_provider.py  | 73 ++++++++++++++++----------

 3 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/src/responder/nss/nss_protocol_grent.c b/src/responder/nss/nss_protocol_grent.c

index 9c443d0e7..6d8e71083 100644

--- a/src/responder/nss/nss_protocol_grent.c

+++ b/src/responder/nss/nss_protocol_grent.c

@@ -141,6 +141,24 @@ nss_protocol_fill_members(struct sss_packet *packet,

     members[0] = nss_get_group_members(domain, msg);

     members[1] = nss_get_group_ghosts(domain, msg, group_name);

+    if (is_files_provider(domain) && members[1] != NULL) {

+        /* If there is a ghost member in files provider it means that we

+         * did not store the user on purpose (e.g. it has uid or gid 0).

+         * Therefore nss_files does handle the user and therefore we

+         * must let nss_files to also handle this group in order to

+         * provide correct membership. */

+        DEBUG(SSSDBG_TRACE_FUNC,

+              "Unknown members found. nss_files will handle it.\n");

+

+        ret = sss_ncache_set_group(rctx->ncache, false, domain, group_name);

+        if (ret != EOK) {

+            DEBUG(SSSDBG_OP_FAILURE, "sss_ncache_set_group failed.\n");

+        }

+

+        ret = ENOENT;

+        goto done;

+    }

+

     sss_packet_get_body(packet, &body, &body_len);

     num_members = 0;

diff --git a/src/tests/intg/files_ops.py b/src/tests/intg/files_ops.py

index c1c4465e7..57959f501 100644

--- a/src/tests/intg/files_ops.py

+++ b/src/tests/intg/files_ops.py

@@ -103,6 +103,13 @@ class FilesOps(object):

         contents = self._read_contents()

+    def _has_line(self, key):

+        try:

+            self._get_named_line(key, self._read_contents())

+            return True

+        except KeyError:

+            return False

+

 class PasswdOps(FilesOps):

     """

@@ -132,6 +139,9 @@ class PasswdOps(FilesOps):

     def userdel(self, name):

         self._del_line(name)

+    def userexist(self, name):

+        return self._has_line(name)

+

 class GroupOps(FilesOps):

     """

@@ -158,3 +168,6 @@ class GroupOps(FilesOps):

     def groupdel(self, name):

         self._del_line(name)

+

+    def groupexist(self, name):

+        return self._has_line(name)

diff --git a/src/tests/intg/test_files_provider.py b/src/tests/intg/test_files_provider.py

index 023333020..90be198c3 100644

--- a/src/tests/intg/test_files_provider.py

+++ b/src/tests/intg/test_files_provider.py

@@ -60,11 +60,13 @@ OV_USER1 = dict(name='ov_user1', passwd='x', uid=10010, gid=20010,

                 dir='/home/ov/user1',

                 shell='/bin/ov_user1_shell')

-ALT_USER1 = dict(name='altuser1', passwd='x', uid=60001, gid=70001,

+ALT_USER1 = dict(name='alt_user1', passwd='x', uid=60001, gid=70001,

                  gecos='User for tests from alt files',

                  dir='/home/altuser1',

                  shell='/bin/bash')

+ALL_USERS = [CANARY, USER1, USER2, OV_USER1, ALT_USER1]

+

 CANARY_GR = dict(name='canary',

                  gid=300001,

                  mem=[])

@@ -365,21 +367,34 @@ def setup_pw_with_canary(passwd_ops_setup):

     return setup_pw_with_list(passwd_ops_setup, [CANARY])

-def setup_gr_with_list(grp_ops, group_list):

+def add_group_members(pwd_ops, group):

+    members = {x['name']: x for x in ALL_USERS}

+    for member in group['mem']:

+        if pwd_ops.userexist(member):

+            continue

+

+        pwd_ops.useradd(**members[member])

+

+

+def setup_gr_with_list(pwd_ops, grp_ops, group_list):

     for group in group_list:

+        add_group_members(pwd_ops, group)

         grp_ops.groupadd(**group)

+

     ent.assert_group_by_name(CANARY_GR['name'], CANARY_GR)

     return grp_ops

 @pytest.fixture

-def add_group_with_canary(group_ops_setup):

-    return setup_gr_with_list(group_ops_setup, [GROUP1, CANARY_GR])

+def add_group_with_canary(passwd_ops_setup, group_ops_setup):

+    return setup_gr_with_list(

+        passwd_ops_setup, group_ops_setup, [GROUP1, CANARY_GR]

+    )

 @pytest.fixture

-def setup_gr_with_canary(group_ops_setup):

-    return setup_gr_with_list(group_ops_setup, [CANARY_GR])

+def setup_gr_with_canary(passwd_ops_setup, group_ops_setup):

+    return setup_gr_with_list(passwd_ops_setup, group_ops_setup, [CANARY_GR])

 def poll_canary(fn, name, threshold=20):

@@ -766,7 +781,9 @@ def test_gid_zero_does_not_resolve(files_domain_only):

     assert res == NssReturnCode.NOTFOUND

-def test_add_remove_add_file_group(setup_gr_with_canary, files_domain_only):

+def test_add_remove_add_file_group(

+        setup_pw_with_canary, setup_gr_with_canary, files_domain_only

+):

     """

     Test that removing a group is detected and the group

     is removed from the sssd database. Similarly, an add

@@ -776,6 +793,7 @@ def test_add_remove_add_file_group(setup_gr_with_canary, files_domain_only):

     res, group = call_sssd_getgrnam(GROUP1["name"])

     assert res == NssReturnCode.NOTFOUND

+    add_group_members(setup_pw_with_canary, GROUP1)

     setup_gr_with_canary.groupadd(**GROUP1)

     check_group(GROUP1)

@@ -817,8 +835,10 @@ def test_mod_group_gid(add_group_with_canary, files_domain_only):

 @pytest.fixture

-def add_group_nomem_with_canary(group_ops_setup):

-    return setup_gr_with_list(group_ops_setup, [GROUP_NOMEM, CANARY_GR])

+def add_group_nomem_with_canary(passwd_ops_setup, group_ops_setup):

+    return setup_gr_with_list(

+        passwd_ops_setup, group_ops_setup, [GROUP_NOMEM, CANARY_GR]

+    )

 def test_getgrnam_no_members(add_group_nomem_with_canary, files_domain_only):

@@ -911,16 +931,19 @@ def test_getgrnam_ghost(setup_pw_with_canary,

                         setup_gr_with_canary,

                         files_domain_only):

     """

-    Test that a group with members while the members are not present

-    are added as ghosts. This is also what nss_files does, getgrnam would

-    return group members that do not exist as well.

+    Test that group if not found (and will be handled by nss_files) if there

+    are any ghost members.

     """

     user_and_group_setup(setup_pw_with_canary,

                          setup_gr_with_canary,

                          [],

                          [GROUP12],

                          False)

-    check_group(GROUP12)

+

+    time.sleep(1)

+    res, group = call_sssd_getgrnam(GROUP12["name"])

+    assert res == NssReturnCode.NOTFOUND

+

     for member in GROUP12['mem']:

         res, _ = call_sssd_getpwnam(member)

         assert res == NssReturnCode.NOTFOUND

@@ -932,7 +955,10 @@ def ghost_and_member_test(pw_ops, grp_ops, reverse):

                          [USER1],

                          [GROUP12],

                          reverse)

-    check_group(GROUP12)

+

+    time.sleep(1)

+    res, group = call_sssd_getgrnam(GROUP12["name"])

+    assert res == NssReturnCode.NOTFOUND

     # We checked that the group added has the same members as group12,

     # so both user1 and user2. Now check that user1 is a member of

@@ -1027,28 +1053,21 @@ def test_getgrnam_add_remove_ghosts(setup_pw_with_canary,

     modgroup = dict(GROUP_NOMEM)

     modgroup['mem'] = ['user1', 'user2']

     add_group_nomem_with_canary.groupmod(old_name=modgroup['name'], **modgroup)

-    check_group(modgroup)

+    time.sleep(1)

+    res, group = call_sssd_getgrnam(modgroup['name'])

+    assert res == sssd_id.NssReturnCode.NOTFOUND

     modgroup['mem'] = ['user2']

     add_group_nomem_with_canary.groupmod(old_name=modgroup['name'], **modgroup)

-    check_group(modgroup)

+    time.sleep(1)

+    res, group = call_sssd_getgrnam(modgroup['name'])

+    assert res == sssd_id.NssReturnCode.NOTFOUND

     res, _ = call_sssd_getpwnam('user1')

     assert res == NssReturnCode.NOTFOUND

     res, _ = call_sssd_getpwnam('user2')

     assert res == NssReturnCode.NOTFOUND

-    # Add this user and verify it's been added as a member

-    pwd_ops.useradd(**USER2)

-    # The negative cache might still have user2 from the previous request,

-    # flushing the caches might help to prevent a failed lookup after adding

-    # the user.

-    subprocess.call(["sss_cache", "-E"])

-    res, groups = sssd_id_sync('user2')

-    assert res == sssd_id.NssReturnCode.SUCCESS

-    assert len(groups) == 2

-    assert 'group_nomem' in groups

-

 def realloc_users(pwd_ops, num):

     # Intentionally not including the last one because

-- 

2.21.3