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

S2S: Reuse session instead of authenticating on every request #12228

Closed
LukasReschke opened this issue Nov 17, 2014 · 18 comments
Closed

S2S: Reuse session instead of authenticating on every request #12228

LukasReschke opened this issue Nov 17, 2014 · 18 comments

Comments

@LukasReschke
Copy link
Member

LukasReschke commented Nov 17, 2014

Currently server-to-server sharing's authentication mechanism is implemented within

protected function validateUserPass($username, $password) {
, in this code the password is verified for every single request.

This is a very huge performance drawback as the password is hashed using bcrypt which consumes a lot of CPU power. Instead we should generate a session which can be then reused by the remote ownCloud server instead of re-authenticating on every request.

@icewind1991 FYI

@PVince81
Copy link
Contributor

Setting to 8.1 as this makes server to server sharing very slow, something to look at.

CC @DeepDiver1975 @karlitschek

@PVince81
Copy link
Contributor

So, as discussed on IRC with @LukasReschke we need to catch the cookie and resend it with auth, then just reuse it for further requests.

But currently we use Sabre\Client which relies on curl.
And you mentionned that Guzzle is a nice HTTP client that could do it.

Possibly solutions:

  1. Use Guzzle ourselves instead of Sabre\Client
    or
  2. Try and make Sabre\Client use Guzzle instead of curl (if possible from the class structure)
    or
  3. Find a nice and hacky way to tell curl to keep cookies (if possible)

@LukasReschke
Copy link
Member Author

Let me dig into the code some time… Assigning to myself.

@LukasReschke LukasReschke self-assigned this Feb 11, 2015
@PVince81
Copy link
Contributor

I had a quick look and our current version of Sabre wouldn't fit.

But the new Sabre version seems to come with an improved client: https://github.com/fruux/sabre-dav/blob/2.1/lib/DAV/Client.php
That one is based on the Sabre\Http\Client here: https://github.com/fruux/sabre-http/blob/master/lib/Client.php#L18 which has events "beforeRequest" and "afterRequest" that give access to the headers. This means that we could just implement those and do the cookie magic.

It also means that it's really time to upgrade Sabre... #12876

@PVince81 PVince81 mentioned this issue Feb 11, 2015
2 tasks
@PVince81
Copy link
Contributor

🔔 the new Sabre has arrived which means it will be possible to work with the cookies now 😄

@PVince81 PVince81 modified the milestones: 8.2-next, 8.1-current Apr 23, 2015
@PVince81
Copy link
Contributor

Moving to 8.2

@PVince81
Copy link
Contributor

@LukasReschke any update ?

@PVince81
Copy link
Contributor

I guess the outcome of #5383 could also help here ?

@PVince81
Copy link
Contributor

#5383 was moved to 9.1 and is likely to be helpful to solve this one here. Moving to 9.1 as well.

@cmonteroluque

@PVince81 PVince81 modified the milestones: 9.1-next, 9.0-current Feb 18, 2016
@PVince81
Copy link
Contributor

Pluggable auth should be able to help with this.

When connecting to a remote federated share, one would need to pass a token in the "Authorization" header. Note that for fed share there is already a token, the share one. Not sure how to integrate this better with 9.1's new token based auth. @ChristophWurst

@PVince81 PVince81 removed this from the 9.1-current milestone Jun 14, 2016
@PVince81 PVince81 modified the milestones: 9.2-next, 9.1-current Jun 14, 2016
@PVince81
Copy link
Contributor

Reuse session or send an Authentication header as per #11815.

But reusing the session should reduce the burden on the remote server.

@PVince81
Copy link
Contributor

There are actually two levels of reusing the session:

  1. Level 1: within a single PHP request, we already do many calls to the remote server. Keeping a single session within that PHP request would already reduce the load significantly.

  2. Level 2: store the session id in the database to be able to reuse it in separate PHP requests. When expired, need to refresh it. (this all somehow reminds me of OAuth...)

@PVince81
Copy link
Contributor

Hmm strange thing, I just tested on master and looking at access_log I only saw a single 401 appearing instead of multiple ones. Maybe one of the Sabre upgrades brought cookie support. Need to double check this.

@PVince81
Copy link
Contributor

From what I see and remember, Sabre client's HTTP layer has changed a bit. And from what I see here https://github.com/fruux/sabre-http/blob/4.2.1/lib/Client.php#L348 it might be using a single curl session, which is likely to keep cookies.

I'll check on the remote server to see if we're indeed reusing a session.
Then: identify which version this stated happening and hopefully close this ticket 😄

@PVince81
Copy link
Contributor

Okay, I did some debugging on the target server and noticed that the sabre client was already sending the Authorization header for any requests that follow the one with 401. So the 401 only happens once.

Now let's see if we can also reuse the session to reduce the load even more.

@PVince81
Copy link
Contributor

Now thinking of it the public webdav usually isn't supposed to have a session.
But it does have one because someone can authenticate several link shares using their password and that information is stored in the session to avoid re-auth multiple times.

So there is still a little bit to save here.

@PVince81
Copy link
Contributor

Will be obsoleted by #29779

@lock
Copy link

lock bot commented Aug 2, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants