Allow numeric [UG]ID in FPM listen.{owner,group} #5148

andrenth · 2020-02-04T13:19:49Z

This patch allows FPM pool configuration settings listen.owner and listen.group to accept numeric IDs as well as user and group names. This makes them mirror the behavior of the user and group settings.

This feature was requested on bug #77062 in the bug tracker

andrenth · 2020-02-19T18:25:59Z

So, there were CI failures but they seem unrelated to the PR.

This patch is useful for users with remote user databases (e.g. LDAP), where there's an inherent race between user creation and FPM pool creation. When the pool is created before the user has been created, FPM dies on reload due to the nonexistent user, taking down all pools. Accepting numeric IDs would prevent this from happening.

nikic · 2020-02-21T13:39:20Z

Looks fine to me, but let's let @bukka have a look as well.

bukka

I think that it makes sense in general.

It might be also possible to add some basic tests. It will be probably reliable only for the current user and some non existent user for errors. That should be enough to verify that uid and gid is accepted.

bukka · 2020-02-23T18:49:17Z

sapi/fpm/fpm/fpm_unix.c

@@ -163,27 +163,35 @@ int fpm_unix_resolve_socket_premissions(struct fpm_worker_pool_s *wp) /* {{{ */
 #endif

 	if (c->listen_owner && *c->listen_owner) {
-		struct passwd *pwd;
+		if (strlen(c->listen_owner) == strspn(c->listen_owner, "0123456789")) {
+			wp->socket_uid = strtoul(c->listen_owner, 0, 10);


Please verify that the uid exists using getpwuid and fail if it doesn't.

The motivation for this patch is to allow the code to work even when UID and GID don't correspond to a valid used (yet). The use case is for situations where creating the user might take longer than creating the pool when using remote authentication, like when using LDAP, and using the numeric IDs would allow FPM to successfully be reloaded when the pool is created, even if the user does not yet exist.

The patch mirrors the code for the user and group settings, which also do not fail if the IDs are not found.

Ok that makes sense.

bukka · 2020-02-23T18:49:58Z

sapi/fpm/fpm/fpm_unix.c

 	}

 	if (c->listen_group && *c->listen_group) {
-		struct group *grp;
+		if (strlen(c->listen_group) == strspn(c->listen_group, "0123456789")) {
+			wp->socket_gid = strtoul(c->listen_group, 0, 10);


Please verify that the gid exists using getgrgid and fail if it doesn't.

Please see my comment w.r.t listen_owner above.

bukka · 2020-03-01T18:13:34Z

So the only missing bit is a test. Otherwise it looks good.

andrenth · 2020-03-02T14:10:48Z

I've added a simple test file.

andrenth · 2020-03-02T21:54:02Z

I believe the tests failed because they're not run as root. Is there a way to skip a test case unless it's running as root?

Edit: I've added a commit for that, thought I'm not sure it's the best way.

bukka · 2020-03-08T19:00:36Z

sapi/fpm/tests/socket-uds-numeric-user-and-group-id.phpt

+EOT;
+
+$tester = new FPM\Tester($cfg);
+$tester->start();


I just tested this to run under root and it actually fails to start for me. Might be my local issue but was wondering if the test actually works for you when you run it under root?

Btw for me this line fails if run under root even with setting TEST_FPM_RUN_AS_ROOT=1:

$this->masterProcess = proc_open($cmd, $desc, $pipes);

Interestingly I tested starting that with passthru and it actually starts FPM so something weird with proc_open. Might need to debug it.

It does work for me:

$ sudo -i # cd /home/andre/git/forks/php-src # TEST_FPM_RUN_AS_ROOT=1 ./sapi/cli/php run-tests.php sapi/fpm/tests/socket-uds-numeric-user-and-group-id.phpt ===================================================================== PHP : /home/andre/git/forks/php-src/sapi/cli/php PHP_SAPI : cli PHP_VERSION : 8.0.0-dev ZEND_VERSION: 4.0.0-dev PHP_OS : Linux - Linux andre 4.15.0-88-generic #88-Ubuntu SMP Tue Feb 11 20:11:34 UTC 2020 x86_64 INI actual : /home/andre/git/forks/php-src More .INIs : --------------------------------------------------------------------- PHP : /home/andre/git/forks/php-src/sapi/phpdbg/phpdbg PHP_SAPI : phpdbg PHP_VERSION : 8.0.0-dev ZEND_VERSION: 4.0.0-dev PHP_OS : Linux - Linux andre 4.15.0-88-generic #88-Ubuntu SMP Tue Feb 11 20:11:34 UTC 2020 x86_64 INI actual : /home/andre/git/forks/php-src More .INIs : --------------------------------------------------------------------- CWD : /home/andre/git/forks/php-src Extra dirs : VALGRIND : Not used ===================================================================== Running selected tests. PASS FPM: UNIX socket owner and group settings can be numeric [sapi/fpm/tests/socket-uds-numeric-user-and-group-id.phpt] ===================================================================== Number of tests : 1 1 Tests skipped : 0 ( 0.0%) -------- Tests warned : 0 ( 0.0%) ( 0.0%) Tests failed : 0 ( 0.0%) ( 0.0%) Tests passed : 1 (100.0%) (100.0%) --------------------------------------------------------------------- Time taken : 0 seconds =====================================================================

The test calls the stat command to check the owner and group of the created sockets. Could it be a PATH issue, or maybe your environment doesn't have this command?

Probably something local then. Don't worry about it then!

bukka · 2020-03-08T19:04:47Z

sapi/fpm/tests/tester.inc

+     */
+    static public function skipIfNotRoot()
+    {
+        if (getmyuid() != 0) {


This doesn't seem correct as it's just for the actual script owner but we should care about the EUID so it should be more something like this:

if ( (extension_loaded('posix_geteuid') && posix_geteuid() !== 0) || (exec('id -u', $output, $rv) !== "0") ) { die('skip not running as root'); }

Oh, I didn't know that's what getmyuid did. However the test still works as intended because when the test is run as root, the PHP script generated from the phpt file will be owned by root, and so getmyuid will return 0.

I can add a commit to replace the test as you suggested, but in skipif.inc there's a similar check that also uses getmyuid:

// Running as root is not allowed without TEST_FPM_RUN_AS_ROOT env if (!getmyuid() && !getenv('TEST_FPM_RUN_AS_ROOT')) { die('skip Refusing to run as root'); }

Ah yeah it should be enough then. I sometimes test things just by running php on the phpt file (without the framework) so that's why it didn't work for me but that's not requirement for the test to work so feel free to leave it as it is.

bukka · 2020-03-08T19:18:19Z

That root test looks like a good idea and it would be also good to have root tests for other things so well done on starting that! The only thing about those tests is that we won't be currently able to run them in CI. I think it would be idea to also have a test that we can run in CI. It could be basically the same as the current one, just it would use posix_geteuid return value in the config. It will at least test that numbers are accepted and should hopefully work. It will probably also require skip if posix extension is not loaded.

andrenth · 2020-03-08T21:33:32Z

I've added a test that works as you suggested. I couldn't find a way to use PHP code or template variables in the EXPECT section of the test, so I went with EXPECTF.

andrenth · 2020-03-09T12:46:57Z

So, the test does work locally, but it's failing in the CI environment:

ERROR: [pool unconfined] failed to chown() the socket '/home/vsts/work/1/s/sapi/fpm/tests/socket-uds-numeric-user-and-group-id-nonroot.9004.sock': Operation not permitted (1)

I don't know enough about the CI environment to understand why this happens. Do the tests run as a different user than the one who owns the test files?

bukka · 2020-03-11T20:34:54Z

it might be that Azure pipeline might be using seccomp or something maybe. Might make sense to skip just in there maybe as it works in Travis.

@nikic is it possible to skip Azure pipelines easily?

sapi/fpm/tests/tester.inc

nikic · 2020-03-11T21:14:27Z

Looks like that did it. There's still a failure on macos though

fsockopen(): socket path exceeded the maximum allowed length of 104 bytes and was truncated in /Users/runner/runners/2.165.0/work/1/s/sapi/fpm/tests/fcgi.inc on line 280

andrenth · 2020-03-11T21:24:06Z

I think renaming the test files to something shorter will fix that.

bukka · 2020-03-15T19:17:05Z

@andrenth It all looks good, just one small thing. Please can you update www.conf.in to somehow state that uid and gid are supported. Currently the description in there (

php-src/sapi/fpm/www.conf.in

Lines 42 to 44 in 4c0ddd0

    
           ; Set permissions for unix socket, if one is used. In Linux, read/write 
        
           ; permissions must be set in order to allow connections from a web server. Many 
        
           ; BSD-derived systems allow connections regardless of permissions.

) is a bit vague so it might be worth to extend it a little a bit or maybe just adding a note about what is accepted.

andrenth · 2020-03-15T19:42:49Z

I've added a short note that those settings accept either names or numeric IDs. If you'd like me to extend the explanation a bit more, let me know.

bukka · 2020-03-15T19:49:40Z

Yeah I think it's good enough. Can you please squash all your commits and push force it?

andrenth · 2020-03-15T20:02:19Z

Done.

bukka · 2020-03-29T19:22:25Z

Merged to 7.4 as 0b4e80b

andrenth · 2020-03-29T19:41:31Z

Thank you!

hakuno · 2020-04-16T20:59:05Z

Nice patch!

nikic added the Feature label Feb 21, 2020

bukka requested changes Feb 23, 2020

View reviewed changes

bukka requested changes Mar 8, 2020

View reviewed changes

nikic reviewed Mar 11, 2020

View reviewed changes

sapi/fpm/tests/tester.inc Outdated Show resolved Hide resolved

Allow numeric [UG]ID in FPM listen.{owner,group}

ac56ce1

andrenth force-pushed the numeric-fpm-socket-owner-and-group branch from 64d0e8a to ac56ce1 Compare March 15, 2020 20:01

bukka closed this Mar 29, 2020

Allow numeric [UG]ID in FPM listen.{owner,group} #5148

Allow numeric [UG]ID in FPM listen.{owner,group} #5148

Uh oh!

Conversation

andrenth commented Feb 4, 2020

Uh oh!

andrenth commented Feb 19, 2020

Uh oh!

nikic commented Feb 21, 2020

Uh oh!

bukka left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Mar 1, 2020

Uh oh!

andrenth commented Mar 2, 2020

Uh oh!

andrenth commented Mar 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bukka commented Mar 8, 2020

Uh oh!

andrenth commented Mar 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrenth commented Mar 9, 2020

Uh oh!

bukka commented Mar 11, 2020

Uh oh!

Uh oh!

nikic commented Mar 11, 2020

Uh oh!

andrenth commented Mar 11, 2020

Uh oh!

bukka commented Mar 15, 2020

Uh oh!

andrenth commented Mar 15, 2020

Uh oh!

bukka commented Mar 15, 2020

Uh oh!

andrenth commented Mar 15, 2020

Uh oh!

bukka commented Mar 29, 2020

Uh oh!

andrenth commented Mar 29, 2020

Uh oh!

hakuno commented Apr 16, 2020

Uh oh!

Uh oh!

andrenth commented Mar 2, 2020 •

edited

Loading

andrenth commented Mar 8, 2020 •

edited

Loading