Server side checksum verification #26655 #27097

IljaN · 2017-02-07T10:08:18Z

Description

Related Issue

Motivation and Context

How Has This Been Tested?

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.

mention-bot · 2017-02-07T10:08:21Z

@IljaN, thanks for your PR! By analyzing the history of the files in this pull request, we identified @DeepDiver1975, @PVince81 and @butonic to be potential reviewers.

PVince81

Great progress.

Some additional things came to mind while reviewing.

PVince81 · 2017-02-07T20:34:48Z

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

+	 */
+	public function fopen($path, $mode) {
+		$stream = $this->getWrapperStorage()->fopen($path, $mode);
+		if (!self::requiresChecksum($path)) {


I think you also don't need to compute the checksum if there is already a checksum value in oc_filecache and $mode is a read operation. Could save a bit of computing power on download.

PVince81 · 2017-02-07T20:35:58Z

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

+		$memoryStream = fopen('php://memory', 'r+');
+		$checksumStream = \OC\Files\Stream\Checksum::wrap($memoryStream, $path);
+
+		fwrite($checksumStream, $data);


does that really work, does it write the whole data ?
needs to be tested with some external storages, I suspect that there are cases where fwrite would write only 8k blocks or so but I might be wrong

According to the fwrite docs it will write the whole string. You can limit it by a optional length parameter. Which external storages should I test?

Best would be to test with

SFTP

SMB

one of GDrive or Dropbox

In the past we had issues with GDrive because the fread from GDrive didn't always return 8k per read and it messed up the encryption wrapper which was expecting 8k blocks...

according to the docblocks file_put_contents of the Wrapper interface (unlike the native version) only expects strings so this shouldn`t be a Problem, because we will always get a full string. ?

PVince81 · 2017-02-07T20:38:50Z

lib/private/Files/Stream/Checksum.php

+			return null;
+		}
+
+		return self::$checksums[$path];


Now thinking of it, something should probably clear all the checksum values from that static array.

In the case of single PHP requests this isn't a problem. But if someone runs an occ command that reads a million files (maybe something like transfer ownership or decrypt-all), it could happen that old files still have their checksum stored in there.

One way to compensate for this is to use our CappedMemoryCache class which will auto-remove entries when full.

PVince81 · 2017-02-07T20:40:10Z

lib/private/legacy/util.php

@@ -175,6 +175,12 @@ public static function setupFS($user = '') {
 			return $storage;
 		});

+		// install storage checksum wrapper
+		\OC\Files\Filesystem::addStorageWrapper('oc_checksum', function ($mountPoint, $storage) {
+			return new \OC\Files\Storage\Wrapper\Checksum(['storage' => $storage]);


I think we shouldn't install the checksum wrapper if $storage is a shared storage. See below for a check to exclude it.

The reason is that the SharedStorage for a share recipient is itself is just a wrapper around the original Storage of the owner, which itself will already have a checksum storage applied.

Is a !$storage->isLocal() check also required? (like in the other wrapper registrations)

No. The isLocal was specific to the encoding wrapper which only makes sense for external storages.

In the case of checksums, we want it both on local and external storages.

PVince81

Great stuff!

PVince81 · 2017-02-14T17:08:37Z

apps/dav/lib/Connector/Sabre/File.php

+		$expectedChecksum = trim($request->server['HTTP_OC_CHECKSUM']);
+		$computedChecksums = $meta['checksum'];
+
+		return strpos($computedChecksums, $expectedChecksum) !== false;


why no direct equality ?

PVince81 · 2017-02-14T17:09:07Z

apps/dav/lib/Connector/Sabre/File.php

-			}
+			$this->fileView->putFileInfo(
+				$this->path,
+				['checksum' => $partStorage->getMetaData($internalPartPath)['checksum']]


Is there a mechanism to disable checksums ?

Maybe better be safe and make sure getMetaData actually does contain a checksum before putting it ?

PVince81 · 2017-02-14T17:12:04Z

apps/dav/lib/Connector/Sabre/File.php

+
+		foreach ($checksums as $checksum) {
+			// starts with $algo
+			if (substr($checksum, 0, strlen($algo)) === $algo) {


I thought we had another separator on which we could explode here ?

What if the checksum had both algos "ABC" and "ABC1" ?
When querying "ABC" it might match "ABC1" too and deliver a piece of the wrong checksum.

I think it would make it more complicated to follow the code we do explode again on ":" I will just check until the ":" to be save.

PVince81 · 2017-02-14T17:12:52Z

apps/dav/lib/Connector/Sabre/FilesPlugin.php

@@ -335,7 +335,7 @@ public function handleGetProperties(PropFind $propFind, \Sabre\DAV\INode $node)
 			});

 			$propFind->handle(self::CHECKSUMS_PROPERTYNAME, function() use ($node) {
-				$checksum = $node->getChecksum();
+				$checksum = $node->getChecksum('SHA1');


"sha1" up there and "SHA1" here ? maybe decide on one casing.

By the way: are checksum algo names automatically capitalized ? If not then you likely have a bug here.

@PVince81 PHP uses lowercase for algo everywhere but our protocol uses uppercase, the getChecksum method internally uppercases every input anyway to do the internal comparisons.

PVince81 · 2017-02-14T17:14:39Z

apps/files_trashbin/tests/TrashbinTest.php

@@ -268,6 +226,52 @@ public function testExpireOldFilesShared() {
 	}

 	/**
+	 * test expiration of files older then the max storage time defined for the trash
+	 */
+	public function testExpireOldFiles() {


What a weird coincidence. Some of my test was affected by this one in another location.

The reason was that a test disabled the trashbin permanently which made other tests fail. Ref: #27042 (comment)

Anyway, I'm fine leaving this as is to not waste more time with old cruddy tests.

PVince81 · 2017-02-14T17:15:46Z

build/integration/features/webdav-related.feature

+		Given user "user0" exists
+		And file "/chksumtst.txt"  does not exist for user "user0"
+		And user "user0" uploads file with checksum "SHA1:f005ba11" and content "Some Text" to "/chksumtst.txt"
+		Then the HTTP status code should be "400"


Also need such tests with chunking.

Pick one of the chunking tests and add the header.

PVince81 · 2017-02-14T19:12:31Z

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

+	 * @return bool
+	 */
+	private static function requiresChecksum($path, $mode) {
+		return substr($path, 0, 6) === 'files/' &&  $mode !== 'r';


&& mode !== 'rb' (if any app is still using that...)

PVince81 · 2017-02-14T19:13:00Z

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

+		$memoryStream = fopen('php://memory', 'r+');
+		$checksumStream = \OC\Files\Stream\Checksum::wrap($memoryStream, $path);
+
+		fwrite($checksumStream, $data);


PVince81 · 2017-02-14T19:14:13Z

lib/private/Files/Stream/Checksum.php

+	private static $checksums;
+
+
+	public function __construct(array $algos = ['sha1', 'md5', 'adler32']) {


Weird, this shows up red. Does PHP 5.6 support this syntax ?

Yes it does: https://3v4l.org/cJnUO, apparently it is a github bug

PVince81 · 2017-02-14T19:15:08Z

lib/private/Files/Stream/Checksum.php

+class Checksum extends Wrapper {
+
+	/** @var  resource[] */
+ 	private $hashingContexts;


Add PHPDoc, what's a hashing context here ? (I know, but explain to others)

PVince81 · 2017-02-20T11:01:11Z

we also need a test for the checksum from ext storage. Integration tests have a local storage but it contains no default data. Might need to change the setup a bit, can you help here @SergioBertolinSG ? Have an example existing file on local_storage at start, or better: a scenario method to put a file directly there bypassing OC, will be useful for future update detection tests.

SergioBertolinSG · 2017-02-22T11:01:34Z

@PVince81 It is not already available to do this?

Using upload function like here:

https://github.com/owncloud/core/blob/master/build/integration/features/webdav-related.feature#L194

But using /local_storage/

Like in https://github.com/owncloud/core/blob/master/build/integration/features/external-storage.feature#L13

IljaN · 2017-02-22T10:38:51Z

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

@@ -112,10 +110,9 @@ private function getChecksumRequirement($path, $mode) {
 	 */
 	public function onClose() {
 		$cache = $this->getCache();
-		foreach ($this->pathsInCacheWithoutChecksum as $path) {
-			$entry = $cache->get($path);
+		foreach ($this->pathsInCacheWithoutChecksum as $cacheId => $path) {


@PVince81 I removed all checks here, because then we can save a query when getting the cache entry. If we have a race condition the worst thing that will happen is a update which will affect 0 rows.

PVince81 · 2017-02-23T09:21:27Z

@SergioBertolinSG no, because using the Webdav upload will already write a checksum into the table.
What we want is a way to have a file stored/created/existing on the external storage before ownCloud had a chance to scan it to simulate remote changes on external storage (remote as in "not through ownCloud"). Such files would not have a checksum value yet and this PR would compute one during download.

PVince81 · 2017-03-01T15:25:38Z

@IljaN please rebase then use #27227 (comment) to create a file directly on the local_storage, that file will not have a checksum since we bypassed ownCloud.
Then download it. Then check that the second download has the checksum (or use propfind to read it).

PVince81 · 2017-03-03T15:57:39Z

Bad luck or real error ? (sqlite)

16:05:36 1) Test\Files\ViewTest::testLockBasicOperation with data set #4 ('fopen', array('test.txt', 'r'), 'test.txt', 'read', 1, 1, null)
16:05:36 Failed asserting that 1 matches expected null.

SergioBertolinSG · 2017-03-07T12:14:41Z

@PVince81 that one seems to be a real error.

PVince81 · 2017-03-07T13:50:24Z

One last rebase to get past the JS bug ?

Then this is good for merge 👍
Great work, guys!

IljaN · 2017-03-07T15:54:39Z

@PVince81 I think i need to look at is lock_error, it seems to happen at every jenkins run, so probably no bad luck ...

PVince81 · 2017-03-09T14:30:50Z

16:33:33 1) Test\Files\ViewTest::testLockBasicOperation with data set #4 ('fopen', array('test.txt', 'r'), 'test.txt', 'read', 1, 1, null)
16:33:33 Failed asserting that 1 matches expected null.
16:33:33 
16:33:33 /var/lib/jenkins/workspace/owncloud-core_core_PR-27097-3J4F6P6NQZBTIIMSRG746A75T7KO5JXZVBVLSAJJMR5VJCNJZFKQ/tests/lib/Files/ViewTest.php:1822

PVince81 · 2017-03-09T14:36:29Z

Seems you have a stray lock after fopen which is not supposed to be there.

PVince81 · 2017-03-09T14:40:48Z

Disabling all tests except the fopen one solves the problem.

So one of the 3 data provider cases before are causing a side effect.

PVince81 · 2017-03-09T14:42:02Z

Nope. I disabled the wrong fopen one. Having only dataset 4 enabled fails too. At least no cross-test side effects.

PVince81 · 2017-03-09T14:43:29Z

Removing the checksum stream wrapper solves the issue.

Diving deeper...

PVince81 · 2017-03-09T14:56:06Z

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

+	 * @return false|resource
+	 */
+	public function fopen($path, $mode) {
+		$stream = $this->getWrapperStorage()->fopen($path, $mode);


please check if $stream is a resource, else return it.
add a unit test for the case where fopen failed. Maybe simply $view->fopen() of a non-existing file and check result, that might be enough to cover all possible registered wrappers already.

PVince81 · 2017-03-09T15:33:28Z

Fixed locking issue, it was a hidden testing issue that got revealed by checksum wrapper bug.

Fixes here: fdd2478 9c4a065

@IljaN please review/approve my two commits

IljaN · 2017-03-09T17:39:07Z

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

@@ -55,6 +55,11 @@ class Checksum extends Wrapper {
 	 */
 	public function fopen($path, $mode) {
 		$stream = $this->getWrapperStorage()->fopen($path, $mode);
+		if (!is_resource($stream)) {
+			// don't wrap on error
+			return $stream;


Maybe we should fail more loudly here (Exception?) Or else we might have silent bugs, what do you think?

The rest looks good to me.

No... silent bugs is the "norm" so far for these APIs. If we throw exceptions it risks breaking other things. One day we'll replace all these PHP-like APIs with proper APIs that throw exceptions: #13792

IljaN · 2017-03-09T18:45:43Z

The checksum behat tests all pass locally :-/

…eOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655

…me weird sideeffects if other tests are running before it, couldn`t find out the root cause, test runs ok in isolation.

…hecksum test

…26655

PVince81 · 2017-03-09T19:03:40Z

Rebased onto master. Bring a bag full of luck !

PVince81 · 2017-03-10T07:11:42Z

@IljaN @SergioBertolinSG legit failures:

22:05:29     /var/lib/jenkins/workspace/owncloud-core_core_PR-27097-3J4F6P6NQZBTIIMSRG746A75T7KO5JXZVBVLSAJJMR5VJCNJZFKQ/tests/integration/features/webdav-related.feature:410
22:05:29     /var/lib/jenkins/workspace/owncloud-core_core_PR-27097-3J4F6P6NQZBTIIMSRG746A75T7KO5JXZVBVLSAJJMR5VJCNJZFKQ/tests/integration/features/webdav-related.feature:420

lock · 2019-08-03T19:01:32Z

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.

IljaN mentioned this pull request Feb 7, 2017

Checksum calculating storage wrapper: Initial draft #26655 #27045

Closed

9 tasks

PVince81 reviewed Feb 7, 2017

View reviewed changes

IljaN added the 3 - To Review label Feb 14, 2017

PVince81 added this to the 10.0 milestone Feb 14, 2017

PVince81 reviewed Feb 14, 2017

View reviewed changes

IljaN commented Feb 22, 2017

View reviewed changes

SergioBertolinSG mentioned this pull request Feb 23, 2017

Added a file to local storage for checksum tests #27227

Merged

SergioBertolinSG force-pushed the 26655_ComputeChecksum2 branch from e4bb97d to 7734f0b Compare March 2, 2017 13:27

SergioBertolinSG force-pushed the 26655_ComputeChecksum2 branch from eb22802 to b5617a3 Compare March 6, 2017 14:01

SergioBertolinSG force-pushed the 26655_ComputeChecksum2 branch from e2c80e9 to ff6b908 Compare March 7, 2017 15:30

SergioBertolinSG mentioned this pull request Mar 9, 2017

Adding integration tests to checksums #27345

Merged

PVince81 reviewed Mar 9, 2017

View reviewed changes

IljaN commented Mar 9, 2017

View reviewed changes

Server side checksum verification #26655

241a172

IljaN and others added 19 commits March 9, 2017 20:03

Fixing failing testExpireOldFilesShared by puting it before testExpir…

395de64

…eOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655

Checksum only on write #26655

2a6b626

strong comparison

d6ba656

Reverting accidentally commited testcode #26655

18c632e

Fixing basic lock test fail #26655

fc780db

#26655 Moved MoveFileIntoSharedFolderAsRecepient up because it has so…

5e2ca3c

…me weird sideeffects if other tests are running before it, couldn`t find out the root cause, test runs ok in isolation.

Adding checksum download test #26655

4cb2c05

Multiple checksums support + tests#26655

b985261

#26655 eventual mimetype error fix

4dd7a11

#26655 Webdav returns only one checksum (SHA1)

05e11ce

#26655 Various smaller improvements, additional upload chunked with c…

63495df

…hecksum test

#26655 strpos bugfix

6bd3aaf

Files without checksum in oc_filecache get a checksum on first read #…

0721f31

…26655

Revert strpos fix #26655

2caae3e

Caching of file id, and various small fixes #26655

d8bd987

Added test about adding a file to local storage

0e68f69

Checksum was not well calculated

c826ea3

Don't wrap failed fopen in checksum wrapper

d7091b5

Fix locking tests in ViewTest

eeb8f07

PVince81 force-pushed the 26655_ComputeChecksum2 branch from 9c4a065 to eeb8f07 Compare March 9, 2017 19:03

IljaN self-assigned this Mar 10, 2017

SergioBertolinSG and others added 2 commits March 10, 2017 09:05

Checksums tests removed from webdav-feature

5f18c5f

Fixed wrong strpos condition

0496a82

PVince81 merged commit c9f5fce into master Mar 10, 2017

PVince81 deleted the 26655_ComputeChecksum2 branch March 10, 2017 11:34

MorrisJobke mentioned this pull request Mar 23, 2017

Server side checksum verification nextcloud/server#4023

Closed

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

		private static $checksums;


		public function __construct(array $algos = ['sha1', 'md5', 'adler32']) {

Server side checksum verification #26655 #27097

Server side checksum verification #26655 #27097

Conversation

IljaN commented Feb 7, 2017 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

mention-bot commented Feb 7, 2017

PVince81 left a 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

IljaN Feb 8, 2017 • edited Loading

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 left a 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

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 Feb 20, 2017

SergioBertolinSG commented Feb 22, 2017

Choose a reason for hiding this comment

PVince81 commented Feb 23, 2017 • edited Loading

PVince81 commented Mar 1, 2017

PVince81 commented Mar 3, 2017

SergioBertolinSG commented Mar 7, 2017

PVince81 commented Mar 7, 2017

IljaN commented Mar 7, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Mar 9, 2017

Choose a reason for hiding this comment

PVince81 commented Mar 9, 2017

IljaN Mar 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IljaN commented Mar 9, 2017

PVince81 commented Mar 9, 2017

PVince81 commented Mar 10, 2017

lock bot commented Aug 3, 2019

IljaN commented Feb 7, 2017 •

edited

Loading

IljaN Feb 8, 2017 •

edited

Loading

PVince81 commented Feb 23, 2017 •

edited

Loading

IljaN Mar 9, 2017 •

edited

Loading