`services.phpfpm.pools.<name>.phpEnv` values are not quoted

[davidtwco]

Describe the bug services.phpfpm.pools.<name>.phpEnv values do not require quotation to be parsed correctly by php-fpm, but if the value contains a = or is true/false (and perhaps other values, then the file fails to parse).

To Reproduce Steps to reproduce the behavior:

1. Use the following NixOS configuration:

services.phpfpm.pools.foo.phpEnv."bar" = "true";

2. Observe that phpfpm-bar.service fails to start.
3. Observe that manually quoting "true" fixes the issue.

Expected behavior phpfpm module appropriately quotes values provided to phpEnv.

Metadata Please run nix-shell -p nix-info --run "nix-info -m" and paste the result.

• system: "x86_64-linux"
• host os: Linux 4.19.88, NixOS, 19.09.1549.45ea6092203 (Loris)
• multi-user?: yes
• sandbox: yes
• version: nix-env (Nix) 2.3
• channels(root): "nixos-19.09.1549.45ea6092203"
• channels(david): ""
• nixpkgs: /nix/var/nix/profiles/per-user/root/channels/nixos

Maintainer information:

# a list of nixpkgs attributes affected by the problem
attribute:
# a list of nixos modules affected by the problem
module: services.phpfpm

[aanderse]

@davidtwco thanks for the report.

I couldn't find any documentation upstream on this behavior. Do you have a link to any? If not, do you have a list of rules you know that should be applied to values in phpEnv?

[davidtwco]

@aanderse I don’t, I’ve not used php-fpm much and only discovered this through experimentation.

Perhaps usage of —test (manpage) could be added to validate the configurations, like the nginx module does?

[davidtwco]

In fact, it appears as if the default php.ini file starts with a comment that explains the format somewhat.

[aanderse]

I think the solution here is to write: phpEnv.myval = "\"true\"";. I'm not inclined to add a bunch of if statements in the logic, but instead add some examples and maybe a link upstream explaining.

What do you think about that? It seems to cover most use cases keeping the module code very simple, has flexibility, and just requires better documentation from us to cover edge cases.

[davidtwco]

I think it would be more intuitive if phpEnv.myval accepted strings or booleans. phpEnv.myval = true; would not be quoted in the generated configuration, but all string values would be.

[aanderse]

Is there ever a scenario where someone would need an unquoted string?

[davidtwco]

I suspect that you’re right and that it wouldn’t be feasible to quote all strings, my reading of the format suggests that numbers, PHP constants, INI constants, expressions and references should be unquoted. It’s unclear which of these are valid to provide to env[...] = directives (numbers certainly could be handled like I suggest booleans are) so it probably isn’t feasible to quote every string - updating the documentation is probably the best we can do.

[aanderse]

@davidtwco were you interested in making a PR for this? Did you have some thoughts on improved wording?

[davidtwco]

I think it would be more intuitive if the option took attrsOf (oneOf [ str int bool ]) and any str type values were quoted in the configuration file - I think that this would avoid cases where the generated configuration file is invalid and the service fails to start as a result.

Users would still be able to use services.phpfpm.pools.<name>.extraConfig for the cases where they want an unquoted value, such as a PHP constant, INI constant, expression or reference - ultimately however, if this case isn't as rare (for phpEnv specifically) as I suspect, then the current behaviour might be better.

Either way, adding support for config validation using phpfpm --test would be useful. Unfortunately, I don't have time to prepare a PR for this at the moment.

[jtojnar]

Environment variables can only be strings so we could just quote everything and let people use extraConfig if they need expressions as you suggest.

The problem is, it is not clear how to escape things correctly. In my experiments, the only working solution is disabling clear_env and setting the environment variables for the systemd service:

import ../make-test-python.nix ({pkgs, lib, php, ...}: {
  name = "php-${php.version}-fpm-nginx-test";
  meta.maintainers = lib.teams.php.members;

  nodes.machine = { config, lib, pkgs, ... }: {
    environment.systemPackages = [ php ];

    services.nginx = {
      enable = true;

      virtualHosts."phpfpm" =
        let
          testdir = pkgs.writeTextDir "web/index.php" "<?php for($i = 1; $i <= 6; $i++) { echo 'test' . $i . '='. $_ENV['test' . $i] . PHP_EOL; }";
        in
        {
          root = "${testdir}/web";
          locations."~ \\.php$".extraConfig = ''
            fastcgi_pass unix:${config.services.phpfpm.pools.foobar.socket};
            fastcgi_index index.php;
            include ${config.services.nginx.package}/conf/fastcgi_params;
            include ${pkgs.nginx}/conf/fastcgi.conf;
          '';
          locations."/" = {
            tryFiles = "$uri $uri/ index.php";
            index = "index.php index.html index.htm";
          };
        };
    };

    services.phpfpm.pools."foobar" = {
      user = "nginx";
      phpPackage = php;
      settings = {
        "listen.group" = "nginx";
        "listen.mode" = "0600";
        "listen.owner" = "nginx";
        "pm" = "dynamic";
        "pm.max_children" = 5;
        "pm.max_requests" = 500;
        "pm.max_spare_servers" = 3;
        "pm.min_spare_servers" = 1;
        "pm.start_servers" = 2;
      };
      phpEnv = {
        test1 = "$2y$10$test1";
        test2 = ''"${lib.escape [ "\\" "\"" ] "$2y$10$test2"}"'';
        test3 = ''"${lib.escape [ "\\" "\"" "$" ] "$2y$10$test3"}"'';
        test4 = "'${lib.escape [ "\\" "\'" ] "$2y$10$test4"}'";
        test5 = "'${lib.escape [ "\\" "\'" "$" ] "$2y$10$test5"}'";
      };
      phpOptions = ''
        ; Set up $_ENV superglobal.
        ; http://php.net/request-order
        variables_order = "EGPCS"
      '';
      settings = {
        clear_env = false;
      };
    };

    systemd.services.phpfpm-foobar.environment = {
      test6 = "$2y$10$test6";
    };
  };

  testScript = { ... }: ''
    machine.wait_for_unit("nginx.service")
    machine.wait_for_unit("phpfpm-foobar.service")

    # Check so we get an evaluated PHP back
    response = machine.succeed("curl -s http://127.0.0.1:80/")
    print(response)
    assert "$2y$10$test1" in response, "test1 not detected"
    assert "$2y$10$test2" in response, "test2 not detected"
    assert "$2y$10$test3" in response, "test3 not detected"
    assert "$2y$10$test4" in response, "test4 not detected"
    assert "$2y$10$test5" in response, "test5 not detected"
    assert "$2y$10$test6" in response, "test6 not detected"
  '';
})

will print

test1=
test2=
test3=
test4=
test5=\$2y\$10\$test5
test6=$2y$10$test6

[aanderse]

@jtojnar I was fairly motivated to remove extraConfig...

[jtojnar]

@aanderse settings should work too, if we add a rawExpression = expr: { _type = "rawExpr"; value = expr; } function whose value would the generator print literally.

[stale[bot]]

I marked this as stale due to inactivity. → More info

[delroth]

Un-staling since I hit this problem again today.

true/false values are a problem as mentioned in the initial comment from this issue, but also other values such as "^".

Jun 21 22:44:11 chaos php-fpm[1877493]: PHP:  syntax error, unexpected '^' in Unknown on line 1