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

Properly clean up configkey values from oc_appconfig table when disabling encryption #35980

Merged
merged 2 commits into from
Sep 30, 2020

Conversation

pako81
Copy link
Contributor

@pako81 pako81 commented Aug 5, 2019

Follow-up of #35756 because of changes in the branching model.

Description

Currently, we have the problem that we do not correctly clean up the configkey values in the oc_appconfig table when disabling encryption.

Specifically the useMasterKey key for the encryption app, which stays set to 1. This seems to lead to the encryption_enabled key for core to remain set to no so when trying to reenable the encryption app and running encryption:select-encryption-type masterkey, this will fail.

The same has been observed for user-keys encryption. This PR fixes this behavior by properly cleaning up the configkey values and making re-enabling of encryption possible.

Related Issue

How Has This Been Tested?

  • Manually by first activating encryption by running: occ app:enable encryption, occ encryption:enable, occ encryption:select-encryption-type masterkey -y and encryption:encrypt-all

  • Then disable it with occ app:disable encryption, occ encryption:disable

  • Re-enabling encryption once again and check that when running occ encryption:select-encryption-type masterkey, no error is triggered.

Tested the same with user-keys encryption.

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:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:

@pako81 pako81 added this to the development milestone Aug 5, 2019
@pako81 pako81 requested a review from micbar August 5, 2019 09:14
@pako81 pako81 self-assigned this Aug 5, 2019
@codecov
Copy link

codecov bot commented Aug 26, 2019

Codecov Report

Merging #35980 into master will increase coverage by 0.00%.
The diff coverage is 94.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #35980   +/-   ##
=========================================
  Coverage     64.75%   64.75%           
- Complexity    19408    19409    +1     
=========================================
  Files          1285     1285           
  Lines         75830    75845   +15     
  Branches       1336     1336           
=========================================
+ Hits          49101    49116   +15     
  Misses        26335    26335           
  Partials        394      394           
Flag Coverage Δ Complexity Δ
#javascript 54.06% <ø> (ø) 0.00 <ø> (ø)
#phpunit 65.93% <94.11%> (+<0.01%) 19409.00 <1.00> (+1.00)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
core/register_command.php 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
core/Command/Encryption/Disable.php 100.00% <100.00%> (ø) 5.00 <1.00> (+1.00)
lib/private/Share/MailNotifications.php 95.69% <0.00%> (-0.05%) 32.00% <0.00%> (-1.00%)
lib/private/legacy/app.php 62.47% <0.00%> (+0.16%) 177.00% <0.00%> (+1.00%)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b962fb4...a59779e. Read the comment docs.

@sharidas sharidas force-pushed the reactivating-encryption-fix2 branch from 223f482 to 4fb37ec Compare August 26, 2019 10:46
@jvillafanez
Copy link
Member

I'm wondering if we should decrypt the files. What could happen if there are encrypted files and we disable the encryption? What happen to those files if we enable encryption (either master key or user key) afterwards?

I guess that we can't decrypt the files ouselves before disabling encryption, but if there is any risk we should at least warn the admin.

@sharidas
Copy link
Contributor

Lets say if admin disables encryption before decrypting the file(s), the admin always have the chance to recover the files, if he/she knows what type of encryption was used.
For example:

  • Admin enables user-key encryption
  • Encrypts the fs with user-key encryption
  • Disables encryption

At this point the files which are encrypted cannot be decrypted. Because the encryption is disabled. Now to recover the encrypted files, admin has to enable encryption and select the type of encryption which was used. In this example user-key encryption. And login to the respective user(s), the encrypted files will be decrypted if user tries to download them.

If the admin attemtps for masterkey then previously encrypted files will not be decrypted.

All I see is the admin should be aware of previously used encryption here.

@jvillafanez
Copy link
Member

Maybe we should call to PM to decide what to do, mainly because the admin will reenable encryption with the wrong type (or we should assume so).

  • Write a wall of text in blood (otherwise the admin won't read it) explaining the risks if there are still encrypted files in the system
  • Ensure that it's possible to disable encryption only if there is no encrypted files.
  • Store somewhere, the previous encryption type in order to ensure the wrong encryption type won't be enabled afterwards. Makes me wonder if we really want to delete the keys stored in the configuration.

@pako81
Copy link
Contributor Author

pako81 commented Sep 9, 2019

My 2 cents: do not allow disabling encryption if we notice there are still encrypted files in FS. This way we are 100% on the safe side.

And this can easily be achieved by querying the filecache for files having the encrypted flag set to > 1.

If we do not want to consider this option, the alternative should be to allow disable but throw a warning when trying to disable encryption.

@micbar
Copy link
Contributor

micbar commented Oct 21, 2019

@pmaier1 @pako81 @cdamken Could we make a decision here?

@phil-davis
Copy link
Contributor

@micbar there is no response here to your ping on Oct 21.
Is this something that will move forward? For 10.4 or?

@pako81
Copy link
Contributor Author

pako81 commented Dec 2, 2019

@cdamken @pmaier1 may we have your thoughts on this, please? My idea on this at #35980 (comment)

@pako81
Copy link
Contributor Author

pako81 commented Dec 27, 2019

This is not making any progress.

@mmattel
Copy link
Contributor

mmattel commented Jan 22, 2020

do not allow disabling encryption if we notice there are still encrypted files in FS. This way we are 100% on the safe side

I would add that and finalize this PR. Then this can be reviewed and merged.

@phil-davis
Copy link
Contributor

@pako81 what is the status of this PR? Moving anywhere?
CC: @micbar

@pako81
Copy link
Contributor Author

pako81 commented Feb 4, 2020

@phil-davis this needs a PM decision and already pinged several times #35980 (comment)

@pmaier1
Copy link
Contributor

pmaier1 commented Sep 8, 2020

@cdamken @pmaier1 may we have your thoughts on this, please? My idea on this at #35980 (comment)

I agree with @pako81, makes sense. First decrypt, then disable.

@micbar
Copy link
Contributor

micbar commented Sep 14, 2020

@pako81 can DEV take over? @VicDeo Can bring this to a mergeable state?

@VicDeo VicDeo self-assigned this Sep 14, 2020
@owncloud owncloud deleted a comment from codecov bot Sep 16, 2020
@VicDeo
Copy link
Member

VicDeo commented Sep 17, 2020

@pako81 does the current state match your proposal in #35980 (comment) ?

@pako81
Copy link
Contributor Author

pako81 commented Sep 17, 2020

@VicDeo yes, looks fine from my POV - thx

@jvillafanez
Copy link
Member

Just to double-check, did you test running the command without encryption? I think fetchColumn will return false if there are no more rows, so I think it's better to check against this, otherwise you'll likely be checking false > 0 which might work but feels strange.

@VicDeo
Copy link
Member

VicDeo commented Sep 29, 2020

you'll likely be checking false > 0 which might work but feels strange.

true. I explicitly casted the result to boolean. Is it better now?

pako81 and others added 2 commits September 29, 2020 12:50
…pconfig table when disabling encryption

Signed-off-by: Sujith H <sharidasan@owncloud.com>
@VicDeo
Copy link
Member

VicDeo commented Sep 29, 2020

No idea about the failures:

apiFederation1-10.4.1-mariadb10.2-php7.4: Error response from daemon: No such image: owncloudci/core:latest

https://hub.docker.com/r/owncloudci/core/tags
connectivity issue?

@phil-davis
Copy link
Contributor

I restarted drone - it's having a bad day.

@VicDeo VicDeo merged commit d308045 into master Sep 30, 2020
@delete-merged-branch delete-merged-branch bot deleted the reactivating-encryption-fix2 branch September 30, 2020 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants