From f5dc68a766936ca616513c0f8cd94fff4ee09c16 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Wed, 4 Sep 2024 12:11:57 +0200 Subject: [PATCH 1/3] prometheus-pgbouncer-exporter: 0.8.0 -> 0.9.0 ChangeLog: https://github.com/prometheus-community/pgbouncer_exporter/blob/v0.9.0/CHANGELOG.md#090--2024-08-01 (cherry picked from commit 4d262e749fd045962e76efca4a3b5fc8f3a3a52e) --- pkgs/servers/monitoring/prometheus/pgbouncer-exporter.nix | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkgs/servers/monitoring/prometheus/pgbouncer-exporter.nix b/pkgs/servers/monitoring/prometheus/pgbouncer-exporter.nix index 970d63ec0f2e..90ffdb48f0d1 100644 --- a/pkgs/servers/monitoring/prometheus/pgbouncer-exporter.nix +++ b/pkgs/servers/monitoring/prometheus/pgbouncer-exporter.nix @@ -2,16 +2,16 @@ buildGoModule rec { pname = "pgbouncer-exporter"; - version = "0.8.0"; + version = "0.9.0"; src = fetchFromGitHub { owner = "prometheus-community"; repo = "pgbouncer_exporter"; rev = "v${version}"; - hash = "sha256-QnA9H4qedCPZKqJQ1I2OJO42mCWcWqYxLmeF3+JXzTw="; + hash = "sha256-fKoyRHYLwVefsZ014eazVCD5B9eV8/CUkuHE4mbUqVo="; }; - vendorHash = "sha256-NYiVW+CNrxFrEUl1nsTeNNgy7SmTYgqs1d50rCvyBcw="; + vendorHash = "sha256-IxmxfF9WsF0Hbym4G0UecyW8hAvucoaCFUE1kXUljJs="; meta = with lib; { description = "Prometheus exporter for PgBouncer"; From 1a6587231b064ad33ed33db113035e37b32c6dcc Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Thu, 5 Sep 2024 12:05:54 +0200 Subject: [PATCH 2/3] nixos/prometheus-exporters: fix assertions declared in exporter modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit And it turns out, the test was using a removed option all along 🙃 (cherry picked from commit 4980a7d93826dc96621c6306b1e4b2cdd076f2ab) --- nixos/modules/services/monitoring/prometheus/exporters.nix | 1 + nixos/tests/prometheus-exporters.nix | 1 - 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/nixos/modules/services/monitoring/prometheus/exporters.nix b/nixos/modules/services/monitoring/prometheus/exporters.nix index 2dc12a221bf0..50200da82e04 100644 --- a/nixos/modules/services/monitoring/prometheus/exporters.nix +++ b/nixos/modules/services/monitoring/prometheus/exporters.nix @@ -230,6 +230,7 @@ let in mkIf conf.enable { warnings = conf.warnings or []; + assertions = conf.assertions or []; users.users."${name}-exporter" = (mkIf (conf.user == "${name}-exporter" && !enableDynamicUser) { description = "Prometheus ${name} exporter service user"; isSystemUser = true; diff --git a/nixos/tests/prometheus-exporters.nix b/nixos/tests/prometheus-exporters.nix index 56569c4de2c8..a0f895d4cb2d 100644 --- a/nixos/tests/prometheus-exporters.nix +++ b/nixos/tests/prometheus-exporters.nix @@ -435,7 +435,6 @@ let json = { exporterConfig = { enable = true; - url = "http://localhost"; configFile = pkgs.writeText "json-exporter-conf.json" (builtins.toJSON { modules = { default = { From f3762903d6ccdafac250ce9a1eb3ff351efa8005 Mon Sep 17 00:00:00 2001 From: Maximilian Bosch Date: Thu, 5 Sep 2024 12:09:05 +0200 Subject: [PATCH 3/3] nixos/prometheus-exporters/pgbouncer: don't leak DB password into cmdline Since `connectionStringFile` reads the file and puts it into the invocation of the exporter, it's part of the cmdline and thus effectively world-readable. Added a new `connectionEnvFile` which is supposed to be an environment file of the form PGBOUNCER_EXPORTER_CONNECTION_STRING=... that will be added to the systemd service. The exporter will read the connection string from that value. (cherry picked from commit 862ecd674f4beced1fff1d5b91527b564eb41182) --- .../monitoring/prometheus/exporters.nix | 24 ++++----- .../prometheus/exporters/pgbouncer.nix | 52 +++++++++++++------ nixos/tests/prometheus-exporters.nix | 4 +- 3 files changed, 49 insertions(+), 31 deletions(-) diff --git a/nixos/modules/services/monitoring/prometheus/exporters.nix b/nixos/modules/services/monitoring/prometheus/exporters.nix index 50200da82e04..b54bdcded180 100644 --- a/nixos/modules/services/monitoring/prometheus/exporters.nix +++ b/nixos/modules/services/monitoring/prometheus/exporters.nix @@ -3,7 +3,7 @@ let inherit (lib) concatStrings foldl foldl' genAttrs literalExpression maintainers mapAttrs mapAttrsToList mkDefault mkEnableOption mkIf mkMerge mkOption - optional types mkOptionDefault flip attrNames; + optional types mkOptionDefault flip attrNames xor; cfg = config.services.prometheus.exporters; @@ -364,13 +364,6 @@ in Please specify either 'services.prometheus.exporters.nextcloud.passwordFile' or 'services.prometheus.exporters.nextcloud.tokenFile' ''; - } { - assertion = cfg.pgbouncer.enable -> ( - (cfg.pgbouncer.connectionStringFile != null || cfg.pgbouncer.connectionString != "") - ); - message = '' - PgBouncer exporter needs either connectionStringFile or connectionString configured" - ''; } { assertion = cfg.pgbouncer.enable -> ( config.services.pgbouncer.ignoreStartupParameters != null && builtins.match ".*extra_float_digits.*" config.services.pgbouncer.ignoreStartupParameters != null @@ -414,7 +407,15 @@ in Please ensure you have either `services.prometheus.exporters.idrac.configuration' or `services.prometheus.exporters.idrac.configurationPath' set! ''; - } ] ++ (flip map (attrNames exporterOpts) (exporter: { + } { + assertion = cfg.pgbouncer.enable -> ( + xor (cfg.pgbouncer.connectionEnvFile == null) (cfg.pgbouncer.connectionString == null) + ); + message = '' + Options `services.prometheus.exporters.pgbouncer.connectionEnvFile` and + `services.prometheus.exporters.pgbouncer.connectionString` are mutually exclusive! + ''; + }] ++ (flip map (attrNames exporterOpts) (exporter: { assertion = cfg.${exporter}.firewallFilter != null -> cfg.${exporter}.openFirewall; message = '' The `firewallFilter'-option of exporter ${exporter} doesn't have any effect unless @@ -428,11 +429,6 @@ in Consider using `services.prometheus.exporters.idrac.configuration` instead. '' ) - (mkIf - (cfg.pgbouncer.enable && cfg.pgbouncer.connectionString != "") '' - config.services.prometheus.exporters.pgbouncer.connectionString is insecure. Use connectionStringFile instead. - '' - ) (mkIf (cfg.pgbouncer.enable && config.services.pgbouncer.authType != "any") '' Admin user (with password or passwordless) MUST exist in the services.pgbouncer.authFile if authType other than any is used. diff --git a/nixos/modules/services/monitoring/prometheus/exporters/pgbouncer.nix b/nixos/modules/services/monitoring/prometheus/exporters/pgbouncer.nix index 71b602638632..471849cad770 100644 --- a/nixos/modules/services/monitoring/prometheus/exporters/pgbouncer.nix +++ b/nixos/modules/services/monitoring/prometheus/exporters/pgbouncer.nix @@ -6,6 +6,7 @@ let mkOption types optionals + getExe escapeShellArg concatStringsSep ; @@ -23,8 +24,8 @@ in }; connectionString = mkOption { - type = types.str; - default = ""; + type = types.nullOr types.str; + default = null; example = "postgres://admin:@localhost:6432/pgbouncer?sslmode=require"; description = '' Connection string for accessing pgBouncer. @@ -35,24 +36,28 @@ in in the services.pgbouncer.authFile if authType other than any is used. WARNING: this secret is stored in the world-readable Nix store! - Use {option}`connectionStringFile` instead. + Use [](#opt-services.prometheus.exporters.pgbouncer.connectionEnvFile) if the + URL contains a secret. ''; }; - connectionStringFile = mkOption { - type = types.nullOr types.path; + connectionEnvFile = mkOption { + type = types.nullOr types.str; default = null; - example = "/run/keys/pgBouncer-connection-string"; description = '' - File that contains pgBouncer connection string in format: - postgres://admin:@localhost:6432/pgbouncer?sslmode=require + File that must contain the environment variable + `PGBOUNCER_EXPORTER_CONNECTION_STRING` which is set to the connection + string used by pgbouncer. I.e. the format is supposed to look like this: - NOTE: You MUST keep pgbouncer as database name (special internal db)!!! + ``` + PGBOUNCER_EXPORTER_CONNECTION_STRING="postgres://admin@localhost:6432/pgbouncer?sslmode=require" + ``` - NOTE: Admin user (with password or passwordless) MUST exist - in the services.pgbouncer.authFile if authType other than any is used. + NOTE: You MUST keep pgbouncer as database name (special internal db)! + NOTE: `services.pgbouncer.settings.pgbouncer.ignore_startup_parameters` + MUST contain "extra_float_digits". - {option}`connectionStringFile` takes precedence over {option}`connectionString` + Mutually exclusive with [](#opt-services.prometheus.exporters.pgbouncer.connectionString). ''; }; @@ -120,10 +125,9 @@ in startScript = pkgs.writeShellScriptBin "pgbouncer-start" "${concatStringsSep " " ([ "${pkgs.prometheus-pgbouncer-exporter}/bin/pgbouncer_exporter" "--web.listen-address ${cfg.listenAddress}:${toString cfg.port}" - "--pgBouncer.connectionString ${if cfg.connectionStringFile != null then - "$(head -n1 ${cfg.connectionStringFile})" else "${escapeShellArg cfg.connectionString}"}" - ] - ++ optionals (cfg.telemetryPath != null) [ + ] ++ optionals (cfg.connectionString != null) [ + "--pgBouncer.connectionString ${escapeShellArg cfg.connectionString}" + ] ++ optionals (cfg.telemetryPath != null) [ "--web.telemetry-path ${escapeShellArg cfg.telemetryPath}" ] ++ optionals (cfg.pidFile != null) [ @@ -145,6 +149,22 @@ in in { ExecStart = "${startScript}/bin/pgbouncer-start"; + EnvironmentFile = lib.mkIf (cfg.connectionEnvFile != null) [ + cfg.connectionEnvFile + ]; }; }; + + imports = [ + (lib.mkRemovedOptionModule [ "connectionStringFile" ] '' + As replacement, the option `services.prometheus.exporters.pgbouncer.connectionEnvFile` + has been added. In contrast to `connectionStringFile` it must be an environment file + with the connection string being set to `PGBOUNCER_EXPORTER_CONNECTION_STRING`. + + The change was necessary since the former option wrote the contents of the file + into the cmdline of the exporter making the connection string effectively + world-readable. + '') + ({ options.warnings = options.warnings; options.assertions = options.assertions; }) + ]; } diff --git a/nixos/tests/prometheus-exporters.nix b/nixos/tests/prometheus-exporters.nix index a0f895d4cb2d..af6978bd8809 100644 --- a/nixos/tests/prometheus-exporters.nix +++ b/nixos/tests/prometheus-exporters.nix @@ -947,7 +947,9 @@ let pgbouncer = { exporterConfig = { enable = true; - connectionStringFile = pkgs.writeText "connection.conf" "postgres://admin:@localhost:6432/pgbouncer?sslmode=disable"; + connectionEnvFile = "${pkgs.writeText "connstr-env" '' + PGBOUNCER_EXPORTER_CONNECTION_STRING=postgres://admin@localhost:6432/pgbouncer?sslmode=disable + ''}"; }; metricProvider = {