You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@zookeeper.apache.org by GitBox <gi...@apache.org> on 2020/10/14 19:19:48 UTC
[GitHub] [zookeeper] ztzg opened a new pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
ztzg opened a new pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503
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.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r508575763
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslSuperUserTest.java
##########
@@ -56,42 +60,41 @@ public static void setupStatic() throws Exception {
fwriter.write(""
+ "Server {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
- + " user_super_duper=\"test\";\n"
+ + " user_super_duper=\"test\"\n"
+ + " user_other_super=\"test\";\n"
+ "};\n"
+ "Client {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ " username=\"super_duper\"\n"
+ " password=\"test\";\n"
+ "};"
+ + "OtherClient {\n"
+ + " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ + " username=\"other_super\"\n"
+ + " password=\"test\";\n"
+ + "};"
+ "\n");
fwriter.close();
oldLoginConfig = System.setProperty("java.security.auth.login.config", saslConfFile.getAbsolutePath());
- oldSuperUser = System.setProperty("zookeeper.superUser", "super_duper");
+ oldSuperUser = System.setProperty(ZooKeeperServer.SASL_SUPER_USER, "super_duper");
otherDigestUser = new Id("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
}
@AfterAll
public static void cleanupStatic() {
- if (oldAuthProvider != null) {
- System.setProperty("zookeeper.authProvider.1", oldAuthProvider);
- } else {
- System.clearProperty("zookeeper.authProvider.1");
- }
- oldAuthProvider = null;
-
- if (oldLoginConfig != null) {
- System.setProperty("java.security.auth.login.config", oldLoginConfig);
- } else {
- System.clearProperty("java.security.auth.login.config");
- }
- oldLoginConfig = null;
+ oldAuthProvider = restoreProperty("zookeeper.authProvider.1", oldAuthProvider);
+ oldClientConfigSection = restoreProperty(ZKClientConfig.LOGIN_CONTEXT_NAME_KEY, oldClientConfigSection);
+ oldLoginConfig = restoreProperty("java.security.auth.login.config", oldLoginConfig);
+ oldSuperUser = restoreProperty(ZooKeeperServer.SASL_SUPER_USER, oldSuperUser);
+ }
- if (oldSuperUser != null) {
- System.setProperty("zookeeper.superUser", oldSuperUser);
+ private static String restoreProperty(String property, String oldValue) {
+ if (oldValue != null) {
+ System.setProperty(property, oldValue);
} else {
- System.clearProperty("zookeeper.superUser");
+ System.clearProperty(property);
}
- oldSuperUser = null;
+ return null;
Review comment:
nit: this function should be void. AFAICS you don't use the return value anywhere (only in the @AfterAll method, but I don't see why it is needed to set to all those static variables to null) Or maybe I'm missing something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli closed pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
eolivelli closed pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r508575763
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslSuperUserTest.java
##########
@@ -56,42 +60,41 @@ public static void setupStatic() throws Exception {
fwriter.write(""
+ "Server {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
- + " user_super_duper=\"test\";\n"
+ + " user_super_duper=\"test\"\n"
+ + " user_other_super=\"test\";\n"
+ "};\n"
+ "Client {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ " username=\"super_duper\"\n"
+ " password=\"test\";\n"
+ "};"
+ + "OtherClient {\n"
+ + " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ + " username=\"other_super\"\n"
+ + " password=\"test\";\n"
+ + "};"
+ "\n");
fwriter.close();
oldLoginConfig = System.setProperty("java.security.auth.login.config", saslConfFile.getAbsolutePath());
- oldSuperUser = System.setProperty("zookeeper.superUser", "super_duper");
+ oldSuperUser = System.setProperty(ZooKeeperServer.SASL_SUPER_USER, "super_duper");
otherDigestUser = new Id("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
}
@AfterAll
public static void cleanupStatic() {
- if (oldAuthProvider != null) {
- System.setProperty("zookeeper.authProvider.1", oldAuthProvider);
- } else {
- System.clearProperty("zookeeper.authProvider.1");
- }
- oldAuthProvider = null;
-
- if (oldLoginConfig != null) {
- System.setProperty("java.security.auth.login.config", oldLoginConfig);
- } else {
- System.clearProperty("java.security.auth.login.config");
- }
- oldLoginConfig = null;
+ oldAuthProvider = restoreProperty("zookeeper.authProvider.1", oldAuthProvider);
+ oldClientConfigSection = restoreProperty(ZKClientConfig.LOGIN_CONTEXT_NAME_KEY, oldClientConfigSection);
+ oldLoginConfig = restoreProperty("java.security.auth.login.config", oldLoginConfig);
+ oldSuperUser = restoreProperty(ZooKeeperServer.SASL_SUPER_USER, oldSuperUser);
+ }
- if (oldSuperUser != null) {
- System.setProperty("zookeeper.superUser", oldSuperUser);
+ private static String restoreProperty(String property, String oldValue) {
+ if (oldValue != null) {
+ System.setProperty(property, oldValue);
} else {
- System.clearProperty("zookeeper.superUser");
+ System.clearProperty(property);
}
- oldSuperUser = null;
+ return null;
Review comment:
nit: this function should be void. AFAICS you don't use the return value anywhere (only in the `@AfterAll` method, but I don't see why it is needed to set to all those static variables to null) Or maybe I'm missing something?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat edited a comment on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat edited a comment on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-709899715
> 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-709899715
> 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 wow 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?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-708977567
> 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](https://github.com/apache/zookeeper/pull/1504#pullrequestreview-509087017), 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 `Id`s they issue should be included in audit logs.
What do you think?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-712401300
> I think it would worth to create a separate Jira for these issues indeed.
Now [ZOOKEEPER-3979](https://issues.apache.org/jira/browse/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…)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r505281836
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1627,6 +1628,22 @@ public void processPacket(ServerCnxn cnxn, ByteBuffer incomingBuffer) throws IOE
}
}
+ private static boolean isSaslSuperUser(String id) {
+ String superUser = System.getProperty(SASL_SUPER_USER);
+
+ if (superUser == null || id == null || id.isEmpty()) {
+ return false;
+ }
+
+ for (String superId : superUser.split(",")) {
Review comment:
I like this! (Particularly since there is a precedent. Sorta.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r505238251
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1627,6 +1628,22 @@ public void processPacket(ServerCnxn cnxn, ByteBuffer incomingBuffer) throws IOE
}
}
+ private static boolean isSaslSuperUser(String id) {
+ String superUser = System.getProperty(SASL_SUPER_USER);
+
+ if (superUser == null || id == null || id.isEmpty()) {
+ return false;
+ }
+
+ for (String superId : superUser.split(",")) {
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-711871833
@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)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
eolivelli commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r505238251
##########
File path: zookeeper-server/src/main/java/org/apache/zookeeper/server/ZooKeeperServer.java
##########
@@ -1627,6 +1628,22 @@ public void processPacket(ServerCnxn cnxn, ByteBuffer incomingBuffer) throws IOE
}
}
+ private static boolean isSaslSuperUser(String id) {
+ String superUser = System.getProperty(SASL_SUPER_USER);
+
+ if (superUser == null || id == null || id.isEmpty()) {
+ return false;
+ }
+
+ for (String superId : superUser.split(",")) {
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-708942365
@symat, @eolivelli: PTAL.
Regarding comma-separated identifiers: I see that [some existing code](https://github.com/apache/zookeeper/blob/05cd214a0cc9/zookeeper-server/src/main/java/org/apache/zookeeper/server/Request.java#L479) and [this fresh contribution](https://github.com/apache/zookeeper/pull/1504) already assume that IDs are comma-less.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] eolivelli commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
eolivelli commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-708951636
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] symat commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
symat commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-711701291
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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-724106239
Thanks!
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-717860618
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.)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#issuecomment-710549123
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` `AuthenticationProvider`s? Both seem to be hard-coded, even in `master`. Moreover, I could not spot any inbound control on `AuthPacket`s. (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.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [zookeeper] ztzg commented on a change in pull request #1503: ZOOKEEPER-3959: Add support for multiple SASL-authenticated super users
Posted by GitBox <gi...@apache.org>.
ztzg commented on a change in pull request #1503:
URL: https://github.com/apache/zookeeper/pull/1503#discussion_r509201772
##########
File path: zookeeper-server/src/test/java/org/apache/zookeeper/test/SaslSuperUserTest.java
##########
@@ -56,42 +60,41 @@ public static void setupStatic() throws Exception {
fwriter.write(""
+ "Server {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
- + " user_super_duper=\"test\";\n"
+ + " user_super_duper=\"test\"\n"
+ + " user_other_super=\"test\";\n"
+ "};\n"
+ "Client {\n"
+ " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ " username=\"super_duper\"\n"
+ " password=\"test\";\n"
+ "};"
+ + "OtherClient {\n"
+ + " org.apache.zookeeper.server.auth.DigestLoginModule required\n"
+ + " username=\"other_super\"\n"
+ + " password=\"test\";\n"
+ + "};"
+ "\n");
fwriter.close();
oldLoginConfig = System.setProperty("java.security.auth.login.config", saslConfFile.getAbsolutePath());
- oldSuperUser = System.setProperty("zookeeper.superUser", "super_duper");
+ oldSuperUser = System.setProperty(ZooKeeperServer.SASL_SUPER_USER, "super_duper");
otherDigestUser = new Id("digest", DigestAuthenticationProvider.generateDigest("jack:jack"));
}
@AfterAll
public static void cleanupStatic() {
- if (oldAuthProvider != null) {
- System.setProperty("zookeeper.authProvider.1", oldAuthProvider);
- } else {
- System.clearProperty("zookeeper.authProvider.1");
- }
- oldAuthProvider = null;
-
- if (oldLoginConfig != null) {
- System.setProperty("java.security.auth.login.config", oldLoginConfig);
- } else {
- System.clearProperty("java.security.auth.login.config");
- }
- oldLoginConfig = null;
+ oldAuthProvider = restoreProperty("zookeeper.authProvider.1", oldAuthProvider);
+ oldClientConfigSection = restoreProperty(ZKClientConfig.LOGIN_CONTEXT_NAME_KEY, oldClientConfigSection);
+ oldLoginConfig = restoreProperty("java.security.auth.login.config", oldLoginConfig);
+ oldSuperUser = restoreProperty(ZooKeeperServer.SASL_SUPER_USER, oldSuperUser);
+ }
- if (oldSuperUser != null) {
- System.setProperty("zookeeper.superUser", oldSuperUser);
+ private static String restoreProperty(String property, String oldValue) {
+ if (oldValue != null) {
+ System.setProperty(property, oldValue);
} else {
- System.clearProperty("zookeeper.superUser");
+ System.clearProperty(property);
}
- oldSuperUser = null;
+ return null;
Review comment:
No; you're right. I have cleaned that up.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org