superlogin with couchdb

[planflux]

Hi, I am new to the superlogin. Please forgive me if what I did is not make any sense to you.

I am wondering what is the proper way to allow a new user to register itself in couchdb/_users.

1. Provide the admin:password in the env.sh file. This works, but I have seen a few issues.

when I used REST API to do a post operation with http://localhost:3000/auth/login with a registered user { "username": "joesmith", "password": "bigsecret", } _users will start to have multiple record with the same user names. This is expected because superlogin is using the admin:password to access couchDB and is able to create multiple records.

2. The above leads me to believe that I don't need to give admin:password in the env.sh file, but as soon as I remove the admin:password in env.sh, I got the following response when trying to register a new user. { "error": "Validation failed", "validationErrors": {}, "status": 400 }

[darksnow]

Superlogin needs access to the CouchDB _users database, and that database should not be accessible to every user. As such the Superlogin API needs admin access to couchdb to do it's thing.

If you are concerned about keeping the admin password in the settings file, then one approach I've used is to create a user in couchdb specifically for superlogin. That way it doesn't have to be a full couchdb user and can have a massive random long password, something nobody could guess or remember.

I've also stored the password in an external file and included that file in the settings file. That way my password is never committed to any git repo. While storing passwords in these files can be a security risk, the settings file should never be publicly accessible so it's probably fine.

I hope this help address your concern.

[planflux]

Thank you. Good suggestion.

I realized that the _users is for session. It appears that Superlogin has used a different couchdb database for user info and password. It took me a while to realize that.

VictorL

Sent from my iPhone

On Sep 13, 2017, at 1:30 PM, Martin Fraser notifications@github.com wrote:

Superlogin needs access to the CouchDB _users database, and that database should not be accessible to every user. As such the Superlogin API needs admin access to couchdb to do it's thing.

If you are concerned about keeping the admin password in the settings file, then one approach I've used is to create a user in couchdb specifically for superlogin. That way it doesn't have to be a full couchdb user and can have a massive random long password, something nobody could guess or remember.

I've also stored the password in an external file and included that file in the settings file. That way my password is never committed to any git repo. While storing passwords in these files can be a security risk, the settings file should never be publicly accessible so it's probably fine.

I hope this help address your concern.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

[snowyu]

It could be created new document via anonymous only on _users db.

[clariontools]

@planflux - In a related concern, it seems that the sessions in _users are not cleaned up (deleted) when the session is removed. It also appears that if the user does not logout and the session expires, the CouchDB access is still allowed. The CouchDB access through the _users session is only revoked if the user session in sl-users is removed through a logout or other means ( superlogin.removeExpiredKeys() ).

It seems like a problem if the CouchDB _user session "expires:" property is really not checked when a session token and password are used to access the user CouchDB database(s). In my testing as long as the sl-users session still exists in the session object, you can still access the CouchDB with the session token and password EVEN if the "expires" property has um... expired.

So currently the CouchDB _user session will always authenticate as long as the sl-user corresponding session key exists session property exists, and it does not otherwise check to see if it is expired. It seems like if the session is expired then the CouchDB _user session should not authenticate the access.

It seems like the "expires" property should also be checked to authenticate a CouchDB database request. I also thought that the couch_httpd_auth for timeout should expire these _users sessions, I have the default value of 600 which should equate to 10 minutes, but even after waiting for 10 minutes the session still seemed to authenticate. Although I would rather have superlogin control the session expiration entirely and currently it does do that but only based on the existence of a the session key from _users to sl_users session.

Anybody have any comments on this?

EDIT: October 2, 2017 Conducted more testing and also found that (using very latest commit) superlogin.logoutAll('message') does not remove all sessions if the session is current expired (same with logout). I seem to recall seeing somewhere that that was fixed and in fact I see that two logout events are emitted one shows the session expired, then second reports logout, however the session object for the user still remains.

I then validated that if the session is not expired then it is removed, but still the _user remains, and as mentioned if the session is not removed from sl-users for any reason, the user can still access the database even with an expired session. To test that I just set a short timeout of 1 for tokenLife and sessionLife properties in the superlogin config. Then I logged in, and called superlogin.logoutAll('') with the expired session, the session in sl-users was not removed AND I then took the dbURL from the session and loaded it in postman to see if I still had access to the db and sure enough, I could access the db even though the session was expired.

The only way that you get the forbidden error on the dbURL: { "error": "forbidden", "reason": "You are not allowed to access this db." } Is when superlogin properly deletes the session through the api. Again, if the user does not logout for whatever reason, or when the session(s) is/are expired, they will remain in sl-users. At that point the session in _users will still validate with the dbURL. So with the most recent commit (as of October 2, 2017) here are some of the issues:

• If session has expired, logout and/or logoutAll does not remove the session.
• You can clear all the expired sessions by running superlogin.removeExpiredKeys() either on an interval or a separate cron job, but there will still be gaps where the dbURL may be accessible with an expired key under the conditions described above.
• The sessions in the _users db are not removed by superlogin, but if the session is removed by any means via the superlogin api, then the _users session will throw a forbidden error if accessed. When I say session removed by superlogin api, I mean has been removed from sl-users and the internal store of superlogin, if the session remains in sl-users, even when expired, the _users session will be accessible.
• In another case even if the superlogin server is completely down and the _users session remains (but expired) then the _users session can still authenticate even though the _users session "expires" property has expired.

It seems that the _users session in dbURL should throw a forbidden error if the "expires" property time has passed no matter what, otherwise, why even bother with an "expires" property on _users it doesn't do anything at the moment?

[clariontools]

To further my comments above, I did see why the failed session logout still allows the dbURL to authenticate. It is because the user db permissions still contains the referenced _users user, it is only when that user db permissions member user is deleted that the session becomes invalid. So while the _users session user is not deleted, in a normal superlogin logout or removeExpiredKeys, the permissions member user on the per user db IS deleted and that is what is controlling access to the user db through the session.

To further complicate things, if all users are removed from the user db then it becomes public. I tried @Onnogeorg suggestion in this issue #174, if you add disable anonymous access by setting require_valid_user = true in the local.ini configuration file, then even with no users in the userdb it should not allow access. EDIT: October 5, 2017, in my original test this did not work, but after looking at the CouchDB docs, I noticed there are two places this property can be set. By default in my configuration it was only set on the couch_httpd_auth section, but I noticed it must also be set on the chttpd section (the property was not even in my configuration at all by default). So setting the chttpd section require_valid_user = true as noted in the CouchDB docs only affects the clustered-port which is what I was looking for:

Note: This setting only affects the clustered-port (5984 by default). To make the same change for the node-local port (5986 by default), set the [couch_httpd_auth] setting of the same name.

EDIT: October 5, 2017, All things considered, taking the following approach in addition to setting require_valid_user would the most secure and manageable solution after all?

I also tried @saip92 suggestion on #174 where he suggests:

I think the easiest fix would be to add a role 'user:' to each of the private databases on register.

and that also solves the problem in a different way (but with other side effects), if this approach was taken you could add the role to the userdb permissions when a session user is added, and of course never deleting it. I tested this solution and it does work for the userdb becoming public issue where the require_valid_user = true did not work. EDIT: October 4, 2017, this does work but the now the side effect is that any dbURL for the user will now have access to the user db as long as the user session remains in _users, that is because of the problems I have been highlighting that the _users sessions are never deleted and by adding a role of "users: solves one problem yet creates another.

So the question remains now should or should not the user session that gets added to _users be deleted with the user is removed from the userdb? I think it should but maybe there is a reason to leave it there? EDIT: October 4, 2017, seems like deleting the _users session, super$user Permissions users, and the sl-users session all at the same time is the best solution. This will only add one more bit of logic to the logout and that is to remove the corresponding _users org.couchdb.user:session. If this is implemented, then adding the role of "users:" to the user db will be secure as long as all the _users sessions are deleted in coordination with the other session objects. Overall there are still multiple points of failure, but by adding the same logic to removeExpiredKeys(), at least a interval based call or separate cron job can do the cleanup of the exceptions.

Also, what is the best way to honor the expired property in the _users db, that just does not seem to work in all my testing. EDIT: October 4, 2017, still wondering if there is an easy way to add this capability directly in couch via the _design/_auth? EDIT: October 5, 2017, the "expired" property is added by superlogin but as I mentioned it does not seem to be serving a purpose with the current api, maybe the intent is that it could be used to cleanup the _users db? Any comments about this would be welcome to fully understand why the property is there and if it is currently being used by the superlogin api in some way?

[clariontools]

I have started to ponder @saip92 's idea in #174 of adding the 'user:' to each of the private databases on register. Now I wonder if that was implemented, then there really would be no need to add each "session" _user to the private database and there would also be no need to manage the _security document of the private database after the user registers. The only requirement at that point would be to make sure you cleanup each "session" user in _users (which currently doesn't happen at all).

This would seem to be a more efficient implementation and just as secure because as I understand only an admin could add a role, thus making 'user:' secure from tampering from the outside? The biggest change is to move the management of user "session" from within the each private database's _security document out to the CouchDB _user document and IMO that should already be managed as part of the logout and removeExpiredKeys apis. Then simply 'user:' to each of the private databases on register, and seems like you would have a much tighter implementation overall?

[clariontools]

UPDATE: I figured out why the _users session user docs were not being deleted on the session logout, see issue #186. The problem is the CouchDB 2.0 (and possibly earlier versions) has a bug that causes allDocs called with an array of keys to fail on each key when running against the _users db.

This still leaves the security issue wide open and it is not solved with CouchDB 2.1, nor do I think it will ever be solved by CouchDB. It is very clear that if no members are defined the database is public, so as @saip92 suggested in #174, you could add the existing member role of 'user:' during the initial creation of the private db. That will work, but if for any reason a _users session is not deleted, that session will still have access to the private db because the member role would match with 'user:'. I am now considering maybe adding a dummy user as maybe a better approach, the loophole being how to avoid a clash with an actual user name. Now if you use superlogin exclusively then that is not a big problem because its _users session user names are hashed so in that case what I have tested is adding a Members -> Users -> Username to each private db such as:

admin

EDIT: If you are using CouchDB 2.1.0, Fauxton vs. 2.0, it now appears that the Permissions are not displayed in the browser Permissions form, but they are still there. One step forward two steps back, if you have the Fauxton with this problem it makes testing much more cumbersome and you cannot delete users, roles from the _security document. Confusing and slows down testing. I use the Docker image so the Fauxton issue may be fixed in other versions. Just keep this in mind if you suddenly see Permissions disappearing from the Fauxton interface even when you know they are there. To verify what is there for sure, you can do a get on the private db with your admin user/password and the db/_security endpoint. Read on...

Basically this is the same idea as @saip92 mentioned in #174, but I think it may be more secure because by adding a role of 'user:' you have a possibility that an orphaned, expired, or active _users session may be used to gain access to a user db outside your application context.

What do you all (maybe not many of you) think about this idea? The superlogin community seems to be either very happy with how things are working or quite a small group? I suppose there are more cloudant users vs CouchDB, so maybe that has a better implementation compared to CouchDB? All comments are welcome and I guess I sort of hijacked this issue, but only because it had to do with superlogin and CouchDB. May need to have @planflux close this issue so I can make a new one?

[saip92]

@clariontools I think the idea that you have presented here is pretty neat. It solves the issue of private DBs from becoming public and also stops previous session credentials from being used. (this essentially breaks the logout all feature allowing someone with stolen session credentials to access private DBs)

Along with these improvements, I believe making sure _users session documents are removed as well is necessary for maximum security. While private data is secured, those orphan session credentials can still be used to mess with public data and/or be used to impersonate another user. (Despite having require_valid_user = true since those orphan credentials are still valid for CouchDB authentication)

If with CouchDB 2.1 that has been solved, then I guess there is nothing to worry but it wouldn't be a bad idea to catch such an error in the future and have another method to secure things (say something like a CRON job that flushes out orphan session credentials)

[clariontools]

@saip92 - I am 100% sure CouchDB 2.1 fixes the bug with keys=[] on allDocs and I have already done a lot of testing on that so it is a must for superlogin users. Unfortunately, there are bugs in the new Fauxton that comes with CouchDB 2.1, specifically you lose the visual of the _security doc even though it is still there, but it makes it more difficult to test.

I think a CRON job or interval on the server.js that calls superlogin.removeExpiredKeys() would be a good addition, but that does not require any changes to superlogin because it already works.

Another thing you could do (with CouchDB 2.1 fix) is call logoutAll and it will remove all existing sessions for the user. Also, you could use logoutOthers maybe right after a successful login to make sure any orphan sessions are removed.

BTW, see my #187 issue and test it out if you have a chance to confirm. Thanks.

[saip92]

@clariontools That sounds great. As for the _security doc, I checked in the docker version of v2.1.0 and I can still see the Permissions view with a special UI (like in v2.0.0 or maybe I misunderstood you? 😛)

It would be best to catch such security risks before hand. A CRON job might be overkill I guess and unnecessary but probably a simple .catch() block at user.js#L1166 would do the trick.

But IMHO for issues like this, a Unit Test which could be used to certify Superlogin against a specific version of CouchDB is the way to go.

Also, you mentioned

use logoutOthers maybe right after a successful login to make sure any orphan sessions are removed

Though this gets the job done, it might not be the way to go for most use cases as applications might want to keep active sessions across multiple devices/browsers.

[clariontools]

After reviewing the config example it looks like there is a way to setup default Admins and Members _security doc for user dbs. Now that I think about it maybe just adding the default admin user as a user on the private db's through the config would be a great idea to protect the user dbs, both shared and private. I don't see any drawback to this because if your admin user is not secure then you have bigger troubles.

So the resolution would be to use the config feature to set the default _security doc for CouchDB per user dbs and add the admin user name to the config. Will have to test this idea to make sure it works but it sounds reasonable.

[clariontools]

@saip92 -

As for the _security doc, I checked in the docker version of v2.1.0 and I can still see the Permissions view with a special UI (like in v2.0.0 or maybe I misunderstood you?

I am saying using the Fauxton that comes with the apache/couchdb docker version 2.1.0. That is if you use the Fauxton that comes with the docker container, it has a bug that has been fixed not too long ago. The problem is when you go into the Fauxton permissions view none of the existing permissions from the _security doc show up in the web view. You can still use curl or postman to view the _security doc but not from the Fauxton UI unless you have the very latest from the git repo which is not in the docker image.

[clariontools]

@saip92 - When you say "special UI" are you talking Fauxton, or do you have something else that you use? If there is something better, please do tell :1st_place_medal:

[clariontools]

@saip92

Though this gets the job done, it might not be the way to go for most use cases as applications might want to keep active sessions across multiple devices/browsers.

Totally agree with you here, it would depend on the use case.

But IMHO for issues like this, a Unit Test which could be used to certify Superlogin against a specific version of CouchDB is the way to go.

For sure, and in the case of the keys=[] on allDocs, it is going to fail on CouchDB 2.0 and could fail on all previous releases for that matter, but will succeed with CouchDB 2.1. Would be a good test and a great addition to the README.md warning of the consequences of not removing the _users sessions. I am a bit interested to see if that was a CouchDB 2.0 specific thing or has been there for a while? The superlogin code was doing the right thing so makes you think it did work at some point during the development.

It would be best to catch such security risks before hand. A CRON job might be overkill I guess and unnecessary but probably a simple .catch() block at user.js#L1166 would do the trick.

The catch would be an improvement and would catch some fringe cases or where the CouchDB is down or unreachable, but it would not catch the situation where a user just closes the browser or closes a mobile app, in that case the logout would most certainly never occur at all and the session would remain active until expired.

[saip92]

@clariontools

The problem is when you go into the Fauxton permissions view none of the existing permissions from the _security doc show up in the web view.

I can see the users/roles as in the _security document. Maybe just a browser refresh would do?

When you say "special UI" are you talking Fauxton, or do you have something else that you use? If there is something better, please do tell

Not really 😄. I meant the simple UI in fauxton for adding/removing Admins/Members Users/Roles instead of directly dealing with the document.

but it would not catch the situation where a user just closes the browser or closes a mobile app, in that case the logout would most certainly never occur at all and the session would remain active until expired.

The catch block would be to notify/workaround issues with clearing the keys. If such a block would have existed, we would have had some workaround which would've cleared the sessions or at least a console message reporting the problem when using CouchDB 2.0. Even with the fix in CouchDB 2.1.0, it would still be good to know of any such failures due to another bug in the future that would leave orphan sessions left out.

[clariontools]

@saip92

I can see the users/roles as in the _security document. Maybe just a browser refresh would do?

It is a known bug that has been fixed in Fauxton and will be part of the CouchDB 2.1.1 release. Based on the issues over at the apache/couchdb repo, it may also have something to do with recent Chrome updates as one commenter mentioned it worked in Firefox. I tried in Chrome and Safari on macOS and it doesn't work for me as reported by others referenced in the apache/couchdb repo issue.

Also, just to compare the version of Fauxton I am using vs. what you have could you do a view source from your browser when Fauxton is up and check the version, you can find it towards the bottom of the page source, I am showing Fauxton Release : 2017-08-04T17:05:44.904Z.

The catch block would be to notify/workaround issues with clearing the keys.

I am not sure about a .catch block here because seems like much of the superlogin code resolves promises to resolve(false) when failed instead of using reject(error). Maybe with the design of superlogin it would be better to add the values to the all(promises).then something like .then(function(values) { at that point I think you would have to iterate through the values array which is the results from all the promises pushed above, and if the second one "removeKeys" returns false then it indicates that the toDelete array contained no valid Keys in the CouchDB _users doc. Not sure what is returned with call to couchAuthDB.bulkDocs(toDelete)? Also, you could have a false returned if all the CouchDB _users sessions are already marked as _deleted = true, so just detecting false is not good enough. Bottom line is at the moment, the removeKeys for CouchDB can fail with the CouchDB bug, but it is a soft failure, there is no way to determine with CouchDB 2.0 (maybe earlier versions too) whether or not the _user session key exists because using keys=[] with allDocs doesn't error, it just indicates that that keys were not found. In the case of Couch 2.0 the code would have to be changed to do a single key="session-key-value" with allDocs, and that would require code changes that would make the function clunky with multiple GET calls for each session. Of course it is not a problem with a single session logout and I already tested that with CouchDB 2.0 and it works fine. Maybe a better approach would be to query the CouchDB version at startup and console log known issues to the console and be done with it? Do you know if this issue existed in CouchDB versions prior to 2.0 as well?

[clariontools]

Tried to test the defaultSecurityRoles to set the security document default in the config as per the example the config example and as Murphy's Law is always in force, but true to the nature of the property's name (defaultSecurityRoles), this only allows you to add a "Role" to the Admins or Members property, in otherwords, you CANNOT add the "names" property.

Ok, so strategy change, I tested by adding to the config:

    defaultDBs: {
      private: ['supertest']
    },
    defaultSecurityRoles: { 
      members: ["admin"]
    }
  },

Get a TypeError: memberRoles.forEach is not a function, so that needs to be fixed at line 81 in couchdb.js and also the same thing for adminRoles on line 75 so change is from:

memberRoles.forEach(function(role) {

to:

Array.from(memberRoles).forEach(function(role) {

Yeah, great that fixes that problem, and testing shows that the results seem to protect the user private dbs from ever becoming public. So with this fix I still need to look at a fix for logout on expired sessions, because it (logout) does not delete the session if it is expired. I recall some talk about a fix for that problem here and I was under the impression it was fixed. I do see two emits when the session is expired, so maybe it deletes the local expired session then does a second logout with no active session, just a guess in hopes that someone remembers this issue before I start debugging it.

[clariontools]

@saip92 - In regards to the idea of calling logoutAll on logout and logoutOthers after a successful login, you rightly pointed out a use case where this would not be desirable.

Though this gets the job done, it might not be the way to go for most use cases as applications might want to keep active sessions across multiple devices/browsers.

I noticed that the ip is saved with the session, I don't see it is used on the logoutAll or logoutOthers, but it seems like that by including the ip, you could allow for that use case by passing an option that would only logout sessions if the ip in the session matched the ip of the logoutAll or logoutOthers call? If this were implemented you could have different options for different use cases:

1. Only call logout and it will only delete the active session (as is no changes required).
2. Call logoutAll or logoutOthers with a flag "filterIP = true" and it will use the current session ip and logout All or Others that have a matching ip in the stored session(s) (requires changes to superlogin).
3. Call logoutAll or logoutOthers as works now (as is no changes required).

In addition to this adding an interval to server.js that calls removeExpiredKeys() will ensure that there are minimal chances for the _users sessions to become flooded with orphaned session users. Of course care would be needed to chose the most appropriate session expiration length.

Another feature that doesn't exist is to run a period cleanup on the CouchDB _users looking at the "expires" property where the date/time has expired but the key does not exist in the superlogin local store. That case should be rare unless you are using CouchDB 2.0 (and maybe earlier) since these _users sessions are never removed due to the CouchDB bug I mentioned (fixed in CouchDB 2.1.0).

[clariontools]

For anyone actually reading this, here is a quick update on this issue:

So with this fix I still need to look at a fix for logout on expired sessions, because it (logout) does not delete the session if it is expired.

EDIT: October 12, 2017, Scratch this, I thought the problem was fixed by upgrading to CouchDB 2.1, not true I started debugging this and suddenly realized that after upgrading from CouchDB 2.0 to CouchDB 2.1, the problem has disappeared. Actually, my debug breakpoints slowed things down so that whatever is happening to not delete expired sessions on logout actually worked. I have to do more testing to track it down. Anyone out there know about this issue and/or a fix?

I haven't verified, yet, but I think I was barking up the wrong tree here. Going to switch over to @micky2be 's superlogin-client that I have been using. Most likely that is where I saw mention of this issue and it may not be any fault of superlogin directly. Yeah, pretty sure if you logout an expired session it first goes through checkExpired() then deauthorizes the token, returns expired, emits a logout, then does the final logout with an unauthorized token which does nothing, leaving all the sessions in the server store, and on CouchDB until you call removeExpiredKeys() at some interval to do cleanup.

EDIT: October 13, 2017, Just a note to self, since ATM, nobody seems to be participating in the discussion. Haven't fully tested, but my understanding is that a call to auth/refresh with a valid non-expired session will return a new session with new expiration, not sure if the token (_users, sl-users, local server store) gets updated haven't validated that yet. However, it seems like refresh should validate the ip address (it may already do that), although it doesn't seem like it does. If refresh does not validate the ip address, then that seems like an area where security could be tightened. Any thoughts on this are welcome :smile: .

EDIT: October 14, 2017, Confirmed this is a superlogin-client issue and have documented the results on a related issiue over at mickey2be/superlogin-client repo.

[clariontools]

@planflux - I think this issue can be closed now (by you since no one else is paying attention here), especially in regards to your questin:

_users will start to have multiple record with the same user names. This is expected because superlogin is using the admin:password to access couchDB and is able to create multiple records.

This is not expected and I have covered all the reasons why this happens (ad nauseam) above :smile_cat: .