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"),