You are viewing a plain text version of this content. The canonical link for it is here.
Posted to oak-dev@jackrabbit.apache.org by Jukka Zitting <ju...@gmail.com> on 2013/03/20 11:24:29 UTC
NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Hi,
On Tue, Mar 19, 2013 at 5:09 PM, Marcel Reutegger <mr...@adobe.com> wrote:
> AFAIU, NodeState was specifically designed to not require a path.
Indeed. The main rationale for the design is to avoid the cost of
having to support things like:
assert state.getChildNode("foo").getName().equals("foo");
That seems like a simple thing, but prevents optimizations like the
EmptyNodeState or the ability of the SegmentMK to reuse existing
content to avoid duplicate copies of the same data. Thus I'd be
strongly against a proposal to introduce a method like
NodeState.getPath().
However, the design is also explicitly designed so that a particular
NodeState implementation *can* keep track of the path by recording all
the child node names used in successive getChildNode() calls until the
target node has been reached. This is what for example the
KernelNodeState does and also what we could do if we pushed (read)
access checks below the NodeState interface.
The only complication there, as already discussed before, is the
inability of the current NodeState interface to handle cases where a
child node is readable even if its parent is not.
To address that case I was thinking that we could revise the
getChildNode() contract to *always* return a new NodeState even if the
named child node did not exist. Coupled with something like a new
NodeState.exists() method (that would replace the current null checks
on getChildNode() return values) this would allow us to access nodes
down the hierarchy even if their parents don't "exist" for the current
user.
For example, to access a node like /foo/bar where the parent node /foo
is not accessible, we'd have:
NodeState root = ...;
NodeState foo = root.getChildNode("foo");
NodeState bar = foo.getChildNode("bar"); // no null checks needed!
The existence of these nodes would then show up as:
assert !foo.exists();
assert bar.exists();
A nice benefit of such a design would be that the difference between a
node not existing at all or it being read-protected is entirely hidden
from higher levels, so it'll be harder to accidentally leak
information about the presence of a particular node.
BR,
Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Mar 20, 2013 at 2:52 PM, Bart van der Schans
<b....@onehippo.com> wrote:
> Isn't the idea in jcr that if you are not allowed to read a node that
> you shouldn't be able to to deduce it's existence?
Yes. On the other hand you could argue that making a node at /foo/bar
readable to someone implicitly grants them knowledge about the
presence of a node at /foo, even if they don't necessarily have direct
read access to that node. A call like session.getNode("/foo") could
still fail.
> Yes of course. But if you can't read/see the intermediary node it's
> hard to detect this case in the session itself. The save will still
> fail of course although the user might not have a clue why.
The problem can be detected even by just looking at the paths being
moved. The failure of move("/A/B", "/A/B/C/D") should be fairly
obvious.
> 3. allow implicit read access to all parent nodes (for example by path
> discovery), but not allow read permissions to its' properties
I'd rather call this implicit execute (or traversal) access instead of
read access, since a call like session.getNode("/foo") on a
non-readable parent should still fail. Otherwise a read-access check
on a node might potentially have to look through the entire subtree
for a readable descendant.
BR,
Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Posted by Bart van der Schans <b....@onehippo.com>.
On Wed, Mar 20, 2013 at 1:11 PM, Jukka Zitting <ju...@gmail.com> wrote:
> Hi,
>
> On Wed, Mar 20, 2013 at 1:34 PM, Bart van der Schans
> <b....@onehippo.com> wrote:
>> This is a quite problematic use-case.
>
> Depends on your point of view. The way I see it, the scenario is
> equivalent to a Unix directory with the execute bit set but the read
> bit cleared.
I don't think there is currently an equivalent of that permission set
for JCR. Of course we can introduce it and if we want to go the "unix
way" of permissions we might also want to consider (some of) the
extended attributes features.
>> Consider the a structure like:
>> /A/B/C/D
>>
>> And a user that is not allowed to read node C and has read/write
>> permissions on all other nodes. The view of that user would be two
>> "subtrees":
>> /A/B
>> /D
>
> The Unix scenario, and the way it's currently implemented in
> Jackrabbit and planned for Oak, is different. The user would be able
> to access the following nodes:
>
> /A
> /A/B
> /A/B/C/D
>
> The user can of course deduce the presence of node /A/B/C from the
> above, but any attempt to directly read the node would result in an
> error.
Isn't the idea in jcr that if you are not allowed to read a node that
you shouldn't be able to to deduce it's existence? For example you get
an itemnotfoundexception if you try to access a node to which you
don't have read permissions. If you allow the user to see the full
path you implicitly grant read permissions to the node (but maybe not
it's children or properties)
>> Now the user tries to move node B below D, aka:
>
> With the above approach this would in any case fail, regardless of
> access controls.
Yes of course. But if you can't read/see the intermediary node it's
hard to detect this case in the session itself. The save will still
fail of course although the user might not have a clue why.
So basically we have a few implementation choices here for this case:
1. introduce permissions to mimic the unix behavior of "+x -rw" (which
would also could allow for -xrw and would disallow reads on child
nodes as well)
2. require read access to all parent nodes to allow read access to a node
3. allow implicit read access to all parent nodes (for example by path
discovery), but not allow read permissions to its' properties
4. don't allow path discovery or "deducing" that the non readable node exist.
Regards,
Bart
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Mar 20, 2013 at 1:34 PM, Bart van der Schans
<b....@onehippo.com> wrote:
> This is a quite problematic use-case.
Depends on your point of view. The way I see it, the scenario is
equivalent to a Unix directory with the execute bit set but the read
bit cleared.
> Consider the a structure like:
> /A/B/C/D
>
> And a user that is not allowed to read node C and has read/write
> permissions on all other nodes. The view of that user would be two
> "subtrees":
> /A/B
> /D
The Unix scenario, and the way it's currently implemented in
Jackrabbit and planned for Oak, is different. The user would be able
to access the following nodes:
/A
/A/B
/A/B/C/D
The user can of course deduce the presence of node /A/B/C from the
above, but any attempt to directly read the node would result in an
error.
> Now the user tries to move node B below D, aka:
With the above approach this would in any case fail, regardless of
access controls.
BR,
Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Angela Schreiber <an...@adobe.com>.
hi bart
> Well that reduces the question to just *how* to do it ;-)
sure...
> I was just jumping in because we experienced many problems with this
> use case with JR2 and was wondering if it's was worth the effort to
> support it.
right... but there is no much use for us in creating a default
implementation in oak that doesn't fit our needs. we kept doing that
in jackrabbit core and ended up having plenty of code that we don't
support nor properly test in production. that doesn't help anybody.
however, what i definitely want to do, is to ease pluggability and
creation of different and custom access control and permission models.
this also includes better separation of those two concerns.
kind regards
angela
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Bart van der Schans <b....@onehippo.com>.
On Wed, Mar 20, 2013 at 12:42 PM, Angela Schreiber <an...@adobe.com> wrote:
>
>
> On 3/20/13 12:34 PM, Bart van der Schans wrote:
>>
>> On Wed, Mar 20, 2013 at 11:24 AM, Jukka Zitting<ju...@gmail.com>
>> wrote:
>>>
>>> The only complication there, as already discussed before, is the
>>> inability of the current NodeState interface to handle cases where a
>>> child node is readable even if its parent is not.
>>
>>
>> This is a quite problematic use-case.
>
>
> as a matter of fact we at adobe have to support this use case for
> our products which are the main driver and sponsor for this oak rewrite
> effort.
Well that reduces the question to just *how* to do it ;-)
I was just jumping in because we experienced many problems with this
use case with JR2 and was wondering if it's was worth the effort to
support it.
Regards,
Bart
Re: NodeStates and security (Re: svn commit: r1458234 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Angela Schreiber <an...@adobe.com>.
On 3/20/13 12:34 PM, Bart van der Schans wrote:
> On Wed, Mar 20, 2013 at 11:24 AM, Jukka Zitting<ju...@gmail.com> wrote:
>> The only complication there, as already discussed before, is the
>> inability of the current NodeState interface to handle cases where a
>> child node is readable even if its parent is not.
>
> This is a quite problematic use-case.
as a matter of fact we at adobe have to support this use case for
our products which are the main driver and sponsor for this oak rewrite
effort.
kind regards
angela
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Posted by Bart van der Schans <b....@onehippo.com>.
On Wed, Mar 20, 2013 at 11:24 AM, Jukka Zitting <ju...@gmail.com> wrote:
> The only complication there, as already discussed before, is the
> inability of the current NodeState interface to handle cases where a
> child node is readable even if its parent is not.
This is a quite problematic use-case.
Consider the a structure like:
/A/B/C/D
And a user that is not allowed to read node C and has read/write
permissions on all other nodes. The view of that user would be two
"subtrees":
/A/B
/D
Now the user tries to move node B below D, aka:
/A
/D/B
This would of course create a "circular" path and the save should fail
(in the sessions or on save?). But what kind of error should you get
without revealing the existence of node C?
This is the most trivial example, but it can get rather complicated
with multiple sessions and multiple "hidden" nodes and a mix of
operations. For this reason we require in our own access manager for
read access to a node that the user has also read access to all parent
nodes. I think it's a fair trade off of functionality versus code
simplicity.
Regards,
Bart
RE: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Marcel Reutegger <mr...@adobe.com>.
OK, thanks. This clarifies things quite a bit.
regards
marcel
> On Thu, Mar 21, 2013 at 3:02 PM, Marcel Reutegger
> <mr...@adobe.com> wrote:
> > Do we really understand well enough what the consequences
> > are moving permission evaluation even further down? I.e.
> > *below* the NodeState API?
>
> That's where I originally envisioned it to be taking place, as a
> mostly transparent wrapper around the underlying storage layer (*).
>
> Of course back when we were originally designing NodeState, we didn't
> fully realize the impact of the "invisible parent" scenario (which
> Angela was right to point out), which then led to the TreeLocation
> concept and related machinery above the NodeState level. The way I see
> it, with better design and encapsulation we should be able to get rid
> of most of that stuff.
>
> Of course the full consequences of a change like what's being
> discussed aren't yet thoroughly understood (for example the impacts on
> node builders, content diffs, the TreeLocation interface, etc. are yet
> to be considered). That's why we're discussing. :-)
>
> *) Instead of a full wrapper, such a "secure NodeState" would/should
> only be applied as the basis of TreeImpls visible to the client.
> Things like CommitHooks or the search engine would still work directly
> against NodeStates coming from the storage layer.
>
> BR,
>
> Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Thu, Mar 21, 2013 at 3:02 PM, Marcel Reutegger <mr...@adobe.com> wrote:
> Do we really understand well enough what the consequences
> are moving permission evaluation even further down? I.e.
> *below* the NodeState API?
That's where I originally envisioned it to be taking place, as a
mostly transparent wrapper around the underlying storage layer (*).
Of course back when we were originally designing NodeState, we didn't
fully realize the impact of the "invisible parent" scenario (which
Angela was right to point out), which then led to the TreeLocation
concept and related machinery above the NodeState level. The way I see
it, with better design and encapsulation we should be able to get rid
of most of that stuff.
Of course the full consequences of a change like what's being
discussed aren't yet thoroughly understood (for example the impacts on
node builders, content diffs, the TreeLocation interface, etc. are yet
to be considered). That's why we're discussing. :-)
*) Instead of a full wrapper, such a "secure NodeState" would/should
only be applied as the basis of TreeImpls visible to the client.
Things like CommitHooks or the search engine would still work directly
against NodeStates coming from the storage layer.
BR,
Jukka Zitting
RE: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
Posted by Marcel Reutegger <mr...@adobe.com>.
Hi,
I don't mean to disrupt your conversation about moving permission
evaluation further down the stack, but I still wanted to mention
that my initial understanding of the issue was actually different.
this may be a misunderstanding on my side, though I still
would like to hear from others what they think.
so, here's how I understood the motivation so far and what
I thought a solution would look like:
Permission evaluation is changed in a way that it does not use
the OAK API anymore, but rather operates on the NodeState
API. E.g. Angela earlier mentioned she'd like to have Tree.equals()
as is available on NodeState. Redesigning the evaluation to
work on NodeStates would have solved this problem.
Do we really understand well enough what the consequences
are moving permission evaluation even further down? I.e.
*below* the NodeState API? So far I considered this API
a low level storage interface, which is not aware of users.
Regards
Marcel
> -----Original Message-----
> From: Jukka Zitting [mailto:jukka.zitting@gmail.com]
> Sent: Donnerstag, 21. März 2013 08:57
> To: Oak devs
> Subject: Re: NodeStates and security (Re: svn commit: r1458234 - in
> /jackrabbit/oak/trunk: oak-
> core/src/main/java/org/apache/jackrabbit/oak/core/ oak-
> core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-
> core/src/main/java/org/apache/jackrabbit/oak/plugins/v...
>
> Hi,
>
> On Wed, Mar 20, 2013 at 6:46 PM, Angela Schreiber <an...@adobe.com>
> wrote:
> > the only thing i keep struggling with is: i don't want to evaluate
> > read access for all parent items
>
> There shouldn't be a need for doing that.
>
> Since the proposed getChildNode() calls would never return null, there
> won't be a need to perform access control checks on that method (the
> return value gives out no information about the presence, absense or
> accessibility of a node). The access check is only needed for the
> exists() and other NodeState methods that get called if a node is
> actually being read instead of just traversed.
>
> For example, accessing a path like the mentioned "/foo/bar", would
> ultimately boil down to something like the following sequence of
> calls:
>
> NodeState root = ...;
> NodeState foo = root.getChildNode("foo"); // no access check
> NodeState bar = foo.getChildNode("bar"); // no access check
> if (bar.exists()) { // access check performed here
> return new Node(bar);
> } else {
> throw new NotFoundException();
> }
>
> BR,
>
> Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/versi...
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Mar 20, 2013 at 6:46 PM, Angela Schreiber <an...@adobe.com> wrote:
> the only thing i keep struggling with is: i don't want to evaluate
> read access for all parent items
There shouldn't be a need for doing that.
Since the proposed getChildNode() calls would never return null, there
won't be a need to perform access control checks on that method (the
return value gives out no information about the presence, absense or
accessibility of a node). The access check is only needed for the
exists() and other NodeState methods that get called if a node is
actually being read instead of just traversed.
For example, accessing a path like the mentioned "/foo/bar", would
ultimately boil down to something like the following sequence of
calls:
NodeState root = ...;
NodeState foo = root.getChildNode("foo"); // no access check
NodeState bar = foo.getChildNode("bar"); // no access check
if (bar.exists()) { // access check performed here
return new Node(bar);
} else {
throw new NotFoundException();
}
BR,
Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in /jackrabbit/oak/trunk:
oak-core/src/main/java/org/apache/jackrabbit/oak/core/ oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/
oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/versi...
Posted by Angela Schreiber <an...@adobe.com>.
hi jukka
i like the idea of having read permission enforced on the node state
level and avoid the null checks as it is a similar approach we
are currently having by the treelocation concept that michael introduced
some time ago.
the only thing i keep struggling with is: i don't want to evaluate
read access for all parent items which most probably will not be
read within the lifespan of a session but rather limit permission
evaluation to those items that are actually being accessed... which
in our use case rather resembles a random access pattern plus
1 or 2 levels at a given tree location than a simple traversal.
in other words: in order to evaluate the permission at some location
in the hierarchy with our mainly path based permission model
we somehow need to be able to provide the hierarchy information
which as far as i saw so far was missing on the nodestate api.
that's the main piece that i was missing... otherwise your proposal
make sense to me.
kind regards
angela
On 3/20/13 11:24 AM, Jukka Zitting wrote:
> Hi,
>
> On Tue, Mar 19, 2013 at 5:09 PM, Marcel Reutegger<mr...@adobe.com> wrote:
>> AFAIU, NodeState was specifically designed to not require a path.
>
> Indeed. The main rationale for the design is to avoid the cost of
> having to support things like:
>
> assert state.getChildNode("foo").getName().equals("foo");
>
> That seems like a simple thing, but prevents optimizations like the
> EmptyNodeState or the ability of the SegmentMK to reuse existing
> content to avoid duplicate copies of the same data. Thus I'd be
> strongly against a proposal to introduce a method like
> NodeState.getPath().
>
> However, the design is also explicitly designed so that a particular
> NodeState implementation *can* keep track of the path by recording all
> the child node names used in successive getChildNode() calls until the
> target node has been reached. This is what for example the
> KernelNodeState does and also what we could do if we pushed (read)
> access checks below the NodeState interface.
>
> The only complication there, as already discussed before, is the
> inability of the current NodeState interface to handle cases where a
> child node is readable even if its parent is not.
>
> To address that case I was thinking that we could revise the
> getChildNode() contract to *always* return a new NodeState even if the
> named child node did not exist. Coupled with something like a new
> NodeState.exists() method (that would replace the current null checks
> on getChildNode() return values) this would allow us to access nodes
> down the hierarchy even if their parents don't "exist" for the current
> user.
>
> For example, to access a node like /foo/bar where the parent node /foo
> is not accessible, we'd have:
>
> NodeState root = ...;
> NodeState foo = root.getChildNode("foo");
> NodeState bar = foo.getChildNode("bar"); // no null checks needed!
>
> The existence of these nodes would then show up as:
>
> assert !foo.exists();
> assert bar.exists();
>
> A nice benefit of such a design would be that the difference between a
> node not existing at all or it being read-protected is entirely hidden
> from higher levels, so it'll be harder to accidentally leak
> information about the presence of a particular node.
>
> BR,
>
> Jukka Zitting
Re: NodeStates and security (Re: svn commit: r1458234 - in
/jackrabbit/oak/trunk: oak-core/src/main/java/org/apache/jackrabbit/oak/core/
oak-core/src/main/java/org/apache/jackrabbit/oak/kernel/ oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/version/
oak-core/src/main/java/o)
Posted by Jukka Zitting <ju...@gmail.com>.
Hi,
On Wed, Mar 20, 2013 at 12:24 PM, Jukka Zitting <ju...@gmail.com> wrote:
> To address that case I was thinking that we could revise the
> getChildNode() contract to *always* return a new NodeState even if the
> named child node did not exist. Coupled with something like a new
> NodeState.exists() method (that would replace the current null checks
> on getChildNode() return values) this would allow us to access nodes
> down the hierarchy even if their parents don't "exist" for the current
> user.
As discussed in OAK-709, I've now committed (see revision 1464516) an
initial version of such a refactoring.
The full patch is pretty large and in one way or another touches
pretty much all code that traverses NodeStates, so please make sure to
update your checkouts and watch out for potential conflicts with
pending changes.
BR,
Jukka Zitting