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"
 cap-tempfile = "1.0.15"
 chrono = { version = "0.4.26", features = ["serde"] }

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,7 +6,7 @@ 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 fn_error_context::context;
 use gio::prelude::*;
 use ostree_ext::{gio, glib};
@@ -49,7 +49,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 +157,70 @@ 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.
+/// 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<()> {
+    use cap_std::io_lifetimes::IntoSocketlike;
+
+    futures::executor::block_on(async {
+        let c = tokio::net::UnixStream::connect("/run/rpm-ostree/client.sock").await?;
+        Ok::<_, anyhow::Error>(c)
+    })
+    .context("Connecting")?;
+
+    let address = sockaddr()?;
+    let socket = rustix::net::socket(
+        rustix::net::AddressFamily::UNIX,
+        rustix::net::SocketType::STREAM,
+        rustix::net::Protocol::from_raw(0),
+    )?;
+    let addr = crate::client::sockaddr()?;
+    tracing::debug!("Starting daemon via {address:?}");
+    rustix::net::connect_unix(&socket, &addr)
+        .with_context(|| anyhow!("Failed to connect to {address:?}"))?;
+    let socket = socket.into_socketlike();
+    crate::daemon::write_message(
+        &socket,
+        crate::daemon::SocketMessage::ClientHello {
+            selfid: crate::core::self_id()?,
+        },
+    )
+    .context("Writing ClientHello")?;
+    rustix::net::shutdown(&socket, rustix::net::Shutdown::Write).expect("shutdown");
+    let resp = crate::daemon::recv_message(&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()),
+    }
+}
+
+/// Returns the address of the client socket.
+pub(crate) fn sockaddr() -> Result<rustix::net::SocketAddrUnix> {
+    rustix::net::SocketAddrUnix::new("/run/rpm-ostree/client.sock").map_err(anyhow::Error::msg)
+}
+
 pub(crate) fn client_start_daemon() -> CxxResult<()> {
-    let service = "rpm-ostreed.service";
-    // Assume non-root can't use systemd right now.
+    // 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(());
     }
-    // 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"])
-        .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.
-        return Ok(());
-    }
+    let socket = "rpm-ostreed.socket";
     let res = Command::new("systemctl")
-        .args(&["--no-ask-password", "start", service])
+        .args(&["--no-ask-password", "start", socket])
         .status()?;
     if !res.success() {
         let _ = Command::new("systemctl")
-            .args(&["--no-pager", "status", service])
+            .args(&["--no-pager", "status", socket])
             .status();
         return Err(anyhow!("{}", res).into());
     }
-    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::*;