Skip to content

test(cache-key): cover the six key-file refusals, and make one reachable - #270

Open
guimard wants to merge 1 commit into
mainfrom
test/cache-key-refusals
Open

guimard wants to merge 1 commit into
mainfrom
test/cache-key-refusals

Conversation

@guimard

@guimard guimard commented Sep 8, 2026

Copy link
Copy Markdown
Member

Item 2 of #268.

The gap

4cbe85c added six refusals to the offline cache key file — not a regular file,
not root-owned, setuid, setgid, any group bit, any other bit — and touched no
test file. There was no tests/test_cache_key*. It was the only hardening of
the 0.7.0 series with no proof, and it corrected a real branch-nesting bug: the
ownership test used to sit inside the loose-permissions branch, so a key file
that was mode 0600 and owned by any other uid slipped through entirely.

The test

tests/test_cache_key.c, 16 assertions, includes the .c to reach its statics
(the same thing tests/test_nss_cache.c does). It covers the
0600-owned-by-someone-else case, each group and other bit one at a time, the
0644 root's umask 022 produces from the dd recipe SECURITY.md used to
document, and — so the rest cannot pass vacuously — the two modes that must be
accepted, 0600 and 0400.

What had to change to reach them

As an ordinary user every file a test can create fails the ownership check
first, so the four refusals below it were unreachable in any CI job that is not
root — and a suite that skips proves nothing. The checks move into
key_file_rejected(), which takes the expected owner as an argument.
read_key_file() passes 0, as it always did, and one assertion pins that
value so the tests cannot quietly become a test of themselves. The suite passes
unprivileged and under unshare -r, so the root path CI's Rocky containers take
is exercised too.

A defect it turned up

read_key_file() opened with O_RDONLY | O_NOFOLLOW, and opening a fifo
read-only blocks until a writer appears
. The "not a regular file" refusal
directly below it was therefore unreachable: a fifo at
/etc/open-bastion/cache.key parked the caller forever instead of being
refused, and on the PAM path that is a login that never finishes.

Placing one there needs write access to /etc/open-bastion, so no privilege
boundary is crossed — this is a check that could not fire. O_NONBLOCK makes it
fire and does nothing to a regular file, which is the only thing that gets past
it.

Proof it can fail

Five entries in tests/mutation/catalogue, one per check plus the non-blocking
open:

$ bash tests/test_ob_mutation.sh
  PASS: cache-key-regular-file — tests/test_ob_cache_key.sh catches it
  PASS: cache-key-nonblocking-open — tests/test_ob_cache_key.sh catches it
  PASS: cache-key-root-owner — tests/test_ob_cache_key.sh catches it
  PASS: cache-key-setid-bits — tests/test_ob_cache_key.sh catches it
  PASS: cache-key-group-other-bits — tests/test_ob_cache_key.sh catches it

The wrapper the runner needs imposes a timeout: the O_NONBLOCK mutant hangs
the suite rather than failing it, and the runner has no timeout of its own.
ctest 23/23, tests/test_ob_ci_coverage.sh and tests/test_ob_changelog.sh
green.

4cbe85c added six refusals to the offline cache key file -- not a regular file,
not root-owned, setuid, setgid, any group bit, any other bit -- and touched no
test file. There was no tests/test_cache_key*. It was the only hardening of the
0.7.0 series with no proof, and it also corrected a real branch-nesting bug: the
ownership test used to sit INSIDE the loose-permissions branch, so a key file
that was mode 0600 and owned by any other uid slipped through entirely.

tests/test_cache_key.c now drives each one, 16 assertions, including that
0600-owned-by-someone-else case and the 0644 that root's umask 022 produces from
the `dd` recipe SECURITY.md used to document.

Reaching them needed one change. As an ordinary user every file a test can
create fails the ownership check first, so the four refusals below it were
unreachable in any CI job that is not root -- and a suite that skips is a suite
that proves nothing. The checks therefore move into key_file_rejected(), which
takes the expected owner as an argument; read_key_file() passes 0, as it always
did, and one assertion pins that value so the tests cannot quietly become a test
of themselves.

Writing it turned up a defect. read_key_file() opened with O_RDONLY|O_NOFOLLOW,
and opening a fifo read-only blocks until a writer appears: the "not a regular
file" refusal directly below it was unreachable, and a fifo at
/etc/open-bastion/cache.key parked the caller forever instead of being refused.
On the PAM path that is a login that never finishes. Placing one there needs
write access to /etc/open-bastion, so this is not a privilege boundary being
crossed -- it is a check that could not fire. O_NONBLOCK makes it fire and does
nothing to a regular file, which is the only thing that gets past it.

Five catalogue entries, one per check plus the non-blocking open, all caught.
The wrapper the runner needs imposes a timeout, because the O_NONBLOCK mutant
hangs the suite rather than failing it, and the runner has no timeout of its own.

ctest 23/23, mutation 5/5 on the new entries, both as an ordinary user and under
`unshare -r` so the root path in CI's Rocky containers is exercised too.

Closes part 2 of #268.
@guimard
guimard force-pushed the test/cache-key-refusals branch from 1ab0f1b to a5be0a2 Compare September 8, 2026 12:44
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.

1 participant