From 15bcc66f2ca6bba73bbe51f0250083d25cf54e89 Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Tue, 4 Dec 2018 15:21:48 -0500 Subject: [PATCH 1/9] daemon: Use the 'sudo' or 'adm' group for the socket Fall back to 'root' if those groups don't exist on the host instead of throwing an exception. Fixes #456 --- src/daemon/daemon_main.cpp | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/daemon/daemon_main.cpp b/src/daemon/daemon_main.cpp index 8c68d493406..de47687b597 100644 --- a/src/daemon/daemon_main.cpp +++ b/src/daemon/daemon_main.cpp @@ -49,6 +49,8 @@ namespace mpp = multipass::platform; namespace { +const std::vector supported_socket_groups{"sudo", "adm"}; + void set_server_permissions(const std::string& server_address) { auto tokens = mp::utils::split(server_address, ":"); @@ -59,12 +61,16 @@ void set_server_permissions(const std::string& server_address) if (server_name != "unix") return; - auto group = getgrnam("sudo"); - if (!group) - throw std::runtime_error("Could not determine group id for 'sudo'."); + struct group* group{nullptr}; + for (const auto socket_group : supported_socket_groups) + { + group = getgrnam(socket_group.c_str()); + if (group) + break; + } const auto socket_path = tokens[1]; - if (chown(socket_path.c_str(), 0, group->gr_gid) == -1) + if (group && chown(socket_path.c_str(), 0, group->gr_gid) == -1) throw std::runtime_error("Could not set ownership of the multipass socket."); if (chmod(socket_path.c_str(), S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP) == -1) From bdafccc9db28956aa8b9d43a5c1cbc39b42dd22f Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Wed, 5 Dec 2018 11:27:58 -0500 Subject: [PATCH 2/9] client: Check if socket can be connected on rpc failure This will detect if the user has proper permission to access the Unix domain socket file. Fixes #460 --- include/multipass/cli/command.h | 23 +++++++++++++++++++++++ src/client/cmd/CMakeLists.txt | 1 + src/client/cmd/common_cli.cpp | 5 ++++- 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index ba7d6654da7..c1c3fdd3f62 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -21,8 +21,13 @@ #include #include #include +#include +#include #include + +#include + #include namespace multipass @@ -81,6 +86,24 @@ class Command { return on_success(reply); } + else + { + const auto tokens = multipass::utils::split(context.peer(), ":"); + if (tokens[0] == "unix") + { + auto socket_path = tokens[1]; + QLocalSocket multipassd_socket; + multipassd_socket.connectToServer(QString::fromStdString(socket_path)); + if (!multipassd_socket.waitForConnected() && + multipassd_socket.error() == QLocalSocket::SocketAccessError) + { + grpc::Status denied_status{ + grpc::StatusCode::PERMISSION_DENIED, "multipass socket accesss denied", + fmt::format("Please check that your user has group permission to access '{}'", socket_path)}; + return on_failure(denied_status); + } + } + } return on_failure(status); } diff --git a/src/client/cmd/CMakeLists.txt b/src/client/cmd/CMakeLists.txt index 4086be8e30c..f2d22ddfbef 100644 --- a/src/client/cmd/CMakeLists.txt +++ b/src/client/cmd/CMakeLists.txt @@ -44,4 +44,5 @@ target_link_libraries(commands ssh_client rpc Qt5::Core + Qt5::Network yaml) diff --git a/src/client/cmd/common_cli.cpp b/src/client/cmd/common_cli.cpp index a7410640b3a..95ec2056b1b 100644 --- a/src/client/cmd/common_cli.cpp +++ b/src/client/cmd/common_cli.cpp @@ -95,7 +95,10 @@ std::string cmd::instance_action_message_for(const mp::InstanceNames& instance_n mp::ReturnCode cmd::standard_failure_handler_for(const std::string& command, std::ostream& cerr, const grpc::Status& status, const std::string& error_details) { - fmt::print(cerr, "{} failed: {}\n{}", command, status.error_message(), error_details); + fmt::print(cerr, "{} failed: {}\n{}", command, status.error_message(), + !error_details.empty() + ? fmt::format("{}\n", error_details) + : !status.error_details().empty() ? fmt::format("{}\n", status.error_details()) : ""); return return_code_for(status.error_code()); } From fe1a7e5beaae189fbb01ecbc1dd03aa3c69ef486 Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Wed, 5 Dec 2018 13:01:12 -0500 Subject: [PATCH 3/9] launch: Change error formatting strings based on last commit --- src/client/cmd/launch.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/client/cmd/launch.cpp b/src/client/cmd/launch.cpp index 3a15c77681b..4be87071533 100644 --- a/src/client/cmd/launch.cpp +++ b/src/client/cmd/launch.cpp @@ -246,15 +246,15 @@ mp::ReturnCode cmd::Launch::request_launch() { if (error == LaunchError::INVALID_DISK_SIZE) { - error_details = fmt::format("Invalid disk size value supplied: {}\n", request.disk_space()); + error_details = fmt::format("Invalid disk size value supplied: {}", request.disk_space()); } else if (error == LaunchError::INVALID_MEM_SIZE) { - error_details = fmt::format("Invalid memory size value supplied: {}\n", request.mem_size()); + error_details = fmt::format("Invalid memory size value supplied: {}", request.mem_size()); } else if (error == LaunchError::INVALID_HOSTNAME) { - cerr << "Invalid instance name supplied: " << request.instance_name() << "\n"; + error_details = fmt::format("Invalid instance name supplied: {}", request.instance_name()); } } From b9e767c6a2c77f697bd14f53bba591b8f7e92f9e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sawicz?= Date: Tue, 11 Dec 2018 08:52:07 -0500 Subject: [PATCH 4/9] Reword text for telling user to check permissions on the socket Co-Authored-By: townsend2010 --- include/multipass/cli/command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index c1c3fdd3f62..9091d3daae7 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -99,7 +99,7 @@ class Command { grpc::Status denied_status{ grpc::StatusCode::PERMISSION_DENIED, "multipass socket accesss denied", - fmt::format("Please check that your user has group permission to access '{}'", socket_path)}; + fmt::format("Please check that your user has read-write permission to '{}'", socket_path)}; return on_failure(denied_status); } } From 7c7d522ea8236016b1458c10aaf4469c455af5dd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sawicz?= Date: Tue, 11 Dec 2018 10:02:39 -0500 Subject: [PATCH 5/9] Fix typo Co-Authored-By: townsend2010 --- include/multipass/cli/command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index 9091d3daae7..1fa2e582746 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -98,7 +98,7 @@ class Command multipassd_socket.error() == QLocalSocket::SocketAccessError) { grpc::Status denied_status{ - grpc::StatusCode::PERMISSION_DENIED, "multipass socket accesss denied", + grpc::StatusCode::PERMISSION_DENIED, "multipass socket access denied", fmt::format("Please check that your user has read-write permission to '{}'", socket_path)}; return on_failure(denied_status); } From 4ff917d9e3fe3f83a4fe9e5d62da5dcbaf5ec00d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sawicz?= Date: Tue, 11 Dec 2018 10:26:29 -0500 Subject: [PATCH 6/9] Better wording for user facing text Co-Authored-By: townsend2010 --- include/multipass/cli/command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index 1fa2e582746..8a0b41f0590 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -99,7 +99,7 @@ class Command { grpc::Status denied_status{ grpc::StatusCode::PERMISSION_DENIED, "multipass socket access denied", - fmt::format("Please check that your user has read-write permission to '{}'", socket_path)}; + fmt::format("Please check that you have read/write permissions to '{}'", socket_path)}; return on_failure(denied_status); } } From 1eddc0547124358bb2d190fbf8dba58c42ed6936 Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Tue, 11 Dec 2018 10:47:02 -0500 Subject: [PATCH 7/9] client: Add catch all error message if the connect to the socket fails --- include/multipass/cli/command.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index 8a0b41f0590..e5d0af88271 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -102,6 +102,13 @@ class Command fmt::format("Please check that you have read/write permissions to '{}'", socket_path)}; return on_failure(denied_status); } + else + { + grpc::Status access_error_status{ + grpc::StatusCode::NOT_FOUND, "cannot connect to the multipass socket", + fmt::format("Please ensure multipassd is running and '{}' exists", socket_path)}; + return on_failure(access_error_status); + } } } From 14b44be86471df4e495cd479a2a993632ed4a9df Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Micha=C5=82=20Sawicz?= Date: Tue, 11 Dec 2018 11:57:56 -0500 Subject: [PATCH 8/9] Change wording for catch all text Co-Authored-By: townsend2010 --- include/multipass/cli/command.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index e5d0af88271..9f47fbd6482 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -106,7 +106,7 @@ class Command { grpc::Status access_error_status{ grpc::StatusCode::NOT_FOUND, "cannot connect to the multipass socket", - fmt::format("Please ensure multipassd is running and '{}' exists", socket_path)}; + fmt::format("Please ensure multipassd is running and '{}' is accessible", socket_path)}; return on_failure(access_error_status); } } From 8c00a1dae7ad797d1aec08e83a82eeed368ef83b Mon Sep 17 00:00:00 2001 From: Chris Townsend Date: Tue, 11 Dec 2018 12:05:34 -0500 Subject: [PATCH 9/9] client: Move catch all socket error handling outside of unix socket check --- include/multipass/cli/command.h | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/include/multipass/cli/command.h b/include/multipass/cli/command.h index 9f47fbd6482..c4916290324 100644 --- a/include/multipass/cli/command.h +++ b/include/multipass/cli/command.h @@ -88,31 +88,29 @@ class Command } else { + auto socket_address{context.peer()}; const auto tokens = multipass::utils::split(context.peer(), ":"); if (tokens[0] == "unix") { - auto socket_path = tokens[1]; + socket_address = tokens[1]; QLocalSocket multipassd_socket; - multipassd_socket.connectToServer(QString::fromStdString(socket_path)); + multipassd_socket.connectToServer(QString::fromStdString(socket_address)); if (!multipassd_socket.waitForConnected() && multipassd_socket.error() == QLocalSocket::SocketAccessError) { grpc::Status denied_status{ grpc::StatusCode::PERMISSION_DENIED, "multipass socket access denied", - fmt::format("Please check that you have read/write permissions to '{}'", socket_path)}; + fmt::format("Please check that you have read/write permissions to '{}'", socket_address)}; return on_failure(denied_status); } - else - { - grpc::Status access_error_status{ - grpc::StatusCode::NOT_FOUND, "cannot connect to the multipass socket", - fmt::format("Please ensure multipassd is running and '{}' is accessible", socket_path)}; - return on_failure(access_error_status); - } } - } - return on_failure(status); + grpc::Status access_error_status{ + grpc::StatusCode::NOT_FOUND, "cannot connect to the multipass socket", + fmt::format("Please ensure multipassd is running and '{}' is accessible", socket_address)}; + + return on_failure(access_error_status); + } } template