From d31a24f0d532b9569558b8ec8fc556f570597334 Mon Sep 17 00:00:00 2001
From: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Date: Tue, 11 May 2021 21:58:13 -0700
Subject: [PATCH 1/5] HDDS-5206. Support revoking S3 secret in Ozone CLI

---
 .../org/apache/hadoop/ozone/OzoneConsts.java  |   1 +
 .../hadoop/ozone/client/ObjectStore.java      |   4 +
 .../ozone/client/protocol/ClientProtocol.java |   8 +
 .../hadoop/ozone/client/rpc/RpcClient.java    |  11 ++
 .../java/org/apache/hadoop/ozone/OmUtils.java |   1 +
 .../apache/hadoop/ozone/audit/OMAction.java   |   3 +-
 .../om/protocol/OzoneManagerProtocol.java     |   9 ++
 ...ManagerProtocolClientSideTranslatorPB.java |  11 ++
 .../src/main/proto/OmClientProtocol.proto     |   8 +
 .../apache/hadoop/ozone/om/OzoneManager.java  |   9 ++
 .../ratis/utils/OzoneManagerRatisUtils.java   |   3 +
 .../s3/security/S3RevokeSecretRequest.java    | 149 ++++++++++++++++++
 .../s3/security/S3RevokeSecretResponse.java   |  58 +++++++
 .../ozone/shell/s3/RevokeS3SecretHandler.java |  63 ++++++++
 .../apache/hadoop/ozone/shell/s3/S3Shell.java |   3 +-
 15 files changed, 339 insertions(+), 2 deletions(-)
 create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
 create mode 100644 hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
 create mode 100644 hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java

diff --git a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
index efaeb2367640..ee5912d302d6 100644
--- a/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
+++ b/hadoop-hdds/common/src/main/java/org/apache/hadoop/ozone/OzoneConsts.java
@@ -323,6 +323,7 @@ private OzoneConsts() {
   public static final String MAX_PARTS = "maxParts";
   public static final String S3_BUCKET = "s3Bucket";
   public static final String S3_GETSECRET_USER = "S3GetSecretUser";
+  public static final String S3_REVOKESECRET_USER = "S3RevokeSecretUser";
   public static final String RENAMED_KEYS_MAP = "renamedKeysMap";
   public static final String UNRENAMED_KEYS_MAP = "unRenamedKeysMap";
   public static final String MULTIPART_UPLOAD_PART_NUMBER = "partNumber";
diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
index fd3d214a90db..1f819ac91f92 100644
--- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
+++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/ObjectStore.java
@@ -156,6 +156,10 @@ public S3SecretValue getS3Secret(String kerberosID) throws IOException {
     return proxy.getS3Secret(kerberosID);
   }
 
+  public void revokeS3Secret(String kerberosID) throws IOException {
+    proxy.revokeS3Secret(kerberosID);
+  }
+
   /**
    * Returns Iterator to iterate over all the volumes in object store.
    * The result can be restricted using volume prefix, will return all
diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
index cc9335506d70..32ab7990e3ac 100644
--- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
+++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
@@ -508,6 +508,14 @@ void cancelDelegationToken(Token<OzoneTokenIdentifier> token)
    */
   S3SecretValue getS3Secret(String kerberosID) throws IOException;
 
+  /**
+   * Revoke S3 Secret of given kerberos user.
+   * @param kerberosID
+   * @return S3SecretValue
+   * @throws IOException
+   */
+  void revokeS3Secret(String kerberosID) throws IOException;
+
   /**
    * Get KMS client provider.
    * @return KMS client provider.
diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
index 718e254d1576..b9c55fbff0e2 100644
--- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
+++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/rpc/RpcClient.java
@@ -590,6 +590,17 @@ public S3SecretValue getS3Secret(String kerberosID) throws IOException {
     return ozoneManagerClient.getS3Secret(kerberosID);
   }
 
+  /**
+   * {@inheritDoc}
+   */
+  @Override
+  public void revokeS3Secret(String kerberosID) throws IOException {
+    Preconditions.checkArgument(Strings.isNotBlank(kerberosID),
+            "kerberosID cannot be null or empty.");
+
+    ozoneManagerClient.revokeS3Secret(kerberosID);
+  }
+
   @Override
   public void setBucketVersioning(
       String volumeName, String bucketName, Boolean versioning)
diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
index c389becf6b8f..577146db8cde 100644
--- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/OmUtils.java
@@ -289,6 +289,7 @@ public static boolean isReadOnly(
     case PurgeKeys:
     case RecoverTrash:
     case DeleteOpenKeys:
+    case RevokeS3Secret:
       return false;
     default:
       LOG.error("CmdType {} is not categorized as readOnly or not.", cmdType);
diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/audit/OMAction.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/audit/OMAction.java
index 3480063d1323..f969e75e0fd7 100644
--- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/audit/OMAction.java
+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/audit/OMAction.java
@@ -70,7 +70,8 @@ public enum OMAction implements AuditAction {
   LOOKUP_FILE,
   LIST_STATUS,
 
-  GET_S3_SECRET;
+  GET_S3_SECRET,
+  REVOKE_S3_SECRET;
 
   @Override
   public String getAction() {
diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
index 68bfa4f9d8fa..31efbefd70f7 100644
--- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
@@ -372,6 +372,7 @@ OmMultipartUploadListParts listParts(String volumeName, String bucketName,
    */
   OmMultipartUploadList listMultipartUploads(String volumeName,
       String bucketName, String prefix) throws IOException;
+
   /**
    * Gets s3Secret for given kerberos user.
    * @param kerberosID
@@ -380,6 +381,14 @@ OmMultipartUploadList listMultipartUploads(String volumeName,
    */
   S3SecretValue getS3Secret(String kerberosID) throws IOException;
 
+  /**
+   * Revokes s3Secret of given kerberos user.
+   * @param kerberosID
+   * @return S3SecretValue
+   * @throws IOException
+   */
+  void revokeS3Secret(String kerberosID) throws IOException;
+
   /**
    * OzoneFS api to get file status for an entry.
    *
diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
index 43d72b90dfda..b3d3514cfccf 100644
--- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocolPB/OzoneManagerProtocolClientSideTranslatorPB.java
@@ -128,6 +128,7 @@
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RenameKeyRequest;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RenameKeysRequest;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RenewDelegationTokenResponseProto;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RevokeS3SecretRequest;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.ServiceListRequest;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.ServiceListResponse;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.SetAclRequest;
@@ -848,7 +849,17 @@ public S3SecretValue getS3Secret(String kerberosID) throws IOException {
         .getGetS3SecretResponse();
 
     return S3SecretValue.fromProtobuf(resp.getS3Secret());
+  }
 
+  @Override
+  public void revokeS3Secret(String kerberosID) throws IOException {
+    RevokeS3SecretRequest request = RevokeS3SecretRequest.newBuilder()
+            .setKerberosID(kerberosID)
+            .build();
+    OMRequest omRequest = createOMRequest(Type.RevokeS3Secret)
+            .setRevokeS3SecretRequest(request)
+            .build();
+    handleError(submitRequest(omRequest));
   }
 
   /**
diff --git a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
index 7269c9a68ff8..a10bb67b7b52 100644
--- a/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
+++ b/hadoop-ozone/interface-client/src/main/proto/OmClientProtocol.proto
@@ -93,6 +93,8 @@ enum Type {
 
   ListTrash = 91;
   RecoverTrash = 92;
+
+  RevokeS3Secret = 93;
 }
 
 message OMRequest {
@@ -165,6 +167,8 @@ message OMRequest {
 
   optional ListTrashRequest                 listTrashRequest               = 91;
   optional RecoverTrashRequest              RecoverTrashRequest            = 92;
+
+  optional RevokeS3SecretRequest            RevokeS3SecretRequest          = 93;
 }
 
 message OMResponse {
@@ -1202,6 +1206,10 @@ message GetS3SecretResponse {
     required S3Secret s3Secret = 2;
 }
 
+message RevokeS3SecretRequest {
+    required string kerberosID = 1;
+}
+
 /**
   This will be used internally by OM to replicate S3 Secret across quorum of
   OM's.
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
index 6c18e0edf5fb..de6180f55030 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
@@ -2698,6 +2698,15 @@ public S3SecretValue getS3Secret(String kerberosID) throws IOException {
     return s3SecretManager.getS3Secret(kerberosID);
   }
 
+  @Override
+  /**
+   * {@inheritDoc}
+   */
+  public void revokeS3Secret(String kerberosID) {
+    throw new UnsupportedOperationException("OzoneManager does not require " +
+            "this to be implemented. As write requests use a new approach");
+  }
+
   @Override
   public OmMultipartInfo initiateMultipartUpload(OmKeyArgs keyArgs) throws
       IOException {
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java
index 14657c6576ce..29a0328eb38c 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/ratis/utils/OzoneManagerRatisUtils.java
@@ -53,6 +53,7 @@
 import org.apache.hadoop.ozone.om.request.s3.multipart.S3MultipartUploadCommitPartRequest;
 import org.apache.hadoop.ozone.om.request.s3.multipart.S3MultipartUploadCompleteRequest;
 import org.apache.hadoop.ozone.om.request.s3.security.S3GetSecretRequest;
+import org.apache.hadoop.ozone.om.request.s3.security.S3RevokeSecretRequest;
 import org.apache.hadoop.ozone.om.request.security.OMCancelDelegationTokenRequest;
 import org.apache.hadoop.ozone.om.request.security.OMGetDelegationTokenRequest;
 import org.apache.hadoop.ozone.om.request.security.OMRenewDelegationTokenRequest;
@@ -156,6 +157,8 @@ public static OMClientRequest createClientRequest(OMRequest omRequest) {
       return new S3GetSecretRequest(omRequest);
     case RecoverTrash:
       return new OMTrashRecoverRequest(omRequest);
+    case RevokeS3Secret:
+      return new S3RevokeSecretRequest(omRequest);
     default:
       throw new IllegalStateException("Unrecognized write command " +
           "type request" + cmdType);
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
new file mode 100644
index 000000000000..759bf4808b27
--- /dev/null
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
@@ -0,0 +1,149 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.request.s3.security;
+
+import org.apache.hadoop.ipc.ProtobufRpcEngine;
+import org.apache.hadoop.ozone.OzoneConsts;
+import org.apache.hadoop.ozone.audit.OMAction;
+import org.apache.hadoop.ozone.om.OMMetadataManager;
+import org.apache.hadoop.ozone.om.OzoneManager;
+import org.apache.hadoop.ozone.om.exceptions.OMException;
+import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
+import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
+import org.apache.hadoop.ozone.om.request.OMClientRequest;
+import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.om.response.s3.security.S3RevokeSecretResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.RevokeS3SecretRequest;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
+import static org.apache.hadoop.ozone.om.lock.OzoneManagerLock.Resource.S3_SECRET_LOCK;
+
+/**
+ * Handles RevokeS3Secret request.
+ */
+public class S3RevokeSecretRequest extends OMClientRequest {
+
+  private static final Logger LOG =
+      LoggerFactory.getLogger(S3RevokeSecretRequest.class);
+
+  public S3RevokeSecretRequest(OMRequest omRequest) {
+    super(omRequest);
+  }
+
+  @Override
+  public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    final RevokeS3SecretRequest s3RevokeSecretRequest =
+        getOmRequest().getRevokeS3SecretRequest();
+    final String kerberosID = s3RevokeSecretRequest.getKerberosID();
+    final UserGroupInformation user = ProtobufRpcEngine.Server.getRemoteUser();
+
+    if (!user.getUserName().equals(kerberosID)) {
+      throw new OMException("User mismatch. Requested user name is " +
+          "mismatched " + kerberosID +", with current user " +
+          user.getUserName(), OMException.ResultCodes.USER_MISMATCH);
+    }
+
+    final RevokeS3SecretRequest revokeS3SecretRequest =
+            RevokeS3SecretRequest.newBuilder()
+                    .setKerberosID(kerberosID).build();
+
+    OMRequest.Builder omRequest = OMRequest.newBuilder()
+        .setUserInfo(getUserInfo())
+        .setRevokeS3SecretRequest(revokeS3SecretRequest)
+        .setCmdType(getOmRequest().getCmdType())
+        .setClientId(getOmRequest().getClientId());
+
+    if (getOmRequest().hasTraceID()) {
+      omRequest.setTraceID(getOmRequest().getTraceID());
+    }
+
+    return omRequest.build();
+  }
+
+  @Override
+  public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
+      long transactionLogIndex,
+      OzoneManagerDoubleBufferHelper ozoneManagerDoubleBufferHelper) {
+
+    OMClientResponse omClientResponse = null;
+    OMResponse.Builder omResponse =
+            OmResponseUtil.getOMResponseBuilder(getOmRequest());
+    boolean acquiredLock = false;
+    IOException exception = null;
+    OMMetadataManager omMetadataManager = ozoneManager.getMetadataManager();
+
+    final RevokeS3SecretRequest revokeS3SecretRequest =
+        getOmRequest().getRevokeS3SecretRequest();
+    String kerberosID = revokeS3SecretRequest.getKerberosID();
+    try {
+      acquiredLock =
+         omMetadataManager.getLock().acquireWriteLock(S3_SECRET_LOCK,
+             kerberosID);
+
+      // Remove if entry exists in table
+      if (omMetadataManager.getS3SecretTable().isExist(kerberosID)) {
+//        omMetadataManager.getS3SecretTable().delete(kerberosID);
+        omClientResponse = new S3RevokeSecretResponse(
+                new S3SecretValue(kerberosID, null),
+                omResponse.setStatus(Status.OK).build());
+      } else {
+        omClientResponse = new S3RevokeSecretResponse(null,
+                omResponse.setStatus(Status.S3_SECRET_NOT_FOUND).build());
+      }
+    } catch (IOException ex) {
+      exception = ex;
+      omClientResponse = new S3RevokeSecretResponse(null,
+          createErrorOMResponse(omResponse, ex));
+    } finally {
+      addResponseToDoubleBuffer(transactionLogIndex, omClientResponse,
+          ozoneManagerDoubleBufferHelper);
+      if (acquiredLock) {
+        omMetadataManager.getLock().releaseWriteLock(S3_SECRET_LOCK,
+            kerberosID);
+      }
+    }
+
+    Map<String, String> auditMap = new HashMap<>();
+    auditMap.put(OzoneConsts.S3_REVOKESECRET_USER, kerberosID);
+    auditLog(ozoneManager.getAuditLogger(), buildAuditMessage(
+        OMAction.REVOKE_S3_SECRET, auditMap,
+        exception, getOmRequest().getUserInfo()));
+
+    if (exception == null) {
+      if (omResponse.getStatus() == Status.OK) {
+        LOG.info("Secret for {} is revoked.", kerberosID);
+      } else {
+        LOG.info("Secret for {} doesn't exist.", kerberosID);
+      }
+    } else {
+      LOG.error("Error when revoking secret for {}.", kerberosID, exception);
+    }
+    return omClientResponse;
+  }
+}
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
new file mode 100644
index 000000000000..8630b52aabda
--- /dev/null
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
@@ -0,0 +1,58 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ * <p>
+ * http://www.apache.org/licenses/LICENSE-2.0
+ * <p>
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.hadoop.ozone.om.response.s3.security;
+
+import org.apache.hadoop.hdds.utils.db.BatchOperation;
+import org.apache.hadoop.ozone.om.OMMetadataManager;
+import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
+import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
+import org.apache.hadoop.ozone.om.response.OMClientResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
+import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.Status;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import java.io.IOException;
+
+import static org.apache.hadoop.ozone.om.OmMetadataManagerImpl.S3_SECRET_TABLE;
+
+/**
+ * Response for RevokeS3Secret request.
+ */
+@CleanupTableInfo(cleanupTables = {S3_SECRET_TABLE})
+public class S3RevokeSecretResponse extends OMClientResponse {
+
+  private final S3SecretValue s3SecretValue;
+
+  public S3RevokeSecretResponse(@Nullable S3SecretValue s3SecretValue,
+                                @Nonnull OMResponse omResponse) {
+    super(omResponse);
+    this.s3SecretValue = s3SecretValue;
+  }
+
+  @Override
+  public void addToDBBatch(OMMetadataManager omMetadataManager,
+      BatchOperation batchOperation) throws IOException {
+
+    if (s3SecretValue != null && getOMResponse().getStatus() == Status.OK) {
+      omMetadataManager.getS3SecretTable().deleteWithBatch(batchOperation,
+          s3SecretValue.getKerberosID());
+    }
+  }
+}
diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
new file mode 100644
index 000000000000..17e71b9df1e3
--- /dev/null
+++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ *  with the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *  Unless required by applicable law or agreed to in writing, software
+ *  distributed under the License is distributed on an "AS IS" BASIS,
+ *  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *  See the License for the specific language governing permissions and
+ *  limitations under the License.
+ */
+package org.apache.hadoop.ozone.shell.s3;
+
+import org.apache.hadoop.ozone.client.OzoneClient;
+import org.apache.hadoop.ozone.shell.OzoneAddress;
+import org.apache.hadoop.security.UserGroupInformation;
+import picocli.CommandLine.Command;
+import picocli.CommandLine.Option;
+
+import java.io.IOException;
+import java.util.Scanner;
+
+/**
+ * Executes revokesecret calls.
+ */
+@Command(name = "revokesecret",
+    description = "Revoke s3 secret for current user")
+public class RevokeS3SecretHandler extends S3Handler {
+
+  @Option(names = "-y",
+      description = "Continue without interactive user confirmation")
+  private boolean yes;
+
+  @Override
+  protected boolean isApplicable() {
+    return securityEnabled();
+  }
+
+  @Override
+  protected void execute(OzoneClient client, OzoneAddress address)
+      throws IOException {
+    String userName = UserGroupInformation.getCurrentUser().getUserName();
+
+    if (!yes) {
+      // Ask for user confirmation
+      out().print("Type 'y' to confirm revoking S3 secret for " + userName + ": ");
+      out().flush();
+      Scanner scanner = new Scanner(System.in);
+      String confirmation = scanner.next().trim().toLowerCase();
+      if (!confirmation.equals("y")) {
+        out().println("Cancelled.");
+        return;
+      }
+    }
+    client.getObjectStore().revokeS3Secret(userName);
+  }
+}
diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Shell.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Shell.java
index 41163723a5b6..d85b0e9b7b6e 100644
--- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Shell.java
+++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/S3Shell.java
@@ -30,7 +30,8 @@
 @Command(name = "ozone s3",
     description = "Shell for S3 specific operations",
     subcommands = {
-        GetS3SecretHandler.class
+        GetS3SecretHandler.class,
+        RevokeS3SecretHandler.class
     })
 public class S3Shell extends Shell {
 

From 1b556c80529abb6e4bb5ca2f1215815f9d560c8d Mon Sep 17 00:00:00 2001
From: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Date: Mon, 17 May 2021 12:09:45 -0700
Subject: [PATCH 2/5] Admins are always allowed to get/revoke secret; Invalid
 entry in table cache immediately (Thanks Aravindan); Impl. isAdmin in
 OzoneManager.

Change-Id: Ia5c8d76be0845c5530d1e84abd82217be7797141
---
 .../hadoop/ozone/TestSecureOzoneCluster.java  | 14 +++++++-
 .../ozone/om/TestOMDbCheckpointServlet.java   |  2 +-
 .../ozone/om/OMDBCheckpointServlet.java       | 12 +++----
 .../apache/hadoop/ozone/om/OzoneManager.java  | 36 +++++++++++++++++--
 .../s3/security/S3GetSecretRequest.java       | 10 ++++--
 .../s3/security/S3RevokeSecretRequest.java    | 19 +++++++---
 .../ozone/shell/s3/GetS3SecretHandler.java    | 11 ++++--
 .../ozone/shell/s3/RevokeS3SecretHandler.java | 18 +++++++---
 8 files changed, 95 insertions(+), 27 deletions(-)

diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java
index 4de9684f8664..7762990efd62 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/TestSecureOzoneCluster.java
@@ -118,6 +118,7 @@
 import org.junit.After;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
@@ -615,7 +616,7 @@ private void setupOm(OzoneConfiguration config) throws Exception {
   }
 
   @Test
-  public void testGetS3Secret() throws Exception {
+  public void testGetS3SecretAndRevokeS3Secret() throws Exception {
 
     // Setup secure OM for start
     setupOm(conf);
@@ -643,6 +644,17 @@ public void testGetS3Secret() throws Exception {
       //access key fetched on both attempts must be same
       assertEquals(attempt1.getAwsAccessKey(), attempt2.getAwsAccessKey());
 
+      // Revoke the existing secret
+      omClient.revokeS3Secret(username);
+
+      // Get a new secret
+      S3SecretValue attempt3 = omClient.getS3Secret(username);
+
+      // secret should differ because it has been revoked previously
+      assertNotEquals(attempt3.getAwsSecret(), attempt2.getAwsSecret());
+
+      // accessKey is still the same because it is derived from username
+      assertEquals(attempt3.getAwsAccessKey(), attempt2.getAwsAccessKey());
 
       try {
         omClient.getS3Secret("HADOOP/JOHNDOE");
diff --git a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java
index b6ac88d7f188..37dd446cf70e 100644
--- a/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java
+++ b/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMDbCheckpointServlet.java
@@ -128,7 +128,7 @@ public void testDoGet() throws ServletException, IOException {
           om.getMetadataManager().getStore(),
           om.getMetrics().getDBCheckpointMetrics(),
           om.getAclsEnabled(),
-          om.getOzoneAdmins(om.getConfiguration()));
+          om.getOmAdminUsernames());
 
       HttpServletRequest requestMock = mock(HttpServletRequest.class);
       // Return current user short name when asked
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
index 39868b44a013..0810052dc602 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
@@ -58,13 +58,9 @@ public void init() throws ServletException {
       return;
     }
 
-    try {
-      initialize(om.getMetadataManager().getStore(),
-          om.getMetrics().getDBCheckpointMetrics(),
-          om.getAclsEnabled(),
-          om.getOzoneAdmins(om.getConfiguration()));
-    } catch (IOException e) {
-      LOG.error("Error in getOzoneAdmins: {}", e.getMessage());
-    }
+    initialize(om.getMetadataManager().getStore(),
+        om.getMetrics().getDBCheckpointMetrics(),
+        om.getAclsEnabled(),
+        om.getOmAdminUsernames());
   }
 }
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
index de6180f55030..093c3aefcfbe 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
@@ -161,6 +161,7 @@
 import org.apache.hadoop.ozone.security.acl.RequestContext;
 import org.apache.hadoop.hdds.ExitManager;
 import org.apache.hadoop.ozone.util.OzoneVersionInfo;
+import org.apache.hadoop.security.AccessControlException;
 import org.apache.hadoop.security.SecurityUtil;
 import org.apache.hadoop.security.UserGroupInformation;
 import org.apache.hadoop.security.UserGroupInformation.AuthenticationMethod;
@@ -200,6 +201,7 @@
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ACL_ENABLED_DEFAULT;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS;
+import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS_WILDCARD;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_KEY_PREALLOCATION_BLOCKS_MAX;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_KEY_PREALLOCATION_BLOCKS_MAX_DEFAULT;
 import static org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_SCM_BLOCK_SIZE;
@@ -273,6 +275,11 @@ public final class OzoneManager extends ServiceRuntimeInfoImpl
   private KeyManager keyManager;
   private PrefixManagerImpl prefixManager;
 
+  /**
+   * OM super user / admin list
+   */
+  private final Collection<String> omAdminUsernames;
+
   private final OMMetrics metrics;
   private final ProtocolMessageMetrics<ProtocolMessageEnum>
       omClientProtocolMetrics;
@@ -436,6 +443,9 @@ private OzoneManager(OzoneConfiguration conf) throws IOException,
       blockTokenMgr = createBlockTokenSecretManager(configuration);
     }
 
+    // Get admin list
+    omAdminUsernames = getOzoneAdminsFromConfig(configuration);
+
     instantiateServices();
 
     // Create special volume s3v which is required for S3G.
@@ -543,7 +553,7 @@ private void instantiateServices() throws IOException {
         authorizer.setBucketManager(bucketManager);
         authorizer.setKeyManager(keyManager);
         authorizer.setPrefixManager(prefixManager);
-        authorizer.setOzoneAdmins(getOzoneAdmins(configuration));
+        authorizer.setOzoneAdmins(omAdminUsernames);
         authorizer.setAllowListAllVolumes(allowListAllVolumes);
       }
     } else {
@@ -3540,9 +3550,9 @@ public OzoneDelegationTokenSecretManager getDelegationTokenMgr() {
   }
 
   /**
-   * Return list of OzoneAdministrators.
+   * Return list of OzoneAdministrators from config.
    */
-  Collection<String> getOzoneAdmins(OzoneConfiguration conf)
+  Collection<String> getOzoneAdminsFromConfig(OzoneConfiguration conf)
       throws IOException {
     Collection<String> ozAdmins =
         conf.getTrimmedStringCollection(OZONE_ADMINISTRATORS);
@@ -3553,6 +3563,26 @@ Collection<String> getOzoneAdmins(OzoneConfiguration conf)
     return ozAdmins;
   }
 
+  /**
+   * Return the list of Ozone administrators in effect.
+   */
+  Collection<String> getOmAdminUsernames() {
+    return omAdminUsernames;
+  }
+
+  /**
+   * Return true if remoteUser is OM admin, false otherwise.
+   * @param remoteUser User name
+   * @throws AccessControlException
+   */
+  public boolean isAdmin(String remoteUser) throws AccessControlException {
+    if (remoteUser == null || remoteUser.isEmpty()) {
+      return false;
+    }
+    return omAdminUsernames.contains(remoteUser)
+        || omAdminUsernames.contains(OZONE_ADMINISTRATORS_WILDCARD);
+  }
+
   /**
    * Returns true if OzoneNativeAuthorizer is enabled and false if otherwise.
    *
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
index b240373b937c..ccfb4bc97a97 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
@@ -73,10 +73,14 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
     String kerberosID = s3GetSecretRequest.getKerberosID();
 
     UserGroupInformation user = ProtobufRpcEngine.Server.getRemoteUser();
+    // Permission check. Users need to be themselves or have admin privilege
     if (!user.getUserName().equals(kerberosID)) {
-      throw new OMException("User mismatch. Requested user name is " +
-          "mismatched " + kerberosID +", with current user " +
-          user.getUserName(), OMException.ResultCodes.USER_MISMATCH);
+      if (!ozoneManager.isAdmin(kerberosID)) {
+        throw new OMException("Requested user name '" + kerberosID +
+            "' doesn't match current user '" + user.getUserName() +
+            "', nor does current user has administrator privilege.",
+            OMException.ResultCodes.USER_MISMATCH);
+      }
     }
 
     String s3Secret = DigestUtils.sha256Hex(OmUtils.getSHADigest());
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
index 759bf4808b27..8f047d033aa0 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
@@ -18,6 +18,9 @@
 
 package org.apache.hadoop.ozone.om.request.s3.security;
 
+import com.google.common.base.Optional;
+import org.apache.hadoop.hdds.utils.db.cache.CacheKey;
+import org.apache.hadoop.hdds.utils.db.cache.CacheValue;
 import org.apache.hadoop.ipc.ProtobufRpcEngine;
 import org.apache.hadoop.ozone.OzoneConsts;
 import org.apache.hadoop.ozone.audit.OMAction;
@@ -63,10 +66,14 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
     final String kerberosID = s3RevokeSecretRequest.getKerberosID();
     final UserGroupInformation user = ProtobufRpcEngine.Server.getRemoteUser();
 
+    // Permission check. Users need to be themselves or have admin privilege
     if (!user.getUserName().equals(kerberosID)) {
-      throw new OMException("User mismatch. Requested user name is " +
-          "mismatched " + kerberosID +", with current user " +
-          user.getUserName(), OMException.ResultCodes.USER_MISMATCH);
+      if (!ozoneManager.isAdmin(kerberosID)) {
+        throw new OMException("Requested user name '" + kerberosID +
+            "' doesn't match current user '" + user.getUserName() +
+            "', nor does current user has administrator privilege.",
+            OMException.ResultCodes.USER_MISMATCH);
+      }
     }
 
     final RevokeS3SecretRequest revokeS3SecretRequest =
@@ -74,7 +81,6 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
                     .setKerberosID(kerberosID).build();
 
     OMRequest.Builder omRequest = OMRequest.newBuilder()
-        .setUserInfo(getUserInfo())
         .setRevokeS3SecretRequest(revokeS3SecretRequest)
         .setCmdType(getOmRequest().getCmdType())
         .setClientId(getOmRequest().getClientId());
@@ -108,7 +114,10 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
 
       // Remove if entry exists in table
       if (omMetadataManager.getS3SecretTable().isExist(kerberosID)) {
-//        omMetadataManager.getS3SecretTable().delete(kerberosID);
+        // Invalid entry in table cache immediately
+        omMetadataManager.getKeyTable().addCacheEntry(
+            new CacheKey<>(kerberosID),
+            new CacheValue<>(Optional.absent(), transactionLogIndex));
         omClientResponse = new S3RevokeSecretResponse(
                 new S3SecretValue(kerberosID, null),
                 omResponse.setStatus(Status.OK).build());
diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java
index b6506a54e747..df58eea80b38 100644
--- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java
+++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/GetS3SecretHandler.java
@@ -34,6 +34,11 @@
     description = "Returns s3 secret for current user")
 public class GetS3SecretHandler extends S3Handler {
 
+  @Option(names = "-u",
+      description = "Specify the user name to perform the operation on "
+          + "(admins only)'")
+  private String username;
+
   @Option(names = "-e",
       description = "Print out variables together with 'export' prefix, to "
           + "use it from 'eval $(ozone s3 getsecret)'")
@@ -47,9 +52,11 @@ protected boolean isApplicable() {
   @Override
   protected void execute(OzoneClient client, OzoneAddress address)
       throws IOException {
-    String userName = UserGroupInformation.getCurrentUser().getUserName();
+    if (username == null || username.isEmpty()) {
+      username = UserGroupInformation.getCurrentUser().getUserName();
+    }
 
-    final S3SecretValue secret = client.getObjectStore().getS3Secret(userName);
+    final S3SecretValue secret = client.getObjectStore().getS3Secret(username);
     if (export) {
       out().println("export AWS_ACCESS_KEY_ID=" + secret.getAwsAccessKey());
       out().println("export AWS_SECRET_ACCESS_KEY=" + secret.getAwsSecret());
diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
index 17e71b9df1e3..7887c5d9509d 100644
--- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
+++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
@@ -33,6 +33,11 @@
     description = "Revoke s3 secret for current user")
 public class RevokeS3SecretHandler extends S3Handler {
 
+  @Option(names = "-u",
+      description = "Specify the user name to perform the operation on "
+          + "(admins only)'")
+  private String username;
+
   @Option(names = "-y",
       description = "Continue without interactive user confirmation")
   private boolean yes;
@@ -45,19 +50,24 @@ protected boolean isApplicable() {
   @Override
   protected void execute(OzoneClient client, OzoneAddress address)
       throws IOException {
-    String userName = UserGroupInformation.getCurrentUser().getUserName();
+    if (username == null || username.isEmpty()) {
+      username = UserGroupInformation.getCurrentUser().getUserName();
+    }
 
     if (!yes) {
       // Ask for user confirmation
-      out().print("Type 'y' to confirm revoking S3 secret for " + userName + ": ");
+      out().print("Enter 'y' to confirm S3 secret revocation for '" +
+          username + "': ");
       out().flush();
       Scanner scanner = new Scanner(System.in);
       String confirmation = scanner.next().trim().toLowerCase();
       if (!confirmation.equals("y")) {
-        out().println("Cancelled.");
+        out().println("Operation cancelled.");
         return;
       }
     }
-    client.getObjectStore().revokeS3Secret(userName);
+
+    client.getObjectStore().revokeS3Secret(username);
+    out().println("S3 secret revoked.");
   }
 }

From fa99fcd48e3c906784201374c436bf6705fa1d8b Mon Sep 17 00:00:00 2001
From: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Date: Mon, 17 May 2021 12:56:09 -0700
Subject: [PATCH 3/5] Improve S3RevokeSecretResponse; checkstyle; findbugs.

Change-Id: Idb7ac40b974380a43946930bb4742d44696cfcea
---
 .../apache/hadoop/ozone/om/OMDBCheckpointServlet.java |  2 --
 .../om/request/s3/security/S3RevokeSecretRequest.java |  3 +--
 .../response/s3/security/S3RevokeSecretResponse.java  | 11 +++++------
 .../hadoop/ozone/shell/s3/RevokeS3SecretHandler.java  |  5 ++++-
 4 files changed, 10 insertions(+), 11 deletions(-)

diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
index 0810052dc602..6045bf4b065a 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OMDBCheckpointServlet.java
@@ -26,8 +26,6 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-import java.io.IOException;
-
 /**
  * Provides the current checkpoint Snapshot of the OM DB. (tar.gz)
  *
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
index 8f047d033aa0..86771b1bd229 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
@@ -118,8 +118,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager,
         omMetadataManager.getKeyTable().addCacheEntry(
             new CacheKey<>(kerberosID),
             new CacheValue<>(Optional.absent(), transactionLogIndex));
-        omClientResponse = new S3RevokeSecretResponse(
-                new S3SecretValue(kerberosID, null),
+        omClientResponse = new S3RevokeSecretResponse(kerberosID,
                 omResponse.setStatus(Status.OK).build());
       } else {
         omClientResponse = new S3RevokeSecretResponse(null,
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
index 8630b52aabda..05633010eb74 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/response/s3/security/S3RevokeSecretResponse.java
@@ -20,7 +20,6 @@
 
 import org.apache.hadoop.hdds.utils.db.BatchOperation;
 import org.apache.hadoop.ozone.om.OMMetadataManager;
-import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
 import org.apache.hadoop.ozone.om.response.CleanupTableInfo;
 import org.apache.hadoop.ozone.om.response.OMClientResponse;
 import org.apache.hadoop.ozone.protocol.proto.OzoneManagerProtocolProtos.OMResponse;
@@ -38,21 +37,21 @@
 @CleanupTableInfo(cleanupTables = {S3_SECRET_TABLE})
 public class S3RevokeSecretResponse extends OMClientResponse {
 
-  private final S3SecretValue s3SecretValue;
+  private final String kerberosID;
 
-  public S3RevokeSecretResponse(@Nullable S3SecretValue s3SecretValue,
+  public S3RevokeSecretResponse(@Nullable String kerberosID,
                                 @Nonnull OMResponse omResponse) {
     super(omResponse);
-    this.s3SecretValue = s3SecretValue;
+    this.kerberosID = kerberosID;
   }
 
   @Override
   public void addToDBBatch(OMMetadataManager omMetadataManager,
       BatchOperation batchOperation) throws IOException {
 
-    if (s3SecretValue != null && getOMResponse().getStatus() == Status.OK) {
+    if (kerberosID != null && getOMResponse().getStatus() == Status.OK) {
       omMetadataManager.getS3SecretTable().deleteWithBatch(batchOperation,
-          s3SecretValue.getKerberosID());
+          kerberosID);
     }
   }
 }
diff --git a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
index 7887c5d9509d..be35bb791960 100644
--- a/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
+++ b/hadoop-ozone/tools/src/main/java/org/apache/hadoop/ozone/shell/s3/RevokeS3SecretHandler.java
@@ -24,6 +24,8 @@
 import picocli.CommandLine.Option;
 
 import java.io.IOException;
+import java.io.InputStreamReader;
+import java.nio.charset.StandardCharsets;
 import java.util.Scanner;
 
 /**
@@ -59,7 +61,8 @@ protected void execute(OzoneClient client, OzoneAddress address)
       out().print("Enter 'y' to confirm S3 secret revocation for '" +
           username + "': ");
       out().flush();
-      Scanner scanner = new Scanner(System.in);
+      Scanner scanner = new Scanner(new InputStreamReader(
+          System.in, StandardCharsets.UTF_8));
       String confirmation = scanner.next().trim().toLowerCase();
       if (!confirmation.equals("y")) {
         out().println("Operation cancelled.");

From 550590b2323593a3d649f81464d6803a28edcae7 Mon Sep 17 00:00:00 2001
From: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Date: Mon, 17 May 2021 13:12:25 -0700
Subject: [PATCH 4/5] Checkstyle.

Change-Id: I8fe03516379f9094769633b82a8a77e06e8a809b
---
 .../src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java  | 2 +-
 .../ozone/om/request/s3/security/S3RevokeSecretRequest.java     | 1 -
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
index 5bd4caaa18ee..982d2297f2ac 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
@@ -278,7 +278,7 @@ public final class OzoneManager extends ServiceRuntimeInfoImpl
   private PrefixManagerImpl prefixManager;
 
   /**
-   * OM super user / admin list
+   * OM super user / admin list.
    */
   private final Collection<String> omAdminUsernames;
 
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
index 86771b1bd229..98265bbe347d 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
@@ -27,7 +27,6 @@
 import org.apache.hadoop.ozone.om.OMMetadataManager;
 import org.apache.hadoop.ozone.om.OzoneManager;
 import org.apache.hadoop.ozone.om.exceptions.OMException;
-import org.apache.hadoop.ozone.om.helpers.S3SecretValue;
 import org.apache.hadoop.ozone.om.ratis.utils.OzoneManagerDoubleBufferHelper;
 import org.apache.hadoop.ozone.om.request.OMClientRequest;
 import org.apache.hadoop.ozone.om.request.util.OmResponseUtil;

From 270e42e32418369a7fff4af98af4153b06211cc8 Mon Sep 17 00:00:00 2001
From: Siyao Meng <50227127+smengcl@users.noreply.github.com>
Date: Tue, 18 May 2021 14:57:29 -0700
Subject: [PATCH 5/5] Improve check; address nits.

Change-Id: I1d6e054010e7046299e65f8bef1d341eb60e5ea8
---
 .../ozone/client/protocol/ClientProtocol.java       |  1 -
 .../ozone/om/protocol/OzoneManagerProtocol.java     |  1 -
 .../om/request/s3/security/S3GetSecretRequest.java  | 13 ++++++-------
 .../request/s3/security/S3RevokeSecretRequest.java  | 13 ++++++-------
 4 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
index 32ab7990e3ac..4e0d7e185a8b 100644
--- a/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
+++ b/hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/protocol/ClientProtocol.java
@@ -511,7 +511,6 @@ void cancelDelegationToken(Token<OzoneTokenIdentifier> token)
   /**
    * Revoke S3 Secret of given kerberos user.
    * @param kerberosID
-   * @return S3SecretValue
    * @throws IOException
    */
   void revokeS3Secret(String kerberosID) throws IOException;
diff --git a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
index 31efbefd70f7..183757972c30 100644
--- a/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
+++ b/hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/protocol/OzoneManagerProtocol.java
@@ -384,7 +384,6 @@ OmMultipartUploadList listMultipartUploads(String volumeName,
   /**
    * Revokes s3Secret of given kerberos user.
    * @param kerberosID
-   * @return S3SecretValue
    * @throws IOException
    */
   void revokeS3Secret(String kerberosID) throws IOException;
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
index ccfb4bc97a97..ef2eee4ce5b6 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java
@@ -74,13 +74,12 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
 
     UserGroupInformation user = ProtobufRpcEngine.Server.getRemoteUser();
     // Permission check. Users need to be themselves or have admin privilege
-    if (!user.getUserName().equals(kerberosID)) {
-      if (!ozoneManager.isAdmin(kerberosID)) {
-        throw new OMException("Requested user name '" + kerberosID +
-            "' doesn't match current user '" + user.getUserName() +
-            "', nor does current user has administrator privilege.",
-            OMException.ResultCodes.USER_MISMATCH);
-      }
+    if (!user.getUserName().equals(kerberosID) &&
+        !ozoneManager.isAdmin(kerberosID)) {
+      throw new OMException("Requested user name '" + kerberosID +
+          "' doesn't match current user '" + user.getUserName() +
+          "', nor does current user has administrator privilege.",
+          OMException.ResultCodes.USER_MISMATCH);
     }
 
     String s3Secret = DigestUtils.sha256Hex(OmUtils.getSHADigest());
diff --git a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
index 98265bbe347d..6ee2007e4da8 100644
--- a/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
+++ b/hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3RevokeSecretRequest.java
@@ -66,13 +66,12 @@ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
     final UserGroupInformation user = ProtobufRpcEngine.Server.getRemoteUser();
 
     // Permission check. Users need to be themselves or have admin privilege
-    if (!user.getUserName().equals(kerberosID)) {
-      if (!ozoneManager.isAdmin(kerberosID)) {
-        throw new OMException("Requested user name '" + kerberosID +
-            "' doesn't match current user '" + user.getUserName() +
-            "', nor does current user has administrator privilege.",
-            OMException.ResultCodes.USER_MISMATCH);
-      }
+    if (!user.getUserName().equals(kerberosID) &&
+        !ozoneManager.isAdmin(kerberosID)) {
+      throw new OMException("Requested user name '" + kerberosID +
+          "' doesn't match current user '" + user.getUserName() +
+          "', nor does current user has administrator privilege.",
+          OMException.ResultCodes.USER_MISMATCH);
     }
 
     final RevokeS3SecretRequest revokeS3SecretRequest =