Skip to content

Commit 262b356

Browse files
mgao0mgao0
authored
Modify fixupACL for znode group acl use cases (#70)
When isX509ClientIdAsAclEnabled is set to true and using X509ZNodeGroupAclProvider as the auth provider, when client send a request, it goes through fixupACL and the results are: If server is dedicated server -> native zk fixupACL logic Client is plaintext port user (no "x509" scheme in authInfo) -> native zk fixupACL logic Client is authenticated by X509ZNodeGroupAclProvider but is not authorized to a domain -> set (x509, clientURI) as znode acl Client is authorized by X509ZNodeGroupAclProvider to a domain -> set (x509, domain) as znode acl Client is authorized by X509ZNodeGroupAclProvider as a cross domain component -> set (x509, domain) as znode acl Client is authorized by X509ZNodeGroupAclProvider as a super user -> set acl list as znode acl
1 parent 97a770a commit 262b356

File tree

5 files changed

+178
-59
lines changed

5 files changed

+178
-59
lines changed

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/PrepRequestProcessor.java‎

Lines changed: 50 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -1003,6 +1003,49 @@ public static List<ACL> fixupACL(String path, List<Id> authInfo, List<ACL> acls)
10031003
throw new KeeperException.InvalidACLException(path);
10041004
}
10051005
List<ACL> rv = new ArrayList<>();
1006+
1007+
// Overwrite the acl list for users (except super user) when the auth provider is
1008+
// X509ZnodeGroupAclProvider, and isX509ClientIdAsAclEnabled is true;
1009+
// Set x509 ZNode ACL as the client's corresponding domain name. This domain name denotes a
1010+
// ZNode group the client belongs to and can be derived from the client URI-domain mapping
1011+
// (UriDomainMappingHelper).
1012+
// Cases where such grouping by override applies are:
1013+
// Single domain user / cross domain components -> set (x509 : domainName) as znode ACL
1014+
// Users whose extracted clientId is not found in the ClientURIDomainMapping
1015+
// -> set (x509: clientURI) as znode ACL
1016+
// Examples that will not be handled by the "following logic" are:
1017+
// x509 super user, plaintext port clients, any user when dedicated server is enabled
1018+
// -> will go through original zk fixupACL logic
1019+
boolean isUserProvidedAclOverriden = false;
1020+
if (X509AuthenticationConfig.getInstance().isX509ClientIdAsAclEnabled()
1021+
&& X509AuthenticationConfig.getInstance().isX509ZnodeGroupAclEnabled()
1022+
&& !X509AuthenticationConfig.getInstance().isZnodeGroupAclDedicatedServerEnabled()) {
1023+
for (Id id : authInfo) {
1024+
boolean isX509 = id.getScheme().equals(X509AuthenticationUtil.X509_SCHEME);
1025+
boolean isX509CrossDomainComponent =
1026+
id.getScheme().equals(X509AuthenticationUtil.SUPERUSER_AUTH_SCHEME) && !id
1027+
.getId().equals(
1028+
X509AuthenticationConfig.getInstance().getZnodeGroupAclSuperUserId());
1029+
if (isX509 || isX509CrossDomainComponent) {
1030+
rv.add(new ACL(ZooDefs.Perms.ALL,
1031+
new Id(X509AuthenticationUtil.X509_SCHEME, id.getId())));
1032+
isUserProvidedAclOverriden = true;
1033+
}
1034+
}
1035+
// If the znode path contains open read access node path prefix, add (world:anyone, r)
1036+
if (X509AuthenticationConfig.getInstance().getZnodeGroupAclOpenReadAccessPathPrefixes()
1037+
.stream().anyMatch(path::startsWith)) {
1038+
rv.add(new ACL(ZooDefs.Perms.READ, ZooDefs.Ids.ANYONE_ID_UNSAFE));
1039+
}
1040+
if (isUserProvidedAclOverriden) {
1041+
// Only for users who are handled by the above logic, return the result,
1042+
// for others should continue to original fixupACL logic. This variable is necessary
1043+
// because if path is open read path, its open read ACL will be added to the list,
1044+
// regardless of user category, so rv's size won't be a good indicator here.
1045+
return rv;
1046+
}
1047+
}
1048+
10061049
for (ACL a : uniqacls) {
10071050
LOG.debug("Processing ACL: {}", a);
10081051
if (a == null) {
@@ -1012,44 +1055,21 @@ public static List<ACL> fixupACL(String path, List<Id> authInfo, List<ACL> acls)
10121055
if (id == null || id.getScheme() == null) {
10131056
throw new KeeperException.InvalidACLException(path);
10141057
}
1015-
if (id.getScheme().equals("world") && id.getId().equals("anyone") && !X509AuthenticationConfig
1016-
.getInstance().isX509ClientIdAsAclEnabled()) {
1058+
if (id.getScheme().equals("world") && id.getId().equals("anyone")) {
10171059
rv.add(a);
1018-
} else if (id.getScheme().equals("auth") || X509AuthenticationConfig
1019-
.getInstance().isX509ClientIdAsAclEnabled()) {
1060+
} else if (id.getScheme().equals("auth")) {
10201061
// This is the "auth" id, so we have to expand it to the
10211062
// authenticated ids of the requestor
10221063
boolean authIdValid = false;
10231064
for (Id cid : authInfo) {
1024-
// Special handling for super user / cross domain component use cases when X509ClientIdAsAcl is enabled
1025-
if (cid.getScheme().equals(X509AuthenticationUtil.SUPERUSER_AUTH_SCHEME)) {
1026-
// No need to check authentication provider because user has "super" scheme
1065+
ServerAuthenticationProvider ap = ProviderRegistry.getServerProvider(cid.getScheme());
1066+
if (ap == null) {
1067+
LOG.error("Missing AuthenticationProvider for {}", cid.getScheme());
1068+
} else if (ap.isAuthenticated()) {
10271069
authIdValid = true;
1028-
if (cid.getId().equals(
1029-
X509AuthenticationConfig.getInstance().getZnodeGroupAclSuperUserId())) {
1030-
// Allow operation and set the passed-in acl list as znode ACL for super user
1031-
rv.add(a);
1032-
} else {
1033-
// Allow operation and set domain name as znode ACL for cross domain components
1034-
rv.add(new ACL(a.getPerms(), new Id("x509", cid.getId())));
1035-
}
1036-
} else {
1037-
ServerAuthenticationProvider ap =
1038-
ProviderRegistry.getServerProvider(cid.getScheme());
1039-
if (ap == null) {
1040-
LOG.error("Missing AuthenticationProvider for {}", cid.getScheme());
1041-
} else if (ap.isAuthenticated()) {
1042-
authIdValid = true;
1043-
rv.add(new ACL(a.getPerms(), cid));
1044-
}
1070+
rv.add(new ACL(a.getPerms(), cid));
10451071
}
10461072
}
1047-
// If the znode path contains open read access node path prefix, add (world:anyone, r)
1048-
if (X509AuthenticationConfig.getInstance().getZnodeGroupAclOpenReadAccessPathPrefixes().stream()
1049-
.anyMatch(path::startsWith)) {
1050-
rv.add(new ACL(ZooDefs.Perms.READ, ZooDefs.Ids.ANYONE_ID_UNSAFE));
1051-
}
1052-
10531073
if (!authIdValid) {
10541074
throw new KeeperException.InvalidACLException(path);
10551075
}

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationConfig.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
import java.util.Set;
2424
import java.util.stream.Collectors;
2525
import com.google.common.annotations.VisibleForTesting;
26+
import org.apache.zookeeper.server.auth.znode.groupacl.X509ZNodeGroupAclProvider;
2627
import org.slf4j.Logger;
2728
import org.slf4j.LoggerFactory;
2829

@@ -343,6 +344,25 @@ private Set<String> loadCrossDomainAccessDomainNames() {
343344
.filter(str -> str.length() > 0).collect(Collectors.toSet());
344345
}
345346

347+
/**
348+
* Check if server dedicated domain config property is set
349+
* This check is only meaningful when x509 znode group acl feature is enabled
350+
* @return true if a domain is set as the server's dedicated domain; false if not set
351+
*/
352+
public boolean isZnodeGroupAclDedicatedServerEnabled() {
353+
return isX509ClientIdAsAclEnabled() && getZnodeGroupAclServerDedicatedDomain() != null
354+
&& !getZnodeGroupAclServerDedicatedDomain().isEmpty();
355+
}
356+
357+
/**
358+
* Check if x509 znode group acl feature is enabled
359+
* @return true if enabled; false if not
360+
*/
361+
public boolean isX509ZnodeGroupAclEnabled() {
362+
return ProviderRegistry
363+
.getServerProvider(X509AuthenticationUtil.X509_SCHEME) instanceof X509ZNodeGroupAclProvider;
364+
}
365+
346366
@VisibleForTesting
347367
public static void reset() {
348368
synchronized (X509AuthenticationConfig.class) {

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/X509AuthenticationUtil.java‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ public class X509AuthenticationUtil extends X509Util {
4646

4747
// Super user Auth Id scheme
4848
public static final String SUPERUSER_AUTH_SCHEME = "super";
49+
public static final String X509_SCHEME = "x509";
4950

5051
@Override
5152
protected String getConfigPrefix() {

‎zookeeper-server/src/main/java/org/apache/zookeeper/server/auth/znode/groupacl/X509ZNodeGroupAclProvider.java‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -136,12 +136,15 @@ public boolean isAuthenticated() {
136136

137137
@Override
138138
public boolean isValid(String id) {
139-
try {
140-
new X500Principal(id);
141-
return true;
142-
} catch (IllegalArgumentException e) {
143-
return false;
144-
}
139+
// Id can be of multiple format since it can be either a domain or a client URI,
140+
// so the check on Id format can be expensive.
141+
// For users, the Id to be set is extracted by server therefore it must be of valid format.
142+
// Validity of client URI is checked in X509AuthenticationUtil.getClientId when authentication
143+
// is done; validity of domain names are checked when client URI - domain pairs are set in
144+
// ClientUriDomainMapping.
145+
// Only superusers can manually set ACL, who we should trust.
146+
// Therefore it is necessary to perform this check.
147+
return true;
145148
}
146149

147150
/**
@@ -216,8 +219,7 @@ private void assignAuthInfo(ServerCnxn cnxn, String clientId, Set<String> domain
216219
// Check if user belongs to super user group
217220
if (clientId.equals(superUser)) {
218221
newAuthIds.add(new Id(X509AuthenticationUtil.SUPERUSER_AUTH_SCHEME, clientId));
219-
} else if (X509AuthenticationConfig.getInstance().getZnodeGroupAclServerDedicatedDomain() != null
220-
&& !X509AuthenticationConfig.getInstance().getZnodeGroupAclServerDedicatedDomain().isEmpty()) {
222+
} else if (X509AuthenticationConfig.getInstance().isZnodeGroupAclDedicatedServerEnabled()) {
221223
// If connection filtering feature is turned on, use connection filtering instead of normal authorization
222224
String serverNamespace = X509AuthenticationConfig.getInstance().getZnodeGroupAclServerDedicatedDomain();
223225
if (domains.contains(serverNamespace)) {

‎zookeeper-server/src/test/java/org/apache/zookeeper/server/auth/znode/groupacl/TestFixupACL.java‎

Lines changed: 97 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
package org.apache.zookeeper.server.auth.znode.groupacl;
2020

21+
import java.util.ArrayList;
2122
import java.util.Collections;
2223
import java.util.HashMap;
2324
import java.util.List;
@@ -29,59 +30,134 @@
2930
import org.apache.zookeeper.data.Id;
3031
import org.apache.zookeeper.server.PrepRequestProcessor;
3132
import org.apache.zookeeper.server.auth.X509AuthenticationConfig;
32-
import org.junit.AfterClass;
33+
import org.apache.zookeeper.server.auth.X509AuthenticationUtil;
34+
import org.junit.After;
3335
import org.junit.Assert;
34-
import org.junit.BeforeClass;
36+
import org.junit.Before;
3537
import org.junit.Test;
3638

3739
/**
3840
* This class test fixupACL method in {@link org.apache.zookeeper.server.PrepRequestProcessor}
3941
* under the znode group acl settings
4042
*/
4143
public class TestFixupACL {
44+
private final String testPath = "/zookeeper/testPath";
45+
private final String dedicatedDomain = "dedicatedDomain";
46+
private final String crossDomain = "crossDomain";
47+
private final List<Id> singleDomainAuthInfo =
48+
Collections.singletonList(new Id(X509AuthenticationUtil.X509_SCHEME, "domain"));
4249
private final List<Id> superUserAuthInfo =
4350
Collections.singletonList(new Id("super", "superUserId"));
4451
private final List<Id> crossDomainComponentAuthInfo =
45-
Collections.singletonList(new Id("super", "crossDomain"));
46-
private final List<ACL> aclList =
47-
Collections.singletonList(new ACL(ZooDefs.Perms.ALL, new Id("x509", "toBeAdded")));
48-
private final String testPath = "/zookeeper/testPath";
49-
private static final Map<String, String> SYSTEM_PROPERTIES = new HashMap<>();
52+
Collections.singletonList(new Id("super", crossDomain));
53+
private final List<Id> dedicatedDomainAuthInfo =
54+
Collections.singletonList(new Id(X509AuthenticationUtil.X509_SCHEME, dedicatedDomain));
55+
private final List<Id> multiIdAuthInfo = new ArrayList<>();
56+
57+
{
58+
multiIdAuthInfo.add(new Id(X509AuthenticationUtil.X509_SCHEME, "domain1"));
59+
multiIdAuthInfo.add(new Id(X509AuthenticationUtil.X509_SCHEME, "domain2"));
60+
}
61+
62+
private final List<Id> nonX509AuthInfo =
63+
Collections.singletonList(new Id("ip", "127.0.0.1:2183"));
64+
65+
private List<ACL> aclList = new ArrayList<>();
66+
67+
{
68+
aclList
69+
.add(new ACL(ZooDefs.Perms.ALL, new Id(X509AuthenticationUtil.X509_SCHEME, "toBeAdded")));
70+
aclList.add(new ACL(ZooDefs.Perms.ALL, ZooDefs.Ids.ANYONE_ID_UNSAFE));
71+
}
72+
73+
private final Map<String, String> SYSTEM_PROPERTIES = new HashMap<>();
5074

51-
static {
75+
{
76+
SYSTEM_PROPERTIES
77+
.put("zookeeper.authProvider.x509", X509ZNodeGroupAclProvider.class.getCanonicalName());
5278
SYSTEM_PROPERTIES.put(X509AuthenticationConfig.SET_X509_CLIENT_ID_AS_ACL, "true");
5379
SYSTEM_PROPERTIES
5480
.put(X509AuthenticationConfig.ZOOKEEPER_ZNODEGROUPACL_SUPERUSER_ID, "superUserId");
55-
SYSTEM_PROPERTIES.put(X509AuthenticationConfig.CROSS_DOMAIN_ACCESS_DOMAIN_NAME, "crossDomain");
5681
}
5782

58-
@BeforeClass
59-
public static void beforeClass() {
83+
@Before
84+
public void before() {
6085
SYSTEM_PROPERTIES.forEach(System::setProperty);
6186
}
6287

63-
@AfterClass
64-
public static void afterClass() {
88+
@After
89+
public void after() {
6590
SYSTEM_PROPERTIES.keySet().forEach(System::clearProperty);
91+
X509AuthenticationConfig.reset();
6692
}
6793

6894
@Test
6995
public void testCrossDomainComponents() throws KeeperException.InvalidACLException {
96+
// User provided ACL list will not be honored for cross domain components.
97+
// (x509 : domain name) will be set to the znodes
7098
List<ACL> returnedList =
7199
PrepRequestProcessor.fixupACL(testPath, crossDomainComponentAuthInfo, aclList);
72100
Assert.assertEquals(1, returnedList.size());
73-
Id returnedId = returnedList.get(0).getId();
74-
Assert.assertEquals("x509", returnedId.getScheme());
75-
Assert.assertEquals("crossDomain", returnedId.getId());
101+
Assert.assertTrue(returnedList.contains(
102+
new ACL(ZooDefs.Perms.ALL, new Id(X509AuthenticationUtil.X509_SCHEME, crossDomain))));
76103
}
77104

78105
@Test
79106
public void testSuperUserId() throws KeeperException.InvalidACLException {
80-
List<ACL> returnedList =
81-
PrepRequestProcessor.fixupACL(testPath, superUserAuthInfo, aclList);
107+
// User provided ACL list will be honored for super users.
108+
// No additional ACLs to be set to the znodes besides the user provided ACL list
109+
List<ACL> returnedList = PrepRequestProcessor.fixupACL(testPath, superUserAuthInfo, aclList);
110+
Assert.assertEquals(2, returnedList.size());
111+
Assert.assertTrue(returnedList.containsAll(aclList));
112+
}
113+
114+
@Test
115+
public void testSingleDomainUser() throws KeeperException.InvalidACLException {
116+
// User provided ACL list will not be honored for single domain users.
117+
// (x509 : domain name) will be set to the znodes
118+
List<ACL> returnedList = PrepRequestProcessor.fixupACL(testPath, singleDomainAuthInfo, aclList);
82119
Assert.assertEquals(1, returnedList.size());
83-
Id returnedId = returnedList.get(0).getId();
84-
Assert.assertEquals("x509", returnedId.getScheme());
85-
Assert.assertEquals("toBeAdded", returnedId.getId());
120+
Assert
121+
.assertTrue(returnedList.contains(new ACL(ZooDefs.Perms.ALL, singleDomainAuthInfo.get(0))));
122+
}
123+
124+
@Test
125+
public void testDedicatedServer() throws KeeperException.InvalidACLException {
126+
// Should route to original fixupACL logic
127+
System.setProperty(X509AuthenticationConfig.DEDICATED_DOMAIN, dedicatedDomain);
128+
List<ACL> returnedList =
129+
PrepRequestProcessor.fixupACL(testPath, dedicatedDomainAuthInfo, aclList);
130+
Assert.assertEquals(2, returnedList.size());
131+
Assert.assertTrue(returnedList.containsAll(aclList));
132+
System.clearProperty(X509AuthenticationConfig.DEDICATED_DOMAIN);
133+
}
134+
135+
@Test
136+
public void testOpenReadPaths() throws KeeperException.InvalidACLException {
137+
// Should add (world: anyone, r) to returned ACL list
138+
System.setProperty(X509AuthenticationConfig.OPEN_READ_ACCESS_PATH_PREFIX, testPath);
139+
List<ACL> returnedList = PrepRequestProcessor.fixupACL(testPath, singleDomainAuthInfo, aclList);
140+
Assert.assertEquals(2, returnedList.size());
141+
Assert.assertTrue(
142+
returnedList.contains(new ACL(ZooDefs.Perms.READ, ZooDefs.Ids.ANYONE_ID_UNSAFE)));
143+
System.clearProperty(X509AuthenticationConfig.OPEN_READ_ACCESS_PATH_PREFIX);
144+
}
145+
146+
@Test
147+
public void testMultiId() throws KeeperException.InvalidACLException {
148+
// User provided ACL list will not be honored for single domain users.
149+
// One ACL of (x509 : domain name) for each of the Id in authInfo will be set to the znodes
150+
List<ACL> returnedList = PrepRequestProcessor.fixupACL(testPath, multiIdAuthInfo, aclList);
151+
Assert.assertEquals(2, returnedList.size());
152+
multiIdAuthInfo
153+
.forEach(id -> Assert.assertTrue(returnedList.contains(new ACL(ZooDefs.Perms.ALL, id))));
154+
}
155+
156+
@Test
157+
public void testNonX509ZnodeGroupAclUser() throws KeeperException.InvalidACLException {
158+
// Should route to original fixupACL logic
159+
List<ACL> returnedList = PrepRequestProcessor.fixupACL(testPath, nonX509AuthInfo, aclList);
160+
Assert.assertEquals(2, returnedList.size());
161+
Assert.assertTrue(returnedList.containsAll(aclList));
86162
}
87163
}

0 commit comments

Comments
 (0)