You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ozone.apache.org by si...@apache.org on 2022/04/20 18:09:17 UTC
[ozone] branch HDDS-4944 updated: HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant (#3288)
This is an automated email from the ASF dual-hosted git repository.
siyao pushed a commit to branch HDDS-4944
in repository https://gitbox.apache.org/repos/asf/ozone.git
The following commit(s) were added to refs/heads/HDDS-4944 by this push:
new 6044ef607d HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant (#3288)
6044ef607d is described below
commit 6044ef607dcc38b65f8accb099d3ecc6280b0d6c
Author: Siyao Meng <50...@users.noreply.github.com>
AuthorDate: Wed Apr 20 11:09:13 2022 -0700
HDDS-6566. [Multi-Tenant] Fix a permission check bug that prevents non-delegated admins from assigning/revoking users to/from the tenant (#3288)
---
.../hadoop/ozone/shell/TestOzoneTenantShell.java | 188 ++++++++++++++++++++-
.../hadoop/ozone/om/OMMultiTenantManager.java | 9 +-
.../hadoop/ozone/om/OMMultiTenantManagerImpl.java | 5 +-
.../s3/tenant/OMTenantAssignAdminRequest.java | 4 +-
.../tenant/OMTenantAssignUserAccessIdRequest.java | 4 +-
.../s3/tenant/OMTenantRevokeAdminRequest.java | 5 +-
.../tenant/OMTenantRevokeUserAccessIdRequest.java | 4 +-
.../s3/security/TestS3GetSecretRequest.java | 3 +-
8 files changed, 206 insertions(+), 16 deletions(-)
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java
index 93e4fedba3..0db0c69e45 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/shell/TestOzoneTenantShell.java
@@ -798,10 +798,11 @@ public class TestOzoneTenantShell {
return null;
});
- // Once we make bob an admin of this tenant, set secret should succeed
+ // Once we make bob an admin of this tenant (non-delegated admin permission
+ // is sufficient in this case), set secret should succeed
executeHA(tenantShell, new String[] {"user", "assign-admin",
tenantName + "$" + ugiBob.getShortUserName(),
- "--tenant=" + tenantName, "--delegated=true"});
+ "--tenant=" + tenantName, "--delegated=false"});
checkOutput(out, "", true);
checkOutput(err, "Assigned admin", false);
@@ -842,4 +843,187 @@ public class TestOzoneTenantShell {
checkOutput(out, "", true);
checkOutput(err, "", true);
}
+
+ @Test
+ @SuppressWarnings("methodlength")
+ public void testTenantAdminOperations()
+ throws IOException, InterruptedException {
+
+ final String tenantName = "tenant-test-tenant-admin-ops";
+ final UserGroupInformation ugiAlice = UserGroupInformation
+ .createUserForTesting("alice", new String[] {"usergroup"});
+ final UserGroupInformation ugiBob = UserGroupInformation
+ .createUserForTesting("bob", new String[] {"usergroup"});
+
+ // Preparation
+
+ // Create test tenant
+ executeHA(tenantShell, new String[] {"create", tenantName});
+ checkOutput(out, "Created tenant '" + tenantName + "'.\n", true);
+ checkOutput(err, "", true);
+
+ // Assign alice and bob as tenant users
+ executeHA(tenantShell, new String[] {
+ "user", "assign", "alice", "--tenant=" + tenantName});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$alice'\n" +
+ "export AWS_SECRET_ACCESS_KEY='", false);
+ checkOutput(err, "Assigned 'alice' to '" + tenantName + "'" +
+ " with accessId '" + tenantName + "$alice'.\n", true);
+
+ executeHA(tenantShell, new String[] {
+ "user", "assign", "bob", "--tenant=" + tenantName});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$bob'\n" +
+ "export AWS_SECRET_ACCESS_KEY='", false);
+ checkOutput(err, "Assigned 'bob' to '" + tenantName + "'" +
+ " with accessId '" + tenantName + "$bob'.\n", true);
+
+ // Make alice a delegated tenant admin
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$" + ugiAlice.getShortUserName(),
+ "--tenant=" + tenantName, "--delegated=true"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Assigned admin", false);
+
+ // Make bob a non-delegated tenant admin
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$" + ugiBob.getShortUserName(),
+ "--tenant=" + tenantName, "--delegated=false"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Assigned admin", false);
+
+ // Start test matrix
+
+ // As a tenant delegated admin, alice can:
+ // - Assign and revoke user accessIds
+ // - Assign and revoke tenant admins
+ // - Set secret
+ ugiAlice.doAs((PrivilegedExceptionAction<Void>) () -> {
+ // Assign carol as a new tenant user
+ executeHA(tenantShell, new String[] {
+ "user", "assign", "carol", "--tenant=" + tenantName});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$carol'\n"
+ + "export AWS_SECRET_ACCESS_KEY='", false);
+ checkOutput(err, "Assigned 'carol' to '" + tenantName + "' with accessId"
+ + " '" + tenantName + "$carol'.\n", true);
+
+ // Set secret should work
+ executeHA(tenantShell, new String[] {
+ "user", "setsecret", tenantName + "$alice",
+ "--secret=somesecret2", "--export"});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$alice'\n" +
+ "export AWS_SECRET_ACCESS_KEY='somesecret2'\n", true);
+ checkOutput(err, "", true);
+
+ // Make carol a tenant delegated tenant admin
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$carol",
+ "--tenant=" + tenantName, "--delegated=true"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Assigned admin", false);
+
+ // Revoke carol's tenant admin privilege
+ executeHA(tenantShell, new String[] {"user", "revoke-admin",
+ tenantName + "$carol"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked admin", false);
+
+ // Make carol a tenant non-delegated tenant admin
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$carol",
+ "--tenant=" + tenantName, "--delegated=false"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Assigned admin", false);
+
+ // Revoke carol's tenant admin privilege
+ executeHA(tenantShell, new String[] {"user", "revoke-admin",
+ tenantName + "$carol"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked admin", false);
+
+ // Revoke carol's accessId from this tenant
+ executeHA(tenantShell, new String[] {
+ "user", "revoke", tenantName + "$carol"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked accessId", false);
+ return null;
+ });
+
+ // As a tenant non-delegated admin, bob can:
+ // - Assign and revoke user accessIds
+ // - Set secret
+ //
+ // But bob can't:
+ // - Assign and revoke tenant admins
+ ugiBob.doAs((PrivilegedExceptionAction<Void>) () -> {
+ // Assign carol as a new tenant user
+ executeHA(tenantShell, new String[] {
+ "user", "assign", "carol", "--tenant=" + tenantName});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$carol'\n"
+ + "export AWS_SECRET_ACCESS_KEY='", false);
+ checkOutput(err, "Assigned 'carol' to '" + tenantName + "' with accessId"
+ + " '" + tenantName + "$carol'.\n", true);
+
+ // Set secret should work, even for a non-delegated admin
+ executeHA(tenantShell, new String[] {
+ "user", "setsecret", tenantName + "$alice",
+ "--secret=somesecret2", "--export"});
+ checkOutput(out, "export AWS_ACCESS_KEY_ID='" + tenantName + "$alice'\n" +
+ "export AWS_SECRET_ACCESS_KEY='somesecret2'\n", true);
+ checkOutput(err, "", true);
+
+ // Attempt to make carol a tenant delegated tenant admin, should fail
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$carol",
+ "--tenant=" + tenantName, "--delegated=true"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Failed to assign admin", false);
+
+ // Attempt to make carol a tenant non-delegated tenant admin, should fail
+ executeHA(tenantShell, new String[] {"user", "assign-admin",
+ tenantName + "$carol",
+ "--tenant=" + tenantName, "--delegated=false"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Failed to assign admin", false);
+
+ // Attempt to revoke tenant admin, should fail at the permission check
+ executeHA(tenantShell, new String[] {"user", "revoke-admin",
+ tenantName + "$carol"});
+ checkOutput(out, "", true);
+ checkOutput(err, "User 'bob' is neither an Ozone admin nor a delegated "
+ + "admin of tenant", false);
+
+ // Revoke carol's accessId from this tenant
+ executeHA(tenantShell, new String[] {
+ "user", "revoke", tenantName + "$carol"});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked accessId", false);
+ return null;
+ });
+
+ // Clean up
+ executeHA(tenantShell, new String[] {"user", "revoke-admin",
+ tenantName + "$" + ugiAlice.getShortUserName()});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked admin", false);
+
+ executeHA(tenantShell, new String[] {
+ "user", "revoke", tenantName + "$" + ugiAlice.getShortUserName()});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked accessId", false);
+
+ executeHA(tenantShell, new String[] {"user", "revoke-admin",
+ tenantName + "$" + ugiBob.getShortUserName()});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked admin", false);
+
+ executeHA(tenantShell, new String[] {
+ "user", "revoke", tenantName + "$" + ugiBob.getShortUserName()});
+ checkOutput(out, "", true);
+ checkOutput(err, "Revoked accessId", false);
+
+ executeHA(tenantShell, new String[] {"delete", tenantName});
+ checkOutput(out, "Deleted tenant '" + tenantName + "'.\n", false);
+ checkOutput(err, "", true);
+ deleteVolume(tenantName);
+ }
}
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManager.java
index f576ed073f..3554ab6f29 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManager.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManager.java
@@ -201,11 +201,16 @@ public interface OMMultiTenantManager {
void checkAdmin() throws OMException;
/**
- * Check if caller is an Ozone cluster admin or tenant (delegated) admin.
+ * Check if caller is a tenant admin of the specified tenant.
+ * Ozone admins will always pass this check.
* Throws PERMISSION_DENIED if the check failed.
+ * @param tenantId tenant name
+ * @param delegated if set to true, only delegated tenant admins can pass this
+ * check; if false, both delegated and non-delegated tenant
+ * admins will pass this check.
* @throws OMException PERMISSION_DENIED
*/
- void checkTenantAdmin(String tenantId) throws OMException;
+ void checkTenantAdmin(String tenantId, boolean delegated) throws OMException;
/**
* Check if the tenantId exists in the table, throws TENANT_NOT_FOUND if not.
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
index 8bbd98be2e..0bc54a6ae4 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMMultiTenantManagerImpl.java
@@ -587,10 +587,11 @@ public class OMMultiTenantManagerImpl implements OMMultiTenantManager {
}
@Override
- public void checkTenantAdmin(String tenantId) throws OMException {
+ public void checkTenantAdmin(String tenantId, boolean delegated)
+ throws OMException {
final UserGroupInformation ugi = ProtobufRpcEngine.Server.getRemoteUser();
- if (!isTenantAdmin(ugi, tenantId, true)) {
+ if (!isTenantAdmin(ugi, tenantId, delegated)) {
throw new OMException("User '" + ugi.getUserName() +
"' is neither an Ozone admin nor a delegated admin of tenant '" +
tenantId + "'.", OMException.ResultCodes.PERMISSION_DENIED);
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignAdminRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignAdminRequest.java
index 8a3cd7e108..f9a6a9e467 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignAdminRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignAdminRequest.java
@@ -96,8 +96,8 @@ public class OMTenantAssignAdminRequest extends OMClientRequest {
multiTenantManager.checkTenantExistence(tenantId);
- // Caller should be an Ozone admin or this tenant's delegated admin
- multiTenantManager.checkTenantAdmin(tenantId);
+ // Caller should be an Ozone admin, or a tenant delegated admin
+ multiTenantManager.checkTenantAdmin(tenantId, true);
OmDBAccessIdInfo accessIdInfo = ozoneManager.getMetadataManager()
.getTenantAccessIdTable().get(accessId);
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
index e30913620a..509d5de6ce 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java
@@ -112,8 +112,8 @@ public class OMTenantAssignUserAccessIdRequest extends OMClientRequest {
final String tenantId = request.getTenantId();
- // Caller should be an Ozone admin or tenant delegated admin
- ozoneManager.getMultiTenantManager().checkTenantAdmin(tenantId);
+ // Caller should be an Ozone admin, or at least a tenant non-delegated admin
+ ozoneManager.getMultiTenantManager().checkTenantAdmin(tenantId, false);
final String userPrincipal = request.getUserPrincipal();
final String accessId = request.getAccessId();
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeAdminRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeAdminRequest.java
index eb74972b74..7d112135a7 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeAdminRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeAdminRequest.java
@@ -94,11 +94,10 @@ public class OMTenantRevokeAdminRequest extends OMClientRequest {
assert (!StringUtils.isEmpty(tenantId));
}
- // Sanity check
multiTenantManager.checkTenantExistence(tenantId);
- // Caller should be an Ozone admin or this tenant's delegated admin
- multiTenantManager.checkTenantAdmin(tenantId);
+ // Caller should be an Ozone admin, or a tenant delegated admin
+ multiTenantManager.checkTenantAdmin(tenantId, true);
OmDBAccessIdInfo accessIdInfo = ozoneManager.getMetadataManager()
.getTenantAccessIdTable().get(accessId);
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeUserAccessIdRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeUserAccessIdRequest.java
index f83d301a09..e779ec8a1a 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeUserAccessIdRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantRevokeUserAccessIdRequest.java
@@ -115,8 +115,8 @@ public class OMTenantRevokeUserAccessIdRequest extends OMClientRequest {
// Sanity check
multiTenantManager.checkTenantExistence(tenantId);
- // Caller should be an Ozone admin or this tenant's delegated admin
- multiTenantManager.checkTenantAdmin(tenantId);
+ // Caller should be an Ozone admin, or at least a tenant non-delegated admin
+ multiTenantManager.checkTenantAdmin(tenantId, false);
if (accessIdInfo.getIsAdmin()) {
throw new OMException("accessId '" + accessId + "' is a tenant admin of "
diff --git a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java
index a78762980d..4864de95b1 100644
--- a/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/s3/security/TestS3GetSecretRequest.java
@@ -329,7 +329,8 @@ public class TestS3GetSecretRequest {
// This effectively makes alice an admin.
when(ozoneManager.isAdmin(ugiAlice)).thenReturn(true);
- when(omMultiTenantManager.isTenantAdmin(ugiAlice, TENANT_ID, true))
+ // Make alice a non-delegated admin
+ when(omMultiTenantManager.isTenantAdmin(ugiAlice, TENANT_ID, false))
.thenReturn(true);
// Init LayoutVersionManager to prevent NPE in checkLayoutFeature
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@ozone.apache.org
For additional commands, e-mail: commits-help@ozone.apache.org