reckless: rust rewrite - #9239
Conversation
8644166 to
6889d36
Compare
vincenzopalazzo
left a comment
There was a problem hiding this comment.
Nice to see that the manifest idea is taking over. Hope you are also using git for versioning, otherwise this would be another good thing to take from https://github.com/coffee-tools/coffee
There was a problem hiding this comment.
Would be nice to reference the original reference implementation https://coffee-docs.netlify.app/support-coffee
There was a problem hiding this comment.
Would be nice to reference the original reference implementation https://coffee-docs.netlify.app/support-coffee
I was planning on giving full credit for the manifest/tip function to coffee in the README. But i want to lock down the functionality first before writing documentation.
There was a problem hiding this comment.
to save credit was also good to just include coffee, I do not think there are more features than coffee, we were also supporting ws (palanning to) to support interactive UX inside the web applications
But these days cloning is easy!
If you mean installing a plugin with a git ref then yes, that is supported. |
Yeah, otherwise it's insanity! Nice, it would be nice to have two lines that reference coffee because the development died because Blockstream was moving to reckless. Referencing a good idea is what is left in this AI era, where rebuilding is easy |
6658af8 to
f6bbfb5
Compare
5833d8b to
853fe7d
Compare
f4513c8 to
bf9e3e2
Compare
|
|
Changelog-Changed: reckless: complete rewrite with new subcommands `listinstalled` and `tip`. Replaced `search` with `listavailable`.
| return Err(anyhow!("install_cmd in manifest is empty!?")); | ||
| } | ||
|
|
||
| let mut cmd = Command::new(cmd_parts.first().unwrap()); |
There was a problem hiding this comment.
install_cmd in the reckless manifest lets any plugin repo specify a list of commands that reckless install will run on the user's node, with no confirmation prompt and no sandbox, contrary to the previous installation behaviour which only ran commands from a list of authorized installers (cargo, uv, poetry, npm, etc..).
Should we treat this kind of user-added sources differently? Would it be wise to keep this but adding a flag that explicitly allow this?
There was a problem hiding this comment.
Not sure a flag would improve security or anything. Unless something else on your machine is worth more than your node's Bitcoin and these commands could get access to that. Otherwise you already have to trust the plugin author's code to not do something nefarious like calling xpay with an offer to the plugin author's node until your balance is drained.
There was a problem hiding this comment.
But isn't that a big change in code capabilities if the installation process was previously sandboxed and now is not? I'm not referring to an improper use of the node, but to potential arbitrary code execution. Is this prevented in some way I'm currently missing?
There was a problem hiding this comment.
Not sure what you mean by "change in code capabilities"? Was the python version doing something like that?
The install commands are read from the manifest.json file included in the plugin's source code. An attacker can write an attack in there or in the plugin itself. Both will run commands under the CLN user, i see no significant difference between the two.
There was a problem hiding this comment.
oh so this is not a new attack vettor, my bad. I must have misinterpreted how install_to_python_virtual_environment worked in the old reckless python file. I thought the provided code was somehow vetted/sandboxed. Nevermind then, I withdraw my first comment :)
| } | ||
| } | ||
|
|
||
| match fs::remove_dir_all(&plugin_dir).await { |
There was a problem hiding this comment.
uninstall reclaims reckless_dir/<plugin_name> but never the cloned source repo at reckless_dir/<owner>/<repo>. Is this intended, to work as some sort of cache?
There was a problem hiding this comment.
Yes, this is intended. Checking out a repo can take some time so ideally we only want to do it once when the source is added. So then we also only remove it when the source is removed not a plugin belonging to that source. We would also have to check if the repo contains a single plugin or multiple for this to make sense.
If the timestamp of the python file does not differ after an update it will load the bytecode of the old version. We remove the pycache to make sure this does not happen. Mostly a test environment issue as in practive the timestamps should always differ.
Not ready for code review yet, we have not decided on some important aspects. Right now it is a drop-in replacement of the python reckless version.
Should
Fixes: #8439
Fixes: #8761
Fixes: #8775
Fixes: #8776
Fixes: #8794
and should supercede #8630
Key differences:
reckless-diris a proper plugin option now--jsonoptionlistavailable, an array of strings is just not nice heremanifest.json