Checksum Mismatch on Download Must be Handled Gracefully

[dragotin]

Expected behaviour

If the client detects a mismatch of checksum after download of a file, it must not completely drop the file but only display a warning that the checksums did not match and the file might be corrupted.

Actual behaviour

Because of owncloud/core#23783 we must not handle the checksum mismatch as a hard error. Because the server potentially delivers wrong checksums for files, there will be files where the checksums will not match even for not corrupted download.

Steps to reproduce

1. Have a wrong checksum stored in servers file cache
2. Have it synced to the client
3. Client will complain about broken checksum and try again.

Client version 2.2.0 git.

[danimo]

Lets disable checksumming completely then. Displaying warnings will do more harm than good, i.e. lead to more bug reports that will wonder why there are warnings, and if they should worry.

[dragotin]

Hm. Yes. Probably true.

[guruz]

Linking owncloud/core#23783

[AnrDaemon]

What use is for such a warning?
Checksums are expected to be correct all along. This is the very reason they exists.
I suggest closing this report as invalig. There must be no wrong checksum in the server storage in first place. If it happened at the server side, the bug is in the server.

[rullzer]

As discussed today once again. We just pass around checksums and do no actual checksum verification on the server. Which makes them not very useful. As such it is also somewhat dangerous for the client to rely on them.

[AnrDaemon]

If you don't do checksum verification, then don't pass them around. Period.
As stated, the problem is in the server, not client.

[moscicki]

Hello,

I would second the above comment: if this checksumming feature is enabled on the client then you should assume that checksums are reliable and re-download automatically. If the checksum problem persists for several retries then this is another problem and should be reported to a user (most likely a bug in the stack somewhere). This would be useful to report to the user.

[ckamm]

It sounds like we need to do something like this:

• Disable hard checksum validation for servers that don't have a fix for occ files:scan does not invalidate checksums if file changed on disk. core#23783. It looks like there might be a patch in 9.0.2, but it's not certain yet. This is easy because we already have OWNCLOUD_DISABLE_CHECKSUM_DOWNLOAD and downloadChecksumEnabled().
• Allow manual enabling of hard checksum validation for servers that support it (e.g. @moscicki). Would a new configuration flag be ok? I could make it a side effect of setting transmissionChecksum but that's not particularly tidy.
• Improve handling of repeated checksum validation failure. While important, I don't think this is critical for 2.2.0 and I'll move it to its own ticket.

[AnrDaemon]

If I understand it right, transmissionChecksum is designed to ensure integrity of message payload.
In that case, it is a parallel feature to storage validation checksumming.

[ckamm]

@AnrDaemon "Transmission checksums" are the checksums the client sends and receives from the server, "content checksums" are the checksums the client uses to detect changes in local data (never communicated to the server atm), see the doc comment in https://github.com/owncloud/client/blob/master/src/libsync/checksums.cpp for details.

[AnrDaemon]

Ah, k. I confused them with a similarly named feature of HTTP protocol.

[dragotin]

@ckamm IIRC if the transmissionChecksum parameter in the config file is empty or not there, we do not handle any checksums, ie. do no check on it. So I don't think we need a new config param.

[ckamm]

Pull request: #4663

[ckamm]

@rperezb @moscicki The current patch adds a validateChecksumOnDownload configuration and branding option. The setting defaults to false. For CERN's client I'm pretty sure you want it to be true, since your storage backend keeps the checksums valid.

[moscicki]

What about adding this flag to the server capabilities to discover if they support checksums? In this way you can avoid local config option and always do the best of the server you are talking to...

https://github.com/cernbox/smashbox/blob/master/protocol/protocol.md#capabilities-call

[ckamm]

@moscicki That would indeed be nice - I just assumed going down this path was not an option because it'd require a modification to the capabilities the server returns. If it's fine to do that on your end, I'm very much up for it.

Suggestion

{ files_sharing: {...},
  checksums: {
    supportedTypes: ["Adler32"], // what checksum types the server accepts for uploads
    preferredUploadType: "Adler32", // default to this one for uploads
    validateDownloads: true // enable strict checksum checking for downloads
  }
}

@rullzer Feel free to chime in if you have comments regarding the capabilities API.

[moscicki]

I would think that if you do not find the capability then you assume the
default that makes sense for the owncloud server (unless different versions
of owncloud server require different defaults!). Like this you can leave
the owncloud php server in peace until they support it properly and
advertise in their capabilities...

On Wed, Apr 13, 2016 at 10:25 AM, ckamm notifications@github.com wrote:

@moscicki https://github.com/moscicki That would indeed be nice - I
just assumed going down this path was not an option because it'd require a
modification to the capabilities the server returns. If it's fine to do
that on your end, I'm very much up for it.

Suggestion

{ files_sharing: {...},
checksums: {
supportedTypes: ["Adler32"], // what checksum types the server accepts for uploads
preferredUploadType: "Adler32", // default to this one for uploads
validateDownloads: true // enable strict checksum checking for downloads
}
}

@rullzer https://github.com/rullzer Feel free to chime in if you have
comments regarding the capabilities API.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4638 (comment)

---

Best regards,
Kuba

[ckamm]

@moscicki Yes exactly. You'd need to update your server to return this capability to retain the current behavior of validating checksums on download.

[rullzer]

It should not be hard to return proper capabilities. But we would need to define a very concrete set of options to be returned.

However the current server implementation can't provide you this info. As we just store a string (and not even invalidate it at the correct times currently :S).

[rullzer]

@ckamm in general a list as such seems like a good idea. Maybe we should also add the capability if the server checks the checksum etc.

[ckamm]

@rullzer I was assuming that supportedTypes lists the checksum types that the server will be able to validate. The current server would return an empty list and we'd continue requiring people to set the transmissionChecksum option to send checksums to the server anyway.

[rullzer]

Ah that is fine with me as well.
But this is what i mean with we need a strict specification to follow so all parties know what to report and what they can expect

[dragotin]

👍 for server capabilities, but we still have to deal with the old servers which do not send anything in that regards. Then we do not do any checksumming.

[moscicki]

@ckamm: for our system I can set both flags, I don't have a problem with that.

However I think "validateDownloads" is a broken concept by design. Owncloud server should not send any checksums if they are not reliable and they should disable sending the checksum headers until they fix it. So this option should disappear with the last broken owncloud php server deployed...

[ckamm]

Okay, let's reevaluate.

In the released ownCloud server version 9.0.1, if you enabled transmissionChecksum in the client, the client would send checksums to the server, the server would store them and echo them back to client. The server does not verify or invalidate the checksum if someone sneakily changed the data.

That doesn't seem like so much of a problem, because users shouldn't use transmissionChecksum with ownCloud servers at this point. @dragotin does someone actually enable that explicitly? Why?

Can we tell people not to do that? If so, we can drop validateDownloads.

[moscicki]

Ah. Okey. In this case we can introduce the checksum capabilty (and drop
transmissionChecksum in the config) and owncloud server will only report
this capability when they have correct checksum handling in place. So there
will be no problem, because until they fix their checksums they would not
activate this feature at all in the client via the server capabilities.

On Thu, Apr 14, 2016 at 10:21 AM, ckamm notifications@github.com wrote:

Okay, let's reevaluate.

In the released ownCloud server version 9.0.1, if you enabled
transmissionChecksum in the client, it'd send checksums to the server,
the server would store them and echo them back. It does not verify or
invalidate the checksum if someone sneakily changed it's data.

That doesn't seem like so much of a problem, because users shouldn't use
transmissionChecksum with ownCloud servers at this point. @dragotin
https://github.com/dragotin does someone actually enable that
explicitly? Why?

Can we tell people not to do that? If so, we can drop validateDownloads.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4638 (comment)

---

Best regards,
Kuba

[dragotin]

@DeepDiver1975 is there any server release out there that returns checksums at all?

[MorrisJobke]

@DeepDiver1975 is there any server release out there that returns checksums at all?

9.0 will only store the checksums and return them on request, but the check of those checksums will not happen on the server. That is all, what currently is released.

[dragotin]

Please 🙏 answer my question: If I do a GET on a file which has a checksum in the filecache, does any server version add that automatically or not?

[moscicki]

sorry but you should disable serving any checksums which are not checksums
until you can serve checksums which are checksums.

On Thu, Apr 14, 2016 at 2:19 PM, Morris Jobke notifications@github.com
wrote:

@DeepDiver1975 https://github.com/DeepDiver1975 is there any server
release out there that returns checksums at all?

9.0 will only store the checksums and return them on request, but the
check of those checksums will not happen on the server. That is all, what
currently is released.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#4638 (comment)

---

Best regards,
Kuba

[dragotin]

@moscicki yes, of course. But if 9.0.0 by accident returns these checksums we have to react on that in the client, as we know that the checksums are not reliable. Version 9.0.0 is already out and we can not change that any more if people have it installed and do not update.

[moscicki]

yeah, I know. it was more a comment to the server implementation...

[AnrDaemon]

c> In the released ownCloud server version 9.0.1, if you enabled
c> transmissionChecksumin the client,

So, server or client?

c> it'd send checksums to the server, the server would store them and echo
c> them back. It does not verify or invalidate the checksum if someone
c> sneakily changed it's data.

Broken server, can't be helped client side.

c> That doesn't seem like so much of a problem, because users shouldn't use
c> transmissionChecksumwith ownCloud servers at this point. @dragotindoes
c> someone actually enable that explicitly? Why?

This shouldn't be an option at client side, to begin with.

c> Can we tell people not to do that? If so, we can drop validateDownloads.

Just remove this option from client. It's much worse than client-managed file
exclusion lists. While I see the reason for the latter as an ADDITIONAL tool
on client side, it must be the server tho decide, what types of files it want
to accept or decline first.
Checksumming is, however, directly relevant to the very operation of the
suite, and must not be an option you can turn on and off on the client.

[AnrDaemon]

c> Unfortunately, due to owncloud/core#23783, we will have servers that send
c> invalid checksums. The client must disregard these.

Since we're discussing new client functionality, you can safely assume, that
any server not sending the expected caps does not support them and act
accordingly.

c> Clearer wording suggestion:
c> // Whether the checksum header returned along with file downloads can be used
c> // for validation.
c> //
c> // Early versions of the ownCloud server had a bug that could result in invalid
c> // checksums being sent to client. See owncloud/core#23783.
c> //
c> // Default: false
c> validateDownloads: true

I still don't see the need for this. At all.
The server needs fixes, not apologies.

[MorrisJobke]

Please answer my question: If I do a GET on a file which has a checksum in the filecache, does any server version add that automatically or not?

@DeepDiver1975 @PVince81 Please answer.

[ckamm]

1. Unless we accidentally told some users or customer to explicitly enable the transmissionChecksum option when using an ownCloud server (in the config or a branding), this whole thing should be a non-problem. As far as I know the ownCloud server will only echo back checksums if we send them to it in the first place.

2. @AnrDaemon your feedback is appreciated, but I ask you to work on tone of voice. Currently I read you as unnecessarily forceful and confrontational.

   You say things like "Broken server, can't be helped client side.", "The server needs fixes, not apologies." and that's now how we work together. If there's an important bug in a released server version that can be worked around without too much upfront and maintenance cost, we will do it. The focus is not on whose bug it is but on whether we can improve the experience for users.

[rullzer]

Please 🙏 answer my question: If I do a GET on a file which has a checksum in the filecache, does any server version add that automatically or not?

Yes

[rullzer]

@DeepDiver1975 is there any server release out there that returns checksums at all?

Server 9.0 does it. If you send it a checksum it will return that cheksum (not always the right one as we have seen).

[moscicki]

So I think it is good: if you drop the transmissionChecksum in config file in favour of capability call on the server then:

1. existing clients who set transmissionChecksum will not send any checksum if capability is not enabled on the server
2. broken server does not advertise the capability so the sync client will not send checksum on upload so the server will not return it
3. good server will return the capability so it will automatically enable the checksum handling in the client as expected

In this way you leave it in hands of the server to sort out correct checksum handling before advertising the capability to you. From a point of view of a client it is not a problem anymore and no need to maintain extra flags such as validateDownloads.

[dragotin]

@moscicki You are right.

Ok, lets agree on this change: With 2.2.0, we remove the theming option transmissionChecksum from the client. Instead, our code checks for an entry in the server capabilities IF the server does checksumming. No current server does, which in fact disables the entire checksumming for now. Once we have a solution to provide reliable checksums, the server will set a checksum entry in the capabilities.

The only change is required on @moscicki s side: If 2.2.0 is released, you will have to add the entry in your capabilities to continue doing the validation on your site. Also some testing in your setup with a beta would be very appreciated.

@DeepDiver1975 @karlitschek @MTRichards do you agree?

[AnrDaemon]

c> @AnrDaemon your feedback is appreciated, but I ask you to work on tone of
c> voice. Currently I read you as unnecessarily forceful and confrontational.
c>
c> You say things like "Broken server, can't be helped client side.", "The
c> server needs fixes, not apologies." and that's now how we work together.

It would help if you read not only my last message, but previous ones too.

c> If there's an important bug in a released server version that can be worked
c> around without too much upfront and maintenance cost, we will do it. The
c> focus is not on whose bug it is but on whether we can improve the
c> experience for users.

It wasn't an attempt to push things around, it was an explanation to the
previous suggestion.
moscicki already explained it better than me, so I leave it at that.

[karlitschek]

@dragotin makes total sense. Thanks!

[ckamm]

This will be ready-to-test once the pull request #4663 is merged.

[mcastroSG]

Hi hi:

Steps Executed

1. Copy a txt file to shared folder on desktop client
2. Wait till sync
3. Pause Sync at Desktop Client
4. Modify txt file at Web server browser
5. Acces to Server DB and modify cksum stored for txt file
6. Wait till sync

Results

At Activity > Not Synced tab a new warning appears with "The downloaded file does not match the checksum. An a retry is launched at any sync, so this part seems to work fine, but there is no overlay icon update that warns this situation as is described at 3735#

[moscicki]

Please note that I have updated the protocol description accordingly:
https://github.com/cernbox/smashbox/blob/master/protocol/checksum.md

I would appreciate if someone could have a look and cross-check.

FYI: @michaelstingl

[dragotin]

doublechecked, looks very reasonable. Thanks @moscicki