Mount encrypt password #38728

jvillafanez · 2021-05-12T13:02:39Z

Description

Ensure configuration parameters marked as passwords in the mount point configuration are encrypted in the DB.
The encryption only happens on rest. The values are decrypted after getting the information from normal means so they can be sent to the storage without problems. The storage implementations won't need to decrypt anything.
A migration is also provided so the mount points are properly updated.

Note that this applies to any password parameter that is saved through the storage. Parameters that don't go through the storage, such as login credentials, and which are saved differently still have its own way of handling. There is no change in this regard.

Related Issue

https://github.com/owncloud/enterprise/issues/4461

Motivation and Context

How Has This Been Tested?

Manually tested.
Test 1

Configure a mount point having any parameter marked as password (textbox in the web UI will hide the content)
Check the DB. Target parameter has the value encrypted

Test 2

Configure a mount point having a parameter marked as password in OC 10.7.0 or less
Check that you can access normally to the mount point
Update the code (planned 10.8.0)
Check that you can still access to the mount point without problems (migrations didn't break anything)
Check that the DB has the parameter encrypted.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Database schema changes (next release will require increase of minor version instead of patch)
Breaking change (fix or feature that would cause existing functionality to change)
Technical debt
Tests only (no source changes)

Checklist:

jvillafanez · 2021-05-12T16:48:37Z

@phil-davis
https://drone.owncloud.com/owncloud/core/29990/115/14 seems a wrong test.
The local storage doesn't have those parameters. The current code, except for a few keys that are left due to compatibility, it hides the password based on the backend definition. Since the local storage doesn't have a "client_secret" parameter, the current code assumes it doesn't need protection.

The unit tests should have been fixed with the last commit

mmattel · 2021-05-13T14:00:21Z

docs relevant (wnd), pls file an issue when close to merge

phil-davis · 2021-05-14T08:21:24Z

PR #37015 added those test scenarios. From reading the PR, I think that we did not know exactly which keys were appropriate to use for local storage (vs Google and other storages).

Now client_secret comes back with the text, in the case of local storage. It no longer displays "***". In this PR we could adjust the test expectation:

client_secret: "userSecretKey"

that will demonstrate that if you set client_secret on local storage, that the value will now be displayed in clear-text.

Or we can remove client_secret from the test completely. (And remove whatever other keys you think are not-useful to have)

jvillafanez · 2021-05-14T08:26:33Z

Taking into account that we don't plan to have a "client_secret" for the local storage, adjusting the test seems good enough to me.

sonarcloud · 2021-05-17T10:26:31Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

No Coverage information
No Duplication information

phil-davis

Maybe not worth changing just a comment.

phil-davis · 2021-05-17T10:50:04Z

apps/files_external/appinfo/Migrations/Version20210511082903.php

+use OCP\IConfig;
+
+/**
+ * Auto-generated migration step: Please modify to your needs!


IMO this comment is not relevant - the migration step has been modified according to our needs.

If that's the only change, let me ignore it.
I'll change it if there is anything more that needs to change.

@jvillafanez you could choose some reviewers, so that they see that this is ready for review.

IljaN

LGTM 👍

jvillafanez · 2021-05-20T12:14:54Z

@mmattel docs issue in owncloud/docs#3585

phil-davis · 2021-05-24T11:25:55Z

apps/files_external/appinfo/Migrations/Version20210511082903.php

+			if ($enabledApp !== 'files_external' && \OC_App::isType($enabledApp, ['filesystem'])) {
+				try {
+					\OC_App::loadApp($enabledApp);
+				} catch (NeedsUpdateException $ex) {


@jvillafanez https://drone.owncloud.com/owncloud/update-testing/1207/1/5 update-testing nightly is failing (all the pipelines):

+ php ./occ up ownCloud or one of the apps require upgrade - only a limited number of commands are available You may use your browser or the occ upgrade command to do the upgrade 2021-05-21T00:06:46+00:00 Set log level to debug 2021-05-21T00:06:46+00:00 Turned on maintenance mode 2021-05-21T00:06:46+00:00 Repair step: Upgrade app code from the marketplace 2021-05-21T00:06:46+00:00 Repair info: Enabling market app to assist with update 2021-05-21T00:06:46+00:00 Repair info: Using market to update existing apps 2021-05-21T00:06:46+00:00 Repair info: Attempting to update the following existing compatible apps from market: dav, federatedfilesharing, files, files_external, files_sharing, files_trashbin, files_versions, firstrunwizard, notifications, provisioning_api 2021-05-21T00:06:46+00:00 Repair info: Fetching app from market: dav 2021-05-21T00:06:46+00:00 Repair info: App (dav) is not installed 2021-05-21T00:06:46+00:00 Repair info: Fetching app from market: federatedfilesharing 2021-05-21T00:06:46+00:00 Repair info: App (federatedfilesharing) is not installed 2021-05-21T00:06:46+00:00 Repair info: Fetching app from market: files 2021-05-21T00:06:47+00:00 Repair info: App (files) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: files_external 2021-05-21T00:06:47+00:00 Repair info: App (files_external) is not installed 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: files_sharing 2021-05-21T00:06:47+00:00 Repair info: App (files_sharing) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: files_trashbin 2021-05-21T00:06:47+00:00 Repair info: App (files_trashbin) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: files_versions 2021-05-21T00:06:47+00:00 Repair info: App (files_versions) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: firstrunwizard 2021-05-21T00:06:47+00:00 Repair info: App (firstrunwizard) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: notifications 2021-05-21T00:06:47+00:00 Repair info: App (notifications) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: Fetching app from market: provisioning_api 2021-05-21T00:06:47+00:00 Repair info: App (provisioning_api) is not known at the marketplace. 2021-05-21T00:06:47+00:00 Repair info: App was not updated: dav 2021-05-21T00:06:47+00:00 Repair info: App was not updated: federatedfilesharing 2021-05-21T00:06:47+00:00 Repair info: App was not updated: files 2021-05-21T00:06:47+00:00 Repair info: App was not updated: files_external 2021-05-21T00:06:47+00:00 Repair info: App was not updated: files_sharing 2021-05-21T00:06:47+00:00 Repair info: App was not updated: files_trashbin 2021-05-21T00:06:47+00:00 Repair info: App was not updated: files_versions 2021-05-21T00:06:47+00:00 Repair info: App was not updated: firstrunwizard 2021-05-21T00:06:47+00:00 Repair info: App was not updated: notifications 2021-05-21T00:06:47+00:00 Repair info: App was not updated: provisioning_api 2021-05-21T00:06:47+00:00 Repair step: Repair MySQL database engine 2021-05-21T00:06:47+00:00 Repair step: Repair MySQL collation 2021-05-21T00:06:47+00:00 Repair info: All tables already have the correct collation -> nothing to do 2021-05-21T00:06:47+00:00 Repair step: Repair SQLite autoincrement 2021-05-21T00:06:47+00:00 Repair step: Repair orphaned reshare 2021-05-21T00:06:47+00:00 Repair step: Repair duplicate entries in oc_lucene_status 2021-05-21T00:06:47+00:00 Repair info: lucene_status table does not exist -> nothing to do 2021-05-21T00:06:47+00:00 Updating database schema 2021-05-21T00:06:50+00:00 Updated database 2021-05-21T00:06:50+00:00 Updating <firstrunwizard> ... 2021-05-21T00:06:50+00:00 Updated <firstrunwizard> to 1.2.0 2021-05-21T00:06:50+00:00 Updating <files> ... 2021-05-21T00:06:50+00:00 Updated <files> to 1.5.2 2021-05-21T00:06:50+00:00 Updating <files_sharing> ... 2021-05-21T00:06:52+00:00 Updated <files_sharing> to 0.14.0 2021-05-21T00:06:52+00:00 Updating <files_trashbin> ... 2021-05-21T00:06:53+00:00 Updated <files_trashbin> to 0.9.1 2021-05-21T00:06:53+00:00 Updating <files_versions> ... 2021-05-21T00:06:53+00:00 Updated <files_versions> to 1.3.0 2021-05-21T00:06:53+00:00 Updating <notifications> ... 2021-05-21T00:06:53+00:00 Updated <notifications> to 0.5.2 2021-05-21T00:06:53+00:00 Updating <provisioning_api> ... 2021-05-21T00:06:53+00:00 Updated <provisioning_api> to 0.5.0 2021-05-21T00:06:56+00:00 OC\NeedsUpdateException: 2021-05-21T00:06:56+00:00 Update failed 2021-05-21T00:06:56+00:00 Maintenance mode is kept active 2021-05-21T00:06:56+00:00 Reset log level

It started failing the morning of 2021-05-21. It passed the morning of 2021-05-20. So something merged on 2021-5-20 might be the cause.

This PR has a migration, and mentions NeedsUpdateException - maybe there is more or different code that is needed here?

(I haven't tried to reproduce locally! I am just guessing at the moment.)

I raised issue #38770 so that we follow-up this.

I don't discard problems since I mostly tested only upgrading the files_external app and not with other apps. This is probably a new case because the migration requires loading other apps, which could also require updating.

I'll need additional information about where it's failing exactly. In theory, if an additional FS app requires updating, the migration tries to update and then load the app, so it shouldn't crash like that....

jvillafanez · 2021-05-26T07:16:28Z

Linking with #38773 for the follow up

jvillafanez added 8 commits May 10, 2021 09:22

Encrypt password fields in the mount point configuration

1bfbea8

Remove encryption from the DBConfigService

edd3583

Migrate passwords to ensure they're encrypted

1084a6b

Do not base64-encode the encrypted password

8642dd5

Raise files_external version for the migration

6724674

Load FS apps for the migration

e5bb3c5

Hide keys marked as passwords in the files_external:list command

70e7f7d

Adjust message

53da2d9

jvillafanez added the 2 - Developing label May 12, 2021

jvillafanez self-assigned this May 12, 2021

jvillafanez added 2 commits May 12, 2021 16:47

Skip the migration for new installations

ce25a63

Adjust unit tests

158b664

Adjust acceptance test

4854e94

jvillafanez force-pushed the mount_encrypt_password branch from 6c9e642 to 85c0fd8 Compare May 14, 2021 11:21

Cleaner code for CI

22f5e1c

jvillafanez force-pushed the mount_encrypt_password branch from 85c0fd8 to 22f5e1c Compare May 14, 2021 11:39

jvillafanez added 3 - To Review and removed 2 - Developing labels May 14, 2021

jvillafanez marked this pull request as ready for review May 14, 2021 13:46

Add changelog entry

e4f5adf

phil-davis reviewed May 17, 2021

View reviewed changes

jvillafanez requested review from AlexAndBear, JammingBen, micbar and IljaN May 17, 2021 11:21

IljaN approved these changes May 18, 2021

View reviewed changes

owncloud deleted a comment from update-docs bot May 19, 2021

jvillafanez mentioned this pull request May 20, 2021

All configuration options for mount points will be encrypted if they're passwords owncloud/docs#3585

Closed

12 tasks

jvillafanez merged commit 1efa14c into master May 20, 2021

delete-merged-branch bot deleted the mount_encrypt_password branch May 20, 2021 12:15

phil-davis reviewed May 24, 2021

View reviewed changes

jvillafanez mentioned this pull request May 25, 2021

Fix migration by loading the app without checking, after correct update #38773

Merged

11 tasks

jnweiger mentioned this pull request Jul 14, 2021

[QA] oc_appconfig stores passwords unencrypted #38989

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mount encrypt password #38728

Mount encrypt password #38728

jvillafanez commented May 12, 2021 •

edited

jvillafanez commented May 12, 2021

mmattel commented May 13, 2021

phil-davis commented May 14, 2021

jvillafanez commented May 14, 2021

sonarcloud bot commented May 17, 2021

phil-davis left a comment

phil-davis May 17, 2021

jvillafanez May 17, 2021

phil-davis May 17, 2021

IljaN left a comment

jvillafanez commented May 20, 2021

phil-davis May 24, 2021

phil-davis May 24, 2021

jvillafanez May 24, 2021

jvillafanez commented May 26, 2021

Mount encrypt password #38728

Mount encrypt password #38728

Conversation

jvillafanez commented May 12, 2021 • edited

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

jvillafanez commented May 12, 2021

mmattel commented May 13, 2021

phil-davis commented May 14, 2021

jvillafanez commented May 14, 2021

sonarcloud bot commented May 17, 2021

phil-davis left a comment

Choose a reason for hiding this comment

phil-davis May 17, 2021

Choose a reason for hiding this comment

jvillafanez May 17, 2021

Choose a reason for hiding this comment

phil-davis May 17, 2021

Choose a reason for hiding this comment

IljaN left a comment

Choose a reason for hiding this comment

jvillafanez commented May 20, 2021

phil-davis May 24, 2021

Choose a reason for hiding this comment

phil-davis May 24, 2021

Choose a reason for hiding this comment

jvillafanez May 24, 2021

Choose a reason for hiding this comment

jvillafanez commented May 26, 2021

jvillafanez commented May 12, 2021 •

edited