Skip to content

fix: remove the insecure default for crypto.keystorePassword #26777

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

Draft
AlexKehayov wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: remove the insecure default for crypto.keystorePassword #26777

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
2 changes: 1 addition & 1 deletion platform-sdk/base-crypto/src/main/java/org/hiero/base/crypto/config/CryptoConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,4 +14,4 @@
*/
@ConfigData("crypto")
public record CryptoConfig(
@ConfigProperty(defaultValue = "password") String keystorePassword) {}
@ConfigProperty(defaultValue = "") String keystorePassword) {}

@anthony-swirldslabs anthony-swirldslabs Aug 11, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I appreciate strengthening the security in general, and this fix does just that.

However, why is this being done? Why now? Where's an issue that describes the problem and provides a rationale for this specific fix (I don't see one linked to this PR)?

The reason I'm asking these questions is because, as you rightly noticed, this fix introduces a backward-incompatible behavior that would affect all the public networks (such as the Hedera previewnet, testnet, and mainnet), all the private networks (such as those running on HashSphere), all the individual networks (such as those using Solo), as well as unit and integration tests, especially in automated GitHub pipelines and stand-alone performance testing environments. This seems like a very large scale impact, and again, there's not a linked issue that would confirm that the change has been discussed with all the affected parties and they all agree to it.

I don't question the technical part of this change - it looks fine (provided all tests pass, but it seems some PR checks are failing currently). I'm merely concerned about the missing problem statement, missing justification for this version of the fix and alternatives considered, and lack of confirmations that the change has been communicated to all the affected parties.

@netopyr netopyr Aug 12, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree 100%. In addition, I wonder if the consequences of this change have been properly evaluated if we consider the huge number of failing tests. We probably do not even know how this change will affect our downstream test environments. The change itself is correct and fits our strategy for default configuration values. However, I think it needs to be better prepared and communicated.

18 changes: 18 additions & 0 deletions platform-sdk/base-crypto/src/test/java/org/hiero/base/crypto/config/CryptoConfigTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: Apache-2.0
package org.hiero.base.crypto.config;

import com.swirlds.config.api.Configuration;
import com.swirlds.config.api.ConfigurationBuilder;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
Expand All @@ -15,4 +16,21 @@ public void testDefaultValuesValid() {
// then
Assertions.assertDoesNotThrow(() -> builder.build(), "All default values of CryptoConfig should be valid");
}

@Test
public void testNoUsableDefaultKeystorePassword() {
// given
final Configuration configuration = ConfigurationBuilder.create()
.withConfigDataType(CryptoConfig.class)
.build();

// when
final CryptoConfig cryptoConfig = configuration.getConfigData(CryptoConfig.class);

// then
Assertions.assertTrue(
cryptoConfig.keystorePassword() == null
|| cryptoConfig.keystorePassword().isBlank(),
"CryptoConfig must not provide a usable default keystore password");
}
}
1 change: 1 addition & 0 deletions .../src/test/java/org/hiero/consensus/gossip/impl/gossip/modular/PeerCommunicationTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ void testSetup() {
configurationBuilder.withValue("socket.timeoutServerAcceptConnect", "100");
configurationBuilder.withValue("socket.timeoutSyncClientSocket", "100");
configurationBuilder.withValue("socket.timeoutSyncClientConnect", "100");
configurationBuilder.withValue("crypto.keystorePassword", "password");

this.configuration = configurationBuilder.build();

Expand Down
13 changes: 9 additions & 4 deletions .../test/java/org/hiero/consensus/gossip/impl/network/connectivity/ConnectivityTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import java.util.Objects;
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.concurrent.atomic.AtomicReference;
import org.hiero.base.crypto.config.CryptoConfig_;
import org.hiero.consensus.gossip.config.SocketConfig;
import org.hiero.consensus.gossip.config.SocketConfig_;

Expand All @@ -25,10 +26,14 @@ class ConnectivityTestBase {
protected static final byte[] TEST_DATA = new byte[] {1, 2, 3};

static {
TLS_NO_IP_TOS_CONFIG =
new TestConfigBuilder().withValue(SocketConfig_.IP_TOS, "-1").getOrCreateConfig();
TLS_IP_TOS_CONFIG =
new TestConfigBuilder().withValue(SocketConfig_.IP_TOS, "100").getOrCreateConfig();
TLS_NO_IP_TOS_CONFIG = new TestConfigBuilder()
.withValue(SocketConfig_.IP_TOS, "-1")
.withValue(CryptoConfig_.KEYSTORE_PASSWORD, "password")
.getOrCreateConfig();
TLS_IP_TOS_CONFIG = new TestConfigBuilder()
.withValue(SocketConfig_.IP_TOS, "100")
.withValue(CryptoConfig_.KEYSTORE_PASSWORD, "password")
.getOrCreateConfig();

final Configuration configurationNoIpTos =
new TestConfigBuilder().withValue(SocketConfig_.IP_TOS, "-1").getOrCreateConfig();
Expand Down
7 changes: 6 additions & 1 deletion ...src/test/java/org/hiero/consensus/gossip/impl/network/connectivity/SocketFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
import java.util.Map;
import java.util.Random;
import java.util.concurrent.atomic.AtomicReference;
import org.hiero.base.crypto.config.CryptoConfig_;
import org.hiero.consensus.gossip.config.GossipConfig;
import org.hiero.consensus.gossip.config.GossipConfig_;
import org.hiero.consensus.gossip.config.NetworkEndpoint;
Expand Down Expand Up @@ -159,6 +160,7 @@ void bindInterfaceTest(
.withValues(
GossipConfig_.INTERFACE_BINDINGS,
List.of("{ \"nodeId\": 0, \"hostname\": \"localhost\", \"port\": 1234 }"))
.withValue(CryptoConfig_.KEYSTORE_PASSWORD, "password")
.getOrCreateConfig();
testInterfaceBinding(node0, roster, keysAndCerts, config, port);
}
Expand All @@ -181,7 +183,9 @@ void bindInterfaceTestWithDefaultConfig(
assertTrue(roster.rosterEntries().size() > 1, "Address book must contain at least 2 nodes");
final NodeId node0 = NodeId.of(roster.rosterEntries().getFirst().nodeId());

final Configuration config = new TestConfigBuilder().getOrCreateConfig();
final Configuration config = new TestConfigBuilder()
.withValue(CryptoConfig_.KEYSTORE_PASSWORD, "password")
.getOrCreateConfig();
testInterfaceBinding(node0, roster, keysAndCerts, config, port);
}

Expand All @@ -207,6 +211,7 @@ void bindInterfaceTestWithFailingClaimingIp(
.withValues(
GossipConfig_.INTERFACE_BINDINGS,
List.of("{ \"nodeId\": 0, \"hostname\": \"10.123.123.123\", \"port\": 1234 }"))
.withValue(CryptoConfig_.KEYSTORE_PASSWORD, "password")
.getOrCreateConfig();

assertThrows(BindException.class, () -> testInterfaceBinding(node0, roster, keysAndCerts, config, port));
Expand Down
6 changes: 3 additions & 3 deletions platform-sdk/docs/consensus-layer/tunables.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,9 +137,9 @@ Startup-time OS health probes; values exceeded at startup produce warning logs b

Module: `base-crypto`. Source: [CryptoConfig.java](../../base-crypto/src/main/java/org/hiero/base/crypto/config/CryptoConfig.java).

| ID | Key | Type | Default | Effect | Range | Fragility |
|---------|---------------------------|--------|------------|----------------------------------------------------------------------------------------|-------|-----------|
| TUN-050 | `crypto.keystorePassword` | String | `password` | Password protecting the PKCS12 key stores that hold node RSA public/private key pairs. | | — |
| ID | Key | Type | Default | Effect | Range | Fragility |
|---------|---------------------------|--------|---------|--------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|-------|-----------|
| TUN-050 | `crypto.keystorePassword` | String | `""` | Password protecting the PKCS12 key stores that hold node RSA public/private key pairs. Intentionally no usable default; node key loading and gossip TLS fail fast if it is not configured. | | — |

## BasicCommonConfig (no prefix)

Expand Down
Loading