Fix bridging authorization

src/client/cli/cmd/common_callbacks.h

@@ -20,6 +20,8 @@
 #include "animated_spinner.h"
 
 #include <multipass/cli/client_common.h>
+#include <multipass/cli/prompters.h>
+#include <multipass/constants.h>
 #include <multipass/terminal.h>
 
 #include <grpc++/grpc++.h>
@@ -71,6 +73,27 @@ auto make_iterative_spinner_callback(AnimatedSpinner& spinner, Terminal& term)
         }
     };
 }
+
+template <typename Request, typename Reply>
+auto make_confirmation_callback(Terminal& term, const QString& key)
+{
+    return [&key, &term](Reply& reply, grpc::ClientReaderWriterInterface<Request, Reply>* client) {
+        if (key.startsWith(daemon_settings_root) && key.endsWith(bridged_network_name) && reply.needs_authorization())
+        {
+            auto bridged_network = reply.reply_message();
+
+            std::vector<std::string> nets(1, bridged_network);
+
+            BridgePrompter prompter(&term);
+
+            auto request = Request{};
+            auto answer = prompter.bridge_prompt(nets);
+            request.set_authorized(answer);
+
+            client->Write(request);
+        }
+    };
+}
 } // namespace multipass
 
 #endif // MULTIPASS_COMMON_CALLBACKS_H

src/client/cli/cmd/remote_settings_handler.cpp

@@ -111,22 +111,29 @@ class RemoteGet : public RemoteSettingsCmd
 class RemoteSet : public RemoteSettingsCmd
 {
 public:
-    RemoteSet(const QString& key, const QString& val, mp::Rpc::StubInterface& stub, mp::Terminal* term, int verbosity)
+    RemoteSet(const QString& key,
+              const QString& val,
+              mp::Rpc::StubInterface& stub,
+              mp::Terminal* term,
+              int verbosity,
+              bool user_authorized)
         : RemoteSettingsCmd{stub, term} // need to ensure refs outlive this
     {
         mp::SetRequest set_request;
         set_request.set_verbosity_level(verbosity);
         set_request.set_key(key.toStdString());
         set_request.set_val(val.toStdString());
+        set_request.set_authorized(user_authorized);
 
         mp::AnimatedSpinner spinner{cout};
 
-        [[maybe_unused]] auto ret =
-            dispatch(&RpcMethod::set,
-                     set_request,
-                     on_success<mp::SetReply>,
-                     on_failure,
-                     mp::make_reply_spinner_callback<mp::SetRequest, mp::SetReply>(spinner, cerr));
+        auto streaming_confirmation_callback = mp::make_confirmation_callback<mp::SetRequest, mp::SetReply>(*term, key);
+
+        [[maybe_unused]] auto ret = dispatch(&RpcMethod::set,
+                                             set_request,
+                                             on_success<mp::SetReply>,
+                                             on_failure,
+                                             streaming_confirmation_callback);
         assert(ret == mp::ReturnCode::Ok && "should have thrown otherwise");
     }
 };
@@ -188,7 +195,7 @@ void mp::RemoteSettingsHandler::set(const QString& key, const QString& val)
     if (key.startsWith(key_prefix))
     {
         assert(term);
-        RemoteSet(key, val, stub, term, verbosity);
+        RemoteSet(key, val, stub, term, verbosity, false);
     }
     else
         throw mp::UnrecognizedSettingException{key};

src/daemon/daemon.cpp

@@ -1250,7 +1250,8 @@
                                            std::function<void()> instance_persister,
                                            std::function<std::string()> bridged_interface,
                                            std::function<std::string()> bridge_name,
-                                           std::function<std::vector<mp::NetworkInterfaceInfo>()> host_networks)
+                                           std::function<std::vector<mp::NetworkInterfaceInfo>()> host_networks,
+                                           std::function<bool()> user_authorized)
 {
     return MP_SETTINGS.register_handler(std::make_unique<mp::InstanceSettingsHandler>(vm_instance_specs,
                                                                                       operative_instances,
@@ -1259,7 +1260,8 @@
                                                                                       std::move(instance_persister),
                                                                                       std::move(bridged_interface),
                                                                                       std::move(bridge_name),
-                                                                                      host_networks));
+                                                                                      host_networks,
+                                                                                      user_authorized));
 }
 
 mp::SettingsHandler* register_snapshot_mod(
@@ -1420,7 +1422,8 @@
           [this]() {
               return config->factory->bridge_name_for(MP_SETTINGS.get(mp::bridged_interface_key).toStdString());
           },
-          [this]() { return config->factory->networks(); })},
+          [this]() { return config->factory->networks(); },
+          [this]() { return user_authorized; })},
       snapshot_mod_handler{
           register_snapshot_mod(operative_instances, deleted_instances, preparing_instances, *config->factory)}
 {
@@ -2545,13 +2548,49 @@
 
     auto key = request->key();
     auto val = request->val();
+    user_authorized = request->authorized();
 
     mpl::log(mpl::Level::trace, category, fmt::format("Trying to set {}={}", key, val));
     MP_SETTINGS.set(QString::fromStdString(key), QString::fromStdString(val));
     mpl::log(mpl::Level::debug, category, fmt::format("Succeeded setting {}={}", key, val));
 
+    user_authorized = false;
+
     status_promise->set_value(grpc::Status::OK);
 }
+catch (const mp::NonAuthorizedBridgeSettingsException& e)
+{
+    auto key = request->key();
+    auto val = request->val();
+    mpl::log(mpl::Level::debug, category, fmt::format("Asking for user authorization to set {}={}", key, val));
+
+    auto reply = SetReply{};
+    reply.set_needs_authorization(true);
+    reply.set_reply_message(get_bridged_interface_name());
+    server->Write(reply);
+
+    auto callback_request = SetRequest{};
+    server->Read(&callback_request);
+
+    user_authorized = callback_request.authorized();
+
+    if (user_authorized)
+    {
+        MP_SETTINGS.set(QString::fromStdString(key), QString::fromStdString(val));
+
+        user_authorized = false;
+
+        mpl::log(mpl::Level::debug, category, fmt::format("Succeeded setting {}={}", key, val));
+
+        status_promise->set_value(grpc::Status::OK);
+    }
+    else
+    {
+        mpl::log(mpl::Level::debug, category, "User did not authorize, cancelling");
+
+        status_promise->set_value(grpc::Status(grpc::StatusCode::FAILED_PRECONDITION, e.what(), ""));
+    }
+}
 catch (const mp::UnrecognizedSettingException& e)
 {
     status_promise->set_value(grpc::Status(grpc::StatusCode::INVALID_ARGUMENT, e.what(), ""));

src/daemon/daemon.h

@@ -225,6 +225,7 @@ public slots:
     SettingsHandler* instance_mod_handler;
     SettingsHandler* snapshot_mod_handler;
     std::unordered_map<std::string, std::unordered_map<std::string, MountHandler::UPtr>> mounts;
+    bool user_authorized = false;
 };
 } // namespace multipass
 #endif // MULTIPASS_DAEMON_H

src/daemon/instance_settings_handler.cpp

@@ -168,10 +168,12 @@ bool is_bridged(const mp::VMSpecs& spec, const std::string& br_interface, const
 
 void checked_update_bridged(const QString& key,
                             const QString& val,
+                            const std::string& instance_name,
                             mp::VirtualMachine& instance,
                             mp::VMSpecs& spec,
                             const std::string& br_interface,
-                            const std::string& br_name)
+                            const std::string& br_name,
+                            bool needs_authorization)
 {
     auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true";
 
@@ -182,18 +184,27 @@ void checked_update_bridged(const QString& key,
 
     if (bridged)
     {
+        if (needs_authorization)
+        {
+            throw mp::NonAuthorizedBridgeSettingsException(operation_msg(Operation::Modify),
+                                                           instance_name,
+                                                           br_interface);
+        }
+
         // The empty string in the MAC indicates the daemon that the interface must be configured.
         spec.extra_interfaces.push_back({br_interface, "", true});
     }
 }
 
 void update_bridged(const QString& key,
                     const QString& val,
+                    const std::string& instance_name,
                     mp::VirtualMachine& instance,
                     mp::VMSpecs& spec,
                     std::function<std::string()> bridged_interface,
                     std::function<std::string()> bridge_name,
-                    std::function<std::vector<mp::NetworkInterfaceInfo>()> host_networks)
+                    std::function<std::vector<mp::NetworkInterfaceInfo>()> host_networks,
+                    std::function<bool()> user_authorized)
 {
     const auto& host_nets = host_networks(); // This will throw if not implemented on this backend.
     const auto& br = bridged_interface();
@@ -208,7 +219,8 @@ void update_bridged(const QString& key,
                         mp::bridged_interface_key));
     }
 
-    checked_update_bridged(key, val, instance, spec, br, bridge_name());
+    bool needs_authorization = info->needs_authorization && !user_authorized();
+    checked_update_bridged(key, val, instance_name, instance, spec, br, bridge_name(), needs_authorization);
 }
 
 } // namespace
@@ -227,15 +239,17 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler(
     std::function<void()> instance_persister,
     std::function<std::string()> bridged_interface,
     std::function<std::string()> bridge_name,
-    std::function<std::vector<NetworkInterfaceInfo>()> host_networks)
+    std::function<std::vector<NetworkInterfaceInfo>()> host_networks,
+    std::function<bool()> user_authorized_bridge)
     : vm_instance_specs{vm_instance_specs},
       operative_instances{operative_instances},
       deleted_instances{deleted_instances},
       preparing_instances{preparing_instances},
       instance_persister{std::move(instance_persister)},
       bridged_interface{std::move(bridged_interface)},
       bridge_name{std::move(bridge_name)},
-      host_networks{std::move(host_networks)}
+      host_networks{std::move(host_networks)},
+      user_authorized_bridge{user_authorized_bridge}
 {
 }
 
@@ -285,7 +299,15 @@ void mp::InstanceSettingsHandler::set(const QString& key, const QString& val)
         update_cpus(key, val, instance, spec);
     else if (property == bridged_suffix)
     {
-        update_bridged(key, val, instance, spec, bridged_interface, bridge_name, host_networks);
+        update_bridged(key,
+                       val,
+                       instance_name,
+                       instance,
+                       spec,
+                       bridged_interface,
+                       bridge_name,
+                       host_networks,
+                       user_authorized_bridge);
     }
     else
     {

src/daemon/instance_settings_handler.h

@@ -45,7 +45,8 @@ class InstanceSettingsHandler : public SettingsHandler
                             std::function<void()> instance_persister,
                             std::function<std::string()> bridged_interface,
                             std::function<std::string()> bridge_name,
-                            std::function<std::vector<NetworkInterfaceInfo>()> host_networks);
+                            std::function<std::vector<NetworkInterfaceInfo>()> host_networks,
+                            std::function<bool()> user_authorized);
 
     std::set<QString> keys() const override;
     QString get(const QString& key) const override;
@@ -66,6 +67,7 @@ class InstanceSettingsHandler : public SettingsHandler
     std::function<std::string()> bridged_interface;
     std::function<std::string()> bridge_name;
     std::function<std::vector<NetworkInterfaceInfo>()> host_networks;
+    std::function<bool()> user_authorized_bridge;
 };
 
 class InstanceSettingsException : public SettingsException
@@ -74,6 +76,15 @@ class InstanceSettingsException : public SettingsException
     InstanceSettingsException(const std::string& reason, const std::string& instance, const std::string& detail);
 };
 
+class NonAuthorizedBridgeSettingsException : public InstanceSettingsException
+{
+public:
+    NonAuthorizedBridgeSettingsException(const std::string& reason, const std::string& instance, const std::string& net)
+        : InstanceSettingsException{reason, instance, fmt::format("Need user authorization to bridge {}", net)}
+    {
+    }
+};
+
 } // namespace multipass
 
 #endif // MULTIPASS_INSTANCE_SETTINGS_HANDLER_H

src/rpc/multipass.proto

@@ -470,11 +470,13 @@ message SetRequest {
     string key = 1;
     string val = 2;
     int32 verbosity_level = 3;
+    bool authorized = 4;
 }
 
 message SetReply {
     string log_line = 1;
     string reply_message = 2;
+    bool needs_authorization = 3;
 }
 
 message KeysRequest {

tests/test_common_callbacks.cpp

@@ -168,3 +168,21 @@ TEST_F(TestSpinnerCallbacks, iterativeSpinnerCallbackHandlesPasswordRequest)
     EXPECT_THAT(err.str(), IsEmpty());
     EXPECT_THAT(out.str(), clearStreamMatcher());
 }
+
+TEST_F(TestSpinnerCallbacks, confirmationCallbackAnswers)
+{
+    constexpr auto key = "local.instance-name.bridged";
+
+    mpt::MockClientReaderWriter<mp::SetRequest, mp::SetReply> mock_client;
+
+    mp::SetReply reply;
+    reply.set_needs_authorization(true);
+
+    EXPECT_CALL(mock_client, Write(_, _)).WillOnce(Return(true));
+
+    auto callback = mp::make_confirmation_callback<mp::SetRequest, mp::SetReply>(term, key);
+    callback(reply, &mock_client);
+
+    EXPECT_THAT(err.str(), IsEmpty());
+    EXPECT_THAT(out.str(), clearStreamMatcher());
+}