Disallow hard link rendering to external directories

It's not clear that supporting this was ever intended, but it was
possible to run spfs-render and ask it to hard link into an external
directory. The various other internal calls had `RenderType::Copy` hard
coded.

Using hard links into an external directory exposes the spfs repository
to unintentional corruption since it allows those files to be edited and
consequently modify payloads that are meant to be immutable.

The change was prompted by wanting to refactor the code to work towards
having different trait bounds for the functions that need a repository
that supports renders (e.g., rendering into a repository) versus
functions that render into a plain directory.

Signed-off-by: J Robert Ray <[email protected]>

Author: jrray

crates/spfs-cli/cmd-render/src/cmd_render.rs

@@ -29,12 +29,14 @@ pub struct CmdRender {
 
     /// The strategy to use when rendering. Defaults to `Copy` when
     /// using a local directory and `HardLink` for the repository.
+    ///
+    /// Only `Copy` is supported when rendering to a directory.
     #[clap(
         long,
-        value_parser = clap::builder::PossibleValuesParser::new(spfs::storage::fs::RenderType::VARIANTS)
-            .map(|s| s.parse::<spfs::storage::fs::RenderType>().unwrap())
+        value_parser = clap::builder::PossibleValuesParser::new(spfs::storage::fs::CliRenderType::VARIANTS)
+            .map(|s| s.parse::<spfs::storage::fs::CliRenderType>().unwrap())
     )]
-    strategy: Option<spfs::storage::fs::RenderType>,
+    strategy: Option<spfs::storage::fs::CliRenderType>,
 
     /// The tag or digest of what to render, use a '+' to join multiple layers
     reference: String,
@@ -73,6 +75,9 @@ impl CmdRender {
         let fallback = FallbackProxy::new(repo, remotes);
 
         let rendered = match &self.target {
+            Some(_) if !matches!(self.strategy, Some(spfs::storage::fs::CliRenderType::Copy)) => {
+                miette::bail!("Only 'Copy' strategy is supported when rendering to a directory")
+            }
             Some(target) => self.render_to_dir(fallback, env_spec, target).await?,
             None => self.render_to_repo(fallback, env_spec).await?,
         };
@@ -118,11 +123,7 @@ impl CmdRender {
             ]),
         );
         renderer
-            .render_into_directory(
-                env_spec,
-                &target_dir,
-                self.strategy.unwrap_or(spfs::storage::fs::RenderType::Copy),
-            )
+            .render_into_directory(env_spec, &target_dir)
             .await?;
         Ok(RenderResult {
             paths_rendered: vec![target_dir],
@@ -165,7 +166,7 @@ impl CmdRender {
             .collect();
         tracing::trace!("stack: {:?}", stack);
         renderer
-            .render(&stack, self.strategy)
+            .render(&stack, self.strategy.map(Into::into))
             .await
             .map(|paths_rendered| RenderResult {
                 paths_rendered,

crates/spfs/src/resolve.rs

@@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize};
 use super::config::get_config;
 use crate::prelude::*;
 use crate::storage::fallback::FallbackProxy;
-use crate::storage::fs::{ManifestRenderPath, RenderSummary};
+use crate::storage::fs::{CliRenderType, ManifestRenderPath, RenderSummary};
 use crate::{graph, runtime, storage, tracking, Error, Result};
 
 #[cfg(test)]
@@ -53,7 +53,7 @@ async fn render_via_subcommand(
         // of overlayfs. To avoid any issues editing files and
         // hardlinks the rendering for them switches to Copy.
         cmd.arg("--strategy");
-        cmd.arg::<&str>(crate::storage::fs::RenderType::Copy.into());
+        cmd.arg::<&str>(CliRenderType::Copy.into());
     }
     cmd.arg(spec.to_string());
     tracing::debug!("{:?}", cmd);

crates/spfs/src/storage/fs/mod.rs

@@ -26,6 +26,8 @@ pub use render_reporter::{
 };
 pub use render_summary::{RenderSummary, RenderSummaryReporter};
 pub use renderer::{
+    CliRenderType,
+    HardLinkRenderType,
     RenderType,
     Renderer,
     DEFAULT_MAX_CONCURRENT_BLOBS,

crates/spfs/src/storage/fs/renderer.rs

@@ -40,13 +40,39 @@ pub const DEFAULT_MAX_CONCURRENT_BLOBS: usize = 100;
 /// See: [`Renderer::with_max_concurrent_branches`]
 pub const DEFAULT_MAX_CONCURRENT_BRANCHES: usize = 5;
 
+/// Render type options available to command line commands.
 #[derive(Debug, Copy, Clone, strum::EnumString, strum::VariantNames, strum::IntoStaticStr)]
-pub enum RenderType {
+pub enum CliRenderType {
     HardLink,
     HardLinkNoProxy,
     Copy,
 }
 
+#[derive(Debug, Default, Copy, Clone)]
+pub enum HardLinkRenderType {
+    #[default]
+    WithProxy,
+    WithoutProxy,
+}
+
+#[derive(Debug, Copy, Clone)]
+pub enum RenderType {
+    HardLink(HardLinkRenderType),
+    Copy,
+}
+
+impl From<CliRenderType> for RenderType {
+    fn from(cli_render_type: CliRenderType) -> Self {
+        match cli_render_type {
+            CliRenderType::HardLink => RenderType::HardLink(HardLinkRenderType::WithProxy),
+            CliRenderType::HardLinkNoProxy => {
+                RenderType::HardLink(HardLinkRenderType::WithoutProxy)
+            }
+            CliRenderType::Copy => RenderType::Copy,
+        }
+    }
+}
+
 impl OpenFsRepository {
     fn get_render_storage(&self) -> Result<&crate::storage::fs::FsHashStore> {
         match &self.fs_impl.renders {
@@ -280,12 +306,13 @@ where
         futures.try_collect().await
     }
 
-    /// Recreate the full structure of a stored environment on disk
+    /// Recreate the full structure of a stored environment on disk.
+    ///
+    /// The `RenderType::Copy` strategy will be used.
     pub async fn render_into_directory<E: Into<tracking::EnvSpec>, P: AsRef<Path>>(
         &self,
         env_spec: E,
         target_dir: P,
-        render_type: RenderType,
     ) -> Result<()> {
         let env_spec = env_spec.into();
         let mut stack = graph::Stack::default();
@@ -306,8 +333,15 @@ where
             manifest.update(&next.to_tracking_manifest());
         }
         let manifest = manifest.to_graph_manifest();
-        self.render_manifest_into_dir(&manifest, target_dir, render_type)
-            .await
+        self.render_manifest_into_dir(
+            &manifest,
+            target_dir,
+            // Creating hard links outside of the spfs repo makes it possible
+            // for users to modify payloads which are meant to be immutable,
+            // therefore using the Copy strategy is enforced here.
+            RenderType::Copy,
+        )
+        .await
     }
 
     /// Render a manifest into the renders area of the underlying repository,
@@ -338,7 +372,7 @@ where
         self.render_manifest_into_dir(
             manifest,
             &working_dir,
-            render_type.unwrap_or(RenderType::HardLink),
+            render_type.unwrap_or(RenderType::HardLink(HardLinkRenderType::default())),
         )
         .await
         .map_err(|err| {