Add a "--callback-file" option #162
Conversation
Previously, the callback needed to reside on the Python Path, which is convenient if you're already making a Python package. Plenty of folks aren't, though, so allow users to provide a file that we'll boldly exec and load a callback from. Fixes fedora-infra#159 Signed-off-by: Jeremy Cline <jcline@redhat.com>
Codecov Report
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #162 +/- ##
==========================================
- Coverage 95.59% 95.53% -0.07%
==========================================
Files 14 14
Lines 1453 1478 +25
Branches 196 198 +2
==========================================
+ Hits 1389 1412 +23
- Misses 49 50 +1
- Partials 15 16 +1
Continue to review full report at Codecov.
|
| @@ -0,0 +1,2 @@ | |||
| def rand(message): | |||
| return 4 | |||
There was a problem hiding this comment.
Hahahahaha https://www.xkcd.com/221/ 🤣
There was a problem hiding this comment.
missing a docstring with the source 😆
| # Using "exec" is generally a Bad Idea (TM), so bandit is upset at | ||
| # us. In this case, it seems like a Good Idea (TM), but I might be | ||
| # wrong. Sorry. | ||
| exec(compile(fd.read(), file_path, "exec"), file_namespace) # nosec |
There was a problem hiding this comment.
I wonder if there would be an alternative way of doing this that would be safer, like maybe modifying the module path and loading it as a module, but I think it would pretty much have the same security implications.
There was a problem hiding this comment.
At the very least @puiterwijk should look at it and tell me how bad an idea this is. I can't think if any reasonable (or unreasonable) way to exploit this for ill, but I haven't spent a ton of time trying either.
There was a problem hiding this comment.
So, I will say that this whole idea gives me an unhappy feeling, I do get the use.
I think that any way we add that lets you import arbitrary code as callback will be equally dangerous: still the same level of access required to register e.g. setuptools entrypoints or other packages.
Given that this needs to be called explicitly on the CLI, I guess it's as good as any arbitrary callback mechanism we're going to get.
So +1 for the # nosec here as long as this function only ever gets called with values passed in explicit CLI arguments (as it is right now).
There was a problem hiding this comment.
So explicitly, it must not be called based on contents of received messages.
|
I'm not sure I understand your concern in the "Note" paragraph you wrote above, could you elaborate a bit? |
|
So the first thing I tried was just overloading the "--callback" parameter for the command, and trying to correctly guess if the argument was a filesystem path or a python path. It seemed easy to get right at first, but there are cases where something could be a python path or a filesystem path: "some.module:function" could be either a Python path or a file called "some.module". It's weird to me to have a "--callback" parameter and a "--callback-file" parameter which are both used to specify the callback to use. On the other hand, I like the clarity of specifying if I mean a file or a Python path. The note was just to indicate it's not a design I'm crazy about, but I don't have better ideas. |
|
Ah, OK I get it now, thanks. We could use the same option and try each method and see which one works first, but I wonder it that's wise, also from a security point of view. |
Yeah, that's one thing that made me shy about going this route. I don't know why anyone would have a Python script called "fedora_messaging.callbacks" in their home directory that deletes all their files in a loop at the module level, but I just know someone would do this and accidentally their home directory or something.
Sounds good to me, assuming Patrick doesn't object. |
|
@abompard, since Patrick approves, are you okay with this? |
Previously, the callback needed to reside on the Python Path, which is
convenient if you're already making a Python package. Plenty of folks
aren't, though, so allow users to provide a file that we'll boldly exec
and load a callback from.
Fixes #159
Note: I added this as a new option since I was concerned about messing up distinguishing the filesystem path from the Python path cases. This way the user is explicit, at the expense of things not quite Just Working. Feedback welcome on that decision.