mz: don't require write access to load an existing config file (#37566)

### Motivation

`mz` fails on read-only commands (e.g. `mz sql`) when `mz.toml` sits on
a read-only mount, such as a sandbox that bind-mounts the config file
read-only. The command never intends to write anything, so this is an
unnecessary failure.

### Description

`ConfigFile::load` always opened `mz.toml` with
`write(true).create(true)`, even though every mutation (`add_profile`,
`remove_profile`, `set_param`, `set_profile_param`) writes the file
independently afterward via `fs::write`. The write-mode open at load
time was only ever needed to create the file when missing.

`load` no longer writes to the file system at all. A missing file loads
as an empty configuration, and the first mutation creates whatever is
missing. Since `fs::write` creates the file but not its parent
directory, all mutations now go through a private `ConfigFile::write`
helper that calls `create_dir_all` first, taking over the directory
creation that `load` used to perform. Read-only commands therefore work
both when `mz.toml` sits on a read-only mount and when it does not exist
yet.

Dropping the write-mode open changes when an unwritable configuration is
reported. `mz profile init` creates a remote app password, and on macOS
a keychain entry, before `add_profile` reaches its write. Failing only
at that write would leave a credential behind that no profile records,
so `ConfigFile::ensure_writable` establishes write access and `init`
calls it before logging in. The check creates the parent directory and
opens the file with `create(true)`, because whether a missing file can
be written depends on the parent hierarchy and no probe settles that as
reliably as performing the creation the later write needs anyway.

Unit tests cover loading a missing file, loading a read-only file, a
missing file inside a read-only directory, and the parent directory
creation that moved out of `load`. The permission-dependent assertions
skip themselves when the file system does not enforce the permission
bits for the process, as is the case under root.

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>

Author: antiguru

Cargo.lock

@@ -44,6 +44,7 @@ url.workspace = true
 
 [dev-dependencies]
 assert_cmd.workspace = true
+tempfile.workspace = true
 
 [target.'cfg(target_os = "macos")'.dependencies]
 security-framework = "3.7.0"

src/mz/Cargo.toml

@@ -170,6 +170,10 @@ pub async fn init(
         }
     }
 
+    // Fail before logging in, so that an unwritable configuration file doesn't
+    // leave behind an app password that no profile records.
+    config_file.ensure_writable().await?;
+
     let app_password = match no_browser {
         true => init_without_browser(admin_endpoint.clone()).await?,
         false => init_with_browser(cloud_endpoint.clone()).await?,

src/mz/src/command/profile.rs

@@ -15,8 +15,6 @@
 
 //! Configuration file management.
 
-use std::fs::OpenOptions;
-use std::io::Read;
 use std::path::PathBuf;
 use std::sync::LazyLock;
 use std::{collections::BTreeMap, str::FromStr};
@@ -84,24 +82,17 @@ impl ConfigFile {
     }
 
     /// Loads a configuration file from the specified path.
+    ///
+    /// A missing file loads as an empty configuration. Loading never creates
+    /// the file or its parent directory, and never requires write access, so
+    /// commands that only read the configuration work when `mz.toml` lives on
+    /// a read-only mount. The first mutation creates whatever is missing.
     pub async fn load(path: PathBuf) -> Result<ConfigFile, Error> {
-        // Create the parent directory if it doesn't exist
-        if let Some(parent) = path.parent() {
-            if !parent.exists() {
-                fs::create_dir_all(parent).await?;
-            }
-        }
-
-        // Create the file if it doesn't exist
-        let mut file = OpenOptions::new()
-            .read(true)
-            .write(true)
-            .create(true)
-            .truncate(false)
-            .open(&path)?;
-
-        let mut buffer = String::new();
-        file.read_to_string(&mut buffer)?;
+        let buffer = match fs::read_to_string(&path).await {
+            Ok(buffer) => buffer,
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound => String::new(),
+            Err(e) => return Err(e.into()),
+        };
 
         let parsed = toml_edit::de::from_str(&buffer)?;
         let editable = buffer.parse()?;
@@ -113,6 +104,50 @@ impl ConfigFile {
         })
     }
 
+    /// Writes `contents` to the configuration file, creating the parent
+    /// directory if it doesn't exist.
+    async fn write(&self, contents: String) -> Result<(), Error> {
+        if let Some(parent) = self.path.parent() {
+            fs::create_dir_all(parent).await?;
+        }
+        fs::write(&self.path, contents).await?;
+
+        Ok(())
+    }
+
+    /// Errors unless the configuration file can be written, creating the file
+    /// and its parent directory if they are missing.
+    ///
+    /// Commands that cause external side effects before mutating the
+    /// configuration, such as creating an app password or writing to the
+    /// keychain, must call this beforehand. Otherwise an unwritable
+    /// configuration file fails the command only after those side effects have
+    /// happened, leaving them unrecorded.
+    ///
+    /// Creating what is missing is what makes the check conclusive: whether a
+    /// missing file can be written depends on the parent hierarchy, which no
+    /// amount of probing establishes as reliably as performing the creation
+    /// that the later write needs anyway.
+    pub async fn ensure_writable(&self) -> Result<(), Error> {
+        let result = async {
+            if let Some(parent) = self.path.parent() {
+                fs::create_dir_all(parent).await?;
+            }
+            fs::OpenOptions::new()
+                .write(true)
+                .create(true)
+                .truncate(false)
+                .open(&self.path)
+                .await?;
+
+            Ok::<(), std::io::Error>(())
+        };
+
+        result
+            .await
+            .map_err(|e| Error::ConfigFileNotWritable(self.path.clone(), e))
+    }
+
     /// Loads a profile from the configuration file.
     /// Panics if the profile is not found.
     pub fn load_profile<'a>(&'a self, name: &'a str) -> Result<Profile<'a>, Error> {
@@ -161,7 +196,7 @@ impl ConfigFile {
         editable["profile"] = editable.entry("profile").or_insert(value(name)).clone();
 
         // TODO: I don't know why it creates an empty [profiles] table
-        fs::write(&self.path, editable.to_string()).await?;
+        self.write(editable.to_string()).await?;
 
         Ok(())
     }
@@ -209,7 +244,7 @@ impl ConfigFile {
             .ok_or(Error::ProfilesMissing)?;
         profiles.remove(name);
 
-        fs::write(&self.path, editable.to_string()).await?;
+        self.write(editable.to_string()).await?;
 
         Ok(())
     }
@@ -290,7 +325,7 @@ impl ConfigFile {
             Some(value) => editable["profiles"][profile_name][name] = toml_edit::value(value),
         }
 
-        fs::write(&self.path, editable.to_string()).await?;
+        self.write(editable.to_string()).await?;
 
         Ok(())
     }
@@ -324,7 +359,7 @@ impl ConfigFile {
             }
             Some(value) => editable[name] = toml_edit::value(value),
         }
-        fs::write(&self.path, editable.to_string()).await?;
+        self.write(editable.to_string()).await?;
         Ok(())
     }
 }
@@ -530,3 +565,107 @@ pub struct TomlProfile {
     /// A custom cloud endpoint used for development.
     pub cloud_endpoint: Option<String>,
 }
+
+#[cfg(test)]
+mod tests {
+    use tempfile::TempDir;
+
+    use super::*;
+
+    /// Returns whether the file system enforces `path`'s permission bits for
+    /// this process, which is not the case when running as root.
+    fn permissions_are_enforced(path: &PathBuf) -> bool {
+        std::fs::OpenOptions::new().write(true).open(path).is_err()
+    }
+
+    /// Makes `path` read-only and returns whether the file system enforces
+    /// that for this process, which is not the case when running as root.
+    fn make_read_only(path: &PathBuf) -> bool {
+        let mut permissions = std::fs::metadata(path).unwrap().permissions();
+        permissions.set_readonly(true);
+        std::fs::set_permissions(path, permissions).unwrap();
+
+        if path.is_dir() {
+            let probe = path.join("probe");
+            let enforced = std::fs::write(&probe, "").is_err();
+            let _ = std::fs::remove_file(probe);
+            enforced
+        } else {
+            permissions_are_enforced(path)
+        }
+    }
+
+    /// Restores write access so that the temporary directory can be cleaned up.
+    fn make_writable(path: &PathBuf) {
+        let mut permissions = std::fs::metadata(path).unwrap().permissions();
+        #[allow(clippy::permissions_set_readonly_false)]
+        permissions.set_readonly(false);
+        std::fs::set_permissions(path, permissions).unwrap();
+    }
+
+    #[mz_ore::test(tokio::test)]
+    #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir`
+    async fn test_load_missing_file() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("missing").join("mz.toml");
+
+        let config = ConfigFile::load(path.clone()).await.unwrap();
+
+        assert!(config.profiles().is_none());
+        // Loading must not create anything on disk.
+        assert!(!path.exists());
+        assert!(!path.parent().unwrap().exists());
+    }
+
+    #[mz_ore::test(tokio::test)]
+    #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir`
+    async fn test_load_read_only_file() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("mz.toml");
+        std::fs::write(&path, "profile = \"default\"\n").unwrap();
+        let enforced = make_read_only(&path);
+
+        let config = ConfigFile::load(path.clone()).await.unwrap();
+
+        assert_eq!(config.profile(), "default");
+        if enforced {
+            assert!(config.ensure_writable().await.is_err());
+        }
+
+        make_writable(&path);
+    }
+
+    #[mz_ore::test(tokio::test)]
+    #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir`
+    async fn test_missing_file_in_read_only_directory() {
+        let dir = TempDir::new().unwrap();
+        let parent = dir.path().join("materialize");
+        std::fs::create_dir(&parent).unwrap();
+        let enforced = make_read_only(&parent);
+
+        let config = ConfigFile::load(parent.join("mz.toml")).await.unwrap();
+
+        // A missing file is only writable if its parent hierarchy accepts it.
+        if enforced {
+            assert!(config.ensure_writable().await.is_err());
+        }
+
+        make_writable(&parent);
+    }
+
+    #[mz_ore::test(tokio::test)]
+    #[cfg_attr(miri, ignore)] // unsupported operation: can't call foreign function `mkdir`
+    async fn test_write_creates_parent_directory() {
+        let dir = TempDir::new().unwrap();
+        let path = dir.path().join("materialize").join("mz.toml");
+
+        let config = ConfigFile::load(path.clone()).await.unwrap();
+        config.ensure_writable().await.unwrap();
+        // The writability check creates what the later write needs.
+        assert!(path.exists());
+
+        config.set_param("profile", Some("default")).await.unwrap();
+
+        assert_eq!(ConfigFile::load(path).await.unwrap().profile(), "default");
+    }
+}

src/mz/src/config_file.rs

@@ -16,6 +16,8 @@
 //! [`Error`](`enum@Error`) is a custom error type containing multiple variants
 //! for erros produced by the self crate, internal crates and external crates.
 
+use std::path::PathBuf;
+
 use hyper::header::{InvalidHeaderValue, ToStrError};
 use thiserror::Error;
 use url::ParseError;
@@ -88,6 +90,9 @@ pub enum Error {
     /// I/O Error
     #[error(transparent)]
     IOError(#[from] std::io::Error),
+    /// Error raised when the configuration file exists but cannot be written.
+    #[error("Error: The configuration file {0} is not writable: {1}")]
+    ConfigFileNotWritable(PathBuf, #[source] std::io::Error),
     /// I/O Error
     #[error(transparent)]
     CSVParseError(#[from] csv::Error),