initial checksum support for ocis #1400

butonic · 2021-01-14T15:56:13Z

TL;dr

Long version

The ocis driver now calculates the sha1, md4 and adler32 checksums when finalizing uploads. While this adds significant io because every file that was written has to be read it ensures that the hashes we store in the metadata reflect what was wraitten to the storage. We may improve performance in two ways:

run a go routine to calculate the hashas asynchronously
for uploads that are finished within one request calculate the hash when writing the file (although this would miss the point of the checksum, but this is what we do in oc10)

For chunked uploads this already does the right thing: read the completed file from disk and calculate the hashes.

oc10 supports multiple checksums for a file, even if the oc10 db contains only one checksum column and the propfind returns sth like

        <oc:checksums>
          <oc:checksum>SHA1:941f6bb205d2e9f MD5:aeoaeoaeoaeoaeo ADLER32:iaeoioeao</oc:checksum>
        </oc:checksums>

that is a multi value tag badly encoded as a single string / xml tag value. 🤦 ... legacy reasons ... 😭

Anyway, the cs3 Resource info can only transport one Checksum. @ishank011 @labkode We should let it hold multiple checksums.

History:
introduced with owncloud/core#22199 ... here the idea still was:

<oc:checksums>
  <oc:checksum>TYPE1:CHECKSUM1</oc:checksum>
  <oc:checksum>TYPE2:CHECKSUM2</oc:checksum>
</oc:checksum>

no idea where it broke. tried the related oc10 issues:
owncloud/core#18716
owncloud/client#3735
owncloud/ios-legacy#719
owncloud/core#22711
owncloud/core#11811
owncloud-archive/documentation#2964

I only learned that the desktop client onld reads and sends the OC-Checksum header.

Will continue digging tomorrow...

update-docs · 2021-01-14T15:56:17Z

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

butonic · 2021-01-14T16:15:24Z

todo:


runsh: Total unexpected failed scenarios throughout the test run:
--
983 | apiOcisSpecific/apiMain-checksums.feature:18
984 | apiOcisSpecific/apiMain-checksums.feature:19
985 | apiOcisSpecific/apiMain-checksums.feature:23
986 | apiOcisSpecific/apiMain-checksums.feature:61
987 | apiOcisSpecific/apiMain-checksums.feature:62

butonic · 2021-01-15T15:43:37Z

The original pr that should have mades checksums more future proof expects a comma separated list of checksums. see owncloud/core#21997 (comment) for the first mention of commas.

PR to fix this is owncloud core is: owncloud/core#38304 with an analysis of what went wrong ...

internal/http/services/owncloud/ocdav/propfind.go

internal/http/services/owncloud/ocdav/put.go

pkg/storage/fs/ocis/node.go

pkg/storage/fs/ocis/ocis.go

pkg/storage/fs/ocis/upload.go

butonic · 2021-01-18T20:22:59Z

k, this fixes 22 checksum features. the remaining failures:


/drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:58 | 70s
-- | --
1264 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:67 | 70s
1265 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:119 | 70s
1266 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:129 | 70s
1267 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:138 | 70s
1268 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:147 | 70s
1269 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:158 | 70s
1270 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:174 | 70s
1271 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:192 | 70s
1272 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:258 | 70s
1273 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:312 | 70s
1274 | /drone/src/tmp/testrunner/tests/acceptance/features/apiMain/checksums.feature:324

are related to chunked upload or sharing and subject of a subsequent PR.

labkode · 2021-01-19T08:40:47Z

@butonic is this compatible with existing sync clients?

butonic · 2021-01-19T08:45:51Z

@labkode yes. I made sure that the rendering, as weird as it looks, matches what clients expect. Unfortunately, the desktop client would not properly handle correct xml like

<oc:checksums>
  <oc:checksum>TYPE1:CHECKSUM1</oc:checksum>
  <oc:checksum>TYPE2:CHECKSUM2</oc:checksum>
</oc:checksum>

so I implemented this bug compatible with oc10 🤷‍♂️ owncloud/core#38304 contains the fix as well as discussion on what iOS and the desktop would do.

refs · 2021-01-19T16:23:13Z

@butonic while trying this out I get tons of errors:

even after deleting everything under /var/tmp/ocis

refs · 2021-01-19T16:31:20Z

I also don't see the <oc:checksum> header on the response XML:

<?xml version="1.0" encoding="UTF-8"?>
<d:multistatus xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns" xmlns:s="http://sabredav.org/ns">
   <d:response>
      <d:href>/remote.php/webdav/</d:href>
      <d:propstat>
         <d:prop>
            <oc:permissions>DNVCKR</oc:permissions>
            <oc:favorite>0</oc:favorite>
            <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OmZhOGU1MjYyLTZhNjMtNDUzNS1hNzY4LTk0NzI1NDU3MzRiNw==</oc:fileid>
            <oc:owner-id>admin</oc:owner-id>
            <oc:owner-display-name>Admin</oc:owner-display-name>
            <oc:size>96</oc:size>
            <d:getlastmodified>Tue, 19 Jan 2021 16:23:43 +0000</d:getlastmodified>
            <d:getetag>"41aead0722284a3a739c9138996acd23"</d:getetag>
            <d:resourcetype>
               <d:collection />
            </d:resourcetype>
         </d:prop>
         <d:status>HTTP/1.1 200 OK</d:status>
      </d:propstat>
      <d:propstat>
         <d:prop>
            <oc:share-types />
            <oc:privatelink />
            <d:getcontentlength />
         </d:prop>
         <d:status>HTTP/1.1 404 Not Found</d:status>
      </d:propstat>
   </d:response>
   <d:response>
      <d:href>/remote.php/webdav/ownCloud_Admin_Training_Handout.pdf</d:href>
      <d:propstat>
         <d:prop>
            <oc:permissions>DNVWR</oc:permissions>
            <oc:favorite>0</oc:favorite>
            <oc:fileid>MTI4NGQyMzgtYWE5Mi00MmNlLWJkYzQtMGIwMDAwMDA5MTU3OmFiZmViNTAzLTNjMzctNDRkMS05ZjJiLWM2ZWJhMWQ4MWIyNg==</oc:fileid>
            <oc:owner-id>admin</oc:owner-id>
            <oc:owner-display-name>Admin</oc:owner-display-name>
            <d:getcontentlength>395672</d:getcontentlength>
            <oc:size>395672</oc:size>
            <d:getlastmodified>Tue, 19 Jan 2021 16:23:43 +0000</d:getlastmodified>
            <d:getetag>"4958951fc0a36dd05d6e200849f54f94"</d:getetag>
            <d:resourcetype />
         </d:prop>
         <d:status>HTTP/1.1 200 OK</d:status>
      </d:propstat>
      <d:propstat>
         <d:prop>
            <oc:share-types />
            <oc:privatelink />
         </d:prop>
         <d:status>HTTP/1.1 404 Not Found</d:status>
      </d:propstat>
   </d:response>
</d:multistatus>

butonic · 2021-01-19T19:58:12Z

those are for existing files without checksums ;-)
...
or might be folders 🤔

refs · 2021-01-19T20:14:05Z

those are for existing files without checksums ;-)
...
or might be folders 🤔

Hm, they also popup when I delete everything on /var/tmp/ocis/*

refs · 2021-01-19T20:15:11Z

so correct me if I'm wrong but after an upload the uploaded file should have a checksum and this should be included in the PROPFIND request, is that right?

labkode · 2021-01-20T12:25:18Z

@refs https://github.com/cernbox/smashbox/blob/master/protocol/protocol.md#checksumming-extensions

butonic · 2021-01-20T21:14:55Z

@refs the errors are caused by the accounts in the metadata storage ... changing log level for not existing checksums to debug. leaving not found or read errors as error. rebasing

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

butonic · 2021-01-20T21:25:13Z

so correct me if I'm wrong but after an upload the uploaded file should have a checksum and this should be included in the PROPFIND request, is that right?

@refs only if it was requested in the PROPFIND:

curl 'https://demo.owncloud.com/remote.php/webdav/ownCloud Manual.pdf'   -X 'PROPFIND' -d $'<?xml version="1.0"?>
> <d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
>   <d:prop>
>     <oc:checksums />
>   </d:prop>
> </d:propfind>' -k -u demo:demo | xmllint -format -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   593  100   451  100   142   1518    478 --:--:-- --:--:-- --:--:--  1996
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/webdav/ownCloud%20Manual.pdf</d:href>
    <d:propstat>
      <d:prop>
        <oc:checksums>
          <oc:checksum>SHA1:d4c9b536a6d082dcef9b96a9956986bd3472196f MD5:7dfd959493b7680b5a9e6dae7e2912f8 ADLER32:f7d46dfc</oc:checksum>
        </oc:checksums>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

<oc:checksums /> is the magic key

refs · 2021-01-21T10:07:42Z

so correct me if I'm wrong but after an upload, the uploaded file should have a checksum and this should be included in the PROPFIND request, is that right?

@refs only if it was requested in the PROPFIND:

curl 'https://demo.owncloud.com/remote.php/webdav/ownCloud Manual.pdf'   -X 'PROPFIND' -d $'<?xml version="1.0"?>
> <d:propfind  xmlns:d="DAV:" xmlns:oc="http://owncloud.org/ns">
>   <d:prop>
>     <oc:checksums />
>   </d:prop>
> </d:propfind>' -k -u demo:demo | xmllint -format -
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100   593  100   451  100   142   1518    478 --:--:-- --:--:-- --:--:--  1996
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:oc="http://owncloud.org/ns">
  <d:response>
    <d:href>/remote.php/webdav/ownCloud%20Manual.pdf</d:href>
    <d:propstat>
      <d:prop>
        <oc:checksums>
          <oc:checksum>SHA1:d4c9b536a6d082dcef9b96a9956986bd3472196f MD5:7dfd959493b7680b5a9e6dae7e2912f8 ADLER32:f7d46dfc</oc:checksum>
        </oc:checksums>
      </d:prop>
      <d:status>HTTP/1.1 200 OK</d:status>
    </d:propstat>
  </d:response>
</d:multistatus>

<oc:checksums /> is the magic key

oooooh I see. Shows I'm still fresh flesh with the WebDAV spec. Thanks for explaining.

ishank011

@butonic mostly looks good. A few minor comments. And totally agree, we should return multiple checksums in ResourceInfo and not rely on storing those in the opaque map.

internal/http/services/owncloud/ocdav/head.go

internal/http/services/owncloud/ocdav/propfind.go

ishank011 · 2021-01-22T11:02:28Z

pkg/storage/fs/ocis/upload.go

+ }
+
+ // compare if they match the sent checksum
+ // TODO the tus checksum extension would do this on every chunk, but I currently don't see an easy way to pass in the requested checksum. for now we do it in FinishUpload which is also called for chunked uploads


Would the client send the checksum only with the last chunk or with all of those?

it would send the checksum for a PATCH request. without the chunking extension that is the complete file. with the chunking extension it would be the checksum of the chunk.

But when FinishUpload is called, binPath would have the complete file, right? In that case, the last chunk request would need to have the checksum of the whole file, otherwise, the asserts would fail.

The metadata["checksum"] is set in InitiateUpload(), which will store it in the info file. So the checksum (if one was sent) is available in FinishUpload(). Even if it was not requested, we still need to store it in the metadata, which is why we have to split checking them from persisting them. The latter of which has to happen always.

pkg/storage/fs/ocis/upload.go

Signed-off-by: Jörn Friedrich Dreyer <jfd@butonic.de>

phil-davis · 2021-01-25T11:49:57Z

🚀 a list of expected-failures were removed - great stuff.

butonic requested a review from labkode as a code owner January 14, 2021 15:56

butonic marked this pull request as draft January 14, 2021 15:56

butonic force-pushed the checksums branch from 1c97880 to c1c1b64 Compare January 18, 2021 13:00