Merge: prometheus-pgbouncer-exporter: 0.8.0 -> 0.9.0, fix secrets handling (#340852)

nixos/doc/manual/release-notes/rl-2411.section.md

@@ -396,6 +396,12 @@
 
 - The `services.trust-dns` module has been renamed to `services.hickory-dns`.
 
+- The option `services.prometheus.exporters.pgbouncer.connectionStringFile` has been removed since
+  it leaked the connection string (and thus potentially the DB password) into the cmdline
+  of process making it effectively world-readable.
+
+  Use [`services.prometheus.exporters.pgbouncer.connectionEnvFile`](#opt-services.prometheus.exporters.pgbouncer.connectionEnvFile) instead.
+
 - The `lsh` package and the `services.lshd` module have been removed as they had no maintainer in Nixpkgs and hadn’t seen an upstream release in over a decade. It is recommended to migrate to `openssh` and `services.openssh`.
 
 - `opencv2` and `opencv3` have been removed, as they are obsolete and

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;
 
@@ -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;
@@ -359,13 +360,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.sql.enable -> (
         (cfg.sql.configFile == null) != (cfg.sql.configuration == null)
@@ -405,7 +399,15 @@ in
         Please ensure you have either `services.prometheus.exporters.deluge.delugePassword'
           or `services.prometheus.exporters.deluge.delugePasswordFile' 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
@@ -419,11 +421,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.
-        ''
-      )
     ] ++ config.services.prometheus.exporters.warnings;
   }]  ++ [(mkIf config.services.prometheus.exporters.rtl_433.enable {
     hardware.rtl-sdr.enable = mkDefault true;

nixos/modules/services/monitoring/prometheus/exporters/pgbouncer.nix

@@ -7,11 +7,8 @@ let
     mkPackageOption
     types
     optionals
-    optionalString
     getExe
-    getExe'
     escapeShellArg
-    escapeShellArgs
     concatStringsSep
     ;
 in
@@ -29,8 +26,8 @@ in
     };
 
     connectionString = mkOption {
-      type = types.str;
+      type = types.nullOr types.str;
-      default = "";
+      default = null;
       example = "postgres://admin:@localhost:6432/pgbouncer?sslmode=require";
       description = ''
         Connection string for accessing pgBouncer.
@@ -43,26 +40,28 @@ in
         auth_file if auth_type 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 {
+    connectionEnvFile = mkOption {
-      type = types.nullOr types.path;
+      type = types.nullOr types.str;
       default = null;
-      example = "/run/keys/pgBouncer-connection-string";
       description = ''
-        File that contains pgBouncer connection string in format:
+        File that must contain the environment variable
-        postgres://admin:@localhost:6432/pgbouncer?sslmode=require
+        `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: ignore_startup_parameters MUST contain "extra_float_digits".
+        NOTE: You MUST keep pgbouncer as database name (special internal db)!
+        NOTE: `services.pgbouncer.settings.pgbouncer.ignore_startup_parameters`
+        MUST contain "extra_float_digits".
 
-        NOTE: Admin user (with password or passwordless) MUST exist in the
+        Mutually exclusive with [](#opt-services.prometheus.exporters.pgbouncer.connectionString).
-        auth_file if auth_type other than "any" is used.
-
-        {option}`connectionStringFile` takes precedence over {option}`connectionString`
       '';
     };
 
@@ -126,16 +125,11 @@ in
 
   serviceOpts = {
     after = [ "pgbouncer.service" ];
-    script = optionalString (cfg.connectionStringFile != null) ''
+    script = concatStringsSep " " ([
-      connectionString=$(${escapeShellArgs [
-        (getExe' pkgs.coreutils "cat") "--" cfg.connectionStringFile
-      ]})
-    '' + concatStringsSep " " ([
       "exec -- ${escapeShellArg (getExe cfg.package)}"
       "--web.listen-address ${cfg.listenAddress}:${toString cfg.port}"
-      "--pgBouncer.connectionString ${if cfg.connectionStringFile != null
+    ] ++ optionals (cfg.connectionString != null) [
-          then "\"$connectionString\""
+      "--pgBouncer.connectionString ${escapeShellArg cfg.connectionString}"
-          else "${escapeShellArg cfg.connectionString}"}"
     ] ++ optionals (cfg.telemetryPath != null) [
       "--web.telemetry-path ${escapeShellArg cfg.telemetryPath}"
     ] ++ optionals (cfg.pidFile != null) [
@@ -151,5 +145,21 @@ in
     ] ++ cfg.extraFlags);
 
     serviceConfig.RestrictAddressFamilies = [ "AF_INET" "AF_INET6" "AF_UNIX" ];
+    serviceConfig.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

@@ -482,7 +482,6 @@ let
     json = {
       exporterConfig = {
         enable = true;
-        url = "http://localhost";
         configFile = pkgs.writeText "json-exporter-conf.json" (builtins.toJSON {
           modules = {
             default = {
@@ -932,7 +931,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 = {

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";