New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Server computes checksum on read/write file operations #26655

Closed
PVince81 opened this Issue Nov 18, 2016 · 12 comments

Comments

Projects
None yet
5 participants
@PVince81
Member

PVince81 commented Nov 18, 2016

Old ticket: #11811

We already have a column "oc_filecache.checksums" in the database.

Tasks

  • at download time, if no checksum exists in DB, the server computes checksum on the fly and stores it in the database
    • this covers external storages somehow, but will lack a checksum for the first download)
  • at upload time, if client provides a checksum, compute on the fly and verify against the given value
    • client side
    • checksum check in chunking ng: #26582
    • checksum check in reguilar Webdav PUT
    • store computed checksum in DB even if no checksum given by client
  • compute and store checksum in database when using fopen in write mode

Open questions:

  • always use SHA1 everywhere ?
  • what if a client sends MD5 checksum, do we then compute both MD5 and SHA1 and use MD5 only for verification and store SHA1 in database ? I'd suggest we limit to only SHA1 for now.

@PVince81 PVince81 added this to the 9.2 milestone Nov 18, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 18, 2016

Member

@guruz I'm starting to have the feeling that maybe we shouldn't compute the checksum in the AssemblyStream but instead implement a StreamWrapper that will compute the checksum.
The stream wrapper is then applied for any operation that involves fopen, file_put_contents to make sure that even apps using file APIs will have their file receive a checksum.

The advantage of the stream wrapper is that it will cover most code paths, including chunking ng assembly and also download.

@DeepDiver1975

Member

PVince81 commented Nov 18, 2016

@guruz I'm starting to have the feeling that maybe we shouldn't compute the checksum in the AssemblyStream but instead implement a StreamWrapper that will compute the checksum.
The stream wrapper is then applied for any operation that involves fopen, file_put_contents to make sure that even apps using file APIs will have their file receive a checksum.

The advantage of the stream wrapper is that it will cover most code paths, including chunking ng assembly and also download.

@DeepDiver1975

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Nov 21, 2016

Contributor

Do you mean stream wrapper or stream filter?
Something like https://github.com/bantuXorg/php-stream-filter-hash ? FYI @bantu

Contributor

guruz commented Nov 21, 2016

Do you mean stream wrapper or stream filter?
Something like https://github.com/bantuXorg/php-stream-filter-hash ? FYI @bantu

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Nov 21, 2016

Member

@guruz I'm only familiar with stream wrappers, but if a stream filter does the same then I'm fine with it.

Member

PVince81 commented Nov 21, 2016

@guruz I'm only familiar with stream wrappers, but if a stream filter does the same then I'm fine with it.

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Nov 21, 2016

Member
Member

bantu commented Nov 21, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 3, 2016

Member

@guruz assigning you for now. Let me know if you need guidance or help.

Member

PVince81 commented Dec 3, 2016

@guruz assigning you for now. Let me know if you need guidance or help.

@PVince81 PVince81 changed the title from Server computes checksum on upload/download to Server computes checksum on read/write file operations Jan 26, 2017

@PVince81 PVince81 assigned IljaN and unassigned guruz Jan 26, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 26, 2017

Member

Posting what I said in the chat:

Basically the goal is to try and always have a checksum value available in the matching oc_filecache column.
Currently we only store one if the client sent one as part of an upload header.
Now we also want the server to verify it at upload time (in Sabre\File that you already know from your chunking PR) and also to compute it when a file is streamed (read/write).
But the code itself should not go in Sabre\File because that's already too high level, too specific

@IljaN you might need to use a similar approach to encryption with a storage wrapper + stream wrapper:

@IljaN start debugging encryption here: https://github.com/owncloud/core/blob/master/lib/private/Encryption/Manager.php#L256, then dig into the storage wrapper and see what it does on fopen. Basically a storage wrapper intercepts any FS calls and does things, so it has all regular storage methods. Since a storage wrapper intercepts FS calls, it's likely the best place to add something that needs to act on fopen/file_put_contents.

The encryption storage wrapper sets up a stream wrapper (PHP stream wrapper) on fopen. the stream wrapper encrypts In the case of checksum, the stream wrapper would only compute the checksum and store it in oc_filecache in stream_close or so

For checksum verification, I think this should happen later. First let the stream wrapper compute the checksum of whatever has been read or written, and add code that runs later (need to find a good place) and compares that checksum with the expected one from the header

Let's focus on the computing part first.

This is basically the task item "compute and store checksum in database when using fopen in write mode".

Member

PVince81 commented Jan 26, 2017

Posting what I said in the chat:

Basically the goal is to try and always have a checksum value available in the matching oc_filecache column.
Currently we only store one if the client sent one as part of an upload header.
Now we also want the server to verify it at upload time (in Sabre\File that you already know from your chunking PR) and also to compute it when a file is streamed (read/write).
But the code itself should not go in Sabre\File because that's already too high level, too specific

@IljaN you might need to use a similar approach to encryption with a storage wrapper + stream wrapper:

@IljaN start debugging encryption here: https://github.com/owncloud/core/blob/master/lib/private/Encryption/Manager.php#L256, then dig into the storage wrapper and see what it does on fopen. Basically a storage wrapper intercepts any FS calls and does things, so it has all regular storage methods. Since a storage wrapper intercepts FS calls, it's likely the best place to add something that needs to act on fopen/file_put_contents.

The encryption storage wrapper sets up a stream wrapper (PHP stream wrapper) on fopen. the stream wrapper encrypts In the case of checksum, the stream wrapper would only compute the checksum and store it in oc_filecache in stream_close or so

For checksum verification, I think this should happen later. First let the stream wrapper compute the checksum of whatever has been read or written, and add code that runs later (need to find a good place) and compares that checksum with the expected one from the header

Let's focus on the computing part first.

This is basically the task item "compute and store checksum in database when using fopen in write mode".

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 26, 2017

Member

it should actually be possible to copy-paste the encryption storage + stream wrapper code and adjust accordingly

encryption also writes some data (unencrypted size) to the filecache. so for checksum replace that code to write the checksum instead

so not that much new stuff to invent :-)

Member

PVince81 commented Jan 26, 2017

it should actually be possible to copy-paste the encryption storage + stream wrapper code and adjust accordingly

encryption also writes some data (unencrypted size) to the filecache. so for checksum replace that code to write the checksum instead

so not that much new stuff to invent :-)

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 26, 2017

Member

I'd also suggest writing the computed checksum into the stream_context, this way the caller of fopen would be able to retrieve it directly without having to go to oc_filecache afterwards.

Member

PVince81 commented Jan 26, 2017

I'd also suggest writing the computed checksum into the stream_context, this way the caller of fopen would be able to retrieve it directly without having to go to oc_filecache afterwards.

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Jan 26, 2017

Contributor

Sorry to jump in to hijack @PVince81 's way of thoughts.

I just wanted to re-state that maybe a stream filter would be interesting/easier to do? I'm not a PHP guy and I didn't try it, but it felt better to me than the wrapping orgy we have ;-)

https://github.com/bantuXorg/php-stream-filter-hash/blob/master/src/HashFilter.php

Maybe it doesn't make sense what I write.

Contributor

guruz commented Jan 26, 2017

Sorry to jump in to hijack @PVince81 's way of thoughts.

I just wanted to re-state that maybe a stream filter would be interesting/easier to do? I'm not a PHP guy and I didn't try it, but it felt better to me than the wrapping orgy we have ;-)

https://github.com/bantuXorg/php-stream-filter-hash/blob/master/src/HashFilter.php

Maybe it doesn't make sense what I write.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Jan 26, 2017

Member

Could work as replacement for stream wrapper. But storage wrapper still needed to do the application of stream filter in fopen.

Member

PVince81 commented Jan 26, 2017

Could work as replacement for stream wrapper. But storage wrapper still needed to do the application of stream filter in fopen.

IljaN added a commit to IljaN/core that referenced this issue Jan 27, 2017

IljaN added a commit to IljaN/core that referenced this issue Jan 31, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 1, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 2, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 3, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 3, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 3, 2017

IljaN added a commit to IljaN/core that referenced this issue Feb 3, 2017

@PVince81 PVince81 referenced this issue Feb 6, 2017

Closed

Tests for checksums #375

9 of 9 tasks complete

IljaN added a commit to IljaN/core that referenced this issue Feb 6, 2017

IljaN added a commit that referenced this issue Feb 7, 2017

IljaN added a commit that referenced this issue Feb 7, 2017

IljaN added a commit that referenced this issue Feb 7, 2017

IljaN added a commit that referenced this issue Feb 7, 2017

IljaN added a commit that referenced this issue Feb 7, 2017

IljaN added a commit that referenced this issue Feb 8, 2017

IljaN added a commit that referenced this issue Feb 8, 2017

- adding memory capped cache for paths
- do not register checksum wrapper for #26655

IljaN added a commit that referenced this issue Feb 8, 2017

IljaN added a commit that referenced this issue Feb 9, 2017

Fixing failing testExpireOldFilesShared by puting it before testExpir…
…eOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655

IljaN added a commit that referenced this issue Feb 9, 2017

SergioBertolinSG added a commit that referenced this issue Mar 7, 2017

Fixing failing testExpireOldFilesShared by puting it before testExpir…
…eOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655

SergioBertolinSG added a commit that referenced this issue Mar 7, 2017

SergioBertolinSG added a commit that referenced this issue Mar 7, 2017

#26655 Moved MoveFileIntoSharedFolderAsRecepient up because it has so…
…me weird sideeffects if other tests are running before it, couldn`t find out the root cause, test runs ok in isolation.

SergioBertolinSG added a commit that referenced this issue Mar 7, 2017

SergioBertolinSG added a commit that referenced this issue Mar 7, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

- adding memory capped cache for paths
- do not register checksum wrapper for #26655

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

Fixing failing testExpireOldFilesShared by puting it before testExpir…
…eOldFiles. If testExpireOldFiles runs before testExpireOldFilesShared, the second test fails randomly. #26655

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

#26655 Moved MoveFileIntoSharedFolderAsRecepient up because it has so…
…me weird sideeffects if other tests are running before it, couldn`t find out the root cause, test runs ok in isolation.

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

PVince81 added a commit that referenced this issue Mar 9, 2017

@PVince81 PVince81 closed this in 2caae3e Mar 10, 2017

PVince81 added a commit that referenced this issue Mar 10, 2017

@bantu

This comment has been minimized.

Show comment
Hide comment
@bantu

bantu Mar 10, 2017

Member
Member

bantu commented Mar 10, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
Member

PVince81 commented Mar 10, 2017

@IljaN ^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment