test: run permission-denied tests as an unprivileged user when root#10633
Merged
basvandijk merged 7 commits intoJul 3, 2026
Conversation
Add the ic-test-utilities-privileges crate exposing run_as_nobody_if_root, which runs a test closure in a forked child that drops to the nobody user when the process is root, and in-process otherwise. Root holds CAP_DAC_OVERRIDE and therefore bypasses filesystem permission bits, so tests asserting PermissionDenied instead observe the operation succeeding and fail when run as root (e.g. on Bazel remote-execution workers). The helper forks, redirects TMPDIR/TEST_TMPDIR to a nobody-owned base, drops supplementary groups, gid, and uid to nobody, and runs the whole test body there; the child's outcome (including any panic message) is mirrored to the parent so #[should_panic] and its expected message keep working. Use it in the affected permission tests in rs/sys, the crypto service provider, and rs/state_manager.
nix 0.24.3 configures `setgroups` out on Apple targets, so the `imp` module failed to compile on darwin. The helper's semantics (the Linux `nobody` uid 65534 and CAP_DAC_OVERRIDE) are Linux-specific anyway, so gate `imp` on `target_os = "linux"` and fall through to the in-process no-op on the other platforms.
The tests test_ensure_file_exists_and_is_writeable_fails_if_non_writeable and test_save_proposal_id_to_file_fails_if_write_fails assert PermissionDenied on a read-only file. Root holds CAP_DAC_OVERRIDE and bypasses filesystem permission bits, so under Bazel remote-execution (running as root) the operations succeed and the assertions fail. Wrap both test bodies in ic_test_utilities_privileges::run_as_nobody_if_root so they drop to the nobody user when the process is root, mirroring the treatment applied in 380255a.
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a small Rust test-utility crate to make filesystem permission-denied assertions reliable when tests execute as root (e.g., on remote-execution workers), and updates affected tests across several crates to run those assertions as an unprivileged user.
Changes:
- Introduces
ic-test-utilities-privilegeswithrun_as_nobody_if_root(Linux: fork + drop to uid/gid 65534; non-Linux: no-op). - Wraps permission-sensitive tests in
rs/sys,rs/state_manager,rs/crypto/internal/crypto_service_provider, andrs/sns/cliwithrun_as_nobody_if_root. - Wires the new crate into Cargo workspace and Bazel targets (plus a small formatting-only change in root
Cargo.toml).
Reviewed changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| rs/test_utilities/privileges/src/lib.rs | New helper that forks and drops privileges to nobody when running as root. |
| rs/test_utilities/privileges/Cargo.toml | Defines new test-utility crate and its dependency on nix. |
| rs/test_utilities/privileges/BUILD.bazel | Bazel rust_library target for the new helper crate. |
| rs/sys/src/fs.rs | Wraps permission-denied assertions to run unprivileged when root. |
| rs/sys/Cargo.toml | Adds new helper crate as a dev-dependency. |
| rs/sys/BUILD.bazel | Adds Bazel dependency on the new helper crate for tests. |
| rs/state_manager/src/checkpoint/tests.rs | Wraps permission-sensitive checkpoint tests with the unprivileged runner. |
| rs/state_manager/Cargo.toml | Adds new helper crate dependency. |
| rs/state_manager/BUILD.bazel | Adds Bazel dependency on the new helper crate for tests. |
| rs/sns/cli/src/propose/propose_tests.rs | Wraps permission-denied tests for file writability with the unprivileged runner. |
| rs/sns/cli/Cargo.toml | Adds new helper crate as a dev-dependency. |
| rs/sns/cli/BUILD.bazel | Adds Bazel dependency on the new helper crate for tests. |
| rs/crypto/internal/crypto_service_provider/src/secret_key_store/proto_store/tests.rs | Wraps SKS permission-denied tests so they behave correctly under root. |
| rs/crypto/internal/crypto_service_provider/src/public_key_store/proto_pubkey_store/tests.rs | Wraps pubkey store permission tests so they behave correctly under root. |
| rs/crypto/internal/crypto_service_provider/Cargo.toml | Adds new helper crate dependency. |
| rs/crypto/internal/crypto_service_provider/BUILD.bazel | Adds Bazel dependency on the new helper crate for tests. |
| Cargo.toml | Adds the new crate to the workspace members; formats opentelemetry-otlp entry. |
| Cargo.lock | Locks the new crate into the dependency graph. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
* Terminate the forked child with _exit(2) instead of std::process::exit, which would run atexit(3) handlers and stdio flushing inherited from the (potentially multi-threaded) parent test process. * Make the child's temp base nobody-owned with mode 0700 instead of root-owned world-writable 0777, matching the documentation and avoiding a predictable world-writable directory created as root. Create it with create_dir rather than create_dir_all so a pre-existing directory of unknown ownership is rejected. * Make the fork path safe in a multi-threaded test process without forcing --test-threads=1 on whole test suites: the temp base and putenv(3) NAME=VALUE strings are prepared in the parent before forking, and the child no longer calls std::env::set_var (whose global environment lock another parent thread could have held at fork time), using putenv with the pre-allocated strings instead. * Add smoke tests for the helper, including a concurrency regression test that forks from multiple threads; verified as root, where they exercise the fork + privilege-drop path.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
This pull request changes code owned by the Governance team. Therefore, make sure that
you have considered the following (for Governance-owned code):
-
Update
unreleased_changelog.md(if there are behavior changes, even if they are
non-breaking). -
Are there BREAKING changes?
-
Is a data migration needed?
-
Security review?
How to Satisfy This Automatic Review
-
Go to the bottom of the pull request page.
-
Look for where it says this bot is requesting changes.
-
Click the three dots to the right.
-
Select "Dismiss review".
-
In the text entry box, respond to each of the numbered items in the previous
section, declare one of the following:
-
Done.
-
$REASON_WHY_NO_NEED. E.g. for
unreleased_changelog.md, "No
canister behavior changes.", or for item 2, "Existing APIs
behave as before.".
Brief Guide to "Externally Visible" Changes
"Externally visible behavior change" is very often due to some NEW canister API.
Changes to EXISTING APIs are more likely to be "breaking".
If these changes are breaking, make sure that clients know how to migrate, how to
maintain their continuity of operations.
If your changes are behind a feature flag, then, do NOT add entrie(s) to
unreleased_changelog.md in this PR! But rather, add entrie(s) later, in the PR
that enables these changes in production.
Reference(s)
For a more comprehensive checklist, see here.
GOVERNANCE_CHECKLIST_REMINDER_DEDUP
|
✅ No security or compliance issues detected. Reviewed everything up to e224f9f. Security Overview
Detected Code Changes
|
|
✅ No security or compliance issues detected. Reviewed everything up to e224f9f. Security Overview
Detected Code Changes
|
daniel-wong-dfinity-org-twin
approved these changes
Jul 2, 2026
Requested in review: each of the 12 wrapped tests now carries a short comment explaining that root bypasses file permission bits (CAP_DAC_OVERRIDE), so the permission denial the test expects only happens when dropping to nobody on root-run workers (e.g. Bazel remote execution).
Requested in review: wrapping whole test bodies in a closure pollutes them with extra code and indentation. Add an attribute that does the wrapping instead. Proc-macros must live in their own crate, so the attribute is defined in the new ic-test-utilities-privileges-macros crate and re-exported from ic-test-utilities-privileges (the serde/serde_derive facade pattern, like canlog re-exports canlog_derive); consumers keep a single dependency. The attribute composes with #[test] and #[should_panic] (the wrapper re-raises the child's panic message) and rejects async fns and non-unit return types at compile time. Convert all 12 call sites to the attribute form and extend the smoke tests with attribute-form cases; verified as root, where the permission-denied tests only pass because the attribute drops privileges to nobody.
alin-at-dfinity
approved these changes
Jul 3, 2026
alin-at-dfinity
left a comment
alin-at-dfinity
left a comment
Contributor
There was a problem hiding this comment.
LGTM, thank you.
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
Supports running rust_tests, that assume being run as an unprivileged user, as root.
Spin off from the RBE @ Namespace experiment.
Why
In certain environments, like on a Remote Build Execution cluster @ Namespace, bazel runs as root. This breaks several tests that assert
PermissionDeniedon files whose permission bits have been restricted. Root holdsCAP_DAC_OVERRIDEand bypasses filesystem permission bits, so when run as root the operations succeed and the tests fail.How
This adds the
ic-test-utilities-privilegescrate exposingrun_as_nobody_if_root, which runs a test closure in a forked child that drops to thenobodyuser when the process is root (redirectingTMPDIR/TEST_TMPDIRto anobody-owned base and mirroring the child's outcome — including panic messages — so#[should_panic]keeps working), and runs in-process otherwise. The helper is gated to Linux since the semantics (uid 65534,CAP_DAC_OVERRIDE) are Linux-specific andnix0.24.3 lackssetgroupson Apple targets.Since wrapping a whole test body in a closure pollutes it with extra code and indentation, the companion proc-macro crate
ic-test-utilities-privileges-macrosprovides the#[as_nobody_when_root]attribute, which performs that wrapping at compile time. Proc-macros must live in their own crate, so it is re-exported throughic-test-utilities-privileges(theserde/serde_derivefacade pattern, likecanlogre-exportscanlog_derive) and consumers keep a single dependency. The attribute composes with#[test]and#[should_panic], and rejects attribute arguments,async fns, and non-unit return types at compile time.The attribute is applied to the affected permission tests in
rs/sys, the crypto service provider,rs/state_manager, andrs/sns/cli.Tested on a RBE cluster running as root @ Namespace in #10579 and manually verified by running
sudo bazel test.