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

feat: allow to configure php.user #45307

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

szaimen
Copy link
Contributor

@szaimen szaimen commented May 14, 2024

config/config.sample.php Outdated Show resolved Hide resolved
console.php Outdated Show resolved Hide resolved
Base automatically changed from revert/27613/get-config-owner to master May 15, 2024 09:30
@szaimen szaimen force-pushed the enh/noid/allow-configure-config.owner branch from ec1b47e to 4494e8f Compare May 15, 2024 10:07
@szaimen szaimen changed the title feat: allow to configure config.owner feat: allow to configure php.user May 15, 2024
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I insist that it should use the new config value instead of the config.php owner if the value is filled.
Is if php.user is empty, it checks that running user is the owner of config.php.
If php.user is filled, it checks that running user is the one in php.user.

See my previous change suggestion.

console.php Outdated Show resolved Hide resolved
@szaimen
Copy link
Contributor Author

szaimen commented May 16, 2024

I insist that it should use the new config value instead of the config.php owner if the value is filled. Is if php.user is empty, it checks that running user is the owner of config.php. If php.user is filled, it checks that running user is the one in php.user.

See my previous change suggestion.

but we already get the user via posix_getuid or not?

@come-nc
Copy link
Contributor

come-nc commented May 16, 2024

I insist that it should use the new config value instead of the config.php owner if the value is filled. Is if php.user is empty, it checks that running user is the owner of config.php. If php.user is filled, it checks that running user is the one in php.user.
See my previous change suggestion.

but we already get the user via posix_getuid or not?

We do, and we want to check that it’s the correct one.

@szaimen
Copy link
Contributor Author

szaimen commented May 17, 2024

I insist that it should use the new config value instead of the config.php owner if the value is filled. Is if php.user is empty, it checks that running user is the owner of config.php. If php.user is filled, it checks that running user is the one in php.user.
See my previous change suggestion.

but we already get the user via posix_getuid or not?

We do, and we want to check that it’s the correct one.

all right, done!

@szaimen szaimen requested a review from come-nc May 17, 2024 06:31
cron.php Fixed Show resolved Hide resolved
cron.php Fixed Show fixed Hide fixed
@szaimen szaimen force-pushed the enh/noid/allow-configure-config.owner branch from b3d21be to de2dbf5 Compare May 21, 2024 11:49
@szaimen szaimen marked this pull request as ready for review May 21, 2024 11:49
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/allow-configure-config.owner branch from de2dbf5 to 087ba07 Compare May 21, 2024 11:49
@szaimen szaimen requested review from ChristophWurst, joshtrichards, skjnldsv, a team and yemkareems and removed request for a team May 21, 2024 11:49
@ChristophWurst ChristophWurst added the pending documentation This pull request needs an associated documentation update label May 21, 2024
@come-nc
Copy link
Contributor

come-nc commented May 21, 2024

Not completely happy about it but too much time was spend already, and it does fix the usecase.

Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 otherwise

console.php Outdated Show resolved Hide resolved
@skjnldsv skjnldsv removed their request for review May 22, 2024 06:47
console.php Outdated Show resolved Hide resolved
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/allow-configure-config.owner branch from f49d3f0 to a0d9dca Compare May 22, 2024 13:54
Signed-off-by: Simon L <szaimen@e.mail.de>
console.php Outdated Show resolved Hide resolved
cron.php Fixed Show fixed Hide fixed
Signed-off-by: Simon L <szaimen@e.mail.de>
@szaimen szaimen force-pushed the enh/noid/allow-configure-config.owner branch from 9281f6f to 1afe55b Compare May 24, 2024 13:00
$configUser = fileowner(OC::$configDir . 'config.php');
if ($user !== $configUser) {
$configuredUser = $config->getSystemValueString('php.user', '');
if ($user !== $configUser && $username !== null && $userName !== $configuredUser) {

Check failure

Code scanning / Psalm

TypeDoesNotContainType Error

Type null for $username is always !null
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this is not true? How can I fix this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure? Documentation says it should return false on failure: https://www.php.net/manual/en/function.posix-getpwuid.php

echo "Console has to be executed with the user that owns the file config/config.php" . PHP_EOL;
echo "Current user id: " . $user . PHP_EOL;
echo "Owner id of config.php: " . $configUser . PHP_EOL;
echo "Try adding 'sudo -u #" . $configUser . "' to the beginning of the command (without the single quotes)" . PHP_EOL;
echo "If running with 'docker exec' try adding the option '-u " . $configUser . "' to the docker command (without the single quotes)" . PHP_EOL;
echo "Another option is to configure 'php.user' in config.php which will overwrite this check.";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think as worded this is misleading, the option doesn't overwrite the check, it changes that the expected value of the check is.

I would go with something like

If the config file is not owned by the user running the webserver you can set the correct user by setting the 'php.user' option in your config.php

$configUser = fileowner(OC::$configDir . 'config.php');
if ($user !== $configUser) {
$configuredUser = $config->getSystemValueString('php.user', '');
if ($user !== $configUser && $username !== null && $userName !== $configuredUser) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check always needs to check for $configuredUser if set. Also accepting $configUser can lead to the very issue this is trying to prevent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like

$phpUser = $config->getSystemValueString('php.user', '');
if (!$phpUser) {
    $userNameArray = posix_getpwuid($user);
	if ($userNameArray !== false) {
		$phpUser = $userNameArray['name'];
	}
}
if ($user != $phpUser) {

maybe

$configUser = fileowner(OC::$configDir . 'config.php');
if ($user !== $configUser) {
$configuredUser = $config->getSystemValueString('php.user', '');
if ($user !== $configUser && $username !== null && $userName !== $configuredUser) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

@Dreamsorcerer
Copy link
Contributor

Looks like a reasonable solution, if there's not an easy way to just verify if we can write to the directory (as the user is not really relevant, just whether we have write access). I don't remember my PHP so well, but in Python I'd probably just try/except a file write as a test.

@icewind1991
Copy link
Member

as the user is not really relevant, just whether we have write access

The user is actually relevant (there is a separate check for checking write access to the data dir iirc). If an occ/cron command ends up create a file it will be owned by whatever user the command is ran as. So if a user does sudo occ .... Any file created by that will then be read-only to the webserver.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants