HDDS-10200. OM may terminate due to NPE in S3SecretValue proto conversion

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/S3InMemoryCache.java

@@ -44,13 +44,7 @@ public void put(String id, S3SecretValue secretValue) {
 
   @Override
   public void invalidate(String id) {
-    S3SecretValue secret = cache.getIfPresent(id);
-    if (secret == null) {
-      return;
-    }
-    secret.setDeleted(true);
-    secret.setAwsSecret(null);
-    cache.put(id, secret);
+    cache.asMap().computeIfPresent(id, (k, secret) -> secret.deleted());
   }
 
   /**

hadoop-ozone/common/src/main/java/org/apache/hadoop/ozone/om/helpers/S3SecretValue.java

@@ -27,7 +27,7 @@
 /**
  * S3Secret to be saved in database.
  */
-public class S3SecretValue {
+public final class S3SecretValue {
   private static final Codec<S3SecretValue> CODEC = new DelegatedCodec<>(
       Proto2Codec.get(S3Secret.getDefaultInstance()),
       S3SecretValue::fromProtobuf,
@@ -38,16 +38,29 @@ public static Codec<S3SecretValue> getCodec() {
   }
 
   // TODO: This field should be renamed to accessId for generalization.
-  private String kerberosID;
-  private String awsSecret;
-  private boolean isDeleted;
-  private long transactionLogIndex;
+  private final String kerberosID;
+  private final String awsSecret;
+  private final boolean isDeleted;
+  private final long transactionLogIndex;
 
-  public S3SecretValue(String kerberosID, String awsSecret) {
-    this(kerberosID, awsSecret, false, 0L);
+  public static S3SecretValue of(String kerberosID, String awsSecret) {
+    return of(kerberosID, awsSecret, 0);
   }
 
-  public S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted,
+  public static S3SecretValue of(String kerberosID, String awsSecret, long transactionLogIndex) {
+    return new S3SecretValue(
+        Objects.requireNonNull(kerberosID),
+        Objects.requireNonNull(awsSecret),
+        false,
+        transactionLogIndex
+    );
+  }
+
+  public S3SecretValue deleted() {
+    return new S3SecretValue(kerberosID, "", true, transactionLogIndex);
+  }
+
+  private S3SecretValue(String kerberosID, String awsSecret, boolean isDeleted,
                        long transactionLogIndex) {
     this.kerberosID = kerberosID;
     this.awsSecret = awsSecret;
@@ -59,26 +72,14 @@ public String getKerberosID() {
     return kerberosID;
   }
 
-  public void setKerberosID(String kerberosID) {
-    this.kerberosID = kerberosID;
-  }
-
   public String getAwsSecret() {
     return awsSecret;
   }
 
-  public void setAwsSecret(String awsSecret) {
-    this.awsSecret = awsSecret;
-  }
-
   public boolean isDeleted() {
     return isDeleted;
   }
 
-  public void setDeleted(boolean status) {
-    this.isDeleted = status;
-  }
-
   public String getAwsAccessKey() {
     return kerberosID;
   }
@@ -87,12 +88,8 @@ public long getTransactionLogIndex() {
     return transactionLogIndex;
   }
 
-  public void setTransactionLogIndex(long transactionLogIndex) {
-    this.transactionLogIndex = transactionLogIndex;
-  }
-
   public static S3SecretValue fromProtobuf(S3Secret s3Secret) {
-    return new S3SecretValue(s3Secret.getKerberosID(), s3Secret.getAwsSecret());
+    return S3SecretValue.of(s3Secret.getKerberosID(), s3Secret.getAwsSecret());
   }
 
   public S3Secret getProtobuf() {

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestSecureOzoneRpcClient.java

@@ -264,7 +264,7 @@ public void testS3Auth() throws Exception {
 
     // Add secret to S3Secret table.
     s3SecretManager.storeSecret(accessKey,
-        new S3SecretValue(accessKey, secret));
+        S3SecretValue.of(accessKey, secret));
 
     OMRequest writeRequest = OMRequest.newBuilder()
         .setCmdType(OzoneManagerProtocolProtos.Type.CreateVolume)
@@ -313,7 +313,7 @@ public void testS3Auth() throws Exception {
 
     // Override secret to S3Secret store with some dummy value
     s3SecretManager
-        .storeSecret(accessKey, new S3SecretValue(accessKey, "dummy"));
+        .storeSecret(accessKey, S3SecretValue.of(accessKey, "dummy"));
 
     // Write request with invalid credentials.
     omResponse = cluster.getOzoneManager().getOmServerProtocol()

hadoop-ozone/interface-storage/src/test/java/org/apache/hadoop/ozone/om/helpers/TestS3SecretValueCodec.java

@@ -43,7 +43,7 @@ public void testCodecWithCorrectData() throws Exception {
     final Codec<S3SecretValue> codec = getCodec();
 
     S3SecretValue s3SecretValue =
-        new S3SecretValue(UUID.randomUUID().toString(),
+        S3SecretValue.of(UUID.randomUUID().toString(),
             UUID.randomUUID().toString());
 
     byte[] data = codec.toPersistedFormat(s3SecretValue);

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/S3SecretManagerImpl.java

@@ -62,8 +62,7 @@ public S3SecretValue getSecret(String kerberosID) throws IOException {
         // purposely deleted the secret. Hence, we do not have to check the DB.
         return null;
       }
-      return new S3SecretValue(cacheValue.getKerberosID(),
-          cacheValue.getAwsSecret());
+      return cacheValue;
     }
     S3SecretValue result = s3SecretStore.getSecret(kerberosID);
     if (result != null) {

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/OMSetSecretRequest.java

@@ -113,26 +113,20 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
     try {
       omClientResponse = ozoneManager.getS3SecretManager()
           .doUnderLock(accessId, s3SecretManager -> {
-            // Intentionally set to final so they can only be set once.
-            final S3SecretValue newS3SecretValue;
 
             // Update legacy S3SecretTable, if the accessId entry exists
-            if (s3SecretManager.hasS3Secret(accessId)) {
-              // accessId found in S3SecretTable. Update S3SecretTable
-              LOG.debug("Updating S3SecretTable cache entry");
-              // Update S3SecretTable cache entry in this case
-              newS3SecretValue = new S3SecretValue(accessId, secretKey);
-              // Set the transactionLogIndex to be used for updating.
-              newS3SecretValue.setTransactionLogIndex(termIndex.getIndex());
-              s3SecretManager
-                  .updateCache(accessId, newS3SecretValue);
-            } else {
+            if (!s3SecretManager.hasS3Secret(accessId)) {
               // If S3SecretTable is not updated,
               // throw ACCESS_ID_NOT_FOUND exception.
               throw new OMException("accessId '" + accessId + "' not found.",
                   OMException.ResultCodes.ACCESS_ID_NOT_FOUND);
             }
 
+            // Update S3SecretTable cache entry in this case
+            // Set the transactionLogIndex to be used for updating.
+            final S3SecretValue newS3SecretValue = S3SecretValue.of(accessId, secretKey, termIndex.getIndex());
+            s3SecretManager.updateCache(accessId, newS3SecretValue);
+
             // Compose response
             final SetS3SecretResponse.Builder setSecretResponse =
                 SetS3SecretResponse.newBuilder()

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/security/S3GetSecretRequest.java

@@ -150,21 +150,16 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
     try {
       omClientResponse = ozoneManager.getS3SecretManager()
           .doUnderLock(accessId, s3SecretManager -> {
-            S3SecretValue assignS3SecretValue;
-            S3SecretValue s3SecretValue =
-                s3SecretManager.getSecret(accessId);
+            final S3SecretValue assignS3SecretValue;
+            S3SecretValue s3SecretValue = s3SecretManager.getSecret(accessId);
 
             if (s3SecretValue == null) {
               // Not found in S3SecretTable.
               if (createIfNotExist) {
                 // Add new entry in this case
-                assignS3SecretValue =
-                    new S3SecretValue(accessId, awsSecret.get());
-                // Set the transactionLogIndex to be used for updating.
-                assignS3SecretValue.setTransactionLogIndex(termIndex.getIndex());
+                assignS3SecretValue = S3SecretValue.of(accessId, awsSecret.get(), termIndex.getIndex());
                 // Add cache entry first.
-                s3SecretManager.updateCache(accessId,
-                    assignS3SecretValue);
+                s3SecretManager.updateCache(accessId, assignS3SecretValue);
               } else {
                 assignS3SecretValue = null;
               }

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/s3/tenant/OMTenantAssignUserAccessIdRequest.java

@@ -271,10 +271,7 @@ public OMClientResponse validateAndUpdateCache(OzoneManager ozoneManager, TermIn
         }
       }
 
-      final S3SecretValue s3SecretValue =
-          new S3SecretValue(accessId, awsSecret);
-      // Set the transactionLogIndex to be used for updating.
-      s3SecretValue.setTransactionLogIndex(transactionLogIndex);
+      final S3SecretValue s3SecretValue = S3SecretValue.of(accessId, awsSecret, transactionLogIndex);
 
       // Add to tenantAccessIdTable
       final OmDBAccessIdInfo omDBAccessIdInfo = new OmDBAccessIdInfo.Builder()

hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/security/TestOzoneDelegationTokenSecretManager.java

@@ -106,9 +106,9 @@ public void setUp() throws Exception {
     serviceRpcAdd = new Text("localhost");
     final Map<String, S3SecretValue> s3Secrets = new HashMap<>();
     s3Secrets.put("testuser1",
-        new S3SecretValue("testuser1", "dbaksbzljandlkandlsd"));
+        S3SecretValue.of("testuser1", "dbaksbzljandlkandlsd"));
     s3Secrets.put("abc",
-        new S3SecretValue("abc", "djakjahkd"));
+        S3SecretValue.of("abc", "djakjahkd"));
     om = mock(OzoneManager.class);
     OMMetadataManager metadataManager = new OmMetadataManagerImpl(conf, om);
     when(om.getMetadataManager()).thenReturn(metadataManager);

hadoop-ozone/s3-secret-store/src/main/java/org/apache/hadoop/ozone/s3/remote/vault/VaultS3SecretStore.java

@@ -115,7 +115,7 @@ public S3SecretValue getSecret(String kerberosID) throws IOException {
         return null;
       }
 
-      return new S3SecretValue(kerberosID, s3Secret);
+      return S3SecretValue.of(kerberosID, s3Secret);
     } catch (VaultException e) {
       LOG.error("Failed to read secret", e);
       throw new IOException("Failed to read secret", e);

hadoop-ozone/s3-secret-store/src/test/java/org/apache/hadoop/ozone/s3/remote/vault/TestVaultS3SecretStore.java

@@ -89,7 +89,7 @@ public void clean() {
   @Test
   public void testReadWrite() throws IOException {
     SUCCESS_OPERATION_LIMIT.set(2);
-    S3SecretValue secret = new S3SecretValue("id", "value");
+    S3SecretValue secret = S3SecretValue.of("id", "value");
     s3SecretStore.storeSecret(
         "id",
         secret);
@@ -101,7 +101,7 @@ public void testReadWrite() throws IOException {
   public void testReAuth() throws IOException {
     SUCCESS_OPERATION_LIMIT.set(1);
     AUTH_OPERATION_PROVIDER.set(1);
-    S3SecretValue secret = new S3SecretValue("id", "value");
+    S3SecretValue secret = S3SecretValue.of("id", "value");
     s3SecretStore.storeSecret("id", secret);
 
     assertEquals(secret, s3SecretStore.getSecret("id"));
@@ -112,7 +112,7 @@ public void testReAuth() throws IOException {
   @Test
   public void testAuthFail() throws IOException {
     SUCCESS_OPERATION_LIMIT.set(1);
-    S3SecretValue secret = new S3SecretValue("id", "value");
+    S3SecretValue secret = S3SecretValue.of("id", "value");
     s3SecretStore.storeSecret("id", secret);
 
     assertThrows(IOException.class,

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/client/ClientProtocolStub.java

@@ -379,7 +379,7 @@ public void cancelDelegationToken(Token<OzoneTokenIdentifier> token)
   @Override
   @Nonnull
   public S3SecretValue getS3Secret(String kerberosID) throws IOException {
-    return new S3SecretValue(STUB_KERBEROS_ID, STUB_SECRET);
+    return S3SecretValue.of(STUB_KERBEROS_ID, STUB_SECRET);
   }
 
   @Override

hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3secret/TestSecretGenerate.java

@@ -42,14 +42,14 @@
 
 import static javax.ws.rs.core.Response.Status.BAD_REQUEST;
 import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.notNull;
 import static org.mockito.Mockito.when;
 
 /**
  * Test for S3 secret generate endpoint.
  */
 @ExtendWith(MockitoExtension.class)
-public class TestSecretGenerate {
+class TestSecretGenerate {
   private static final String USER_NAME = "test";
   private static final String OTHER_USER_NAME = "test2";
   private static final String USER_SECRET = "test_secret";
@@ -69,12 +69,11 @@ public class TestSecretGenerate {
 
   private static S3SecretValue getS3SecretValue(InvocationOnMock invocation) {
     Object[] args = invocation.getArguments();
-    return new S3SecretValue((String) args[0], USER_SECRET);
+    return S3SecretValue.of((String) args[0], USER_SECRET);
   }
 
   @BeforeEach
-  void setUp() throws IOException {
-    when(proxy.getS3Secret(any())).then(TestSecretGenerate::getS3SecretValue);
+  void setUp() {
     OzoneConfiguration conf = new OzoneConfiguration();
     OzoneClient client = new OzoneClientStub(new ObjectStoreStub(conf, proxy));
 
@@ -89,23 +88,20 @@ void setUp() throws IOException {
 
   @Test
   void testSecretGenerate() throws IOException {
-    when(principal.getName()).thenReturn(USER_NAME);
-    when(securityContext.getUserPrincipal()).thenReturn(principal);
-    when(context.getSecurityContext()).thenReturn(securityContext);
+    setupSecurityContext();
+    hasNoSecretYet();
 
     S3SecretResponse response =
             (S3SecretResponse) endpoint.generate().getEntity();
+
     assertEquals(USER_SECRET, response.getAwsSecret());
     assertEquals(USER_NAME, response.getAwsAccessKey());
   }
 
   @Test
   void testIfSecretAlreadyExists() throws IOException {
-    when(principal.getName()).thenReturn(USER_NAME);
-    when(securityContext.getUserPrincipal()).thenReturn(principal);
-    when(context.getSecurityContext()).thenReturn(securityContext);
-    when(proxy.getS3Secret(any())).thenThrow(new OMException("Secret already exists",
-        OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS));
+    setupSecurityContext();
+    hasSecretAlready();
 
     Response response = endpoint.generate();
 
@@ -116,9 +112,27 @@ void testIfSecretAlreadyExists() throws IOException {
 
   @Test
   void testSecretGenerateWithUsername() throws IOException {
+    hasNoSecretYet();
+
     S3SecretResponse response =
             (S3SecretResponse) endpoint.generate(OTHER_USER_NAME).getEntity();
     assertEquals(USER_SECRET, response.getAwsSecret());
     assertEquals(OTHER_USER_NAME, response.getAwsAccessKey());
   }
+
+  private void setupSecurityContext() {
+    when(principal.getName()).thenReturn(USER_NAME);
+    when(securityContext.getUserPrincipal()).thenReturn(principal);
+    when(context.getSecurityContext()).thenReturn(securityContext);
+  }
+
+  private void hasNoSecretYet() throws IOException {
+    when(proxy.getS3Secret(notNull()))
+        .then(TestSecretGenerate::getS3SecretValue);
+  }
+
+  private void hasSecretAlready() throws IOException {
+    when(proxy.getS3Secret(notNull()))
+        .thenThrow(new OMException("Secret already exists", OMException.ResultCodes.S3_SECRET_ALREADY_EXISTS));
+  }
 }