[pve-devel] [RFC PATCH installer] auto: allow a binary executable as the first boot executable

[Daniel Kral <d.kral@proxmox.com>]

As the initial use case for the first boot feature request [0] was for
running shell scripts, the auto installer retrieved the binary as a
`String`. Unfortunately, this tries to interpret binary data as UTF-8
and will transform 'invalid' characters to replacement characters [1].

This causes the auto-installer to create an invalid binary when fetching
from an URL, which will likely cause a segmentation fault and the binary
to be never removed by the systemd target, and error with a `stream did
not contain valid UTF-8` when fetching from the ISO image.

To allow binary executables to be used as a first boot executable, this
commit changes the fetching from being read as a `String` to being read
as a vector of bytes `Vec<u8>` to not interfere with the content of a
binary executable.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5579
[1] https://doc.rust-lang.org/src/alloc/string.rs.html#635

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
To verify the former, I've created two simple test executables to verify
that both were executed as expected:

```
#include <syslog.h>

int main() {
	int logprio = LOG_USER | LOG_INFO;
        syslog(logprio, "Hello from first-boot!");
        return 0;
}
```

```
#!/bin/bash

logger <<EOF
Hello from the first-boot script!
EOF
```

As described in the commit message, with an unpatched auto-installer,
the binary will fail to run when fetched from an URL or fail the
installation when fetched from the ISO image. The shell script works as
expected.

After applying the patch when booting the install ISO in debug mode, the
auto-installer correctly writes and execute the binary when fetched from
an URL (both HTTP and HTTPS) and the same works when fetched from ISO.

I've also tested two common error scenarios: When the HTTP is offline,
there's an human-readable error that the connection was refused (e.g.
host is online, cannot establish a connection). When the HTTP response
is missing the "Content-Length" header, it will be explicitly stated in
the error message.

 .../src/bin/proxmox-auto-installer.rs         | 12 ++++++---
 proxmox-installer-common/src/http.rs          | 25 +++++++++++++++----
 2 files changed, 28 insertions(+), 9 deletions(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index ece9a94..408fc0e 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -38,14 +38,18 @@ fn setup_first_boot_executable(first_boot: &FirstBootHookInfo) -> Result<()> {
         FirstBootHookSourceMode::FromUrl => {
             if let Some(url) = &first_boot.url {
                 info!("Fetching first-boot hook from {url} ..");
-                Some(http::get(url, first_boot.cert_fingerprint.as_deref())?)
+                Some(http::get_as_bytes(
+                    url,
+                    first_boot.cert_fingerprint.as_deref(),
+                    FIRST_BOOT_EXEC_MAX_SIZE,
+                )?)
             } else {
                 bail!("first-boot hook source set to URL, but none specified!");
             }
         }
-        FirstBootHookSourceMode::FromIso => Some(fs::read_to_string(format!(
-            "/cdrom/{FIRST_BOOT_EXEC_NAME}"
-        ))?),
+        FirstBootHookSourceMode::FromIso => {
+            Some(fs::read(format!("/cdrom/{FIRST_BOOT_EXEC_NAME}"))?)
+        }
     };
 
     if let Some(content) = content {
diff --git a/proxmox-installer-common/src/http.rs b/proxmox-installer-common/src/http.rs
index f4afe14..a748266 100644
--- a/proxmox-installer-common/src/http.rs
+++ b/proxmox-installer-common/src/http.rs
@@ -1,6 +1,7 @@
-use anyhow::Result;
+use anyhow::{bail, Result};
 use rustls::ClientConfig;
 use sha2::{Digest, Sha256};
+use std::io::Read;
 use std::sync::Arc;
 use ureq::{Agent, AgentBuilder};
 
@@ -53,12 +54,26 @@ fn build_agent(fingerprint: Option<&str>) -> Result<Agent> {
 /// # Arguments
 /// * `url` - URL to fetch
 /// * `fingerprint` - SHA256 cert fingerprint if certificate pinning should be used. Optional.
-pub fn get(url: &str, fingerprint: Option<&str>) -> Result<String> {
-    Ok(build_agent(fingerprint)?
+/// * `max_size` - Maximum amount of bytes that will be read.
+pub fn get_as_bytes(url: &str, fingerprint: Option<&str>, max_size: usize) -> Result<Vec<u8>> {
+    let mut result: Vec<u8> = Vec::new();
+
+    let response = build_agent(fingerprint)?
         .get(url)
         .timeout(std::time::Duration::from_secs(60))
-        .call()?
-        .into_string()?)
+        .call()?;
+
+    match response
+        .into_reader()
+        .take(max_size as u64)
+        .read_to_end(&mut result)
+    {
+        Err(ref err) if err.kind() == std::io::ErrorKind::UnexpectedEof => {
+            bail!("Unexpected end of line. Does the HTTP server provide a Content-Length header?")
+        }
+        Err(err) => bail!("Error while reading GET request: {err}"),
+        Ok(_) => Ok(result),
+    }
 }
 
 /// Issues a POST request with the payload (JSON). Optionally a SHA256 fingerprint can be used to
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel