Use a UUID instead of volume name for fstab on Mac (#215)

* Use a UUID instead of volume name for fstab on Mac

* reflect review

* Don't quote the UUID in fstab

Cargo.lock

@@ -918,6 +918,7 @@ dependencies = [
  "tracing-subscriber",
  "typetag",
  "url",
+ "uuid",
  "xz2",
 ]
 
@@ -1862,6 +1863,12 @@ dependencies = [
  "serde",
 ]
 
+[[package]]
+name = "uuid"
+version = "1.2.2"
+source = "registry+https://github.com/rust-lang/crates.io-index"
+checksum = "422ee0de9031b5b948b97a8fc04e3aa35230001a722ddd27943e0be31564ce4c"
+
 [[package]]
 name = "valuable"
 version = "0.1.0"

Cargo.toml

@@ -52,6 +52,7 @@ dyn-clone = { version = "1.0.9", default-features = false }
 rand = { version = "0.8.5", default-features = false, features = [ "std", "std_rng" ] }
 semver = { version = "1.0.14", default-features = false, features = ["serde", "std"] }
 term = { version = "0.7.0", default-features = false }
+uuid = "1.2.2"
 
 [dev-dependencies]
 eyre = { version = "0.6.8", default-features = false, features = [ "track-caller" ] }

src/action/base/create_or_insert_into_file.rs

@@ -185,10 +185,19 @@ impl Action for CreateOrInsertIntoFile {
             tokio::fs::set_permissions(&temp_file_path, PermissionsExt::from_mode(*mode))
                 .await
                 .map_err(|e| ActionError::SetPermissions(*mode, path.to_owned(), e))?;
-        } else if orig_file.is_some() {
+        } else if let Some(original_file) = orig_file {
-            tokio::fs::set_permissions(&temp_file_path, PermissionsExt::from_mode(0o644))
+            let original_file_mode = original_file
+                .metadata()
                 .await
-                .map_err(|e| ActionError::SetPermissions(0o644, path.to_owned(), e))?;
+                .map_err(|e| ActionError::GettingMetadata(path.to_path_buf(), e))?
+                .permissions()
+                .mode();
+            tokio::fs::set_permissions(
+                &temp_file_path,
+                PermissionsExt::from_mode(original_file_mode),
+            )
+            .await
+            .map_err(|e| ActionError::SetPermissions(original_file_mode, path.to_owned(), e))?;
         }
 
         tokio::fs::rename(&temp_file_path, &path)

src/action/darwin/create_fstab_entry.rs

@@ -0,0 +1,237 @@
+use nix::unistd::{chown, Group, User};
+use uuid::Uuid;
+
+use crate::{
+    action::{Action, ActionDescription, ActionError, StatefulAction},
+    execute_command,
+};
+use rand::Rng;
+use std::{
+    io::SeekFrom,
+    os::unix::prelude::PermissionsExt,
+    path::{Path, PathBuf},
+    str::FromStr,
+};
+use tokio::{
+    fs::{remove_file, OpenOptions},
+    io::{AsyncReadExt, AsyncSeekExt, AsyncWriteExt},
+    process::Command,
+};
+use tracing::{span, Span};
+
+const FSTAB_PATH: &str = "/etc/fstab";
+
+/** Create an `/etc/fstab` entry for the given volume
+
+
+This action queries `diskutil info` on the volume to fetch it's UUID and
+add the relevant information to `/etc/fstab`.
+ */
+// Initially, a `NAME` was used, however in https://github.com/DeterminateSystems/nix-installer/issues/212
+// several users reported issues. Using a UUID resolved the issue for them.
+#[derive(Debug, serde::Deserialize, serde::Serialize, Clone)]
+pub struct CreateFstabEntry {
+    apfs_volume_label: String,
+}
+
+impl CreateFstabEntry {
+    #[tracing::instrument(level = "debug", skip_all)]
+    pub async fn plan(apfs_volume_label: String) -> Result<StatefulAction<Self>, ActionError> {
+        let fstab_path = Path::new(FSTAB_PATH);
+        if fstab_path.exists() {
+            let fstab_buf = tokio::fs::read_to_string(&fstab_path)
+                .await
+                .map_err(|e| ActionError::Read(fstab_path.to_path_buf(), e))?;
+            let prelude_comment = fstab_prelude_comment(&apfs_volume_label);
+            if fstab_buf.contains(&prelude_comment) {
+                return Err(ActionError::Custom(Box::new(
+                    CreateFstabEntryError::EntryExists(apfs_volume_label.clone()),
+                )));
+            }
+        }
+
+        Ok(Self { apfs_volume_label }.into())
+    }
+}
+
+#[async_trait::async_trait]
+#[typetag::serde(name = "create_fstab_entry")]
+impl Action for CreateFstabEntry {
+    fn tracing_synopsis(&self) -> String {
+        format!(
+            "Add a UUID based entry for the APFS volume `{}` to `/etc/fstab`",
+            self.apfs_volume_label
+        )
+    }
+
+    fn tracing_span(&self) -> Span {
+        let span = span!(
+            tracing::Level::DEBUG,
+            "create_fstab_entry",
+            apfs_volume_label = self.apfs_volume_label,
+        );
+
+        span
+    }
+
+    fn execute_description(&self) -> Vec<ActionDescription> {
+        vec![ActionDescription::new(self.tracing_synopsis(), vec![])]
+    }
+
+    #[tracing::instrument(level = "debug", skip_all)]
+    async fn execute(&mut self) -> Result<(), ActionError> {
+        let Self { apfs_volume_label } = self;
+        let fstab_path = Path::new(FSTAB_PATH);
+        let uuid = get_uuid_for_label(&apfs_volume_label)
+            .await?
+            .ok_or_else(|| {
+                ActionError::Custom(Box::new(CreateFstabEntryError::NoVolume(
+                    apfs_volume_label.clone(),
+                )))
+            })?;
+        let fstab_entry = fstab_entry(&uuid, apfs_volume_label);
+
+        let mut fstab = tokio::fs::OpenOptions::new()
+            .create(true)
+            .write(true)
+            .read(true)
+            .open(fstab_path)
+            .await
+            .map_err(|e| ActionError::Open(fstab_path.to_path_buf(), e))?;
+
+        // Make sure it doesn't already exist before we write to it.
+        let mut fstab_buf = String::new();
+        fstab
+            .read_to_string(&mut fstab_buf)
+            .await
+            .map_err(|e| ActionError::Read(fstab_path.to_owned(), e))?;
+
+        if fstab_buf.contains(&fstab_entry) {
+            tracing::debug!("Skipped writing to `/etc/fstab` as the content already existed")
+        } else {
+            fstab
+                .write_all(fstab_entry.as_bytes())
+                .await
+                .map_err(|e| ActionError::Write(fstab_path.to_owned(), e))?;
+        }
+
+        Ok(())
+    }
+
+    fn revert_description(&self) -> Vec<ActionDescription> {
+        let Self { apfs_volume_label } = &self;
+        vec![ActionDescription::new(
+            format!(
+                "Remove the UUID based entry for the APFS volume `{}` in `/etc/fstab`",
+                apfs_volume_label
+            ),
+            vec![],
+        )]
+    }
+
+    #[tracing::instrument(level = "debug", skip_all)]
+    async fn revert(&mut self) -> Result<(), ActionError> {
+        let Self { apfs_volume_label } = self;
+        let fstab_path = Path::new(FSTAB_PATH);
+        let uuid = get_uuid_for_label(&apfs_volume_label)
+            .await?
+            .ok_or_else(|| {
+                ActionError::Custom(Box::new(CreateFstabEntryError::NoVolume(
+                    apfs_volume_label.clone(),
+                )))
+            })?;
+        let fstab_entry = fstab_entry(&uuid, apfs_volume_label);
+
+        let mut file = OpenOptions::new()
+            .create(false)
+            .write(true)
+            .read(true)
+            .open(&fstab_path)
+            .await
+            .map_err(|e| ActionError::Open(fstab_path.to_owned(), e))?;
+
+        let mut file_contents = String::default();
+        file.read_to_string(&mut file_contents)
+            .await
+            .map_err(|e| ActionError::Read(fstab_path.to_owned(), e))?;
+
+        if let Some(start) = file_contents.rfind(fstab_entry.as_str()) {
+            let end = start + fstab_entry.len();
+            file_contents.replace_range(start..end, "")
+        }
+
+        file.seek(SeekFrom::Start(0))
+            .await
+            .map_err(|e| ActionError::Seek(fstab_path.to_owned(), e))?;
+        file.set_len(0)
+            .await
+            .map_err(|e| ActionError::Truncate(fstab_path.to_owned(), e))?;
+        file.write_all(file_contents.as_bytes())
+            .await
+            .map_err(|e| ActionError::Write(fstab_path.to_owned(), e))?;
+        file.flush()
+            .await
+            .map_err(|e| ActionError::Flush(fstab_path.to_owned(), e))?;
+
+        Ok(())
+    }
+}
+
+async fn get_uuid_for_label(apfs_volume_label: &str) -> Result<Option<Uuid>, ActionError> {
+    let output = execute_command(
+        Command::new("/usr/sbin/diskutil")
+            .process_group(0)
+            .arg("info")
+            .arg(apfs_volume_label)
+            .stdin(std::process::Stdio::null())
+            .stdout(std::process::Stdio::piped()),
+    )
+    .await
+    .map_err(|e| ActionError::Command(e))?;
+
+    let stdout = String::from_utf8(output.stdout)?;
+
+    let mut found = None;
+    for line in stdout.lines() {
+        let prefix = "Volume UUID:";
+        let trimmed = line.trim();
+        if let Some(index) = trimmed.find(prefix) {
+            let maybe_uuid = trimmed[(index + prefix.len())..].trim();
+            let uuid = Uuid::parse_str(maybe_uuid).map_err(|err| {
+                ActionError::Custom(Box::new(CreateFstabEntryError::Uuid(
+                    maybe_uuid.to_string(),
+                    err,
+                )))
+            })?;
+
+            found = Some(uuid);
+            break;
+        }
+    }
+
+    Ok(found)
+}
+
+fn fstab_prelude_comment(apfs_volume_label: &str) -> String {
+    format!("# nix-installer created volume labelled `{apfs_volume_label}`")
+}
+
+fn fstab_entry(uuid: &Uuid, apfs_volume_label: &str) -> String {
+    let prelude_comment = fstab_prelude_comment(apfs_volume_label);
+    format!(
+        "\
+        {prelude_comment}\n\
+        UUID={uuid} /nix apfs rw,noauto,nobrowse,suid,owners\n\
+        "
+    )
+}
+
+#[derive(thiserror::Error, Debug)]
+pub enum CreateFstabEntryError {
+    #[error("UUID error: {0}")]
+    Uuid(String, #[source] uuid::Error),
+    #[error("No volume labelled `{0}` present, cannot get UUID to add to /etc/fstab")]
+    NoVolume(String),
+    #[error("An `/etc/fstab` entry for the volume labelled `{0}` already exists. If a Nix Store already exists it may need to be deleted with `diskutil apfs deleteVolume \"{0}\") and should be removed from `/etc/fstab`")]
+    EntryExists(String),
+}

src/action/darwin/create_nix_volume.rs

@@ -13,6 +13,8 @@ use std::{
 use tokio::process::Command;
 use tracing::{span, Span};
 
+use super::create_fstab_entry::CreateFstabEntry;
+
 pub const NIX_VOLUME_MOUNTD_DEST: &str = "/Library/LaunchDaemons/org.nixos.darwin-store.plist";
 
 /// Create an APFS volume
@@ -26,7 +28,7 @@ pub struct CreateNixVolume {
     create_synthetic_objects: StatefulAction<CreateSyntheticObjects>,
     unmount_volume: StatefulAction<UnmountApfsVolume>,
     create_volume: StatefulAction<CreateApfsVolume>,
-    create_or_append_fstab: StatefulAction<CreateOrInsertIntoFile>,
+    create_fstab_entry: StatefulAction<CreateFstabEntry>,
     encrypt_volume: Option<StatefulAction<EncryptApfsVolume>>,
     setup_volume_daemon: StatefulAction<CreateFile>,
     bootstrap_volume: StatefulAction<BootstrapApfsVolume>,
@@ -59,14 +61,7 @@ impl CreateNixVolume {
 
         let create_volume = CreateApfsVolume::plan(disk, name.clone(), case_sensitive).await?;
 
-        let create_or_append_fstab = CreateOrInsertIntoFile::plan(
+        let create_fstab_entry = CreateFstabEntry::plan(name.clone())
-            "/etc/fstab",
-            None,
-            None,
-            0o0655,
-            format!("NAME=\"{name}\" /nix apfs rw,noauto,nobrowse,suid,owners"),
-            create_or_insert_into_file::Position::End,
-        )
             .await
             .map_err(|e| ActionError::Child(Box::new(e)))?;
 
@@ -125,7 +120,7 @@ impl CreateNixVolume {
             create_synthetic_objects,
             unmount_volume,
             create_volume,
-            create_or_append_fstab,
+            create_fstab_entry,
             encrypt_volume,
             setup_volume_daemon,
             bootstrap_volume,
@@ -173,7 +168,7 @@ impl Action for CreateNixVolume {
             create_synthetic_objects,
             unmount_volume,
             create_volume,
-            create_or_append_fstab,
+            create_fstab_entry,
             encrypt_volume,
             setup_volume_daemon,
             bootstrap_volume,
@@ -184,7 +179,7 @@ impl Action for CreateNixVolume {
         create_synthetic_objects.try_execute().await?;
         unmount_volume.try_execute().await.ok(); // We actually expect this may fail.
         create_volume.try_execute().await?;
-        create_or_append_fstab.try_execute().await?;
+        create_fstab_entry.try_execute().await?;
         if let Some(encrypt_volume) = encrypt_volume {
             encrypt_volume.try_execute().await?;
         }
@@ -236,7 +231,7 @@ impl Action for CreateNixVolume {
             create_synthetic_objects,
             unmount_volume,
             create_volume,
-            create_or_append_fstab,
+            create_fstab_entry,
             encrypt_volume,
             setup_volume_daemon,
             bootstrap_volume,
@@ -249,7 +244,7 @@ impl Action for CreateNixVolume {
         if let Some(encrypt_volume) = encrypt_volume {
             encrypt_volume.try_revert().await?;
         }
-        create_or_append_fstab.try_revert().await?;
+        create_fstab_entry.try_revert().await?;
 
         unmount_volume.try_revert().await?;
         create_volume.try_revert().await?;

src/action/darwin/mod.rs

@@ -3,6 +3,7 @@
 
 pub(crate) mod bootstrap_apfs_volume;
 pub(crate) mod create_apfs_volume;
+pub(crate) mod create_fstab_entry;
 pub(crate) mod create_nix_volume;
 pub(crate) mod create_synthetic_objects;
 pub(crate) mod enable_ownership;

tests/fixtures/darwin/darwin-multi.json

@@ -38,14 +38,9 @@
           },
           "state": "Uncompleted"
         },
-        "create_or_append_fstab": {
+        "create_fstab_entry": {
           "action": {
-            "path": "/etc/fstab",
+            "apfs_volume_label": "Nix Store"
-            "user": null,
-            "group": null,
-            "mode": 429,
-            "buf": "NAME=\"Nix Store\" /nix apfs rw,noauto,nobrowse,suid,owners",
-            "position": "End"
           },
           "state": "Uncompleted"
         },