Hypertopic / AAAforREST

An HTTP reverse proxy to bring authentication, authorization and accounting to RESTful applications
GNU Affero General Public License v3.0
6 stars 5 forks source link

Session management #22

Closed franck-eyraud closed 9 years ago

franck-eyraud commented 9 years ago

"à la CouchDB"

franck-eyraud commented 9 years ago

Some parts are missing, do not merge now.

franck-eyraud commented 9 years ago

It should be fine now !

benel commented 9 years ago

I'm confused. Is it:

  1. authentication against CouchDB or equivalent (which name authenticator.cookie.js means that it is similar to other authenticators like LDAP),
  2. emulation of CouchDB (or equivalent) sessions (which would be very useful for applications like Agorae),
  3. or both?

I thought it was # 1 and I've just realized it is probably # 2.

In other words, are sessions between the browser and the proxy, or between the proxy and the server?

benel commented 9 years ago

Or maybe:

franck-eyraud commented 9 years ago

I'm confused. Is it:

  1. authentication against CouchDB or equivalent (which name authenticator.cookie.js means that it is similar to other authenticators like LDAP),

No, for such a feature I would think the name to be authenticator.couchdb

  1. emulation of CouchDB (or equivalent) sessions (which would be very useful for applications like Agorae),

Yes, for the feature.

emulation of CouchDB sessions by the proxy for the browser AND proxy authentication mode by the proxy for CouchDB.

Also correct for the internal functioning.

I named the file authenticator.cookie because as an internal module, it is exactly what it does : authenticate the request by recognizing a cookie. But as a feature, it is not sufficient as you need the mechanism to create the cookie (which was inside session.js and moved in authenticator.cookie). Naming could be reviewed if something more meaningful was found.

In other words, are sessions between the browser and the proxy, or between the proxy and the server?

Between the browser and the proxy.

But, having this feature allows in the same request to either create a cookie between browser and proxy for an external authentication source (like LDAP) or allow the CouchDB backend server to set a cookie between the browser and the server if no suitable external source is found.

benel commented 9 years ago

But, having this feature allows in the same request to either create a cookie between browser and proxy for an external authentication source (like LDAP) or allow the CouchDB backend server to set a cookie between the browser and the server if no suitable external source is found.

Am I correct if I say that the second part of the feature (aka "forwarding cookie management to the CouchDB backend service") could be replaced now by the combination of frontend cookie authentication (the first part of the feature) with backend HTTP authentication (bb540146e462281d86d72d9e7b058e2b77b44edd) and login forwarding (a59714778febb6a4d27fda922ad2696321ea27f7)?

franck-eyraud commented 9 years ago

Hem... Correct that the result would somewhat be the same, but here are the tiny differences I can see :

In other word, yes the users could configure that this way if the limitations above are irrelevant to them, but I would not suggest to remove the forwarding cookie management to the CouchDB backend service feature from the code.

benel commented 9 years ago

OK. I see your point for preserving cookies as a fallback when all authenticators have failed.

Then, for coherence (aaa461ad2a22797396073c506bd30872135e4485) with HTTP basic authentication, we should remove preserveCredentials directive from settings, and preserve credentials similarly when all authenticators fail.

franck-eyraud commented 9 years ago

I'm no sure we are talking about the same thing... for me preserveCredentials is an important setting to preserve so that the proxy administrator activates it on purpose, because if the backend server is not meant to participate in the authentication, it should not receive credentials. Maybe someone would want to set a proxy to a server he doesn't administrate, so can't trust.

If I understood well the coherence point, you mean that the cookie placed by the session management should not be forwarded to the backend server. If this is your concern, then I agree and it might be that currently the cookie is forwarded by default. If yes, then it should be also remove from the proxied request after consumption. I overlooked this aspect.

But the forwarding cookie management to the CouchDB backend service does not share the cookie, just the POST and DELETE _session calls.

franck-eyraud commented 9 years ago

Forwarding credentials in the session handler currently has the same behaviour as the basic auth : they are forwarded only if sessionHandler.preserveCredentials is set to true (if not set, defaults to site.preserveCredentials which itself defaults to false)

benel commented 9 years ago

The test (aaa461a) is very illustrative I think. Although the first test needs preserveCredentials to work, the second works off-the-shelf.

No matter how authentication is done, no matter if user and password are sent every time or only at the first time, I think we should have a coherent decision on when to forward user and password to backend server.

benel commented 9 years ago

they are forwarded only if sessionHandler.preserveCredentials is set to true

In my test, the cookie is forwarded even if I don't have any sessionHandler (see config.sample.json lines 23-27).

benel commented 9 years ago

if not set, defaults to site.preserveCredentials

Oh! Interesting. Then we should have three failing tests when deleting "preserveCredentials": true, from my test. And we have only one failing.

franck-eyraud commented 9 years ago

Well if the couchdb server is also an authentication source (url:http://127.0.0.1:5984/) all three should pass even if preserveCredentials is false. Because credentials are sent to authenticators (as for LDAP) anyway.

benel commented 9 years ago

Here are the alteration of settings and the effects on the tests:

As we can see, the only test to be altered is the one with HTTP basic authentication, and not the ones for cookie creation and use.

benel commented 9 years ago

Well if the couchdb server is also an authentication source

We are not in these settings: there are no authenticators at all (since this test is for a passive proxy). Please look at my files ;)

franck-eyraud commented 9 years ago

Sorry I am distracted, I was focusing on your comments, not enough on the test it self, the site is a simple one. I think that now I get you point.

To make it short, I think that preserveCredentials should be considered true if no authenticators are present. The same was that the call to _session is left untouched because no sessionHanlder is here to try and interpret it.

benel commented 9 years ago

I think that now I get you point.

Cool :)

To make it short, I think that preserveCredentials should be considered true if no authenticators are present.

I agree with that. That was also what I was thinking about in order to simplify settings.

The same was that the call to _session is left untouched because no sessionHanlder is here to try and interpret it.

That makes sense.

I was less convinced by the idea that CouchDB could be silently asked after LDAP (or other) fails (especially if it is undocumented). I will create specific tests to see if it does or not.

franck-eyraud commented 9 years ago

Maybe this makes it coherent : e486cea

franck-eyraud commented 9 years ago

And even : ade29b1

franck-eyraud commented 9 years ago

I was less convinced by the idea that CouchDB could be silently asked after LDAP (or other) fails (especially if it is undocumented).

No,no, this should not occur (if by CouchDB you mean the upstream server and not the authentication source). If it does, it is a bug, not a feature ! ;)

benel commented 9 years ago

And even : ade29b1

Something like this, but the idea would be to completely remove preserveCredentials from the settings (and to replace its negation with site.authentication && site.authentication.length>0). But don't worry, I'll do that.

No,no, this should not occur (if by CouchDB you mean the upstream server and not the authentication source). If it does, it is a bug, not a feature ! ;)

Good to know :) But I'm afraid it does. Here are:

benel commented 9 years ago

Same problem even when the same login (but different passwords) is set on the reverse proxy and on CouchDB.

franck-eyraud commented 9 years ago

You're right. It seems that in this case, the sessionHandler rule is simply ignored (probably you get 401 for the static dummy credentials). I'm searching why...

franck-eyraud commented 9 years ago

Site matching : xcouchdb.local matches couchdb.local, so the passive proxy site above is considered....

benel commented 9 years ago

Site matching : xcouchdb.local matches couchdb.local, so the passive proxy site above is considered....

True. Well done.

benel commented 9 years ago

To finish with, what are the settings for our closed-source backend? How do you combine LDAP and CouchDB on the backend and cookies on the frontend?

franck-eyraud commented 9 years ago

what are the settings for our closed-source backend?

These settings seem to work : (not yet in production)

    preserveCredentials: true,                                                   
    sessionHandler: {                                                            
      path:"/_session",                                                          
      forward:true,        
      preserveCredentials:true,
      userfield:"name",                                                          
      passfield:"password"                                                       
    },                                                                           
    authentication: [                                                            
      {url: "ldap://ldap.acme.org", id: "cn", dn: "dc=acme,dc=org",domain:"acme.org"},                                                          
    ],                                                                                                                                                                
    proxyAuth: {                                                                   
      headerName:"X-Auth-CouchDB-UserName"                                         
    },                                                                             
franck-eyraud commented 9 years ago

Something like this, but the idea would be to completely remove preserveCredentials from the settings (and to replace its negation with site.authentication && site.authentication.length>0).

The settings above illustrates the doubt I had for this : we want preserveCredentials even it we have authenticators. This means that the backend server can take part of the authentication process as fallback.

benel commented 9 years ago

For a perfect architecture, I would prefer an explicit CouchDB's authenticator (which can be done now with authenticator.http). But I understand the reasons you gave and we can keep it.

At least, this is explicit here. It will be OK when documented with a test.

benel commented 9 years ago

I am just wondering about the need of two parameters sessionHandler.forward and sessionHandler.preserveCredentials and the potential difficulty to understand their interaction, and therefore the risks of misconfiguration.

So far we have a scenario for false/false (proxy with extended user base towards an application with cookie) and true/true (proxy with official and community users). Do we have a scenario for false/true or true/false?

franck-eyraud commented 9 years ago

You are right, this aspect needs discussion, and probably modifications. It is a case where I tried to generalize a specific use case to potential others, but did it from the programming point of view instead of user point of view.

First of all forward means forward the _session request to the upstream server, not forward the session cookie (which would not be usable anyway). This technically means to use proxyWork to send it, and get the result. And this means that the client receives CouchDB formatted answer. Then preserveCredentials defines if the credentials are to be sent if not already recognized.

Currently our closed source project uses true/true because it can use CouchDB users. But if we would like to stop using them, we would need to set preserveCredentials to false but keep forward to true (otherwise the client might not work any more). The same would be for a project which runs directly on CouchDB (and uses the _session information) and would like to use AAAforRest to replace couchdb users by an external source.

The false/true would be if we didn't want to get couchdb formatted session but want it to participate to the authentication. However, this is currently impossible because forward is read before sending the request. But it could be achieved by replacing preserveCredentials by the http authenticator as you suggested.

This is for the (long...) explanation, but I think there is a need to update the code. In particular, in the case true/false and invalid credentials, the client gets the anonymous session instead of 401 status code... You might have suggestions to the settings meaning, when to use them, and their name. preserveCredentials is used to recall the already existing one, forward might be modified to only distinguish the format of the result, preserveCredentials being informative enough to decide to send the authentication request to the server.

franck-eyraud commented 9 years ago

Then, for coherence (aaa461a) with HTTP basic authentication, we should remove preserveCredentials directive from settings, and preserve credentials similarly when all authenticators fail. [...] For a perfect architecture, I would prefer an explicit CouchDB's authenticator (which can be done now with authenticator.http). But I understand the reasons you gave and we can keep it.

After having thought about it better, I guess that you were talking only of the sessionHandler.preserveCredentials setting, not the one of a site for basic auth. I answered considering the general one, because the CouchDB session is also linked to basic auth which is used in many cases by couchdb. I am starting to see more sense on your proposal if it is to remove sessionHandler.preserveCredentials. But adding a http authenticator would enable it also for basic auth request and would always doulbe the requests even if site.preserveCredentials is true.

I completely agree that we could gather all ideas for use cases to build a configuration scheme for these 2 parameters.

benel commented 9 years ago

Here are three use cases: https://github.com/Hypertopic/AAAforREST/commits/fix-cookie-forwarding

Any idea for other ones?

franck-eyraud commented 9 years ago

I drafted some extra tests to illustrate the forward/preserveCredentials situation. For sure you'll prefer another structure of the test files. And also some other aspects of the feature, it is a just first step.

What fails are the last tests for what would be normally expected of the format of the answer with the "forward":false,"preserveCredentials":true case, but doesn't work as I was anticipating. I could modify the code or the tests to have it passed, but I think we should decide what is the best, I agree that configuration is currently puzzling.

I have not much ideas for now, but debate is open. Maybe what would be a good solution is to always guarantee that the format sent is the one of CouchDB, and using the upstream server depends on the case. But at least the client knows what it will get, and we can get rid of the forward setting.

benel commented 9 years ago

By testing CouchDB's proxy authentication, I have just realized that it works also with logins not defined in _users database. This makes a lot of sense, but it was not so clear in CouchDB's documentation.

franck-eyraud commented 9 years ago

Of course, otherwise it would be not very easy to use an external authentication authority like LDAP. The first sentence of your link is :

Proxy authentication is very useful in case your application already uses some external authentication service and you don’t want to duplicate users and their roles in CouchDB.