Skip to content

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

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.10) : Fix NullPointerException when a client is misconfigured with digital signature #316

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 @@ -91,6 +91,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 @@ -2472,7 +2473,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 @@ -2507,6 +2508,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

@gemini-code-assist gemini-code-assist bot Oct 29, 2025

Choose a reason for hiding this comment

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

medium

While this change correctly adapts to the new behavior of registry.lookup (throwing MissingSecretException instead of returning null), the implementation uses exceptions for control flow. This is generally considered an anti-pattern as it can make the code harder to read and can have performance implications.

A more robust and clearer approach would be to implement an atomic registerIfNotExists operation within the SecretRegistry. This could be achieved using a conditional Put operation in ScalarDB (e.g., with PutIfNotExists condition). This would also eliminate a potential race condition between checking for existence and binding the new entry.

As this pattern existed before, this is a suggestion for future improvement rather than a required change for this PR.


/**
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);
}

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