fix creation of versions of encrypted files on external storages #23675

schiessle · 2016-03-30T21:34:09Z

in order to create a 1:1 copy of a file if a version gets created we need to store this information on copyBetweenStorage(). This allows us to by-pass the encryption wrapper if we read the source file.

@karlitschek we should consider to backport it to 9.0, 8.2 and 8.1. At the moment versions of files on external storages are stored unencrypted!

adjust and add unit tests

cc @PVince81 @LukasReschke please have a look... Thanks!

karlitschek · 2016-03-30T21:57:38Z

backport is fine if reviewed successfully by others 👍

PVince81 · 2016-03-31T07:04:55Z

lib/private/Server.php

@@ -301,6 +305,9 @@ public function __construct($webRoot, \OC\Config $config) {
 				'\\OC\\Memcache\\ArrayCache'
 			);
 		});
+		$this->registerService('ArrayCache', function(Server $c) {
+			return new ArrayCache();


I don't think this should be global at this level. If other apps use it, there will be key name conflicts.

If you still think it should be that way, then please use prefixes for the key names you use.

@icewind1991 what do you think ?

its fine here, on usage a sane prefix should be used

I don't see the reason for a global ArrayCache in the server container

We need the same instance all the time

There is only one encryption manager right, so that would pass one instance of the cache to all wrappers it's creates

Well it's the util class that wraps, and that one also is generated new each time.

I agree with @icewind1991, we should find a way to this ArrayCache global only for the encryption code, not OC-wide global. If this was in the encryption app, we could have it in the encryption app's container.

This is in core, not in the encryption app, but @schiesbn is trying to do it nicely anyway

PVince81 · 2016-03-31T07:08:03Z

Setting to critical since the bug breaks versions with encryption + ext storage (data loss)

PVince81 · 2016-03-31T07:10:56Z

Fixes #23681

schiessle · 2016-03-31T10:14:07Z

I removed the global array cache, this increased the size of the PR a bit but I tried to keep the changes as small as possible.

icewind1991 · 2016-03-31T10:22:16Z

lib/private/encryption/encryptionwrapper.php

+use OCP\Files\Storage;
+use OCP\ILogger;
+
+class EncryptionWrapper {


add a note that this is not the actuall wrapper but the thing that applies the wrapper

good point... done.

PVince81 · 2016-03-31T10:24:46Z

@schiesbn thank you, the code looks much better!

Doesn't work though, when I overwrite more than once (with Webdav), it seems the next versions get broken:

schiessle · 2016-03-31T10:28:05Z

@PVince81 I can't reproduce it with sftp, maybe a independent webdav external storage issue?

davitol · 2016-03-31T10:29:58Z

@PVince81 @schiesbn As Vincent said, it only works for the 1st version of the file. The following via webdavversions are broken for me too. I used SFTP as external storage

PVince81 · 2016-03-31T10:30:34Z

@schiesbn my test was with SFTP. If you can't reproduce we can debug it on my machine later.

PVince81 · 2016-03-31T10:37:00Z

My steps:

Setup OC from scratch
Enable encryption, log out, log in again
Enable external storage app
Mount SFTP as "/sftp" (the remote folder is completely empty)
Start cadaver
mkdir /sftp/enc
`cd enc'
Download https://s3.owncloud.com/owncloud/index.php/s/w6x2yXIxOqqYjhy/download and put it in a local folder for uploading with cadaver
cadaver: put bacon.txt three times
Go to the web UI, open the versions dropdown
Download the file that has a broken thumbnail

Make sure to NOT use the web UI until the end to not pre-generate previews, in case it matters.

schiessle · 2016-03-31T10:37:04Z

I always tested it with small text files, updating it with the text editor. I remember @LukasReschke said yesterday that we also need the RetryWrapper for sftp. Maybe that's the issue if you test it with larger files? @LukasReschke do you already prepared a pull request for it?

PVince81 · 2016-03-31T10:38:07Z

@schiesbn #23672 was merged, please rebase onto master so we get it

PVince81 · 2016-03-31T10:39:07Z

Same result with RetryWrapper on SFTP 😦

schiessle · 2016-03-31T10:47:20Z

I think that's a different issue. If I try it with large files not only some versions are broken but also the original file after some updates "Bad Signature"...

schiessle · 2016-03-31T10:49:42Z

... if you check the versions on the hard disc all versions should be encrypted. That's what is fixed by this issue. We need to investigate other issues independently

PVince81 · 2016-03-31T10:50:57Z

@schiesbn the version files on-disk are indeed encrypted properly. Fine by me, I'll make a separate issue for it.

PVince81 · 2016-03-31T10:51:11Z

👍 fixes the "unencrypted versions" creation bug

schiessle · 2016-03-31T10:51:31Z

If I look at the file cache the 'encryption' is set for all files (original and all versions) to '1' which is of course wrong.

LukasReschke · 2016-03-31T17:08:35Z

@PVince81 @schiesbn As Vincent said, it only works for the 1st version of the file. The following via webdavversions are broken for me too. I used SFTP as external storage

@davitol Can you retest this including #23707?

LukasReschke · 2016-03-31T17:11:02Z

lib/private/encryption/manager.php

@@ -61,13 +62,14 @@ class Manager implements IManager {
 	 * @param View $rootView
 	 * @param Util $util
 	 */
-	public function __construct(IConfig $config, ILogger $logger, IL10N $l10n, View $rootView, Util $util) {
+	public function __construct(IConfig $config, ILogger $logger, IL10N $l10n, View $rootView, Util $util, ArrayCache $arrayCache) {


PHPDoc missing

LukasReschke · 2016-03-31T17:13:02Z

Code works fine. Can you adjust my remarks? Thanks a lot 🚀 👍

in order to create a 1:1 copy of a file if a version gets created we need to store this information on copyBetweenStorage(). This allows us to by-pass the encryption wrapper if we read the source file.

schiessle · 2016-03-31T17:25:56Z

addressed @LukasReschke comments.

PVince81 · 2016-03-31T17:26:41Z

@karlitschek many code changes, please reconfirm backport

karlitschek · 2016-03-31T18:35:49Z

backport is fine 👍

schiessle · 2016-03-31T19:12:58Z

stable 9.0: #23710
stable 8.2: #23711
stable 8.1: no backport, code base differs to much

lock · 2019-08-06T15:01:59Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

schiessle added Type:Bug 3 - To Review app:encryption labels Mar 30, 2016

DeepDiver1975 added the in progress label Mar 30, 2016

PVince81 reviewed Mar 31, 2016
View reviewed changes

PVince81 added the sev1-critical label Mar 31, 2016

PVince81 added this to the 9.1-current milestone Mar 31, 2016

PVince81 mentioned this pull request Mar 31, 2016

Encryption + ext storage + versions broken #23681

Closed

schiessle force-pushed the fix_encryption_versions_on_external_storages branch 2 times, most recently from 3540868 to c46643e Compare March 31, 2016 10:11

icewind1991 reviewed Mar 31, 2016
View reviewed changes

schiessle force-pushed the fix_encryption_versions_on_external_storages branch from c46643e to f5e5195 Compare March 31, 2016 10:30

LukasReschke mentioned this pull request Mar 31, 2016

Make sure that the encrypted version is set #23707

Merged

LukasReschke reviewed Mar 31, 2016
View reviewed changes

fix creation of versions of encrypted files on external storages

93ed965

in order to create a 1:1 copy of a file if a version gets created we need to store this information on copyBetweenStorage(). This allows us to by-pass the encryption wrapper if we read the source file.

schiessle force-pushed the fix_encryption_versions_on_external_storages branch from f5e5195 to 93ed965 Compare March 31, 2016 17:25

schiessle added 4 - To release and removed 3 - To Review labels Mar 31, 2016

schiessle added the backport-request label Mar 31, 2016

karlitschek merged commit fdb5907 into master Mar 31, 2016

DeepDiver1975 removed the in progress label Mar 31, 2016

karlitschek deleted the fix_encryption_versions_on_external_storages branch March 31, 2016 18:36

This was referenced Mar 31, 2016

[stable9] fix creation of versions of encrypted files on external storages #23710

Merged

[stable8.2] fix creation of versions of encrypted files on external storages #23711

Merged

schiessle mentioned this pull request Mar 31, 2016

make sure that we always remove the file again from the cache after t… #23713

Merged

MorrisJobke added 8.2.4-current and removed backport-request labels Apr 4, 2016

lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019

fix creation of versions of encrypted files on external storages #23675

fix creation of versions of encrypted files on external storages #23675

Conversation

schiessle commented Mar 30, 2016

karlitschek commented Mar 30, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 31, 2016

PVince81 commented Mar 31, 2016

schiessle commented Mar 31, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Mar 31, 2016

schiessle commented Mar 31, 2016

davitol commented Mar 31, 2016

PVince81 commented Mar 31, 2016

PVince81 commented Mar 31, 2016

schiessle commented Mar 31, 2016

PVince81 commented Mar 31, 2016

PVince81 commented Mar 31, 2016

schiessle commented Mar 31, 2016

schiessle commented Mar 31, 2016

PVince81 commented Mar 31, 2016

PVince81 commented Mar 31, 2016

schiessle commented Mar 31, 2016

LukasReschke commented Mar 31, 2016

Choose a reason for hiding this comment

LukasReschke commented Mar 31, 2016

schiessle commented Mar 31, 2016

PVince81 commented Mar 31, 2016

karlitschek commented Mar 31, 2016

schiessle commented Mar 31, 2016

lock bot commented Aug 6, 2019