Ensure logfile permissions are maintained after rotation (#7246)

Update our `logroller` dependency to the new version which supports permission control. See -> alasdairpan/logroller#6

Author: macladson

Cargo.toml

@@ -159,7 +159,7 @@ hyper = "1"
 itertools = "0.10"
 libsecp256k1 = "0.7"
 log = "0.4"
-logroller = "0.1.4"
+logroller = "0.1.8"
 lru = "0.12"
 maplit = "1"
 milhouse = "0.5"

common/logging/src/tracing_libp2p_discv5_logging_layer.rs

@@ -59,6 +59,7 @@ impl tracing_core::field::Visit for LogMessageExtractor {
 pub fn create_libp2p_discv5_tracing_layer(
     base_tracing_log_path: Option<PathBuf>,
     max_log_size: u64,
+    file_mode: u32,
 ) -> Option<Libp2pDiscv5TracingLayer> {
     if let Some(mut tracing_log_path) = base_tracing_log_path {
         // Ensure that `tracing_log_path` only contains directories.
@@ -75,12 +76,14 @@ pub fn create_libp2p_discv5_tracing_layer(
         let libp2p_writer =
             LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("libp2p.log"))
                 .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size)))
-                .max_keep_files(1);
+                .max_keep_files(1)
+                .file_mode(file_mode);
 
         let discv5_writer =
             LogRollerBuilder::new(tracing_log_path.clone(), PathBuf::from("discv5.log"))
                 .rotation(Rotation::SizeBased(RotationSize::MB(max_log_size)))
-                .max_keep_files(1);
+                .max_keep_files(1)
+                .file_mode(file_mode);
 
         let libp2p_writer = match libp2p_writer.build() {
             Ok(writer) => writer,

lcli/src/main.rs

@@ -675,6 +675,7 @@ fn run<E: EthSpec>(env_builder: EnvironmentBuilder<E>, matches: &ArgMatches) ->
                     extra_info: false,
                 },
                 "",
+                0o600,
             );
 
     let env = env_builder

lighthouse/environment/src/lib.rs

@@ -26,14 +26,7 @@ use types::{EthSpec, GnosisEthSpec, MainnetEthSpec, MinimalEthSpec};
 #[cfg(target_family = "unix")]
 use {
     futures::Future,
-    std::{
-        fs::{read_dir, set_permissions, Permissions},
-        os::unix::fs::PermissionsExt,
-        path::Path,
-        pin::Pin,
-        task::Context,
-        task::Poll,
-    },
+    std::{pin::Pin, task::Context, task::Poll},
     tokio::signal::unix::{signal, Signal, SignalKind},
 };
 
@@ -208,6 +201,7 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
         mut self,
         config: LoggerConfig,
         logfile_prefix: &str,
+        file_mode: u32,
     ) -> (
         Self,
         LoggingLayer,
@@ -220,9 +214,6 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
             _ => logfile_prefix,
         };
 
-        #[cfg(target_family = "unix")]
-        let file_mode = if config.is_restricted { 0o600 } else { 0o644 };
-
         let file_logging_layer = match config.path {
             None => {
                 eprintln!("No logfile path provided, logging to file is disabled");
@@ -239,17 +230,15 @@ impl<E: EthSpec> EnvironmentBuilder<E> {
                     .max_keep_files(config.max_log_number.try_into().unwrap_or_else(|e| {
                         eprintln!("Failed to convert max_log_number to u64: {}", e);
                         10
-                    }));
+                    }))
+                    .file_mode(file_mode);
 
                 if config.compression {
                     appender = appender.compression(Compression::Gzip);
                 }
 
                 match appender.build() {
                     Ok(file_appender) => {
-                        #[cfg(target_family = "unix")]
-                        set_logfile_permissions(&path, filename_prefix, file_mode);
-
                         let (writer, guard) = tracing_appender::non_blocking(file_appender);
                         Some(LoggingLayer::new(
                             writer,
@@ -543,37 +532,3 @@ impl Future for SignalFuture {
         }
     }
 }
-
-#[cfg(target_family = "unix")]
-fn set_logfile_permissions(log_dir: &Path, filename_prefix: &str, file_mode: u32) {
-    let newest = read_dir(log_dir)
-        .ok()
-        .into_iter()
-        .flat_map(|entries| entries.filter_map(Result::ok))
-        .filter_map(|entry| {
-            let path = entry.path();
-            let fname = path.file_name()?.to_string_lossy();
-            if path.is_file() && fname.starts_with(filename_prefix) && fname.ends_with(".log") {
-                let modified = entry.metadata().ok()?.modified().ok()?;
-                Some((path, modified))
-            } else {
-                None
-            }
-        })
-        .max_by_key(|(_path, mtime)| *mtime);
-
-    match newest {
-        Some((file, _mtime)) => {
-            if let Err(e) = set_permissions(&file, Permissions::from_mode(file_mode)) {
-                eprintln!("Failed to set permissions on {}: {}", file.display(), e);
-            }
-        }
-        None => {
-            eprintln!(
-                "Couldn't find a newly created logfile in {} matching prefix \"{}\".",
-                log_dir.display(),
-                filename_prefix
-            );
-        }
-    }
-}

lighthouse/environment/src/tracing_common.rs

@@ -33,8 +33,14 @@ pub fn construct_logger<E: EthSpec>(
     let subcommand_name = matches.subcommand_name();
     let logfile_prefix = subcommand_name.unwrap_or("lighthouse");
 
+    let file_mode = if logger_config.is_restricted {
+        0o600
+    } else {
+        0o644
+    };
+
     let (builder, stdout_logging_layer, file_logging_layer, sse_logging_layer_opt) =
-        environment_builder.init_tracing(logger_config.clone(), logfile_prefix);
+        environment_builder.init_tracing(logger_config.clone(), logfile_prefix, file_mode);
 
     let libp2p_discv5_layer = if let Some(subcommand_name) = subcommand_name {
         if subcommand_name == "beacon_node"
@@ -49,6 +55,7 @@ pub fn construct_logger<E: EthSpec>(
                 create_libp2p_discv5_tracing_layer(
                     logger_config.path.clone(),
                     logger_config.max_log_size,
+                    file_mode,
                 )
             }
         } else {