Skip to content

fix(image): fold permissions into the OS build id so they OTA - #208

Open
nicksinas wants to merge 2 commits into
mainfrom
nsinas/permission-build-id
Open

fix(image): fold permissions into the OS build id so they OTA#208
nicksinas wants to merge 2 commits into
mainfrom
nsinas/permission-build-id

Conversation

@nicksinas

@nicksinas nicksinas commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

ENG-2437

Problem

A rootfs change that only edits permissions: (users/groups/passwords) builds fine locally but never lands on a device over avocado deploy. The device logs:

OS already at target version (AVOCADO_OS_BUILD_ID=…), skipping OS bundle download

…and no-ops.

Root cause

AVOCADO_OS_BUILD_ID is a pure function of the rootfs rpmdbuuid5(namespace, sha256(sorted NEVRA of every installed RPM)). The permissions: section rewrites /etc/{passwd,shadow,group} in the image work dir but never touches the package DB, so an identical package set yields an identical build id regardless of the auth-file bytes. The deploy-side gate compares build ids and short-circuits both the download and the apply of the OS bundle, so the change silently never ships.

(Same class of blind spot existed for the initramfs build id.)

Fix

Fold a hash of the assembled auth files into the build id alongside the NEVRA hash:

  • Rootfs: uuid5(ns, "$PKG_HASH:$AUTH_HASH")
  • Initramfs: uuid5(ns, "$INITRAMFS_PKG_HASH:$INITRAMFS_AUTH_HASH")

AUTH_HASH is produced by a new shared helper, render_auth_files_hash, that lives beside the existing render_hook_block / resolve_install_hooks / render_users_groups_script helpers and is imported by initramfs::image — so rootfs and initramfs derive their build id the exact same way, from one implementation.

AUTH_HASH=$(cat \
    "$ROOTFS_WORK/etc/passwd" \
    "$ROOTFS_WORK/etc/shadow" \
    "$ROOTFS_WORK/etc/group" \
    "$ROOTFS_WORK/etc/gshadow" 2>/dev/null | LC_ALL=C sort | sha256sum | awk '{print $1}')
  • LC_ALL=C sort makes the digest independent of the order the permissions section appends entries in (the users:/groups: maps have no guaranteed iteration order), so it's fully deterministic.
  • 2>/dev/null tolerates a missing gshadow (minimal images) — absent files contribute nothing rather than aborting.
  • Computed after the permissions section runs, so it observes the change.

Why this doesn't cause spurious updates

The id is a content hash: an unchanged permissions: block hashes identically → same id → no OTA. It moves if and only if the auth-file content actually changes. This is the minimal correct mechanism — the deploy gate reads nothing but AVOCADO_OS_BUILD_ID, so a permissions-only change has to move the id to land.

One-time migration note

Because the build-id formula changes ($PKG_HASH$PKG_HASH:$AUTH_HASH), every existing image re-ids once on the first rebuild after this lands, triggering a single OS OTA even when nothing else changed. Steady state is churn-free thereafter.

Tests

  • test_render_auth_files_hash_snippet — exact-output unit test on the pure helper + reuse under the initramfs var names.
  • test_build_id_folds_auth_files (rootfs and initramfs) — structural + ordering assertions (auth hash computed after the permissions section, folded into the uuid5 input).
  • Full lib suite: 1452 passing; cargo fmt / cargo clippy clean on the changed files.

Scope / follow-up

This is the correctness fix only. A separate --force override for avocado deploy (an escape hatch for any future rootfs change that doesn't move the id) was intentionally split out into its own PR — this fix largely subsumes the permissions: case, so --force is a nice-to-have rather than a dependency.

Fixes ENG-2437.

A rootfs change that only edits `permissions:` (users/groups/passwords)
builds fine but never lands over `avocado deploy` — the device logs
`OS already at target version, skipping OS bundle download` and no-ops.

AVOCADO_OS_BUILD_ID is a pure function of the rootfs rpmdb (uuid5 over
the sorted NEVRA set), and `permissions:` rewrites /etc/{passwd,shadow,
group} without touching the package DB. So an identical package set
produced an identical build id regardless of the auth-file bytes, and
the deploy-side gate short-circuited the update.

Fold a hash of the assembled auth files into the build id alongside the
NEVRA hash, via a shared `render_auth_files_hash` helper used by both the
rootfs and initramfs build scripts. The hash is `LC_ALL=C`-sorted so it
is independent of the order the permissions section appends entries in,
and tolerates a missing gshadow. Any permissions change now moves the id
and therefore OTAs; an unchanged block hashes identically, so there is no
spurious update churn.

Note: because the build-id formula changes, every existing image re-ids
once on the first rebuild after this lands, triggering a single OS OTA
even when nothing else changed. Steady state is churn-free thereafter.

@mobileoverlord mobileoverlord left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approach looks right and appropriately scoped — content hash folded in beside the NEVRA hash, one shared helper for both images, computed after the permissions section. Ordering checks out in both generated scripts, and the sysroot os-release gets the same id the manifest patcher reads back (runtime/build.rs:2455), so no split-brain. Migration note is accurate.

One blocking issue inline: the missing-file tolerance doesn't survive pipefail.

Two follow-ups filed rather than piled onto this PR:

  • ENG-2440 — overlay: contents aren't content-hashed by the install stamps, so an overlay edit leaves the sysroot stale and never reaches the image at all. Same blind spot, one layer earlier.
  • ENG-2441 — derive the id from the assembled work tree instead of an enumerated component list, on one shared rootfs/initramfs code path, so this doesn't need a third patch. Notes the HashMapBTreeMap determinism prerequisite (which is also what the LC_ALL=C sort here is working around).

Comment thread src/commands/rootfs/image.rs Outdated
The `2>/dev/null` suppressed the message but not cat's exit status. Under
the `set -euo pipefail` used by the standalone `avocado rootfs image` and
`avocado initramfs image` scripts, a missing file (e.g. a minimal
initramfs with no /etc/shadow) failed the pipeline, the $(…) assignment
took that status, and set -e killed the build with no output at all.

Wrap the cat in a `{ …; } || true` brace group so a missing file no
longer fails the pipeline. Update the exact-output test and add a
pipefail-safety assertion.
Copilot AI lite review requested due to automatic review settings August 18, 2026 17:13

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes ENG-2437 by ensuring permissions:-only changes (which rewrite /etc/{passwd,shadow,group,gshadow} without changing the rpmdb) still change the computed build IDs, so avocado deploy will no longer skip the OS/initramfs bundle download/apply when only auth files changed.

Changes:

  • Added a shared helper render_auth_files_hash(work_var, out_var) that emits a deterministic shell snippet to hash assembled auth files (tolerating missing files and pinning collation).
  • Updated both rootfs and initramfs build-id derivations to compute uuid5(namespace, "$PKG_HASH:$AUTH_HASH") (with initramfs using its corresponding var names), keeping rootfs/initramfs logic aligned via the shared helper.
  • Added/updated unit tests to assert the auth hash snippet shape and verify the build-id input folds in the auth hash and is computed after the permissions section.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
src/commands/rootfs/image.rs Adds shared auth-file hash snippet helper, folds it into rootfs build-id derivation, and adds regression/unit tests for ordering and uuid5 input.
src/commands/initramfs/image.rs Reuses the shared helper from rootfs to fold auth-file hashing into initramfs build-id derivation and adds a matching regression test.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two blocking findings inline, both reproduced. The core intent (fold permissions into the build id so a permissions:-only change OTAs, per ENG-2437) is right and needed; the hash input isn't order-stable yet, and the id rotates unconditionally for every project regardless of whether permissions: is even set. Everything else - the four-file staging guard, the pipefail-safe { …; } || true wrapping, the passwd/shadow sed application - checked out clean (1446 tests pass, clippy/fmt clean, ran the rendered script directly under set -euo pipefail with files missing).

/// the same way.
///
/// `work_var` names the image work-dir shell variable (e.g. `ROOTFS_WORK`,
/// `INITRAMFS_WORK`). `LC_ALL=C sort` makes the hash independent of the

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LC_ALL=C sort can't make this deterministic - the inputs it's sorting are already order-dependent. sort normalizes line order only. It can't normalize the uid/gid values baked into each line, or the member order within one /etc/group line. PermissionsConfig.users/groups are HashMap<String, serde_yaml::Value> (config.rs:840-841), and mapping_from_hashmap (permissions.rs:380) copies them via for (k, v) in map with no sort - Rust's HashMap iteration order is randomized per process. stamps.rs:1283-1292 documents exactly this failure mode for the same reason ("HashMap iteration order varies per process... so the keys have to be sorted here or the hash is unstable between runs") and sorts before hashing; mapping_from_hashmap doesn't.

Ran the real mapping_from_hashmap -> render_users_groups_script path in 4 separate processes with identical input: 3 distinct user-emission orders. For a user declared without an explicit uid: (optional per configs/default.yaml:82-86), that changes the assigned uid/gid, and for two users sharing a supplementary group, it changes the member order within one /etc/group line - both survive the line-level sort and change AUTH_HASH.

Before this PR the build id was PKG_HASH-only and stable across rebuilds. After, a rebuild that changed nothing in the config can OTA the whole fleet, contradicting "deterministic build ID" at line 138. Fix upstream of the hash - sort the keys in mapping_from_hashmap, the same way packages_for_hash already does.

# OTAs (ENG-2437). See render_auth_files_hash.
{auth_hash_block}

OS_BUILD_ID=$(python3 -c "import uuid; print(uuid.uuid5(uuid.UUID('{namespace_uuid}'), '$PKG_HASH:$AUTH_HASH'))")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rotates the build id for every project, even one with no permissions: block at all. The uuid5 input changes unconditionally from '$PKG_HASH' to '$PKG_HASH:$AUTH_HASH', and AUTH_HASH is the digest of the base packages' own passwd/shadow/group - never empty, even with zero permissions: config. The id is appended to os-release before mkfs.erofs runs (line 206 vs 220), so this changes the image bytes, sha256, image_id, spot hashes, and the re-signed AMF for every build across the merge boundary.

First build after this merges, zero config change, zero package change: rootfs + initramfs + os-bundle all re-upload and every device takes a full OS OTA for nothing. Worth a CHANGELOG rollout note - this repo already carries one for smaller changes (the avocado --version entry under [Unreleased]) - so downstream fleets know to expect one unscheduled full OTA rather than reading it as a regression.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants