-
Notifications
You must be signed in to change notification settings - Fork 354
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
linux_masked_paths integration test #2950
base: main
Are you sure you want to change the base?
Changes from 8 commits
1656207
9b8c4b9
d40d1a2
6b6a95c
64dd8af
e54350d
e6bdefa
f9bcfe3
a298687
ed15bf7
b4e3511
5d79a48
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,225 @@ | ||
use std::path::PathBuf; | ||
|
||
use anyhow::{anyhow, bail}; | ||
use nix::sys::stat::SFlag; | ||
use oci_spec::runtime::{LinuxBuilder, ProcessBuilder, Spec, SpecBuilder}; | ||
use test_framework::{Test, TestGroup, TestResult}; | ||
|
||
use crate::utils::test_inside_container; | ||
use crate::utils::test_utils::CreateOptions; | ||
|
||
fn get_spec(masked_paths: Vec<String>) -> Spec { | ||
SpecBuilder::default() | ||
.linux( | ||
LinuxBuilder::default() | ||
.masked_paths(masked_paths) | ||
.build() | ||
.expect("could not build"), | ||
) | ||
.process( | ||
ProcessBuilder::default() | ||
.args(vec!["runtimetest".to_string(), "masked_paths".to_string()]) | ||
.build() | ||
.unwrap(), | ||
) | ||
.build() | ||
.unwrap() | ||
} | ||
|
||
fn check_masked_paths() -> TestResult { | ||
let masked_dir = "masked-dir"; | ||
let masked_subdir = "masked-subdir"; | ||
let masked_file = "masked-file"; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make them the const values. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Others are also the same. |
||
|
||
let masked_dir_top = PathBuf::from(masked_dir); | ||
let masked_file_top = PathBuf::from(masked_file); | ||
|
||
let masked_dir_sub = masked_dir_top.join(masked_subdir); | ||
let masked_file_sub = masked_dir_top.join(masked_file); | ||
let masked_file_sub_sub = masked_dir_sub.join(masked_file); | ||
|
||
let root = PathBuf::from("/"); | ||
|
||
let masked_paths = vec![ | ||
root.join(&masked_dir_top).to_string_lossy().to_string(), | ||
root.join(&masked_file_top).to_string_lossy().to_string(), | ||
root.join(&masked_dir_sub).to_string_lossy().to_string(), | ||
root.join(&masked_file_sub).to_string_lossy().to_string(), | ||
root.join(&masked_file_sub_sub) | ||
.to_string_lossy() | ||
.to_string(), | ||
]; | ||
|
||
let spec = get_spec(masked_paths); | ||
|
||
test_inside_container(spec, &CreateOptions::default(), &|bundle_path| { | ||
use std::fs; | ||
let test_dir = bundle_path.join(&masked_dir_sub); | ||
fs::create_dir_all(&test_dir)?; | ||
|
||
fs::File::create(test_dir.join("tmp"))?; | ||
|
||
// runtimetest cannot check the readability of empty files, so | ||
// write something. | ||
let test_sub_sub_file = bundle_path.join(&masked_file_sub_sub); | ||
fs::File::create(&test_sub_sub_file)?; | ||
fs::write(&test_sub_sub_file, b"secrets")?; | ||
|
||
let test_sub_file = bundle_path.join(&masked_file_sub); | ||
fs::File::create(&test_sub_file)?; | ||
fs::write(&test_sub_file, b"secrets")?; | ||
|
||
let test_file = bundle_path.join(masked_file); | ||
fs::File::create(&test_file)?; | ||
fs::write(&test_file, b"secrets")?; | ||
|
||
Ok(()) | ||
}) | ||
} | ||
|
||
fn check_masked_rel_paths() -> TestResult { | ||
// Deliberately set a relative path to be masked, | ||
// and expect an error | ||
let masked_rel_path = "../masked_rel_path"; | ||
let masked_paths = vec![masked_rel_path.to_string()]; | ||
let spec = get_spec(masked_paths); | ||
|
||
let res = test_inside_container(spec, &CreateOptions::default(), &|_bundle_path| Ok(())); | ||
// If the container creation succeeds, we expect an error since the masked paths does not support relative paths. | ||
if let TestResult::Passed = res { | ||
TestResult::Failed(anyhow!( | ||
"expected error in container creation with invalid symlink, found no error" | ||
)) | ||
} else { | ||
TestResult::Passed | ||
} | ||
} | ||
|
||
fn check_masked_symlinks() -> TestResult { | ||
// Deliberately create a masked symlink that points an invalid file, | ||
// and expect an error. | ||
let root = PathBuf::from("/"); | ||
let masked_symlink = "masked_symlink"; | ||
let masked_paths = vec![root.join(masked_symlink).to_string_lossy().to_string()]; | ||
let spec = get_spec(masked_paths); | ||
|
||
let res = test_inside_container(spec, &CreateOptions::default(), &|bundle_path| { | ||
use std::{fs, io}; | ||
let test_file = bundle_path.join(masked_symlink); | ||
// ln -s ../masked-symlink ; readlink -f /masked-symlink; ls -L /masked-symlink | ||
match std::os::unix::fs::symlink("../masked_symlink", &test_file) { | ||
io::Result::Ok(_) => { /* This is expected */ } | ||
io::Result::Err(e) => { | ||
bail!("error in creating symlink, to {:?} {:?}", test_file, e); | ||
} | ||
} | ||
|
||
let r_path = match fs::read_link(&test_file) { | ||
io::Result::Ok(p) => p, | ||
io::Result::Err(e) => { | ||
bail!("error in reading symlink at {:?} : {:?}", test_file, e); | ||
} | ||
}; | ||
|
||
// It ensures that the symlink points not to exist. | ||
match fs::metadata(r_path) { | ||
YJDoc2 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
io::Result::Ok(md) => { | ||
bail!( | ||
"reading path {:?} should have given error, found {:?} instead", | ||
test_file, | ||
md | ||
) | ||
} | ||
io::Result::Err(e) => { | ||
let err = e.kind(); | ||
if let io::ErrorKind::NotFound = err { | ||
Ok(()) | ||
} else { | ||
bail!("expected not found error, got {:?}", err); | ||
} | ||
} | ||
} | ||
}); | ||
|
||
// If the container creation succeeds, we expect an error since the masked paths does not support symlinks. | ||
if let TestResult::Passed = res { | ||
TestResult::Failed(anyhow!( | ||
"expected error in container creation with invalid symlink, found no error" | ||
)) | ||
} else { | ||
TestResult::Passed | ||
} | ||
} | ||
|
||
fn test_mode(mode: u32) -> TestResult { | ||
let root = PathBuf::from("/"); | ||
let masked_device = "masked_device"; | ||
let masked_paths = vec![root.join(masked_device).to_string_lossy().to_string()]; | ||
let spec = get_spec(masked_paths); | ||
|
||
test_inside_container(spec, &CreateOptions::default(), &|bundle_path| { | ||
use std::os::unix::fs::OpenOptionsExt; | ||
use std::{fs, io}; | ||
let test_file = bundle_path.join(masked_device); | ||
|
||
let mut opts = fs::OpenOptions::new(); | ||
opts.mode(mode); | ||
opts.create(true); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are correct. It seems that |
||
if let io::Result::Err(e) = fs::OpenOptions::new() | ||
.mode(mode) | ||
.create(true) | ||
.write(true) | ||
.open(&test_file) | ||
{ | ||
bail!( | ||
"could not create file {:?} with mode {:?} : {:?}", | ||
test_file, | ||
mode ^ 0o666, | ||
e | ||
); | ||
} | ||
|
||
match fs::metadata(&test_file) { | ||
io::Result::Ok(_) => Ok(()), | ||
io::Result::Err(e) => { | ||
let err = e.kind(); | ||
if let io::ErrorKind::NotFound = err { | ||
bail!("error in creating device node, {:?}", e) | ||
} else { | ||
Ok(()) | ||
} | ||
} | ||
} | ||
}) | ||
} | ||
|
||
fn check_masked_device_nodes() -> TestResult { | ||
let modes = [ | ||
SFlag::S_IFBLK.bits() | 0o666, | ||
SFlag::S_IFCHR.bits() | 0o666, | ||
SFlag::S_IFIFO.bits() | 0o666, | ||
]; | ||
for mode in modes { | ||
let res = test_mode(mode); | ||
if let TestResult::Failed(_) = res { | ||
return res; | ||
} | ||
} | ||
TestResult::Passed | ||
utam0k marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
pub fn get_linux_masked_paths_tests() -> TestGroup { | ||
let mut tg = TestGroup::new("masked_paths"); | ||
let masked_paths_test = Test::new("masked_paths", Box::new(check_masked_paths)); | ||
let masked_rel_paths_test = Test::new("masked_rel_paths", Box::new(check_masked_rel_paths)); | ||
let masked_symlinks_test = Test::new("masked_symlinks", Box::new(check_masked_symlinks)); | ||
let masked_device_nodes_test = | ||
Test::new("masked_device_nodes", Box::new(check_masked_device_nodes)); | ||
tg.add(vec![ | ||
Box::new(masked_paths_test), | ||
Box::new(masked_rel_paths_test), | ||
Box::new(masked_symlinks_test), | ||
Box::new(masked_device_nodes_test), | ||
]); | ||
tg | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
mod masked_paths; | ||
|
||
pub use masked_paths::get_linux_masked_paths_tests; |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -820,3 +820,46 @@ pub fn validate_fd_control(_spec: &Spec) { | |||||
eprintln!("mismatched fds inside container! {:?}", fd_details); | ||||||
} | ||||||
} | ||||||
|
||||||
pub fn validate_masked_paths(spec: &Spec) { | ||||||
let linux = spec.linux().as_ref().unwrap(); | ||||||
let masked_paths = match linux.masked_paths() { | ||||||
Some(p) => p, | ||||||
None => { | ||||||
eprintln!("in masked paths, expected some masked paths to be set, found none"); | ||||||
return; | ||||||
} | ||||||
}; | ||||||
|
||||||
if masked_paths.is_empty() { | ||||||
eprintln!("in masked paths, expected some masked paths to be set, found none"); | ||||||
return; | ||||||
} | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need it. |
||||||
|
||||||
for path_str in masked_paths { | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about handling
Suggested change
Following the above change, we'll amend some functions, like:
|
||||||
let path = Path::new(path_str); | ||||||
if !path.is_absolute() { | ||||||
eprintln!("in masked paths, the path must be absolute.") | ||||||
} | ||||||
match test_read_access(path_str) { | ||||||
Ok(true) => { | ||||||
eprintln!( | ||||||
"in masked paths, expected path {path_str} to be masked, but was found readable" | ||||||
); | ||||||
return; | ||||||
} | ||||||
Ok(false) => { /* This is expected */ } | ||||||
Err(e) => { | ||||||
let errno = Errno::from_raw(e.raw_os_error().unwrap()); | ||||||
if errno == Errno::ENOENT { | ||||||
/* This is expected */ | ||||||
} else { | ||||||
eprintln!( | ||||||
"in masked paths, error in testing read access for path {path_str} : {errno:?}" | ||||||
); | ||||||
return; | ||||||
} | ||||||
} | ||||||
} | ||||||
} | ||||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,22 +1,46 @@ | ||
use std::fs; | ||
use std::fs::{metadata, symlink_metadata}; | ||
use std::fs::{metadata, symlink_metadata, OpenOptions}; | ||
use std::io::Read; | ||
use std::os::unix::prelude::MetadataExt; | ||
use std::path::PathBuf; | ||
use std::process::Command; | ||
use std::{fs, io}; | ||
|
||
use nix::sys::stat::{stat, SFlag}; | ||
|
||
fn test_file_read_access(path: &str) -> Result<(), std::io::Error> { | ||
let _ = std::fs::OpenOptions::new() | ||
.create(false) | ||
.read(true) | ||
.open(path)?; | ||
Ok(()) | ||
fn test_file_read_access(path: &str) -> Result<bool, std::io::Error> { | ||
let mut file = OpenOptions::new().create(false).read(true).open(path)?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. May I ask you to make the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was an implementation mistake. The following code seems unnecessary: Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => Ok(false), I have addressed this in ed15bf7. Is everything clear now? If additional docstrings are needed, please let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please add a comment about the return value or use a type alias. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I fixed it in commit b4e3511. |
||
|
||
// Create a buffer with a capacity of 1 byte | ||
let mut buffer = [0u8; 1]; | ||
match file.read(&mut buffer) { | ||
// Our contest tests only use non-empty files for read-access | ||
// tests. So if we get an EOF on the first read or zero bytes, the runtime did | ||
// successfully block readability. | ||
Ok(0) => Ok(false), | ||
Ok(_) => Ok(true), | ||
Err(e) if e.kind() == io::ErrorKind::UnexpectedEof => Ok(false), | ||
Err(e) => Err(e), | ||
} | ||
} | ||
|
||
pub fn test_dir_read_access(path: &str) -> Result<(), std::io::Error> { | ||
let _ = std::fs::read_dir(path)?; | ||
Ok(()) | ||
pub fn test_dir_read_access(path: &str) -> Result<bool, std::io::Error> { | ||
let entries = std::fs::read_dir(path); | ||
|
||
match entries { | ||
Ok(mut entries_iter) => { | ||
// Get the first entry | ||
match entries_iter.next() { | ||
Some(entry) => { | ||
match entry { | ||
Ok(_) => Ok(true), // If the entry is Ok, then it's readable | ||
Err(_) => Ok(false), // If the entry is Err, then it's not readable | ||
} | ||
} | ||
None => Ok(false), // If there's an error, then it's not readable, or otherwise, it may indicate different conditions. | ||
} | ||
} | ||
Err(e) => Err(e), | ||
} | ||
} | ||
|
||
fn is_file_like(mode: u32) -> bool { | ||
|
@@ -30,7 +54,7 @@ fn is_dir(mode: u32) -> bool { | |
mode & SFlag::S_IFDIR.bits() != 0 | ||
} | ||
|
||
pub fn test_read_access(path: &str) -> Result<(), std::io::Error> { | ||
pub fn test_read_access(path: &str) -> Result<bool, std::io::Error> { | ||
let fstat = stat(path)?; | ||
let mode = fstat.st_mode; | ||
if is_file_like(mode) { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.