Skip to content

fix(daemon): fail closed when a peer cannot be pinned - #382

Merged
vladimirrott merged 3 commits into
lacs-project:mainfrom
QinXi-ai:fix/250-peer-pin-failures
Sep 7, 2026
Merged

vladimirrott merged 3 commits into
lacs-project:mainfrom
QinXi-ai:fix/250-peer-pin-failures

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

A failed SO_PEERPIDFD lookup previously enabled the old-kernel fallback even when the peer had already been reaped. The daemon now distinguishes Pinned, Unsupported (only ENOPROTOOPT), and Unpinnable; other failures skip the supplementary-group lookup and retain the primary GID captured by SO_PEERCRED.

Tests cover errno classification, preservation of the unsupported-kernel path, and primary-group-only caller resolution with an injected unpinnable peer. The existing live-self test now rejects unexpected pin failures. Two Rust tests were added.

Validation on Windows: cargo fmt --all -- --check, cargo metadata --locked --no-deps --format-version 1, python -X utf8 scripts/check_evidence_claims.py, and git diff --check passed. cargo test -p sysknife-daemon --lib peer_pin_only_unsupported_option_allows_best_effort_groups --locked could not compile the unchanged vsock dependency (missing Unix/VSOCK libc and std APIs), so no Rust execution result is claimed locally. Linux kernel 5.15/6.8 live validation is NOT_TESTED locally.

Following CONTRIBUTING.md's supported fallback, the four published test-count files are untouched; maintainer regeneration is needed before merge. Codex assisted with this change.

Closes #250.

Linux CI at commit 9eecc1c: 1839 tests run, 1839 passed, 6 skipped. Both added tests and the live-self pidfd test passed. The rust job fails only at the supported-fallback count check (1839 measured versus 1837 recorded); see https://github.com/lacs-project/sysknife/actions/runs/34081372750/job/101617144278. The postgres-contract, docs-and-hygiene, frontend, security-audit and scripts-lint jobs passed.

…tests

lacs-project#382 adds two dispatcher tests (errno classification, and an unpinnable peer
keeping only its primary group). The published baseline and the three prose
claims move with them. The CHANGELOG conflict came from me populating
[Unreleased] after this PR was opened; resolved here, keeping both entries.

@vladimirrott vladimirrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Approving at e4cf823, which carries a merge and a baseline commit of mine; both are explained at the end and neither touches your code.

The insight here is that rc != 0 was being read as one thing when it is two. Only ENOPROTOOPT means "this kernel does not have the option"; EINVAL means "the peer you are asking about is already gone", and the old code answered both with None and then took that as permission to go read /proc/{pid}. Which is the one situation where the PID may already belong to somebody else. Splitting Unsupported from Unpinnable is the whole fix, and it is a two-line idea buried in an errno.

Passing read_groups in rather than calling it is what makes the dangerous branch testable. Forcing real PID reuse in a test is somewhere between flaky and impossible, and you sidestepped it without weakening what is asserted.

I ran it on Linux, since you said plainly that you could not. That statement is worth more than a green checkbox, and it told me exactly where to point the machine.

S0  peer_pin_only_unsupported_option_allows_best_effort_groups ... ok
    peer_pidfd_reports_live_self ....................... ok
    unpinnable_peer_keeps_only_its_primary_group ....... ok
    3 passed

M1  classify every errno as Unsupported (re-open the hole)
    -> FAILED, "errno 22"

M2  Unpinnable returns read_groups() instead of dropping them
    -> FAILED, "an unpinnable peer must not read a potentially recycled PID"

Both mutations are caught, and M2's message names the actual hazard rather than the assertion. The property that matters holds: a peer that cannot be pinned, whose PID happens to sit in an admin group, resolves to Observer.

I checked the fd < 0 arm too. getsockopt returning 0 with a negative fd is not a case anyone has seen, and treating it as Unpinnable rather than trusting it is the right default for a function whose output decides an authorization level.

Two commits of mine on your branch.

The CHANGELOG conflict was mine to create and mine to fix: I populated [Unreleased] earlier today with entries for four PRs that had merged without them, after you opened this. I merged main and kept both entries, yours leading the Fixed section. Nothing of yours was dropped.

The baseline moved 1,843 to 1,845 for your two tests, across tests/evidence/workspace-tests.json and three prose claims. You were right to leave it alone and say so; CONTRIBUTING documents that fallback and I would rather contributors not have to learn the coupling.

Merging when the board is green. No new issue from me: you have #383 and #384 open with reviews on both, and I released the three I had reserved for you earlier today so you would have room rather than a queue.

@vladimirrott
vladimirrott merged commit 7eca3f3 into lacs-project:main Sep 7, 2026
12 checks passed
@vladimirrott vladimirrott mentioned this pull request Sep 7, 2026
@vladimirrott

Copy link
Copy Markdown
Member

Narrowing Unsupported to ENOPROTOOPT alone is the whole fix, and it is the
kind of change that is easy to get almost right.

The old code read any SO_PEERPIDFD failure as "this kernel is too old" and
turned on the best-effort path, so a peer that had already been reaped bought a
supplementary-group lookup it should never have got. Three states instead of two,
with only the one errno that actually means "the kernel does not have this"
mapping to the fallback, makes the ambiguous case fail closed. Everything else
keeps the primary GID from SO_PEERCRED and stops there.

The second thing I want to name is the validation section. You wrote that
cargo test -p sysknife-daemon --lib ... could not compile vsock on Windows
and that you were therefore claiming no Rust execution result, and you marked the
5.15/6.8 live check NOT_TESTED rather than leaving it implied. A PR that says
which command did not run is worth more to me than one that reports a pass I
cannot place. It is also the harder thing to write when you want the patch to
land.

Merged at 7eca3f3. The count files you left alone have been regenerated and the
workspace is green on them:

$ cargo nextest run --workspace --locked
     Summary [  10.351s] 1845 tests run: 1845 passed, 6 skipped
$ python3 scripts/check_evidence_claims.py
Published figures match the evidence artifacts.

Both issues behind your open PRs are now assigned to you, not only labelled, so
they reach your dashboard:

$ gh issue view 237 --repo lacs-project/sysknife --json assignees --jq '[.assignees[].login]'
["QinXi-ai"]
$ gh issue view 362 --repo lacs-project/sysknife --json assignees --jq '[.assignees[].login]'
["QinXi-ai"]

#383 and #384 are the next things in the queue and I have not reviewed either
yet. Nothing new from me until those two are settled.

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.

peer_pidfd cannot tell a pre-6.5 kernel from a peer that already exited, so the PID-reuse check is off in the reuse case

2 participants