Verify a source root lost+found on both sides (#959) - #961
Merged
Conversation
The metadata manifests dropped `./lost+found` symmetrically so that a relocation onto a freshly made ext4 reserve could close: `mke2fs` seeds that directory in the destination root and the holding directory normally has no counterpart, so the comparison would fault every correct migration. But the exemption also fired on the side the copy came from. A holding directory that legitimately holds a root `lost+found` — the whole source tree of the replacement procedure, whose source is itself a mounted reserve — had that directory's type, mode, numeric ownership and link count compared against nothing at all, and a mismatch on it passed the gate that guards the closing rename. The exemption now belongs to the destination side alone, and even there it is conditional: the rendered shell asks whether the source root carries a `lost+found` directory and takes the unfiltered walk when it does. The predicate is `-d` together with not `-L`, because `test -d` answers for a symbolic link's target and this procedure follows none — which is what `find -type d` also does, so the two agree on every entry. The branch is a plain `if`, not a pipeline, so a failed walk inside it still stops the sequence. Closes #959
The destination walk's exemption turns on what the source root holds, so each shape that root can present it with is a separate branch of the same rule and only some were exercised. A source `lost+found` holding files had one same-sized difference checked; the metadata and size passes reach underneath it too, and each now breaks on its own over a copy that had already passed. A non-directory bearing the name is planted at the destination root as well as the source, which is where the exemption is actually spelled and so where `-type d` has to be doing the work. The conditional's fail-closed walk is driven down both of its branches rather than the exempting one alone. A symbolic link at the source root under that name went untested altogether, and it is the reading `! -L` exists to rule out: `test -d` answers for a link's target, so one pointing at a directory would otherwise send the destination down the unfiltered walk. The type guard refuses it before the question is asked, which is the property worth pinning. Part of #959
The ownership case labelled its assertion with the uid and gid it had just set, and CodeQL's `rust/cleartext-logging` reads any value bound to `uid` reaching a message as a leak — a red required check over two loop constants in a Linux-only test module. The message says which manifest field the case moves instead, which is what a failure needed to distinguish anyway. Part of #959
Contributor
Author
|
[Reviewer Round 1] Review: no findings. I would approve PR #961. The destination-only conditional correctly preserves ext4’s seeded The tests meaningfully cover the new branches: source/destination metadata mismatches, descendants, same-named non-directories, symlink refusal, and both conditional paths. English and Korean documentation match the rendered behavior. PR linkage and test-plan hygiene are also correct. |
Contributor
Author
|
[Review Verdict Round 1: APPROVED] |
Contributor
Author
Suggested squash commitTitle Body |
9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this changes
The migration metadata manifests dropped
./lost+foundfrom both sides. That exemption exists becausemke2fsseeds alost+founddirectory in the root of every filesystem it makes, and that root is the destination of the relocation copy — without it, the destination carries one record the holding directory never had and no correct migration could ever verify.Applied symmetrically, it also silenced the source side. A holding directory that legitimately holds a root
lost+found— the whole source tree of the replacement procedure, whose source is itself a mounted reserve — had that directory's type, mode, numeric uid, numeric gid and link count compared against nothing at all, and a mismatch on it passed the gate that guards the closing rename.The exemption now belongs to the destination side alone, and even there it is conditional. The rendered shell asks, at the moment it runs, whether the source root carries a
lost+founddirectory:The source walk carries no filter at all — a record the copy did not put there cannot arise on the side the copy came from.
The predicate is
-dand not-Lrather than-dalone:test -danswers for a symbolic link's target, and nothing in this procedure follows one.find -type ddoes not either, so the two spellings agree on every entry.Everything else stands: the walks are NUL-delimited, each stage is its own command over its own staged file, the three manifests are byte-compared with
cmp, and no recursive delete is introduced. The branch is a plainif, not a pipeline, so a failed walk inside it still stops the sequence —set -efires on the branch body, and there is a test for exactly that, driven down both branches.Also here: a CodeQL
rust/cleartext-loggingalert. The ownership case labelled its assertion with the uid and gid it had just set, and the query reads any value bound touidreaching a message as a leak — a red required check over two loop constants in a Linux-only test module. The message names the manifest field the case moves instead, which is what a failure needed to distinguish anyway.Acceptance criteria
lost+foundverifies successfully — theelsebranch, covered bythe_reserves_own_lost_and_found_does_not_fail_the_verificationand by theregistrar-internal-initE2E scenario, which now also asserts the conditional is rendered and that the branch taken really was the exempting one.lost+foundmismatch fails the comparison —a_mismatched_source_root_lost_and_found_fails_the_verificationbreaks type, mode and link count in turn over a copy that had already passed;a_reowned_source_root_lost_and_found_fails_the_verificationdoes uid and gid where the run has the privilege tochown, and the rendered%y %m %U %G %n %Pform is asserted unconditionally beside it.lost+foundstay in every comparison —everything_under_lost_and_found_is_still_comparednow breaks the metadata, size and content passes separately, each over a copy that had already passed, rather than only the same-sized content case.lost+foundstays under the ordinary rules —a_regular_file_merely_bearing_the_name_is_not_exemptat the source root, anda_non_directory_bearing_the_name_at_the_destination_root_is_not_exemptat the destination root, which is where the exemption is actually spelled and so where-type dhas to be the thing doing the work.a_symlink_bearing_the_name_at_the_source_root_is_refused_by_the_guardcovers the third shape the name can take: a link to a real directory, whichtest -dalone would read as a directory, and which the type guard refuses before the question is ever asked.cmp-compared —the_rendered_migration_commands_are_pinned_exactlypins the whole sequence verbatim and re-checks the pipe,diff -r,rm -rand trailing-field bans.both_manuals_pin_the_asymmetric_lost_and_found_exemptionasserts each page's relocation block carries the exclusion exactly once and carries the conditional, so a symmetric form cannot come back in either language.Test plan
The
rendered_sequencemodule is Linux-only (GNUfind -printf,sort -z,sha256sum --zero), so it was run in arust:latestcontainer; everything else was run on macOS as well.lost+foundverifies, and the destination's seeded directory is not treated as audit datalost+foundwhose type, mode, numeric uid, numeric gid or link count differs fails the metadata comparison and stops before the closing renamelost+foundstill participate in the metadata, regular-file-size and content comparisons — each broken separatelycargo test --bin bootroot audit_store::migration— 27 passed, in the container both as root (so the uid/gid case ran) and as an unprivileged user (so the fail-closed-inside-the-conditional case ran, down both of its branches)cargo test— green on macOS, and green in the Linux container (lib 1269, bin 1303, plus the other bins and integration suites). The 8tests/bootroot_rotate.rscases that fail in that container fail on a missingdockerCLI and pass locally on macOS where Docker is present:46 passed; 0 failed. The container's ownumaskmatters: at the image default of002theregistrar::audit::scanandregistrar::openbao_auditsuites refuse their own fixtures as group-writable, which is the safe-path check working rather than a failure of this branchcargo clippy --all-targets -- -D warnings— clean on macOS and Linuxcargo fmt -- --config group_imports=StdExternalCrate --check— clean on macOS and Linux./scripts/check-docs.sh—mkdocs build --strictpasses and the theme verification passesmarkdownlint-cli2over the repository — 0 issuesshellcheck -x scripts/impl/run-registrar-internal-init-e2e.sh— clean (onlySC1091info for sourced libs, resolved when run from the script's directory)Docker E2E (registrar-internal-init)Local run notes
scripts/preflight/ci/e2e-matrix.shdid not run here. Its step 13 —run-registrar-internal-init-e2e.sh, the one arm that exercises this rendering end to end — requires passwordlesssudo, andsudo -n trueon this machine reports "a password is required"; the matrix's own usage text states--skip-hostsdoes not stand in for it. Port 8200 is also held by an unrelated live process from another session, which the earlier steps' port preflight refuses to start over, and tearing that down is not mine to do. CI'sDocker E2Ejobs gate that arm, andDocker E2E (registrar-internal-init)is green. Nothing was weakened, skipped, markedcontinue-on-erroror deleted to get a local run to pass. What did run locally is therendered_sequencesuite, which executes the rendered shell for real against fixture trees on Linux, including everylost+foundshape on both sides.CHANGELOG.mdentry. The audit-store reserve, its relocation procedure and its verification are all announced under[Unreleased]; no user of the last release ever saw the symmetric exemption, so this is a rework of unreleased work rather than an observable change.%nis two plus its subdirectory count, so no fixture can move that field alone without also changing the entries the manifest lists underneath. The link-count case therefore faults on both, which still demonstrates the field is compared rather than skipped; it is not an isolated one-field test and the code comment says so.Closes #959
Part of #926