Backport to branch(3.9) : Fix NullPointerException when a client is misconfigured with digital signature for HMAC (#317)

Co-authored-by: Jun Nemoto <35618893+jnmt@users.noreply.github.com>

Author: feeblefakie

common/src/main/java/com/scalar/dl/ledger/service/Constants.java

@@ -123,6 +123,12 @@ public enum StatusCode {
   /** StatusCode: 413. This indicates that the given secret is already registered. */
   SECRET_ALREADY_REGISTERED(413),
 
+  /** StatusCode: 414. This indicates that the argument is invalid. */
+  INVALID_ARGUMENT(414),
+
+  /** StatusCode: 415. This indicates that the given secret is not found. */
+  SECRET_NOT_FOUND(415),
+
   /**
    * StatusCode: 500. This indicates that the system encountered a database error such as IO error.
    */

common/src/main/java/com/scalar/dl/ledger/service/StatusCode.java

@@ -89,6 +89,7 @@
 import com.scalar.dl.ledger.exception.ContractContextException;
 import com.scalar.dl.ledger.exception.LedgerException;
 import com.scalar.dl.ledger.exception.MissingContractException;
+import com.scalar.dl.ledger.exception.MissingSecretException;
 import com.scalar.dl.ledger.exception.SignatureException;
 import com.scalar.dl.ledger.model.CertificateRegistrationRequest;
 import com.scalar.dl.ledger.model.ContractExecutionRequest;
@@ -2384,7 +2385,7 @@ public void execute_HmacConfiguredAndValidHmacSignatureGiven_ShouldExecuteProper
   }
 
   @Test
-  public void execute_HmacConfiguredAndInvalidHmacSignatureGiven_ShouldExecuteProperly() {
+  public void execute_HmacConfiguredAndInvalidHmacSignatureGiven_ShouldThrowSignatureException() {
     // Arrange
     Properties props2 = createProperties();
     props2.put(LedgerConfig.AUTHENTICATION_METHOD, AuthenticationMethod.HMAC.getMethod());
@@ -2419,6 +2420,43 @@ public void execute_HmacConfiguredAndInvalidHmacSignatureGiven_ShouldExecuteProp
     assertThat(thrown).isExactlyInstanceOf(SignatureException.class);
   }
 
+  @Test
+  public void
+      execute_HmacConfiguredAndValidDigitalSignatureGiven_ShouldThrowMissingSecretException() {
+    // Arrange
+    Properties props2 = createProperties();
+    props2.put(LedgerConfig.AUTHENTICATION_METHOD, AuthenticationMethod.HMAC.getMethod());
+    props2.put(LedgerConfig.AUTHENTICATION_HMAC_CIPHER_KEY, SOME_CIPHER_KEY);
+    createServices(new LedgerConfig(props2));
+    String nonce = UUID.randomUUID().toString();
+    JsonNode contractArgument =
+        mapper
+            .createObjectNode()
+            .put(ASSET_ATTRIBUTE_NAME, SOME_ASSET_ID_1)
+            .put(AMOUNT_ATTRIBUTE_NAME, SOME_AMOUNT_1);
+    String argument = Argument.format(contractArgument, nonce, Collections.emptyList());
+
+    byte[] serialized =
+        ContractExecutionRequest.serialize(CREATE_CONTRACT_ID1, argument, ENTITY_ID_A, KEY_VERSION);
+    ContractExecutionRequest request =
+        new ContractExecutionRequest(
+            nonce,
+            ENTITY_ID_A,
+            KEY_VERSION,
+            CREATE_CONTRACT_ID1,
+            argument,
+            Collections.emptyList(),
+            null,
+            dsSigner1.sign(serialized),
+            null);
+
+    // Act
+    Throwable thrown = catchThrowable(() -> ledgerService.execute(request));
+
+    // Assert
+    assertThat(thrown).isExactlyInstanceOf(MissingSecretException.class);
+  }
+
   @Test
   public void execute_AuditorEnabledAndValidAuditorHmacSignatureGiven_ShouldExecuteProperly() {
     // Arrange

ledger/src/integration-test/java/com/scalar/dl/ledger/service/LedgerServiceEndToEndTest.java

@@ -8,6 +8,7 @@
 import com.google.inject.Inject;
 import com.scalar.dl.ledger.database.SecretRegistry;
 import com.scalar.dl.ledger.exception.DatabaseException;
+import com.scalar.dl.ledger.exception.MissingSecretException;
 import com.scalar.dl.ledger.service.StatusCode;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import javax.annotation.concurrent.Immutable;
@@ -56,12 +57,13 @@ public SecretManager(
    * @param entry a {@code SecretEntry}
    */
   public void register(SecretEntry entry) {
-    SecretEntry existing = registry.lookup(entry.getKey());
-    if (existing != null) {
+    try {
+      registry.lookup(entry.getKey());
       throw new DatabaseException(
           "The specified secret is already registered", StatusCode.SECRET_ALREADY_REGISTERED);
+    } catch (MissingSecretException e) {
+      registry.bind(entry);
     }
-    registry.bind(entry);
   }
 
   /**

ledger/src/main/java/com/scalar/dl/ledger/crypto/SecretManager.java

@@ -16,11 +16,11 @@
 import com.scalar.dl.ledger.crypto.SecretEntry;
 import com.scalar.dl.ledger.database.SecretRegistry;
 import com.scalar.dl.ledger.exception.DatabaseException;
+import com.scalar.dl.ledger.exception.MissingSecretException;
 import com.scalar.dl.ledger.exception.UnexpectedValueException;
 import com.scalar.dl.ledger.service.StatusCode;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
 import java.nio.charset.StandardCharsets;
-import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 
 @Immutable
@@ -72,7 +72,6 @@ public void unbind(SecretEntry.Key key) {
   }
 
   @Override
-  @Nullable
   public SecretEntry lookup(SecretEntry.Key key) {
     Get get =
         new Get(
@@ -81,12 +80,18 @@ public SecretEntry lookup(SecretEntry.Key key) {
             .withConsistency(Consistency.SEQUENTIAL)
             .forTable(TABLE);
 
+    Result result;
     try {
-      return storage.get(get).map(this::toSecretEntry).orElse(null);
+      result =
+          storage
+              .get(get)
+              .orElseThrow(() -> new MissingSecretException("the specified secret is not found"));
     } catch (ExecutionException e) {
       throw new DatabaseException(
           "can't get the secret key from storage", e, StatusCode.DATABASE_ERROR);
     }
+
+    return toSecretEntry(result);
   }
 
   private SecretEntry toSecretEntry(Result result) {

ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/ScalarSecretRegistry.java

@@ -0,0 +1,14 @@
+package com.scalar.dl.ledger.exception;
+
+import com.scalar.dl.ledger.service.StatusCode;
+
+public class MissingSecretException extends DatabaseException {
+
+  public MissingSecretException(String message) {
+    super(message, StatusCode.SECRET_NOT_FOUND);
+  }
+
+  public MissingSecretException(String message, Throwable cause) {
+    super(message, cause, StatusCode.SECRET_NOT_FOUND);
+  }
+}

ledger/src/main/java/com/scalar/dl/ledger/exception/MissingSecretException.java

@@ -13,6 +13,7 @@
 import com.google.common.cache.CacheBuilder;
 import com.scalar.dl.ledger.database.SecretRegistry;
 import com.scalar.dl.ledger.exception.DatabaseException;
+import com.scalar.dl.ledger.exception.MissingSecretException;
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
 import org.mockito.Mock;
@@ -43,7 +44,8 @@ public void setUp() {
   @Test
   public void register_ProperSecretEntryGiven_ShouldCallBind() {
     // Arrange
-    when(registry.lookup(entry.getKey())).thenReturn(null);
+    MissingSecretException toThrow = mock(MissingSecretException.class);
+    when(registry.lookup(entry.getKey())).thenThrow(toThrow);
 
     // Act
     manager.register(entry);

ledger/src/test/java/com/scalar/dl/ledger/crypto/SecretManagerTest.java

@@ -22,6 +22,7 @@
 import com.scalar.dl.ledger.crypto.Cipher;
 import com.scalar.dl.ledger.crypto.SecretEntry;
 import com.scalar.dl.ledger.exception.DatabaseException;
+import com.scalar.dl.ledger.exception.MissingSecretException;
 import java.nio.charset.StandardCharsets;
 import java.util.Optional;
 import org.junit.jupiter.api.BeforeEach;
@@ -153,18 +154,17 @@ public void lookup_ValidArgumentGiven_ShouldLookupProperly() throws ExecutionExc
   }
 
   @Test
-  public void lookup_ValidArgumentGivenButEmptyResultReturned_ShouldReturnNull()
+  public void lookup_ValidArgumentGivenButEmptyResultReturned_ShouldThrowMissingSecretException()
       throws ExecutionException {
     // Arrange
     SecretEntry.Key key = new SecretEntry.Key(SOME_ENTITY_ID, SOME_KEY_VERSION);
     when(storage.get(any(Get.class))).thenReturn(Optional.empty());
 
     // Act Assert
-    SecretEntry actual = registry.lookup(key);
+    assertThatThrownBy(() -> registry.lookup(key)).isInstanceOf(MissingSecretException.class);
 
     // Assert
     verify(storage).get(any(Get.class));
-    assertThat(actual).isNull();
   }
 
   @Test