Merge pull request #7845 from sylvestre/selinux-error

set_selinux_security_context should return an Error, not String

Author: cakebaker

src/uu/mkdir/src/mkdir.rs

@@ -298,10 +298,7 @@ fn create_dir(path: &Path, is_parent: bool, config: &Config) -> UResult<()> {
                 if let Err(e) = uucore::selinux::set_selinux_security_context(path, config.context)
                 {
                     let _ = std::fs::remove_dir(path);
-                    return Err(USimpleError::new(
-                        1,
-                        format!("failed to set SELinux security context: {}", e),
-                    ));
+                    return Err(USimpleError::new(1, e.to_string()));
                 }
             }
 

src/uucore/Cargo.toml

@@ -114,7 +114,7 @@ proc-info = ["tty", "walkdir"]
 quoting-style = []
 ranges = []
 ringbuffer = []
-selinux = ["dep:selinux"]
+selinux = ["dep:selinux", "thiserror"]
 signals = []
 sum = [
   "digest",

src/uucore/src/lib/features/selinux.rs

@@ -6,22 +6,34 @@
 use std::path::Path;
 
 use selinux::SecurityContext;
+use thiserror::Error;
 
-#[derive(Debug)]
-pub enum Error {
+#[derive(Debug, Error)]
+pub enum SeLinuxError {
+    #[error("SELinux is not enabled on this system")]
     SELinuxNotEnabled,
-    FileOpenFailure,
-    ContextRetrievalFailure,
-    ContextConversionFailure,
+
+    #[error("Failed to open the file: {0}")]
+    FileOpenFailure(String),
+
+    #[error("Failed to retrieve the security context: {0}")]
+    ContextRetrievalFailure(String),
+
+    #[error("Failed to set default file creation context to '{0}': {1}")]
+    ContextSetFailure(String, String),
+
+    #[error("Failed to set default file creation context to '{0}': {1}")]
+    ContextConversionFailure(String, String),
 }
 
-impl From<Error> for i32 {
-    fn from(error: Error) -> i32 {
+impl From<SeLinuxError> for i32 {
+    fn from(error: SeLinuxError) -> i32 {
         match error {
-            Error::SELinuxNotEnabled => 1,
-            Error::FileOpenFailure => 2,
-            Error::ContextRetrievalFailure => 3,
-            Error::ContextConversionFailure => 4,
+            SeLinuxError::SELinuxNotEnabled => 1,
+            SeLinuxError::FileOpenFailure(_) => 2,
+            SeLinuxError::ContextRetrievalFailure(_) => 3,
+            SeLinuxError::ContextSetFailure(_, _) => 4,
+            SeLinuxError::ContextConversionFailure(_, _) => 5,
         }
     }
 }
@@ -76,31 +88,39 @@ pub fn is_selinux_enabled() -> bool {
 ///     eprintln!("Failed to set context: {}", err);
 /// }
 /// ```
-pub fn set_selinux_security_context(path: &Path, context: Option<&String>) -> Result<(), String> {
+pub fn set_selinux_security_context(
+    path: &Path,
+    context: Option<&String>,
+) -> Result<(), SeLinuxError> {
     if !is_selinux_enabled() {
-        return Err("SELinux is not enabled on this system".to_string());
+        return Err(SeLinuxError::SELinuxNotEnabled);
     }
 
     if let Some(ctx_str) = context {
         // Create a CString from the provided context string
-        let c_context = std::ffi::CString::new(ctx_str.as_str())
-            .map_err(|_| "Invalid context string (contains null bytes)".to_string())?;
+        let c_context = std::ffi::CString::new(ctx_str.as_str()).map_err(|e| {
+            SeLinuxError::ContextConversionFailure(ctx_str.to_string(), e.to_string())
+        })?;
 
         // Convert the CString into an SELinux security context
-        let security_context = selinux::OpaqueSecurityContext::from_c_str(&c_context)
-            .map_err(|e| format!("Failed to create security context: {}", e))?;
+        let security_context =
+            selinux::OpaqueSecurityContext::from_c_str(&c_context).map_err(|e| {
+                SeLinuxError::ContextConversionFailure(ctx_str.to_string(), e.to_string())
+            })?;
 
         // Set the provided security context on the specified path
         SecurityContext::from_c_str(
-            &security_context.to_c_string().map_err(|e| e.to_string())?,
+            &security_context.to_c_string().map_err(|e| {
+                SeLinuxError::ContextConversionFailure(ctx_str.to_string(), e.to_string())
+            })?,
             false,
         )
         .set_for_path(path, false, false)
-        .map_err(|e| format!("Failed to set context: {}", e))
+        .map_err(|e| SeLinuxError::ContextSetFailure(ctx_str.to_string(), e.to_string()))
     } else {
         // If no context provided, set the default SELinux context for the path
         SecurityContext::set_default_for_path(path)
-            .map_err(|e| format!("Failed to set default context: {}", e))
+            .map_err(|e| SeLinuxError::ContextSetFailure(String::new(), e.to_string()))
     }
 }
 
@@ -117,17 +137,18 @@ pub fn set_selinux_security_context(path: &Path, context: Option<&String>) -> Re
 ///
 /// * `Ok(String)` - The SELinux context string if successfully retrieved. Returns an empty
 ///   string if no context was found.
-/// * `Err(Error)` - An error variant indicating the type of failure:
-///   - `Error::SELinuxNotEnabled` - SELinux is not enabled on the system.
-///   - `Error::FileOpenFailure` - Failed to open the specified file.
-///   - `Error::ContextRetrievalFailure` - Failed to retrieve the security context.
-///   - `Error::ContextConversionFailure` - Failed to convert the security context to a string.
+/// * `Err(SeLinuxError)` - An error variant indicating the type of failure:
+///   - `SeLinuxError::SELinuxNotEnabled` - SELinux is not enabled on the system.
+///   - `SeLinuxError::FileOpenFailure` - Failed to open the specified file.
+///   - `SeLinuxError::ContextRetrievalFailure` - Failed to retrieve the security context.
+///   - `SeLinuxError::ContextConversionFailure` - Failed to convert the security context to a string.
+///   - `SeLinuxError::ContextSetFailure` - Failed to set the security context.
 ///
 /// # Examples
 ///
 /// ```
 /// use std::path::Path;
-/// use uucore::selinux::{get_selinux_security_context, Error};
+/// use uucore::selinux::{get_selinux_security_context, SeLinuxError};
 ///
 /// // Get the SELinux context for a file
 /// match get_selinux_security_context(Path::new("/path/to/file")) {
@@ -138,29 +159,30 @@ pub fn set_selinux_security_context(path: &Path, context: Option<&String>) -> Re
 ///             println!("SELinux context: {}", context);
 ///         }
 ///     },
-///     Err(Error::SELinuxNotEnabled) => println!("SELinux is not enabled on this system"),
-///     Err(Error::FileOpenFailure) => println!("Failed to open the file"),
-///     Err(Error::ContextRetrievalFailure) => println!("Failed to retrieve the security context"),
-///     Err(Error::ContextConversionFailure) => println!("Failed to convert the security context to a string"),
+///     Err(SeLinuxError::SELinuxNotEnabled) => println!("SELinux is not enabled on this system"),
+///     Err(SeLinuxError::FileOpenFailure(e)) => println!("Failed to open the file: {}", e),
+///     Err(SeLinuxError::ContextRetrievalFailure(e)) => println!("Failed to retrieve the security context: {}", e),
+///     Err(SeLinuxError::ContextConversionFailure(ctx, e)) => println!("Failed to convert context '{}': {}", ctx, e),
+///     Err(SeLinuxError::ContextSetFailure(ctx, e)) => println!("Failed to set context '{}': {}", ctx, e),
 /// }
 /// ```
-pub fn get_selinux_security_context(path: &Path) -> Result<String, Error> {
+pub fn get_selinux_security_context(path: &Path) -> Result<String, SeLinuxError> {
     if !is_selinux_enabled() {
-        return Err(Error::SELinuxNotEnabled);
+        return Err(SeLinuxError::SELinuxNotEnabled);
     }
 
-    let f = std::fs::File::open(path).map_err(|_| Error::FileOpenFailure)?;
+    let f = std::fs::File::open(path).map_err(|e| SeLinuxError::FileOpenFailure(e.to_string()))?;
 
     // Get the security context of the file
     let context = match SecurityContext::of_file(&f, false) {
         Ok(Some(ctx)) => ctx,
         Ok(None) => return Ok(String::new()), // No context found, return empty string
-        Err(_) => return Err(Error::ContextRetrievalFailure),
+        Err(e) => return Err(SeLinuxError::ContextRetrievalFailure(e.to_string())),
     };
 
     let context_c_string = context
         .to_c_string()
-        .map_err(|_| Error::ContextConversionFailure)?;
+        .map_err(|e| SeLinuxError::ContextConversionFailure(String::new(), e.to_string()))?;
 
     if let Some(c_str) = context_c_string {
         // Convert the C string to a Rust String
@@ -180,29 +202,50 @@ mod tests {
         let tmpfile = NamedTempFile::new().expect("Failed to create tempfile");
         let path = tmpfile.path();
 
-        let result = set_selinux_security_context(path, None);
+        if !is_selinux_enabled() {
+            let result = set_selinux_security_context(path, None);
+            assert!(result.is_err(), "Expected error when SELinux is disabled");
+            match result.unwrap_err() {
+                SeLinuxError::SELinuxNotEnabled => {
+                    // This is the expected error when SELinux is not enabled
+                }
+                err => panic!("Expected SELinuxNotEnabled error but got: {}", err),
+            }
+            return;
+        }
 
-        if result.is_ok() {
-            // SELinux enabled and successfully set default context
-            assert!(true, "Successfully set SELinux context");
-        } else {
-            let err = result.unwrap_err();
-            let valid_errors = [
-                "SELinux is not enabled on this system",
-                &format!(
-                    "Failed to set default context: selinux_lsetfilecon_default() failed on path '{}'",
-                    path.display()
-                ),
-            ];
+        let default_result = set_selinux_security_context(path, None);
+        assert!(
+            default_result.is_ok(),
+            "Failed to set default context: {:?}",
+            default_result.err()
+        );
+
+        let context = get_selinux_security_context(path).expect("Failed to get context");
+        assert!(
+            !context.is_empty(),
+            "Expected non-empty context after setting default context"
+        );
+
+        let test_context = String::from("system_u:object_r:tmp_t:s0");
+        let explicit_result = set_selinux_security_context(path, Some(&test_context));
+
+        if explicit_result.is_ok() {
+            let new_context = get_selinux_security_context(path)
+                .expect("Failed to get context after setting explicit context");
 
             assert!(
-                valid_errors.contains(&err.as_str()),
-                "Unexpected error message: {}",
-                err
+                new_context.contains("tmp_t"),
+                "Expected context to contain 'tmp_t', but got: {}",
+                new_context
+            );
+        } else {
+            println!(
+                "Note: Could not set explicit context {:?}",
+                explicit_result.err()
             );
         }
     }
-
     #[test]
     fn test_invalid_context_string_error() {
         let tmpfile = NamedTempFile::new().expect("Failed to create tempfile");
@@ -213,10 +256,18 @@ mod tests {
         let result = set_selinux_security_context(path, Some(&invalid_context));
 
         assert!(result.is_err());
-        assert_eq!(
-            result.unwrap_err(),
-            "Invalid context string (contains null bytes)"
-        );
+        if let Err(err) = result {
+            match err {
+                SeLinuxError::ContextConversionFailure(ctx, msg) => {
+                    assert_eq!(ctx, "invalid\0context");
+                    assert!(
+                        msg.contains("nul byte"),
+                        "Error message should mention nul byte"
+                    );
+                }
+                _ => panic!("Expected ContextConversionFailure error but got: {}", err),
+            }
+        }
     }
 
     #[test]
@@ -243,17 +294,56 @@ mod tests {
         let result = get_selinux_security_context(path);
 
         if result.is_ok() {
-            println!("Retrieved SELinux context: {}", result.unwrap());
+            let context = result.unwrap();
+            println!("Retrieved SELinux context: {}", context);
+
+            assert!(
+                is_selinux_enabled(),
+                "Got a successful context result but SELinux is not enabled"
+            );
+
+            if !context.is_empty() {
+                assert!(
+                    context.contains(':'),
+                    "SELinux context '{}' doesn't match expected format",
+                    context
+                );
+            }
         } else {
             let err = result.unwrap_err();
 
-            // Valid error types
             match err {
-                Error::SELinuxNotEnabled => assert!(true, "SELinux not supported"),
-                Error::ContextRetrievalFailure => assert!(true, "Context retrieval failure"),
-                Error::ContextConversionFailure => assert!(true, "Context conversion failure"),
-                Error::FileOpenFailure => {
-                    panic!("File open failure occurred despite file being created")
+                SeLinuxError::SELinuxNotEnabled => {
+                    assert!(
+                        !is_selinux_enabled(),
+                        "Got SELinuxNotEnabled error, but is_selinux_enabled() returned true"
+                    );
+                }
+                SeLinuxError::ContextRetrievalFailure(e) => {
+                    assert!(
+                        is_selinux_enabled(),
+                        "Got ContextRetrievalFailure when SELinux is not enabled"
+                    );
+                    assert!(!e.is_empty(), "Error message should not be empty");
+                    println!("Context retrieval failure: {}", e);
+                }
+                SeLinuxError::ContextConversionFailure(ctx, e) => {
+                    assert!(
+                        is_selinux_enabled(),
+                        "Got ContextConversionFailure when SELinux is not enabled"
+                    );
+                    assert!(!e.is_empty(), "Error message should not be empty");
+                    println!("Context conversion failure for '{}': {}", ctx, e);
+                }
+                SeLinuxError::ContextSetFailure(ctx, e) => {
+                    assert!(false);
+                }
+                SeLinuxError::FileOpenFailure(e) => {
+                    assert!(
+                        Path::new(path).exists(),
+                        "File open failure occurred despite file being created: {}",
+                        e
+                    );
                 }
             }
         }
@@ -266,9 +356,16 @@ mod tests {
         let result = get_selinux_security_context(path);
 
         assert!(result.is_err());
-        assert!(
-            matches!(result.unwrap_err(), Error::FileOpenFailure),
-            "Expected file open error for nonexistent file"
-        );
+        if let Err(err) = result {
+            match err {
+                SeLinuxError::FileOpenFailure(e) => {
+                    assert!(
+                        e.contains("No such file"),
+                        "Error should mention file not found"
+                    );
+                }
+                _ => panic!("Expected FileOpenFailure error but got: {}", err),
+            }
+        }
     }
 }

tests/by-util/test_mkdir.rs

@@ -411,7 +411,7 @@ fn test_selinux_invalid() {
         .arg(at.plus_as_string(dest))
         .fails()
         .no_stdout()
-        .stderr_contains("failed to set SELinux security context:");
+        .stderr_contains("Failed to set default file creation context to 'testtest':");
     // invalid context, so, no directory
     assert!(!at.dir_exists(dest));
 }

tests/by-util/test_mkfifo.rs

@@ -160,7 +160,7 @@ fn test_mkfifo_selinux_invalid() {
             .arg(arg)
             .arg(dest)
             .fails()
-            .stderr_contains("Failed to");
+            .stderr_contains("failed to");
         if at.file_exists(dest) {
             at.remove(dest);
         }

tests/by-util/test_mknod.rs

@@ -187,7 +187,7 @@ fn test_mknod_selinux_invalid() {
             .arg(dest)
             .arg("p")
             .fails()
-            .stderr_contains("Failed to");
+            .stderr_contains("failed to");
         if at.file_exists(dest) {
             at.remove(dest);
         }