{Core} Add cli subprocess #29503
{Core} Add cli subprocess #29503
Conversation
AllyW
requested review from
jiasli,
evelyn-ys,
jsntcy,
kairu-ms,
zhoxing-ms,
calvinhzy,
necusjz and
bebound
as code owners
️✔️AzureCLI-FullTest
|
|
Hi @AllyW, |
️✔️AzureCLI-BreakingChangeTest
|
|
Core |
microsoft-github-policy-service
bot
added
Auto-Assign
AllyW
force-pushed
the
cli-process
branch
2 times, most recently
from
9118941
to
36dacda
Compare
| if not isinstance(args, list): | ||
| return | ||
| for i, val in enumerate(args): | ||
| args[i] = re.sub(ARG_MASK_CHAR, "", val) |
There was a problem hiding this comment.
Replacing ARG_MASK_CHAR is fragile. Consider using shlex.quote: https://docs.python.org/3/library/shlex.html#shlex.quote
There was a problem hiding this comment.
As the shlex doc said, "The shlex module is only designed for Unix shells. ", so I planned to put it in cli subprocess doc later for guiding users to split and quote their cmd and args themselves according to the platform.
shell=False and arg list is safe enough for avoid cmd injection.
| process = cli_subprocess.CliPopen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True, | ||
| enable_arg_mask=True) |
There was a problem hiding this comment.
What if the caller doesn't pass enable_arg_mask? This still needs manual check.
There was a problem hiding this comment.
Cause arg mask itself is optional. By default it's false and in most, almost all, scenarios it's not that necessary.
| @cli_subprocess_decorator | ||
| def run(*popenargs, **kwargs): | ||
| """Run command with arguments and remove shell=True | ||
|
|
||
| The other arguments are the same as for the subprocess.run. | ||
| """ | ||
| return subprocess.run(*popenargs, **kwargs) |
There was a problem hiding this comment.
When subprocessing azcopy and bicep, high-level APIs are used.
-
azcopy(though it is using an older high-level API): -
bicep:azure-cli/src/azure-cli/azure/cli/command_modules/resource/_bicep.py
Lines 327 to 331 in cff8d9b
azdev only uses subprocess.call: https://github.com/Azure/azure-cli-dev-tools/blob/db1212ac069ce2f4149315b5860c76fc1499b736/azdev/utilities/command.py#L34
call, check_call and check_output are all older high-level APIs. We should avoid using them. Exposing only one function for subprocess.run may be sufficient.
https://docs.python.org/3/library/subprocess.html#using-the-subprocess-module
The recommended approach to invoking subprocesses is to use the
run()function for all use cases it can handle.
There was a problem hiding this comment.
I think it's ok to expose these commonly used three older high-level APIs cause subprocess itself does this. And it's easier for developers to adjust their current code snippet.
| def subprocess_kwarg_mask(kwargs): | ||
| if kwargs.get("shell", False): | ||
| logger.warning("Removed shell=True for cli processor") | ||
| kwargs["shell"] = False |
There was a problem hiding this comment.
If we believe shell=True is unnecessary, why not raise an error to fail the tests and let the code owner to fix it?
There was a problem hiding this comment.
We can but that requests a developers' second-check, which actually can be easily eliminated. I prefer this to be a default behaviour for clisubprocess so that it is a one-time effort for developers to migrate.
There was a problem hiding this comment.
I agree with @bebound that developers should be guided to follow the best practice, instead of silently changing the parameters.
|
|
||
|
|
||
| def subprocess_arg_mask(args, kwargs): | ||
| enable_arg_mask = kwargs.get("enable_arg_mask", None) |
There was a problem hiding this comment.
enable_arg_mask and skip_arg_type_check are hidden in kwargs. In that case, they should be included in the doc string. We can't expect developers to look into the source code to know them.
| subprocess_kwarg_mask(kwargs) | ||
|
|
||
|
|
||
| def run(*popenargs, **kwargs): |
There was a problem hiding this comment.
subprocess.run only exposes one positional argument args, so using *popenargs is unnecessary.
| if skip_arg_type_check is not None: | ||
| del kwargs["skip_arg_type_check"] |
There was a problem hiding this comment.
It should be
if "skip_arg_type_check" in kwargs:
del kwargs["skip_arg_type_check"]Otherwise, if skip_arg_type_check is None, it will not be deleted and passed to subprocess.
Related command
Description
There are some situations that cli developers need to call a subsystem command like the proxy for ssh commands or kubectl for aks. Considering current subprocess use cases and potential vulnerability, cli decided to provide a central function that developers can use under cli dev's guide in a more secure manner.
After researching on current cli code base and the official doc from subprocess, especially those related to security items, we come up with this cli_subprocess which inherited all official subprocess Popen funcs, including those older ones cause it's commonly used in cli and in real world programs, and at the same time, with improper key values masked, and preferred arg type check and etc.
Testing Guide
History Notes
[Component Name 1] BREAKING CHANGE:
az command a: Make some customer-facing breaking change[Component Name 2]
az command b: Add some customer-facing featureThis checklist is used to make sure that common guidelines for a pull request are followed.
The PR title and description has followed the guideline in Submitting Pull Requests.
I adhere to the Command Guidelines.
I adhere to the Error Handling Guidelines.