{doc} Add subprocess security guidance doc #29681
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
️✔️AzureCLI-FullTest
|
️✔️AzureCLI-BreakingChangeTest
|
Collaborator
|
guidance doc for subprocess |
AllyW
force-pushed
the
add-subprocess-guide-doc
branch
from
4d354a3 to
f09d2ff
Compare
jsntcy
reviewed
Aug 29, 2024
jsntcy
reviewed
Aug 29, 2024
jiasli
reviewed
Aug 30, 2024
jiasli
reviewed
Aug 30, 2024
jiasli
reviewed
Aug 30, 2024
jiasli
reviewed
Aug 30, 2024
jiasli
reviewed
Aug 30, 2024
jiasli
reviewed
Aug 30, 2024
doc/cli_subprocess_guidelines.md
Outdated
| Besides that, users might need to know some parts of the accessibility in both `run_cmd` and `subprocess` | ||
| 1) when calling shell built-in cmds, like `dir` or `echo`, using `shell=True` **in windows platform**, `subprocess` implicitly uses `cmd.exe`, while `run_cmd` asks developers to provide the `cmd.exe` as executable file specifically in the arg list's first item, like `["cmd.exe", "/c", "echo", "abc"]` | ||
| 2) if developers want to find an easy way to split their current cmd string into list, **for unix-like platforms**, developers can apply [`shlex.split`](https://docs.python.org/3/library/shlex.html#shlex.split) for quick access. But a prepared cmd statement is still more recommended (for more info about prepared cmd statement, please read below sections). | ||
| 3) it might be not that obvious to find target command's executable file **in windows platform**, a tool developer can use is `shutil.which` that gives the executable file path in windows system, like `shutil.which(git)`. The cmd `git --version` can be adjusted as `[shutil.which(git), "--version"]`. Please provide the corresponding executable path in target platforms. |
Member
There was a problem hiding this comment.
Could you share more information on this? I think subprocess will look for the executable on PATH.
Contributor
Author
There was a problem hiding this comment.
Yes, subprocess searches executable under PATH, as the same way as shutil.which.
In windows platform, cmd-running can be cmd arg0 arg2.. or cmd.exe arg0 arg1.... Just want to give a hint to users how to check the absolute path of corresponding executable file in case they are confused with it.
Doc adjusted.
jiasli
reviewed
Aug 30, 2024
doc/cli_subprocess_guidelines.md
Outdated
| @@ -0,0 +1,110 @@ | |||
| # Azure CLI Subprocess Guidelines | |||
|
|
|||
| In certain cli modules, there are scenarios that need to call a subsystem to run commands outside cli, like getting kubectl info in aks, or deployment setup in mysql using `git` and `gh`, through python built-in [subprocess](https://docs.python.org/3/library/subprocess.html) module. Despite its simplicity and versatility, ensuring the security of applying it can be challenging in different platforms under different circumstance, and it is error-prone for developers to neglect security best practices during development. | |||
Member
There was a problem hiding this comment.
Will it be clearer to use "subprocess" instead of "subsystem"? I don't think "subsystem" stands for the scenario this document is talking about.
https://docs.python.org/3/library/subprocess.html doesn't mention the term "subsystem" at all.
Contributor
Author
There was a problem hiding this comment.
subsystem is taken as a synonym or a phase in the meaning nearby to subprocess here.
AllyW
force-pushed
the
add-subprocess-guide-doc
branch
from
3e8e2d0 to
3992e56
Compare
jiasli
reviewed
Sep 4, 2024
jiasli
reviewed
Sep 4, 2024
doc/cli_subprocess_guidelines.md
Outdated
Comment on lines
20
to
24
| ```python | ||
| input a git command to run: --version;echo aa | ||
| git version 2.34.1 | ||
| aa | ||
| ``` |
Contributor
Author
There was a problem hiding this comment.
right, it is a mock for the console input and output of running python code above.
Changed to console here
Co-authored-by: Jiashuo Li <[email protected]>
Co-authored-by: Jiashuo Li <[email protected]>
Co-authored-by: Jiashuo Li <[email protected]>
Co-authored-by: Jiashuo Li <[email protected]>
Co-authored-by: Jiashuo Li <[email protected]>
AllyW
force-pushed
the
add-subprocess-guide-doc
branch
from
e493c5d to
08f8342
Compare
dcaro
reviewed
Sep 5, 2024
doc/cli_subprocess_guidelines.md
Outdated
|
|
||
|
|
||
| ## Summary | ||
| Ensuring the safety of Azure CLI from command injection under subprocess calling requires an in-depth understanding of these vulnerabilities and also proactive measures to counteract potential exploits. CLI developers can apply the three security practices, if applicable, when using builtin `subprocess`, but it's recommended to use the centralized function `run_cmd` CLI provided, to safeguard CLI modules from command injection attack and for future more accessible security enforcements. |
Contributor
There was a problem hiding this comment.
Should we have a more prescriptive approach and require CLI developers to use the run_cmd for any subprocess operation to meet our security standards?
Contributor
Author
There was a problem hiding this comment.
Agreed. We can add a checking rule on those unsupervised subprocess call and give cli core's run_cmd as the substitution, with this doc provided as assistance.
jiasli
approved these changes
Sep 9, 2024
juliusl
pushed a commit
to juliusl/azure-cli
that referenced
this pull request
Sep 23, 2024
* Update doc/cli_subprocess_guidelines.md Co-authored-by: Jiashuo Li <[email protected]>
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Related command
Description
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.