Block old legacy clients #15683

LukasReschke · 2015-04-16T20:43:27Z

This Pull Request introduces a SabreDAV plugin that will block all older clients than 1.6.1 to connect and sync with the ownCloud instance.

This has multiple reasons:

Old ownCloud client versions before 1.6.0 are not properly working with sticky cookies for load balancers and thus generating sessions en masse
Old ownCloud client versions tend to be horrible buggy

In some cases we had in 80minutes about 10'000 sessions created by a single user. While this change set does not really "fix" the problem as 3rdparty legacy clients are affected as well, it is a good work-around and hopefully should force users to update their client

@danimo @dragotin @DeepDiver1975 @karlitschek @butonic … and whoever might feel involved 🙈

LukasReschke · 2015-04-16T20:44:47Z

In the future we need to come up with something that also works for regular third-party WebDAV clients… Creating tens of thousands of sessions for single users is not really what we should do. This is only a drop on the hot stone (though a big one…)

LukasReschke · 2015-04-16T20:45:31Z

… please let's discuss the impact of this first before merging :) …

also @MTRichards we really need to define which desktop versions are supported and which ones not :)

LukasReschke · 2015-04-16T20:46:48Z

lib/private/connector/sabre/blocklegacyclientplugin.php

+		if(isset($versionMatches[1]) && version_compare($versionMatches[1], '1.6.0') === -1) {
+			// FIXME: Why can't we uset he ResponseInterface here?
+			http_response_code(503);
+			exit();


@evert Sorry for disturbing but I somewhat had problems to use the ResponseInterface here and then the setStatus stuff etc. – Do you have any idea what I am doing wrong here? Would be totally awesome if you could give me a hint 👯 🍻

(I also returned false)

@LukasReschke I'd simple throw an exception - specifically ServiceUnavailable

Not sure why it didn't work, but @DeepDiver1975 is right, it's better to throw the exception there.

Also: $request->getHeader('User-Agent') instead of $request->getRawServerValue('HTTP_USER_AGENT');

ghost · 2015-04-16T21:12:19Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11591/
🚀 Test PASSed.🚀

MTRichards · 2015-04-16T21:59:29Z

While I reserve the right to change this, or have exceptions based on real customer scenarios, my plan is to support the current release (1.8.x) and the previous major release (1.7.x). We would support any 1.7.x and 1.8.x, but that means we would need to force the update to the latest in that line if there is a problem. This also provides the opportunity for a customer to test the latest release before updating.

Once we ship the updater for on premise, and document how to host a linux package, there is no longer a good reason for not pushing upgrades. BUT, it is still up to the customer, and there are scenarios where this can't fully work.

So...Please don't just block older clients. This will lead to all sorts of unintended problems! It needs a graceful error message, or a notice of deprecation or something...

DeepDiver1975 · 2015-04-16T22:14:03Z

lib/private/connector/sabre/blocklegacyclientplugin.php

+	function beforeHandler(RequestInterface $request) {
+		$userAgent = $request->getRawServerValue('HTTP_USER_AGENT');
+		preg_match("/(?:mirall\\/)([\d.]+)/i", $userAgent, $versionMatches);
+		if(isset($versionMatches[1]) && version_compare($versionMatches[1], '1.6.0') === -1) {


make this configurable?

evert · 2015-04-16T22:33:51Z

I also agree with @LukasReschke that sessions should ideally not ever be started =)

karlitschek · 2015-04-16T22:42:34Z

I agree with @MTRichards We need to guide the users to update the client first before we block it completely.

DeepDiver1975 · 2015-04-17T07:08:54Z

For customers we need this configurable (as in allow all, 1.6.0+ or 1.7.0 ....) - for the community we should by default disallow 1.6.0 and less - it is know that this client has issues and purely talking about the community: most recent clients are available in any linux distro or via OBS, Mac and Windows are already on our own update mechanism.

I'm all in for merging this into 8.1 with a config option.

@MTRichards @karlitschek does that make sense? THX

karlitschek · 2015-04-17T11:28:15Z

i think it makes sense with a config option. what would be the default?
what do you think @MTRichards ?

MTRichards · 2015-04-17T11:56:36Z

I agree - with a config option, a release note and some documentation, and a line in sample.config.php, A configurable value is acceptable. Clearly the versions allowed should be configurable (such as allow this version or newer, or deny this version or earlier where the admin enters the version), since we could see an oC client 2.4.1 accessing ownCloud 8.0.2 in the future.

That said, we can't expect our customers to know our version history numbering, so we need something better than enter something, such as comments in the config file that say the release date of the latest versions as of the time of release. And also in the documentation.

@carlaschroder adding release note tag for you.
@jnfrmarks for testing implications.

DeepDiver1975 · 2015-04-17T12:00:03Z

I also agree with @LukasReschke that sessions should ideally not ever be started =)

That said, we can't expect our customers to know our version history numbering, so we need something better than enter something, such as comments in the config file that say the release date of the latest versions as of the time of release. And also in the documentation.

from a technical perspective we can only check on the explicit version number - the history is within the changelog of the client I assume. Would that be enough @MTRichards - or do I miss anything?

MTRichards · 2015-04-17T12:12:52Z

I don't mind checking explicit version numbers in the code somewhere, but we can't expect an admin to know the explicit version numbers, or change a config option when a new client is released - they won't do it.

An admin should just be able to say (and our default for new installations should be) the current version minus 2, so right now 1.7 and 1.8 clients would be allowed by default. 1.6 and earlier would be denied. Then also of course 1.9 or 2.0 or 2.1 etc would be allowed in this situation. In other words <=1.6 would be denied.

All the admin has to do is know to leave the 1.6 in there, and all 1.6.x versions and earlier will be denied. When we release each ownCloud version, we just would need to increment the config file properly using autoconfig or the ee config file so that at the time of release, the latest 2 dot releases for clients can access.

Of course if we are upgrading customers, this should not be forced on our customers and users, rather it is optional.

DeepDiver1975 · 2015-04-17T12:32:39Z

An admin should just be able to say (and our default for new installations should be) the current version minus 2, so right now 1.7 and 1.8 clients would be allowed by default. 1.6 and earlier would be denied. Then also of course 1.9 or 2.0 or 2.1 etc would be allowed in this situation. In other words <=1.6 would be denied.

So with every ownCloud major release you want us to update the default config value for the blocked clients to be current version - 2 ?

jnfrmarks · 2015-04-17T12:34:17Z

@rperezb

FYI

DeepDiver1975 · 2015-04-17T12:44:52Z

@MTRichards is there a need to have the same functionality for mobile clients?

THX @jnfrmarks for the hint 😉

MTRichards · 2015-04-17T13:21:10Z

@DeepDiver1975

So with every ownCloud major release you want us to update the default config value for the blocked clients to be current version - 2 ?

Ideally, current major version - 2. 1.8 or 1.7 = major version.

That said, how deep down this hole do we want to get? It is possible that a problem in QT that we found makes 1.7.3 work, but not 1.7.2, 1.7.1 or 1.7.0. And 1.8.2, but not 1.8.1. So a customer would want to enable this for 1.7.2 and 1.8.2 and greater only.

The end solution might be

A default version - 2. The best default is current major - 2
A config understanding two things: a specific version number and a blacklist.
The specific version number would be the version like 1.7.1. That would be the version cutoff, anything older (lower version) than that is denied. Then, a blacklist which denies specific versions in addition to this rule.

For example, a setting such as 1.7.1, blacklist: 1.8.0, 1.8.1
This would be anything older than 1.7.1 denied. And also, 1.8.0 and 1.8.1 denied but otherwise anything 1.7.1 and newer is allowed.

For now, I would stick with 1) above and not worry about 2).

MTRichards · 2015-04-17T13:26:19Z

@DeepDiver1975

@MTRichards is there a need to have the same functionality for mobile clients?

Probably. I know how we do propfinds in 8.1 changes, so older mobile versions will not work correctly. Asking @rperezb for more thoughts.

The main difference in mobile is that most people upgrade the mobile to the latest when they find they have a problem (or automatically), so the scale of the issue is a lot smaller. The problem really comes in older versions out there that can't upgrade, like iOS 5 devices. This is a really small %, but we should cover it anyway.
In this case, the default would be the latest version - 2 at the time of server release.

Same rule in my head:
Android and iOS version number set. Any older version is denied, any equal or newer version is allowed. Same syntax, config file notation, release note and all.
It is possible that we have to make it different than major release - 2 simply because of a known issue - like prop finds in 8.1. This can be decided at release time.
(what a maintenance nightmare for us) 😦

@carlaschroder more documentation / release note fun!

DeepDiver1975 · 2015-04-17T13:39:52Z

Ideally, current major version - 2. 1.8 or 1.7 = major version.

I was not precise enough - I was talking about ownCloud Server major releases (as in 8.1, 8.2 ...).

DeepDiver1975 · 2015-04-17T13:42:37Z

And 1.8.2, but not 1.8.1. So a customer would want to enable this for 1.7.2 and 1.8.2 and greater only.

well - as of today: as soon as we are delivering e.g. 1.8.0 - there will be no further development in previous major versions. Based on this assumption we can keep the config entry quite simple.

'minimum_desktop_client_version' => 1.6.2

dragotin · 2015-04-17T14:12:58Z

I think to do that properly, you need to split the version into its three parts: 1.8.0 => major => 1, minor => 8 and patch => 0. With that, the implementation can support fine granular checks, such as

if( major_version >= 1 && minor_version >= 7 && patch_version > 1 ) {
  // allow client version 1.7.2 and 1.8.2 and bigger...
} else {
  // do not.
}

A lot of projects handle the versions according to that theme.

If the code checks for hard coded release strings, it might become tricky. I would not recommend that.

DeepDiver1975 · 2015-04-17T14:19:48Z

@dragotin doing version comparison the right way is for sure necessary - this is out of question.
But we need a config parameter - which should be easy from my understanding

carlaschroder · 2015-04-17T14:20:41Z

@carlaschroder more documentation / release note fun!

Cool, I like fun :)

MTRichards · 2015-04-17T14:55:32Z

@DeepDiver1975

Ideally, current major version - 2. 1.8 or 1.7 = major version.
I was not precise enough - I was talking about ownCloud Server major releases (as in 8.1, 8.2 ...).

Now I am confused. How does this work with server version? I am missing something.

DeepDiver1975 · 2015-04-17T14:56:48Z

Now I am confused. How does this work with server version? I am missing something.

well - checking and rejecting a client is job of the server - we have to provide a default minimum client version - right?

LukasReschke · 2015-04-20T19:22:19Z

@owncloud-bot Retest this please.

ghost · 2015-04-20T19:47:52Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11699/
🚀 Test PASSed.🚀

DeepDiver1975 · 2015-04-23T12:20:47Z

@LukasReschke please add this to the feature wiki - THX
@carlaschroder since you removed the 'release note' - has this been properly documented? THX

DeepDiver1975 · 2015-04-23T12:21:15Z

Release note: owncloud-archive/documentation@1ba4328

okay - looks like - ability to read helps - THX @carlaschroder

DeepDiver1975 · 2015-04-23T12:23:22Z

@DeepDiver1975 I changed the code to use an exception however every invalid client access will now be logged in the server log as exception, since the client does endless retries this gets quite spammy:

{
"reqId": "Sc2f1vM/let4+RTaX/0V",
"remoteAddr": "::1",
"app": "webdav",
"message": "Exception: {"Message":"Unsupported client version.","Code":0,"Trace":"#0 [internal function]: OC\Connector\Sabre\BlockLegacyClientPlugin->beforeHandler(Object(Sabre\HTTP\Request), Object(Sabre\HTTP\Response))\n#1 /Users/lukasreschke/Documents/Programming/master/3rdparty/sabre/event/lib/EventEmitterTrait.php(105): call_user_func_array(Array, Array)\n#2 /Users/lukasreschke/Documents/Programming/master/3rdparty/sabre/dav/lib/DAV/Server.php(456): Sabre\Event\EventEmitter->emit('beforeMethod', Array)\n#3 /Users/lukasreschke/Documents/Programming/master/3rdparty/sabre/dav/lib/DAV/Server.php(254): Sabre\DAV\Server->invokeMethod(Object(Sabre\HTTP\Request), Object(Sabre\HTTP\Response))\n#4 /Users/lukasreschke/Documents/Programming/master/apps/files/appinfo/remote.php(81): Sabre\DAV\Server->exec()\n#5 /Users/lukasreschke/Documents/Programming/master/remote.php(84): require_once('/Users/lukasres...')\n#6 {main}","File":"/Users/lukasreschke/Documents/Programming/master/lib/private/connector/sabre/blocklegacyclientplugin.php","Line":73}",
"level": 4,
"time": "2015-04-20T09:08:20+00:00"
}

Any ideas to prevent this?

@LukasReschke this has been resolved from my understanding - right?

DeepDiver1975 · 2015-04-23T12:23:30Z

ready to merge from my understanding 👍

LukasReschke · 2015-04-23T13:48:57Z

@LukasReschke this has been resolved from my understanding - right?

Yes.

@LukasReschke please add this to the feature wiki - THX

Done.

DeepDiver1975 · 2015-04-23T14:12:15Z

lib/private/connector/sabre/blocklegacyclientplugin.php

+	 * @throws \Sabre\DAV\Exception\Forbidden If the client version is not supported
+	 */
+	public function beforeHandler(RequestInterface $request) {
+		$userAgent = $request->getHeader('User-Agent');


what happens if no user agent is defined?

Indeed. That will probably fail as \Sabre\HTTP\MessageInterface::getHeader returns null then. Let me add a unit test for this as well as a test. Nice catch 🚀

In case of an not sent UA header consider the client as valid

LukasReschke · 2015-04-23T14:42:38Z

@owncloud-bot Retest this please.

ghost · 2015-04-23T15:11:24Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/11774/
🚀 Test PASSed.🚀

DeepDiver1975 · 2015-04-23T15:19:15Z

DeepDiver1975 · 2015-04-23T15:19:34Z

one more review please @nickvergessen @th3fallen THX

scrutinizer-notifier · 2015-04-23T15:27:13Z

The inspection completed: 8 new issues, 8 updated code elements

nickvergessen · 2015-04-23T15:51:32Z

tests/lib/connector/sabre/BlockLegacyClientPluginTest.php

@@ -0,0 +1,129 @@
+<?php


lower case file names as per DeepDiver somewhere else in the PRs

Honestly, I will not change this on my own. If @DeepDiver1975 wants this then we shall document this properly and enforce this in Jenkins. We have a hell of test files that use upper case and PHPStorm will highlight PHPUnit files only using this syntax. Since we don't have that mad autoloader madness problem here it will also work quite well…

As in: Needs to get documented at https://doc.owncloud.org/server/8.0/developer_manual/general/codingguidelines.html 🙈

nickvergessen · 2015-04-23T15:51:34Z

@DeepDiver1975 I don't see a comment on https://github.com/owncloud/core/pull/15683/files#r28717907 yet. Other then that, fine by me

Block old legacy clients

carlaschroder · 2015-04-24T17:01:17Z

doc updates owncloud-archive/documentation#1077

Block old legacy clients

LukasReschke added 3 - To Review discussion labels Apr 16, 2015

LukasReschke reviewed Apr 16, 2015
View reviewed changes

DeepDiver1975 reviewed Apr 16, 2015
View reviewed changes

MTRichards added the release note label Apr 17, 2015

DeepDiver1975 added this to the 8.1-current milestone Apr 17, 2015

Reword configuration text

21ad440

carlaschroder removed the release note label Apr 21, 2015

DeepDiver1975 reviewed Apr 23, 2015
View reviewed changes

LukasReschke mentioned this pull request Apr 23, 2015

File corruption owncloud/client#2425

Closed

Catch not existing User-Agent header

ab9ea97

In case of an not sent UA header consider the client as valid

carlaschroder mentioned this pull request Apr 23, 2015

document new blocking old client behavior owncloud-archive/documentation#1077

Closed

nickvergessen reviewed Apr 23, 2015
View reviewed changes

vader2014 mentioned this pull request Apr 24, 2015

Processors without SSE2 instruction set fail with error 0xc000001d on Windows owncloud/client#2941

Closed

LukasReschke added a commit that referenced this pull request Apr 24, 2015

Merge pull request #15683 from owncloud/block-legacy-clients

b9df932

Block old legacy clients

LukasReschke merged commit b9df932 into master Apr 24, 2015

LukasReschke deleted the block-legacy-clients branch April 24, 2015 16:21

rperezb mentioned this pull request May 12, 2015

[1.6.x] Error message is not very clear when the desktop version is not allowed by server owncloud/client#3220

Closed

mmattel pushed a commit to mmattel/core that referenced this pull request May 22, 2015

Merge pull request owncloud#15683 from owncloud/block-legacy-clients

93d181b

Block old legacy clients

stonerl mentioned this pull request Jun 25, 2015

Disallow old clients form connecting to the server #15032

Closed

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Block old legacy clients #15683

Block old legacy clients #15683

Conversation

LukasReschke commented Apr 16, 2015

LukasReschke commented Apr 16, 2015

LukasReschke commented Apr 16, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 16, 2015

MTRichards commented Apr 16, 2015

Choose a reason for hiding this comment

evert commented Apr 16, 2015

karlitschek commented Apr 16, 2015

DeepDiver1975 commented Apr 17, 2015

karlitschek commented Apr 17, 2015

MTRichards commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

MTRichards commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

jnfrmarks commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

MTRichards commented Apr 17, 2015

MTRichards commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

dragotin commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

carlaschroder commented Apr 17, 2015

MTRichards commented Apr 17, 2015

DeepDiver1975 commented Apr 17, 2015

LukasReschke commented Apr 20, 2015

ghost commented Apr 20, 2015

DeepDiver1975 commented Apr 23, 2015

DeepDiver1975 commented Apr 23, 2015

DeepDiver1975 commented Apr 23, 2015

DeepDiver1975 commented Apr 23, 2015

LukasReschke commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasReschke commented Apr 23, 2015

ghost commented Apr 23, 2015

DeepDiver1975 commented Apr 23, 2015

DeepDiver1975 commented Apr 23, 2015

scrutinizer-notifier commented Apr 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Apr 23, 2015

carlaschroder commented Apr 24, 2015