-
Notifications
You must be signed in to change notification settings - Fork 13
Feature/extension scripts #224
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
… to InterpreterGroup
CLA Assistant Lite bot All contributors have signed the CLA ✍️ |
A couple thoughts here:
|
src/code42cli/click_ext/groups.py
Outdated
def parse_args(self, ctx, args): | ||
if len(args) > 1 and args[0] == "--script": | ||
args[0] = sys.executable | ||
if platform.system() == "Windows": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we sanitize the args somehow?
Probably a far-fetched concern, but if the CLI was running on a server, like maybe as part of an integration of some sort, and users were able to call custom scripts remotely, could this could be used as part of a shell-injection attack?
Bandit scan:
Issue: [B605:start_process_with_a_shell] Starting a process with a shell, possible injection detected, security issue.
Severity: High Confidence: High
Location: src/code42cli/click_ext/groups.py:40
More Info: https://bandit.readthedocs.io/en/latest/plugins/b605_start_process_with_a_shell.html
39 cmd = shlex.join(args)
40 status = os.system(cmd)
41 sys.exit(status)
Oddly, bandit
claims it is a HIGH severity but the documentation says it should be LOW/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to worry about that since this function is explicitly for running user-provided code. If someone can hijack the actual execution of the CLI to get it to call --script
on something unintended, they likely already have the ability to just run any arbitrary command they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was imagining a scenario where some integration or app intentionally allows user input to the Code42 CLI, which like I said, is maybe a far-fetched scenario. But, in this scenario, they expose the CLI, so it's not hijacked (yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but it would be outside of our control if someone did that, albeit a dangerous decision on their part, but sanitization on our end could be an extra layer of security
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did start out playing around with just having code42 --python
just return the full path to the interpreter code42
was installed under. So then if a user was having problems with the regular python script.py
they could try $(code42 --python) script.py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll just revert the exec python logic back to just printing the python path. It keeps the code simple and gives users a relatively easy path to figuring things out if their environment is broken.
A command that executes raw input from the user is kind of a red flag to me. That should be avoided whenever possible. What if this PR could be made into a private extension for internal Code42 use? They would use this private CLI package (who's upstream is this public repo), and from there, they can add their own scripts? Just a thought. |
What if - we made another python package, like Edit: Is it important that the be able call it via Edit2: Or it can just be in this package -- just a |
I am sure this might have been discussed but for my benefit I would like to raise it, if someone knows python then they can use |
the standard way to support such thing would be to have |
Also tested out the same,
|
@kiran-chaudhary the background for this is that while we do want internal teams who write scripts that go to customers (Pro Service, Support, SST, etc) to contribute to the CLI development, the CLI isn't going to be able to accommodate every specific use-case. Sometimes a customer has a very particular need that the general goal of the CLI doesn't accomplish, and so support/PS/SST will need to write something custom for them. Exposing this "extension" functionality in the CLI relieves the writer of the burden of having to do any boilerplate logic around getting credentials from the user, initializing the sdk, and handling/logging every possible Py42 error. So remember the use case here is primarily for internal teams to be able to quickly and easily write custom scripts that are intended to be given to and run by customers (who often don't have a lot of programming experience). With these, our teams can write up a script quickly, give it to the customer and just say "run And to @unparalleled-js's question about is it important if they call it |
Apparently |
Can we at least get reach out to the security guild or someone and get their advice? |
@timabrmsn If this is intended for internal people to use, as you say, shouldn't it not be part of the main/publicly available CLI? |
The primary writers of these scripts are going to be internal teams, yes. But the primary runners of these scripts will be customers, so they have to work with regular public-facing CLI installs. |
Ah ok I did not realize that. I will process this. |
@timabrmsn @unparalleled-js @kiran-chaudhary there's a lot of chatter happening here and comments are getting buried. Lets set up a meeting and get everyone on the same page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM from code's point of view. Wondering why we haven't documented this feature, the description given in the PR should be documented for future reference.
src/code42cli/extensions.py
Outdated
@@ -0,0 +1,5 @@ | |||
from code42cli.click_ext.groups import ExtensionGroup | |||
from code42cli.main import CONTEXT_SETTINGS | |||
from code42cli.options import sdk_options # noqa: F401 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wondering why are ignoring unused imports or suppressing warnings, noqa...
statement? I need to read more on the usage of this statement, but the first thing that comes to my mind is will this suppress warnings only in context to this module or will it suppress all-over code42cli
!!?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also can this be resolved by abstraction/design, breaking up the modules in such a way that we don't have to suppress any warnings or unused imports!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm pretty sure the comment only suppresses it for the line being commented, I still got unused import style failures after this commit went in.
I imported sdk_options here just for the convenience of script-writers, so they can just do:
from code42cli.extensions import script, sdk_options
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way to get around this would be to create a directory named extensions
with an __init__.py
importing things for convenience; it seems like that is ok to do because we do it other places to make importing things more convenient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good call. Making it a module would also allow us to better expand/organize things in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, so the style cop still balks at unused imports even in the module init. I wonder why it doesn't do that in py42? I'm not seeing any config differences...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind. I found where we're ignoring it in the setup.cfg
Documentation will be coming, I'm just holding off for now until I can demo the feature to our customer-facing teams to get any feedback before we finalize this implementation. I'm also not sure where the best spot is for the documentation to live. It could be in the README, ReadTheDocs, or the Dev Portal articles... |
LGTM too, but I'll leave this for now in case you get feedback and want to add changes here. |
Documentation would be a plus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I accidentally approved early - everything looks good, just need a changelog and perhaps a readme update
Built user-guide can be found here: https://clidocs.code42.com/en/feature-extension_scripts/userguides/extensions.html |
docs/guides.md
Outdated
@@ -5,3 +5,4 @@ | |||
* [Ingest file events or alerts into a SIEM](userguides/siemexample.md) | |||
* [Manage detection list users](userguides/detectionlists.md) | |||
* [Manage legal hold users](userguides/legalhold.md) | |||
* [Write custom extension scripts using the code42cli and py42](userguides/extensions.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the code42cli
be the Code42 CLI
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, probably.
from code42cli.options import profile_option | ||
|
||
|
||
def sdk_options(f): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe you just want to be really explicit, but this could just be
from code42cli.options import sdk_options as sdk_opts
def sdk_options(f):
"""Decorator that adds two `click.option`s (--profile, --debug) to wrapped command, as well as
passing the `code42cli.options.CLIState` object using the [click.make_pass_decorator](https://click.palletsprojects.com/en/7.x/api/#click.make_pass_decorator),
which automatically instantiates the `py42` sdk using the Code42 profile provided from the `--profile`
option. The `py42` sdk can be accessed from the `state.sdk` attribute.
Example:
@click.command()
@sdk_options
def get_current_user_command(state):
my_user = state.sdk.users.get_current()
print(my_user)
"""
f = sdk_opts()(f)
return f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or even sdk_options = sdk_opts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mainly wanted to have the object we expose be it's own thing in case we ever want to change our internal sdk_options
we're not having to work around the extension backwards compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool!
docs/userguides/extensions.md
Outdated
|
||
## Before you begin | ||
|
||
The Code42 CLI is a python application written using the [click framework](https://click.palletsprojects.com/en/7.x/), and the exposed extension objects are custom `click` classes. A basic knowledge of how to define `click` commands, arguments, and options is required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a fan of line breaks in markdown because of horizontal scrolling in text editors. It seems like most markdown renderers can handle converting the newlines to spaces too. Maybe this is the issue with my IDE -- but I have seen markdown rules around line length too, so I know it's not just me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huh, for some reason I had it in my mind that the newlines could cause odd linebreaks in the rendered sphinx text, but I tested it out and it doesn't seem to matter. Made it much more readable now!
This PR adds some helpers for writing code42cli powered extensions with a minimal amount of boilerplate, giving the extension-writer cli profiles, exception handling, and sdk initialization out of the box, so all they need to do is their custom script logic. Extensions can be standalone scripts that require no installation, or can be installed as a plugin so the commands become part of the main CLI usage (using the
click_plugins
package that helpfully keeps everything else working if an extension breaks).Standalone scripts
To write a cli-powered standalone script, you just need to import the
script
customclick.Group
andsdk_options
, then add your click commands. This example is a script that takes a guid from an option and pretty-prints the resulting settings dict:If there's only a single defined command, the custom group makes it the default, so it can be invoked just from the base script itself:
If multiple commands are defined, then
script
operates as a normal group and enumerates them, requiring the subcommand be called:To help when a user might have multiple python installations in their path and
python script.py
isn't finding thecode42cli
package (but the regularcode42 -h
command works), a--python
option has been added to print out the path to the interpreter that code42 is installed in. So the user can then just call$(code42 --python) script.py
and it should just work without having to fiddle with their paths.Installable extensions
To make the above script installable as a permanent extension to the CLI, you can put it in a package structure with a setup.py where the defined entrypoints are
code42cli.plugins
, the CLI will then be able to find them.I've created an example repo with two installable packages here: https://github.com/timabrmsn/cli_plugin_test
To install the settings example command from above you can run
And to install the users one:
This allows support/PS to have a single repo of their extension scripts, and they can either link directly to the standalone one, or instruct the user to install it, depending on the need/use-case.