Support symlinks for the `blobs/sha256` directory

[ariel-miculas]

Context: At LPC I presented the changes made to PuzzleFS so it can be integrated with other tools, such as skopeo and lxc. In particular, I showed how to create an LXC container which has its rootfs backed by a PuzzleFS image. In particular, the following command line:

sudo lxc-create --name mycontainer --template oci -- --url oci:/tmp/pfs_ubuntu:eg \
--no-cache --mount-helper pfs-ovl-mount-helper

sets up a PuzzleFS rootfs. Notice the --no-cache argument for this template.

The reason we need this is because the oci template sets up a shared cache for the OCI blobs. The resulting OCI structure looks like this:

root@amiculas-l-PF3FCGJH:/var/lib/lxc/mycontainer# tree -L 2 oci/
oci/
├── blobs
│   └── sha256 -> /var/cache/lxc/sha256
├── index.json
└── oci-layout

2 directories, 2 files

This is a problem because ocidir-rs doesn't follow symlinks by design (using the cap-std model). To allow for such an LXC setup, I have two ideas:

• Extend the ocidir-rs API to support symlinks; the users can then opt-in to the capability-oriented API or choose the unrestricted option which can access files outside the oci directory
• Store an additional directory into the OciDir structure which references the blobs/sha256 directory (ok, maybe not blobs/sha256 but actually blobs, so we don't hardcode the hashing method; this of course will require changes to the LXC template) and extend the API to allow for two ambient directories passed to open: one for the oci directory which is required, and one for the oci/blobs/sha256 (or oci/blobs, as I mentioned earlier) which is optional (and in this case we just read it from the oci directory)

I like the second method better because it requires less changes to ocidir-rs, but also because it still provides security benefits by not allowing random symlink accesses: we're confined to the oci/blobs/sha256 directory.

Any objections against implementing the 2nd option?

[cgwalters]

There's a few design choices here...I started to stub out something like this but it got ugly:

diff --git a/src/lib.rs b/src/lib.rs
index 3245ae3..d48a8b1 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -152,11 +152,18 @@ impl<'a> Debug for GzipLayerWriter<'a> {
     }
 }

-#[derive(Debug)]
 /// An opened OCI directory.
 pub struct OciDir {
     /// The underlying directory.
     pub dir: std::sync::Arc<Dir>,
+    external_links: Option<cap_std::AmbientAuthority>,
+}
+
+impl Debug for OciDir {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // Unfortunately AmbientAuthority doesn't impl Debug
+        f.debug_struct("OciDir").field("dir", &self.dir).finish()
+    }
 }

 /// Write a serializable data (JSON) as an OCI blob
@@ -237,7 +244,21 @@ impl OciDir {
     /// Open an existing OCI directory.
     pub fn open(dir: &Dir) -> Result<Self> {
         let dir = std::sync::Arc::new(dir.try_clone()?);
-        Ok(Self { dir })
+        Ok(Self {
+            dir,
+            external_links: None,
+        })
+    }
+
+    pub fn open_allow_external_links(
+        dir: &Dir,
+        authority: cap_std::AmbientAuthority,
+    ) -> Result<Self> {
+        let dir = std::sync::Arc::new(dir.try_clone()?);
+        Ok(Self {
+            dir,
+            external_links: Some(authority),
+        })
     }

     /// Write a serializable data (JSON) as an OCI blob
@@ -337,7 +358,21 @@ impl OciDir {

     /// Open a blob; its size is validated as a sanity check.
     pub fn read_blob(&self, desc: &oci_spec::image::Descriptor) -> Result<File> {
-        let path = Self::parse_descriptor_to_path(desc)?;
+        let mut path = Self::parse_descriptor_to_path(desc)?;
+        let meta = self.dir.symlink_metadata(path)?;
+        let f = match (meta.is_symlink(), self.external_links.as_ref()) {
+            (true, Some(authority)) => {
+                let authority = authority.clone();
+                let abspath = format!("/proc/self/fd/{}/{path}")
+            }
+            (true, None) => todo!(),
+            (false, None) => todo!(),
+            (false, Some(_)) => todo!(),
+        }
+        if let Some(authority) = self.external_links.as_ref() {
+
+            
+        }
         let f = self.dir.open(path).map(|f| f.into_std())?;
         let expected: u64 = desc.size();
         let found = f.metadata()?.len();

Store an additional directory into the OciDir structure which references the blobs/sha256 directory (ok, maybe not blobs/sha256 but actually blobs, so we don't hardcode the hashing method; this of course will require changes to the LXC template)

Yeah I like this the best, something like:

diff --git a/src/lib.rs b/src/lib.rs
index 3245ae3..406effd 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -240,6 +240,10 @@ impl OciDir {
         Ok(Self { dir })
     }

+    pub fn add_external_blobdir(&mut self, dir: Dir) {
+        self.external_blobdirs.push(dir);
+    }
+
     /// Write a serializable data (JSON) as an OCI blob
     pub fn write_json_blob<S: serde::Serialize>(
         &self,

so we don't hardcode the hashing method;

Sounds nice in theory, but realistically I don't think we're going to get away from sha256 in the near (years) future. Maybe sha512 or something should be supported as a fallback Just In Case but the trauma to the ecosystem I suspect would be pretty high.

[ariel-miculas]

I was thinking about the case where a blob in sha256 would reference a blob in sha512, for example. Then we cannot get to that blob because we are confined in blobs/sha256 by the capability model. Anyway, maybe I'm thinking too theoretically. I'll sketch something up for option two in the following days.

[ariel-miculas]

@cgwalters I can't figure out why dir needs to be wrapped in an std::sync::Arc here:

/// An opened OCI directory.
pub struct OciDir {
    /// The underlying directory.
    pub dir: std::sync::Arc<Dir>,
}

I don't see any clones on dir and the caller still has a reference &Dir, shouldn't the caller have a std::sync::Arc instance as well?

I'm thinking about this change:

diff --git a/src/lib.rs b/src/lib.rs
index 3245ae3..e297f23 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -209,7 +209,7 @@ fn sha256_of_descriptor(desc: &Descriptor) -> Result<&str> {
 impl OciDir {
     /// Open the OCI directory at the target path; if it does not already
     /// have the standard OCI metadata, it is created.
-    pub fn ensure(dir: &Dir) -> Result<Self> {
+    pub fn ensure(dir: Dir) -> Result<(Self, std::sync::Arc<Dir>)> {
         let mut db = cap_std::fs::DirBuilder::new();
         db.recursive(true).mode(0o755);
         dir.ensure_dir_with(BLOBDIR, &db)?;
@@ -220,10 +220,10 @@ impl OciDir {
     }

     /// Clone an OCI directory, using reflinks for blobs.
-    pub fn clone_to(&self, destdir: &Dir, p: impl AsRef<Path>) -> Result<Self> {
+    pub fn clone_to(&self, destdir: Dir, p: impl AsRef<Path>) -> Result<(Self, std::sync::Arc<Dir>)> {
         let p = p.as_ref();
         destdir.create_dir(p)?;
-        let cloned = Self::ensure(&destdir.open_dir(p)?)?;
+        let cloned = Self::ensure(destdir.open_dir(p)?)?;
         for blob in self.dir.read_dir(BLOBDIR)? {
             let blob = blob?;
             let path = Path::new(BLOBDIR).join(blob.file_name());
@@ -235,9 +235,9 @@ impl OciDir {
     }

     /// Open an existing OCI directory.
-    pub fn open(dir: &Dir) -> Result<Self> {
+    pub fn open(dir: Dir) -> Result<(Self, std::sync::Arc<Dir>)> {
         let dir = std::sync::Arc::new(dir.try_clone()?);
-        Ok(Self { dir })
+        Ok((Self { dir: dir.clone() }, dir))
     }

     /// Write a serializable data (JSON) as an OCI blob

Then the caller accesses are synchronized with the ocidir accesses.

[cgwalters]

I don't see any clones on dir and the caller still has a reference &Dir, shouldn't the caller have a std::sync::Arc instance as well?

Yeah this is a legacy from this crate being split out of some internal code before, where I had some partially-done work to try to use Arc<Dir> more to avoid some unnecessary file descriptor duplication.

I guess for now we could drop the internal Arc easily since it doesn't buy us anything.

Or if we were to lean into this pattern we could have a fn new_from(d: impl Into<Arc<Dir>>) or so...