Repository: NordicHPC/sonar (language: python)

Issue:
Supply chain security / simplify dependencies
This has been on my mind for a while but the recent exploitation of xz makes it a good time to talk about it.  Sonar runs (or will run) on every node on almost every supercomputer in Norway.  It pulls in not too many dependencies but some of them (clap, serde, libc) are quite large and actively maintained and some (subprocess, libc) are expected to do dark things by their nature.

https://news.ycombinator.com/item?id=39902241
https://research.swtch.com/xz-timeline

serde is an interesting case in point because for a while (possibly still but I've not been able to substantiate that) it shipped a precompiled binary in order to speed up compilation time, https://github.com/serde-rs/serde/issues/2538.  With this type of setup, vetting what we pull in and how it works is very hard.

Sonar actually has very simple needs and most of its libraries have been pulled in not because we need the full functionality but only a small part of it.  It may be to our benefit to go over the list of dependencies, remove what we can, and perhaps audit and pin what we can't remove, and define some kind of upgrade protocol.  For example, we have simple needs for csv and json production and for command line parsing and could easily replace those libraries if we wanted to.

I'm uncertain about how worried to be about this.  Sonar *currently* runs with no special privileges and its only superpowers are that (a) it runs regularly and everywhere and (b) if an admin sees it running, she will think it is fine that it's running.  It is otherwise no worse than any user program, indeed user programs whose source and binary we do not control.  On the other hand, being everywhere and running all the time under the radar it is a very convenient stepping stone for a more sophisticated attack leveraging other mechanisms.  And it is not out of the realm of the possible that Sonar will need some special powers eventually, for some types of monitoring.  And if not Sonar, then maybe a program that Sonar runs - same problem.


Discussion / hints:
Great point. Let's go together through list of dependencies and let's check what we use them for and how much work it would be to replace some of them.

Fix patch:
diff --git a/Cargo.lock b/Cargo.lock
index 1cdabe9..a0ef9ec 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -140,12 +140,6 @@ version = "2.1.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "9a3a5bfb195931eeb336b2a7b4d761daec841b97f947d34394601737a7bba5e4"
 
-[[package]]
-name = "itoa"
-version = "1.0.10"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b1a46d1a171d865aa5f83f92695765caa047a9b4cbae2cbf37dbd613a793fd4c"
-
 [[package]]
 name = "libc"
 version = "0.2.153"
@@ -211,43 +205,6 @@ version = "0.8.2"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "c08c74e62047bb2de4ff487b251e4a92e24f48745648451635cec7d591162d9f"
 
-[[package]]
-name = "ryu"
-version = "1.0.16"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "f98d2aa92eebf49b69786be48e4477826b256916e84a57ff2a4f21923b48eb4c"
-
-[[package]]
-name = "serde"
-version = "1.0.197"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "3fb1c873e1b9b056a4dc4c0c198b24c3ffa059243875552b2bd0933b1aee4ce2"
-dependencies = [
- "serde_derive",
-]
-
-[[package]]
-name = "serde_derive"
-version = "1.0.197"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "7eb0b34b42edc17f6b7cac84a52a1c5f0e1bb2227e997ca9011ea3dd34e8610b"
-dependencies = [
- "proc-macro2",
- "quote",
- "syn",
-]
-
-[[package]]
-name = "serde_json"
-version = "1.0.114"
-source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "c5f09b1bd632ef549eaa9f60a1f8de742bdbc698e6cee2095fc84dde5f549ae0"
-dependencies = [
- "itoa",
- "ryu",
- "serde",
-]
-
 [[package]]
 name = "signal-hook"
 version = "0.3.17"
@@ -275,8 +232,6 @@ dependencies = [
  "env_logger",
  "libc",
  "log",
- "serde",
- "serde_json",
  "signal-hook",
  "subprocess",
 ]
diff --git a/Cargo.toml b/Cargo.toml
index aa01b2f..04abfd9 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -12,5 +12,3 @@ log = "0.4"
 env_logger = "0.11"
 libc = "0.2"
 signal-hook = { version = "0.3", default-features = false, features = [] }
-serde_json = "1.0.114"
-serde = { version = "1.0.197", features = ["derive"] }
diff --git a/src/sysinfo.rs b/src/sysinfo.rs
index afc0af6..95e80bd 100644
--- a/src/sysinfo.rs
+++ b/src/sysinfo.rs
@@ -6,14 +6,14 @@ use crate::hostname;
 use crate::nvidia;
 use crate::procfs;
 use crate::procfsapi;
+use crate::util;
 
-use serde::Serialize;
-
-use std::io::{self, Write};
+use std::io;
 
 pub fn show_system(timestamp: &str) {
     let fs = procfsapi::RealFS::new();
-    match do_show_system(&fs, timestamp) {
+    let mut writer = io::stdout();
+    match do_show_system(&mut writer, &fs, timestamp) {
         Ok(_) => {}
         Err(e) => {
             log::error!("sysinfo failed: {e}");
@@ -23,22 +23,7 @@ pub fn show_system(timestamp: &str) {
 
 const GIB: usize = 1024 * 1024 * 1024;
 
-// Note the field names here are used by decoders developed separately and should be considered set
-// in stone.  All fields will be serialized; missing numeric values must be zero; consumers must
-// deal with that.
-
-#[derive(Serialize)]
-struct NodeConfig {
-    timestamp: String,
-    hostname: String,
-    description: String,
-    cpu_cores: i32,
-    mem_gb: i64, // This is GiB despite the name - the name is frozen
-    gpu_cards: i32,
-    gpumem_gb: i64, // Ditto
-}
-
-fn do_show_system(fs: &dyn procfsapi::ProcfsAPI, timestamp: &str) -> Result<(), String> {
+fn do_show_system(writer: &mut dyn io::Write, fs: &dyn procfsapi::ProcfsAPI, timestamp: &str) -> Result<(), String> {
     let (model, sockets, cores_per_socket, threads_per_core) = procfs::get_cpu_info(fs)?;
     let mem_by = procfs::get_memtotal_kib(fs)? * 1024;
     let mem_gib = (mem_by as f64 / GIB as f64).round() as i64;
@@ -91,22 +76,32 @@ fn do_show_system(fs: &dyn procfsapi::ProcfsAPI, timestamp: &str) -> Result<(),
     } else {
         ("".to_string(), 0, 0)
     };
-    let config = NodeConfig {
-        timestamp: timestamp.to_string(),
-        hostname,
-        description: format!("{sockets}x{cores_per_socket}{ht} {model}, {mem_gib} GiB{gpu_desc}"),
-        cpu_cores: sockets * cores_per_socket * threads_per_core,
-        mem_gb: mem_gib,
-        gpu_cards,
-        gpumem_gb,
-    };
-    match serde_json::to_string_pretty(&config) {
-        Ok(s) => {
-            let _ = io::stdout().write(s.as_bytes());
-            let _ = io::stdout().write(b"\n");
-            let _ = io::stdout().flush();
-            Ok(())
-        }
-        Err(_) => Err("JSON encoding failed".to_string()),
-    }
+    let timestamp = util::json_quote(timestamp);
+    let hostname = util::json_quote(&hostname);
+    let description = util::json_quote(&format!("{sockets}x{cores_per_socket}{ht} {model}, {mem_gib} GiB{gpu_desc}"));
+    let cpu_cores = sockets * cores_per_socket * threads_per_core;
+
+    // Note the field names here are used by decoders that are developed separately, and they should
+    // be considered set in stone.
+
+    let s = format!(r#"{{
+  "timestamp": "{timestamp}",
+  "hostname": "{hostname}",
+  "description": "{description}",
+  "cpu_cores": {cpu_cores},
+  "mem_gb": {mem_gib},
+  "gpu_cards": {gpu_cards},
+  "gpumem_gb": {gpumem_gb}
+}}
+"#);
+
+    // Ignore I/O errors.
+
+    let _ = writer.write(s.as_bytes());
+    let _ = writer.flush();
+    Ok(())
 }
+
+// Currently the test for do_show_system() is black-box, see ../tests.  The reason for this is partly
+// that not all the system interfaces used by that function are virtualized at this time, and partly
+// that we only care that the output syntax looks right.
diff --git a/src/util.rs b/src/util.rs
index 330fb09..7c05ff9 100644
--- a/src/util.rs
+++ b/src/util.rs
@@ -64,6 +64,48 @@ pub fn three_places(n: f64) -> f64 {
     (n * 1000.0).round() / 1000.0
 }
 
+// Insert \ before " and \
+// Insert escape sequences for well-known control chars.
+// Translate all other control chars to spaces (it's possible to do better).
+pub fn json_quote(s: &str) -> String {
+    let mut t = "".to_string();
+    for c in s.chars() {
+        match c {
+            '"' | '\\' => {
+                t.push('\\');
+                t.push(c);
+            }
+            '\n' => {
+                t.push_str("\\n");
+            }
+            '\r' => {
+                t.push_str("\\r");
+            }
+            '\t' => {
+                t.push_str("\\t");
+            }
+            _ctl if c < ' ' => {
+                t.push(' ');
+            }
+            _ => {
+                t.push(c);
+            }
+        }
+    }
+    t
+}
+
+#[test]
+pub fn json_quote_test() {
+    assert!(&json_quote("abcde") == "abcde");
+    assert!(&json_quote(r#"abc\de"#) == r#"abc\\de"#);
+    assert!(&json_quote(r#"abc"de"#) == r#"abc\"de"#);
+    assert!(&json_quote("abc\nde") == r#"abc\nde"#);
+    assert!(&json_quote("abc\rde") == r#"abc\rde"#);
+    assert!(&json_quote("abc	de") == r#"abc\tde"#);
+    assert!(&json_quote("abc\u{0008}de") == r#"abc de"#);
+}
+
 // If the value contains a , or " then quote the string, and double every "
 pub fn csv_quote(s: &str) -> String {
     let mut t = "".to_string();


Test patch:
diff --git a/tests/sysinfo-syntax.sh b/tests/sysinfo-syntax.sh
new file mode 100755
index 0000000..2769281
--- /dev/null
+++ b/tests/sysinfo-syntax.sh
@@ -0,0 +1,13 @@
+#!/bin/bash
+#
+# Check that sysinfo produces properly formatted JSON.
+# Requirement: the `jq` utility.
+
+set -e
+( cd .. ; cargo build )
+if [[ $(command -v jq) == "" ]]; then
+    echo "Install jq first"
+    exit 1
+fi
+output=$(../target/debug/sonar sysinfo)
+jq . <<< $output > /dev/null