ZOOKEEPER-2590:exists() should check read ACL permission

[maoling]

• exist is a kind of read operation, so it should check read ACL permission. In the original release, only two operation don't check ACL permission(exist and getAcl) which is an omission. I saw getAcl had also amended it
• more details in the ZOOKEEPER-2590

[eolivelli]

Overall okay

What about adding a configuration parameter?
The check will be enabled by default.
Maybe this additional check may break (broken) applications.
We should leave a way to let the system work in production while fixing the broken application

[maoling]

@eolivelli
BTW, where is Jenkins these days? without Jenkins unit cases build, sometimes cannot make sure whether one patch has changed the behavior

[anmolnar]

@maoling @eolivelli Would you mind resurrecting this patch?

Patch looks good to me, but it's not enough alone to fix the original issue. Currently exists() doesn't check ACL, so nodes can be easily 'scanned' for existing even if the user doesn't have permission to the znode or the parent znode.

For instance:

/a - protected
/a/b - protected
/a/c - not protected

In this case an unauthorized user should not be able to see anything under /a, because we check ACL in getChildren(). With the current impl of exists(), he can scan through nodes /a/a, /a/b, /a/c, etc. to see which one exists. This still can be done with this patch, because he will get:

stat /a - Insuff perm
stat /a/b - Insuff perm
stat /a/c - exists
stat /a/d - Node does not exist

I suggest the following:

• Check nearest parent znode ACL in exists(), which is /a in my example and it's /b for stat /a/b/d/e/f/g/h. In this case the user will get Insuff perm for all the above requests.
• Put this behind a feature flag and let the default behaviour of exists() unprotected (current situation).

cc @phunt

[phunt]

That's good insight Andor. I think we should also document this, and perhaps consider changing the default behaviour of the feature flag at some point? Eg we could add as you suggest in 3.10, and change the default to "protected by default" in 3.11? That would give people sufficient warning to upgrade their client code.

[ztzg]

Hi @maoling, @eolivelli, @anmolnar,

I have forward-ported @maoling's patch here:

https://github.com/ztzg/zookeeper/tree/ZOOKEEPER-2590-exists-acl-check

Here is the specific commit:

ztzg@830aecd

(I have not opened another PR; this one is perfectly fine. @maoling, don't hesitate to update it with the contents of my tree.)

[anmolnar]

I changed my mind, implementing the parent ACL check doesn't make much sense. The same 'scan' issue can be achieved with get too:

/a - protected
/a/b - protected
/a/c - not protected
/a/d - not exist

Scanning with get:

[zk: localhost:2181(CONNECTED) 9] ls /a
Authentication is not valid : /a
[zk: localhost:2181(CONNECTED) 10] get /a/b
org.apache.zookeeper.KeeperException$NoAuthException: KeeperErrorCode =
NoAuth for /a/b
[zk: localhost:2181(CONNECTED) 11] get /a/c
null
[zk: localhost:2181(CONNECTED) 12] get /a/d
org.apache.zookeeper.KeeperException$NoNodeException: KeeperErrorCode =
NoNode for /a/d

There's no point fixing exists() recursively. ZooKeeper ACLs are not recursive means that such protection against scanning attacks cannot be provided.

I tend to accept @maoling 's patch as it is, because doing anything more will increase complexity without adding value.

[eolivelli]

Makes sense.
We just have to document the behavior

[anmolnar]

@eolivelli @ztzg @phunt

Unfortunately @maoling is not active on this patch, I suggest to fork his patch and create a new PR to move forward with the fix.

[eolivelli]

I agree. But I don't have cycles

[anmolnar]

I agree. But I don't have cycles

No worries, I'll do it.

[anmolnar]

Closing this PR in favor of #2093