Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use PHP sys_temp_dir by default #10428

Merged
merged 10 commits into from Aug 22, 2019

Conversation

@deajan
Copy link
Contributor

commented Jul 9, 2019

This fixes a usecase when open_basedir is set, not allowing php to write to default temp directory /tmp.
In that case, temp directory is set by php_admin_value[sys_temp_dir] = /var/www/site/tmp
Without setting config['temp_dir'] in generated config.php, LibreNMS will use /tmp regardless of what is set in fpm configuration.
This will set config['temp_dir'] to /tmp if not if no special sys_temp_dir directive is configured in php-fpm pool.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

Fix installer not generating tmp directory directive
This fixes a usecase when open_basedir is set, not allowing php to write to default temp directory /tmp.
In that case, temp directory is set by `php_admin_value[sys_temp_dir] = /var/www/site/tmp`
Without setting config['temp_dir'] in generated config.php, LibreNMS will use /tmp regardless of what is set in fpm configuration.
@CLAassistant

This comment has been minimized.

Copy link

commented Jul 9, 2019

CLA assistant check
All committers have signed the CLA.

@PipoCanaja

This comment has been minimized.

Copy link
Contributor

commented Jul 9, 2019

Bonjour du Léman, @deajan

Thanx for your PR. Tests are still running, but the PR looks good to me.

@murrant I am not enough into the install code now, I leave this to you ;)

@PipoCanaja PipoCanaja requested a review from murrant Jul 9, 2019

@@ -367,6 +391,9 @@
//Please ensure this user is created and has the correct permissions to your install
\$config['user'] = 'librenms';
// This is the temporary directory used by php, set by php_admin_value[sys_temp_dir], defaults to /tmp when not set.

This comment has been minimized.

Copy link
@murrant

murrant Jul 18, 2019

Member

Why are you are adding this to the default config.php? (which I plan to remove entirely at some point)
Setting it here actually doesn't allow php_admin_value[sys_temp_dir] to set it after you have installed LibreNMS

This comment has been minimized.

Copy link
@deajan

deajan Jul 19, 2019

Author Contributor

In my test setup, I've set the following:

php_admin_value[open_basedir] = /var/www/myvhost
php_admin_value[sys_temp_dir] = /var/www/myvhost/tmp

With those settings, I still get php errors saying that /tmp isn't writable, unless I set

$config['temp_dir'] = /var/www/myvhost/tmp or sys_get_temp_dir();

So this is mainly a workaround for LibreNMS not honoring the tmp dir set by php_admin_value[sys_temp_dir].

@deajan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Just tried a fresh installation (without the $config['temp_dir'] patch).
Running CentOS 7 with php-fpm 7.2.

Here's what I get altough the following php parameters are configured:

php_admin_value[sys_temp_dir] = /var/www/librenms.local/tmp
php_admin_value[upload_tmp_dir] = /var/www/librenms.local/tmp
php_admin_value[open_basedir] = /var/www/librenms.local:/dev/urandom:/usr/sbin/fping:/usr/sbin/fping6:/usr/bin/snmpgetnext:/usr/bin/rrdtool

librenms.log output:

[2019-07-23 06:42:18] production.ERROR: is_dir(): open_basedir restriction in effect. File(/tmp) is not within the allowed path(s): (/var/www/librenms.local:/dev/urandom:/usr/sbin/fping:/usr/sbin/fping6:/usr/bin/snmpgetnext:/usr/bin/rrdtool) {"userId":1,"email":"infra@example.comr","exception":"[object] (ErrorException(code: 0): is_dir(): open_basedir restriction in effect. File(/tmp) is not within the allowed path(s): (/var/www/librenms.local:/dev/urandom:/usr/sbin/fping:/usr/sbin/fping6:/usr/bin/snmpgetnext:/usr/bin/rrdtool) at /var/www/librenms.local/ftp/www/app/Checks.php:99)
[...]

I suppose that those temp dir php parameters are overriden somewhere so I searched via grep for "/tmp" in librenms.
The directory actually gets overriden by a $config['temp_dir'] = '/tmp'; statement in includes/defaults.inc.php, so it makes sense for me to set that parameter in the installer if function sys_get_temp_dir(); does return something else.

@murrant

This comment has been minimized.

Copy link
Member

commented Jul 28, 2019

Why not just make the default setting for temp_dir be sys_temp_dir instead of /tmp?

@deajan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Sure, I'll check if this is working, and arrange my PR, but I'll keep the tmp dir test since it's nice to have :)

deajan added 3 commits Jul 28, 2019
@deajan

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2019

Done, tested on my open_basedir php setup, works for me.

murrant added 5 commits Aug 20, 2019

@murrant murrant changed the title Fix installer not generating tmp directory directive Use PHP sys_get_temp_dir by default Aug 20, 2019

@murrant murrant changed the title Use PHP sys_get_temp_dir by default Use PHP sys_temp_dir by default Aug 20, 2019

@laf laf merged commit ccf2d9a into librenms:master Aug 22, 2019

5 of 6 checks passed

codeclimate 1 issue to fix
Details
Inspection Summary
Details
Node: analysis
Details
Travis CI - Pull Request Build Passed
Details
WIP Ready for review
Details
license/cla Contributor License Agreement is signed.
Details
@murrant

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-55-release-changelog-august-2019/9428/1

@deajan

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

@murrant Your commit 29ba259 breaks my PR, where you define a fixed default temp_dir value again, not giving the Config class a chance to set a default temp_dir.
Removing line $config['temp_dir'] = '/tmp'; from includes/defaults.inc.php fixes that again.

@murrant

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

@deajan not sure why that is in there, probably a rebase/merge issue. Make a new PR to fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.