From 42d28cb5302f86fe591f34c4aacb407f1178aea0 Mon Sep 17 00:00:00 2001 From: loa212 Date: Wed, 29 Jul 2026 20:59:18 +0200 Subject: [PATCH] One switch for self-upgrade, not two MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit NUDO_ALLOW_SELF_UPGRADE is gone; the dashboard toggle is the whole gate. The flag existed on the theory that whoever installs an instance and whoever operates it are different people, and both should have to say yes. For a tool one person installs and operates on their own box, that was a second lock on the same door. Its only visible effect was a dashboard that refused an upgrade someone had already opted into, and a "now also set this environment variable" instruction repeated across the upgrade page, the settings page, the unit file and the README. The packaged unit has always put a fresh install in the release layout, so a new install now needs exactly one thing to be able to upgrade itself: the tick in settings. Nothing about the security of the path changes — the release is still verified against the digest published in the manifest, still staged beside the running one, still reverted by the boot guard if it cannot start, and a container or a flat /usr/local/bin install is still refused. The proto field is reserved rather than reused, since 0.4.0 published it. --- CHANGES.md | 23 +++++++++++----- README.md | 12 ++++----- controlplane.proto | 12 ++++++--- crates/allinone/tests/self_upgrade.rs | 4 +-- crates/server/src/api/self_upgrade.rs | 8 +++--- crates/server/src/config.rs | 17 +++--------- crates/server/src/self_upgrade.rs | 16 +++++------ crates/server/src/self_upgrade/tests.rs | 34 +++++++++--------------- crates/web/src/render/settings.rs | 11 ++++---- crates/web/src/render/tests/banners.rs | 4 +-- crates/web/src/render/tests/dashboard.rs | 21 +++++---------- crates/web/src/render/updates.rs | 31 +++++++-------------- crates/web/src/routes/dashboard.rs | 1 - packaging/nudo.service | 14 +++++----- scripts/screenshots.py | 1 - 15 files changed, 88 insertions(+), 121 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index b6c6738..1467ac1 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -657,13 +657,22 @@ store opened and migrations ran. `nudo-boot-guard`, run as `ExecStartPre=` from a stable path deliberately outside the symlink it may revert, counts unconfirmed boots and puts the previous release back after three. -**Off by default, twice.** `NUDO_ALLOW_SELF_UPGRADE` (whoever installs) and a -dashboard toggle (whoever operates) must both be on, and only a managed-layout -binary install on a published target is eligible; containers keep `docker pull` -and legacy binary installs keep the manual commands, now with the one-time -migration to the layout printed beside them. Rollback of the database is -deliberately manual — an automatic restore would silently discard writes made -after the snapshot — and the page says so, with the snapshot's path. +**Off until switched on, once.** The dashboard toggle is the whole gate, and +only a managed-layout binary install on a published target is eligible; +containers keep `docker pull`, and a binary install laid out flat keeps the +manual commands, with the move to the release layout printed beside them. +Rollback of the database is deliberately manual — an automatic restore would +silently discard writes made after the snapshot — and the page says so, with +the snapshot's path. + +0.4.0 shipped a second gate, `NUDO_ALLOW_SELF_UPGRADE`, on the theory that +whoever installs and whoever operates are different people and both should +have to say yes. For a tool one person installs and operates that was a second +lock on the same door: its only visible effect was a dashboard that refused an +upgrade the operator had already opted into, and a set-this-env-var instruction +in three places. Removed in 0.4.1; the proto field is reserved rather than +reused. The packaged unit has always put a fresh install in the release layout, +so what a new install now needs is one tick in settings and nothing else. **Tested end to end.** `crates/allinone/tests/self_upgrade.rs` builds the real binary, serves a fixture release over loopback, drives the RPC, and asserts the diff --git a/README.md b/README.md index 9b5dd1b..efd6269 100644 --- a/README.md +++ b/README.md @@ -106,12 +106,12 @@ downloaded, verified against the sha256 the release workflow commits into atomically — and reverted by `nudo-boot-guard` if it cannot start. Nothing is ever fetched and piped to a shell. -Self-upgrading is off unless two switches are both on: start the instance with -`NUDO_ALLOW_SELF_UPGRADE=true` *and* enable it in the dashboard's settings. The -trade-off of the layout is that the service user can overwrite its own binaries — -that is what self-upgrading is — so if you will never use it, installing to -`/usr/local/bin` with a root-owned binary remains the tighter posture, and the -`/upgrade` page keeps printing exact manual commands for it. +Self-upgrading is off until you switch it on in the dashboard's settings — one +tick, no configuration. The trade-off of the layout is that the service user can +overwrite its own binaries — that is what self-upgrading is — so if you will +never use it, installing to `/usr/local/bin` with a root-owned binary remains +the tighter posture, and the `/upgrade` page keeps printing exact manual +commands for it. ### From source diff --git a/controlplane.proto b/controlplane.proto index efc510c..efd82d9 100644 --- a/controlplane.proto +++ b/controlplane.proto @@ -1123,9 +1123,15 @@ message SelfUpgradeStatus { string from_version = 2; string to_version = 3; string error = 4; - // The three gates, reported separately so the dashboard can say which one - // is closed rather than a generic "unavailable". - bool allowed_by_config = 5; + // The two gates, reported separately so the dashboard can say which one is + // closed rather than a generic "unavailable". + // + // 5 was `allowed_by_config`, a process flag that had to be on as well. + // Removed in 0.4.1: for a tool one person installs and operates, it was a + // second lock on the same door, and its only visible effect was a dashboard + // that refused an upgrade someone had already opted into. + reserved 5; + reserved "allowed_by_config"; bool enabled_in_settings = 6; bool eligible = 7; // RFC 3339; empty when no upgrade has ever run. diff --git a/crates/allinone/tests/self_upgrade.rs b/crates/allinone/tests/self_upgrade.rs index a03a2f8..2b503d9 100644 --- a/crates/allinone/tests/self_upgrade.rs +++ b/crates/allinone/tests/self_upgrade.rs @@ -84,7 +84,6 @@ impl Harness { let web_port = free_port(); let child = Command::new(self_dir.join("current").join("nudo-all-in-one")) .env("NUDO_SELF_DIR", &self_dir) - .env("NUDO_ALLOW_SELF_UPGRADE", "true") .env( "NUDO_SELF_UPGRADE_DOWNLOAD_BASE", format!("http://127.0.0.1:{}", artifact_server.port), @@ -262,8 +261,7 @@ async fn an_instance_upgrades_itself_and_comes_back_as_the_new_version() { let status = harness .wait_for_status("the instance to come up", |status| status.state == "idle") .await; - assert!(status.allowed_by_config, "{}", harness.debug_logs()); - assert!(status.enabled_in_settings); + assert!(status.enabled_in_settings, "{}", harness.debug_logs()); assert!(status.eligible, "the child must detect the managed layout"); let pid_before_upgrade = harness.child.id(); diff --git a/crates/server/src/api/self_upgrade.rs b/crates/server/src/api/self_upgrade.rs index 2a4cf27..d6400fd 100644 --- a/crates/server/src/api/self_upgrade.rs +++ b/crates/server/src/api/self_upgrade.rs @@ -31,7 +31,6 @@ fn to_proto(view: StatusView) -> SelfUpgradeStatus { from_version: view.from_version, to_version: view.to_version, error: view.error, - allowed_by_config: view.allowed_by_config, enabled_in_settings: view.enabled_in_settings, eligible: view.eligible, updated_at: view.updated_at, @@ -99,8 +98,7 @@ mod tests { .expect("status") .into_inner(); assert_eq!(status.state, "idle"); - assert!(!status.allowed_by_config, "default config allows nothing"); - assert!(!status.enabled_in_settings); + assert!(!status.enabled_in_settings, "the toggle defaults off"); assert!(!status.eligible); } @@ -118,7 +116,7 @@ mod tests { } #[tokio::test] - async fn start_is_refused_by_the_config_gate_and_audited_anyway() { + async fn start_is_refused_while_the_toggle_is_off_and_audited_anyway() { let context = test_support::context().await; let service = service(context.clone()); let error = service @@ -129,7 +127,7 @@ mod tests { .await .expect_err("must refuse"); assert_eq!(error.code(), tonic::Code::FailedPrecondition); - assert!(error.message().contains("--allow-self-upgrade")); + assert!(error.message().contains("switched off")); // The attempt itself is on the record: asking an instance to replace // its binaries is audit-worthy even when refused. diff --git a/crates/server/src/config.rs b/crates/server/src/config.rs index 4c97182..8b1d19b 100644 --- a/crates/server/src/config.rs +++ b/crates/server/src/config.rs @@ -86,21 +86,13 @@ pub struct Config { #[arg(long, env = "NUDO_ALLOW_SETUP", default_value_t = true)] pub allow_setup: bool, - /// Allow this instance to download a verified release and replace its own - /// binaries when asked to from the dashboard. - /// - /// Off by default, and this flag alone is not enough: the dashboard's own - /// self-upgrade toggle must also be on. Two switches because the flag is - /// set by whoever installs, while the toggle belongs to whoever operates. - #[arg(long, env = "NUDO_ALLOW_SELF_UPGRADE", default_value_t = false)] - pub allow_self_upgrade: bool, - /// Root of the self-release layout: `releases/`, the `current` symlink, /// and the upgrade journal. /// - /// Set by the packaged unit (`/var/lib/nudo/self`). Absent means the - /// binary was installed the pre-layout way, and self-upgrade is - /// unavailable until the documented one-time migration. + /// Set by the packaged unit (`/var/lib/nudo/self`), so a standard install + /// has it. Absent means the binaries were installed somewhere flat — + /// straight into `/usr/local/bin`, say — and self-upgrade is unavailable + /// until the layout the `/upgrade` page describes is adopted. #[arg(long, env = "NUDO_SELF_DIR")] pub self_dir: Option, @@ -198,7 +190,6 @@ impl Default for Config { update_manifest_url: crate::updates::DEFAULT_MANIFEST_URL.to_string(), update_interval_hours: 24, allow_setup: true, - allow_self_upgrade: false, self_dir: None, self_upgrade_download_base: crate::self_upgrade::DEFAULT_DOWNLOAD_BASE.to_string(), } diff --git a/crates/server/src/self_upgrade.rs b/crates/server/src/self_upgrade.rs index 238254c..ec47567 100644 --- a/crates/server/src/self_upgrade.rs +++ b/crates/server/src/self_upgrade.rs @@ -23,8 +23,13 @@ //! version is ever executed. //! - Nothing is piped to a shell; the tarball is unpacked and exec'd by this //! code. -//! - Two switches must both be on: `--allow-self-upgrade` from whoever -//! installs, and the dashboard toggle from whoever operates. +//! - It is off until switched on. The dashboard toggle is the whole gate: one +//! decision, made by the person who would live with it, in the place they +//! are already looking. An earlier design also required a flag on the +//! process, on the theory that whoever installs and whoever operates are +//! different people — for a self-hosted tool run by one person that was a +//! second lock on the same door, and it only ever made the feature look +//! broken to someone who had already said yes. use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; @@ -137,7 +142,6 @@ pub struct StatusView { pub error: String, /// RFC 3339, empty when nothing has happened yet. pub updated_at: String, - pub allowed_by_config: bool, pub enabled_in_settings: bool, pub eligible: bool, } @@ -175,7 +179,6 @@ impl SelfUpgrader { let eligibility = eligibility(&self.config); let mut view = StatusView { state: "idle".to_string(), - allowed_by_config: self.config.allow_self_upgrade, enabled_in_settings: self.store.self_upgrade_enabled().await.unwrap_or(false), eligible: matches!(eligibility, Eligibility::Managed { .. }), ..StatusView::default() @@ -212,11 +215,6 @@ impl SelfUpgrader { /// gRPC handler whose response must reach the dashboard before exec() /// replaces the process serving it. pub async fn start(&self, target_version: &str) -> anyhow::Result<()> { - if !self.config.allow_self_upgrade { - bail!( - "self-upgrade is not allowed by this instance's configuration (--allow-self-upgrade)" - ); - } if !self.store.self_upgrade_enabled().await? { bail!("self-upgrade is switched off in the dashboard settings"); } diff --git a/crates/server/src/self_upgrade/tests.rs b/crates/server/src/self_upgrade/tests.rs index 2f9e9d8..0e803a9 100644 --- a/crates/server/src/self_upgrade/tests.rs +++ b/crates/server/src/self_upgrade/tests.rs @@ -1,9 +1,8 @@ use super::*; use crate::store::Store; -fn config(allow: bool, self_dir: Option) -> Arc { +fn config(self_dir: Option) -> Arc { Arc::new(crate::Config { - allow_self_upgrade: allow, self_dir, ..crate::Config::default() }) @@ -28,33 +27,25 @@ fn layout(dir: &Path, version: &str) { // ---- gates ---- -#[tokio::test] -async fn the_config_flag_gates_everything() { - let upgrader = SelfUpgrader::new(store().await, config(false, None)); - let error = upgrader.start("99.0.0").await.expect_err("must refuse"); - assert!( - error.to_string().contains("--allow-self-upgrade"), - "{error}" - ); -} - #[tokio::test] async fn the_settings_toggle_gates_everything() { - // Flag on, toggle off (the default): still refused. Two different people - // control these two switches; both must have said yes. - let upgrader = SelfUpgrader::new(store().await, config(true, None)); + // Off is the default, and the default refuses. This is the whole gate: + // an instance that has not been told it may replace its own binaries + // will not, however it was installed. + let upgrader = SelfUpgrader::new(store().await, config(None)); let error = upgrader.start("99.0.0").await.expect_err("must refuse"); assert!(error.to_string().contains("switched off"), "{error}"); } #[tokio::test] -async fn a_legacy_install_is_refused_even_with_both_switches_on() { +async fn a_flat_install_is_refused_even_with_the_toggle_on() { // The test binary does not run from a managed layout, so even a - // configured self_dir leaves eligibility at BinaryLegacy. + // configured self_dir leaves eligibility at BinaryLegacy — the toggle + // cannot conjure a layout that is not there. let dir = tempfile::tempdir().expect("tempdir"); let store = store().await; store.set_self_upgrade_enabled(true).await.expect("toggle"); - let upgrader = SelfUpgrader::new(store, config(true, Some(dir.path().to_path_buf()))); + let upgrader = SelfUpgrader::new(store, config(Some(dir.path().to_path_buf()))); let error = upgrader.start("99.0.0").await.expect_err("must refuse"); assert!( error.to_string().contains("cannot upgrade itself"), @@ -65,10 +56,9 @@ async fn a_legacy_install_is_refused_even_with_both_switches_on() { #[tokio::test] async fn the_status_reports_which_gates_are_open() { let store = store().await; - let upgrader = SelfUpgrader::new(store.clone(), config(true, None)); + let upgrader = SelfUpgrader::new(store.clone(), config(None)); let view = upgrader.status().await; - assert!(view.allowed_by_config); - assert!(!view.enabled_in_settings); + assert!(!view.enabled_in_settings, "off until switched on"); assert!(!view.eligible); assert_eq!(view.state, "idle"); @@ -380,7 +370,7 @@ async fn a_version_that_is_not_newer_is_refused_before_anything_else_network_sha .await .expect("record"); let dir = tempfile::tempdir().expect("tempdir"); - let upgrader = SelfUpgrader::new(store, config(true, Some(dir.path().to_path_buf()))); + let upgrader = SelfUpgrader::new(store, config(Some(dir.path().to_path_buf()))); // The test binary is not in a managed layout, so this refusal is the // eligibility one — which is fine: it proves order (gates before network). upgrader.start("0.0.1").await.expect_err("must refuse"); diff --git a/crates/web/src/render/settings.rs b/crates/web/src/render/settings.rs index 51f8716..06892ee 100644 --- a/crates/web/src/render/settings.rs +++ b/crates/web/src/render/settings.rs @@ -204,11 +204,12 @@ pub fn settings_page( span { "Allow this instance to upgrade itself from the " a href="/upgrade" { "upgrade page" } - ". Only half the permission: the instance must \ - also have been started with " - code { "NUDO_ALLOW_SELF_UPGRADE=true" } - ", and it only applies to a binary install \ - running from the release layout." + ". Releases are verified against a digest \ + published in the manifest and rolled back \ + automatically if the new version cannot \ + start. Applies to a binary install running \ + from the release layout; a container upgrades \ + with " code { "docker pull" } "." } } } diff --git a/crates/web/src/render/tests/banners.rs b/crates/web/src/render/tests/banners.rs index f688306..5cc51e4 100644 --- a/crates/web/src/render/tests/banners.rs +++ b/crates/web/src/render/tests/banners.rs @@ -155,8 +155,8 @@ fn the_settings_page_carries_every_switch_and_says_nothing_is_sent() { assert!(rendered.contains("2 hours ago")); // The claim that matters most on that page. assert!(rendered.contains("no usage")); - // The self-upgrade toggle must say it is only half the permission. - assert!(rendered.contains("NUDO_ALLOW_SELF_UPGRADE")); + // The self-upgrade toggle must say what it actually authorises. + assert!(rendered.contains("upgrade itself")); } #[test] diff --git a/crates/web/src/render/tests/dashboard.rs b/crates/web/src/render/tests/dashboard.rs index 7340b32..ff9f2f0 100644 --- a/crates/web/src/render/tests/dashboard.rs +++ b/crates/web/src/render/tests/dashboard.rs @@ -11,11 +11,10 @@ fn an_upgrade(install: UpgradeInstall) -> UpgradeView { } } -/// A managed install with both opt-ins given and nothing in flight. +/// A managed install with self-upgrade switched on and nothing in flight. fn a_managed_status() -> SelfUpgradeView { SelfUpgradeView { state: "idle".to_string(), - allowed_by_config: true, enabled_in_settings: true, eligible: true, ..SelfUpgradeView::default() @@ -148,9 +147,9 @@ fn the_upgrade_page_never_pipes_anything_into_a_shell() { #[test] fn only_a_fully_opted_in_managed_install_gets_the_button() { // A form on this page is a big deal — it used to be banned outright. It - // appears in exactly one configuration: managed layout, config flag on, - // settings toggle on, newer release available. Everything else stays a - // page of instructions. + // appears in exactly one configuration: managed layout, self-upgrade + // switched on, newer release available. Everything else stays a page of + // instructions. let with_button = s(upgrade_page(&an_upgrade(UpgradeInstall::BinaryManaged { status: a_managed_status(), }))); @@ -179,15 +178,7 @@ fn only_a_fully_opted_in_managed_install_gets_the_button() { ); } - // Managed but a gate closed: no form, and it says which gate. - let mut flag_off = a_managed_status(); - flag_off.allowed_by_config = false; - let rendered = s(upgrade_page(&an_upgrade(UpgradeInstall::BinaryManaged { - status: flag_off, - }))); - assert!(!rendered.contains(" Markup { p .small.muted { "The trade-off is real and worth knowing: in this layout the \ service user can overwrite its own binaries, which is what \ - self-upgrading means. It stays inert unless you also start \ - nudo with " code { "NUDO_ALLOW_SELF_UPGRADE=true" } " and turn \ - the switch on in settings." + self-upgrading means. It stays inert until you turn the \ + switch on in settings." } })) } @@ -399,7 +398,7 @@ fn binary_upgrade(version: &str) -> Markup { /// the card says which gate is closed and what opening it means, because a /// button that is sometimes missing without explanation reads as a bug. fn managed_upgrade(view: &UpgradeView, status: &SelfUpgradeView) -> Markup { - let gates_open = status.allowed_by_config && status.enabled_in_settings; + let gates_open = status.enabled_in_settings; let in_flight = status.in_flight(); html! { div .card { @@ -422,21 +421,10 @@ fn managed_upgrade(view: &UpgradeView, status: &SelfUpgradeView) -> Markup { } } @else if !gates_open { (callout("info", "Switched off", html! { - @if !status.allowed_by_config { - p { - "The instance was started without " - code { "NUDO_ALLOW_SELF_UPGRADE=true" } - ", so it will not replace its own binaries no matter \ - what is clicked here. Set it in the unit and restart \ - to allow it." - } - } @else { - p { - "Self-upgrade is off in " - a href="/settings#instance" { "settings" } - ". Turning it on is the operator's half of the \ - opt-in; the config flag is the installer's." - } + p { + "Self-upgrade is off for this instance. Turn it on in " + a href="/settings#instance" { "settings" } + " and this card grows a button whenever a release is out." } })) } @else if view.available { @@ -612,7 +600,6 @@ pub struct SelfUpgradeView { pub from_version: String, pub to_version: String, pub error: String, - pub allowed_by_config: bool, pub enabled_in_settings: bool, pub eligible: bool, } @@ -627,9 +614,9 @@ impl SelfUpgradeView { } /// Whether the "Update now" button should appear: this install can do it, - /// both opt-ins are given, and nothing is already running. + /// the operator opted in, and nothing is already running. pub fn can_upgrade_now(&self) -> bool { - self.eligible && self.allowed_by_config && self.enabled_in_settings && !self.in_flight() + self.eligible && self.enabled_in_settings && !self.in_flight() } } diff --git a/crates/web/src/routes/dashboard.rs b/crates/web/src/routes/dashboard.rs index 35c2405..afee52d 100644 --- a/crates/web/src/routes/dashboard.rs +++ b/crates/web/src/routes/dashboard.rs @@ -219,7 +219,6 @@ fn to_self_upgrade_view(status: SelfUpgradeStatus) -> render::SelfUpgradeView { from_version: status.from_version, to_version: status.to_version, error: status.error, - allowed_by_config: status.allowed_by_config, enabled_in_settings: status.enabled_in_settings, eligible: status.eligible, } diff --git a/packaging/nudo.service b/packaging/nudo.service index 759aed4..120a4dc 100644 --- a/packaging/nudo.service +++ b/packaging/nudo.service @@ -25,10 +25,10 @@ # # The trade-off, stated plainly: with the binaries under /var/lib/nudo the # service user can overwrite what it runs. That is inherent to letting the -# process upgrade itself, and it is why the feature sits behind both a config -# flag (NUDO_ALLOW_SELF_UPGRADE) and a dashboard toggle, both off by default. -# If you will never use self-upgrade, the old /usr/local/bin install remains -# the tighter posture. +# process upgrade itself, and it is why self-upgrade stays off until it is +# switched on in the dashboard's settings. If you will never use it, installing +# to /usr/local/bin with root-owned binaries remains the tighter posture, and +# the /upgrade page keeps printing the manual commands for that. # # Put a TLS-terminating reverse proxy in front of it and set NUDO_BASE_URL to the # https:// URL you reach it on — that is what marks the session cookie Secure and @@ -56,9 +56,9 @@ WorkingDirectory=/var/lib/nudo # The key file rather than the environment, so the key is not visible in # `systemctl show` or to anything that can read this unit's environment. Environment="NUDO_SECRET_KEY_FILE=/etc/nudo/secret.key" -# Where the self-release layout lives; also what tells the process it may -# manage its own binaries (still gated behind NUDO_ALLOW_SELF_UPGRADE and the -# dashboard toggle). +# Where the self-release layout lives, which is what makes upgrading from the +# dashboard possible at all. It stays inert until the switch in settings is +# turned on. Environment="NUDO_SELF_DIR=/var/lib/nudo/self" Environment="NUDO_DB=/var/lib/nudo/nudo.db" Environment="NUDO_DATA_DIR=/var/lib/nudo/data" diff --git a/scripts/screenshots.py b/scripts/screenshots.py index 30c6147..704e698 100755 --- a/scripts/screenshots.py +++ b/scripts/screenshots.py @@ -954,7 +954,6 @@ def start(self) -> None: env.update( { "NUDO_SELF_DIR": str(self.root / "self"), - "NUDO_ALLOW_SELF_UPGRADE": "true", "NUDO_SELF_UPGRADE_DOWNLOAD_BASE": f"http://127.0.0.1:{self.artifacts.port}", "NUDO_UPDATE_MANIFEST_URL": f"http://127.0.0.1:{self.manifest_port}/releases.json", "NUDO_DB": str(self.root / "nudo.db"),