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 862ecd674f)

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.

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; })
+  ];
 }

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 = {