exec runs sudo commands with -u to preserve user
#3959
Conversation
4a2ddab to
1e95dc2
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3959 +/- ##
==========================================
+ Coverage 89.11% 89.14% +0.03%
==========================================
Files 255 257 +2
Lines 14603 14644 +41
==========================================
+ Hits 13014 13055 +41
Misses 1589 1589 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1e95dc2 to
15d9e46
Compare
src/client/cli/cmd/exec.cpp
Outdated
| // ran with sudo, what forces us to run everything through `sh`. | ||
| auto sh_args = fmt::format("cd {} && {}", *dir, fmt::join(args, " ")); | ||
| // Extract the command without the sudo prefix | ||
| std::vector<std::string> cmd_args(args.begin() + 1, args.end()); |
There was a problem hiding this comment.
unchecked iterator access: begin() + 1 might not be present in the args
There was a problem hiding this comment.
also, std::next(args.begin()) would express the intent better here.
There was a problem hiding this comment.
| std::vector<std::string> cmd_args(args.begin() + 1, args.end()); | |
| const auto cmd_args = [&args]() -> std::vector<std::string> { | |
| if(args.size() < 2){ | |
| return {}; | |
| } | |
| return {std::next(args.begin()), args.end()}; | |
| }(); |
There was a problem hiding this comment.
It seems that this change results in a compiler error for me, possibly because the initializer list constructors {std::next(args.begin()), args.end()} create temporary objects that cannot be bound to references when using decltype(args).
multipass/src/client/cli/cmd/exec.cpp: In lambda function:
multipass/src/client/cli/cmd/exec.cpp:174:33: error: returning reference to temporary [-Werror=return-local-addr]
174 | return {};
| ^
multipass/src/client/cli/cmd/exec.cpp:176:64: error: returning reference to temporary [-Werror=return-local-addr]
176 | return {std::next(args.begin()), args.end()};
| ^
cc1plus: all warnings being treated as errorsThere was a problem hiding this comment.
Ah right, I missed that args is a const std::vector<std::string>&. Replacing decltype(args) with std::vector<std::string> should fix the issue.
There was a problem hiding this comment.
BTW feel free to use any approach you're comfortable with -- the code was just an example. I'm okay with any alternative as long as it ensures that the referenced iterators are valid.
There was a problem hiding this comment.
I think the original code is safe with args.begin() + 1 in the if (args[0] == "sudo") branch. Because that guarantees the arguments number is >=1, when there is only one argument args.begin() + 1 is args.end() and will result in cmd_args empty vector, which is expected. As to args.begin() + 1 vs std::next, std::next in the case of std::vector uses random access iterator, so it is the same performance-wise but maybe more expressive as @xmkg mentioned.
Actually, what is dubious to me is if (args[0] == "sudo") check, there is no check whether args is non-empty. So that 0 index deference might be invalid. But this is from the original code instead of this PR.
There was a problem hiding this comment.
std::nextin the case ofstd::vectoruses random access iterator, so it is the same performance-wise but maybe more expressive as @xmkg mentioned.
I strongly agree with the benefit of replacing args.begin() + 1 with std::next in terms of readability and expressiveness
I'm okay with any alternative as long as it ensures that the referenced iterators are valid.
I have no issues with a lambda, but I believe we can also do something simpler like
if (args[0] == "sudo")
{
// Extract the command without the sudo prefix
std::vector<std::string> cmd_args;
if (args.size() > 1)
cmd_args.assign(std::next(args.begin()), args.end());
// Use sudo to access the directory, but run the command as the ubuntu user,
// then use sudo again to execute the original command
// This preserves the correct SUDO_ environment variables
auto sh_args = fmt::format("cd {} && sudo -u {} sudo {}", *dir, username, fmt::join(cmd_args, " "));
all_args = {{"sudo", "sh", "-c", sh_args}};
}@georgeliao thoughts?
There was a problem hiding this comment.
The new code snippet also looks good to me.
There was a problem hiding this comment.
But now if I think about it again with the whole context. You do not need to extract the cmd_args, right? you can just do
const auto sh_args = fmt::format("cd {} && sudo -u {} {}", *dir, username, fmt::join(args, " "));, to avoid the 2nd hard code sudo string because the args[0] is sudo already.
src/client/cli/cmd/exec.cpp
Outdated
| // Use sudo to access the directory, but run the command as the ubuntu user, | ||
| // then use sudo again to execute the original command | ||
| // This preserves the correct SUDO_ environment variables | ||
| auto sh_args = fmt::format("cd {} && sudo -u {} sudo {}", *dir, username, fmt::join(cmd_args, " ")); |
There was a problem hiding this comment.
| auto sh_args = fmt::format("cd {} && sudo -u {} sudo {}", *dir, username, fmt::join(cmd_args, " ")); | |
| const auto sh_args = fmt::format("cd {} && sudo -u {} sudo {}", *dir, username, fmt::join(cmd_args, " ")); |
|
@levkropp |
|
After cloning and re-building this PR again, it appears that all tests are passing successfully! And the PR resolves the issue seen with SUDO_ env vars: |
|
It looks good to me. @levkropp Do we need a rebase to fix the doc build failure? |
|
@xmkg I do not have any questions on this PR anymore. Feel free to have a final check. If things are fine, we can go ahead and merge. |
fixes #3933
I had to run the
sudo concierge prepare -p dev --extra-snaps terraformcommand twice whether I was shelled in withmultipass shellor usingmultipass exec. Runningmultipass exec juju-dev -- sudo env | grep SUDO_gives the correct SUDO environment variablesIn order to
cdinto directories we need to dosudo sh -cbecausecdcannot be run withsudo. This changes our SUDO_ user. We could usesudo -Eto preserve the entire environment but that may cause unintended behavior. Instead we usesudo -uto restore the user. We know the user from thessh_info.username()