fix: one stat-identity model for the target-secret filesystem store - #12
Merged
Conversation
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.
src/auth/targetSecretSourceFsPublishImmutable.test.ts— "fails closed when thefinal inode is replaced" — was red on roughly a third of full-suite runs on
main, blocking #9 and #11. It is not a flaky test. It was a correct testsitting on top of an identity model that could not answer the question it asked.
Two identity models for one store
The auth-owned target-secret store has two filesystem paths, and both of them
re-observe a pathname they already
stated and decide whether they are stilllooking at the same node.
The read path compared
birthtimeNs. The publish path did not. One store, twomodels, and the weaker one on the side that writes.
birthtimeNsis the field that matters here because ext4 recycles inodenumbers freely. The publisher allocates the election token (inode A), then the
final (inode B), then frees A. When the test unlinks the final and recreates it,
ext4 normally hands back A — so
inodiffers and the swap is caught byaccident. Under parallel vitest load a sibling worker takes A first, the
replacement lands back on B, and every field the publish path compared was
identical. Measured on Linux 6.8 / overlayfs, the count of undetected swaps and
the count of recycled inode numbers were the same number in every run.
This change adds
src/auth/targetSecretSourceFsIdentity.ts: oneFileIdentity/DirectoryIdentitypair and four comparators, used by bothpaths.
birthtimeNsis in all four. Everything else is the behaviour that wasalready there, moved.
Why ctime is not the fix
Two independent reasons, both measured rather than reasoned about.
clock — 1 ms on the diagnostic host, 4 ms on GitHub's
CONFIG_HZ=250runners. A ~50 µs unlink/recreate lands in the same tick almost every time.
The "196/200" figure that made ctime look promising came from a tight probe
with no work between the unlink and the recreate, not from this code path.
being observed: a cooperating peer publishing identical bytes appends the
second half of the record between two observations. That is the same reason
sameNodealready omittedsize. AddingctimeMsto it makesjoins identical writers and honors actual short writesfail every time.birthtimeNshas neither problem. It is fixed for the life of an inode andchanges on every reallocation.
ctimeNsstays where it was already safe —sameFileExactonly — and never goes nearsameDirectory, whose ctime thepublisher moves itself on every link and unlink.
On a filesystem that reports no birth time libuv yields
0nor the ctime forevery file, so the comparison degrades to exactly today's behaviour. It can
never produce a false failure, only fail to add one.
What this actually guarantees
Stated exactly, in the module and here:
That is not full inode-replacement detection, and no stat-based scheme can
deliver it: nothing holds a file descriptor pinning the inode across the
lstat-then-openwindow. The old test title claimed the stronger property, soit is retitled to what the design can actually deliver —
"fails closed when the final is unlinked and recreated with identical bytes
after the named observation" — and now sleeps 25 ms before recreating, which
guarantees a tick boundary at HZ >= 100, and asserts that the replacement's
birth time really differs so the run cannot pass green on a filesystem that
lacks the discriminator.
Tests
src/auth/targetSecretSourceFsIdentity.test.tsis the real guard: syntheticBigIntStatspairs, no filesystem, so it behaves identically on ext4, APFS,tmpfs and overlayfs. Mutation-checked — deleting
birthtimeNsfrom any one ofthe four comparators turns it red (verified for all four).
Measured on Linux 6.8 / overlayfs (Node 22), replaying this exact scenario 200
times with two background processes churning inodes in the shared tmpdir:
targetSecretSourceFsPublishImmutablefile, 25 vitest runsIsolated, with no churn, the pre-fix code detected 200/200 — which is exactly
why an isolated green run proved nothing here.
Full gate green (3399 tests, 950 suites, coverage thresholds met), plus the auth
suite run separately on APFS, where nanosecond ctime resolution makes
sameFileExactmaximally strict, to confirm the added field causes no falserejection.
node check-boundaries.mjsclean.Scope
Per
specs/TARGETS.mdthis is a same-uid trusted local-state boundary and aprocess already running as that uid is explicitly outside the threat model, so
this is a P4 security issue and a P1 engineering one. It is on the second count
that it is worth fixing: it unblocks #9 and #11, and it removes a divergence
that would have kept producing surprises.