netdata service: fix permissions for apps.plugin #33331
Conversation
nixos/tests/netdata.nix
Outdated
|
|
||
| $netdata->waitForUnit("netdata.service"); | ||
| #check if netdata can read disk ops for root owned processes. if > 0, successful. verifies both netdata working and apps.plugin has elevated capabilities. | ||
| $netdata->waitUntilSucceeds("curl -s http://localhost:19999/api/v1/data\?chart=users.pwrites | jq -e '[.data[range(10)][.labels | indices(\"root\")[0]]] | add | . > 0'") ; |
There was a problem hiding this comment.
Please split this string across multiple lines for readability:
my $cmd = <<'CMD';
curl -s http://localhost:19999/api/v1/data\?chart=users.pwrites | \
jq -e '[.data[range(10)][.labels | indices("root")[0]]] | add | . > 0'
CMD
$netdata->waitUntilSucceeds($cmd);
| configFile = pkgs.writeText "netdata.conf" cfg.configText; | ||
| localConfig = { | ||
| global = { | ||
| "plugins directory" = "/run/wrappers/bin ${pkgs.netdata}/libexec/netdata/plugins.d"; |
There was a problem hiding this comment.
Could including wrappers directory lead to executable being misclassified as netdata plugins and executed?
There was a problem hiding this comment.
Plugins for netdata must end with .plugin so the likely hood of conflicting with other apps in wrappers would be quite low.
If that's not enough guarantee, the configuration can be called out specifically to only enable the bundled plugins but version bumps for new features in netdata would require config updates from either the user or maintainer here which would cause some churn.
I think that netdata requiring a .plugin extension is enough to keep accidents from happening.
There was a problem hiding this comment.
what do you think about the following solution?
╰─ % git diff
diff --git a/nixos/modules/services/monitoring/netdata.nix b/nixos/modules/services/monitoring/netdata.nix
index 5c78f5203e2..43edc7c5e08 100644
--- a/nixos/modules/services/monitoring/netdata.nix
+++ b/nixos/modules/services/monitoring/netdata.nix
@@ -4,9 +4,15 @@ with lib;
let
cfg = config.services.netdata;
+
+ wrappedPlugins = pkgs.runCommand "wrapped-plugins" {} ''
+ mkdir -p $out/libexec/netdata/plugins.d
+ ln -s /run/wrappers/bin/apps.plugin $out/libexec/netdata/plugins.d/apps.plugin
+ '';
+
localConfig = {
global = {
- "plugins directory" = "/run/wrappers/bin ${pkgs.netdata}/libexec/netdata/plugins.d";
+ "plugins directory" = "${wrappedPlugins}/libexec/netdata/plugins.d ${pkgs.netdata}/libexec/netdata/plugins.d";
};
};
mkConfig = generators.toINI {} (recursiveUpdate localConfig cfg.config);There was a problem hiding this comment.
I'm ok with that. Are there no concerns about sym linking out of the store? I've been trying to avoid it even though the wrapper set up a few lines down will guarantee it's there.
cransom
force-pushed
the
netdata-module
branch
from
3c3f7e1
to
c62fc11
Compare
apps.plugin requires capabilities for full process monitoring. with 1.9.0, netdata allows multiple directories to search for plugins and the setuid directory can be specified here. the module is backwards compatible with older configs. a test is included that verifies data gathering for the elevated privileges. one additional attribute is added to make configuration more generic than including configuration in string form.
cransom
force-pushed
the
netdata-module
branch
from
c62fc11
to
f3cba4f
Compare
|
Is there anything else to address in this PR? |
|
@GrahamcOfBorg test netdata |
There was a problem hiding this comment.
Success for system: x86_64-linux
netdata: exit status 1
syncing
netdata: running command: sync
netdata: exit status 0
test script finished in 7.00s
cleaning up
killing netdata (pid 593)
vde_switch: EOF on stdin, cleaning up and exiting
vde_switch: Could not remove ctl dir '/tmp/nix-build-vm-test-run-netdata.drv-0/vde1.ctl': Directory not empty
/nix/store/xgr2mqxzrpa8wb113ykgzp0fn1769kyf-vm-test-run-netdata
|
Thanks! |
apps.plugin requires capabilities for full process monitoring. with
1.9.0, netdata allows multiple directories to search for plugins and the
setuid directory can be specified here.
the module is backwards compatible with older configs. a test is
included that verifies data gathering for the elevated privileges. one
additional attribute is added to make configuration more generic than
including configuration in string form.
Motivation for this change
Allow full process/io monitoring via the
appsplugin without running the netdata process as root nor allow access to elevated privilege binary to other users.Things done
build-use-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)