Harden PCP systemd units with minimal, service-only changes#2655
Conversation
Add least-privilege directives to unit files only; omit SystemCallFilter, ProtectHostname on pmcd, and PrivateTmp on farm services to avoid performance impact and preserve existing behavior.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbit
WalkthroughSystemd service templates across PCP components now add sandboxing, privilege, namespace, kernel-interface, keyring, and syscall-architecture restrictions. ChangesSystemd service hardening
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pcp/geolocate/pcp-geolocate.service.in`:
- Around line 12-27: Harden all three units by adding UMask=0027 immediately
after KeyringMode=private in src/pcp/geolocate/pcp-geolocate.service.in (lines
12-27), src/pmproxy/pmproxy_check.service.in (lines 14-30), and
src/pmproxy/pmproxy_daily.service.in (lines 14-30); no other service settings
need to change.
In `@src/pmie/pmie_check.service.in`:
- Around line 17-32: Add UMask=0027 to the hardening settings in
src/pmie/pmie_check.service.in lines 17-32 and src/pmie/pmie_daily.service.in
lines 16-32, matching the existing PMIE units; only document an exception
instead if either service intentionally requires different file-creation
permissions.
In `@src/pmie/pmie_farm_check.service.in`:
- Around line 16-32: Document the intentional omission of RemoveIPC=yes in the
hardening settings near the existing NoNewPrivileges comment, briefly explaining
the root-running service’s IPC cleanup requirement if applicable. Do not add the
directive; only add a concise rationale consistent with the surrounding
comments.
In `@src/pmie/pmie.service.in`:
- Around line 23-38: Document the intentional omission of ProtectHostname and
PrivateTmp in pmie.service.in by adding a brief inline comment near the related
hardening directives. State the required runtime behavior, such as
hostname-change detection or /tmp access for inference temporary files, and
follow the exception-documentation style used by the other service units.
In `@src/pmproxy/pmproxy.service.in`:
- Around line 14-30: Document the intentional omission of PrivateTmp in the
hardening settings for src/pmproxy/pmproxy.service.in lines 14-30 and
src/pmcd/pmcd.service.in lines 14-30 by adding brief inline comments explaining
why shared /tmp access is required, matching the existing
exception-documentation style; no other service settings need changes.
- Around line 14-30: Remove the RemoveIPC=yes directive from the pmproxy systemd
unit hardening settings, leaving the surrounding protections unchanged.
🪄 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: Repository YAML (base), Repository UI (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: e33c0695-89fe-4107-afa9-623997cb6a7d
📒 Files selected for processing (19)
src/pcp/atop/atop-daily.servicesrc/pcp/atop/atop.service.insrc/pcp/geolocate/pcp-geolocate.service.insrc/pmcd/pmcd.service.insrc/pmfind/pmfind.service.insrc/pmie/pmie.service.insrc/pmie/pmie_check.service.insrc/pmie/pmie_daily.service.insrc/pmie/pmie_farm.service.insrc/pmie/pmie_farm_check.service.insrc/pmlogger/pmlogger.service.insrc/pmlogger/pmlogger_check.service.insrc/pmlogger/pmlogger_daily.service.insrc/pmlogger/pmlogger_farm.service.insrc/pmlogger/pmlogger_farm_check.service.insrc/pmproxy/pmproxy.service.insrc/pmproxy/pmproxy_check.service.insrc/pmproxy/pmproxy_daily.service.insrc/pmseries/pmseries_import.service.in
| # Hardening: no SystemCallFilter (performance impact). | ||
| # NoNewPrivileges omitted because pmiectl uses runaspcp (setuid). | ||
| PrivateTmp=yes | ||
| ProtectHome=yes | ||
| ProtectClock=yes | ||
| ProtectHostname=yes | ||
| ProtectKernelLogs=yes | ||
| ProtectKernelModules=yes | ||
| ProtectControlGroups=yes | ||
| ProtectKernelTunables=yes | ||
| RestrictRealtime=yes | ||
| RestrictSUIDSGID=yes | ||
| LockPersonality=yes | ||
| RestrictNamespaces=yes | ||
| SystemCallArchitectures=native | ||
| KeyringMode=private | ||
| UMask=0027 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the omission of RemoveIPC=yes.
All other PMIE service units in this PR include RemoveIPC=yes, but pmie_farm_check.service.in omits it without comment. If this is intentional (e.g., the root-running service must not strip IPC objects on exit), add a brief comment explaining why, consistent with the documented NoNewPrivileges exception.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pmie/pmie_farm_check.service.in` around lines 16 - 32, Document the
intentional omission of RemoveIPC=yes in the hardening settings near the
existing NoNewPrivileges comment, briefly explaining the root-running service’s
IPC cleanup requirement if applicable. Do not add the directive; only add a
concise rationale consistent with the surrounding comments.
| # Hardening: no SystemCallFilter (performance impact). | ||
| ProtectHome=yes | ||
| NoNewPrivileges=yes | ||
| RemoveIPC=yes | ||
| ProtectClock=yes | ||
| ProtectKernelLogs=yes | ||
| ProtectKernelModules=yes | ||
| ProtectControlGroups=yes | ||
| ProtectKernelTunables=yes | ||
| RestrictRealtime=yes | ||
| RestrictSUIDSGID=yes | ||
| LockPersonality=yes | ||
| RestrictNamespaces=yes | ||
| SystemCallArchitectures=native | ||
| KeyringMode=private | ||
| UMask=0027 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the omission of ProtectHostname and PrivateTmp.
All other PMIE service units in this PR include ProtectHostname=yes, and most include PrivateTmp=yes (or document its absence, as pmie_farm.service.in does). pmie.service.in omits both without any inline comment explaining why. If these omissions are intentional (e.g., pmie needs hostname-change detection like pmcd.service, or needs /tmp access for inference temp files), add a brief comment for consistency with the documented exceptions in the other units.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/pmie/pmie.service.in` around lines 23 - 38, Document the intentional
omission of ProtectHostname and PrivateTmp in pmie.service.in by adding a brief
inline comment near the related hardening directives. State the required runtime
behavior, such as hostname-change detection or /tmp access for inference
temporary files, and follow the exception-documentation style used by the other
service units.
Leave the default umask so PMDA Install/make artifacts stay world-readable/executable; UMask=0027 broke sample PMDA and CI QA.
Drop ProtectKernelModules and RestrictNamespaces from pmcd.service so the dm PMDA can ioctl /dev/mapper/control and run dmsetup; fixes qa/359 on Ubuntu 20.04/22.04 CI.
|
@kurik are you proposing this for the 7.1.6 release? Do you have empirical data to show that there's no significant performance impact here (the syscall assertion in issue #2644 is speculation, I don't have any data (yet) to prove this is the root cause of the slowdown I observed in my parallel efforts down this path). Of course, I have no visibility into any firm customer-pressure to address this quickly which would be a non-technical counter argument. 😄 |
No, I am not proposing this for 7.1.6 release. It still might need some work as there are some differences on Debian (as I see in the CI).
I do not have the data anymore, but I can easily generate it. I was using
There is no pressure on my side. |
|
Excellent @kurik, let's try and nail this down as early as possible in the next release cycle. So I don't forget, tasks I can see are:
|
* Removed "RemoveIPC" from all PCP's unit files to avoid unexpected data lost * Tweaked pmcd.service to ensure proper work of dm PMDA
|
Hi @kmcdonell , As part of this exercise I tried to compare performance with and without this hardening. At first, I run all tests in the Average performance impact
Largest single spike (not reproducible):
Largest reproducible slowdown (both hardened runs):
Reproducible speedups (both hardened runs faster):
Conclusion:The data does not support a measurable suite-wide performance cost. The only plausibly hardening-related reproducible case is 1845. One more comment:I haven't yet investigated why the success rate is higher with hardening enabled compared to the baseline run. I know some tests are unstable, but this still warrants a closer look when I have some spare time:
|
That would be great. What I learned during this exercise is the fact there might be some side effects one (at least me :-) ) do not expect. Specifically I mean As such, I definitely agree with you to merge these changes early in the release cycle, so there is as much time as possible to catch possible issues.
This is great. I tried to come up with a minimal set of hardening that wouldn't even affect the test suite. However, it seems that settings like
I am not sure how much we can have some controlled environment when nowadays is "everything" virtual, even the hardware :-) |
2 participants
Add least-privilege directives to unit files only; omit SystemCallFilter, ProtectHostname on pmcd, and PrivateTmp on farm services to avoid performance impact and preserve existing behavior.
Patch description
This change hardens PCP systemd service units for issue #2644
(#2644) by adding common least-privilege directives
(ProtectHome, ProtectClock, ProtectKernel*, RestrictNamespaces, UMask=0027, etc.) to the main PCP services
and their timer/check/farm companions.
The patch is intentionally minimal in scope: it touches only systemd unit template files (*.service.in) and
packaging metadata. There are no changes to application code, libraries, PMDAs, or rc scripts.
SystemCallFilter is deliberately not used, based on upstream discussion that syscall filtering caused large
QA slowdowns (e.g. qa/966 going from ~17s to ~89s).
Omitted hardening options
Two options that would otherwise be straightforward to add were left out after test-suite analysis:
ProtectHostname=yesonpmcd.serviceIn normal production use, this is unlikely to matter much: pmcd reads the hostname at startup and most
deployments do not change the system hostname at runtime.
It does break hostname-following behavior that PCP relies on when the hostname changes. pmcd detects changes
via
gethostname()and signals clients withPMCD_HOSTNAME_CHANGE; pmlogger and pmie then restart to pick upthe new name. With
ProtectHostname=yes, pmcd sees a frozen hostname namespace and never propagates thatchange.
Test impact: qa/1441 failed — after a hostname change, the primary pmlogger never restarted and still
referenced the old hostname.
Status: Omitted from pmcd.service with an inline comment. This is a reasonable hardening candidate later, but
would need either a different pmcd hostname-detection approach or test rework.
PrivateTmp=yesonpmie_farm.serviceandpmlogger_farm.serviceFor typical deployments, farm instances write logs under
/var/log/pcp/... from control files, soPrivateTmpmay have little practical effect.
It does break non-primary farm instances that use log paths outside the service’s private
/tmp— includinguser-configured locations and QA tests that deliberately use
/tmp/<test-pid>/....Test impact: qa/1060 failed — the non-primary pmie instance could not create its nested log tree under
/tmp/1060-*.
Status: Omitted from both farm service units with an inline comment. Low-hanging fruit for a future hardening
pass, but farm log paths are user-configurable, so proper validation would need more than a quick unit-file
change.
What was added
Hardening was applied across pmcd, pmproxy, pmlogger, pmie, pmfind, atop, geolocate, pmseries, and their
check/daily/farm companion units, with service-specific exceptions documented in comments (e.g.
NoNewPrivilegesomitted on pmcd because it drops to the pcp user via setuid;NotifyAccess=mainon pmproxy).Rationale for deferring
ProtectHostnameandPrivateTmpBoth options are attractive from a
systemd-analyze securityperspective and would likely be fine in manyproduction setups. They were not included in this iteration because:
enough confidence without broader validation.
scope for this minimal, time-constrained patch.
The goal here is a small, low-risk hardening step that improves exposure scores without touching runtime code
or risking the performance regressions seen with heavier sandboxing.
ProtectHostnameandPrivateTmpremaindocumented candidates for a follow-up once there is time to validate behavior and adjust tests as needed.