NSFS | set supplemental groups dynamically to users groups

Signed-off-by: nadav mizrahi <[email protected]>

docs/NooBaaNonContainerized/AccountsAndBuckets.md

@@ -32,7 +32,8 @@ See all available account properties - [NC Account Schema](../../src/server/syst
   - `uid/gid/user` - An account's access key is mapped to a file system uid/gid (or user). Before performing any file system operation, NooBaa switches to the account's UID/GID, ensuring that accounts access to buckets and objects is enforced by the file system.  
 
   - `supplemental_groups` - In addition to the account main GID, an account can have supplementary group IDs that are used to determine permissions for accessing files. These GIDs are validated against a files group (GID) permissions.
-  Note: depending on the file system there may be 'sticky bit' enabled somewhere on the files path. 'sticky bit' is a user ownership access right flag that prevents other users than the file owner and root from deleting or moving files.
+  By default, supplemental groups are based on user's groups in the filesystem. In case this value was set in the CLI it will override the user's groups in the filesystem. In case this value was not set in account configuration (in the CLI) and failed to fetch the user's group in the filesystem (either because no record exists or because the operation failed), supplemental groups will be unset.
+  Note: Depending on the file system there may be 'sticky bit' enabled somewhere on the files path. 'sticky bit' is a user ownership access right flag that prevents other users than the file owner and root from deleting or moving files.
   In that case some actions will still get access denied regardless of group permissions enabled. sticky bit is denoted by `t` at the end of the permissions list (example: `drwxrwxrwt`). see https://en.wikipedia.org/wiki/Sticky_bit
 
   - `new_buckets_path` - When an account creates a bucket using the S3 protocol, NooBaa will create the underlying file system directory. This directory will be created under new_buckets_path. Note that the account must have read and write access to its `new_buckets_path`.  Must be an absolute path.  

src/native/util/os_darwin.cpp

@@ -6,6 +6,8 @@
 #include <sys/param.h>
 #include <unistd.h>
 #include <algorithm>
+#include <pwd.h>
+#include <grp.h>
 
 namespace noobaa
 {
@@ -42,6 +44,55 @@ const uid_t ThreadScope::orig_uid = getuid();
 const gid_t ThreadScope::orig_gid = getgid();
 const std::vector<gid_t> ThreadScope::orig_groups = get_process_groups();
 
+static int
+get_supplemental_groups_by_uid(uid_t uid, std::vector<gid_t>& groups)
+{
+    // getpwuid will only indicate if an error happened by setting errno. set it to 0, so will know if there is a change
+    errno = 0;
+    struct passwd* pw = getpwuid(uid);
+    if (pw == NULL) {
+        if (errno == 0) {
+            LOG("get_supplemental_groups_by_uid: no record for uid " << uid);
+        } else {
+            LOG("WARNING: get_supplemental_groups_by_uid: getpwuid failed: " << strerror(errno));
+        }
+        return -1;
+    }
+    int ngroups = NGROUPS_MAX;
+    //for some reason on mac getgrouplist accepts an array of int instead of gid_t. so need to create a vector of int and then insert it into groups
+    std::vector<int> tmp_groups(ngroups);
+    if (getgrouplist(pw->pw_name, pw->pw_gid, &tmp_groups[0], &ngroups) < 0) {
+        LOG("get_supplemental_groups_by_uid: getgrouplist failed: ngroups too small " << ngroups);
+        return -1;
+    }
+    groups.insert(groups.begin(), tmp_groups.begin(), tmp_groups.begin() + ngroups);
+    return 0;
+}
+
+/**
+ * set supplemental groups of the thread according to the following:
+ * 1. if groups were defined in the account configuration, set the groups list to the one defined
+ * 2. try to get the list of groups corresponding to the user in the system recods, and set it to it
+ * 3. if supplemental groups were not defined for the account and getting it from system record failed (either because record doesn't exist ot because of an error)
+ *    keep it as an empty set
+ */
+static void
+set_supplemental_groups(uid_t uid, gid_t gid, std::vector<gid_t>& groups) {
+    //first check if groups were defined in the account configuration
+    if (groups.empty()) {
+        if (get_supplemental_groups_by_uid(uid, groups) < 0) {
+            //aready unset by _mac_thread_setugid
+            return;
+        }
+    }
+    /*accourding to BSD Manual https://man.freebsd.org/cgi/man.cgi?query=setgroups
+    which darwin is occasionally compliant to setgroups changes the effective gid according to
+    the first element on the list. add the effective gid as the first element to prevent issues*/
+    groups.push_back(gid);
+    std::swap(groups.front(), groups.back());
+    MUST_SYS(setgroups(groups.size(), &groups[0]));
+}
+
 
 /**
  * set the effective uid/gid/supplemental_groups of the current thread using pthread_getugid_np and setgroups
@@ -60,14 +111,7 @@ ThreadScope::change_user()
 {
     if (_uid != orig_uid || _gid != orig_gid) {
         MUST_SYS(_mac_thread_setugid(_uid, _gid));
-        if (!_groups.empty()) {
-            /*accourding to BSD Manual https://man.freebsd.org/cgi/man.cgi?query=setgroups
-            which darwin is occasionally compliant to setgroups changes the effective gid according to
-            the first element on the list. add the effective gid as the first element to prevent issues*/
-            _groups.push_back(_gid);
-            std::swap(_groups.front(), _groups.back());
-            MUST_SYS(setgroups(_groups.size(), &_groups[0]));
-        }
+        set_supplemental_groups(_uid, _gid, _groups);
     }
 }
 

src/native/util/os_linux.cpp

@@ -8,6 +8,7 @@
 #include <sys/capability.h>
 #include <grp.h>
 #include <limits.h>
+#include <pwd.h>
 
 namespace noobaa
 {
@@ -29,6 +30,50 @@ const gid_t ThreadScope::orig_gid = getgid();
 
 const std::vector<gid_t> ThreadScope::orig_groups = get_process_groups();
 
+static int
+get_supplemental_groups_by_uid(uid_t uid, std::vector<gid_t>& groups)
+{
+    // getpwuid will only indicate if an error happened by setting errno. set it to 0, so will know if there is a change
+    errno = 0;
+    struct passwd* pw = getpwuid(uid);
+    if (pw == NULL) {
+        if (errno == 0) {
+            LOG("get_supplemental_groups_by_uid: no record for uid " << uid);
+        } else {
+            LOG("WARNING: get_supplemental_groups_by_uid: getpwuid failed: " << strerror(errno));
+        }
+        return -1;
+    }
+    int ngroups = NGROUPS_MAX;
+    groups.resize(ngroups);
+    if (getgrouplist(pw->pw_name, pw->pw_gid, &groups[0], &ngroups) < 0) {
+        LOG("get_supplemental_groups_by_uid: getgrouplist failed: ngroups too small " << ngroups);
+        return -1;
+    }
+    groups.resize(ngroups);
+    return 0;
+}
+
+/**
+ * set supplemental groups of the thread according to the following:
+ * 1. if groups were defined in the account configuration, set the groups list to the one defined
+ * 2. try to get the list of groups corresponding to the user in the system recods, and set it to it
+ * 3. if supplemental groups were not defined for the account and getting it from system record failed (either because record doesn't exist ot because of an error)
+ *    set it to be an empty set
+ */
+static void
+set_supplemental_groups(uid_t uid, std::vector<gid_t>& groups) {
+    //first check if groups were defined in the account configuration
+    if (groups.empty()) {
+        if (get_supplemental_groups_by_uid(uid, groups) < 0) {
+            //couldn't get supplemental groups dynamically. set it to be an empty set
+            MUST_SYS(syscall(SYS_setgroups, 0, NULL));
+            return;
+        }
+    }
+    MUST_SYS(syscall(SYS_setgroups, groups.size(), &groups[0]));
+}
+
 /**
  * set the effective uid/gid/supplemental_groups of the current thread using direct syscalls
  * we have to bypass the libc wrappers because posix requires it to syncronize
@@ -38,12 +83,7 @@ void
 ThreadScope::change_user()
 {
     if (_uid != orig_uid || _gid != orig_gid) {
-        if (_groups.empty()) {
-            MUST_SYS(syscall(SYS_setgroups, 0, NULL));
-        }
-        else {
-            MUST_SYS(syscall(SYS_setgroups, _groups.size(), &_groups[0]));
-        }
+        set_supplemental_groups(_uid, _groups);
         // must change gid first otherwise will fail on permission
         MUST_SYS(syscall(SYS_setresgid, -1, _gid, -1));
         MUST_SYS(syscall(SYS_setresuid, -1, _uid, -1));

src/test/system_tests/test_utils.js

@@ -337,7 +337,54 @@ async function delete_fs_user_by_platform(name) {
     }
 }
 
-/** 
+/**
+ * creates a new file system group. if user_name is define add it to the group
+ * @param {string} group_name
+ * @param {number} gid
+ * @param {string} [user_name]
+ */
+async function create_fs_group_by_platform(group_name, gid, user_name) {
+    if (process.platform === 'darwin') {
+        const create_group_cmd = `sudo dscl . create /Groups/${group_name} UserShell /bin/bash`;
+        const create_group_realname_cmd = `sudo dscl . create /Groups/${group_name} RealName ${group_name}`;
+        const create_group_gid_cmd = `sudo dscl . create /Groups/${group_name} gid ${gid}`;
+        await os_utils.exec(create_group_cmd, { return_stdout: true });
+        await os_utils.exec(create_group_realname_cmd, { return_stdout: true });
+        await os_utils.exec(create_group_gid_cmd, { return_stdout: true });
+        if (user_name) {
+            const add_user_to_group_cmd = `sudo dscl . append /Groups/${group_name} GroupMembership ${user_name}`;
+            await os_utils.exec(add_user_to_group_cmd, { return_stdout: true });
+        }
+
+    } else {
+        const create_group_cmd = `groupadd -g ${gid} ${group_name}`;
+        await os_utils.exec(create_group_cmd, { return_stdout: true });
+        if (user_name) {
+            const add_user_to_group_cmd = `usermod -a -G ${group_name} ${user_name}`;
+            await os_utils.exec(add_user_to_group_cmd, { return_stdout: true });
+        }
+    }
+}
+
+/**
+ * deletes a file system group. if force is true, delete the group even if its the primary group of a user
+ * @param {string} group_name
+ * @param {boolean} [force]
+ */
+async function delete_fs_group_by_platform(group_name, force = false) {
+    if (process.platform === 'darwin') {
+        const delete_group_cmd = `sudo dscl . -delete /Groups/${group_name}`;
+        const delete_group_home_cmd = `sudo rm -rf /Groups/${group_name}`;
+        await os_utils.exec(delete_group_cmd, { return_stdout: true });
+        await os_utils.exec(delete_group_home_cmd, { return_stdout: true });
+    } else {
+        const flags = force ? '-f' : '';
+        const delete_group_cmd = `groupdel ${flags} ${group_name}`;
+        await os_utils.exec(delete_group_cmd, { return_stdout: true });
+    }
+}
+
+/**
  * set_path_permissions_and_owner sets path permissions and owner and group
  * @param {string} p
  * @param {object} owner_options
@@ -742,6 +789,8 @@ exports.get_coretest_path = get_coretest_path;
 exports.exec_manage_cli = exec_manage_cli;
 exports.create_fs_user_by_platform = create_fs_user_by_platform;
 exports.delete_fs_user_by_platform = delete_fs_user_by_platform;
+exports.create_fs_group_by_platform = create_fs_group_by_platform;
+exports.delete_fs_group_by_platform = delete_fs_group_by_platform;
 exports.set_path_permissions_and_owner = set_path_permissions_and_owner;
 exports.set_nc_config_dir_in_config = set_nc_config_dir_in_config;
 exports.generate_anon_s3_client = generate_anon_s3_client;

src/test/unit_tests/test_bucketspace_versioning.js

@@ -53,7 +53,7 @@ mocha.describe('bucketspace namespace_fs - versioning', function() {
     const nested_keys_full_path = path.join(tmp_fs_root, nested_keys_bucket_path);
     const versions_path = path.join(full_path, '.versions/');
     const suspended_versions_path = path.join(suspended_full_path, '.versions/');
-    let s3_uid5;
+    let s3_uid1055;
     let s3_uid6;
     let s3_admin;
     const accounts = [];
@@ -173,8 +173,8 @@ mocha.describe('bucketspace namespace_fs - versioning', function() {
             Policy: JSON.stringify(policy)
         });
 
-        res = await generate_nsfs_account(rpc_client, EMAIL, new_bucket_path_param, { uid: 5, gid: 5 });
-        s3_uid5 = generate_s3_client(res.access_key, res.secret_key, CORETEST_ENDPOINT);
+        res = await generate_nsfs_account(rpc_client, EMAIL, new_bucket_path_param, { uid: 1055, gid: 1055 });
+        s3_uid1055 = generate_s3_client(res.access_key, res.secret_key, CORETEST_ENDPOINT);
         accounts.push(res.email);
 
         res = await generate_nsfs_account(rpc_client, EMAIL, new_bucket_path_param);
@@ -206,7 +206,7 @@ mocha.describe('bucketspace namespace_fs - versioning', function() {
 
         mocha.it('set bucket versioning - Enabled - should fail - no permissions', async function() {
             try {
-                await s3_uid5.putBucketVersioning({ Bucket: bucket_name, VersioningConfiguration: { MFADelete: 'Disabled', Status: 'Enabled' } });
+                await s3_uid1055.putBucketVersioning({ Bucket: bucket_name, VersioningConfiguration: { MFADelete: 'Disabled', Status: 'Enabled' } });
                 assert.fail(`put bucket versioning succeeded for account without permissions`);
             } catch (err) {
                 assert.equal(err.Code, 'AccessDenied');
@@ -232,7 +232,7 @@ mocha.describe('bucketspace namespace_fs - versioning', function() {
 
         mocha.it('set bucket versioning - Suspended - should fail - no permissions', async function() {
             try {
-                await s3_uid5.putBucketVersioning({ Bucket: suspended_bucket_name, VersioningConfiguration: { MFADelete: 'Disabled', Status: 'Suspended' } });
+                await s3_uid1055.putBucketVersioning({ Bucket: suspended_bucket_name, VersioningConfiguration: { MFADelete: 'Disabled', Status: 'Suspended' } });
                 assert.fail(`put bucket versioning succeeded for account without permissions`);
             } catch (err) {
                 assert.equal(err.Code, 'AccessDenied');

src/test/unit_tests/test_nsfs_access.js

@@ -20,6 +20,8 @@ mocha.describe('new tests check', async function() {
     const root_dir = 'root_dir';
     const non_root_dir = 'non_root_dir';
     const non_root_dir2 = 'non_root_dir2';
+    const test_user_name = 'test_user';
+    const test_group_name = 'test_group';
     const full_path_root = path.join(p, root_dir);
     const full_path_non_root = path.join(full_path_root, non_root_dir);
     const full_path_non_root1 = path.join(p, non_root_dir);
@@ -52,16 +54,28 @@ mocha.describe('new tests check', async function() {
         supplemental_groups: [1572, 1577], //gid of non-root1 and unrelated gid
         warn_threshold_ms: 100,
     };
+
+    const NON_ROOT4_FS_CONFIG = {
+        uid: 1575,
+        gid: 1575,
+        backend: '',
+        warn_threshold_ms: 100,
+    };
     mocha.before(async function() {
         if (test_utils.invalid_nsfs_root_permissions()) this.skip(); // eslint-disable-line no-invalid-this
         await fs_utils.create_fresh_path(p, 0o777);
         await fs_utils.file_must_exist(p);
         await fs_utils.create_fresh_path(full_path_root, 0o770);
         await fs_utils.file_must_exist(full_path_root);
+        await test_utils.create_fs_user_by_platform(test_user_name, test_user_name, NON_ROOT4_FS_CONFIG.uid, NON_ROOT4_FS_CONFIG.gid); //non root 4
+        await test_utils.create_fs_group_by_platform(test_group_name, NON_ROOT1_FS_CONFIG.gid, test_user_name); //non root 1 group
     });
 
     mocha.after(async function() {
+        this.timeout(60000); // eslint-disable-line no-invalid-this
         await fs_utils.folder_delete(p);
+        await test_utils.delete_fs_user_by_platform(test_user_name);
+        await test_utils.delete_fs_group_by_platform(test_group_name);
     });
 
     mocha.it('ROOT readdir - sucsses', async function() {
@@ -127,7 +141,23 @@ mocha.describe('new tests check', async function() {
         try {
             //non root3 doesn't have non-root2 group as supplemental group, so it should fail
             const non_root_entries = await nb_native().fs.readdir(NON_ROOT3_FS_CONFIG, full_path_non_root2);
-            assert.fail(`non root 3 has access to a folder created by user with gid not insupplemental groups - ${p} ${non_root_entries}`);
+            assert.fail(`non root 3 has access to a folder created by user with gid not in supplemental groups - ${p} ${non_root_entries}`);
+        } catch (err) {
+            assert.equal(err.code, 'EACCES');
+        }
+    });
+
+    mocha.it('NON ROOT 4 with dynamicly allocated suplemental groups - success', async function() {
+        //non root4 has non-root1 group as supplemental group, so it should succeed
+        const non_root_entries = await nb_native().fs.readdir(NON_ROOT4_FS_CONFIG, full_path_non_root1);
+        assert.equal(non_root_entries && non_root_entries.length, 0);
+    });
+
+    mocha.it('NON ROOT 4 with dynamicly allocated suplemental groups that dont contains the files gid - failure', async function() {
+        try {
+            //non root4 doesn't have non-root2 group as supplemental group, so it should fail
+            const non_root_entries = await nb_native().fs.readdir(NON_ROOT4_FS_CONFIG, full_path_non_root2);
+            assert.fail(`non root 4 has access to a folder created by user with gid not in supplemental groups - ${p} ${non_root_entries}`);
         } catch (err) {
             assert.equal(err.code, 'EACCES');
         }

src/test/unit_tests/test_nsfs_versioning.js

@@ -45,7 +45,7 @@ mocha.describe('namespace_fs - versioning', function() {
 
     const dummy_object_sdk = make_dummy_object_sdk(true);
     const dummy_object_sdk_no_nsfs_config = make_dummy_object_sdk(false);
-    const dummy_object_sdk_no_nsfs_permissions = make_dummy_object_sdk(true, 5, 5);
+    const dummy_object_sdk_no_nsfs_permissions = make_dummy_object_sdk(true, 1055, 1055);
     const ns_tmp = new NamespaceFS({ bucket_path: ns_tmp_bucket_path, bucket_id: '1', namespace_resource_id: undefined });
 
     mocha.it('set bucket versioning - Enabled - should fail - no permissions', async function() {