From 689a655a83ff1325aaa0c35ce3883df40e829575 Mon Sep 17 00:00:00 2001 From: David Baird Date: Mon, 7 Aug 2023 19:33:03 -0400 Subject: [PATCH] Fix image-create with ACLs. Fixes #1394. Signed-off-by: David Baird --- rafs/src/metadata/layout/mod.rs | 2 +- rafs/src/metadata/layout/v6.rs | 89 ++++++++++++++++++++++++--------- smoke/tests/texture/layer.go | 12 +++++ 3 files changed, 79 insertions(+), 24 deletions(-) diff --git a/rafs/src/metadata/layout/mod.rs b/rafs/src/metadata/layout/mod.rs index bc702259e25..8581e89578b 100644 --- a/rafs/src/metadata/layout/mod.rs +++ b/rafs/src/metadata/layout/mod.rs @@ -267,7 +267,7 @@ impl RafsXAttrs { return Err(einval!("xattr key/value is too big")); } for p in RAFS_XATTR_PREFIXES { - if buf.len() > p.as_bytes().len() && &buf[..p.as_bytes().len()] == p.as_bytes() { + if buf.len() >= p.as_bytes().len() && &buf[..p.as_bytes().len()] == p.as_bytes() { self.pairs.insert(name, value); return Ok(()); } diff --git a/rafs/src/metadata/layout/v6.rs b/rafs/src/metadata/layout/v6.rs index d386855105c..a5fba6f9b16 100644 --- a/rafs/src/metadata/layout/v6.rs +++ b/rafs/src/metadata/layout/v6.rs @@ -2085,7 +2085,7 @@ impl RafsXAttrs { for (key, value) in self.pairs.iter() { let (index, prefix_len) = Self::match_prefix(key) .map_err(|_| einval!(format!("invalid xattr key {:?}", key)))?; - if key.len() <= prefix_len { + if key.len() < prefix_len { return Err(einval!(format!("invalid xattr key {:?}", key))); } if value.len() > u16::MAX as usize { @@ -2408,10 +2408,29 @@ mod tests { let mut reader: Box = Box::new(r); let mut xattrs = RafsXAttrs::new(); - xattrs.add(OsString::from("user.nydus"), vec![1u8]).unwrap(); + // These xattrs are in "e_name_index" order for easier reading: xattrs .add(OsString::from("security.rafs"), vec![2u8, 3u8]) .unwrap(); + xattrs + .add( + OsString::from("system.posix_acl_access"), + vec![4u8, 5u8, 6u8], + ) + .unwrap(); + xattrs + .add( + OsString::from("system.posix_acl_default"), + vec![7u8, 8u8, 9u8, 10u8], + ) + .unwrap(); + xattrs + .add( + OsString::from("trusted.abc"), + vec![11u8, 12u8, 13u8, 14u8, 15u8], + ) + .unwrap(); + xattrs.add(OsString::from("user.nydus"), vec![1u8]).unwrap(); xattrs.store_v6(&mut writer).unwrap(); writer.flush().unwrap(); @@ -2422,35 +2441,59 @@ mod tests { assert_eq!(header.h_shared_count, 0u8); let target1 = RafsV6XattrEntry { - e_name_len: 4u8, - e_name_index: 6u8, - e_value_size: u16::to_le(2u16), + e_name_len: 5u8, // "nydus" + e_name_index: 1u8, // EROFS_XATTR_INDEX_USER + e_value_size: u16::to_le(1u16), }; let target2 = RafsV6XattrEntry { - e_name_len: 5u8, - e_name_index: 1u8, - e_value_size: u16::to_le(1u16), + e_name_len: 0u8, // "" + e_name_index: 2u8, // EROFS_XATTR_INDEX_POSIX_ACL_ACCESS + e_value_size: u16::to_le(3u16), }; - let mut entry1 = RafsV6XattrEntry::new(); - reader.read_exact(entry1.as_mut()).unwrap(); - assert!((entry1 == target1 || entry1 == target2)); + let target3 = RafsV6XattrEntry { + e_name_len: 0u8, // "" + e_name_index: 3u8, // EROFS_XATTR_INDEX_POSIX_ACL_DEFAULT + e_value_size: u16::to_le(4u16), + }; - size += size_of::() - + entry1.name_len() as usize - + entry1.value_size() as usize; + let target4 = RafsV6XattrEntry { + e_name_len: 3u8, // "abc" + e_name_index: 4u8, // EROFS_XATTR_INDEX_TRUSTED + e_value_size: u16::to_le(5u16), + }; - reader - .seek_to_offset(round_up(size as u64, size_of::() as u64)) - .unwrap(); + let target5 = RafsV6XattrEntry { + e_name_len: 4u8, // "rafs" + e_name_index: 6u8, // EROFS_XATTR_INDEX_SECURITY + e_value_size: u16::to_le(2u16), + }; - let mut entry2 = RafsV6XattrEntry::new(); - reader.read_exact(entry2.as_mut()).unwrap(); - if entry1 == target1 { - assert!(entry2 == target2); - } else { - assert!(entry2 == target1); + let targets = vec![target1, target2, target3, target4, target5]; + + let mut entries: Vec = Vec::new(); + entries.reserve(targets.len()); + for _i in 0..targets.len() { + let mut entry = RafsV6XattrEntry::new(); + reader.read_exact(entry.as_mut()).unwrap(); + size += round_up( + (size_of::() + + entry.e_name_len as usize + + entry.e_value_size as usize) as u64, + size_of::() as u64, + ) as usize; + reader.seek_to_offset(size as u64).unwrap(); + entries.push(entry); + } + + for (i, target) in targets.iter().enumerate() { + let j = entries + .iter() + .position(|entry| entry == target) + .unwrap_or_else(|| panic!("Test failed for: target{}", i + 1)); + // Note: swap_remove() is faster than remove() when order doesn't matter: + entries.swap_remove(j); } } diff --git a/smoke/tests/texture/layer.go b/smoke/tests/texture/layer.go index 0bfcfbb61eb..7a5f8139dfb 100644 --- a/smoke/tests/texture/layer.go +++ b/smoke/tests/texture/layer.go @@ -88,6 +88,18 @@ func MakeLowerLayer(t *testing.T, workDir string, makers ...LayerMaker) *tool.La // Set file xattr (only `security.capability` xattr is supported in OCI layer) tool.Run(t, fmt.Sprintf("setcap CAP_NET_RAW+ep %s", filepath.Join(workDir, "dir-1/file-2"))) + // Note: The following test is omitted for now because containerd does not + // support created layers with any xattr except "security." xattrs described + // in this issue: https://github.com/containerd/containerd/issues/8947 + // Create file with an ACL: + //layer.CreateFile(t, "acl-file.txt", []byte("")) + // The following xattr key and value are equivalent to running this ACL + // command: "setfacl -x user:root:rwx acl-file.txt" + //layer.SetXattr(t, "acl-file.txt", "system.posix_acl_access", []byte{ + // 0x02, 0x00, 0x00, 0x00, 0x01, 0x00, 0x07, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x02, 0x00, 0x07, 0x00, + // 0x00, 0x00, 0x00, 0x00, 0x04, 0x00, 0x05, 0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x10, 0x00, 0x07, 0x00, + // 0xFF, 0xFF, 0xFF, 0xFF, 0x20, 0x00, 0x05, 0x00, 0xFF, 0xFF, 0xFF, 0xFF }); + // Customized files for _, maker := range makers { maker(t, layer)