ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users

[ztzg]

This implements a simple variant of the "multiple SASL-authenticated super users" functionality discussed on ticket ZOOKEEPER-3959.

With this patch, properties matching zookeeper.superUser.* are considered, in addition to zookeeper.superUser, to determine whether an authenticated ID should be given super privileges.

As before, super privileges remain a sharp tool and should be avoided where possible--but where necessary, allowing multiple IDs makes audit logs more useful.

(Many thanks to @eolivelli and @symat for their comments and suggestions.)

[ztzg]

@symat, @eolivelli: PTAL.

Regarding comma-separated identifiers: I see that some existing code and this fresh contribution already assume that IDs are comma-less.

[eolivelli]

In order to be 100% compatible....what about using a list of properties

• zookeeper.superUser=foo
• zookeeper.superUser.1=foo2
• zookeeper.superUser.2=foo3
• zookeeper.superUser.3=foo4

we will also be supporting comma as we won't have any special character

[ztzg]

I like this! (Particularly since there is a precedent. Sorta.)

[eolivelli]

This is only for debug/print purposes
https://github.com/apache/zookeeper/blob/05cd214a0cc9/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java#L479

[ztzg]

This is only for debug/print purposes
https://github.com/apache/zookeeper/blob/05cd214a0cc9/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java#L479

Unfortunately—and independently of the syntax issue—I'm afraid that this is not true anymore… These strings end up, notably, in audit logs—and those are likely to be parsed by downstream tools (as opposed to eyeballed by "human debuggers").

Worse: I just noticed that one could generate very misleading audit logs just by doing, e.g.:

addauth digest veryimportant@EXAMPLE.COM:whatever
create /dangerousnode

which results in entries such as:

2020-10-15 09:40:43,173 INFO audit.Log4jAuditLogger: session=0x100eefe34a40000	user=zkcli@CROSSTWINE.COM,veryimportant@EXAMPLE.COM,0:0:0:0:0:0:0:1	ip=0:0:0:0:0:0:0:1	operation=create	znode=/dangerousnode	znode_type=persistent	result=success

I added a similar comment on PR 1504, which, if merged, would further extend the reach of these strings.

It seems to me that getUsers should be amended to filter its output by scheme. This could, perhaps, be done by configuration—but we may also want to query the providers (e.g., at startup time) to know whether the Ids they issue should be included in audit logs.

What do you think?

[symat]

It seems to me that getUsers should be amended to filter its output by scheme.

Honestly I don't see this to be a huge problem... I don't know how frequently is it to configure digest user names in this "misleading" fashion. (Also I only saw SASL scheme on the secure clusters of our customers...). Anyway, I'm happy if these printouts gets improved.

But maybe we should stick to a backward-compatible solution, by making this configurable and behaving in the "old ways" by default.

What about making an other Jira for this?

[ztzg]

Hi @symat,

It seems to me that getUsers should be amended to filter its output by scheme.

Honestly I don't see this to be a huge problem...

I don't think it is a huge problem, but didn't want to stay silent as I noticed that audit logs could be spammed. I understand that nobody expects ZooKeeper to be able to withstand advanced malicious adversaries, but I'd rather plug the holes unless there is a serious reason not to.

I don't know how frequently is it to configure digest user names in this "misleading" fashion. (Also I only saw SASL scheme on the secure clusters of our customers...).

I may be missing something here, but isn't it currently impossible to disable the ip and digest AuthenticationProviders? Both seem to be hard-coded, even in master. Moreover, I could not spot any inbound control on AuthPackets. (I'd be happy to learn more if I'm wrong!)

Note that digest's handleAuthentication does not verify anything; it just associates id:digest(id:password) with the connection—the id part being left untouched. That means that a string 100% under client control can end up being written, unquoted and unescaped, to the audit log.

I just tried, it is even possible to insert newlines in there just using Ctrl+Q Ctrl+J in zkCli.sh:

addauth digest "fakeid^JTHIS REALLY SHOULDN'T BE THERE:whatever"

2020-10-16 21:42:06,546 INFO audit.Log4jAuditLogger: session=0x100f6b85af80001 user="fakeid
THIS REALLY SHOULDN'T BE THERE,zkcli@CROSSTWINE.COM,0:0:0:0:0:0:0:1 ip=0:0:0:0:0:0:0:1	operation=create	znode=/yolo4	znode_type=persistent	result=success

(I'd be curious to know/understand if you cannot reproduce that on the aforementioned clusters.)

Anyway, I'm happy if these printouts gets improved.

Sure.

But maybe we should stick to a backward-compatible solution, by making this configurable and behaving in the "old ways" by default.

Fine, but I'd include suggested mitigations (if/when we have some) in the docs.

What about making an other Jira for this?

Yeah, that is the plan, and I'm happy to provide patch(es). I just wanted to sanity check my findings.

[symat]

Thanks for your tests / feedback! I think it would worth to create a separate Jira for these issues indeed.

but I'd rather plug the holes unless there is a serious reason not to.

sure, I agree

it currently impossible to disable the ip and digest AuthenticationProviders

I don't know if this is a problem or not. Having the auth providers enabled doesn't mean you have to use them. In our secure clusters we use TLS for wire encryption and SASL/kerberos to identify the users. Then each client is protecting it's zNodes by setting the ACLs (enabling access only to the given kerberos principals). So even if a user authenticate itself with digest or IP, it won't get access to any zNodes.

On the other hand, I can accept that it should be configurable to disable these providers. They should remain enabled by default, but having two new config parameters to disable them make sense for me.

a string 100% under client control can end up being written, unquoted and unescaped, to the audit log.

This indeed seems to be wrong.

[symat]

@ztzg I just realized this PR is for branch-3.6. Would you mind create a PR to the master branch? And when it gets merged there, then we can backport it to 3.6.
(the best practice is to start with branch master and only create PRs to other branches if the cherry-pick is not clean)

[ztzg]

I think it would worth to create a separate Jira for these issues indeed.

Now ZOOKEEPER-3979.

Would you mind create a PR to the master branch?

Sure, will do. (I had already heard of that "best practice," and applied it exactly backwards. Oh well…)

[symat]

Looks good to me, thanks! +1

[ztzg]

Edited the PR's description as it ends up in merged commits. It used to be:

This implements the simplest variant of multiple SASL-authenticated super users as discussed on ZOOKEEPER-3959.

With it, a zookeeper.superUser property containing commas is interpreted as multiple IDs to match against. As before, a match causes the connection to be upgraded with the super scheme.

(Note that this is not a strictly backwards compatible change, as SASL IDs can, in theory, contain commas--e.g., in DIGEST-MD5; see https://tools.ietf.org/html/rfc2831#section-2.1.2. That is also true for Kerberos.)

[eolivelli]

good work, I left just one nit about a typo, it is worth to fix it, as no one else will every change it in the future

[eolivelli]

typo: "but current the implementation looks"
-> "but the implementation currently looks"

[ztzg]

Fixed; thanks!

[ztzg]

(I'll merge this if I get a green CI.)

[eolivelli]

We are used to not merge our own patches.
Let that a fellow committer merge the patch for you.
There is no hurry.
I will merge it tomorrow or on Monday if no one else does it

[ztzg]

We are used to not merge our own patches.

I see. Good policy. I was hesitating, and did not want this to look like I had abandoned it. There is no hurry at all, indeed; don't worry.

[eolivelli]

LGTM

[ztzg]

Thanks!