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

ownCloud: Revision for v10.15.0 #6280

Merged
merged 4 commits into from
Oct 19, 2024
Merged

Conversation

mreid-tt
Copy link
Contributor

@mreid-tt mreid-tt commented Oct 14, 2024

Description

This update builds on #6225 and #6279 to resolve an intermittent issue with admin user creation failing due to randomly generated passwords not meeting Synology MariaDB 10's password policy requirements. The setup now prompts users to specify a database user password, enforcing the required policy. Additionally, it creates the database, assigns the necessary permissions to the user, and refreshes the PHP configuration and extensions according to the latest ownCloud documentation.

Fixes #

Checklist

  • Build rule all-supported completed successfully
  • New installation of package completed successfully
  • Package upgrade completed successfully (Manually install the package again)
  • Package functionality was tested
  • Any needed documentation is updated/created

Type of change

  • Bug fix

@mreid-tt
Copy link
Contributor Author

During testing of new installations, an intermittent error was observed in the install log:

Error while trying to create admin user: Failed to connect to the database: An exception occurred in driver: SQLSTATE[HY000] [1045] Access denied for user 'oc_admin'@'localhost' (using password: YES)

Based on a previously reported issue (owncloud/core#27219), it was suspected that the database password policy might be causing the problem. This was confirmed by the following entry in the MariaDB error log:

[ERROR] mariadbd: Your password does not satisfy the current policy requirements: [Include special characters]

The issue was traced to a flaw in the upstream random password generation logic. The code in SecureRandom.php generates passwords by randomly selecting characters from a predefined set:

public function generate(
	$length,
	$characters = 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'
) {
	$maxCharIndex = \strlen($characters) - 1;
	$randomString = '';

	while ($length > 0) {
		$randomNumber = \random_int(0, $maxCharIndex);
		$randomString .= $characters[$randomNumber];
		$length--;
	}
	return $randomString;
}

Since the character set does not guarantee the inclusion of each character type (uppercase, lowercase, digits, symbols), the generated password may not always meet the policy requirements, especially given the limited number of symbol options.

This pull request addresses the issue by retrying the installation process if the password generation fails, repeating the process until a compliant password is produced.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

just curious: doesn't exec_occ take the password given by --database-pass?

It is very bad to have an endless loop. You should at least limit the number of loops and terminate with an error if it doesn't succeed within e.g. 5 rounds...

But if it takes the password defined in the install wizard, the loop does not make any sense. You shold validate the password in validate_preinst (or add a validator to the password in the wizard page; hopefully you can read the password validation expression in the database).

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

ok didn't see your explanations while writing the comment above,

So it looks like OwnCloud does not use the password given by --database-pass.

So I propose to patch the oc sources
either:

  • force the use of the given password by --database-pass
  • chanage the password generation function to genderate secure passwords

@mreid-tt
Copy link
Contributor Author

@hgy59, with MySQL, the database user is automatically created with a randomly generated password, and there isn't an option to specify one manually.

The integrity check function in ownCloud's PHP code will likely flag any modified code during setup as a mismatch with the source repository, potentially marking it as corrupted. Therefore, applying a patch may not be a viable solution.

For now, I believe this workaround adequately addresses the intermittent installation issue until I can persuade the upstream repository to improve their random string generator.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

So I propose to report an error to owncloud that the db password given for installation is not used...

@mreid-tt
Copy link
Contributor Author

So I propose to report an error to owncloud that the db password given for installation is not used...

The error occurs during the installation script, resulting in a broken ownCloud instance. If the 5 retry attempts fail, the user’s only option is to uninstall and try again. Although reaching the 5 retry limit is unlikely, I've included it as recommended.

Please let me know if I've misunderstood your suggestion.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

ok, my missundertanding, I meant --admin-pass not --database-pass - but the generated password is none of those

  • --admin-pass is for the web-admin-user,
  • --database-pass is the mysql root password
  • the generated password is for the internal db-user used by oc

Please let me know, if I am wrong...

So owncloud installer should either:

  • provide an argument to define the db-scheme, db-user and db-user-password for the database (or a db-scheme-prefix if multiple tables are cerated): the db-scheme is required for parallel installation of multiple oc instances using the same database
  • provide an argument to define the db-user and db-user-password for the database
  • provide an argument to define the db-user-password for the database
  • provide an argument to define the complexity of the db-user-password for the database
  • provide an argument to use an existing db-user with db-user-password for the database

Any password generated by the OC installer may not meet the password complexity enforced by a MySQL installation.

I am only I db user and not a professional, but I suppose in a managed db environment the db-user (and related schemes) is predefined by an administrator and the db-root password must not be known by an application installer.
In such a scenario only the spk installer must know the root password to create the db-user in a way that it is allowed to create the db-schemes in exec_occ.

So the oc installer seems very limitted.

BTW didn't find any documentation of available exec_occ arguments...

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

Found some more information https://doc.owncloud.com/server/next/admin_manual/configuration/database/linux_database_configuration.html#mysql-mariadb

the configuration is documented as:

<?php

"dbtype"        => "mysql",
"dbname"        => "owncloud",
"dbuser"        => "username",
"dbpassword"    => "password",
"dbhost"        => "localhost",
"dbtableprefix" => "oc_",

If we define the configuration like above, it shouldn't be an issue anymore.
If exec_occ can not handle this, it is probably the wrong way to install oc?

@mreid-tt
Copy link
Contributor Author

mreid-tt commented Oct 14, 2024

  • --admin-pass is for the web-admin-user,
  • --database-pass is the mysql root password
  • the generated password is for the internal db-user used by oc

@hgy59, the above is correct.

the configuration is documented as:
[--snip--]
If we define the configuration like above, it shouldn't be an issue anymore.
If exec_occ can not handle this, it is probably the wrong way to install oc?

Before upgrading to ownCloud v10, the setup involved manually creating the database and using an autoconfig file for the initial configuration. However, this method proved unreliable during my initial testing, and the developers are already in the process of deprecating it (see owncloud/core#31073), which means it won't be supported long-term.

The preferred approach is to use the occ script for installation and to raise enhancement requests upstream to improve the install script options.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

@mreid-tt thanks for the feedback

I started to install ownclound with the version of this branch.
What I am missing so far:

  • for DSM 7 the uninstall wizard does not provide the uninstall page that we normally add for DSM 7 packages to keep the settings or remove all
  • I miss an option to uninstall without db backup
  • I miss an option to unsinstall and keeping the db

The install with "restore" from database did not work.
I provided the same folder as in uninstall and uninstall correctly created this folder and the backup file:

# ls -l /volume1/owncloud/backup/
total 27500
-rw-r--r-- 1 sc-owncloud synocommunity 28156792 Oct 14 14:11 owncloud_backup_v10.15.0_20241014.tar.gz

The installer fails with:
Failed to install the package.
The backup file path specified is incorrect or not accessible.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

OK, my fault
To restore the db from backup, the installer wants the full name not the path for the backup file.
But the text in the empty input field is /volume1/owncloud/backup/ without a filename!
Please add a regex validator for the backup file. You already have such a validation in # Check for valid backup to restore, so the same validation should be added to the wizard field.

BTW I hate this "emptyText" value in wizard fields, because when you start typing it disapears and you can't copy the value and reuse it.
I would prefer a "defaultValue" instead an "emptyText" for this.

@mreid-tt
Copy link
Contributor Author

@hgy59, thanks for assisting with testing.

  • for DSM 7 the uninstall wizard does not provide the uninstall page that we normally add for DSM 7 packages to keep the settings or remove all

This is deliberate and all settings are removed by default. Leaving settings is not really possible since the config.php is in the webroot and not the appdata folder. This is why the backup mechanism was created.

  • I miss an option to uninstall without db backup

You simply don't enter a backup path and a backup won't be taken.

  • I miss an option to unsinstall and keeping the db

This is not an option I presented to users. Having a DB without a matching config.php can be problematic in my view and the two should be added or removed together. The exception to this would be upgrade installs which leaves the DB in place because we have precise control of the state of the config files.

The install with "restore" from database did not work. I provided the same folder as in uninstall and uninstall correctly created this folder and the backup file:

# ls -l /volume1/owncloud/backup/
total 27500
-rw-r--r-- 1 sc-owncloud synocommunity 28156792 Oct 14 14:11 owncloud_backup_v10.15.0_20241014.tar.gz

The installer fails with: Failed to install the package. The backup file path specified is incorrect or not accessible.

This would only occur if the sc-owncloud user had an issue accessing the file. The code which checks for this is:

if [ ! -f "${wizard_backup_file}" ]; then
echo "The backup file path specified is incorrect or not accessible."
exit 1
fi

As you can see, it's nothing fancy.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

Before upgrading to ownCloud v10, the setup involved manually creating the database and using an autoconfig file for the initial configuration. However, this method proved unreliable during my initial testing, and the developers are already in the process of deprecating it (see owncloud/core#31073), which means it won't be supported long-term.

The preferred approach is to use the occ script for installation and to raise enhancement requests upstream to improve the install script options.

In the given issue someone removed autoconfig to use prefilled config/config.php.
So I guess the solution would go that way...

@mreid-tt
Copy link
Contributor Author

In the given issue someone removed autoconfig to use prefilled config/config.php.
So I guess the solution would go that way...

That would require us having to create the database, its permissions and the database user. I don't like that approach as there are too many things that can change over time with future versions.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

As you can see, it's nothing fancy.

I ment the further checks for file prefix and version...

The value for the backup file must be a path with a filename of the expected prefix and version

And you can add a check for file extension .tar.gz.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

That would require us having to create the database, its permissions and the database user. I don't like that approach as there are too many things that can change over time with future versions.

And for security reason I do not like to give occ the mysql root password.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

That would require us having to create the database, its permissions and the database user. I don't like that approach as there are too many things that can change over time with future versions.

No, not creating the database, only creating the db-user with the permissions to create the database (IMHO).

@mreid-tt
Copy link
Contributor Author

@hgy59, one of the main goals behind my work in the original PR was to ensure a setup free of security and configuration warnings in the Settings -> General screen. Achieving this clean state requires several configurations:

  1. Redirecting Apache HTTP to HTTPS
  2. Enabling Apache HTTP Strict Transport Security
  3. Configuring PHP memory caching
  4. Setting up PHP file locking
  5. Scheduling ownCloud background jobs

While the first four can be handled via configuration files, setting up cron jobs requires a fully configured ownCloud instance. This can't be achieved if the user has to complete additional setup steps after the installation wizard.

I also prefer having a finalized setup immediately after the wizard, without requiring further configuration. In my experience, additional setup steps often introduce potential points of failure with limited options for troubleshooting.

Regarding the root password, I'm not sure what challenges arise when passing it to occ. I could explore the possibility of pre-creating the database user as you suggested, with permissions to create a database. However, this approach would involve granting global CREATE permissions to the user, which poses a security risk.

@hgy59
Copy link
Contributor

hgy59 commented Oct 14, 2024

Just successfully installed owncloud with a precreated db user and it works as expected.

occ creates a db user oc_admin with a generated password, when the given --database-user is the root user.
But if the given --database-user is an existing user with privileges for the given --database-name, everything works as expected and the given --database-pass is used and none is generated.

All you have to change is to create the db user and use it for occ.

This brings several advantages

  • occ must not have mysql root access
  • the db password can be provided by the user in the wizard or generated in service_postinst
  • db-user and password can be stored in installer-variables for later use (if kept in package uninstall)
  • optinal a different db-name could be provided.

@mreid-tt
Copy link
Contributor Author

@hgy59, that is very interesting. Let me roll-back some stuff and try your approach.

@mreid-tt
Copy link
Contributor Author

@hgy59, thanks for the guidance—it worked well. The documentation doesn’t clearly cover this type of deployment, but it seems to be functioning as expected. I'll run a few more tests to confirm.

I've also added regex validation for the backup restore process to explicitly check the file path earlier in the workflow.

BTW I hate this "emptyText" value in wizard fields, because when you start typing it disapears and you can't copy the value and reuse it.
I would prefer a "defaultValue" instead an "emptyText" for this.

I typically use "emptyText" to display examples. When the examples are specific to the user, I prefer "defaultValue," but otherwise, I use "emptyText" as a form of documentation. If there are any particular instances you’d like me to change, just let me know.

@mreid-tt
Copy link
Contributor Author

@tbc0309 you can go ahead and test this build as it should resolve all open issues with installation.

@tbc0309
Copy link

tbc0309 commented Oct 15, 2024

Okay, today I'm here to invite multiple people to test and provide feedback.

@mreid-tt
Copy link
Contributor Author

Okay, today I'm here to invite multiple people to test and provide feedback.

Thanks for this. How has the testing been going so far?

@mreid-tt
Copy link
Contributor Author

@tbc0309 any further feedback before I merge and publish this?

@mreid-tt mreid-tt merged commit 5d0616d into SynoCommunity:master Oct 19, 2024
7 checks passed
@mreid-tt mreid-tt deleted the owncloud-patch branch October 19, 2024 14:01
@mreid-tt mreid-tt added status/published Published and activated (may take up to 48h until visible in DSM package manager) and removed status/ready-to-merge labels Oct 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status/published Published and activated (may take up to 48h until visible in DSM package manager)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants