NSFS | NC | add option to set account supplemental groups

Signed-off-by: nadav mizrahi <[email protected]>
noobaa · Nov 24, 2024 · 9abd913 · 9abd913
1 parent 412e804
commit 9abd913
Show file tree

Hide file tree

Showing 15 changed files with 179 additions and 28 deletions.
diff --git a/src/api/account_api.js b/src/api/account_api.js
@@ -312,7 +312,8 @@ module.exports = {
                             uid: { type: 'number' },
                             gid: { type: 'number' },
                             new_buckets_path: { type: 'string' },
-                            nsfs_only: { type: 'boolean' }
+                            nsfs_only: { type: 'boolean' },
+                            supplemental_groups: { type: 'string'},
                         }
                     },
                 },

diff --git a/src/api/common_api.js b/src/api/common_api.js
@@ -1310,7 +1310,8 @@ module.exports = {
                     uid: { type: 'number' },
                     gid: { type: 'number' },
                     new_buckets_path: { type: 'string' },
-                    nsfs_only: { type: 'boolean' }
+                    nsfs_only: { type: 'boolean' },
+                    supplemental_groups: { type: 'string'},
                 }
             }, {
                 type: 'object',

diff --git a/src/cmd/manage_nsfs.js b/src/cmd/manage_nsfs.js
@@ -338,6 +338,7 @@ async function fetch_account_data(action, user_input) {
             distinguished_name: user_input.user,
             uid: user_input.user ? undefined : user_input.uid,
             gid: user_input.user ? undefined : user_input.gid,
+            supplemental_groups: _.isUndefined(user_input.supplemental_groups) ? undefined : String(user_input.supplemental_groups),
             new_buckets_path: user_input.new_buckets_path,
             fs_backend: user_input.fs_backend ? String(user_input.fs_backend) : config.NSFS_NC_STORAGE_BACKEND
         }

diff --git a/src/manage_nsfs/manage_nsfs_cli_errors.js b/src/manage_nsfs/manage_nsfs_cli_errors.js
@@ -346,6 +346,11 @@ ManageCLIError.InvalidGlacierOperation = Object.freeze({
     message: 'only "migrate", "restore" and "expiry" subcommands are supported',
     http_code: 400,
 });
+ManageCLIError.InvalidSupplementalGroupsList = Object.freeze({
+    code: 'InvalidSupplementalGroupsList',
+    message: 'supplemental groups must be a list of comma seperated numbers',
+    http_code: 400,
+});
 
 
 ////////////////////////

diff --git a/src/manage_nsfs/manage_nsfs_cli_utils.js b/src/manage_nsfs/manage_nsfs_cli_utils.js
@@ -91,7 +91,7 @@ function get_boolean_or_string_value(value) {
 }
 
 /**
- * get_options_from_file will read a JSON file that include key-value of the options 
+ * get_options_from_file will read a JSON file that include key-value of the options
  * (instead of flags) and return its content
  * @param {string} file_path
  */

diff --git a/src/manage_nsfs/manage_nsfs_constants.js b/src/manage_nsfs/manage_nsfs_constants.js
@@ -44,16 +44,16 @@ const FROM_FILE = 'from_file';
 const ANONYMOUS = 'anonymous';
 
 const VALID_OPTIONS_ACCOUNT = {
-    'add': new Set(['name', 'uid', 'gid', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', FROM_FILE, ...CLI_MUTUAL_OPTIONS]),
-    'update': new Set(['name', 'uid', 'gid', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', 'new_name', 'regenerate', ...CLI_MUTUAL_OPTIONS]),
+    'add': new Set(['name', 'uid', 'gid', 'supplemental_groups', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', FROM_FILE, ...CLI_MUTUAL_OPTIONS]),
+    'update': new Set(['name', 'uid', 'gid', 'supplemental_groups', 'new_buckets_path', 'user', 'access_key', 'secret_key', 'fs_backend', 'allow_bucket_creation', 'force_md5_etag', 'iam_operate_on_root_account', 'new_name', 'regenerate', ...CLI_MUTUAL_OPTIONS]),
     'delete': new Set(['name', ...CLI_MUTUAL_OPTIONS]),
     'list': new Set(['wide', 'show_secrets', 'gid', 'uid', 'user', 'name', 'access_key', ...CLI_MUTUAL_OPTIONS]),
     'status': new Set(['name', 'access_key', 'show_secrets', ...CLI_MUTUAL_OPTIONS]),
 };
 
 const VALID_OPTIONS_ANONYMOUS_ACCOUNT = {
-    'add': new Set(['uid', 'gid', 'user', 'anonymous', ...CLI_MUTUAL_OPTIONS]),
-    'update': new Set(['uid', 'gid', 'user', 'anonymous', ...CLI_MUTUAL_OPTIONS]),
+    'add': new Set(['uid', 'gid', 'supplemental_groups', 'user', 'anonymous', ...CLI_MUTUAL_OPTIONS]),
+    'update': new Set(['uid', 'gid', 'supplemental_groups', 'user', 'anonymous', ...CLI_MUTUAL_OPTIONS]),
     'delete': new Set(['anonymous', ...CLI_MUTUAL_OPTIONS]),
     'status': new Set(['anonymous', ...CLI_MUTUAL_OPTIONS]),
 };

diff --git a/src/manage_nsfs/manage_nsfs_help_utils.js b/src/manage_nsfs/manage_nsfs_help_utils.js
@@ -78,6 +78,7 @@ Flags:
 --name <string>                                                         Set the name for the account
 --uid <number>                                                          Set the User Identifier (UID) (UID and GID can be replaced by --user option)
 --gid <number>                                                          Set the Group Identifier (GID) (UID and GID can be replaced by --user option)
+--supplemental_groups <string>                        (optional)        Set the supplemntal group list (List of GID) seperated by comma (,)
 --new_buckets_path <string>                           (optional)        Set the filesystem's root path where each subdirectory is a bucket
 --user <string>                                       (optional)        Set the OS user name (instead of UID and GID)
 --access_key <string>                                 (optional)        Set the access key for the account (default is generated)
@@ -98,6 +99,7 @@ Flags:
 --new_name <string>                                   (optional)        Update the account name
 --uid <number>                                        (optional)        Update the User Identifier (UID)
 --gid <number>                                        (optional)        Update the Group Identifier (GID)
+--supplemental_groups <string>                        (optional)        Update the list of supplemental groups (List of GID) seperated by comma('). will override existing list
 --new_buckets_path <string>                           (optional)        Update the filesystem's root path where each subdirectory is a bucket
 --user <string>                                       (optional)        Update the OS user name (instead of uid and gid)
 --regenerate                                          (optional)        Update automatically generated access key and secret key
@@ -303,13 +305,13 @@ But updates of the config directory will be blocked during the upgrade of the co
 'upgrade start' should be executed on one node, the config directory changes will be available for all the nodes of the cluster.
 
 Usage:
-        
+
     noobaa-cli upgrade start [flags]
 
 Flags:
 
 --expected_version                             <string>                             The expected target version of the upgrade
---expected_hosts                               <string>                             The expected hosts running NooBaa NC, a string of hosts separated by , 
+--expected_hosts                               <string>                             The expected hosts running NooBaa NC, a string of hosts separated by ,
 --skip_verification                            <boolean>        (optional)          skip verification of the hosts package version
                                                                                     WARNING: can cause corrupted config dir files created by hosts running old code
 --custom_upgrade_scripts_dir                   <string>         (optional)          custom upgrade scripts dir, use for running custom config dir upgrade scripts

diff --git a/src/manage_nsfs/manage_nsfs_validations.js b/src/manage_nsfs/manage_nsfs_validations.js
@@ -175,7 +175,11 @@ function validate_options_type_by_value(input_options_with_data) {
             if ((option === 'bucket_policy' || option === 'notifications') && type_of_value === 'object') {
                 continue;
             }
-            const details = `type of flag ${option} should be ${type_of_option}`;
+            //special case for supplemental groups
+            if (option === 'supplemental_groups' && validate_supplemental_groups(value)) {
+                continue;
+            }
+            const details = `type of flag ${option} should be ${type_of_option} ${type_of_value} ${value}`;
             throw_cli_error(ManageCLIError.InvalidArgumentType, details);
         }
     }
@@ -197,6 +201,19 @@ function validate_boolean_string_value(value) {
     return false;
 }
 
+function validate_supplemental_groups(value) {
+    if (value && typeof value === 'string') {
+        const regex = /^[0-9]+(,[0-9]+)+$/;
+        if (!regex.test(value)) {
+            throw_cli_error(ManageCLIError.InvalidSupplementalGroupsList);
+        }
+        return true;
+    } else if (typeof value === 'number') {
+        return true;
+    }
+    return false;
+}
+
 /**
  * validate_min_flags_for_update validates that we have at least one flag of a property to update
  * @param {string} type

diff --git a/src/native/fs/fs_napi.cpp b/src/native/fs/fs_napi.cpp
@@ -537,6 +537,17 @@ load_xattr_get_keys(Napi::Object& options, std::vector<std::string>& _xattr_get_
     }
 }
 
+template<typename T>
+static std::vector<T> convert_delimited_string_to_number_vector(const std::string str, const char delimiter) {
+    std::vector<T> new_vector;
+    std::stringstream sstream(str);
+    std::string item;
+    while (std::getline(sstream, item, delimiter)) {
+        new_vector.push_back(std::stoi(item));
+    }
+    return new_vector;
+}
+
 /**
  * FSWorker is a general async worker for our fs operations
  */
@@ -560,6 +571,7 @@ struct FSWorker : public Napi::AsyncWorker
     double _took_time;
     Napi::FunctionReference _report_fs_stats;
     bool _should_add_thread_capabilities;
+    std::vector<gid_t> _supplemental_groups;
 
     // executes the ctime check in the stat and read file fuctions
     // NOTE: If _do_ctime_check = false, then some functions will fallback to using mtime check
@@ -576,6 +588,7 @@ struct FSWorker : public Napi::AsyncWorker
         , _warn_threshold_ms(0)
         , _took_time(0)
         , _should_add_thread_capabilities(false)
+        , _supplemental_groups()
         , _do_ctime_check(false)
     {
         for (int i = 0; i < (int)info.Length(); ++i) _args_ref.Set(i, info[i]);
@@ -586,6 +599,9 @@ struct FSWorker : public Napi::AsyncWorker
             if (fs_context.Get("backend").ToBoolean()) {
                 _backend = fs_context.Get("backend").ToString();
             }
+            if(fs_context.Has("supplemental_groups")) {
+                _supplemental_groups = convert_delimited_string_to_number_vector<gid_t>(fs_context.Get("supplemental_groups").ToString(), ',');
+            }
             if (fs_context.Get("warn_threshold_ms").ToBoolean()) {
                 _warn_threshold_ms = fs_context.Get("warn_threshold_ms").ToNumber();
             }
@@ -606,7 +622,7 @@ struct FSWorker : public Napi::AsyncWorker
     {
         DBG1("FS::FSWorker::Execute: " << _desc << DVAL(_uid) << DVAL(_gid) << DVAL(_backend));
         ThreadScope tx;
-        tx.set_user(_uid, _gid);
+        tx.set_user(_uid, _gid, &_supplemental_groups);
         DBG1("FS::FSWorker::Execute: " << _desc << DVAL(_uid) << DVAL(_gid) << DVAL(geteuid()) << DVAL(getegid()) << DVAL(getuid()) << DVAL(getgid()));
 
         if(_should_add_thread_capabilities) {

diff --git a/src/native/util/os.h b/src/native/util/os.h
@@ -31,10 +31,11 @@ class ThreadScope
         restore_user();
     }
 
-    void set_user(uid_t uid, gid_t gid)
+    void set_user(uid_t uid, gid_t gid, std::vector<gid_t>* groups)
     {
         _uid = uid;
         _gid = gid;
+        _groups = groups;
         change_user();
     }
 
@@ -49,6 +50,7 @@ class ThreadScope
     void restore_user();
     uid_t _uid;
     gid_t _gid;
+    std::vector<gid_t>* _groups;
 };
 
 } // namespace noobaa
diff --git a/src/native/util/os_linux.cpp b/src/native/util/os_linux.cpp
@@ -43,9 +43,14 @@ const std::vector<gid_t> ThreadScope::orig_groups = [] {
 void
 ThreadScope::change_user()
 {
+    if (_groups == NULL || _groups->empty()) {
+        MUST_SYS(syscall(SYS_setgroups, 0, NULL));
+    }
+    else if (_groups != NULL && *_groups != orig_groups) {
+        MUST_SYS(syscall(SYS_setgroups, _groups->size(), &_groups->at(0)));
+    }
     if (_uid != orig_uid || _gid != orig_gid) {
         // must change gid first otherwise will fail on permission
-        MUST_SYS(syscall(SYS_setgroups, 0, NULL));
         MUST_SYS(syscall(SYS_setresgid, -1, _gid, -1));
         MUST_SYS(syscall(SYS_setresuid, -1, _uid, -1));
     }
@@ -61,7 +66,9 @@ ThreadScope::restore_user()
         // must restore uid first otherwise will fail on permission
         MUST_SYS(syscall(SYS_setresuid, -1, orig_uid, -1));
         MUST_SYS(syscall(SYS_setresgid, -1, orig_gid, -1));
-        MUST_SYS(syscall(SYS_setgroups, ThreadScope::orig_groups.size(), &ThreadScope::orig_groups[0]));
+        if(_groups == NULL || orig_groups != *_groups) {
+            MUST_SYS(syscall(SYS_setgroups, orig_groups.size(), &orig_groups[0]));
+        }
     }
 }
 

diff --git a/src/sdk/nb.d.ts b/src/sdk/nb.d.ts
@@ -1027,6 +1027,7 @@ interface NativeFSContext {
     uid?: number;
     gid?: number;
     backend?: string;
+    supplemental_groups?: string;
     warn_threshold_ms?: number;
     report_fs_stats?: Function;
     do_ctime_check?: boolean;

diff --git a/src/server/system_services/schemas/nsfs_account_schema.js b/src/server/system_services/schemas/nsfs_account_schema.js
@@ -81,6 +81,7 @@ module.exports = {
                 properties: {
                     uid: { type: 'number' },
                     gid: { type: 'number' },
+                    supplemental_groups: { type: 'string' },
                     new_buckets_path: { type: 'string' },
                     fs_backend: {
                         $ref: 'common_api#/definitions/fs_backend'

diff --git a/src/test/unit_tests/jest_tests/test_nc_nsfs_account_cli.test.js b/src/test/unit_tests/jest_tests/test_nc_nsfs_account_cli.test.js
@@ -504,6 +504,69 @@ describe('manage nsfs cli account flow', () => {
             const res = await exec_manage_cli(type, action, account_options);
             expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.InvalidAccountName.code);
         });
+
+        it('cli account add - cli create account with supplemental groups)', async function() {
+            const { type, new_buckets_path, uid, gid, name } = defaults;
+            const supplemental_groups = "303,211";
+            const account_options = { config_root, name, new_buckets_path, uid, gid, supplemental_groups};
+            const action = ACTIONS.ADD;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
+            await exec_manage_cli(type, action, account_options);
+            const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+            expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups);
+        });
+
+        it('cli account add - cli create account with supplemental groups - single group (string))', async function() {
+            const { type, new_buckets_path, uid, gid, name } = defaults;
+            const supplemental_groups = "303";
+            const account_options = { config_root, name, new_buckets_path, uid, gid, supplemental_groups};
+            const action = ACTIONS.ADD;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
+            await exec_manage_cli(type, action, account_options);
+            const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+            expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups);
+        });
+
+        it('cli account add - cli create account with supplemental groups - single group (number))', async function() {
+            const { type, new_buckets_path, uid, gid, name } = defaults;
+            const supplemental_groups = 303;
+            const account_options = { config_root, name, new_buckets_path, uid, gid, supplemental_groups};
+            const action = ACTIONS.ADD;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
+            await exec_manage_cli(type, action, account_options);
+            const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+            expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups.toString());
+        });
+
+        it('cli account add - cli create account with supplemental groups - invalid list', async function() {
+            const { type, new_buckets_path, uid, gid, name } = defaults;
+            const supplemental_groups = "303l,201"; //group cannot contain charecters
+            const account_options = { config_root, name, new_buckets_path, uid, gid, supplemental_groups};
+            const action = ACTIONS.ADD;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
+            const res = await exec_manage_cli(type, action, account_options);
+            expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.InvalidSupplementalGroupsList.code);
+        });
+
+        it('cli account add - cli create account with supplemental groups - invalid list end with comma', async function() {
+            const { type, new_buckets_path, uid, gid, name } = defaults;
+            const supplemental_groups = "303,"; //group cannot end with comma
+            const account_options = { config_root, name, new_buckets_path, uid, gid, supplemental_groups};
+            const action = ACTIONS.ADD;
+            await fs_utils.create_fresh_path(new_buckets_path);
+            await fs_utils.file_must_exist(new_buckets_path);
+            await set_path_permissions_and_owner(new_buckets_path, account_options, 0o700);
+            const res = await exec_manage_cli(type, action, account_options);
+            expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.InvalidSupplementalGroupsList.code);
+        });
     });
 
     describe('cli update account', () => {
@@ -1004,6 +1067,33 @@ describe('manage nsfs cli account flow', () => {
                 const res = await exec_manage_cli(type, ACTIONS.UPDATE, account_options);
                 expect(JSON.parse(res.stdout).error.code).toBe(ManageCLIError.InvalidAccountName.code);
             });
+
+            it('cli account add - cli update account with supplemental groups)', async function() {
+                const { name } = defaults;
+                const supplemental_groups = "303,211";
+                const account_options = { config_root, name, supplemental_groups};
+                await exec_manage_cli(type, ACTIONS.UPDATE, account_options);
+                const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+                expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups);
+            });
+
+            it('cli account add - cli update account with supplemental groups - single group (string))', async function() {
+                const { name } = defaults;
+                const supplemental_groups = "303";
+                const account_options = { config_root, name, supplemental_groups};
+                await exec_manage_cli(type, ACTIONS.UPDATE, account_options);
+                const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+                expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups);
+            });
+
+            it('cli account add - cli update account with supplemental groups - single group (number))', async function() {
+                const { name } = defaults;
+                const supplemental_groups = 303;
+                const account_options = { config_root, name, supplemental_groups};
+                await exec_manage_cli(type, ACTIONS.UPDATE, account_options);
+                const account = await config_fs.get_account_by_name(name, config_fs_account_options);
+                expect(account.nsfs_account_config.supplemental_groups).toBe(supplemental_groups.toString());
+            });
         });
 
         describe('cli update account (has distinguished name)', () => {