Transfer ownership work with masterkey #28107

sharidas · 2017-06-08T15:43:44Z

This change helps users to get transfer-ownership
command work when masterkey is enabled as mode
of encryption.

Signed-off-by: Sujith H sharidasan@owncloud.com

Description

This change will help users to transfer files using transfer-ownership command when masterkey mode is enabled for encryption.

Related Issue

#27427

Motivation and Context

This change would help user tranfer files between users using transfer-ownership command when masterkey mode is enabled for encryption.

How Has This Been Tested?

Below are the steps executed for testing:
`sujith@sujith-Inspiron-5567 ~/test/owncloud $ ./occ files:transfer-ownership --path transferred\ from\ user1\ on\ 20170608_115804/1 admin user1
Cannot load Xdebug - it was already loaded
Analysing files of admin ...
1 [->--------------------------]
Check if master key enabled 1
1 [============================]
Collecting all share information for files and folder of admin ...
0 [>---------------------------]
Transferring files to user1/files/transferred from admin on 20170608_032359 ...
Restoring shares ...
0 [>---------------------------]
sujith@sujith-Inspiron-5567 ~/test/owncloud $ ./occ files:transfer-ownership --path transferred\ from\ admin\ on\ 20170608_032359/1 user1 admin
Cannot load Xdebug - it was already loaded
Analysing files of user1 ...
1 [============================]
Collecting all share information for files and folder of user1 ...
0 [>---------------------------]
Transferring files to admin/files/transferred from user1 on 20170608_033618 ...
Restoring shares ...
0 [>---------------------------]
sujith@sujith-Inspiron-5567 ~/test/owncloud $

This has also been tested by running the test transfer-ownership.feature`

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

sharidas · 2017-06-11T07:39:38Z

lib/private/Files/Filesystem.php

@@ -944,6 +944,9 @@ public static function getDirectoryContent($directory, $mimetype_filter = '') {
 	 * @return string
 	 */
 	public static function getPath($id) {
+		if (self::$defaultInstance === null) {
+			throw new NotFoundException("defaultInstance is null");


This is one solution I found to get rid of the fatal error thrown by symfony. Any suggestions how to improve this?

thrown by symfony.

when exactly is this happening ? when running CI ?

when exactly is this happening ? when running CI ?

When integration test is executed.

sharidas · 2017-06-12T05:20:07Z

Attaching the integration-test result :
integration-test-result.txt

PVince81 · 2017-06-19T16:29:46Z

apps/encryption/lib/Crypto/Crypt.php

@@ -480,7 +480,9 @@ public function symmetricDecryptFileContent($keyFileContents, $passPhrase, $ciph
 	private function checkSignature($data, $passPhrase, $expectedSignature) {
 		$signature = $this->createSignature($data, $passPhrase);
 		if (!hash_equals($expectedSignature, $signature)) {
-			throw new HintException('Bad Signature', $this->l->t('Bad Signature'));
+			if (\OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', 0) === 0) {


As far as I know the master key should still use signatures to detect when encrypted files have been tampered with.

Last time there was another bug that led to "Bad signature" which was because something in the encryption code wasn't properly initialized. I suggest you verify that.

You can read from here #27261 (comment).
My solution was here #27261 (comment) but will probably not apply. Still, having a look there and at my PR might give some potential clues.

PVince81 · 2017-06-19T16:31:52Z

apps/files/lib/Command/TransferOwnership.php

@@ -194,6 +195,10 @@ function (FileInfo $fileInfo) use ($progress, $self) {
 					$progress->advance();
 					$this->allFiles[] = $fileInfo;
 					if ($fileInfo->isEncrypted()) {
+
+						if (\OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', 0) !== 0) {


why not add it to the array if it's encrypted with master key ? answer in a PHPDoc to de-confuse future devs

PVince81 · 2017-06-19T16:32:18Z

apps/files/lib/Command/TransferOwnership.php

+					try {
+						$this->shareManager->deleteShare($share);
+					} catch (\OCP\Files\NotFoundException $e) {
+					}


add error handling or a comment why we don't handle / ignore

sharidas · 2017-06-20T06:27:24Z

Updated the PR by:

Reverted the change made in apps/encryption/lib/Crypto/Crypt.php
Added phpdoc for the exception in TransferOwnership.php
Tried to write the message to log file when the exception is called. Rather than making it empty
Added phpdoc explaining why the masterkey encryption doesn't need to be added to the array in TransferOwnership.php

PVince81 · 2017-06-21T09:03:50Z

apps/files/lib/Command/TransferOwnership.php

+						 * master key. The method getPath from \OC\Files\Filesystem tries
+						 * to call getPath using defaultInstance variable which
+						 * is null. Hence this causes our integration tests to break.
+						 */


Thanks for the explanation. I think we shouldn't catch and ignore an exception like this just because of integration tests.

I was fine with your other workaround in the other method when instance is null but here I think it's a bit too much.
Do you know which test fails ? Maybe there's a setupFS missing somewhere to initialize that default instance ?

From what I see the defaultInstance is created when calling Filesystem::init().
Can you debug and compare why that one is properly set when running OCC manually and not when running integration tests ? There shouldn't be any difference...

I have updated the PR. This change is not required :( Hence I have removed the code in the latest PR. Here is the output of : OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED=1 ./run.sh features/transfer-ownership.feature &> out.txt ( the out.txt :
out1.txt )

PVince81 · 2017-06-21T15:22:26Z

apps/files/lib/Command/TransferOwnership.php

@@ -194,6 +195,14 @@ function (FileInfo $fileInfo) use ($progress, $self) {
 					$progress->advance();
 					$this->allFiles[] = $fileInfo;
 					if ($fileInfo->isEncrypted()) {
+						if (\OC::$server->getAppConfig()->getValue('encryption', 'useMasterKey', 0) !== 0) {


Note that in general the transfer ownership app isn't supposed to know about specific encryption implementations.
Some people might implement their own custom encryption schemes, so would need a better abstraction.

But as we likely don't have time for better abstractions let's merge this.

PVince81

👍

sharidas · 2017-07-03T08:10:37Z

Updated the PR and the verified the transfer-ownership.feature on my machine:
log.txt

sharidas · 2017-07-03T08:13:03Z

lib/private/Files/Storage/Wrapper/Encryption.php

+			 * version will be 1 and the expected version is at 2. And this results in Bad
+			 * Signature. This change helps to fix the issue.
+			 */
+			if ($encryptedVersion === 2) {


Not able to get any better solution. As per my understanding this version is causing "Bad Signature". The problem is when the expected signature is for version 2 and the signature generated is for version 1.

Please try creating and overwriting a file multiple times before the transfer. You'll see that the value of encrypted in the database can be higher, so this hard-coded 2 likely will not work.

I also suggest to add this as an additional integration test ^

sharidas · 2017-07-03T09:19:48Z

lib/private/Files/Storage/Wrapper/Encryption.php

+			 * Instead of the reusing old version, we stick with version 1. This helps
+			 * the data viewed in UI and helps resolve "Bad Signature".
+			 */
+			$encryptedVersion = 1;


Yes, that was right. When the data is updated in the file multiple times or file is edited multiple times, old logic fails. So if we make the version 1, then it looks like the data doesn't hit "Bad Signature".

the "Bad signature" was only observed with transfer ownership or also other cases ?

cc @tomneedham

One of my observation is that whenever a new file is created the version is set to 0. So when transfer:ownership is called, altogether a new file is being created. Which is resulting in version to set to zero: https://github.com/owncloud/core/blob/master/lib/private/Files/Storage/Wrapper/Encryption.php#L743. Whenever new file is created the version is set: https://github.com/owncloud/encryption/blob/master/lib/Crypto/Encryption.php#L192. Need to figure out how to use the sourceInternalPath's version assigned to targetInternalPath.

Did another experiment where I had 2 instances community edition(CE) and EE. Shared a folder of EE with CE. Now without my reset change, tried to create a file and moved to EE folder. It caused "Bad Signature". With my change enabled, created another file and moved to same EE folder. I was able to open the text file and read the contents. Moved the working file back to CE. No BadSignature was observed.

PVince81 · 2017-07-10T11:25:23Z

@sharidas strange failures, I suggest rebasing

sharidas · 2017-07-10T11:47:23Z

Rebased the branch with current master.

sharidas · 2017-07-11T13:29:20Z

It looks like the transfer-ownership.feature is run as ./run.sh features/transfer-ownership.feature. Which causes issue when run on the terminal. Where as if ran as OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED=1 ./run.sh features/transfer-ownership.feature doesn't cause any issue. Which also means need further investigation to get it working without setting OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED.

PVince81 · 2017-07-13T09:47:29Z

failing tests, please have a look

sharidas · 2017-07-17T17:17:20Z

The test cases were passing without encryption:
log.txt The file generated using command sujith@sujith-Inspiron-5567 ~/test/owncloud/tests/integration $ ./run.sh features/transfer-ownership.feature &> /tmp/log.txt
and with encryption masterkey:
encrypt.txt The file generated using command : sujith@sujith-Inspiron-5567 ~/test/owncloud/tests/integration $ OC_TEST_ENCRYPTION_MASTER_KEY_ENABLED=1 ./run.sh features/transfer-ownership.feature &> /tmp/encrypt.txt

sharidas · 2017-07-27T12:13:01Z

lib/private/Files/Storage/Wrapper/Encryption.php

+				if($isRename) {
+					$absSourcePath = Filesystem::normalizePath($sourceStorage->getOwner($sourceInternalPath). '/' . $sourceInternalPath);
+					$absTargetPath = Filesystem::normalizePath($this->getFullPath($targetInternalPath));
+					\OC::$server->getSession()->set('renameFile',[$absSourcePath, $absTargetPath]);


One mechanism adopted to give a hint that rename action is taking place.

And associated changes here: owncloud/encryption#8

sharidas · 2017-07-27T12:14:58Z

lib/private/Files/Storage/Wrapper/Encryption.php

+			 * Instead of the reusing old version, we stick with version 1. This helps
+			 * the data viewed in UI and helps resolve "Bad Signature".
+			 */
+			$cacheInformation['encryptedVersion'] += 1;


No reset to 1. Instead increment the version.

sharidas · 2017-07-27T12:19:25Z

This change works with transfer-ownership. The transfer of files between external storage found problematic :(

PVince81 · 2017-07-28T09:29:10Z

lib/public/Encryption/IEncryptionModule.php

 	 *
 	 * $return array $header contain data as key-value pairs which should be
 	 *                       written to the header, in case of a write operation
 	 *                       or if no additional data is needed return a empty array
 	 * @since 8.1.0
 	 */
-	public function begin($path, $user, $mode, array $header, array $accessList);
+	public function begin($path, $user, $mode, array $header, array $accessList, $renameFile);


since you are using a string instead of bool I suggest renaming this everywhere to $sourceFileOfRename = null.

This change helps users to get transfer-ownership command work when masterkey is enabled as mode of encryption. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas · 2017-07-28T14:19:57Z

lib/private/Files/Storage/Wrapper/Encryption.php

+			 * Rename is a process of creating a new file. Here we try to use the
+			 * incremented version of source file, for the destination file.
+			 */
+			$encryptedVersion = $sourceStorage->getCache()->get($sourceInternalPath)['encryptedVersion'];


Modified this change. Trivial change. There was failure in couple of tests : apps/files_trashbin/tests/StorageTest.php and apps/files_trashbin/tests/TrashbinTest.php

PVince81 · 2017-07-31T06:45:27Z

@sharidas please backport to stable10

lock · 2019-08-03T00:02:12Z

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.

sharidas added the 2 - Developing label Jun 8, 2017

sharidas force-pushed the transfer-ownership-masterkey-task branch from e8b352c to cd0b99a Compare June 9, 2017 07:55

sharidas changed the title ~~Transfer ownership work with masterkey~~ [WIP] Transfer ownership work with masterkey Jun 9, 2017

sharidas force-pushed the transfer-ownership-masterkey-task branch from cd0b99a to fdaae0a Compare June 11, 2017 07:36

sharidas changed the title ~~[WIP] Transfer ownership work with masterkey~~ Transfer ownership work with masterkey Jun 11, 2017

sharidas added the 3 - To Review label Jun 11, 2017

sharidas requested review from tomneedham, butonic, PVince81 and DeepDiver1975 June 11, 2017 07:37

sharidas commented Jun 11, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch from fdaae0a to 432b6bb Compare June 11, 2017 08:20

PVince81 reviewed Jun 19, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch 2 times, most recently from 172f754 to f032e9f Compare June 20, 2017 06:22

PVince81 reviewed Jun 21, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch from f032e9f to 2c38732 Compare June 21, 2017 14:52

PVince81 reviewed Jun 21, 2017

View reviewed changes

PVince81 approved these changes Jun 21, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch 2 times, most recently from 0b55ee0 to 1fb8a88 Compare July 3, 2017 07:43

sharidas commented Jul 3, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch from 1fb8a88 to 1a717b4 Compare July 3, 2017 09:16

sharidas commented Jul 3, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch from 1a717b4 to b985093 Compare July 4, 2017 06:13

sharidas force-pushed the transfer-ownership-masterkey-task branch from e79cc5f to 5bd426b Compare July 10, 2017 11:46

sharidas force-pushed the transfer-ownership-masterkey-task branch from 5bd426b to b8465f2 Compare July 17, 2017 17:07

sharidas force-pushed the transfer-ownership-masterkey-task branch 4 times, most recently from 83f79ad to 33db0ff Compare July 24, 2017 06:07

sharidas force-pushed the transfer-ownership-masterkey-task branch from 33db0ff to 2fa1d6a Compare July 27, 2017 12:04

sharidas commented Jul 27, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch from 2fa1d6a to bb7d4c7 Compare July 28, 2017 04:56

sharidas mentioned this pull request Jul 28, 2017

Change required for move operation owncloud/encryption#8

Merged

PVince81 reviewed Jul 28, 2017

View reviewed changes

sharidas force-pushed the transfer-ownership-masterkey-task branch 3 times, most recently from efaa8fd to 83e8467 Compare July 28, 2017 13:40

Transfer ownership work with masterkey

9602182

This change helps users to get transfer-ownership command work when masterkey is enabled as mode of encryption. Signed-off-by: Sujith H <sharidasan@owncloud.com>

sharidas force-pushed the transfer-ownership-masterkey-task branch from 83e8467 to 9602182 Compare July 28, 2017 14:17

sharidas commented Jul 28, 2017

View reviewed changes

sharidas merged commit 082eb95 into master Jul 28, 2017

PVince81 deleted the transfer-ownership-masterkey-task branch July 31, 2017 06:45

sharidas mentioned this pull request Jul 31, 2017

[stable10] Transfer ownership work with masterkey #28537

Merged

16 tasks

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer ownership work with masterkey #28107

Transfer ownership work with masterkey #28107

sharidas commented Jun 8, 2017 •

edited

Loading

sharidas Jun 11, 2017

PVince81 Jun 19, 2017

sharidas Jun 20, 2017 •

edited

Loading

sharidas commented Jun 12, 2017

PVince81 Jun 19, 2017

PVince81 Jun 19, 2017

PVince81 Jun 19, 2017

PVince81 Jun 19, 2017

sharidas commented Jun 20, 2017

PVince81 Jun 21, 2017

sharidas Jun 21, 2017

PVince81 Jun 21, 2017

PVince81 left a comment

sharidas commented Jul 3, 2017

sharidas Jul 3, 2017

PVince81 Jul 3, 2017

PVince81 Jul 3, 2017

sharidas Jul 3, 2017

PVince81 Jul 25, 2017

sharidas Jul 26, 2017 •

edited

Loading

sharidas Jul 26, 2017

PVince81 commented Jul 10, 2017

sharidas commented Jul 10, 2017

sharidas commented Jul 11, 2017

PVince81 commented Jul 13, 2017

sharidas commented Jul 17, 2017

sharidas Jul 27, 2017

sharidas Jul 27, 2017

sharidas Jul 27, 2017 •

edited

Loading

sharidas commented Jul 27, 2017 •

edited

Loading

PVince81 Jul 28, 2017

sharidas Jul 28, 2017

PVince81 commented Jul 31, 2017

lock bot commented Aug 3, 2019

Transfer ownership work with masterkey #28107

Transfer ownership work with masterkey #28107

Conversation

sharidas commented Jun 8, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Jun 20, 2017 • edited Loading

Choose a reason for hiding this comment

sharidas commented Jun 12, 2017

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

sharidas commented Jun 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 left a comment

Choose a reason for hiding this comment

sharidas commented Jul 3, 2017

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

sharidas Jul 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 10, 2017

sharidas commented Jul 10, 2017

sharidas commented Jul 11, 2017

PVince81 commented Jul 13, 2017

sharidas commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

sharidas commented Jul 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Jul 31, 2017

lock bot commented Aug 3, 2019

sharidas commented Jun 8, 2017 •

edited

Loading

sharidas Jun 20, 2017 •

edited

Loading

sharidas Jul 26, 2017 •

edited

Loading

sharidas Jul 27, 2017 •

edited

Loading

sharidas commented Jul 27, 2017 •

edited

Loading