daemon: Add socket activation via /run/rpm-ostreed.socket

For historical reasons, the daemon ends up doing a lot of
initialization before even claiming the DBus name.  For example,
it calls `ostree_sysroot_load()`.  We also end up scanning
the RPM database, and actually parse all the GPG keys
in `/etc/pki/rpm-gpg` etc.

This causes two related problems:

- By doing all this work before claiming the bus name, we
  race against the (pretty low) DBus service timeout of 25s.
- If something hard fails at initialization, the client can't
  easily see the error because it just appears in the systemd
  journal.  The client will just see a service timeout.

The solution to this is to adopt systemd socket activation.
There's a new `rpm-ostreed.socket` and the daemon can be activated
that way.

The client (when run as root, the socket is only accessible to root
right now) connects, which will activate the daemon and attempt
initialization - without claiming the DBus name yet.

If something goes wrong here, the daemon will reply to the client
that activated it with the error, and then also exit with failure.

On success, everything continues as normal, including claiming
the DBus name.

Note that this also logically replaces the code that does explicit
`systemctl start rpm-ostreed` invocations.

After this patch:

```
$ systemctl stop rpm-ostreed
$ umount /boot
$ rpm-ostree status
error: Couldn't start daemon: Error setting up sysroot: loading sysroot: Unexpected state: /run/ostree-booted found, but no /boot/loader directory
```

Co-authored-by: Colin Walters <[email protected]>

Author: RishabhSaini

Cargo.toml

@@ -51,7 +51,7 @@ cap-std-ext = "2.0"
 cap-std = { version = "1.0.3", features = ["fs_utf8"] }
 containers-image-proxy = "0.5.1"
 # Explicitly force on libc
-rustix = { version = "0.37", features = ["use-libc"] }
+rustix = { version = "0.37", features = ["use-libc", "net"] }
 cap-primitives = "1.0.3"
 chrono = { version = "0.4.26", features = ["serde"] }
 clap = { version = "4.3", features = ["derive"] }

Makefile-daemon.am

@@ -63,14 +63,15 @@ systemdunit_service_file_names = \
 	$(NULL)
 
 systemdunit_service_files = $(addprefix $(srcdir)/src/daemon/,$(systemdunit_service_file_names))
-systemdunit_timer_files = \
+systemdunit_other_files = \
 	$(srcdir)/src/daemon/rpm-ostreed-automatic.timer \
 	$(srcdir)/src/daemon/rpm-ostree-countme.timer \
+       $(srcdir)/src/daemon/rpm-ostreed.socket \
 	$(NULL)
 
 systemdunit_DATA = \
 	$(systemdunit_service_files) \
-	$(systemdunit_timer_files) \
+	$(systemdunit_other_files) \
 	$(NULL)
 
 systemdunitdir       = $(prefix)/lib/systemd/system/
@@ -103,7 +104,7 @@ EXTRA_DIST += \
 	$(sysconf_DATA) \
 	$(service_in_files) \
 	$(systemdunit_service_in_files) \
-	$(systemdunit_timer_files) \
+	$(systemdunit_other_files) \
 	$(NULL)
 
 CLEANFILES += \

rpmostree-cxxrs.cxx

@@ -8,6 +8,7 @@
 #include "rpmostree-package-variants.h"
 #include "rpmostree-rpm-util.h"
 #include "rpmostree-util.h"
+#include "rpmostreed-daemon.hpp"
 #include "rpmostreemain.h"
 #include "src/libpriv/rpmostree-cxxrs-prelude.h"
 #include <algorithm>
@@ -2195,6 +2196,10 @@ extern "C"
                                                ::rust::Str opt_deploy_id, ::rust::Str opt_os_name,
                                                ::rpmostreecxx::OstreeDeployment **return$) noexcept;
 
+  ::rust::repr::PtrLen rpmostreecxx$cxxbridge1$daemon_main (bool debug) noexcept;
+
+  void rpmostreecxx$cxxbridge1$daemon_terminate () noexcept;
+
   ::rust::repr::PtrLen rpmostreecxx$cxxbridge1$daemon_sanitycheck_environment (
       const ::rpmostreecxx::OstreeSysroot &sysroot) noexcept;
 
@@ -2970,6 +2975,34 @@ extern "C"
     return throw$;
   }
 
+  ::rust::repr::PtrLen
+  rpmostreecxx$cxxbridge1$daemon_init_inner (bool debug) noexcept
+  {
+    void (*daemon_init_inner$) (bool) = ::rpmostreecxx::daemon_init_inner;
+    ::rust::repr::PtrLen throw$;
+    ::rust::behavior::trycatch (
+        [&] {
+          daemon_init_inner$ (debug);
+          throw$.ptr = nullptr;
+        },
+        ::rust::detail::Fail (throw$));
+    return throw$;
+  }
+
+  ::rust::repr::PtrLen
+  rpmostreecxx$cxxbridge1$daemon_main_inner () noexcept
+  {
+    void (*daemon_main_inner$) () = ::rpmostreecxx::daemon_main_inner;
+    ::rust::repr::PtrLen throw$;
+    ::rust::behavior::trycatch (
+        [&] {
+          daemon_main_inner$ ();
+          throw$.ptr = nullptr;
+        },
+        ::rust::detail::Fail (throw$));
+    return throw$;
+  }
+
   ::rust::repr::PtrLen
   rpmostreecxx$cxxbridge1$client_require_root () noexcept
   {
@@ -4038,6 +4071,22 @@ deployment_get_base (::rpmostreecxx::OstreeSysroot &sysroot, ::rust::Str opt_dep
   return ::std::move (return$.value);
 }
 
+void
+daemon_main (bool debug)
+{
+  ::rust::repr::PtrLen error$ = rpmostreecxx$cxxbridge1$daemon_main (debug);
+  if (error$.ptr)
+    {
+      throw ::rust::impl< ::rust::Error>::error (error$);
+    }
+}
+
+void
+daemon_terminate () noexcept
+{
+  rpmostreecxx$cxxbridge1$daemon_terminate ();
+}
+
 void
 daemon_sanitycheck_environment (const ::rpmostreecxx::OstreeSysroot &sysroot)
 {

rpmostree-cxxrs.h

@@ -8,6 +8,7 @@
 #include "rpmostree-package-variants.h"
 #include "rpmostree-rpm-util.h"
 #include "rpmostree-util.h"
+#include "rpmostreed-daemon.hpp"
 #include "rpmostreemain.h"
 #include "src/libpriv/rpmostree-cxxrs-prelude.h"
 #include <algorithm>
@@ -1843,6 +1844,10 @@ ::rpmostreecxx::OstreeDeployment *deployment_get_base (::rpmostreecxx::OstreeSys
                                                        ::rust::Str opt_deploy_id,
                                                        ::rust::Str opt_os_name);
 
+void daemon_main (bool debug);
+
+void daemon_terminate () noexcept;
+
 void daemon_sanitycheck_environment (const ::rpmostreecxx::OstreeSysroot &sysroot);
 
 ::rust::String deployment_generate_id (const ::rpmostreecxx::OstreeDeployment &deployment) noexcept;

rust/src/client.rs

@@ -6,12 +6,14 @@ use crate::core::OSTREE_BOOTED;
 use crate::cxxrsutil::*;
 use crate::ffi::SystemHostType;
 use crate::utils;
-use anyhow::{anyhow, Result};
+use anyhow::{anyhow, Context, Result};
+use camino::Utf8Path;
 use fn_error_context::context;
 use gio::prelude::*;
 use ostree_ext::{gio, glib};
 use std::io::{BufRead, Write};
 use std::os::unix::io::IntoRawFd;
+use std::os::unix::net::UnixStream;
 use std::process::Command;
 
 /// The well-known bus name.
@@ -20,6 +22,7 @@ const BUS_NAME: &str = "org.projectatomic.rpmostree1";
 const SYSROOT_PATH: &str = "/org/projectatomic/rpmostree1/Sysroot";
 const OS_INTERFACE: &str = "org.projectatomic.rpmostree1.OS";
 const OS_EX_INTERFACE: &str = "org.projectatomic.rpmostree1.OSExperimental";
+const CLIENT_SOCKET_PATH: &str = "/run/rpm-ostree/client.sock";
 
 /// A unique DBus connection to the rpm-ostree daemon.
 /// This currently wraps a C++ client connection.
@@ -49,7 +52,8 @@ impl ClientConnection {
             SYSROOT_PATH,
             "org.projectatomic.rpmostree1.Sysroot",
             gio::Cancellable::NONE,
-        )?;
+        )
+        .context("Initializing sysroot proxy")?;
         // Today the daemon mode requires running inside a booted deployment.
         let booted = sysroot_proxy
             .cached_property("Booted")
@@ -156,46 +160,63 @@ pub(crate) fn client_handle_fd_argument(
     }
 }
 
-/// Explicitly ensure the daemon is started via systemd, if possible.
-///
-/// This works around bugs from DBus activation, see
-/// https://github.com/coreos/rpm-ostree/pull/2932
-///
-/// Basically we load too much data before claiming the bus name,
-/// and dbus doesn't give us a useful error.  Instead, let's talk
-/// to systemd directly and use its client tools to scrape errors.
-///
-/// What we really should do probably is use native socket activation.
-pub(crate) fn client_start_daemon() -> CxxResult<()> {
-    let service = "rpm-ostreed.service";
-    // Assume non-root can't use systemd right now.
-    if rustix::process::getuid().as_raw() != 0 {
-        return Ok(());
+// If the client socket doesn't exist, try to ask systemd to start it.
+// This can happen even when the socket unit is installed because
+// presets may not enable it.
+fn check_and_start_daemon_socket() -> Result<bool> {
+    if Utf8Path::new(CLIENT_SOCKET_PATH).exists() {
+        return Ok(true);
     }
-    // Unfortunately, RHEL8 systemd will count "systemctl start"
-    // invocations against the restart limit, so query the status
-    // first.
-    let activeres = Command::new("systemctl")
-        .args(&["is-active", "rpm-ostreed"])
+    let socket_unit = "rpm-ostreed.socket";
+    tracing::debug!("{CLIENT_SOCKET_PATH} does not exist, explicitly starting socket unit");
+    let start_result = Command::new("systemctl")
+        .args(&["--no-ask-password", "start", socket_unit])
         .output()?;
-    // Explicitly don't check the error return value, we don't want to
-    // hard fail on it.
-    if String::from_utf8_lossy(&activeres.stdout).starts_with("active") {
-        // It's active, we're done.  Note that while this is a race
-        // condition, that's fine because it will be handled by DBus
-        // activation.
+    if !start_result.status.success() {
+        let err = String::from_utf8_lossy(&start_result.stderr);
+        tracing::warn!("Failed to start {socket_unit}: {err}");
+        return Ok(false);
+    }
+    Ok(true)
+}
+
+/// Connect to the client socket and ensure the daemon is initialized;
+/// this avoids DBus and ensures that we get any early startup errors
+/// returned cleanly.
+#[context("Starting daemon via socket")]
+fn start_daemon_via_socket() -> Result<()> {
+    let capable = check_and_start_daemon_socket()?;
+    if !capable {
+        tracing::debug!("Falling back to DBus activation");
         return Ok(());
     }
-    let res = Command::new("systemctl")
-        .args(&["--no-ask-password", "start", service])
-        .status()?;
-    if !res.success() {
-        let _ = Command::new("systemctl")
-            .args(&["--no-pager", "status", service])
-            .status();
-        return Err(anyhow!("{}", res).into());
+
+    tracing::debug!("Starting daemon via {CLIENT_SOCKET_PATH}");
+    let mut socket = UnixStream::connect(CLIENT_SOCKET_PATH)?;
+    crate::daemon::write_message(
+        &mut socket,
+        crate::daemon::SocketMessage::ClientHello {
+            selfid: crate::core::self_id()?,
+        },
+    )
+    .context("Writing ClientHello")?;
+    let resp = crate::daemon::recv_message(&mut socket)?;
+    match resp {
+        crate::daemon::SocketMessage::ServerOk => Ok(()),
+        crate::daemon::SocketMessage::ServerError { msg } => {
+            Err(anyhow!("server error: {msg}").into())
+        }
+        o => Err(anyhow!("unexpected message: {o:?}").into()),
     }
-    Ok(())
+}
+
+pub(crate) fn client_start_daemon() -> CxxResult<()> {
+    // systemctl and socket paths only work for root right now; in the future
+    // the socket may be opened up.
+    if rustix::process::getuid().as_raw() != 0 {
+        return Ok(());
+    }
+    return start_daemon_via_socket().map_err(Into::into);
 }
 
 /// Convert the GVariant parameters from the DownloadProgress DBus API to a human-readable English string.

rust/src/core.rs

@@ -350,6 +350,14 @@ fn stage_container_rpm_files(rpms: Vec<File>) -> CxxResult<Vec<String>> {
     Ok(r)
 }
 
+/// Return an opaque identifier for the current executing binary.  This can
+/// be passed via IPC to verify that client and server are running the same code.
+pub(crate) fn self_id() -> Result<String> {
+    use std::os::unix::fs::MetadataExt;
+    let metadata = std::fs::metadata("/proc/self/exe").context("Failed to read /proc/self/exe")?;
+    Ok(format!("dev={};inode={}", metadata.dev(), metadata.ino()))
+}
+
 #[cfg(test)]
 mod test {
     use super::*;