Skip to content

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

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

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

@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

Using a try-catch block for control flow, as done here, can make the code harder to understand. The expected path for registration (where the secret does not yet exist) is handled in the catch block, which is unconventional.

A clearer approach would be to introduce an exists() method in the SecretRegistry interface. This would make the intent of the code more explicit and improve readability.

For example, you could change the register method to:

public void register(SecretEntry entry) {
    if (registry.exists(entry.getKey())) {
        throw new DatabaseException(CommonError.SECRET_ALREADY_REGISTERED);
    }
    registry.bind(entry);
}

This would require adding an exists() method to the SecretRegistry interface and its implementations, but it would represent a more idiomatic way to handle this check.


/**
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

@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

This implementation can be made more concise by combining the variable declaration and assignment, and returning the result of toSecretEntry from within the try block. This improves readability by reducing the scope of the result variable.

Suggested change
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);
try {
Result result =
storage
.get(get)
.orElseThrow(() -> new MissingSecretException(CommonError.SECRET_NOT_FOUND));
return toSecretEntry(result);
} 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);
Comment on lines +47 to +48
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

It's generally better to avoid mocking simple data-holder classes like exceptions. You can directly instantiate the exception. This simplifies the test setup and avoids unnecessary mocking.

Suggested change
MissingSecretException toThrow = mock(MissingSecretException.class);
when(registry.lookup(entry.getKey())).thenThrow(toThrow);
when(registry.lookup(entry.getKey())).thenThrow(new MissingSecretException("secret not found"));


// 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