[t] Add Group with custom GID fails with type error

[Hooverdan96]

When trying to create a new User Group using a custom GID (e.g., 1001), the system will throw a syntax error. Here is the traceback:
[Edit: see also commit: https://github.com//issues/2807#issuecomment-1995226097 for another reproducer]

            Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/system/osi.py", line 236, in run_command
    p = subprocess.Popen(
        ^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.11/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib64/python3.11/subprocess.py", line 1883, in _execute_child
    self.pid = _fork_exec(
               ^^^^^^^^^^^
TypeError: expected str, bytes or os.PathLike object, not int

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/rest_framework_custom/generic_view.py", line 41, in _handle_exception
    yield
  File "/opt/rockstor/src/rockstor/storageadmin/views/group.py", line 74, in post
    groupadd(groupname, gid)
  File "/opt/rockstor/src/rockstor/system/users.py", line 254, in groupadd
    return run_command(cmd)
           ^^^^^^^^^^^^^^^^
  File "/opt/rockstor/src/rockstor/system/osi.py", line 253, in run_command
    raise Exception("Exception while running command({}): {}".format(cmd, e))
Exception: Exception while running command(['/usr/sbin/groupadd', '-g', 1001, 'plex']): expected str, bytes or os.PathLike object, not int

when leaving the field blank, i.e. allow AutoGenerate, this is not an issue. I assume, in that case the -g parameter is not passed and the OS assigns a new number.

Note, when creating a new user with a custom GID, this error does not occur.

[FroggyFlox]

Nice find!
That looks like a Python3 requirement that was missed in the transition so far.so I've added this issue to our next Stable milestone.

[Hooverdan96]

I suspect we might have some more candidates that take advantage of the run_command(cmd) or similar, where usually integers are passed instead of strings? ... Non-exhaustive list mind you:

like this if there is a numeric connection name:

rockstor-core/src/rockstor/system/network.py

Lines 344 to 349 in 4d60a2c

	run_command(
	[NMCLI, "c", "add", "type", "ethernet", "con-name", name, "ifname", ifname]
	)
	new_connection_helper(name, ipaddr, gateway, dns_servers, search_domains, mtu=mtu)
	# @todo: probably better to get the uuid and reload with it instead of
	# name.

possibly this:

rockstor-core/src/rockstor/scripts/qgroup_maxout_limit.py

Lines 53 to 57 in 4d60a2c

	for q in qgroup_ids:
	print("relaxing the limit on qgroup %s" % q)
	run_command([BTRFS, "qgroup", "limit", "none", q, mnt_pt])
	print("Finished processing pool(%s)" % p.name)

rockstor-core/src/rockstor/system/acl.py

Lines 31 to 34 in 4d60a2c

	if group is not None:
	owner = "%s:%s" % (owner, group)
	cmd.extend([owner, share])
	return run_command(cmd)

only relevant, if a numeric domain is used (which I believe I have seen in the past, whereas the rule exists that a computer belonging to an AD domain cannot have only numerals in its name - apparently a DNS restriction):

rockstor-core/src/rockstor/smart_manager/views/active_directory.py

Lines 105 to 110 in 4d60a2c

	cmd = [REALM, "discover", "--name-only", domain]
	o, e, rc = run_command(cmd)
	except Exception as e:
	e_msg = (
	"Failed to discover the given({}) AD domain. "
	"Error: {}".format(domain, e.__str__())

and finally, we have this issue open, which likely has the same root cause:

#2805

[Hooverdan96]

Looking further into the run_command function, it seems that the mapping of all command parameters to strings has been commented out in Line 233:

rockstor-core/src/rockstor/system/osi.py

Lines 228 to 245 in 6c86e38

	# We force run_command to always use en_US
	# to avoid issues on date and number formats
	# on not Anglo-Saxon systems (ex. it, es, fr, de, etc)
	fake_env = dict(os.environ)
	fake_env["LANG"] = "en_US.UTF-8"
	# cmd = map(str, cmd)
	if log:
	logger.debug(f"Running command: {' '.join(cmd)}")
	p = subprocess.Popen(
	cmd,
	shell=shell,
	stdin=stdin,
	stdout=stdout,
	stderr=stderr,
	encoding="utf-8",
	env=fake_env,
	universal_newlines=True, # 3.7 adds text parameter universal_newlines alias
	)

based on this PR:

#2564

and this commit:

316654c

For this specific error message, when checking the comparable useradd I noticed that the ID is "stringified"

rockstor-core/src/rockstor/system/users.py

Lines 239 to 246 in 6c86e38

	cmd = [USERADD, "-s", shell, "-m", username]
	if uid is not None:
	cmd.insert(-1, "-u")
	cmd.insert(-1, str(uid))
	if gid is not None:
	cmd.insert(-1, "-g")
	cmd.insert(-1, str(gid))
	return run_command(cmd)

whereas in the groupadd it is not:

rockstor-core/src/rockstor/system/users.py

Lines 250 to 254 in 6c86e38

	cmd = [GROUPADD, groupname]
	if gid is not None:
	cmd.insert(-1, "-g")
	cmd.insert(-1, gid)
	return run_command(cmd)

which would explain the above that the error occurs for a group addition, but not a user addition.

It could be fixed here (probably should for consistency purposes in the group/user management), but that would leave examples like the above, where, if a numeric AD name was passed, the run_command would still fail.

So, not sure, whether it's more secure to force the string conversion in the run_command function to ensure that only string values are passed into it, or enforce the "stringification" of any cmd parameter by the caller?

Finally, for my own education, when looking at the security consideration here:
https://docs.python.org/3.11/library/subprocess.html#security-considerations
I am getting the impression that this should not throw an error, the shell=true is not invoked (and specifically set to False in the Rockstor code), but I might just misinterpret that statement there...

[phillxnet]

@Hooverdan96 Another simple reproducer for the original issue here is of course creating a custom group:

            Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/system/osi.py", line 236, in run_command
    p = subprocess.Popen(
        ^^^^^^^^^^^^^^^^^
  File "/usr/lib64/python3.11/subprocess.py", line 1026, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/usr/lib64/python3.11/subprocess.py", line 1883, in _execute_child
    self.pid = _fork_exec(
               ^^^^^^^^^^^
TypeError: expected str, bytes or os.PathLike object, not int

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/rest_framework_custom/generic_view.py", line 41, in _handle_exception
    yield
  File "/opt/rockstor/src/rockstor/storageadmin/views/group.py", line 74, in post
    groupadd(groupname, gid)
  File "/opt/rockstor/src/rockstor/system/users.py", line 254, in groupadd
    return run_command(cmd)
           ^^^^^^^^^^^^^^^^
  File "/opt/rockstor/src/rockstor/system/osi.py", line 253, in run_command
    raise Exception("Exception while running command({}): {}".format(cmd, e))
Exception: Exception while running command(['/usr/sbin/groupadd', '-g', 1000, 'auth-config']): expected str, bytes or os.PathLike object, not int

I think we should be more TYPE orientated here, and rather than force map all elements passed to run_command we should pass correct types in the first place. I.e. fix in place; rather than work around incorrect types. Maybe a type hint of sorts in run_command and fix all other code to use only the new str type as command list elements.

[Hooverdan96]

sounds good to me. Pardon my ignorance, when you talk about hint, do you mean adding some comment in the code, indicating that the caller should have string only elements or are you referring to some other "hinting"?

[phillxnet]

@Hooverdan96 Re:

when you talk about hint

My apologies, I was referencing what @FroggyFlox began within Rockstor's code, once we were a little more 'modern' on the Python version front: "type hinting", introduced in Py3.5:

https://realpython.com/lessons/type-hinting/

[Hooverdan96]

ah, ok, so you mean like this:

rockstor-core/src/rockstor/system/osi.py

Lines 209 to 218 in 4d60a2c

	def run_command(
	cmd,
	shell=False,
	stdout=subprocess.PIPE,
	stderr=subprocess.PIPE,
	stdin=subprocess.PIPE,
	throw=True,
	log=False,
	input=None,
	):

turning into something like this:

def run_command(
    cmd: str,
    shell: bool = False,
    stdout: PIPE = subprocess.PIPE,
    stderr: PIPE = subprocess.PIPE,
    stdin: PIPE = subprocess.PIPE,
    throw: bool = True,
    log: bool = False,
    input=None,
):

I know, probably a little overboard 😄 in the type hinting - I guess, the important one would be for the cmd parameter.

[phillxnet]

@Hooverdan96 yes, that was the type of thing I was thinking. But our cmd is a LIST type with all str() members expected: not sure how that is typed yet. Plus with input there is a 'type or None' definition in type hints. Maybe that would be the way to go on that front.

I know, probably a little overboard

Not at all. Plus there is no typing on the return type! I like this extra stringency. Early Python was just too loose. Hence this more recent introduction. And besides helping with errors at run-time: i.e. wrong type being more informative, the IDE's are also informed by this and can help us by hinting that we are doing it wrong: via their being informed from the hints themselves. All good stuff. I should have taken this approach from the get-go when doing the initial Python 2 to 3 migration. But alas there was much to do and I had thought type hints were not yet available for some reason. Then saw @FroggyFlox popping them in and was much relieved to finally have them available.

All our newer code, and significant code changes, should be, at least trying to use these to their fullest extend now. Take a look at my recent replication fixes, they were super helpful there as we had some major str() / byte type issues going on there from our Py2.7 days.

[phillxnet]

@Hooverdan96

like this if there is a numeric connection name:

rockstor-core/src/rockstor/system/network.py

Lines 344 to 349 in 4d60a2c

	run_command(
	[NMCLI, "c", "add", "type", "ethernet", "con-name", name, "ifname", ifname]
	)
	new_connection_helper(name, ipaddr, gateway, dns_servers, search_domains, mtu=mtu)
	# @todo: probably better to get the uuid and reload with it instead of
	# name.

Ultimately "name" is from: requests here:

rockstor-core/src/rockstor/storageadmin/views/network.py

Lines 285 to 286 in d270ba2

	name = request.data.get("name")
	if NetworkConnection.objects.filter(name=name).exists():

So I think we are OK there.

[phillxnet]

for q in qgroup_ids:

Likewise qgourp_ids is constructed from strings: but I'll pop in a type-hint anyway. Cheers.

[phillxnet]

only relevant, if a numeric domain is used (which I believe I have seen in the past,

I wouldn't have though that would float ! I'll look to add some casting maybe.

[phillxnet]

Another potential inadvertent int left over from our Py2.7 to Py3.11 modifications.

Create a docker net 111: fine.
Delete the docker-net named "111"

[22/Mar/2024 15:33:25] ERROR [storageadmin.util:45] Exception: '>' not supported between instances of 'BridgeConnection' and 'int'
Traceback (most recent call last):
  File "/opt/rockstor/src/rockstor/rest_framework_custom/generic_view.py", line 41, in _handle_exception
    yield
  File "/opt/rockstor/src/rockstor/storageadmin/views/network.py", line 548, in delete
    if nco.bridgeconnection_set.first() > 0:  # If docker network
       ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
TypeError: '>' not supported between instances of 'BridgeConnection' and 'int'

After further investigation this looks like something in it's own right. I have, as a result, spun off the following issue:
TypeError when deleting a Rocknet #2814

[FroggyFlox]

@phillxnet, interesting timing... I was just refreshing myself on that part of the code due to #2775.
The more I look at that line, the more I think we're doing that check the wrong way, at least for modern Django/Python. We'll thus have to redo that check when fixing #2775.
In that case, maybe we can keep this change for the PR addressing #2775?

[phillxnet]

@FroggyFlox Thanks for taking a look at this. Agreed something is a drift here, and it looked a little different from my current draft pr against this issue hence the spin-off issue I've just created. Also I'm now on the home run for my current draft PR so will likely present that shortly. You may want to rebase and work on it once it's merged as there are a few hopefully useful type hint additions etc. But again, it looks like it's something a little different to what I'm currently working on.

[phillxnet]

Closing as:
Fixed by #2815