mcu: Restart mcu by running executable on host

[majastanislawska]

Signed-off-by: Maja Stanislawska [email protected]

[KevinOConnor]

Thanks, but I'm not sure this is a good candidate for the master Klipper branch. I'm leery of running arbitrary commands from the Klipper config file for two reasons - it could introduce security issues, and arbitrary shell commands can cause strange interactions with the code (eg, reusing file descriptors, causing spurious unix signals, etc.).

FYI, I raised a similar concerns in PR #2173.

Could you describe the particular use case you had in mind for this support? Perhaps we could find an alternate solution.

Cheers,
-Kevin

[majastanislawska]

Purpose of this is not to run arbitrary command, only one very specific to task of resetting mcu.
It is being executed only when klipper is doing literally nothing, after mcu has been closed, and before it is reopened.
i personally use it to reload (rmmod/insmod) inkernel mcu i'm writing (for bbb).
for those running linux_mcu it could be 'sysctl restart klipper_mcu'
but as written in doc one could use it to pull down gpio connected to reset pin on mcu for setups that use plain uart, thats usually what usb adapters do as DTR. Or pull relay or whatever.
One could logrotate logfiles there too

Only other alternative solution is clutering code,config and documentation with more of "specific" solutions like rpi_reset is already

Could you please elaborate on your "security issues" concerns.
Because from what i see this is just spooky monster cliche.
If someone have capacity to hack into place they could place malicious script, and then modify klipper config to use it,
it's already too late and i do not think such hacker would even bother with doing that at this point.
script is executed with same user privileges as klipper, unless sudoers is involved and this this host admin responsibility not klipper developers. (so is running klipper as root), right to execute is separate to right to modify script on unix.
there is no risk of "interference with klipper" becasue kliper is practically not running during this time
only signal involved is SIGCHLD which is handled by subprocess.

[kpochwala]

I've got a printer that is connected do RPi via UART (also reset is tied to raspberry's GPIO), and to reset it I have to go to the printer and press physical button on the main board or run script on RPi that toggles that for me. Klipper could easily do it with this PR, so I think its useful.

[KevinOConnor]

i personally use it to reload (rmmod/insmod) inkernel mcu i'm writing (for bbb).

Okay, thanks. My high-level feedback is that we should evaluate the best way to restart that code when (and if) it is submitted for inclusion to the master Klipper branch.

Could you please elaborate on your "security issues" concerns.

It's common for users to cut-and-paste config file snippets from the internet into their config file. I'm leery of adding a direct method to invoke arbitrary unix commands from the config file.

only signal involved is SIGCHLD which is handled by subprocess

FYI, due to the way Unix handles "close on exec" and "terminal groups", it can cause confusing file descriptor leaks and signals to both calling and called processes - in particular for long running processes.

Because from what i see this is just spooky monster cliche.

I understand that you do not agree, and I understand that you would prefer a different outcome. However, my current feedback is that I do not think this is a good candidate for the master Klipper branch.

-Kevin

[majastanislawska]

i personally use it to reload (rmmod/insmod) inkernel mcu i'm writing (for bbb).

Okay, thanks. My high-level feedback is that we should evaluate the best way to restart that code when (and if) it is submitted for inclusion to the master Klipper branch.

It probably never will, as it hardly shares anything with klipper codebase besides protocol.
and i can imagine how many "concerns" about "security issues" you would have about it.

It's common for users to cut-and-paste config file snippets from the internet into their config file. I'm leery of adding a direct method to invoke arbitrary unix commands from the config file.

It's also common that people stuff strange things to their anuses,
and yet we do not have them surgically sewn tight after birth.

only signal involved is SIGCHLD which is handled by subprocess
FYI, due to the way Unix handles "close on exec" and "terminal groups", it can cause confusing file descriptor leaks and signals to both calling and called processes - in particular for long running processes.

Yeah, bring more spooky monsters, are we bordering to mansplaining already?
FYI, this is not long running process, and there is close_fds=True option to subprocess, you could add this if you're scared.

Because from what i see this is just spooky monster cliche.
I understand that you do not agree, and I understand that you would prefer a different outcome. However, my current feedback is that I do not think this is a good candidate for the master Klipper branch.

no you dont.
You need to decide if you are developer or control freak.

-Kevin

Maja

[majastanislawska]

pressed wrong button.

[kpochwala]

i personally use it to reload (rmmod/insmod) inkernel mcu i'm writing (for bbb).

Okay, thanks. My high-level feedback is that we should evaluate the best way to restart that code when (and if) it is submitted for inclusion to the master Klipper branch.

Then please reevaluate restarting UART only boards right now. E.g. AVR board connected directly to RPi UART - which doesn't have flow control pins, so it cannot reset firmware properly.

Could you please elaborate on your "security issues" concerns.

It's common for users to cut-and-paste config file snippets from the internet into their config file. I'm leery of adding a direct method to invoke arbitrary unix commands from the config file.\

People also cut and paste sudo rm -rf / --no-preserve-root from Internet, and yet it is not blocked in rm program (but it has a switch for that in case you've typed only rm -rf / by mistake).

[majastanislawska]

You can reopen and merge this when you grow up, Kevin.

[lrstanley]

Thinking about this a bit and brainstorming some solutions that could work for everyone... if there is a strong concern about safety with users copying/pasting commands or configs with commands, there are a few ways of ensuring this is a non-issue.

1. Define a step in which users who want to use this functionality must do, to enable such commands (e.g. write a specific file, or toggle an option that's not in their printer.cfg). If a script call exists in the config, and the user hasn't enabled the option to run commands, skip over such commands (probably with a warning in the console output to make it obvious the requested calls cannot happen).
   • This has the benefit of adding some prompt/acknowledgement identifying the dangers of such functionality.
2. Restrict to just a single command, with an optional list of arguments.
   • If one tries to support full shell lines, rather than a direct command invocation, it opens the gates to more potential concerns and makes it harder to prevent safety issues.
3. Restrict arguments in some way.
   • E.g. if you want to pass parameters to the command, maybe it has to be done via environment variables, and you can't pass in regular arguments.
   • Or could enforce the use of stdin. e.g. a JSON object of parameters via stdin. This could prevent calls to standard commands, and require the user to parse the input. This would cut out almost ALL safety concerns, as it essentially requires a custom script to be written, and you couldn't just run rm -rf /, etc.
4. Another random oddball idea: support webhooks.
   • Webhooks would allow others to host the logic anywhere (locally or remote), and do the logic on the end of the webhook.
   • VERY small concern with process fd (or other sharing).
   • Could be setup to error if the http request fails (or support ignoring if it fails), optionally ignore tls cert errors, etc.
   • Could pass in parameters via GET parameters, or POST body.
   • Klipper should NOT do anything based off the response body, ONLY adjust continuing vs erroring, based on the http code.

None of the above is too complex or difficult in my opinion, and still allow a very reasonable amount of security/safety prevention. It would be very easy for someone to write their own script that adheres to these standards to do their custom logic. I personally would like to setup scripts for custom notification webhooks among other things.

On the notes of file descriptors, and similar, I feel this is a non-issue. Klipper should safely handle this anyway (and if it doesn't, that's a concern exclusive of this feature). If anything happens to the resources that are used by klipper in a way that could break klipper, shouldn't klipper emergency shutdown anyway? e.g an error so deep, that klipper crashes?

Lastly what I'd like to make sure we don't forget, is that if Klipper doesn't support this, which I totally understand if that's the case, users will find a way around this that is likely more dangerous than what I or others have mentioned above as concerns. E.g. writing their own plugins and throwing them in the extras/ dir, that klipper cannot control, or other more hacky solutions. They will find a way, and usually it's not cookie-cutter/pretty.

Serious thoughts on some of the suggestions? If the webhook thing sounds promising, could open up another PR for that.

[majastanislawska]

1. Define a step in which users who want to use this functionality must do, to enable such commands [..]

adding this option to config file and providing value of path to a script, which then literally need to writen by themselves is such step. What more you want? doing that at midnight, during full moon, in a made of salt pentagram with candle at corners, barefoot and while chanting some spells??
only thing you will achieve is that there will be sub on reddit with tantrums about how #!@$%&^ is that

   * This has the benefit of adding some prompt/acknowledgement identifying the dangers of such functionality.

there is no danger in such functionality only control freak paranoia about it
whatever you will invent it will be done once, during initial setup, strictly according to documentation, without much focus and will to 'acknowledge ' alleged "dangers', only for sake to have things up and running finally.
this is completely unnecessary, not worth minute of time spent on coding it, not worth increased complexity, and back pressue on support.

2. Restrict to just a single command, with an optional list of arguments.
3. Restrict arguments in some way.

it is a single command - there is single call to supbrocess,
actually adding capacity to allow more would be unnecessary overkill, if you need more commands just put them int oscript and call that script from here. Doing this this way is way simpler, elegant, less error prone, already done and requires no maintenance,.
there is no need really for arguments because script that is called supposed to serve single purpose (of restarting mcu)
if said restarting mcu need to call other commands with parameters, that is again content and scope of said script and responsibility of one who writes it, not a concern of klipper.
imposing such limits in klipper config serves no purpose, ads unnecessary complexity, and need for maintaining code.

whoever have write access to klipper config is already in position to inflict more damage more direclty than using klipper to do it. and restricting them in klipper can't save neither klipper nor system that is compromised already.

   * E.g. if you want to pass parameters to the command, maybe it has to be done via environment variables, and you can't pass in regular arguments.

You dont need to pass parameters from klipper and it definitely doesn't need to be done via ENV, "passing agruments" via ENV is not "safer" its more dangerous, because it ads unnecessary complexity and obscurity, and because of life cycleand scope of ENV makes whole thing more prone to human mistakes. this is not only absurd overkill, this actually could be dangerous if whole this ting wasnt just about just restarting mcu

   * Or could enforce the use of stdin. e.g. a JSON object of parameters via stdin. 

Yet another unnecessary overkill.
Particularly JSON part is just crime against humanity.

This could prevent calls to standard commands,

No, it doesnt. (try yourself)

and require the user to parse the input.

Not user, script writer.
This will actually force less experienced users to seek solutions on internet,
because while they could manage to figure out how to do gpioset or 'echo 1 >/sys/clas/gpio' on their own,
parsing json from stdin in shell is way beyond their league
and instead of having oneliner or 2 liner shellscript there, you will "force" peole to write full blown "applications"

This would cut out almost ALL safety concerns, as it essentially requires a custom script to be written,

No, all this would make whole that paranoia about "security concerns" little more real actually
(just little bit, dont get excited that whole this is anywere close being real issue, especially an actual security issue)

and you couldn't just run rm -rf /, etc.

putting aside that rm -rf / is just cliche, if someone really really wants to erase their disk, nothing on earth is able to stop them anyway,
and that putting aside, that someone with shell access already wanting to erase their disk, would do that from their shell prompt and wouldn't bother with putting it to script executed once per blue moon by klipper

And finally, whole this stuff runs on just Raspberry for god sake,
even if somebody is "so unexperienced" to put rm -rf to their script because they read it "somewhere on internet",
they will learn about "that mistake" several minutes later on "first try",
and than, they can just go back to their PC and reflash damn SDcard.

and finally: "Artificial intelligence is not a cure for natural born stupidity".
There is pleny way more dangerous stuff out there in the world, stuff that can actually kill you.

4. Another random oddball idea: support webhooks.

oddball indeed

On the notes of file descriptors, and similar, I feel this is a non-issue. Klipper should safely handle this anyway (and if it doesn't, that's a concern exclusive of this feature). If anything happens to the resources that are used by klipper in a way that could break klipper, shouldn't klipper emergency shutdown anyway? e.g an error so deep, that klipper crashes?

this is indeed not an issue, this was randomly thrown and misfired (another) spooky monster,

Lastly what I'd like to make sure we don't forget, is that if Klipper doesn't support this, which I totally understand if that's the case, users will find a way around this that is likely more dangerous than what I or others have mentioned above as concerns. E.g. writing their own plugins and throwing them in the extras/ dir, that klipper cannot control, or other more hacky solutions. They will find a way, and usually it's not cookie-cutter/pretty.

exactly, (besides that "i totally udnserstand")
+using mainsail or octoprint (maybe even cron ;-) ) to do this, tearing klipper abruptly out from main loop by exception on serialport, and this is a case that could leave astray stuff that could backfire later, if klipper was poorly written,(which isn't)

Serious thoughts on some of the suggestions?

it's seriously tough to remain serious in that amount of absurdity and paranoia.
None of this was ever security issue, this is issue of sexism and fragile masculity.

If the webhook thing sounds promising, could open up another PR for that.

good luck with that. ;-)

[lrstanley]

Adding guardrails to prevent people from shooting themselves in the foot is standard practice with almost all widely adopted software. E.g. --no-preserve-root to rm, windows confirmation prompts before destructive actions, Klipper and Octoprint adding preventative measures to ensure you don't move outside of your print-zone or run dangerous gcode, linux terminals having confirmation prompts by default, to prevent yourself from accidentally pasting multiple lines (people have been targeted before by adding addl. commands to the buffer when you copy commands).

It makes sense to be precautionary with something that can easily severely damage a printer, catch a persons house on fire, or worse, if someone else decides they want to have malicious intent and share incorrect configurations. That is a indisputable fact. People copy/paste configs without reading through it, and that's something that's hard to prevent. I don't think it makes any sense to allow the config to simply run any command that are defined in them. Yes, configs can be shared today that have incorrect settings, but they're generally not bad enough to have a serious impact, due to various safety measures currently in place. Unrestricted shell access means any safety precautions, other than hardware ones, are out the window.

Unless I happened to have missed something not in this thread, seems like you'd rather attack and name call people despite others being rather helpful/friendly, just because someone disagrees with your ideas. Open-source doesn't mean implementing every request just because it's something you want, even if you think it's correct/incorrect.

[majastanislawska]

People who are allegedly dumb enough to copy paste rm -rf / from internet are equally prone to copy/paste rm -rf / --no-preserve-root this switch is not to protect them for being them.

This PR is not about running any command for god sake,
its for adding ability to have one that does one specific task only.
this is no about "unrestricted shell access"

Yes, you are missing WHOLE POINT
There is nothing friendly about amonut of absurdity and paranoia being poured on this topic.
There is nothing friendly abut fact that whole this stuff if just smoke screen fo plain old sexism and apparently transphobia.

[majastanislawska]

Tell me more how this "community" was "just being friendly" to Near or Rachel Bryk.