Skip to content
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

FileFirewall: Handle Reply Codes Gracefully #3490

Closed
dragotin opened this issue Jul 22, 2015 · 22 comments

Comments

@dragotin
Copy link
Contributor

commented Jul 22, 2015

If the client connects to a server with a file firewall enabled, it should handle the reply codes sent by the firewall gracefully, ie. display proper error messages and not block the entire sync.

@MTRichards is it that?

@nickvergessen can you provide us with a list of the possible reply codes?

@dragotin dragotin added this to the 2.0 - Multi-account milestone Jul 22, 2015

@dragotin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

@nickvergessen maybe you want take this issue, even though it's client? We do not expect that there is anything to code, just more testing.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2015

I have no idea of the firewall so far, but will put it on my todo for tomorrow.

@nickvergessen nickvergessen self-assigned this Jul 22, 2015

@MTRichards

This comment has been minimized.

Copy link

commented Jul 22, 2015

Yep - This is correct - so adding more detail here.

There are really three conditions we need to check on the desktop client.
Assume that the file firewall is in operation on the server and that a rule is set.
Outcome 1) The file firewall rejects all requests because of the rule, for example the user is not on the corporate IP address range. Result: desktop client says this is the case and the sync run fails due to server file firewall rule settings. This is a complete rejection of the sync request.

Outcome 2) The file firewall rejects an upload or download for a specific folder or file, for example an mp3 upload, or a file / folder that is prohibited access from the desktop outside of the corporate network. This is a partial rejection - but the rest of the sync run and all of the other files and folders are synced properly. The ones that fail get errors on the file or folder.

More specifically, say a folder "financials" is prevented by the file firewall from being seen or synced by our clients. The client should red x that folder and everything in it, they don't get synced, but everything else not in the folder is still synced.

Outcome 3) The rule is passed and everything is allowed to be synced. This already works.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2015

Okay, looks like currently it just replies with 403. Will have a look for improvements

@dragotin dragotin added the p2-high label Jul 29, 2015

@dragotin

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2015

@nickvergessen have had time to look into this? I think in case 1) the file firewall should reply with 503 to make the client silently disconnect (kindof).

@danimo danimo modified the milestones: 2.0.1-next, 2.0 - Multi-account Aug 4, 2015

@MTRichards

This comment has been minimized.

Copy link

commented Aug 4, 2015

Pushed to the next maintenance release for timing and availability.

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Aug 12, 2015

Sorry @dragotin I was away until sunday...

I had a quick look at the firewall code and it looks like all the rules just return a boolean. These booleans are then combined and the result is "block" or "pass". So I guess we need to change the logic of the firewall quite heavily, so we can get the reason of the fail in order to send it back as a status code.

As for the "blocking only the sync of a specific folder" point, I didn't find that feature/rule yet. All persons that ever worked on FW are not here anymore, so it will take a while until I find if that rule exists and how we can change the outcome of that.

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

As for the "blocking only the sync of a specific folder" point, I didn't find that feature/rule yet. All persons that ever worked on FW are not here anymore, so it will take a while until I find if that rule exists and how we can change the outcome of that.

We do this the other way round:

We did request a representative rule set from PM and GCX - is the rule set in there?
No -> ignore

@DeepDiver1975

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

We did request a representative rule set from PM and GCX - is the rule set in there?
No -> ignore

I'm sorry to sound rude - but this firewall nightmare is way to expensive when it comes down to time being eaten by core devs

@MTRichards

This comment has been minimized.

Copy link

commented Aug 19, 2015

@DeepDiver1975 Please separate this into two different requests, as they are different. Related, of course, but different:

  1. A list of file firewall rules
  2. The ability to have responses understood by the client

I agree that 1) is still open, got it. 2), though, is a here and now problem. The client tends to fatally fail as it assumes rules pass or fail, and fail stops the sync.

Outcome 2) The file firewall rejects an upload or download for a specific folder or file, for example an mp3 upload, or a file / folder that is prohibited access from the desktop outside of the corporate network. This is a partial rejection - but the rest of the sync run and all of the other files and folders are synced properly. The ones that fail get errors on the file or folder.

This rule can be done today, using regex, and the client doesn't handle it gracefully. All we need is a way for the client to understand "oh, this particular request failed non fatally" and to move on. That is the basis for this request.

@MTRichards

This comment has been minimized.

Copy link

commented Aug 19, 2015

So basically a response that handles three types of rule problems:

  1. Failed, fatal.
  2. Failed, non-fatal (in other words, this specific request is denied)
  3. Success

The rules that throw those errors, and more lists of file firewall rules - this is still owed.

@MTRichards

This comment has been minimized.

Copy link

commented Aug 19, 2015

this may already exist. If it does, we just need to know what the error codes are

@danimo

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2015

@nickvergessen Now that you are back, can we revisit this issue? Or do you think the FF needs more work before we can address this issue in the client?

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2015

The FF needs a lot more work, https://github.com/owncloud/firewall/issues/133
I will try to fix this asap, afterwards we can talk about which "rule" should be handled by the client and what you need for that.

@danimo

This comment has been minimized.

Copy link
Contributor

commented Sep 11, 2015

@dragotin @MTRichards Let's move this to 2.1 then, so it doesn't block 2.0.2.

@dragotin dragotin modified the milestones: 2.1-next, 2.0.2-next Sep 11, 2015

@nickvergessen

This comment has been minimized.

Copy link
Contributor

commented Sep 18, 2015

Okay, here I am again. I'm currently working on the part that returns the Error.
Atm it is always a HTML page, which of course is bad.

Now the question is, what do you guys want as returned status codes:
I'd say 403 (Forbidden) makes sense and is quite obvious.

We will throw an Sabre Exception with that status code, similar to maintenance mode:

<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\ServiceUnavailable</s:exception>
  <s:message>System in maintenance mode.</s:message>
</d:error>

Would you like to have custom exceptions, so you know something that needs to change before you can retry it, or would a simple clean exception also be fine?

The things that can currently be combined are:

  • Invalid Time (changes timebased)
  • Invalid IP / Subnet (changes on network change)
  • Invalid UploadSize (only this file/request)
  • Invalid RequestUrl (only this request)
  • Invalid Device / UserAgent (does not change)
  • Invalid GroupMembership (does not change)
  • Invalid Others (unknown)

Starting from 8.2 the rules are defined as a set of "Ands".
So once either the time, ip or item changes, it's worth trying again.

@MTRichards

This comment has been minimized.

Copy link

commented Sep 18, 2015

Ultimately the best thing is to have an error code that is either fatal or non fatal to a sync, so at least those two separate error codes. I we can have that, it allows the clients to decide "stop the sync run" or "stop syncing one folder or file". User agent could be stop sync, whereas file size would be stop syncing that specific file.

After that, error codes or strings could be used for more specifics, but not the expert on how to do that best.

@felixboehm

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2015

Use case: Block syncing of a specific folder for a group of users
File Firewall does its job, but Sync Client stops syncing for all folders, as access is forbidden for single folder.
@dragotin Please confirm, I assume this is the the way it behaves right now?

dragotin added a commit that referenced this issue Oct 12, 2015

SyncEngine: Handle 403 Forbidden properly.
A 403 is a reply code sent from the file firewall to indicate that
this directory is forbidden to use for the user.

The patch handles it by setting the state to IGNORED.

This addresses #3490

dragotin added a commit that referenced this issue Nov 18, 2015

SyncEngine: Handle 403 Forbidden properly.
A 403 is a reply code sent from the file firewall to indicate that
this directory is forbidden to use for the user.

The patch handles it by setting the state to IGNORED.

This addresses #3490

dragotin added a commit that referenced this issue Nov 18, 2015

Merge pull request #4159 from owncloud/fix_3490
csync updater: Handle file firewall reply codes correctly: Ignore the files.

Fixes #3490
@dragotin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2015

Ok, I tested that again, with #3490 merged: The file firewall of the test server forbids mp3 files. When I add an mp3 file to the local sync folder, the server comes back with an error, and the client shows a red-X for this particular file. Other files upload properly. After a short while, the red X turns to a information icon in the activity-issues-view, telling that the file was not uploaded because of firewall violation.

looklike

In the screenshot you see the previously uploaded file die_drei_bösen_buben.mp3 that is now blacklisted, while adele.mp3 was uploaded before and the red-X entry is gone.

I think this is the exact behaviour how we want it.

@dragotin

This comment has been minimized.

Copy link
Contributor Author

commented Nov 24, 2015

@Dianafg76 you might want to verify.

@dragotin dragotin assigned dragotin and unassigned nickvergessen Nov 24, 2015

@MTRichards

This comment has been minimized.

Copy link

commented Nov 24, 2015

I think so to.
@jancborchardt ?

@Dianafg76

This comment has been minimized.

Copy link

commented Nov 25, 2015

I verify the issue and is working OK
Desktop v ownCloud-2.1.0.2907-nightly20151125.pkg
Server v {"installed":true,"maintenance":false,"version":"8.2.1.4","versionstring":"8.2.1","edition":"Enterprise"}

Closed

@Dianafg76 Dianafg76 closed this Nov 25, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.