fix: tedge write remove hardcoded path#3099
Conversation
| match which::which(self.sudo_program.as_ref()) { | ||
| Ok(sudo) => { | ||
| let mut c = Command::new(sudo); | ||
| c.arg("--non-interactive"); |
There was a problem hiding this comment.
I'd recommend using the short form option -n here (even though it is less readable) because -n is more cross compatible, e.g. freebsd only has the -n option and not the long form.
Codecov ReportAll modified and coverable lines are covered by tests ✅ Additional details and impacted files📢 Thoughts on this report? Let us know! |
29e3b55 to
99bb5e7
Compare
When thin-edge spawns processes using sudo without `--non-inteactive` flag, sudo printed the following message: ``` sudo: a terminal is required to read the password; either use the -S option to read from standard input or configure an askpass helper ``` The flag makes sudo not ask for a password and instead fail immediately if a password is required. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
99bb5e7 to
3466c64
Compare
Robot Results
|
Oops I failed to notice the system tests are failing.
gligorisaev
left a comment
There was a problem hiding this comment.
Have checked the added Test step and keyword, working as expected
API for spawning a tedge-write process no longer uses a hardcoded `/usr/bin/tedge-write` path. The path was hardcoded because of the wrong assumption of how `sudo` works: when provided with a command without the full path, e.g. `sudo tedge-write /etc/config1`, `sudo` will internally look for `tedge-write` in $PATH and then compare path with its sudoers entry, so it's not necessary to use full path for binaries used with `sudo`. Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
3466c64 to
f8e94a5
Compare
albinsuresh
left a comment
There was a problem hiding this comment.
So, we're doing an explicit which lookup from the tedge-agent process to find tedge-write in its customized $PATH because sudo, by default, ignores the $PATH env of the parent process, right?
LGTM and I don't want to block this PR but I'm just wondering if running all sudo commands issued by tedge-agent with the --preserve-env option would be a better fix, as that'll make sure that not just tedge-write but any other process spawned by tedge-agent, like the software management plugins as well, would benefit from the same fix. I'm only concerned if it's some sort of "bad security practice" invoking sudo preserving the calling process's env.
| // if tedge-write is in PATH of tedge process, use it, if not, defer PATH lookup to sudo | ||
| let tedge_write_binary = | ||
| which::which_global(TEDGE_WRITE_BINARY).unwrap_or(TEDGE_WRITE_BINARY.into()); | ||
|
|
||
| let mut command = self.sudo.command(tedge_write_binary); |
There was a problem hiding this comment.
Oops, I have not been fast enough to answer @albinsuresh question before this PR being merged by @Bravo555 !
I'm just wondering if running all sudo commands issued by
tedge-agentwith the--preserve-envoption would be a better fix, as that'll make sure that not justtedge-writebut any other process spawned bytedge-agent, like the software management plugins as well, would benefit from the same fix. I'm only concerned if it's some sort of "bad security practice" invokingsudopreserving the calling process's env.
One simple way, to have all the components benefiting from the same fix, is to move the call to which::which_global() inside the method sudo.command().
Proposed changes
API for spawning a tedge-write process no longer uses a hardcoded
/usr/bin/tedge-writepath.The path was hardcoded because of the wrong assumption of how
sudoworks: when provided with a command without the full path, e.g.sudo tedge-write /etc/config1,sudowill internally look fortedge-writein $PATH and then compare path with its sudoers entry, so it's not necessary to use full path for binaries used withsudo.Types of changes
Paste Link to the issue
Checklist
cargo fmtas mentioned in CODING_GUIDELINEScargo clippyas mentioned in CODING_GUIDELINESFurther comments