* [pve-devel] [PATCH installer v2] auto: allow a binary executable as the first boot executable
@ 2024-12-11 9:24 Daniel Kral
2024-12-13 10:00 ` Christoph Heiss
0 siblings, 1 reply; 3+ messages in thread
From: Daniel Kral @ 2024-12-11 9:24 UTC (permalink / raw)
To: pve-devel
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, and error with a `stream did not contain valid UTF-8` error
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 `Vec<u8>` to not interfere with the content of the 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>
---
changes from v1:
- remove explicit handling of `std::io::ErrorKind::UnexpectedEof`
.../src/bin/proxmox-auto-installer.rs | 12 ++++++++----
proxmox-installer-common/src/http.rs | 18 ++++++++++++++----
2 files changed, 22 insertions(+), 8 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..a520c40 100644
--- a/proxmox-installer-common/src/http.rs
+++ b/proxmox-installer-common/src/http.rs
@@ -1,6 +1,7 @@
use anyhow::Result;
use rustls::ClientConfig;
use sha2::{Digest, Sha256};
+use std::io::Read;
use std::sync::Arc;
use ureq::{Agent, AgentBuilder};
@@ -53,12 +54,21 @@ 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()?;
+
+ response
+ .into_reader()
+ .take(max_size as u64)
+ .read_to_end(&mut result)?;
+
+ 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH installer v2] auto: allow a binary executable as the first boot executable
2024-12-11 9:24 [pve-devel] [PATCH installer v2] auto: allow a binary executable as the first boot executable Daniel Kral
@ 2024-12-13 10:00 ` Christoph Heiss
2024-12-18 7:29 ` Daniel Kral
0 siblings, 1 reply; 3+ messages in thread
From: Christoph Heiss @ 2024-12-13 10:00 UTC (permalink / raw)
To: Daniel Kral; +Cc: Proxmox VE development discussion
LGTM. Tested with a shell script and a compiled binary, w/ and w/o
sending the `Content-Length` header in the response.
The "Automated Installation" page needs to be updated though, to
reflect:
a) that either shell-scripts or compiled binaries can be used and
b) that the compiled binaries must be (obviously) x86_64 and best case
statically linked, as there otherwise no guarantees that it will run
on the target system and might just fail blindly.
Please consider this patch:
Tested-by: Christoph Heiss <c.heiss@proxmox.com>
Reviewed-by: Christoph Heiss <c.heiss@proxmox.com>
FWIW, just thinking out loud here: Maybe the fetching of the first-boot
hook should be moved into `proxmox-fetch-answer` altogether. In a sense
that `proxmox-fetch-answer` would fetch all the needed things in
advance, such that the auto-installer, post-hook etc. doesn't have to do
that anymore.
But that's really not pressing of any kind, rather a long-term thing.
On Wed Dec 11, 2024 at 10:24 AM CET, Daniel Kral wrote:
> 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, and error with a `stream did not contain valid UTF-8` error
> 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 `Vec<u8>` to not interfere with the content of the 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>
> ---
> changes from v1:
> - remove explicit handling of `std::io::ErrorKind::UnexpectedEof`
>
> .../src/bin/proxmox-auto-installer.rs | 12 ++++++++----
> proxmox-installer-common/src/http.rs | 18 ++++++++++++++----
> 2 files changed, 22 insertions(+), 8 deletions(-)
> [..]
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [pve-devel] [PATCH installer v2] auto: allow a binary executable as the first boot executable
2024-12-13 10:00 ` Christoph Heiss
@ 2024-12-18 7:29 ` Daniel Kral
0 siblings, 0 replies; 3+ messages in thread
From: Daniel Kral @ 2024-12-18 7:29 UTC (permalink / raw)
To: Christoph Heiss; +Cc: Proxmox VE development discussion
On 12/13/24 11:00, Christoph Heiss wrote:
> The "Automated Installation" page needs to be updated though, to
> reflect:
>
> a) that either shell-scripts or compiled binaries can be used and
> b) that the compiled binaries must be (obviously) x86_64 and best case
> statically linked, as there otherwise no guarantees that it will run
> on the target system and might just fail blindly.
Thanks for taking a second look at this! I've added the following to the
"Automated Installation" page:
```diff
@@ -233,7 +233,7 @@
=== First Boot Hook Section ===
-Optional. It can be used to configure a script to run on the first boot
of the new system after a successful installation.
+Optional. It can be used to configure a shell script or a compiled
binary to run on the first boot of the new system after a successful
installation.
If configured, this installs an additional package named
<code>proxmox-first-boot</code>. After booting the new system for the
first time, this package can safely be removed using <code>apt purge
proxmox-first-boot</code>.
@@ -250,6 +250,8 @@
{{Note| The maximum executable file size is 1 MiB, for both
integrating it into the ISO and fetching it from a URL.}}
+{{Note| Compiled binaries must target x86_64 Linux and should be
statically linked, as there are no guarantees for libraries installed on
the first boot.}}
+
=== Answer File Validation ===
The <code>proxmox-auto-install-assistant</code> tool can also be used
to validate the syntax of an answer file and display the identifying
information that will be sent to the HTTP(s) server when fetching the
answer file.
```
On 12/13/24 11:00, Christoph Heiss wrote:
> FWIW, just thinking out loud here: Maybe the fetching of the first-boot
> hook should be moved into `proxmox-fetch-answer` altogether. In a sense
> that `proxmox-fetch-answer` would fetch all the needed things in
> advance, such that the auto-installer, post-hook etc. doesn't have to do
> that anymore.
> But that's really not pressing of any kind, rather a long-term thing.
Sounds like a good idea! If you've got other things to do, I can take a
look at this before the next release for sure.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-12-18 7:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-11 9:24 [pve-devel] [PATCH installer v2] auto: allow a binary executable as the first boot executable Daniel Kral
2024-12-13 10:00 ` Christoph Heiss
2024-12-18 7:29 ` Daniel Kral
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox