encryption 2.0 - early stage #14376

schiessle · 2015-02-19T13:43:21Z

This is a really early stage of encryption 2.0. I created the pull request to let people have a early look and to discuss some stuff but most of it will still change, so no need to review this yet.

The whole idea is to make encryption more modular and define the current encryption app into two parts:

Encryption modules implemented as app which can implement various encryption algorithm and key management, etc.
The part which will be always the same for any encryption module like the storage wrapper, a stream wrapper, managing the header of the encrypted file, etc. This will be part of core.

A short explanation of what you can see at this pull request:

lib/private/encryption/keystorage.php provides a keystorage for encryption keys. This can be used by every encryption module to read/write user or file specific keys. The keystorage makes sure that independent from the used encryption module we always know where to find which keys for which user or file, e.g. for the trash bin.
lib/private/encryption/manager.php: each encryption module will register itself at the encryption manager. The manager will then take care of all loaded encryption module and make sure that the right one is used to read/write files
lib/private/files/storage/wrapper/encryption.php: This should replace the file proxies from the old encryption app and make sure that all files are encrypted/decrypted before written/read from disk
lib/private/files/stream/encryption.php: The stream wrapper which will read/write encrypted files. The stream wrapper will send the data to the encryption module to encrypt/decrypt it.
lib/public/encryption/iencryptionmodule.php: That's the interface each encryption module needs to implement

At the moment this is more or less a skeleton which will be filled with real code. Some will be copied from the current encryption app located in apps/files_encryption and some parts will be written from scratch.

I will focus on the part in core while @th3fallen will work on the first encryption module which will behave exactly like the current encryption app.

TODO:

kill file proxy - added by @DeepDiver1975

schiessle · 2015-02-19T13:46:16Z

At the moment I work on the storage wrapper and encounter some problems.

In some cases we need to bypass the wrapper, e.g if I want to read the raw file to extract some information from the header. In the current encryption app (which used proxies instead of the storage wrapper) we disabled the file proxies for these kind of operations which was not the most elegant solution but it works. I'm struggling with how I should to this with the storage wrapper. Is there also a way to by-pass the storage wrapper for some operations? @icewind1991 any idea?

DeepDiver1975 · 2015-02-19T13:47:29Z

lib/private/files/storage/wrapper/encryption.php

+
+namespace OC\Files\Storage\Wrapper;
+
+class Encryption extends Wrapper {


🚀 encryption as storage wrapper 🚀

Yes, let's hope everything will work out well and that we don't get stopped by stuff like I mentioned here #14376 (comment)

MorrisJobke · 2015-02-19T14:13:46Z

In some cases we need to bypass the wrapper, e.g if I want to read the raw file to extract some information from the header. In the current encryption app (which used proxies instead of the storage wrapper) we disabled the file proxies for these kind of operations which was not the most elegant solution but it works. I'm struggling with how I should to this with the storage wrapper. Is there also a way to by-pass it for some operations

Can we please avoid black magic and special by-pass of components. It will definetly cause headaches and problems later. Please please please don't by-pass anything. Improve the storage or whatever ... and don't start again with kung fu ✨ black magic 🔮 .

Sorry if this sounds harsh, I don't meant it so. I just want a encryption layer, that more than one person could understand, debug, fix and maintain. ;) 🚀

icewind1991 · 2015-02-19T15:34:58Z

Is there also a way to by-pass the storage wrapper for some operations?

The wrapper itself can bypass it by calling the wrapper storage directly but the main way to "bypass" it is to not wrap any storage we want to access unencrypted in the first place.

For example encryption keys can be put in a seperate storage (but still on the same physical location) and the encryption wrapper leaves that storage alone.

icewind1991 · 2015-02-19T15:35:44Z

lib/private/encryption/keystorage.php

+ * License along with this library.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+namespace OC\Encryption {


I would prefer not using the block style namespaces

👍 to that, we use these no where else in OC or anywhere else in the community for that matter ;)

I'm quite sure that it is also used somewhere else in OC and I copy&pasted it. Anyway, I agree with you and will change it.

schiessle · 2015-02-19T16:15:07Z

The wrapper itself can bypass it by calling the wrapper storage directly but the main way to "bypass" it is to not wrap any storage we want to access unencrypted in the first place.

The problem is that we are talking about a storage which we want to access unencrypted in general but not in some special situations. We are talking about a normal file uploaded to ownCloud which is encrypted and for normal read operation it should of course get decrypted. But in some cases we want to read the header only, so in this case a fopen shouldn't strip the header away and start reading and decrypting the payload.

icewind1991 · 2015-02-19T16:17:43Z

But in some cases we want to read the header only

Add a method to the storage wrapper to get that?

schiessle · 2015-02-19T16:27:31Z

Add a method to the storage wrapper to get that?

Hm, I'm not sure. One example is here: https://github.com/owncloud/core/pull/14376/files#diff-a36b9c0307e0126dd50baa38fb4bb74aR169

To call the right encryption module to re-calculate the unencrypted size I need the encryption-module-id from the header. But as soon as I do a fopen to read the header of the file the stream wrapper will be executed again for fopen and perform all the steps the same way as I would like to read the unencrypted payload even if I want to read the raw file at this point in time.

Don't see how a extra method could help. It will always need to start a read operation on a file covered by the wrapper so it will always call the wrapper and perform the necessary steps to read the payload unless I can tell him that I want the raw file in this case.

Or do I miss something?

icewind1991 · 2015-02-19T16:39:37Z

Instead of passing just the path to calculateUnencryptedSize either pass a stream with the unencrypted content (easy since you're calling it from the wrapper) or pass the unwrapped storage and the internalpath.

icewind1991 · 2015-02-19T16:41:11Z

lib/private/files/storage/wrapper/encryption.php

+	 */
+	public function filesize($path) {
+		$fullPath = $this->getFullPath($path);
+		$fileInfo = \OC\Files\Filesystem::getFileInfo($fullPath);


please avoid calling call View operations from a storage (wrapper), instead do something like

$info = $this->getCache()->get($path); if($info['size'] > 0 and $info['encrypted']) {

Thanks for the pointer, will update the code

schiessle · 2015-02-19T16:56:36Z

Instead of passing just the path to calculateUnencryptedSize either pass a stream with the unencrypted content (easy since you're calling it from the wrapper)

This only works if I'm in the wrapper and already perform a operation on the stream, e.g. if I call it from within a wrapped fopen call. But in this case I call it from a filesize() call so I don't have a stream I could pass on.

or pass the unwrapped storage and the internalpath.

You mean that I could call fopen on $this->storage within the storage wrapper which then would bypass the encryption wrapper because $this->storage will no longer be wrapped? Sound like a good solution in this case.

Although, if you look at the current code I fear that we might also have situations where we want to read the file from outside of the storage wrapper. In this case this trick will no longer work. For example if the encryption module want to recalculate the unencrypted size which at the moment rely on the possibility to read the raw file, but maybe @th3fallen will find a way to rewrite this method for the new encryption module. 😉

icewind1991 · 2015-02-19T17:00:25Z

But in this case I call it from a filesize() call so I don't have a stream I could pass on.

Just do $fh = $this->storage->fopen($path, 'r')

icewind1991 · 2015-02-19T17:05:53Z

Although, if you look at the current code I fear that we might also have situations where we want to read the file from outside of the storage wrapper

If you need to access the encrypted file from outside the wrapper then there's probably something wrong with the design of the app, the encryption wrapper should be transparent.

schiessle · 2015-02-19T17:21:53Z

If you need to access the encrypted file from outside the wrapper then there's probably something wrong with the design of the app, the encryption wrapper should be transparent.

Let's see how far we come... Maybe we can re-factor this parts to make it no longer necessary. I just want to mention it early in the process that we, and especially @th3fallen who works on the "app-part", are aware of it.

ghost · 2015-02-23T09:13:50Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9840/
Test PASSed.

DeepDiver1975 · 2015-02-23T14:22:46Z

especially @th3fallen who works on the "app-part", are aware of it.

@th3fallen please follow @schiesbn 's approach an open your pr early as well - THX

ghost · 2015-02-23T15:44:24Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9862/
Test PASSed.

th3fallen · 2015-02-23T15:58:10Z

@DeepDiver1975 no problem, when i create the WIP pr for that app should i set it to merge into master or this branch?

DeepDiver1975 · 2015-02-23T16:56:25Z

@DeepDiver1975 no problem, when i create the WIP pr for that app should i set it to merge into master or this branch?

excellent question - general speaking everything should be a PR against master - but in this case you rely on the changes with this branch.

@schiesbn would it make sense to push changes with respect to the encryption app into this branch? Or would you prefer this going into an branch of it's own?

Both has pros and cons - your call - THX

DeepDiver1975 · 2015-02-23T16:56:40Z

@schiesbn looks like you need to rebase 🙊

schiessle · 2015-02-24T14:05:32Z

@DeepDiver1975 @th3fallen as discussed yesterday and as explained by @DeepDiver1975 everything has his pros and cons so I would keep it to @th3fallen to chose what works best for him. We can also work on the same branch we just have to be careful with rebase and force-push than. So maybe it is better to keep it separate for now.

Also keep in mind that the PR we open today doesn't necessarily has to be the branch we merge at the end. We still have the possibility to open a new PR once everything is finished and ready for review if the development branch become to chaotic over time.

th3fallen · 2015-02-24T14:28:00Z

so @schiesbn The decision is on my own branch? Just to be clear

schiessle · 2015-02-24T15:10:10Z

so @schiesbn The decision is on my own branch? Just to be clear

yes, maybe for now it is better to keep it separated so nobody has to fear to mess it up from each other. If we reach the point where we and other can test and review it we can still consider merging it to one PR to make it easier to test/review

ghost · 2015-02-25T12:04:29Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9944/

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git3501811308828787963.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/14376/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/14376/merge^{commit} # timeout=10Checking out Revision cc341328089c8aed7e6ad6a07a69336a30e36924 (refs/remotes/origin/pr/14376/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f cc341328089c8aed7e6ad6a07a69336a30e36924 > git rev-list ee13828e3d2916a52bcae0d33b942feabcc125e8 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 85e530cbd41931f9b4677126fa8cd011fd376c7d to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/9944/ and message: Merged build finished.
Test FAILed.

ghost · 2015-02-25T18:34:33Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9968/

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git9138639046596617379.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/14376/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/14376/merge^{commit} # timeout=10Checking out Revision 3d48f0c7e895693acae1066622d54502251b6705 (refs/remotes/origin/pr/14376/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 3d48f0c7e895693acae1066622d54502251b6705 > git rev-list cc341328089c8aed7e6ad6a07a69336a30e36924 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 278e08b9e0b09a690c96306d6d2436ebde68e3ad to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/9968/ and message: Merged build finished.
Test FAILed.

scrutinizer-notifier · 2015-02-25T21:14:22Z

The inspection completed: 29 new issues, 60 updated code elements

MorrisJobke · 2015-02-26T09:09:50Z

lib/base.php

@@ -640,6 +640,7 @@ public static function init() {
 		self::registerShareHooks();
 		self::registerLogRotate();
 		self::registerLocalAddressBook();
+		self::registerEncryptionWrapper();


Is this really needed to hooked into the core that much? Shouldn't it be possible to hook this in from an app? cc @icewind1991

I don't really like to introduce again more complexity to the base.php and then it's again static code :(

Should work just fine when done from an app

BernhardPosselt · 2015-02-26T12:40:38Z

I dont want to kill the mood but why arent we focusing on client side encryption? This would be:

faster
less error prone (before upload sync encrypt, before download sync decrypt, no hooks or other apps that are relying on this)
actually secure
handle the same usecases as the serverside encryption app

The server would manage the public keys, so sharing will work by simply syncing the pubkeys in the client. For that we could create a small app that functions as a keyserver so you can also host your pubkeys for email on it. Encrypted folders would become readonly in the webinterface, so you cant accidentally upload confidential stuff.

icewind1991 · 2015-02-26T12:52:55Z

impossible to do safely in a browser

BernhardPosselt · 2015-02-26T13:10:29Z

@icewind1991 yep, thats why we should move this to the sync client and just lock encrypted directories in the browser

icewind1991 · 2015-02-26T14:37:34Z

reading files has the same security issues

BernhardPosselt · 2015-02-26T14:42:59Z

Why? The file is encrypted and by default we could also encrypt the filename.

icewind1991 · 2015-02-26T14:46:01Z

Yes, we cant access the file from either the WI or WebDAV which is not something we want to do.

BernhardPosselt · 2015-02-26T15:05:57Z

You can access the file from webdav by using the path which was generated by the sync client. Webdav clients will see only encrypted files which is perfectly fine, things will still continue to work. Anyways, implementation details.

You want to encrypt stuff because it is not safe on the server. Using serverside encryption does not help you in any way. Using it for external storage is a use case which affects maybe 1% of all the people that want to use encryption. For that use case this solution is overkill. If someone wants to use encrypted external storage, let the admin set a global password and pipe it through AES when writing it to the storage: simple implementation, less problems and bugs, no need to fiddle around with sharing, reencrypting, pubkeys, etc.

As for the webinterface: not accessible, not a problem because you chose security. Theres always the possibility to write a small browser plugin (like megaupload handles it) that handles the decryption when downloading for you.

BernhardPosselt · 2015-02-26T17:09:14Z

It may also be a good idea to just push simple AES encryption support into the external storage app so it is clear where it is actually useful and start working on a design document about clientside encryption :)

PVince81 · 2015-03-04T18:48:17Z

@Raydiation please put your ideas here in the relevant ticket #106. Best would be to have a design/concept and a list of tasks/steps/checkboxes breakdown so that interested people might be able to work on it.
Feel free to team up with the guys from the mailing list who said would implement client side encryption (https://mailman.owncloud.org/pipermail/devel/2014-November/000719.html). They might appreciate your help and input.

PVince81 · 2015-03-04T18:50:11Z

Looks like this guy is already on it #106 (comment)

DeepDiver1975 · 2015-03-04T20:05:46Z

I temporily close this PR - any further work has to be done on #14472

MorrisJobke · 2015-06-27T07:03:43Z

@schiesbn @DeepDiver1975 Can this branch be deleted?

schiessle added 2 - Developing app:files_encryption labels Feb 19, 2015

schiessle added this to the 8.1-next milestone Feb 19, 2015

DeepDiver1975 reviewed Feb 19, 2015
View reviewed changes

icewind1991 reviewed Feb 19, 2015
View reviewed changes

schiessle force-pushed the enc_ng branch from a0cf626 to 536f1d6 Compare February 19, 2015 16:37

icewind1991 reviewed Feb 19, 2015
View reviewed changes

schiessle force-pushed the enc_ng branch from d76cd87 to 440b178 Compare February 24, 2015 14:36

th3fallen mentioned this pull request Feb 24, 2015

encryption 2.0 app #14472

Merged

12 tasks

schiessle force-pushed the enc_ng branch 4 times, most recently from 6e27cb4 to 85e530c Compare February 25, 2015 12:02

schiessle force-pushed the enc_ng branch from 85e530c to 278e08b Compare February 25, 2015 18:32

create basic interfaces and wrapper to make encryption more modular

0bc7b27

schiessle force-pushed the enc_ng branch from 278e08b to 0bc7b27 Compare February 25, 2015 20:44

MorrisJobke reviewed Feb 26, 2015
View reviewed changes

DeepDiver1975 closed this Mar 4, 2015

schiessle deleted the enc_ng branch June 29, 2015 09:20

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


		namespace OC\Files\Storage\Wrapper;

		class Encryption extends Wrapper {

encryption 2.0 - early stage #14376

encryption 2.0 - early stage #14376

Conversation

schiessle commented Feb 19, 2015

schiessle commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Feb 19, 2015

icewind1991 commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Feb 19, 2015

icewind1991 commented Feb 19, 2015

schiessle commented Feb 19, 2015

icewind1991 commented Feb 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schiessle commented Feb 19, 2015

icewind1991 commented Feb 19, 2015

icewind1991 commented Feb 19, 2015

schiessle commented Feb 19, 2015

ghost commented Feb 23, 2015

DeepDiver1975 commented Feb 23, 2015

ghost commented Feb 23, 2015

th3fallen commented Feb 23, 2015

DeepDiver1975 commented Feb 23, 2015

DeepDiver1975 commented Feb 23, 2015

schiessle commented Feb 24, 2015

th3fallen commented Feb 24, 2015

schiessle commented Feb 24, 2015

ghost commented Feb 25, 2015

Build result: FAILURE

ghost commented Feb 25, 2015

Build result: FAILURE

scrutinizer-notifier commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BernhardPosselt commented Feb 26, 2015

icewind1991 commented Feb 26, 2015

BernhardPosselt commented Feb 26, 2015

icewind1991 commented Feb 26, 2015

BernhardPosselt commented Feb 26, 2015

icewind1991 commented Feb 26, 2015

BernhardPosselt commented Feb 26, 2015

BernhardPosselt commented Feb 26, 2015

PVince81 commented Mar 4, 2015

PVince81 commented Mar 4, 2015

DeepDiver1975 commented Mar 4, 2015

MorrisJobke commented Jun 27, 2015