feat(matic): add filesystem hardening and immutable root protection - #1251
Conversation
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the system's security posture by introducing robust filesystem protections. It aims to prevent common attack vectors related to symbolic and hard links, and provides a critical safeguard against accidental or malicious modification of the root filesystem, ensuring system stability and integrity. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
📝 WalkthroughWalkthroughThe change adds filesystem security hardening to a NixOS host configuration by introducing kernel-level protections through sysctl settings ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Mesa DescriptionTL;DRAdds filesystem hardening and immutable root protection for the What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable security hardening through kernel sysctl parameters and a mechanism to protect the root filesystem from accidental deletion. The filesystem hardening settings are appropriate and well-implemented. However, the approach for creating an immutable root using a systemd service has a critical flaw: it will interfere with the standard NixOS update process (nixos-rebuild switch) by preventing necessary modifications to top-level directories. I have provided a more robust solution using NixOS activation scripts (preActivation and postActivation) that seamlessly integrates with the system update lifecycle, ensuring both protection and maintainability.
| # Immutable root — prevents rm -rf / by blocking top-level entry removal | ||
| systemd.services.immutable-root = { | ||
| description = "Set immutable flag on /"; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig = { | ||
| Type = "oneshot"; | ||
| RemainAfterExit = true; | ||
| ExecStart = "${pkgs.e2fsprogs}/bin/chattr +i /"; | ||
| ExecStop = "${pkgs.e2fsprogs}/bin/chattr -i /"; | ||
| }; | ||
| }; |
There was a problem hiding this comment.
The current implementation using a systemd service to make the root filesystem immutable will break the standard NixOS update process (nixos-rebuild switch). When the service is active, chattr +i / prevents NixOS activation scripts from modifying files and symlinks in top-level directories (e.g., in /etc), which is a necessary part of an update. This will cause nixos-rebuild switch to fail.
A more idiomatic and robust approach in NixOS is to use system.preActivation and system.postActivation scripts. This ensures the root filesystem is automatically made mutable before an update begins and is made immutable again immediately after it completes, requiring no manual intervention.
# Immutable root — prevents rm -rf / by blocking top-level entry removal
# This is handled by pre/post activation scripts to allow `nixos-rebuild switch` to work.
system.preActivation = ''
echo "Making root mutable for system update..."
# Don't fail if the flag isn't set (e.g., on first boot).
${pkgs.e2fsprogs}/bin/chattr -i / || true
'';
system.postActivation = ''
echo "Making root immutable..."
${pkgs.e2fsprogs}/bin/chattr +i /
'';
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@named-hosts/matic/default.nix`:
- Around line 101-109: The unit systemd.services.immutable-root flips /
immutable (ExecStart/ExecStop) but has no ordering constraints; add an After=
stanza to systemd.services.immutable-root so it runs only after filesystem
mounts and any services that may write to / (e.g. local-fs.target and
systemd-remount-fs.service, plus your distro package/update services), or move
its activation to a later target (create/use a late-boot/graphical.target and
set WantedBy accordingly) so the chattr +i runs after boot-critical and
root-mutating units complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c1f6acb-18a9-4314-b688-4fbbf776b2b7
📒 Files selected for processing (1)
named-hosts/matic/default.nix
| systemd.services.immutable-root = { | ||
| description = "Set immutable flag on /"; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig = { | ||
| Type = "oneshot"; | ||
| RemainAfterExit = true; | ||
| ExecStart = "${pkgs.e2fsprogs}/bin/chattr +i /"; | ||
| ExecStop = "${pkgs.e2fsprogs}/bin/chattr -i /"; | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In systemd, if a service is only WantedBy=multi-user.targetand has noAfter=dependency, can it start in parallel with other units wanted bymulti-user.target?
💡 Result:
Yes, a systemd service with only WantedBy=multi-user.target and no After= (or other ordering dependencies like Before=) can start in parallel with other units that are also wanted by multi-user.target. Systemd activates all units wanted by a target (via Wants= or WantedBy=) when the target is reached. Without explicit ordering dependencies (After=/Before=), these units have no required sequence and are scheduled as parallel jobs. Systemd's design emphasizes parallel activation to speed up boot, as confirmed in official documentation: "NB: ordering and requirement dependencies are orthogonal. If only a requirement dependency exists between two units (e.g. foo.service requires bar.service), but no ordering dependency (e.g. foo.service after bar.service) and both are requested to start, they will be started in parallel." (systemd(1) man page). Units wanted by multi-user.target typically share common implicit or default dependencies (e.g., After=local-fs.target), but among themselves, without After=, they activate concurrently.
Citations:
- 1: https://www.man7.org/linux/man-pages/man5/systemd.unit.5.html
- 2: https://fedoramagazine.org/systemd-unit-dependencies-and-order/
- 3: https://www.freedesktop.org/software/systemd/man/latest/systemd.html
- 4: https://unix.stackexchange.com/questions/506930/does-systemctl-consider-service-dependencies-when-starting-or-stopping-multiple
- 5: https://www.freedesktop.org/software/systemd/man/latest/bootup.html
Add explicit After= dependencies or use a later target to prevent boot failures.
The immutable-root service sets the immutable flag on / but lacks ordering constraints. With only WantedBy=multi-user.target and no After=, systemd will activate this unit in parallel with other multi-user services. Any peer service that needs to write to root-level filesystem locations will fail once the immutable bit is flipped (line 107). Either add After= edges for all root-mutating units, or move this behind a later target that executes after boot-critical services complete.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@named-hosts/matic/default.nix` around lines 101 - 109, The unit
systemd.services.immutable-root flips / immutable (ExecStart/ExecStop) but has
no ordering constraints; add an After= stanza to systemd.services.immutable-root
so it runs only after filesystem mounts and any services that may write to /
(e.g. local-fs.target and systemd-remount-fs.service, plus your distro
package/update services), or move its activation to a later target (create/use a
late-boot/graphical.target and set WantedBy accordingly) so the chattr +i runs
after boot-critical and root-mutating units complete.
There was a problem hiding this comment.
Pull request overview
Adds host-level filesystem hardening for the matic NixOS configuration, aiming to reduce common local FS attack vectors and add an extra guardrail against destructive operations on /.
Changes:
- Enables kernel sysctl protections for symlink/hardlink handling and sticky-directory unsafe file creation.
- Adds a
systemdoneshot service to set the immutable attribute on/at boot (and remove it when the unit is stopped).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Immutable root — prevents rm -rf / by blocking top-level entry removal | ||
| systemd.services.immutable-root = { | ||
| description = "Set immutable flag on /"; | ||
| wantedBy = [ "multi-user.target" ]; |
There was a problem hiding this comment.
The comment/PR intent says this “prevents rm -rf /”, but chattr +i / only prevents creating/removing top-level entries under / (e.g., /etc itself). It does not prevent deletion of contents inside existing directories (e.g., rm -rf --no-preserve-root /etc/* would still succeed), so this provides a false sense of protection. Please either (a) reword the comment (and PR description/test plan) to match the actual behavior, or (b) change the approach if the goal is to prevent destructive recursive deletion of the filesystem contents.
| systemd.services.immutable-root = { | ||
| description = "Set immutable flag on /"; | ||
| wantedBy = [ "multi-user.target" ]; | ||
| serviceConfig = { | ||
| Type = "oneshot"; | ||
| RemainAfterExit = true; | ||
| ExecStart = "${pkgs.e2fsprogs}/bin/chattr +i /"; | ||
| ExecStop = "${pkgs.e2fsprogs}/bin/chattr -i /"; | ||
| }; |
There was a problem hiding this comment.
immutable-root is wanted by multi-user.target but has no explicit ordering. To avoid racing with activation/tmpfiles work that might still need to create top-level mountpoints/directories on first boot (or after config changes), consider ordering it after nixos-activation.service, systemd-tmpfiles-setup.service, and local-fs.target (and optionally making it requires = [ "local-fs.target" ]). This makes the hardening less likely to cause boot-time failures.
Summary
fs.protected_regular,fs.protected_fifos,fs.protected_symlinks,fs.protected_hardlinks) to prevent symlink/hardlink attacks and unsafe file creation in sticky directorieschattr +i /at boot, preventingrm -rf /by blocking creation/removal of top-level entries without affecting normal operations within subdirectoriesTest plan
nixos-rebuild switchcompletes without errorslsattr -d /shows immutable flag (i) after bootrm -rf /is blocked (Permission denied on top-level entries)systemctl stop immutable-rootremoves the flag when needed🤖 Generated with Claude Code
Summary by cubic
Adds filesystem hardening and immutable root protection for the matic host. Symlink/hardlink defenses are enabled, and
/is set immutable at boot to block rm -rf / while normal subdirectory operations continue to work.immutable-rootsystemd oneshot service that runschattr +i /at boot and supportsExecStopwithchattr -i /(viae2fsprogs).Written for commit 41230ba. Summary will update on new commits.