Faulty session handling (creating massive amount of sessions when webdav-interface is in use) - workaround

[Cymaphore]

ownCloud 5.0.11
lighttpd/1.4.28 (via reverse proxy)
Linux 3.8.0-31-generic #46~precise1-Ubuntu SMP Wed Sep 11 18:21:16 UTC 2013 x86_64 x86_64 x86_64 GNU/Linux

Many (most?) WebDAV-Clients don't support cookie-handling very well, since session credentials are handled using HTTP Authentication. ownCloud however enforces the creation of sessions, even if they are not needed.

This causes the creation of a MASSIVE amount of useless session files when using caldav, carddav, etc. ... In my personal instance approx. 20000 files are created per week. That means a serious performance impact.

I created a workaround as well as a suggestion to resolve this issue permanently, if you provide me with proper feedback.

Workaround - Remove session after remote.php-query is done:

--- remote.php 2013-10-17 12:12:35.797631235 +0200
+++ remote.php 2013-10-17 12:12:40.365631235 +0200
@@ -38,3 +38,6 @@
}
$baseuri = OC::$WEBROOT . '/remote.php/'.$service.'/';
require_once $file;
+
+session_unset();
+session_destroy();

This is a safe way to resolve the issue, however:

There is a performance gain possible but not creating the session when involving remote.php at all. Is there already a way to suppress session-creation? If not, it could be supressed using a variable like $RUNTIME_NOAPPS (for example set $RUNTIME_NOSESSION = true; at the top of remote.php, in base.php don't create session when it is set).

If you like that solution I could implement it. However, I highly recommend to do something about it.

Best regards,
Martin

[Cymaphore]

Okay, same problem applies to app requests, etc., so a central solution is advised.

[DeepDiver1975]

Because the main use case for the ownCloud WebDAV insterface is file syncing via our desktop and mobile clients we need to maintain the session because they make heavily use of it in order to gain speed.

We could think of a solution where the clients send a custom http header which indicates that the session has to be kept alive/needs to be created.

Anyway this is a feature we can think of for OC7.

@Cymaphore thanks a lot for offering your interest in contributing - very much welcome.
Feel free to build a solution and submit a pull request here on github - THX!

[DeepDiver1975]

@dragotin @danimo @rperezb your ideas on session handling and webdav please - THX

[Cymaphore]

Okay, caldav obviously doesn't use remote.php, here my suggested fix using RUNTIME_-variable. You will also notice nice performance improvement.

--- lib/base.php   2013-10-17 12:43:42.109631235 +0200
+++ lib/base.php        2013-10-17 12:44:16.945631235 +0200
@@ -479,7 +479,10 @@
                self::checkConfig();
                self::checkInstalled();
                self::checkSSL();
-               self::initSession();
+               if(!$RUNTIME_NOSESSION)
+               {
+                       self::initSession();
+               }

                $errors = OC_Util::checkServer();
                if (count($errors) > 0) {

--- remote.php     2013-10-17 12:12:35.797631235 +0200
+++ remote.php  2013-10-17 12:43:27.825631235 +0200
@@ -1,5 +1,6 @@
 <?php
 $RUNTIME_NOAPPS = true;
+$RUNTIME_NOSESSION = true;
 require_once 'lib/base.php';
 $path_info = OC_Request::getPathInfo();
 if ($path_info === false || $path_info === '') {

--- apps/calendar/caldav.php       2013-10-17 12:44:56.417631235 +0200
+++ apps/calendar/caldav.php    2013-10-17 12:45:16.509631235 +0200
@@ -1,4 +1,5 @@
 <?php
+$RUNTIME_NOSESSION = true;
 if(!file_exists('../../lib/base.php')) {
        die('Please update the path to /lib/base.php in caldav.php or make use of /remote.php/caldav/');
 }

[DeepDiver1975]

@Cymaphore Thanks a lot! Can I ask you to submit a pull request? THX

[Cymaphore]

@DeepDiver1975 I don't use WebDAV for file access, I use CardDAV and CalDAV from my android devices and different web apps, none of them has proper cookie handling.

A solution would be to disable cookies only for carddav / caldav and/or detect the User-Agent (there is a small number of user agents, for example PHP-Scripts, witch never handle the cookie stuff when doing dav-calls).

I will submit a pull request when someone confirms that this makes sense for you :-) ... Makes sense for me, but you mentioned something about webdav-issues...

[Cymaphore]

Okay, i fixed the workaround and added it to carddav. Using a definition is not beautiful but reliable.

--- lib/base.php   2013-10-17 12:43:42.109631235 +0200
+++ lib/base.php        2013-10-17 13:07:36.085631235 +0200
@@ -479,7 +479,10 @@
                self::checkConfig();
                self::checkInstalled();
                self::checkSSL();
-               self::initSession();
+               if(!defined("DISABLE_SESSION"))
+               {
+                       self::initSession();
+               }

                $errors = OC_Util::checkServer();
                if (count($errors) > 0) {

--- remote.php     2013-10-17 12:12:35.797631235 +0200
+++ remote.php  2013-10-17 13:08:25.713631235 +0200
@@ -1,5 +1,6 @@
 <?php
 $RUNTIME_NOAPPS = true;
+define("DISABLE_SESSION", 1);
 require_once 'lib/base.php';
 $path_info = OC_Request::getPathInfo();
 if ($path_info === false || $path_info === '') {

--- apps/calendar/caldav.php       2013-10-17 12:44:56.417631235 +0200
+++ apps/calendar/caldav.php    2013-10-17 13:08:35.981631235 +0200
@@ -1,4 +1,5 @@
 <?php
+define("DISABLE_SESSION", 1);
 if(!file_exists('../../lib/base.php')) {
        die('Please update the path to /lib/base.php in caldav.php or make use of /remote.php/caldav/');
 }

--- apps/contacts/carddav.php      2013-10-17 13:04:43.201631235 +0200
+++ apps/contacts/carddav.php   2013-10-17 13:08:46.961631235 +0200
@@ -1,4 +1,5 @@
 <?php
+define("DISABLE_SESSION", 1);
 if(!file_exists('../../lib/base.php')) {
        die('Please update the path to /lib/base.php in carddav.php or make use of /remote.php/carddav/');
 }

[Cymaphore]

I Created pull requests and I highly recommend to also add this to the next 5.x release in some way :-)

[jancborchardt]

So then let’s try it again: @dragotin @danimo @rperezb your ideas on session handling and webdav please

[DeepDiver1975]

The answer is pretty clear from the perspective of our clients (mobile and desktop):
we require sessions!

As @Cymaphore pointed out there are clients which don't support sessions and in this case there is not need to create a session on the server - it's a waste of resources, because the sessions are not needed.

If we want to go into the direction of enabling/disabling sessions for *DAV we need a criteria to decide if we create a session of not.

I'm honestly voting against any further enhancements in this area - the more conditions we add - the more we can fail ...

👎 for the sake of stability

[Cymaphore]

I fully agree. My solution is just a workaround.

From my point of view, what needs to be done:

• Enhancing the core to support classification of the client software. Could be useful in future in general, in case support for other kinds of interfaces is added. For now it would be enough, if the classification designates weather or not the current client device supports session handling. This can be done by interpreting header informations according to RFC 2616 and RFC 2518, as well as interpreting the HTTP User-Agent Header according to certain criteria (Known-Good, Known-Bad, Uncertain, Default behaviour).
• Enhancing components in places, where session handling is used to work properly according to the informations provided by the client classification.

As I mentioned before, I use this approach in several places to manage capabilities of (mobile) browsers server sided.

Best regards,
Martin

[Cymaphore]

By @DeepDiver1975:

we require sessions!

My understanding:

If ownCloud has a mandatory dependency on HTTP Cookies to work properly, then it will not be able to fully comply to RFC 2518 & Co.

If you make something mandatory, witch by standards must not be mandatory, you have a Product that breaks with Standard.

So, two solutions from my Point of view:

• Change Manuals and the Website and add the information, that ownCloud's Standard Compliance for WebDAV/CalDAV/CardDAV/... is only limited and that it will not be comparable to many properly implemented *DAV-Clients. Would be okay for me, I'll continue to keep my workaround patches.
• Remove mandatory dependency on cookies for proper operation. Personally I would vote for that. To be honest: Currently, Android, iPhone and other Mobile as well as some Desktop and Web-Clients will break ownCloud Servers when operating with CardDAV and CalDAV. It's only a matter of time before you run out of iNodes if you don't implement some sort of session cleanup by yourself. That is not a feature, that's a bug. :-)

And as I stated, If you decide for some solution, I would gladly implement it.

Best regards,
Martin

[DeepDiver1975]

If ownCloud has a mandatory dependency on HTTP Cookies to work properly, then it will not be able to fully comply to RFC 2518 & Co.

Let me be a bit more precise on that:
we require sessions for the best user experience regarding file synchronization which we do via WebDAV.
It will work without sessions/cookies - but the performance will drop dramatically.

We are compliant to the standards!

[Cymaphore]

Sorry if my words were a bit harsh. Maybe i missunderstood the discussion.

If it works without sessions/cookies that would be okay for me. For as long as the serious bug is fixed. :-)

[DeepDiver1975]

Sorry if my words were a bit harsh.

No worries! 😉

If it works without sessions/cookies that would be okay for me. For as long as the serious bug is fixed. :-)

Let's see what we can do ....

[quenenni]

Hello everyone,

I wanted to give my experience about that problem.

Having a multi-instance owncloud, some instances with a lot of files accessed via webdav and several users, I could have like 50-70 000 session files in a few hours.
My server became nearly frozen several times (load average > 20) because of the php5 cron launched every 5 min to manage the session files.

So I had to find a solution. I did, even if it's an ugly one, but maybe it will help others while waiting for the right solution.
The idea was to not use session when using webdav and keep using session when connecting from owncloud clients.

In remote.php:

...
$baseuri = OC::$WEBROOT . '/remote.php/'.$service.'/';
require_once $file;

// Patch to remove session when using webdav
if (!(isset($_SERVER['HTTP_USER_AGENT']) && preg_match("/(csyncoC|mirall)/", $_SERVER['HTTP_USER_AGENT']))
              && preg_match("/(remote\.php\/webdav)/", $_SERVER['REQUEST_URI'])) {

                session_unset();
                session_destroy();
}

I tried to find a better way, but the problem is that available infos when using different webdav clients ('mount -type davfs ..' in linux, netdrive in windows) offers few possibilities.

I tested my patch with owncloud client for linux, mac & windows and webdav access from linux & windows and it works.

Also, sharing contacts (caldav) and mozilla_sync still use sessions.

Hope this will help someone.

[gig13]

@MorrisJobke
I see the milestone for OC8, when will we know definitely if it is in OC8, and which version?

[MorrisJobke]

@gig13 Once someone has done this and this ticket is closed. Currently no one is assigned. Let me look for that.

[MorrisJobke]

Oh. It's the backlog for ownCloud 8 @craigpg Can you re-triage this please?

[MorrisJobke]

1. request: We set a YOLO=42 cookie if no session exists
2. request: If no session exists and cookie YOLO exist then we do start_session
3. request: If a session exists then we use the session.

Should fix the problem IMHO.

Sounds good, but only if we actually use YOLO as cookie name and 42 as value 😜

[guruz]

@karlitschek @LukasReschke You can optionally shortcut this by checking if the request sends any cookies at all. (because our sync clients will already send some because they got them from request status.php and capabilities)

[MorrisJobke]

@karlitschek @LukasReschke You can optionally shortcut this by checking if the request sends any cookies at all. (because our sync clients will already send some because they got them from request status.php and capabilities)

This would then be even better. Because it doesn't involve any additional stuff that needs to be documented why we send an unused cookie.

[karlitschek]

O.K. Would be good to simplify. @DeepDiver1975 @LukasReschke Can you confirm?

[LukasReschke]

Would be possible, would however only work for our own clients. I didn't check our mobile apps though but they may be negligible considering that they don't sync files all at once.

This means that other clients that support cookies (there are some) will not profit from our cookies and need to re-authenticate on each request. I'd rather prefer the YOLO cookie as it will be supported by any client that supports cookies as per the spec while the other one will only work for clients that do a status.php request which is also some kind of magic.

[PVince81]

Note that the outcome of this also has the potential to accelerate federated shares, see #12228

[guruz]

@LukasReschke Yes that's why I wrote optionally shortcut :-)

[PVince81]

We're past feature freeze, what to do with this issue ?

[PVince81]

@ChristophWurst mind linking to the ticket/PR with the workaround we talked about ?

[PVince81]

From what I understand, if you use token-based auth (aka application passwords) in 9.1 it should not create any additional sessions now. To be confirmed.

[PVince81]

@PhilippSchaffrath @butonic for session handling for clients that do not support cookies.
Not sure if OAuth2 can help there.

[PVince81]

YOLO cookie as mentionned in #5383 (comment) might be the "cookie_test" cookie that was added in 9.1 and which apparently doesn't work correctly.

See #26651 and #27549

[PVince81]

Closing in favor of #29779

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.