Per document access control #4673

janl · 2023-07-12T15:09:43Z

first draft implementation of apache/couchdb-documentation#424 (which is itself a little out of date, but it paints the big picture).

jaydoane · 2023-08-03T22:12:23Z

src/chttpd/src/chttpd_view.erl

+%    {ok, Resp} = fabric:query_view(Db, Options, DDoc, ViewName,
+%            fun view_cb/2, VAcc, Args),
+%    {ok, Resp#vacc.resp}.
+%    % TODO: This might just be a debugging leftover, we might be able
+%    %       to undo this by just returning {ok, Resp#vacc.resp}
+%    %       However, this *might* be here because we need to handle
+%    %       errors here now, because access might tell us to.
+%    case fabric:query_view(Db, Options, DDoc, ViewName,
+%            fun view_cb/2, VAcc, Args) of
+%        {ok, Resp} ->
+%            {ok, Resp#vacc.resp};
+%        {error, Error} ->
+%            throw(Error)
+%    end.
+


Can this be removed, or is more work needed here?

jaydoane

I pointed out some compiler warnings that I saw when poking around.

I also noticed a few TODO and other comments that I'm guessing need to still be addressed or removed?

Currently this PR has no description, and I wonder if you could add some high level documentation of how this works, or maybe link to an architectural document?

src/couch/src/couch_db.erl

janl · 2023-08-07T09:07:06Z

Currently this PR has no description, and I wonder if you could add some high level documentation of how this works, or maybe link to an architectural document?

sorry for not making this explicit, but if you follow the superseded PRs backwards, you should find more commentary that I didn’t think was necessary to repeat here. The main design is outlined here: apache/couchdb-documentation#424

janl · 2023-08-07T09:08:35Z

I also noticed a few TODO and other comments that I'm guessing need to still be addressed or removed?

thanks and yes, these are all minor or already resolved, but not cleaned up yet. None of these should be major changes. I wanted to get a review on the general shape before getting everything into perfect shape.

Also thanks for the compiler warnings pass, I’ll get to those.

This reverts commit 576c90f.

janl · 2023-08-07T09:49:31Z

latest push fixes compiler warnings noted by Jay

rel/overlay/etc/default.ini

rnewson · 2023-08-07T13:07:07Z

src/chttpd/src/chttpd.erl

@@ -1034,6 +1034,8 @@ error_info({bad_request, Error, Reason}) ->
    {400, couch_util:to_binary(Error), couch_util:to_binary(Reason)};
 error_info({query_parse_error, Reason}) ->
    {400, <<"query_parse_error">>, Reason};
+error_info(access) ->


this is too generic for me, per_doc_access_denied or something similarly descriptive please.

src/chttpd/src/chttpd_db.erl

src/couch/src/couch_db_updater.erl

rnewson · 2023-08-07T14:07:44Z

src/couch/src/couch_httpd_auth.erl

+    Roles = couch_util:get_value(<<"roles">>, UserProps, []),
+    case lists:member(<<"_admin">>, Roles) of
+        true -> Roles;
+        _ -> Roles ++ [<<"_users">>]


[<<"_users">> | Roles]

we’ve been over. this before, I want to explicitly append the new role to the array so any tests and code that expect their existing roles in the beginning just keep working and you OK’d that reasoning on an older PR :)

rnewson · 2023-08-07T14:08:05Z

src/couch/src/couch_httpd_auth.erl

@@ -199,7 +206,7 @@ proxy_auth_user(Req) ->
            Roles =
                case header_value(Req, XHeaderRoles) of
                    undefined -> [];
-                    Else -> re:split(Else, "\\s*,\\s*", [trim, {return, binary}])
+                    Else -> re:split(Else, "\\s*,\\s*", [trim, {return, binary}]) ++ [<<"_users">>]


[<<"_users">> | re:split ...]

c.f. #4673 (comment)

rnewson · 2023-08-07T14:08:16Z

src/couch/src/couch_httpd_auth.erl

@@ -248,7 +255,7 @@ jwt_authentication_handler(Req) ->
                            Req#httpd{
                                user_ctx = #user_ctx{
                                    name = User,
-                                    roles = Roles
+                                    roles = Roles ++ [<<"_users">>]


[<<"_users">> | Roles]

c.f. #4673 (comment)

rnewson · 2023-09-06T21:17:59Z

src/couch/src/couch_db.erl

+                {Body} ->
+                    Access = couch_util:get_value(<<"_access">>, Body),
+                    apply_open_options(Db, {ok, Doc#doc{access = Access}}, Options);
+                _Else ->


will do a full review tomorrow but I am uncomfortable with a giant _Else here. what else would we get except a body? undefined or nil? let's be specific in couch_db.erl.

arturog · 2023-09-07T10:18:54Z

I am coming back to couchdb updating an old application that has already implemented the above (per-document access, with linux-like permissions like owner/read/write access, even per-field protection on the doc). I personally don't like this feature and wouldn't like to see it merged:

This kind of security requirement is almost always specific to the application. In my case, for example, this solution is not fit for purpose.
Most of the time a front-end application server is better suited to handle security requirements, which might be complex.
I believe CouchDB should stick to being a database. We should refine the current codebase instead.
If we need to implement this kind of functionality (like in my case), we are better off providing a plugin/hook system where can enter the validate and read lifecycle of the document. Devs are free to implement what they want and we could provide a working example. (This is what I did). Providing a _custom_<field> on documents for devs to freely use would be great.
Are we going to filter the views? Are we going to filter the document counts on views? What about leaking information on views? Some views need some info, some others not? Will I sometimes use per-user view, sometimes the global views? And what about searching with Lucene? I feel we should not touch our indices.
Replicating documents into your database where the _access field changes, are we doing that? Will devs want a "master" where the _access field is allowed to change, and slaves where you won't? Is the ultimate purpose of this PR to segregate documents within your own DB? Then why not create per-user DBs?

I would be in favour of document lifecycle plugins/hooks, where we can implement this feature and many others.

rnewson · 2023-09-07T12:16:08Z

@arturog

I think secondary indexes would need to be updated to support this for it to have the desired effect (restricting a user's visibility of a database to a subset). I'm sure we've not done that work for dreyfus/clouseau (and I definitely haven't for nouveau).

I share your concern that this implementation might not meet everyone's needs, but that's not necessarily a blocker. It just needs to meet a significant number of people's needs.

I don't find "I believe CouchDB should stick to being a database." compelling. Other databases have richer permissions and access controls that we currently do, they're not off-topic.

The PR changes a number of internal records which prevents a smooth upgrade of existing CouchDB clusters and is blocked from merging until that is fixed imo, so there's time to think.

We currently have validate_doc_update where you can implement any kind of write control logic, but no equivalent for reads (or view queries besides the deprecated _list option). If Javascript were evaluated more efficiently (c.f, the quickjs embedding idea) we might add that.

arturog · 2023-09-07T14:01:27Z

We currently have validate_doc_update where you can implement any kind of write control logic, but no equivalent for reads (or view queries besides the deprecated _list option). If Javascript were evaluated more efficiently (c.f, the quickjs embedding idea) we might add that.

We implemented a fork of CouchDB with exactly that (a validate_doc_read), with code written in Erlang. We also added our own validate_doc_write in Erlang before the JS one kicks in. If extended, I believe it can provide not only this feature, but much more. Also, the issue of leaking out data in indices (specially to the lucene index) lead me to limit indexing to only public fields -- so the field-access was also born. Indices in CouchDB were kept untouched, and though all views had the full set of _ids, on document access (or requesting the full document via views), you would get <unauthorized>. Our implementation was not meant to present a "subset" of documents, but instead control access to the document.

Perhaps the requirement to present a subset of the database needs to be revisited? If this requirement were to be dropped, things might become easier.

janl · 2023-09-12T16:06:45Z

@arturog thank you for your comments, practical experience with variants of the same idea here are invaluable.

This kind of security requirement is almost always specific to the application. In my case, for example, this solution is not fit for purpose.
Most of the time a front-end application server is better suited to handle security requirements, which might be complex.

I’m 100% on board with you here. That’s why this is designed as an opt-in feature. If you have a system on top of CouchSB that works for you, that will just continue to work.

To avoid confusion, let me restate the design goal of this PR: “make the db-per-user pattern obsolete”. It is specifically not “support arbitrary ACLs inside a database”.

With that out of the way, the scope of the potential work for a complete solution here is very big. In accordance with common wisdom: to built a complex system that works, one first has to build a simple system that works. The current PR reflects the simplest system that I could think of to get the minimal functionality working that satisfies the design goal.

To this end, per-user design docs (and all associated indexes of any form, JS, Mango, Search/Nouveau) are not supported. A future version of this PR however can use the new by-access-seq index to build per-user indexes, including reduces that are guaranteed to not leak any data. Global views are already supported but are limited to database admins.

Perhaps the requirement to present a subset of the database needs to be revisited? If this requirement were to be dropped, things might become easier.

Maybe you want a different feature? You are stating the explicit design goal of this PR that should be dropped.

janl mentioned this pull request Jul 12, 2023

[WIP, but please review]: per-doc-access-control #4139

Closed

janl marked this pull request as draft July 12, 2023 15:11

janl force-pushed the rebase/access-2023 branch 2 times, most recently from 3b15aae to c51a1eb Compare July 13, 2023 13:21

janl force-pushed the rebase/access-2023 branch 3 times, most recently from 9e26c53 to 5ec411a Compare July 28, 2023 09:21

janl marked this pull request as ready for review July 28, 2023 12:40

janl changed the title ~~[WIP, but please review] per doc access 2023~~ Per document access control Jul 28, 2023

jaydoane reviewed Aug 3, 2023

View reviewed changes

jaydoane reviewed Aug 4, 2023

View reviewed changes

janl added 17 commits August 7, 2023 11:41

feat(access): add access handling to chttpd

11302dd

feat(access): add access to couch_db internal records

3d225cc

feat(access): handle new records in couch_doc

d182a9c

feat(access): add new _users role for all authenticated users

973c407

feat(access): add access query server

3231b08

feat(access): expand couch_btree / bt_engine to handle access

10525e9

feat(access): handle access in couch_db[_updater]

5cb4b09

feat(access): add util functions

a76a1a9

feat(access): adjust existing tests

033683b

feat(access): add mrview machinery

698f268

feat(access): add access tests

8a0d901

feat(access): add access handling to replicator

eb2f0e4

feat(access): add access handling to ddoc cache

26e6c78

feat(access): add access handling to fabric

db803af

feat(access): additional test fixes

12abedf

fix: make tests pass again

0109b04

feat(access): add global off switch

6cc46f4

janl added 21 commits August 7, 2023 11:41

chore: remove debug log

7f46c70

chore: undo whitespace

2b53bce

refactor: resolve layer boundary violation

441dc28

chore: remove debug comments

8ec3a0e

feat: add _users role for jwt auth

f8586a8

chore: undo unwanted ws changes

f6e409f

chore: remove debugging comments

5bc6d1b

chore: remove debug comments

88dd94a

chore: remove debug comments

3db3dde

chore: remove debug code

c3cdc60

Revert "chore: remove debug code"

fa31ba9

This reverts commit 576c90f.

chore: remove debugging comment

c76b511

refactor: simplify

f3a0110

refactor: simplify

42638d6

debugging on three sites

6cec6b6

fix outstanding test cases

b6b61a0

chore: lint

92dc3d1

force new CI run

fae4a03

re-enable fixed test

d0eecff

fix remaining access tests

6d1d1fa

chore: fix compiler warnings

072d467

janl force-pushed the rebase/access-2023 branch from b1e1899 to 072d467 Compare August 7, 2023 09:49

rnewson requested changes Aug 7, 2023

View reviewed changes

chore: address various rerview notes by @rnewson

b1c6308

rnewson reviewed Sep 6, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Per document access control #4673

Per document access control #4673

janl commented Jul 12, 2023 •

edited

Loading

jaydoane Aug 3, 2023

jaydoane left a comment

janl commented Aug 7, 2023

janl commented Aug 7, 2023

janl commented Aug 7, 2023

rnewson Aug 7, 2023

rnewson Aug 7, 2023

janl Aug 7, 2023

rnewson Aug 7, 2023

janl Aug 7, 2023

rnewson Aug 7, 2023

janl Aug 7, 2023

rnewson Sep 6, 2023

arturog commented Sep 7, 2023

rnewson commented Sep 7, 2023

arturog commented Sep 7, 2023

janl commented Sep 12, 2023 •

edited

Loading

Per document access control #4673

Are you sure you want to change the base?

Per document access control #4673

Conversation

janl commented Jul 12, 2023 • edited Loading

Choose a reason for hiding this comment

jaydoane left a comment

Choose a reason for hiding this comment

janl commented Aug 7, 2023

janl commented Aug 7, 2023

janl commented Aug 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arturog commented Sep 7, 2023

rnewson commented Sep 7, 2023

arturog commented Sep 7, 2023

janl commented Sep 12, 2023 • edited Loading

janl commented Jul 12, 2023 •

edited

Loading

janl commented Sep 12, 2023 •

edited

Loading