Skip to content

Backport to branch(3) : Fix NullPointerException when a client is misconfigured with digital signature #318

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 29, 2025
Merged

Backport to branch(3) : Fix NullPointerException when a client is misconfigured with digital signature #318

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions common/src/main/java/com/scalar/dl/ledger/error/CommonError.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,12 @@ public enum CommonError implements ScalarDlError {
"",
""),

//
// Errors for SECRET_NOT_FOUND(415)
//
SECRET_NOT_FOUND(
StatusCode.SECRET_NOT_FOUND, "001", "The specified secret is not found.", "", ""),

//
// Errors for DATABASE_ERROR(500)
//
Expand Down
5 changes: 0 additions & 5 deletions common/src/main/java/com/scalar/dl/ledger/service/Constants.java
View file
Open in desktop

This file was deleted.

3 changes: 3 additions & 0 deletions common/src/main/java/com/scalar/dl/ledger/service/StatusCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,9 @@ public enum StatusCode {
/** 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.
*/
Expand Down
40 changes: 39 additions & 1 deletion ledger/src/integration-test/java/com/scalar/dl/ledger/service/LedgerServiceEndToEndTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,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;
Expand Down Expand Up @@ -2495,7 +2496,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());
Expand Down Expand Up @@ -2530,6 +2531,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
Expand Down
8 changes: 5 additions & 3 deletions ledger/src/main/java/com/scalar/dl/ledger/crypto/SecretManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import com.scalar.dl.ledger.database.SecretRegistry;
import com.scalar.dl.ledger.error.CommonError;
import com.scalar.dl.ledger.exception.DatabaseException;
import com.scalar.dl.ledger.exception.MissingSecretException;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import javax.annotation.concurrent.Immutable;

Expand Down Expand Up @@ -56,11 +57,12 @@ 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(CommonError.SECRET_ALREADY_REGISTERED);
} catch (MissingSecretException e) {
registry.bind(entry);
}
registry.bind(entry);
}
Comment on lines 59 to 66
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This implementation of register uses a "Look-Before-You-Leap" (LBYL) approach by first calling lookup and then bind. This pattern is vulnerable to a race condition in a concurrent environment. Two threads could both check for the existence of a secret, find it missing, and then both attempt to bind it, with the second one overwriting the first.

A more robust approach would be to use an atomic 'put-if-absent' operation in the underlying SecretRegistry. The bind method in SecretRegistry could be modified to use a conditional Put (e.g., with PutIfNotExists in ScalarDB) and throw an exception if the secret already exists. This would make the registration atomic.

The register method here would then simplify to just calling registry.bind(entry) and handling the potential exception.

Since this race condition existed before this change, and fixing it would require changes outside the scope of this file, I recommend creating a follow-up task to address this atomicity issue.


/**
Expand Down
11 changes: 8 additions & 3 deletions ledger/src/main/java/com/scalar/dl/ledger/database/scalardb/ScalarSecretRegistry.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
import com.scalar.dl.ledger.database.SecretRegistry;
import com.scalar.dl.ledger.error.CommonError;
import com.scalar.dl.ledger.exception.DatabaseException;
import com.scalar.dl.ledger.exception.MissingSecretException;
import com.scalar.dl.ledger.exception.UnexpectedValueException;
import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
import java.nio.charset.StandardCharsets;
import javax.annotation.Nullable;
import javax.annotation.concurrent.Immutable;

@Immutable
Expand Down Expand Up @@ -72,7 +72,6 @@ public void unbind(SecretEntry.Key key) {
}

@Override
@Nullable
public SecretEntry lookup(SecretEntry.Key key) {
Get get =
new Get(
Expand All @@ -81,11 +80,17 @@ 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(CommonError.SECRET_NOT_FOUND));
} catch (ExecutionException e) {
throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage());
}

return toSecretEntry(result);
Comment on lines +83 to +93
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The logic in this try-catch block can be made more concise and idiomatic by better leveraging the Optional API. You can chain map and orElseThrow to achieve the same result with less code.

    try {
      return storage
          .get(get)
          .map(this::toSecretEntry)
          .orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND));
    } catch (ExecutionException e) {
      throw new DatabaseException(CommonError.GETTING_SECRET_KEY_FAILED, e, e.getMessage());
    }

}

private SecretEntry toSecretEntry(Result result) {
Expand Down
19 changes: 19 additions & 0 deletions ledger/src/main/java/com/scalar/dl/ledger/exception/MissingSecretException.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package com.scalar.dl.ledger.exception;

import com.scalar.dl.ledger.error.ScalarDlError;
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);
}

public MissingSecretException(ScalarDlError error, Object... args) {
super(error.buildMessage(args), error.getStatusCode());
}
}
4 changes: 3 additions & 1 deletion ledger/src/test/java/com/scalar/dl/ledger/crypto/SecretManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down
6 changes: 3 additions & 3 deletions ledger/src/test/java/com/scalar/dl/ledger/database/scalardb/ScalarSecretRegistryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down