diff --git a/CHANGELOG.md b/CHANGELOG.md index 073417c..b659ac1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.htm ### Added +- `module_spec::UnitTemplate::limit_nofile`, an optional `LimitNOFILE=` a + package declares against its own unit, so a service whose store outgrows the + soft descriptor limit systemd hands a unit says so itself instead of leaving + an operator to raise it on the host. The renderer emits the directive between + `RestartSec=` and the sandbox booleans. Absence, which is what every unit + rendered until now carries, inherits the host's soft limit; there is no + spelling for systemd's `infinity`, so leaving a service unbounded stays the + host's decision rather than a package's: a declared zero is refused as + `module_spec::ModuleSpecError::ZeroLimitNofile`, and `u64::MAX` — the + numeric value of Linux's `RLIM_INFINITY`, which systemd's own rlimit parser + refuses — as `module_spec::ModuleSpecError::InfiniteLimitNofile`. A producer + now stamps manifest format version 5 and this build still reads 3, so a + payload already published stays readable, installable, and byte-identical in + what it renders. - `module_spec::RenderVar::ManagerEndpoint`, with which a unit template names the manager endpoint its module is pointed at, and the `render::RenderContext::manager_endpoint` field the renderer resolves it diff --git a/assets/test-fixtures/unsigned-container/manifest.json b/assets/test-fixtures/unsigned-container/manifest.json index b2a19a9..1cbda2d 100644 --- a/assets/test-fixtures/unsigned-container/manifest.json +++ b/assets/test-fixtures/unsigned-container/manifest.json @@ -1 +1 @@ -{"format_version":4,"archive_members":[{"name":"bin/app","length":17}],"artifacts":[{"component":"example","version":"1.0.0","commit":"0123456789abcdef0123456789abcdef01234567","target_arch":"x86_64","kind":"native-binary","dispositions":["install"],"archive_path":"bin/app","sha256":"bcf23e595c273e3202d8fc0fb47b5d6107a14fbc2d0a08c97ae4dd0262907d81"}]} \ No newline at end of file +{"format_version":5,"archive_members":[{"name":"bin/app","length":17}],"artifacts":[{"component":"example","version":"1.0.0","commit":"0123456789abcdef0123456789abcdef01234567","target_arch":"x86_64","kind":"native-binary","dispositions":["install"],"archive_path":"bin/app","sha256":"bcf23e595c273e3202d8fc0fb47b5d6107a14fbc2d0a08c97ae4dd0262907d81"}]} \ No newline at end of file diff --git a/src/manifest.rs b/src/manifest.rs index bfa1661..a7be336 100644 --- a/src/manifest.rs +++ b/src/manifest.rs @@ -22,7 +22,7 @@ use crate::module_spec::{ModuleSpec, ModuleSpecError}; /// Stamped unconditionally, never derived from a manifest's contents: a /// manifest's version is the producer's, not a function of which optional /// variables its templates happen to name. -pub const MANIFEST_FORMAT_VERSION: u32 = 4; +pub const MANIFEST_FORMAT_VERSION: u32 = 5; /// Inclusive floor of the `format_version` range this build accepts. /// @@ -40,7 +40,7 @@ pub const MIN_MANIFEST_FORMAT_VERSION: u32 = 3; /// Tracks [`MANIFEST_FORMAT_VERSION`]: this build reads what it writes, and a /// manifest naming anything newer is refused for its version alone rather than /// failing as an opaque decode error somewhere inside it. -pub const MAX_MANIFEST_FORMAT_VERSION: u32 = 4; +pub const MAX_MANIFEST_FORMAT_VERSION: u32 = 5; /// Container footer version the pre-versioned baseline payloads were written /// at. @@ -1037,6 +1037,7 @@ mod tests { ], restart: RestartPolicy::Always, restart_sec: 5, + limit_nofile: Some(8000), protect_home: true, private_tmp: true, no_new_privileges: true, @@ -1249,17 +1250,18 @@ mod tests { } #[test] - fn the_producer_writes_the_ceiling_and_the_floor_is_no_higher() { - // The range has a floor as well as a ceiling, and both are written by - // repointing a constant rather than by searching for a literal. The - // ceiling tracks the producer exactly — this build reads what it - // writes — while the floor only ever trails it, because an additive - // schema bump leaves every manifest already published at the floor - // readable. - assert_eq!(MAX_MANIFEST_FORMAT_VERSION, MANIFEST_FORMAT_VERSION); - // In a `const` block, so a floor raised past the producer is a build - // failure rather than a failing test. - const { assert!(MIN_MANIFEST_FORMAT_VERSION <= MANIFEST_FORMAT_VERSION) }; + fn the_accepted_range_is_open_below_the_producer_and_closed_at_it() { + // The producer writes at the ceiling — a build never stamps a version + // it would then refuse to read — and the floor sits strictly below it, + // so the window admits more than one version. Pinned rather than left + // implicit because a later bump that moved all three together would + // close the window again and silently strand every payload published + // at the older version. + // + // Both hold at compile time, so both are stated that way: a bump that + // closed the window fails the build rather than one test run. + const _: () = assert!(MAX_MANIFEST_FORMAT_VERSION == MANIFEST_FORMAT_VERSION); + const _: () = assert!(MIN_MANIFEST_FORMAT_VERSION < MANIFEST_FORMAT_VERSION); } #[test] @@ -1820,6 +1822,7 @@ mod tests { assert_eq!(unit.environment, expected.environment); assert_eq!(unit.restart, expected.restart); assert_eq!(unit.restart_sec, expected.restart_sec); + assert_eq!(unit.limit_nofile, expected.limit_nofile); assert_eq!(unit.protect_home, expected.protect_home); assert_eq!(unit.private_tmp, expected.private_tmp); assert_eq!(unit.no_new_privileges, expected.no_new_privileges); @@ -1827,14 +1830,14 @@ mod tests { assert_eq!(spec.placement, PlacementClass::ModuleHosts); } + /// Names which spec rule a read-path rejection must report. + type RuleCheck = fn(&ModuleSpecError) -> bool; + #[test] fn the_read_path_runs_the_spec_validator_rather_than_trusting_the_producer() { // Each of these decodes cleanly and is refused by the validator, so a // consumer that never links a producer still refuses it. The case is // named by the source variant, not by "some error occurred". - /// Names which spec rule a read-path rejection must report. - type RuleCheck = fn(&ModuleSpecError) -> bool; - let cases: [(&str, RuleCheck); 4] = [ ( r#""exec_start":[{"var":"artifact-path"},{"var":"main-pid"}]"#, @@ -1871,6 +1874,44 @@ mod tests { } } + #[test] + fn the_read_path_refuses_a_unit_declaring_an_unloadable_limit() { + // Both limit rules reach a consumer the way every other unit rule + // does, through the read path, so a consumer that never links a + // producer refuses the manifest rather than rendering a directive that + // starves the service of every descriptor or one systemd's rlimit + // parser refuses outright. + let cases: [(&str, RuleCheck); 2] = [ + ("0", |source| { + matches!(source, ModuleSpecError::ZeroLimitNofile) + }), + ("18446744073709551615", |source| { + matches!(source, ModuleSpecError::InfiniteLimitNofile) + }), + ]; + for (limit, is_expected) in cases { + let unit = unit_json(VALID_EXEC_START).replace( + r#""restart_sec":5,"#, + &format!(r#""restart_sec":5,"limit_nofile":{limit},"#), + ); + let entry = entry_json_with_spec("bin/c", Some(GIT_COMMIT), &spec_json(&unit)); + let json = format!( + r#"{{"format_version":{MANIFEST_FORMAT_VERSION},{MEMBERS_JSON}"artifacts":[{entry}]}}"# + ); + let error = PayloadManifest::parse(json.as_bytes(), LEGACY_UNVERSIONED_FOOTER_VERSION) + .expect_err("an unloadable limit must be refused on read"); + let ManifestError::InvalidSpec { + ref archive_path, + ref source, + } = error + else { + panic!("limit {limit}: expected a spec rejection, got {error:?}"); + }; + assert_eq!(archive_path, "bin/c"); + assert!(is_expected(source), "limit {limit}: got {source:?}"); + } + } + #[test] fn the_read_path_enforces_the_kind_conditional_unit_rule() { // A `native-binary` artifact declaring a spec with no unit. @@ -2006,21 +2047,17 @@ mod tests { } #[test] - fn the_range_accepts_the_unmoved_floor_as_well_as_the_current_version() { - // Every version is written by interpolating a constant, so the next - // bump stays a repoint of the three rather than a search for a - // literal. A manifest at the floor is what an earlier build published: - // it is still accepted, and reports the version it was written at. - // Either side of the range is refused for the version alone, which - // `a_version_below_the_floor_is_rejected_with_the_same_variant` and - // `a_version_above_the_ceiling_is_rejected_for_its_version_alone` each - // pin against its own end. - for version in [MIN_MANIFEST_FORMAT_VERSION, MANIFEST_FORMAT_VERSION] { + fn every_version_in_the_window_is_accepted_including_the_floor() { + // The two refusals either side of the window have their own tests; what + // this one pins is that the window is not a point. Written by + // interpolating the constants, so the next bump stays a repoint of them + // rather than a search for a literal. + for version in MIN_MANIFEST_FORMAT_VERSION..=MAX_MANIFEST_FORMAT_VERSION { let manifest = PayloadManifest::parse( versioned_json(version).as_bytes(), LEGACY_UNVERSIONED_FOOTER_VERSION, ) - .expect("a version inside the accepted range must parse"); + .expect("a version inside the window must be accepted"); assert_eq!(manifest.format_version(), Some(version)); } } @@ -2090,6 +2127,40 @@ mod tests { ); } + #[test] + fn a_manifest_at_the_floor_version_still_decodes_validates_and_renders_without_a_limit() { + // The floor stayed put across the `limit_nofile` bump precisely so an + // already-published payload keeps working. That promise is about the + // whole read path, so this exercises it to the rendered bytes rather + // than stopping at the decode. + let entry = entry_json_with_spec( + "bin/c", + Some(GIT_COMMIT), + &spec_json(&unit_json(VALID_EXEC_START)), + ); + let json = format!( + r#"{{"format_version":{MIN_MANIFEST_FORMAT_VERSION},{MEMBERS_JSON}"artifacts":[{entry}]}}"# + ); + let manifest = PayloadManifest::parse(json.as_bytes(), LEGACY_UNVERSIONED_FOOTER_VERSION) + .expect("a manifest at the floor version must still parse"); + assert_eq!(manifest.format_version(), Some(MIN_MANIFEST_FORMAT_VERSION)); + + let spec = manifest + .artifacts() + .first() + .expect("one artifact") + .spec + .as_ref() + .expect("the artifact declares a spec"); + let unit = spec.unit.as_ref().expect("the spec declares a unit"); + assert_eq!(unit.limit_nofile, None); + + let rendered = render_unit(spec, &read_path_context(None)) + .expect("a floor-version spec must render") + .expect("the spec declares a unit"); + assert!(!rendered.text.contains("Limit"), "got: {}", rendered.text); + } + #[test] fn a_manifest_at_the_current_version_renders_a_spec_naming_the_manager_endpoint() { // The mirror of the test above, and the read path the bump was cut diff --git a/src/module_spec.rs b/src/module_spec.rs index c0371d0..f024cff 100644 --- a/src/module_spec.rs +++ b/src/module_spec.rs @@ -72,7 +72,7 @@ pub struct ModuleSpec { /// element in declared order. /// 2. `[Service]` — `User=`, `ExecStart=`, `ExecReload=`, `WorkingDirectory=`, /// one `Environment=` line per [`Self::environment`] entry in declared -/// order, `Restart=`, `RestartSec=`, then `ProtectHome=yes`, +/// order, `Restart=`, `RestartSec=`, `LimitNOFILE=`, then `ProtectHome=yes`, /// `PrivateTmp=yes` and `NoNewPrivileges=yes`, each emitted only when its /// flag is `true`. /// 3. `[Install]` — one `WantedBy=` line per [`Self::wanted_by`] element in @@ -113,6 +113,18 @@ pub struct UnitTemplate { pub restart: RestartPolicy, /// `RestartSec=`, in seconds. pub restart_sec: u32, + /// `LimitNOFILE=`, in descriptors. When present, neither zero nor + /// [`u64::MAX`]. + /// + /// Absent means the unit inherits the host's soft limit, which is what + /// every unit rendered before this field existed does. There is no + /// spelling for systemd's `infinity` on purpose: a package that wants no + /// limit omits the field, leaving the unbounded case the host's decision + /// rather than a package's. [`u64::MAX`] is refused for the same reason — + /// it is the numeric value of Linux's `RLIM_INFINITY`, so it is that + /// sentinel written out rather than a limit. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub limit_nofile: Option, /// Emits `ProtectHome=yes` when set. pub protect_home: bool, /// Emits `PrivateTmp=yes` when set. @@ -339,6 +351,13 @@ pub enum ModuleSpecError { /// `exec_reload` was present and empty. #[error("unit template `exec_reload` is present and empty")] EmptyExecReload, + /// `limit_nofile` was present and zero. + #[error("unit template `limit_nofile` is present and zero")] + ZeroLimitNofile, + /// `limit_nofile` was present and `u64::MAX`, which is Linux's + /// `RLIM_INFINITY`. + #[error("unit template `limit_nofile` is `u64::MAX`, which systemd reads as `infinity`")] + InfiniteLimitNofile, /// `main-pid` appeared outside `exec_reload`. #[error("the `main-pid` variable is permitted only inside `exec_reload`")] MainPidOutsideExecReload, @@ -449,6 +468,19 @@ fn validate_unit(unit: &UnitTemplate) -> Result<(), ModuleSpecError> { validate_arg(value, false)?; } + // Zero is not a limit a package can mean: it would deny the service every + // descriptor, including the ones systemd hands it before `ExecStart=` runs. + // `u64::MAX` is not one either: it is the numeric value of Linux's + // `RLIM_INFINITY`, and systemd's rlimit parser refuses any value at or + // above that, so the unit would render fine and then fail to load. + // Refusing it here keeps absence the only way a package leaves a service + // unbounded. Absence is also how a package declines to set one. + match unit.limit_nofile { + Some(0) => return Err(ModuleSpecError::ZeroLimitNofile), + Some(u64::MAX) => return Err(ModuleSpecError::InfiniteLimitNofile), + _ => {} + } + Ok(()) } @@ -581,6 +613,7 @@ mod tests { environment: Vec::new(), restart: RestartPolicy::Always, restart_sec: RESTART_SEC, + limit_nofile: None, protect_home: false, private_tmp: false, no_new_privileges: false, @@ -678,6 +711,34 @@ mod tests { )); } + #[test] + fn a_present_limit_nofile_must_be_a_limit_systemd_can_load() { + // Absence is how a package declines to set a limit, so `None` is not a + // violation; `Some(0)` is, because it would deny the service every + // descriptor rather than raising anything. `Some(u64::MAX)` is too: it + // is Linux's `RLIM_INFINITY`, which systemd's rlimit parser refuses, + // so it would render a unit that cannot load. + for limit in [None, Some(1), Some(8000), Some(u64::MAX - 1)] { + let mut unit = review_unit(); + unit.limit_nofile = limit; + validate_unit_template(unit).expect("an absent or loadable limit is valid"); + } + + for (limit, expected) in [ + (0, ModuleSpecError::ZeroLimitNofile), + (u64::MAX, ModuleSpecError::InfiniteLimitNofile), + ] { + let mut unit = review_unit(); + unit.limit_nofile = Some(limit); + let error = validate_unit_template(unit).expect_err("limit must be rejected"); + assert_eq!( + std::mem::discriminant(&error), + std::mem::discriminant(&expected), + "limit {limit}: got {error:?}" + ); + } + } + #[test] fn main_pid_is_confined_to_exec_reload() { let mut in_exec_start = review_unit(); @@ -928,6 +989,9 @@ mod tests { ); assert_eq!(unit.restart, RestartPolicy::Always); assert_eq!(unit.restart_sec, RESTART_SEC); + // The anchor sets no limit, so this is also the assertion that an + // unchanged package's bytes did not move when the field was added. + assert_eq!(unit.limit_nofile, None); assert!(unit.protect_home); assert!(!unit.private_tmp); assert!(unit.no_new_privileges); @@ -946,6 +1010,70 @@ mod tests { assert_eq!(re_encoded, WIRE_SPEC); } + #[test] + fn limit_nofile_round_trips_only_when_it_is_present() { + // The absent case is asserted as whole bytes by the wire-form test + // above; what this adds is the present one, on the same anchor, so the + // two differ by exactly the key under test. + const LIMIT: u64 = 8000; + let with_limit = WIRE_SPEC.replace( + r#""restart_sec":5,"#, + &format!(r#""restart_sec":5,"limit_nofile":{LIMIT},"#), + ); + let decoded: ModuleSpec = + serde_json::from_str(&with_limit).expect("a declared limit must decode"); + let unit = decoded.unit.as_ref().expect("a unit is declared"); + assert_eq!(unit.limit_nofile, Some(LIMIT)); + let re_encoded = serde_json::to_string(&decoded).expect("serialization must succeed"); + assert_eq!(re_encoded, with_limit); + + // An explicit `null` is not a spelling of absence here: the field is + // `Option`, so it decodes, and what it must not do is round-trip back + // to a `null` key. + let with_null = WIRE_SPEC.replace( + r#""restart_sec":5,"#, + r#""restart_sec":5,"limit_nofile":null,"#, + ); + let decoded: ModuleSpec = + serde_json::from_str(&with_null).expect("an explicit null must decode"); + assert_eq!( + decoded + .unit + .as_ref() + .expect("a unit is declared") + .limit_nofile, + None + ); + let re_encoded = serde_json::to_string(&decoded).expect("serialization must succeed"); + assert_eq!(re_encoded, WIRE_SPEC); + + // The new field does not open the record: a sibling beside it is still + // refused, and so is a value outside the field's own domain. + assert!( + serde_json::from_str::( + &with_limit.replace(r#""limit_nofile""#, r#""limit_nproc":64,"limit_nofile""#) + ) + .is_err(), + "a second `Limit*` key must not decode" + ); + assert!( + serde_json::from_str::(&WIRE_SPEC.replace( + r#""restart_sec":5,"#, + r#""restart_sec":5,"limit_nofile":"infinity","# + )) + .is_err(), + "`infinity` must not be expressible" + ); + assert!( + serde_json::from_str::(&WIRE_SPEC.replace( + r#""restart_sec":5,"#, + r#""restart_sec":5,"limit_nofile":-1,"# + )) + .is_err(), + "a negative sentinel must not be expressible" + ); + } + #[test] fn every_render_var_spells_its_kebab_case_name() { for (var, spelling) in [ diff --git a/src/payload.rs b/src/payload.rs index b2ed2a6..c59ccf0 100644 --- a/src/payload.rs +++ b/src/payload.rs @@ -3235,6 +3235,7 @@ mod tests { environment: Vec::new(), restart: RestartPolicy::Always, restart_sec: 5, + limit_nofile: None, protect_home: true, private_tmp: true, no_new_privileges: true, diff --git a/src/render.rs b/src/render.rs index 1ca275f..ab40f5d 100644 --- a/src/render.rs +++ b/src/render.rs @@ -394,6 +394,9 @@ fn render_text(unit: &UnitTemplate, context: &RenderContext<'_>) -> Result( /// second copy of the range would leave that mapping dead and give the two /// copies somewhere to drift apart. /// -/// Since [`MIN_MANIFEST_FORMAT_VERSION`](crate::manifest::MIN_MANIFEST_FORMAT_VERSION) -/// and [`MAX_MANIFEST_FORMAT_VERSION`] are equal today, the floor is only -/// observable *above* the implemented range, -/// where the accepted set is empty and the reported range says so. +/// The injected floor is observable wherever it sits above +/// [`MIN_MANIFEST_FORMAT_VERSION`](crate::manifest::MIN_MANIFEST_FORMAT_VERSION), +/// which since manifest format version 4 includes values *inside* the +/// implemented range: release ops can refuse a manifest at the build's floor +/// while the build still accepts every later version. Above +/// [`MAX_MANIFEST_FORMAT_VERSION`] the accepted set is empty, and the reported +/// range says so. fn check_format_version(found: u32, floor: u32) -> Result<(), VerifyError> { if found < floor { return Err(VerifyError::UnsupportedManifestFormat { @@ -2327,6 +2330,56 @@ mod tests { )); } + #[test] + fn an_injected_floor_inside_the_implemented_range_refuses_only_below_itself() { + // Since the `limit_nofile` bump the build's window spans more than one + // version, so a floor can sit *inside* it — the case the injected floor + // could not reach while the range was a point. Release ops raising the + // floor to the producer's version must refuse a package written at the + // build's own floor and still accept one written at the producer's. + let pair = keypair(); + let at_floor = signed_pkg( + &pair, + &manifest_json_at( + MIN_MANIFEST_FORMAT_VERSION, + &[(MEMBER, len_u64(ARTIFACT_BYTES))], + &[default_artifact()], + ), + &default_archive(), + None, + ); + // Both versions verify under the build's own floor, so the refusal + // below is the injected floor and nothing else about the fixture. + assert_eq!( + accepted(&at_floor, &trusting(&pair)) + .manifest() + .format_version(), + Some(MIN_MANIFEST_FORMAT_VERSION) + ); + + let floor = MANIFEST_FORMAT_VERSION; + let trust = TrustSet::new( + vec![TrustAnchor::new(public_key_of(&pair), false)], + Vec::new(), + floor, + 0, + ) + .expect("a single anchor builds a trust set"); + assert!( + matches!( + refusal(&at_floor, &trust), + VerifyError::UnsupportedManifestFormat { found, min, max } + if found == MIN_MANIFEST_FORMAT_VERSION + && min == floor + && max == MAX_MANIFEST_FORMAT_VERSION + ), + "a package below the injected floor must be refused" + ); + // The floor is a floor and not a pin: the producer's own version, at + // the top of the same window, still verifies under it. + assert!(verify_package(Cursor::new(default_pkg(&pair)), &trust, &request()).is_ok()); + } + #[test] fn verification_never_reads_the_archive_block() { // The completeness checks are decided from the bound member list and