Address comments

karencfv · Apr 15, 2024 · 91732b7 · 91732b7
1 parent a285290
commit 91732b7
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 30 deletions.
diff --git a/smf/chrony-setup/manifest.xml b/smf/chrony-setup/manifest.xml
@@ -13,7 +13,13 @@
 
   <exec_method type='method' name='start'
     exec='/opt/oxide/zone-setup-cli/bin/zone-setup chrony-setup -b %{config/boundary} -s %{config/server} -a %{config/allow}'
-    timeout_seconds='0' />
+    timeout_seconds='0'>
+  <method_context security_flags="aslr">
+  <method_credential user="root" group="root"
+    privileges="basic,file_chown" />
+  </method_context>
+  </exec_method>
+
 
   <property_group name='startd' type='framework'>
     <propval name='duration' type='astring' value='transient' />

diff --git a/smf/ntp/manifest/manifest.xml b/smf/ntp/manifest/manifest.xml
@@ -65,7 +65,7 @@
 
   <exec_method type="method" name="start" 
     exec='/usr/sbin/chronyd -d &amp;' 
-    timeout_seconds="600">
+    timeout_seconds="60">
   <method_context security_flags="aslr">
   <method_credential user="root" group="root"
     privileges="basic,!file_link_any,!proc_info,!proc_session,file_chown_self,file_dac_search,file_dac_write,net_privaddr,proc_lock_memory,proc_priocntl,proc_setid,sys_time" />

diff --git a/zone-setup/src/bin/zone-setup.rs b/zone-setup/src/bin/zone-setup.rs
@@ -150,26 +150,27 @@ async fn do_run() -> Result<(), CmdError> {
                 .arg(
                     arg!(-f --file <String> "Chrony configuration file")
                     .default_value(CHRONY_CONFIG_FILE)
-            .value_parser(parse_chrony_conf)
+                    .value_parser(parse_chrony_conf)
                 )
-            .arg(
-                arg!(-b --boundary <bool> "Whether this is a boundary or internal NTP zone")
-                .required(true)
-        .value_parser(parse_boundary),
-            )
-            .arg(
-                Arg::new("servers")
-                .short('s')
-                .long("servers")
-                .num_args(1..)
-                .value_delimiter(' ')
-                .value_parser(value_parser!(String))
-                .help("List of NTP servers separated by a space")
-                .required(true)
-            )
-            .arg(
-                arg!(-a --allow <String> "Allowed IPv6 range")                       
-            ),
+                .arg(
+                    arg!(-b --boundary <bool> "Whether this is a boundary or internal NTP zone")
+                    .required(true)
+                    .value_parser(parse_boundary),
+                )
+                .arg(
+                    Arg::new("servers")
+                    .short('s')
+                    .long("servers")
+                    .num_args(1..)
+                    .value_delimiter(' ')
+                    .value_parser(value_parser!(String))
+                    .help("List of NTP servers separated by a space")
+                    .required(true)
+                )
+                .arg(
+                    arg!(-a --allow <String> "Allowed IPv6 range")
+                    .num_args(0..=1)                     
+                ),
         )
         .get_matches();
 
@@ -203,15 +204,15 @@ async fn chrony_setup(
         servers, file, allow, is_boundary
     );
 
-    generate_chrony_config(&log, is_boundary, allow, file, servers).await?;
+    generate_chrony_config(&log, is_boundary, allow, file, servers)?;
 
     // The NTP zone delivers a logadm fragment into /etc/logadm.d/ that needs to
     // be added to the system's /etc/logadm.conf. Unfortunately, the service which
     // does this - system/logadm-upgrade - only processes files with mode 444 and
     // root:sys ownership so we need to adjust things here (until omicron package
     // supports including ownership and permissions in the generated tar files).
     info!(&log, "Setting mode 444 and root:sys ownership to logadm fragment file"; "logadm config" => ?LOGADM_CONFIG_FILE);
-    set_permissions_for_logadm_config().await?;
+    set_permissions_for_logadm_config()?;
 
     info!(&log, "Updating logadm"; "logadm config" => ?LOGADM_CONFIG_FILE);
     Svcadm::refresh_logadm_upgrade()
@@ -220,7 +221,7 @@ async fn chrony_setup(
     Ok(())
 }
 
-async fn set_permissions_for_logadm_config() -> Result<(), CmdError> {
+fn set_permissions_for_logadm_config() -> Result<(), CmdError> {
     let mut perms = metadata(LOGADM_CONFIG_FILE)
         .map_err(|err| {
             CmdError::Failure(anyhow!(
@@ -250,7 +251,7 @@ async fn set_permissions_for_logadm_config() -> Result<(), CmdError> {
     Ok(())
 }
 
-async fn generate_chrony_config(
+fn generate_chrony_config(
     log: &Logger,
     is_boundary: &bool,
     allow: Option<&String>,
@@ -273,11 +274,15 @@ async fn generate_chrony_config(
             err
         ))
     })?;
-    let new_config = if *is_boundary {
-        let mut contents = contents.replace(
+
+    if let Some(allow) = allow {
+        contents = contents.replace(
             "@ALLOW@",
-            &allow.expect("Chrony allowed address not supplied").to_string(),
-        );
+            &allow.to_string(),
+        );    
+    }
+
+    let new_config = if *is_boundary {
         for s in servers {
             let str_line =
                 format!("pool {} iburst maxdelay 0.1 maxsources 16\n", s);
@@ -328,7 +333,7 @@ async fn generate_chrony_config(
 
     if old_file.clone().is_some_and(|f| f != new_config) {
         info!(&log, "Chrony configuration file has changed"; 
-        "old configuration file" => ?old_file);
+        "old configuration file" => ?old_file, "new configuration file" => ?new_config,);
     }
 
     Ok(())