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

Clustered purge should be restricted to admins #1799

Closed
fkaempfer opened this Issue Dec 7, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@fkaempfer
Copy link
Contributor

fkaempfer commented Dec 7, 2018

I would say that the _purge operation should be treated similarily to _compact in terms of the security level required to run it. It is a operation that affects the structure of the database and is invisible in _changes or VDU.

Expected Behavior

POST to the _purge endpoint should check if the user is server admin before allowing the clustered pruge to be executed.

Current Behavior

Every user that has access to the db can run it:
$ curl -X POST -H "content-type: application/json" -d "{"test":["2-eec205a9d413992850a6e32678485900"]}" http://localhost:5984/test/_purge
{"purge_seq":null,"purged":{"test":["2-eec205a9d413992850a6e32678485900"]}}

Compared with compact:
$ curl -X POST -H "content-type: application/json" -d {} http://localhost:5984/test/_compact
{"error":"unauthorized","reason":"You are not a server or db admin."}

Possible Solution

Add the same security check as for _compact

Steps to Reproduce (for bugs)

See above

Context

DB that is available to users via HTTP

Your Environment

CouchDB 2.3.0 (master) on Linux

I can create a PR for it, if you agree that we should have an admin check in place here.

@wohali

This comment has been minimized.

Copy link
Member

wohali commented Dec 7, 2018

The intent was to restore the same purge functionality that existed in 1.x. 1.x did not require server admin permissions.

I disagree that this should require server admin permissions. _compact is set to server-wide permissions because it can consume valuable server resources in a fashion that affect the performance of all other server operations when triggered. _purge does not meet that same criteria.

I might agree that it should be reserved for database admins, but I'd like others to chime in first.

@janl

This comment has been minimized.

Copy link
Member

janl commented Dec 9, 2018

I think _purge should be member facing. If I can write docs to a db and accidentally post a credit card number, I want a way to take it out again without having to ask an admin.

@fkaempfer

This comment has been minimized.

Copy link
Contributor Author

fkaempfer commented Dec 9, 2018

Good points! I did not know about the 1.x legacy and I thought that the missing security was simply an oversight.

I still think it is important to note that _purge let's you essentially bypass validate_doc_update, because it allows you to delete any document you wish. So, because of this, it might still be a good idea to restrict the endpoint to DB admins. Also the purge is not tracked in _changes, so it might become difficult to tell what was going on in the DB.

@jiangphcn

This comment has been minimized.

Copy link
Contributor

jiangphcn commented Dec 10, 2018

Given current code base, if there is need only to allow admin to perform _purge, it is feasible to write additional codes to perform additional check to only authorize admin for _purge endpoint. Also it is possible to write audit log to track what was going on in the DB. handle_req_after_auth/2 in https://github.com/apache/couchdb/blob/master/src/chttpd/src/chttpd.erl#L303 is one starting point which is based on EPI approach.

@fkaempfer

This comment has been minimized.

Copy link
Contributor Author

fkaempfer commented Dec 14, 2018

@janl @wohali : Do you think that the possibility to bypass VDU with _purge for deleting documents is enough of a reason to restrict it to admins? Normally deletions can be disallowed by throwing an exception in VDU, but with _purge, which is accessible to every member/user, VDU is never called (and that's probably correct as it is not a normal doc update).

If you think it's not a problem, I can close the issue. Otherwise I can submit a PR adding a simple check in chttpd_auth_request.erl.

@jiangphcn I think a log file would not help that much, it can not be as easily consumed as the _changes feed.

Thanks!

@janl

This comment has been minimized.

Copy link
Member

janl commented Dec 15, 2018

Good point about the VDU. I somehow assumed those would still be run.

@jiangphcn do you think this could be retro-fitted?

@jiangphcn

This comment has been minimized.

Copy link
Contributor

jiangphcn commented Dec 16, 2018

Agree with @janl. Good reference of using VDU in normal deletion case.

I am quite open to accept the feasible change for _purge endpoint check. Just want to avoid ending up with another different case. If any document needs to be purged, is it always necessary to go to admin to purge document? Is it possible to have option to enable/disable such check according to preference of admin?

BTW: for handle_req_after_auth/2 plug-in, it can be "writing log file", or any other customized codes performing check.

@janl janl added this to the 2.3.1 milestone Jan 24, 2019

@wohali

This comment has been minimized.

Copy link
Member

wohali commented Jan 24, 2019

@jiangphcn We'd like to get 2.3.1 out very shortly - any chance of getting to this one soonish? Should be simple.

@jiangphcn

This comment has been minimized.

Copy link
Contributor

jiangphcn commented Jan 25, 2019

hey @wohali i will follow in coming days (there are some other stuff in parallel).

jiangphcn added a commit that referenced this issue Jan 28, 2019

restrict _purge to server admin
This restrict _purge and _purged_infos_limit to server admin
in terms of the security level required to run them.

Fixes #1799

@jiangphcn jiangphcn closed this in 1353450 Jan 29, 2019

janl added a commit that referenced this issue Feb 7, 2019

restrict _purge to server admin
This restrict _purge and _purged_infos_limit to server admin
in terms of the security level required to run them.

Fixes #1799

janl added a commit that referenced this issue Feb 17, 2019

restrict _purge to server admin
This restrict _purge and _purged_infos_limit to server admin
in terms of the security level required to run them.

Fixes #1799
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.