Repository navigation
Checksums on files / reliable up/download #11138
Description
Activity
GitMate.io thinks possibly related issues are #56 (File Drop: Create confirmation / show file checksum), #3475 (question about file count), #6129 (When using the REST API to find out checksums of files, a 404 is returned.), #7629 (Create Folders when moving files), and #1371 (No file-uploading possible at all).
I believe the clients checksums files, not sure if it syncs the checksums and/or if the server verifies them. But that's more meant to check if files are transferred correctly.
This should be done on server side, as even if all clients would compute & upload a checksum, the webUI would be missing and then we cannot use this at all.
- changed the title
[-]Checksums on files[/-][+]Checksums on files / reliable up/download[/+]on Oct 10, 2018 @icewind1991 we talked about this on NC15 meeting.
Do you think this is feasible to get it still in NC15?This should be done on server side, as even if all clients would compute & upload a checksum, the webUI would be missing and then we cannot use this at all.
The big problem with this is that it is nearly impossible to generate the checksums for external storages. We can't do anything there and also updating them in a timely manner is nearly out of scope. For a pure "all data goes through NC" this would be no problem, but the external storages are a real problem.
cc @rullzer @icewind1991 because we discussed quite a lot about this.
FYI the fact that NC does not have this apparently causes owncloud-client to warn when connecting to a nextcloud instance, which is a problem in Debian because there is no nextcloud-client (yet):
https://alioth-lists.debian.net/pipermail/pkg-owncloud-maintainers/2019-January/003498.html
35 remaining items
Ideally the mecanism should rely on a part of the API whose behavior is not customizable, as we don't want any bad app to be able to mess with it.
Well what about apps that e.g. want to offer a feature like converting/compressing images upon upload? Or removing metadata or similar features? AFAIK extensibility should allow this and you likely cannot prevent them to do anything bad, you need to trust the apps installed.
The point of this mechanism is to ensure the transmission process is graceful and error-free. Whatever happens afterward, such as automated or manual editing of media stored within Nextcloud should be a completely different aspect. What we don't want however is during the process of transmission some app interfering with the data as it's still in transmission. I think that was @JulienFS's point. (correct me if I misunderstood you)
Ideally the mecanism should rely on a part of the API whose behavior is not customizable, as we don't want any bad app to be able to mess with it.
Well what about apps that e.g. want to offer a feature like converting/compressing images upon upload? Or removing metadata or similar features? AFAIK extensibility should allow this and you likely cannot prevent them to do anything bad, you need to trust the apps installed.
I would advocate for the decision to be left to the final user. My reasoning is : bringing hashes to the core upload mechanism might require a costly major rework, doing it opportunistically with lightweight plugin based tweaks might not offer strong enough guarantees for integrity. Having a side mechanism that checks end-to-end integrity might be good enough. In this regard I would say : I'd better like compressing/whatever apps to have to integrate to the integrity app than let them potentially mess up with the integrity of my data. In the same reasoning, I'd rather like to choose between integrity and post-upload modification than not having a reliable integrity/sync check.
Obviously there's not only integrity involved in using hashes : silent file modification or manual efficient resync are also to consider. At this point I'm trying to figure out if there's any easy path to bring an opt-in solution for people who consider these problems more than eventual post-upload modifications by other apps.
I'm not a NC developer, simply a FOSS enthusiast which happens to also be a pro dev. I'm willing to put some effort to solve these things but it might be a big piece of work and I'm unsure if I'll have the resources to achieve it. Finding a workaround here and now would at least offer some of us enough time to build or wait for a core feature without having to leave NC.
Ideally the mecanism should rely on a part of the API whose behavior is not customizable, as we don't want any bad app to be able to mess with it.
Well what about apps that e.g. want to offer a feature like converting/compressing images upon upload? Or removing metadata or similar features? AFAIK extensibility should allow this and you likely cannot prevent them to do anything bad, you need to trust the apps installed.
The point of this mechanism is to ensure the transmission process is graceful and error-free. Whatever happens afterward, such as automated or manual editing of media stored within Nextcloud should be a completely different aspect. What we don't want however is during the process of transmission some app interfering with the data as it's still in transmission. I think that was @JulienFS's point. (correct me if I misunderstood you)
That would be the "end game". Obviously keeping NC's modularity while fixing hash related problems is the ideal goal.
I'll keep investigatin, maybe adding hashes it not that a big deal, but I prefer to be careful as I'm not familiar yet with the code base.
PS : if anyone landing here has some product management skills, feel free to start aggregating things related to the lack of hashes and get me in the dev loop.
Reacted by SiressUpdate from the rabbit hole.
There might be something to do with hashes as there's plenty of event to plug to on the Sabre side. At least I feel confident in the possibility to intercept all file writes with enough context. There's a priority mechanism for the event handling which would allow an app to intercept writes before any other app, considering that other apps are "educated" and don't register for a low priority if not needed. I still need to check if we can monitor registered apps to see if anyone has a higher priority, that would be a great way to at least notice if another app could mess our things up.I found something disturbing during my little exploration : the file creation and update part on the Sabre side. Basically it uses PHP's
php://inputstream to get the file body, which is fine. But it uses a simplefile_put_contentswithout even checking the result. Maybe worse than that, it looks that for update of an existing file it also uses plainfile_put_contentson the existing file. I have no clue of what would happen is the input file stream was ever to be interrupted, especially since there's no error handling here. But even with error handling we would still have overwritten the existing file.
https://github.com/sabre-io/dav/blob/7a736b73bea7040a7b6535fd92ff6de9981e5190/lib/DAV/FS/File.php#L25C9-L25C26
https://github.com/sabre-io/dav/blob/7a736b73bea7040a7b6535fd92ff6de9981e5190/lib/DAV/FS/Directory.php#L46C9-L46C26
ERRATUM : see following messageAt this point I can't be really sure that there's no extra mechanism plugged somehow that would make the situation better, so lets not jump to conclusion. However I'm still trying to figure out how some zeros ended in my files, and a stream interruption on an update combined with a zero-filled write buffer would match the pattern.
EDIT : I forgot to mention that the bulk upload feature is not leveraging the same code as Sabre's PUT handler. It's re-implementing on top of some of it. For instance and to my understanding files created with bulk upload don't seem to trigger Sabre events, but NC events instead.
Reacted by svenb1234After digging a little bit more : it's not Sabre's default file handling that is being used. I missed the CachingTree passed to the default implementation constructor, which is the starting point for NC's style file handling.
server/apps/dav/lib/Connector/Sabre/File.php
Line 139 in 54afea4
Glad to see that the implementation is more complex than a simple
file_put_contents.
Few things to note at this point :- there is indeed server side error handling : a decent client implementation should be well informed if something went wrong and act accordingly. For example there is a check on the effective file size
- there is a hash mechanism, it the client sends the
X-HASHheader, it should receive aX-Hash-*like header in the responses (that it can check against) - it still looks like we are writing straight on the existing file, but I need to check the whole path generation thing plus the versionning app before reaching any conclusion
I still need to dig deeper.
- Bulk upload needs to be checked to see if it uses the same files as the regular DAV endpoint. At least for what I've seen the hash header is not available for bulk upload.
- Also the chunked upload also needs some exploration : clients split big files and send them as chunks. This is not standard HTTP chunked encoding, this is an application level chunking introduced to ease the resume of interrupted upload of large file (from what I've read).
At this point it starts to look clear to me that a well implemented client should be able to do reliable upload, especially since there's an optional hash check mechanism. I also think that reliability could be increased by not writing to the final file directly, this way we would preserve actual content and not rely on a reupload by the client to fix the file (but this part still needs investigation).
There's a chance that my data corruption was induced by a combination of a bad client(outdated), bad network, and a write final data before sanity checks. The bad client is a big problem that is going to be hard to solve with only server-side work, as even an end to end check would require the client end to be sane. I'm starting to think, in addition of the rest, to some sort of client user agent header check on the server side. We should probably offer an option to block outdated clients or at least ring an alarm. For example the client shipped on ubuntu 22.04 (which is not that old) is few years old and have some bad behavior.When it comes to faster resync, it could be handled by supplying a client hash to an extra endpoint/http method . It has been said previously that "(you) wouldn't trust a file to exist based on a hash" but I would personally trust a modern secure hash. Obviously we would be drifting a bit from standard DAV, but as long as it is optional it shouldn't be a problem (and I think chunked transfers is not that standard, same for bulk upload). That would be a reasonable tradeoff between network usage and CPU usage if we don't store the hash. Only people with a backend storage that bills more for read than write might have a problem, but they could simply not opt-in (or opt out).
Maybe we would have a problem with HTTP timeouts as checksums for big files can take a lot of time to compute (but there are solutions to that). One thing is sure : I would trust a "live" hash but that would be harder for a stored hash, as it would bring more complexity and one more risk of silent data corruption in case of hash desync.We could also send a hash from the client, in addition to the write final data after sanity check change: this way we could ensure data integrity before writing the final content to the final path. I know that there's already plenty of CRC/checksum mechanism on the underlying mechanisms (TCP has been cited) but here we could safeguard against application level errors.
Reacted by SiressIt looks like bulk upload is not using the DAV file handling but is using the native OC file handling. Here is where I landed by trying to follow the epic thread of hooks and inherited classes and autoloaded stuff :
server/lib/private/Files/View.php
Line 625 in 54afea4
So the bulk upload doesn't have some handy stuff from NC's Sabre implementation. But it has its own handy stuff. There's an 'in stream' checksum that happens for each embedded file. The problem is that it operates on the raw stream instead of the actual file data, that is fetched from the stream later. If something gets messed up in the actual fetching of data the previous checksum won't detect it. This is better than nothing as it somehow safeguard against dramatic stream handling problem but it could be enhanced.
The chunked upload is on another path than the regular one and doesn't have the checksum compute/check option. I might be wrong but it looks like it blindly pushes the eventual 'oc-checksum' header to the file info cache without checking it. Thus we could have some checksum already sitting somewhere that could be simply wrong.
I don't really know why there are filesystem related classes for DAV/Sabre and other FS related classes in the OwnCloud lib, and why one is not using the other.
At some point I was affraid that NC drifted from OC and that we ended with a mess, but things look pretty similar upstream. That being said, maybe OC has shifted its resources to it's "infinite scale" backend...
So the first thing would be to ensure that we have a hash coming from the client for the four upload scenarios :
- bulk upload : the hash would need to be per embedded file, nice we already have it, it's md5 but its probably good enough as hostile hash forging is not a major threat here
- regular upload : probably that a header would fit, there's already something computed, we need an extra header with the expected checksum
- chunked upload : we could probably pass the whole file checksum in header for every chunk, so we would be sure that's it's there when reassembling the file
- browser upload : it is in fact the same as chunked upload, at least I think so, but the browser is the client
For all these scenarios, we should perform a last minute checksum before writing anything to the final destination. It involves writing aside from the final destination during the upload. It is already the case for chunked upload, obviously. In this later case we would need to read the reassembled file twice : once to get the checksum, then another time to write to the final destination.
Ideally in all these scenarios the final compute checksum should be return to the client, a header would fit. That would be a nice way to check that things didn't go crazy.
Now to be extra safe we would probably need an extra round-trip from the client to the server. We could check that was is written on disk is actually what should be written on disk. It might sound paranoiac to do this extra check but with the level on inherited filesystem class involved during a simple write operation that might be an option.
The endpoint in use for this extra check could also be used for periodical/manually triggered sync check. It could also serve when bootstrapping a synchronized directory to detect already existing files remotely.
Considering the mess that filesystem handling is at this point I would strongly advise against server-side hash storing. Maybe there's opportunistic reliable hash storing with some object storage backend but otherwise it would be way too hard to guarantee that any modification to a file event through vanilla NC would properly update the hash.
Most likely, before any hash storage to be implemented, we should consider refactoring the whole filesystem handling. Anyway "live" hashes should be good enough.
I'm going to take some times to think about all of this then I'm probably going to start writing some code if it fits my schedule. If anyone has some suggestion feel free to share.
Reacted by Pavel RojtbergI don't necessarily nextcloud doing the safety checks, I just want to be able to access the checksum from the server, so I can write my own scripts that deal with checksum checks in the way I want. Is there no way at all to retrieve the checksum at the moment (without reading the file into memory and computing it, which would be way too heavy) ? It is really frustrating not being able to implement safety checks, I don't understand how the checksum is not already stored, it feels like very basic information that any storage solution should have. Nextcloud always screws up a small percentage of uploads/downloads in a way that it doesn't clean itself up, which makes it pretty much unusable.
Reacted by Torbjørn Viem Ness@sk1806 storing hashes would bring another class of problem though. What if the hash stored doesn't match the actual file ? This is especially problematic since nextcloud can deal with many storage backends. And to be fair with the way hashes are actually dealt with we could end up storing header-provided hashes without even checking that the file was actually written properly...
FTR I still plan to work on this hash thing but my job and my family was very demanding for the past few month. I really hope that I can get myself back on that topic by the end of the year. At least at this point I have a good overview of the code base especially related to file upload. If any dev passing by has some throughput please reach out I can surely put someone on a fast track.
Reacted by Pavel RojtbergReacted by Glassed SilverIs there an update for this yet?
I found an error when uploading large files using the automatic upload feature of the Android app. Most large files (4GB+) had errors (HASH smartphone != HASH server). However, the app didn't report any issues. Files uploaded via the browser are error-free.Reacted by Siress and Michael Keck
To have reliable up- and downloads, generating a checksum on server is needed.
Upload
Download
On backward compatibility:
Upload:
Download:
Additional for Android Files we can check if files are already uploaded (despite on relying on file names).
(this was shortly discussed in NC15 planning meeting)
Additionally, the ability to request checksum of file via endpoint from #25949: