Use non-templated systemd service

The previous setup was broken

This commit also adds some code to check that the database connection is
valid before it starts, as well as refactors the code that splits
between starting and external or internal server.

nix/module.nix

@@ -38,7 +38,7 @@ in
             socket_path = lib.mkOption {
               type = lib.types.path;
               default = "/run/mysqladm/mysqladm.sock";
-              description = "Path to the MySQL socket";
+              description = "Path to the mysqladm socket";
             };
           };
 
@@ -96,15 +96,20 @@ in
       }
     ];
 
-    systemd.services."mysqladm@" = {
+    systemd.services."mysqladm" = {
       description = "MySQL administration tool for non-admin users";
       restartTriggers = [ config.environment.etc."mysqladm/config.toml".source ];
+      requires = [ "mysqladm.socket" ];
       serviceConfig = {
         Type = "notify";
         ExecStart = "${lib.getExe cfg.package} ${cfg.logLevel} server --systemd socket-activate";
 
         WatchdogSec = 15;
 
+        # Although this is a multi-instance unit, the constant `User` field is needed
+        # for authentication via mysql's auth_socket plugin to work.
+        User = "mysqladm";
+        Group = "mysqladm";
         DynamicUser = true;
 
         ConfigurationDirectory = "mysqladm";
@@ -117,8 +122,12 @@ in
         PrivateNetwork = false;
         PrivateIPC = false;
 
-        IPAddressDeny =
+        IPAddressDeny = "any";
-          lib.optionals (lib.elem cfg.settings.mysql.host [ null "localhost" "127.0.0.1" ]) [ "any" ];
+        IPAddressAllow = [
+          "127.0.0.0/8"
+        ] ++ lib.optionals (cfg.settings.mysql.host != null) [
+          cfg.settings.mysql.host
+        ];
 
         RestrictAddressFamilies = [ "AF_UNIX" ]
           ++ (lib.optionals (cfg.settings.mysql.host != null) [ "AF_INET" "AF_INET6" ]);
@@ -162,7 +171,7 @@ in
       wantedBy = [ "sockets.target" ];
       socketConfig = {
         ListenStream = cfg.settings.server.socket_path;
-        Accept = "yes";
+        Accept = "no";
         PassCredentials = true;
       };
     };

src/cli/mysql_admutils_compatibility/mysql_dbadm.rs

@@ -151,8 +151,11 @@ pub fn main() -> anyhow::Result<()> {
         return Ok(());
     }
 
-    let server_connection =
+    let server_connection = bootstrap_server_connection_and_drop_privileges(
-        bootstrap_server_connection_and_drop_privileges(args.server_socket_path, args.config)?;
+        args.server_socket_path,
+        args.config,
+        Default::default(),
+    )?;
 
     let command = match args.command {
         Some(command) => command,

src/cli/mysql_admutils_compatibility/mysql_useradm.rs

@@ -124,8 +124,11 @@ pub fn main() -> anyhow::Result<()> {
         }
     };
 
-    let server_connection =
+    let server_connection = bootstrap_server_connection_and_drop_privileges(
-        bootstrap_server_connection_and_drop_privileges(args.server_socket_path, args.config)?;
+        args.server_socket_path,
+        args.config,
+        Default::default(),
+    )?;
 
     tokio_run_command(command, server_connection)?;
 

src/core/bootstrap.rs

@@ -1,33 +1,43 @@
 use std::{fs, path::PathBuf};
 
 use anyhow::Context;
+use clap_verbosity_flag::Verbosity;
 use nix::libc::{EXIT_SUCCESS, exit};
 use std::os::unix::net::UnixStream as StdUnixStream;
 use tokio::net::UnixStream as TokioUnixStream;
 
 use crate::{
-    core::common::{DEFAULT_CONFIG_PATH, DEFAULT_SOCKET_PATH, UnixUser},
+    core::common::{
+        DEFAULT_CONFIG_PATH, DEFAULT_SOCKET_PATH, UnixUser, executable_is_suid_or_sgid,
+    },
     server::{config::read_config_from_path, server_loop::handle_requests_for_single_session},
 };
 
-// TODO: this function is security critical, it should be integration tested
+/// Determine whether we will make a connection to an external server
-//       in isolation.
+/// or start an internal server with elevated privileges.
-/// Drop privileges to the real user and group of the process.
+///
-/// If the process is not running with elevated privileges, this function
+/// If neither is feasible, an error is returned.
-/// is a no-op.
+fn will_connect_to_external_server(
-pub fn drop_privs() -> anyhow::Result<()> {
+    server_socket_path: Option<&PathBuf>,
-    log::debug!("Dropping privileges");
+    config_path: Option<&PathBuf>,
-    let real_uid = nix::unistd::getuid();
+) -> anyhow::Result<bool> {
-    let real_gid = nix::unistd::getgid();
+    if server_socket_path.is_some() {
+        return Ok(true);
+    }
 
-    nix::unistd::setuid(real_uid).context("Failed to drop privileges")?;
+    if config_path.is_some() {
-    nix::unistd::setgid(real_gid).context("Failed to drop privileges")?;
+        return Ok(false);
+    }
 
-    debug_assert_eq!(nix::unistd::getuid(), real_uid);
+    if fs::metadata(DEFAULT_SOCKET_PATH).is_ok() {
-    debug_assert_eq!(nix::unistd::getgid(), real_gid);
+        return Ok(true);
+    }
 
-    log::debug!("Privileges dropped successfully");
+    if fs::metadata(DEFAULT_CONFIG_PATH).is_ok() {
-    Ok(())
+        return Ok(false);
+    }
+
+    anyhow::bail!("No socket path or config path provided, and no default socket or config found");
 }
 
 /// This function is used to bootstrap the connection to the server.
@@ -45,31 +55,44 @@ pub fn drop_privs() -> anyhow::Result<()> {
 ///    with the socket for the server.
 ///
 /// If neither of these options are available, the function will fail.
+///
+/// Note that this function is also responsible for setting up logging,
+/// because in the case of an internal server, we need to drop privileges
+/// before we can initialize logging.
 pub fn bootstrap_server_connection_and_drop_privileges(
     server_socket_path: Option<PathBuf>,
-    config_path: Option<PathBuf>,
+    config: Option<PathBuf>,
+    verbose: Verbosity,
 ) -> anyhow::Result<StdUnixStream> {
-    if server_socket_path.is_some() && config_path.is_some() {
+    if will_connect_to_external_server(server_socket_path.as_ref(), config.as_ref())? {
-        anyhow::bail!("Cannot provide both a socket path and a config path");
+        assert!(
+            !executable_is_suid_or_sgid()?,
+            "The executable should not be SUID or SGID when connecting to an external server"
+        );
+
+        env_logger::Builder::new()
+            .filter_level(verbose.log_level_filter())
+            .init();
+
+        connect_to_external_server(server_socket_path)
+    } else {
+        // NOTE: We need to be really careful with the code up until this point,
+        //       as we might be running with elevated privileges.
+        let server_connection = bootstrap_internal_server_and_drop_privs(config)?;
+
+        env_logger::Builder::new()
+            .filter_level(verbose.log_level_filter())
+            .init();
+
+        Ok(server_connection)
     }
-
-    log::debug!("Starting the server connection bootstrap process");
-
-    let socket = bootstrap_server_connection(server_socket_path, config_path)?;
-
-    drop_privs()?;
-
-    Ok(socket)
 }
 
-/// Inner function for [`bootstrap_server_connection_and_drop_privileges`].
+fn connect_to_external_server(
-/// See that function for more information.
+    server_socket_path: Option<PathBuf>,
-fn bootstrap_server_connection(
-    socket_path: Option<PathBuf>,
-    config_path: Option<PathBuf>,
 ) -> anyhow::Result<StdUnixStream> {
     // TODO: ensure this is both readable and writable
-    if let Some(socket_path) = socket_path {
+    if let Some(socket_path) = server_socket_path {
         log::debug!("Connecting to socket at {:?}", socket_path);
         return match StdUnixStream::connect(socket_path) {
             Ok(socket) => Ok(socket),
@@ -80,15 +103,6 @@ fn bootstrap_server_connection(
             },
         };
     }
-    if let Some(config_path) = config_path {
-        // ensure config exists and is readable
-        if fs::metadata(&config_path).is_err() {
-            return Err(anyhow::anyhow!("Config file not found or not readable"));
-        }
-
-        log::debug!("Starting server with config at {:?}", config_path);
-        return invoke_server_with_config(config_path);
-    }
 
     if fs::metadata(DEFAULT_SOCKET_PATH).is_ok() {
         log::debug!("Connecting to default socket at {:?}", DEFAULT_SOCKET_PATH);
@@ -102,13 +116,60 @@ fn bootstrap_server_connection(
         };
     }
 
+    anyhow::bail!("No socket path provided, and no default socket found");
+}
+
+// TODO: this function is security critical, it should be integration tested
+//       in isolation.
+/// Drop privileges to the real user and group of the process.
+/// If the process is not running with elevated privileges, this function
+/// is a no-op.
+fn drop_privs() -> anyhow::Result<()> {
+    log::debug!("Dropping privileges");
+    let real_uid = nix::unistd::getuid();
+    let real_gid = nix::unistd::getgid();
+
+    nix::unistd::setuid(real_uid).context("Failed to drop privileges")?;
+    nix::unistd::setgid(real_gid).context("Failed to drop privileges")?;
+
+    debug_assert_eq!(nix::unistd::getuid(), real_uid);
+    debug_assert_eq!(nix::unistd::getgid(), real_gid);
+
+    log::debug!("Privileges dropped successfully");
+    Ok(())
+}
+
+fn bootstrap_internal_server_and_drop_privs(
+    config_path: Option<PathBuf>,
+) -> anyhow::Result<StdUnixStream> {
+    if let Some(config_path) = config_path {
+        if !executable_is_suid_or_sgid()? {
+            anyhow::bail!("Executable is not SUID/SGID - refusing to start internal sever");
+        }
+
+        // ensure config exists and is readable
+        if fs::metadata(&config_path).is_err() {
+            return Err(anyhow::anyhow!("Config file not found or not readable"));
+        }
+
+        log::debug!("Starting server with config at {:?}", config_path);
+        let socket = invoke_server_with_config(config_path)?;
+        drop_privs()?;
+        return Ok(socket);
+    };
+
     let config_path = PathBuf::from(DEFAULT_CONFIG_PATH);
     if fs::metadata(&config_path).is_ok() {
+        if !executable_is_suid_or_sgid()? {
+            anyhow::bail!("Executable is not SUID/SGID - refusing to start internal sever");
+        }
         log::debug!("Starting server with default config at {:?}", config_path);
-        return invoke_server_with_config(config_path);
+        let socket = invoke_server_with_config(config_path)?;
-    }
+        drop_privs()?;
+        return Ok(socket);
+    };
 
-    anyhow::bail!("No socket path or config path provided, and no default socket or config found");
+    anyhow::bail!("No config path provided, and no default config found");
 }
 
 // TODO: we should somehow ensure that the forked process is killed on completion,

src/core/common.rs

@@ -1,5 +1,6 @@
 use anyhow::Context;
 use nix::unistd::{Group as LibcGroup, User as LibcUser};
+use std::{fs, os::unix::fs::PermissionsExt};
 
 #[cfg(not(target_os = "macos"))]
 use std::ffi::CString;

src/main.rs

@@ -124,14 +124,13 @@ fn main() -> anyhow::Result<()> {
         return Ok(());
     }
 
-    let server_connection =
+    let connection = bootstrap_server_connection_and_drop_privileges(
-        bootstrap_server_connection_and_drop_privileges(args.server_socket_path, args.config)?;
+        args.server_socket_path,
+        args.config,
+        args.verbose,
+    )?;
 
-    env_logger::Builder::new()
+    tokio_run_command(args.command, connection)?;
-        .filter_level(args.verbose.log_level_filter())
-        .init();
-
-    tokio_run_command(args.command, server_connection)?;
 
     Ok(())
 }

src/server/command.rs

@@ -1,23 +1,16 @@
-use std::os::fd::FromRawFd;
 use std::path::PathBuf;
 
 use anyhow::Context;
 use clap::Parser;
 use clap_verbosity_flag::Verbosity;
-use futures::SinkExt;
-use indoc::concatdoc;
 use systemd_journal_logger::JournalLog;
 
-use std::os::unix::net::UnixStream as StdUnixStream;
-use tokio::net::UnixStream as TokioUnixStream;
-
-use crate::core::common::UnixUser;
-use crate::core::protocol::{Response, create_server_to_client_message_stream};
-use crate::server::config::read_config_from_path_with_arg_overrides;
-use crate::server::server_loop::listen_for_incoming_connections;
 use crate::server::{
-    config::{ServerConfig, ServerConfigArgs},
+    config::{ServerConfigArgs, read_config_from_path_with_arg_overrides},
-    server_loop::handle_requests_for_single_session,
+    server_loop::{
+        listen_for_incoming_connections_with_socket_path,
+        listen_for_incoming_connections_with_systemd_socket,
+    },
 };
 
 #[derive(Parser, Debug, Clone)]
@@ -97,7 +90,9 @@ pub async fn handle_command(
     let config = read_config_from_path_with_arg_overrides(config_path, args.config_overrides)?;
 
     match args.subcmd {
-        ServerCommand::Listen => listen_for_incoming_connections(socket_path, config).await,
+        ServerCommand::Listen => {
+            listen_for_incoming_connections_with_socket_path(socket_path, config).await
+        }
         ServerCommand::SocketActivate => {
             if !args.systemd {
                 anyhow::bail!(concat!(
@@ -106,7 +101,7 @@ pub async fn handle_command(
                 ));
             }
 
-            socket_activate(config).await
+            listen_for_incoming_connections_with_systemd_socket(config).await
         }
     }
 }
@@ -136,73 +131,3 @@ fn start_watchdog_thread_if_enabled() {
         log::debug!("Systemd watchdog not enabled, skipping watchdog thread");
     }
 }
-
-async fn socket_activate(config: ServerConfig) -> anyhow::Result<()> {
-    let conn = get_socket_from_systemd().await?;
-
-    let uid = match conn.peer_cred() {
-        Ok(cred) => cred.uid(),
-        Err(e) => {
-            log::error!("Failed to get peer credentials from socket: {}", e);
-            let mut message_stream = create_server_to_client_message_stream(conn);
-            message_stream
-                .send(Response::Error(
-                    (concatdoc! {
-                        "Server failed to get peer credentials from socket\n",
-                        "Please check the server logs or contact the system administrators"
-                    })
-                    .to_string(),
-                ))
-                .await
-                .ok();
-            anyhow::bail!("Failed to get peer credentials from socket");
-        }
-    };
-
-    log::debug!("Accepted connection from uid {}", uid);
-
-    let unix_user = match UnixUser::from_uid(uid) {
-        Ok(user) => user,
-        Err(e) => {
-            log::error!("Failed to get username from uid: {}", e);
-            let mut message_stream = create_server_to_client_message_stream(conn);
-            message_stream
-                .send(Response::Error(
-                    (concatdoc! {
-                        "Server failed to get user data from the system\n",
-                        "Please check the server logs or contact the system administrators"
-                    })
-                    .to_string(),
-                ))
-                .await
-                .ok();
-            anyhow::bail!("Failed to get username from uid");
-        }
-    };
-
-    log::info!("Accepted connection from {}", unix_user.username);
-
-    sd_notify::notify(false, &[sd_notify::NotifyState::Ready]).ok();
-
-    handle_requests_for_single_session(conn, &unix_user, &config).await?;
-
-    Ok(())
-}
-
-async fn get_socket_from_systemd() -> anyhow::Result<TokioUnixStream> {
-    let fd = sd_notify::listen_fds()
-        .context("Failed to get file descriptors from systemd")?
-        .next()
-        .context("No file descriptors received from systemd")?;
-
-    debug_assert!(fd == 3, "Unexpected file descriptor from systemd: {}", fd);
-
-    log::debug!(
-        "Received file descriptor from systemd with id: '{}', assuming socket",
-        fd
-    );
-
-    let std_unix_stream = unsafe { StdUnixStream::from_raw_fd(fd) };
-    let socket = TokioUnixStream::from_std(std_unix_stream)?;
-    Ok(socket)
-}

src/server/server_loop.rs

@@ -1,8 +1,14 @@
-use std::{collections::BTreeSet, fs, path::PathBuf};
+use std::{
+    collections::BTreeSet,
+    fs,
+    os::unix::{io::FromRawFd, net::UnixListener as StdUnixListener},
+    path::PathBuf,
+};
 
+use anyhow::Context;
 use futures_util::{SinkExt, StreamExt};
 use indoc::concatdoc;
-use tokio::net::{UnixListener, UnixStream};
+use tokio::net::{UnixListener as TokioUnixListener, UnixStream as TokioUnixStream};
 
 use sqlx::MySqlConnection;
 use sqlx::prelude::*;
@@ -34,10 +40,9 @@ use crate::{
 
 // TODO: consider using a connection pool
 
-pub async fn listen_for_incoming_connections(
+pub async fn listen_for_incoming_connections_with_socket_path(
     socket_path: Option<PathBuf>,
     config: ServerConfig,
-    // db_connection: &mut MySqlConnection,
 ) -> anyhow::Result<()> {
     let socket_path = socket_path.unwrap_or(PathBuf::from(DEFAULT_SOCKET_PATH));
 
@@ -55,8 +60,35 @@ pub async fn listen_for_incoming_connections(
         Err(e) => return Err(e.into()),
     }
 
-    let listener = UnixListener::bind(socket_path)?;
+    let listener = TokioUnixListener::bind(socket_path)?;
 
+    listen_for_incoming_connections_with_listener(listener, config).await
+}
+
+pub async fn listen_for_incoming_connections_with_systemd_socket(
+    config: ServerConfig,
+) -> anyhow::Result<()> {
+    let fd = sd_notify::listen_fds()
+        .context("Failed to get file descriptors from systemd")?
+        .next()
+        .context("No file descriptors received from systemd")?;
+
+    debug_assert!(fd == 3, "Unexpected file descriptor from systemd: {}", fd);
+
+    log::debug!(
+        "Received file descriptor from systemd with id: '{}', assuming socket",
+        fd
+    );
+
+    let std_unix_listener = unsafe { StdUnixListener::from_raw_fd(fd) };
+    let listener = TokioUnixListener::from_std(std_unix_listener)?;
+    listen_for_incoming_connections_with_listener(listener, config).await
+}
+
+pub async fn listen_for_incoming_connections_with_listener(
+    listener: TokioUnixListener,
+    config: ServerConfig,
+) -> anyhow::Result<()> {
     sd_notify::notify(false, &[sd_notify::NotifyState::Ready]).ok();
 
     while let Ok((conn, _addr)) = listener.accept().await {
@@ -113,12 +145,23 @@ pub async fn listen_for_incoming_connections(
     Ok(())
 }
 
+async fn close_or_ignore_db_connection(db_connection: MySqlConnection) {
+    if let Err(e) = db_connection.close().await {
+        log::error!("Failed to close database connection: {}", e);
+        log::error!("{}", e);
+        log::error!("Ignoring...");
+    }
+}
+
 pub async fn handle_requests_for_single_session(
-    socket: UnixStream,
+    socket: TokioUnixStream,
     unix_user: &UnixUser,
     config: &ServerConfig,
 ) -> anyhow::Result<()> {
     let mut message_stream = create_server_to_client_message_stream(socket);
+
+    log::debug!("Opening connection to database");
+
     let mut db_connection = match create_mysql_connection_from_config(&config.mysql).await {
         Ok(connection) => connection,
         Err(err) => {
@@ -136,6 +179,24 @@ pub async fn handle_requests_for_single_session(
         }
     };
 
+    log::debug!("Verifying that database connection is valid");
+
+    if let Err(e) = db_connection.ping().await {
+        log::error!("Failed to ping database: {}", e);
+        message_stream
+            .send(Response::Error(
+                (concatdoc! {
+                    "Server failed to connect to database\n",
+                    "Please check the server logs or contact the system administrators"
+                })
+                .to_string(),
+            ))
+            .await?;
+        message_stream.flush().await?;
+        close_or_ignore_db_connection(db_connection).await;
+        return Err(e.into());
+    }
+
     log::debug!("Successfully connected to database");
 
     let result = handle_requests_for_single_session_with_db_connection(
@@ -145,11 +206,7 @@ pub async fn handle_requests_for_single_session(
     )
     .await;
 
-    if let Err(e) = db_connection.close().await {
+    close_or_ignore_db_connection(db_connection).await;
-        log::error!("Failed to close database connection: {}", e);
-        log::error!("{}", e);
-        log::error!("Ignoring...");
-    }
 
     result
 }
@@ -157,7 +214,7 @@ pub async fn handle_requests_for_single_session(
 // TODO: ensure proper db_connection hygiene for functions that invoke
 //       this function
 
-pub async fn handle_requests_for_single_session_with_db_connection(
+async fn handle_requests_for_single_session_with_db_connection(
     mut stream: ServerToClientMessageStream,
     unix_user: &UnixUser,
     db_connection: &mut MySqlConnection,