fix: remove the insecure default for crypto.keystorePassword - #26777
fix: remove the insecure default for crypto.keystorePassword#26777AlexKehayov wants to merge 1 commit into
Conversation
Signed-off-by: Alex Kehayov <aleks.kehayov@limechain.tech>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
🚨 TestLens detected 3319 failed tests 🚨Here is what you can do:
Test Summary600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch) > :test-clients:testSubprocessConcurrent (first 40 of 626)
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Misc Records, Crypto & Misc Serial) > :test-clients:testSubprocessConcurrent (first 40 of 374)
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Misc) > :test-clients:testSubprocessConcurrent (first 40 of 374)
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Restart) > :test-clients:testSubprocess
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Simple Fees & ND Reconnect) > :test-clients:testSubprocessConcurrent (first 40 of 817)
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Smart Contracts & ISS) > :test-clients:testSubprocessConcurrent (first 40 of 548)
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (State Throttling) > :test-clients:testSubprocess
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Token & Time Consuming) > :test-clients:testSubprocess
600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Token & Time Consuming) > :test-clients:testSubprocessConcurrent (first 40 of 515)
600: [FLOW] PR Checks / MATS / Integration Tests / Integration Tests > :consensus-otter-tests:testIntegration
600: [FLOW] PR Checks / MATS / Otter Tests / Fast Otter Tests > :consensus-otter-tests:testOtter
600: [FLOW] PR Checks / MATS / Unit Tests / Unit Tests > :swirlds-platform-core:test
🏷️ Commit: c25837b Test Failures (first 10 of 3322)AtomicAutoAccountCreationSuite (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicAutoAccountUpdateSuite (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAddress167Test (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAddress16cTest (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchApproveAllowanceTest (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAssociatePrecompileV2SecurityModelTest (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAutoAccountCreationBasicTests > Auto Account Create with Public Key ED25519 and ECDSA and FT Transfer success in Atomic Batch - Parametrized > as written (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAutoAccountCreationBasicTests > Auto Account Create with Public Key ED25519 and ECDSA and FT Transfer success in Atomic Batch - Parametrized > as written (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAutoAccountCreationBasicTests > Auto Account Create with Public Key ED25519 and ECDSA and HBAR Transfer success in Atomic Batch - Parametrized > as written (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))AtomicBatchAutoAccountCreationBasicTests > Auto Account Create with Public Key ED25519 and ECDSA and HBAR Transfer success in Atomic Batch - Parametrized > as written (:test-clients:testSubprocessConcurrent in 600: [FLOW] PR Checks / MATS / HAPI Tests / HAPI Tests (Atomic Batch))Muted Tests (first 20 of 3319)Note Muting tests is disabled for this repository. Select tests to mute in this pull request: 🔲 AccessListTest Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app. |
| @ConfigData("crypto") | ||
| public record CryptoConfig( | ||
| @ConfigProperty(defaultValue = "password") String keystorePassword) {} | ||
| @ConfigProperty(defaultValue = "") String keystorePassword) {} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Description
CryptoConfig.keystorePasswordshipped@ConfigProperty(defaultValue = "password"). Its consumers (CryptoUtils.getConfiguredKeystorePassword, used by node key loading and gossip TLS setup) are written to fail fast when the password is not configured, but the non-blank"password"default meant that check never fired: an unconfigured node silently protected its PKCS12 key stores with the well-known string"password".This removes the default (sets it to
"") so an unconfigured node fails fast during key loading / gossip TLS setup instead of falling back to"password". It also updates the tunables doc and adds a test asserting there is no usable default.Compatibility — breaking change
Any deployment that relied on the old
"password"default will fail to start untilcrypto.keystorePasswordis set. Existing nodes whose key stores were created with"password"must setcrypto.keystorePassword=password(or re-key the stores) before upgrading; leaving it unset will fail fast. Sequence this with the deploy: setcrypto.keystorePasswordout of band and confirm the existing key stores open with it before this change lands.Testing
base-crypto: addedCryptoConfigTest.testNoUsableDefaultKeystorePassword(the default is null/blank). The existing "all defaults valid" test still passes because""is a valid build-time value. 55 tests pass.consensus-gossip-impl:SocketFactoryTest,ConnectivityTestBase, andPeerCommunicationTestsnow setcrypto.keystorePasswordexplicitly (they relied on the old default). 253 tests pass.TlsFactoryTest,EnhancedKeyStoreLoaderTest) already set the password and are unaffected../gradlew :base-crypto:test :consensus-gossip-impl:test— pass.