Linus/ds 576 nix store should be in path directly (#148)

* CreateOrAppendFile: setuid safety

* Apply a sensible default mode if the file didn't already exist

* remove some incorrect docs

* Implement CreateOrInsertFile and use it instead of Append for shell rcs

Ubuntu's bashrc has the following lines in it:

  # If not running interactively, don't do anything
  [ -z "$PS1" ] && return

This means that anything appended to the file will only take effect in
interactive sessions. However, we want noninteractive shells to have
Nix on PATH too, e.g. for remote builds. Therefore, we need to add our
snippet to the beginning of the file, not the end.

* Adjust test fixtures to match new behaviour

* interaction: indicate default for confirm correctly

* CreateOrInsertFile: use randomised temporary filename

* Fix temp file creation

* Apply permissions to temp file, not final file

The final file may not exist, and the temporary file is the one whose
permissions will actually be preserved.

* Use the right ActionError for renaming

* Test the SSH behaviour properly

* cargo fmt

* [DS-574] Write to zshenv instead of zshrc

* CreateOrInsertFile -> CreateOrInsertIntoFile

Also move appending behaviour in here.

* Update Linux fixtures

* update darwin-multi fixture

* fix fixture

* fmt

nix/tests/vm-test/default.nix

@@ -185,12 +185,6 @@ let
         $ssh <<EOF
           set -ex
 
-          # FIXME: get rid of this; ideally ssh should just work.
-          source ~/.bash_profile || true
-          source ~/.bash_login || true
-          source ~/.profile || true
-          source /etc/bashrc || true
-
           nix-env --version
           nix --extra-experimental-features nix-command store ping
 

src/action/base/create_or_insert_into_file.rs

@@ -1,6 +1,7 @@
 use nix::unistd::{chown, Group, User};
 
 use crate::action::{Action, ActionDescription, ActionError, StatefulAction};
+use rand::Rng;
 use std::{
     io::SeekFrom,
     os::unix::prelude::PermissionsExt,
@@ -12,24 +13,29 @@ use tokio::{
 };
 use tracing::{span, Span};
 
-/** Create a file at the given location with the provided `buf`,
-optionally with an owning user, group, and mode.
+#[derive(Debug, serde::Deserialize, serde::Serialize, Clone, PartialEq, Eq)]
+pub enum Position {
+    Beginning,
+    End,
+}
 
-If the file exists, the provided `buf` will be appended.
+/** Create a file at the given location with the provided `buf` as
+contents, optionally with an owning user, group, and mode.
 
-If `force` is set, the file will always be overwritten (and deleted)
-regardless of its presence prior to install.
+If the file exists, the provided `buf` will be inserted at its
+beginning or end, depending on the position field.
  */
 #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)]
-pub struct CreateOrAppendFile {
+pub struct CreateOrInsertIntoFile {
     path: PathBuf,
     user: Option<String>,
     group: Option<String>,
     mode: Option<u32>,
     buf: String,
+    position: Position,
 }
 
-impl CreateOrAppendFile {
+impl CreateOrInsertIntoFile {
     #[tracing::instrument(level = "debug", skip_all)]
     pub async fn plan(
         path: impl AsRef<Path>,
@@ -37,6 +43,7 @@ impl CreateOrAppendFile {
         group: impl Into<Option<String>>,
         mode: impl Into<Option<u32>>,
         buf: String,
+        position: Position,
     ) -> Result<StatefulAction<Self>, ActionError> {
         let path = path.as_ref().to_path_buf();
 
@@ -46,22 +53,23 @@ impl CreateOrAppendFile {
             group: group.into(),
             mode: mode.into(),
             buf,
+            position,
         }
         .into())
     }
 }
 
 #[async_trait::async_trait]
-#[typetag::serde(name = "create_or_append_file")]
-impl Action for CreateOrAppendFile {
+#[typetag::serde(name = "create_or_insert_into_file")]
+impl Action for CreateOrInsertIntoFile {
     fn tracing_synopsis(&self) -> String {
-        format!("Create or append file `{}`", self.path.display())
+        format!("Create or insert file `{}`", self.path.display())
     }
 
     fn tracing_span(&self) -> Span {
         let span = span!(
             tracing::Level::DEBUG,
-            "create_or_append_file",
+            "create_or_insert_file",
             path = tracing::field::display(self.path.display()),
             user = self.user,
             group = self.group,
@@ -89,23 +97,63 @@ impl Action for CreateOrAppendFile {
             group,
             mode,
             buf,
+            position,
         } = self;
 
-        let mut file = OpenOptions::new()
+        let mut orig_file = match OpenOptions::new().read(true).open(&path).await {
+            Ok(f) => Some(f),
+            Err(e) if e.kind() == std::io::ErrorKind::NotFound => None,
+            Err(e) => return Err(ActionError::Open(path.to_owned(), e)),
+        };
+
+        // Create a temporary file in the same directory as the one
+        // that the final file goes in, so that we can rename it
+        // atomically
+        let parent_dir = path.parent().expect("File must be in a directory");
+        let mut temp_file_path = parent_dir.to_owned();
+        {
+            let mut rng = rand::thread_rng();
+            temp_file_path.push(format!("nix-installer-tmp.{}", rng.gen::<u32>()));
+        }
+        let mut temp_file = OpenOptions::new()
             .create(true)
             .write(true)
-            .read(true)
-            .open(&path)
+            // If the file is created, ensure that it has harmless
+            // permissions regardless of whether the mode will be
+            // changed later (if we ever create setuid executables,
+            // they should only become setuid once they are owned by
+            // the appropriate user)
+            .mode(0o600)
+            .open(&temp_file_path)
             .await
-            .map_err(|e| ActionError::Open(path.to_owned(), e))?;
+            .map_err(|e| {
+                ActionError::Open(temp_file_path.clone(), e)
+            })?;
 
-        file.seek(SeekFrom::End(0))
-            .await
-            .map_err(|e| ActionError::Seek(path.to_owned(), e))?;
+        if *position == Position::End {
+            if let Some(ref mut orig_file) = orig_file {
+                tokio::io::copy(orig_file, &mut temp_file)
+                    .await
+                    .map_err(|e| {
+                        ActionError::Copy(path.to_owned(), temp_file_path.to_owned(), e)
+                    })?;
+            }
+        }
 
-        file.write_all(buf.as_bytes())
+        temp_file
+            .write_all(buf.as_bytes())
             .await
-            .map_err(|e| ActionError::Write(path.to_owned(), e))?;
+            .map_err(|e| ActionError::Write(temp_file_path.clone(), e))?;
+
+        if *position == Position::Beginning {
+            if let Some(ref mut orig_file) = orig_file {
+                tokio::io::copy(orig_file, &mut temp_file)
+                    .await
+                    .map_err(|e| {
+                        ActionError::Copy(path.to_owned(), temp_file_path.to_owned(), e)
+                    })?;
+            }
+        }
 
         let gid = if let Some(group) = group {
             Some(
@@ -128,13 +176,24 @@ impl Action for CreateOrAppendFile {
             None
         };
 
+        // Change ownership _before_ applying mode, to ensure that if
+        // a file needs to be setuid it will never be setuid for the
+        // wrong user
+        chown(&temp_file_path, uid, gid).map_err(|e| ActionError::Chown(path.clone(), e))?;
+
         if let Some(mode) = mode {
-            tokio::fs::set_permissions(&path, PermissionsExt::from_mode(*mode))
+            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() {
+            tokio::fs::set_permissions(&temp_file_path, PermissionsExt::from_mode(0o644))
+                .await
+                .map_err(|e| ActionError::SetPermissions(0o644, path.to_owned(), e))?;
         }
 
-        chown(path, uid, gid).map_err(|e| ActionError::Chown(path.clone(), e))?;
+        tokio::fs::rename(&temp_file_path, &path)
+            .await
+            .map_err(|e| ActionError::Rename(path.to_owned(), temp_file_path.to_owned(), e))?;
 
         Ok(())
     }
@@ -146,6 +205,7 @@ impl Action for CreateOrAppendFile {
             group: _,
             mode: _,
             buf,
+            position: _,
         } = &self;
         vec![ActionDescription::new(
             format!("Delete Nix related fragment from file `{}`", path.display()),
@@ -164,6 +224,7 @@ impl Action for CreateOrAppendFile {
             group: _,
             mode: _,
             buf,
+            position: _,
         } = self;
         let mut file = OpenOptions::new()
             .create(false)
@@ -171,12 +232,12 @@ impl Action for CreateOrAppendFile {
             .read(true)
             .open(&path)
             .await
-            .map_err(|e| ActionError::Read(path.to_owned(), e))?;
+            .map_err(|e| ActionError::Open(path.to_owned(), e))?;
 
         let mut file_contents = String::default();
         file.read_to_string(&mut file_contents)
             .await
-            .map_err(|e| ActionError::Seek(path.to_owned(), e))?;
+            .map_err(|e| ActionError::Read(path.to_owned(), e))?;
 
         if let Some(start) = file_contents.rfind(buf.as_str()) {
             let end = start + buf.len();

src/action/base/mod.rs

@@ -3,7 +3,7 @@
 pub(crate) mod create_directory;
 pub(crate) mod create_file;
 pub(crate) mod create_group;
-pub(crate) mod create_or_append_file;
+pub(crate) mod create_or_insert_into_file;
 pub(crate) mod create_user;
 pub(crate) mod fetch_and_unpack_nix;
 pub(crate) mod move_unpacked_nix;
@@ -12,7 +12,7 @@ pub(crate) mod setup_default_profile;
 pub use create_directory::CreateDirectory;
 pub use create_file::CreateFile;
 pub use create_group::CreateGroup;
-pub use create_or_append_file::CreateOrAppendFile;
+pub use create_or_insert_into_file::CreateOrInsertIntoFile;
 pub use create_user::CreateUser;
 pub use fetch_and_unpack_nix::{FetchAndUnpackNix, FetchUrlError};
 pub use move_unpacked_nix::{MoveUnpackedNix, MoveUnpackedNixError};

src/action/common/configure_shell_profile.rs

@@ -1,4 +1,4 @@
-use crate::action::base::{CreateDirectory, CreateOrAppendFile};
+use crate::action::base::{create_or_insert_into_file, CreateDirectory, CreateOrInsertIntoFile};
 use crate::action::{Action, ActionDescription, ActionError, StatefulAction};
 
 use nix::unistd::User;
@@ -23,9 +23,9 @@ const PROFILE_FISH_PREFIXES: &[&str] = &[
 const PROFILE_TARGETS: &[&str] = &[
     "/etc/bashrc",
     "/etc/profile.d/nix.sh",
-    "/etc/zshrc",
+    "/etc/zshenv",
     "/etc/bash.bashrc",
-    "/etc/zsh/zshrc",
+    "/etc/zsh/zshenv",
 ];
 const PROFILE_NIX_FILE_SHELL: &str = "/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.sh";
 const PROFILE_NIX_FILE_FISH: &str = "/nix/var/nix/profiles/default/etc/profile.d/nix-daemon.fish";
@@ -36,13 +36,13 @@ Configure any detected shell profiles to include Nix support
 #[derive(Debug, serde::Deserialize, serde::Serialize, Clone)]
 pub struct ConfigureShellProfile {
     create_directories: Vec<StatefulAction<CreateDirectory>>,
-    create_or_append_files: Vec<StatefulAction<CreateOrAppendFile>>,
+    create_or_insert_into_files: Vec<StatefulAction<CreateOrInsertIntoFile>>,
 }
 
 impl ConfigureShellProfile {
     #[tracing::instrument(level = "debug", skip_all)]
     pub async fn plan() -> Result<StatefulAction<Self>, ActionError> {
-        let mut create_or_append_files = Vec::default();
+        let mut create_or_insert_files = Vec::default();
         let mut create_directories = Vec::default();
 
         let shell_buf = format!(
@@ -61,17 +61,18 @@ impl ConfigureShellProfile {
             if let Some(parent) = profile_target_path.parent() {
                 if !parent.exists() {
                     tracing::trace!(
-                        "Did not plan to edit `{profile_target}` as it's parent folder does not exist."
+                        "Did not plan to edit `{profile_target}` as its parent folder does not exist."
                     );
                     continue;
                 }
-                create_or_append_files.push(
-                    CreateOrAppendFile::plan(
+                create_or_insert_files.push(
+                    CreateOrInsertIntoFile::plan(
                         profile_target_path,
                         None,
                         None,
                         0o0755,
                         shell_buf.to_string(),
+                        create_or_insert_into_file::Position::Beginning,
                     )
                     .await?,
                 );
@@ -106,9 +107,16 @@ impl ConfigureShellProfile {
                 );
             }
 
-            create_or_append_files.push(
-                CreateOrAppendFile::plan(profile_target, None, None, 0o0755, fish_buf.to_string())
-                    .await?,
+            create_or_insert_files.push(
+                CreateOrInsertIntoFile::plan(
+                    profile_target,
+                    None,
+                    None,
+                    0o0755,
+                    fish_buf.to_string(),
+                    create_or_insert_into_file::Position::Beginning,
+                )
+                .await?,
             );
         }
 
@@ -123,13 +131,22 @@ impl ConfigureShellProfile {
                     runner.name
                 );
             }
-            create_or_append_files
-                .push(CreateOrAppendFile::plan(&github_path, None, None, None, buf).await?)
+            create_or_insert_files.push(
+                CreateOrInsertIntoFile::plan(
+                    &github_path,
+                    None,
+                    None,
+                    None,
+                    buf,
+                    create_or_insert_into_file::Position::End,
+                )
+                .await?,
+            )
         }
 
         Ok(Self {
             create_directories,
-            create_or_append_files,
+            create_or_insert_into_files: create_or_insert_files,
         }
         .into())
     }
@@ -156,7 +173,7 @@ impl Action for ConfigureShellProfile {
     #[tracing::instrument(level = "debug", skip_all)]
     async fn execute(&mut self) -> Result<(), ActionError> {
         let Self {
-            create_or_append_files,
+            create_or_insert_into_files,
             create_directories,
         } = self;
 
@@ -167,22 +184,22 @@ impl Action for ConfigureShellProfile {
         let mut set = JoinSet::new();
         let mut errors = Vec::default();
 
-        for (idx, create_or_append_file) in create_or_append_files.iter().enumerate() {
+        for (idx, create_or_insert_into_file) in create_or_insert_into_files.iter().enumerate() {
             let span = tracing::Span::current().clone();
-            let mut create_or_append_file_clone = create_or_append_file.clone();
+            let mut create_or_insert_into_file_clone = create_or_insert_into_file.clone();
             let _abort_handle = set.spawn(async move {
-                create_or_append_file_clone
+                create_or_insert_into_file_clone
                     .try_execute()
                     .instrument(span)
                     .await?;
-                Result::<_, ActionError>::Ok((idx, create_or_append_file_clone))
+                Result::<_, ActionError>::Ok((idx, create_or_insert_into_file_clone))
             });
         }
 
         while let Some(result) = set.join_next().await {
             match result {
-                Ok(Ok((idx, create_or_append_file))) => {
-                    create_or_append_files[idx] = create_or_append_file
+                Ok(Ok((idx, create_or_insert_into_file))) => {
+                    create_or_insert_into_files[idx] = create_or_insert_into_file
                 },
                 Ok(Err(e)) => errors.push(Box::new(e)),
                 Err(e) => return Err(e.into()),
@@ -211,24 +228,24 @@ impl Action for ConfigureShellProfile {
     async fn revert(&mut self) -> Result<(), ActionError> {
         let Self {
             create_directories,
-            create_or_append_files,
+            create_or_insert_into_files,
         } = self;
 
         let mut set = JoinSet::new();
         let mut errors: Vec<Box<ActionError>> = Vec::default();
 
-        for (idx, create_or_append_file) in create_or_append_files.iter().enumerate() {
-            let mut create_or_append_file_clone = create_or_append_file.clone();
+        for (idx, create_or_insert_into_file) in create_or_insert_into_files.iter().enumerate() {
+            let mut create_or_insert_file_clone = create_or_insert_into_file.clone();
             let _abort_handle = set.spawn(async move {
-                create_or_append_file_clone.try_revert().await?;
-                Result::<_, _>::Ok((idx, create_or_append_file_clone))
+                create_or_insert_file_clone.try_revert().await?;
+                Result::<_, _>::Ok((idx, create_or_insert_file_clone))
             });
         }
 
         while let Some(result) = set.join_next().await {
             match result {
-                Ok(Ok((idx, create_or_append_file))) => {
-                    create_or_append_files[idx] = create_or_append_file
+                Ok(Ok((idx, create_or_insert_into_file))) => {
+                    create_or_insert_into_files[idx] = create_or_insert_into_file
                 },
                 Ok(Err(e)) => errors.push(Box::new(e)),
                 Err(e) => return Err(e.into()),

src/action/darwin/create_nix_volume.rs

@@ -1,5 +1,5 @@
 use crate::action::{
-    base::{CreateFile, CreateOrAppendFile},
+    base::{create_or_insert_into_file, CreateFile, CreateOrInsertIntoFile},
     darwin::{
         BootstrapApfsVolume, CreateApfsVolume, CreateSyntheticObjects, EnableOwnership,
         EncryptApfsVolume, UnmountApfsVolume,
@@ -22,11 +22,11 @@ pub struct CreateNixVolume {
     name: String,
     case_sensitive: bool,
     encrypt: bool,
-    create_or_append_synthetic_conf: StatefulAction<CreateOrAppendFile>,
+    create_or_append_synthetic_conf: StatefulAction<CreateOrInsertIntoFile>,
     create_synthetic_objects: StatefulAction<CreateSyntheticObjects>,
     unmount_volume: StatefulAction<UnmountApfsVolume>,
     create_volume: StatefulAction<CreateApfsVolume>,
-    create_or_append_fstab: StatefulAction<CreateOrAppendFile>,
+    create_or_append_fstab: StatefulAction<CreateOrInsertIntoFile>,
     encrypt_volume: Option<StatefulAction<EncryptApfsVolume>>,
     setup_volume_daemon: StatefulAction<CreateFile>,
     bootstrap_volume: StatefulAction<BootstrapApfsVolume>,
@@ -42,12 +42,13 @@ impl CreateNixVolume {
         encrypt: bool,
     ) -> Result<StatefulAction<Self>, ActionError> {
         let disk = disk.as_ref();
-        let create_or_append_synthetic_conf = CreateOrAppendFile::plan(
+        let create_or_append_synthetic_conf = CreateOrInsertIntoFile::plan(
             "/etc/synthetic.conf",
             None,
             None,
             0o0655,
             "nix\n".into(), /* The newline is required otherwise it segfaults */
+            create_or_insert_into_file::Position::End,
         )
         .await
         .map_err(|e| ActionError::Child(Box::new(e)))?;
@@ -58,12 +59,13 @@ impl CreateNixVolume {
 
         let create_volume = CreateApfsVolume::plan(disk, name.clone(), case_sensitive).await?;
 
-        let create_or_append_fstab = CreateOrAppendFile::plan(
+        let create_or_append_fstab = CreateOrInsertIntoFile::plan(
             "/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)))?;

src/cli/interaction.rs

@@ -19,8 +19,8 @@ pub(crate) async fn confirm(question: impl AsRef<str>, default: bool) -> eyre::R
     ",
         question = question.as_ref(),
         are_you_sure = "Proceed?".bold(),
-        no = "N".red().bold(),
-        yes = "y".green(),
+        no = if default { "n" } else { "N" }.red(),
+        yes = if default { "Y" } else { "y" }.green(),
     );
 
     term.write_all(with_confirm.as_bytes())?;