From c63928633fef5cb02dce1b5f94469108fa41cc4b Mon Sep 17 00:00:00 2001 From: nadav mizrahi Date: Tue, 14 Jan 2025 14:09:15 +0200 Subject: [PATCH] NSFS | set supplemental groups dynamically to users groups Signed-off-by: nadav mizrahi --- .../AccountsAndBuckets.md | 3 +- src/native/util/os_darwin.cpp | 60 ++++++++++++++++--- src/native/util/os_linux.cpp | 52 ++++++++++++++-- src/test/system_tests/test_utils.js | 51 +++++++++++++++- .../unit_tests/test_bucketspace_versioning.js | 10 ++-- src/test/unit_tests/test_nsfs_access.js | 32 +++++++++- src/test/unit_tests/test_nsfs_versioning.js | 2 +- 7 files changed, 187 insertions(+), 23 deletions(-) diff --git a/docs/NooBaaNonContainerized/AccountsAndBuckets.md b/docs/NooBaaNonContainerized/AccountsAndBuckets.md index 27789a80b1..e1b5ac4c7a 100644 --- a/docs/NooBaaNonContainerized/AccountsAndBuckets.md +++ b/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. diff --git a/src/native/util/os_darwin.cpp b/src/native/util/os_darwin.cpp index fc6207dcf2..01ed046e7d 100644 --- a/src/native/util/os_darwin.cpp +++ b/src/native/util/os_darwin.cpp @@ -6,6 +6,8 @@ #include #include #include +#include +#include namespace noobaa { @@ -42,6 +44,55 @@ const uid_t ThreadScope::orig_uid = getuid(); const gid_t ThreadScope::orig_gid = getgid(); const std::vector ThreadScope::orig_groups = get_process_groups(); +static int +get_supplemental_groups_by_uid(uid_t uid, std::vector& 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 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& 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); } } diff --git a/src/native/util/os_linux.cpp b/src/native/util/os_linux.cpp index fdd9eb0202..3fa07f8e4a 100644 --- a/src/native/util/os_linux.cpp +++ b/src/native/util/os_linux.cpp @@ -8,6 +8,7 @@ #include #include #include +#include namespace noobaa { @@ -29,6 +30,50 @@ const gid_t ThreadScope::orig_gid = getgid(); const std::vector ThreadScope::orig_groups = get_process_groups(); +static int +get_supplemental_groups_by_uid(uid_t uid, std::vector& 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& 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)); diff --git a/src/test/system_tests/test_utils.js b/src/test/system_tests/test_utils.js index 7a6b097b5c..9cafa2fb51 100644 --- a/src/test/system_tests/test_utils.js +++ b/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; diff --git a/src/test/unit_tests/test_bucketspace_versioning.js b/src/test/unit_tests/test_bucketspace_versioning.js index db0491ba1f..6bf777ed2b 100644 --- a/src/test/unit_tests/test_bucketspace_versioning.js +++ b/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'); diff --git a/src/test/unit_tests/test_nsfs_access.js b/src/test/unit_tests/test_nsfs_access.js index 11909ce878..3d63d74654 100644 --- a/src/test/unit_tests/test_nsfs_access.js +++ b/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'); } diff --git a/src/test/unit_tests/test_nsfs_versioning.js b/src/test/unit_tests/test_nsfs_versioning.js index 81ce66cff4..e79a46ecff 100644 --- a/src/test/unit_tests/test_nsfs_versioning.js +++ b/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() {