Skip to content
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.

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

feat: Update cryptography tools to support tssEncryptionKey loading and generation #16780

Open
wants to merge 13 commits into
base: develop
Choose a base branch
from

Conversation

derektriley
Copy link
Contributor

Description:
This pull request addresses the need for the cryptography code to load a tssEncryptionKey from disk or generate it and write the private key to disk.

Enhancements to TSS key handling:

  • platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/CryptoStatic.java: Added a new method generateBlsKeyPair to generate a BlsKeyPair using a SignatureSchema and an optional SecureRandom instance.

  • platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/EnhancedKeyStoreLoader.java:

    • Added private fields tssPrivateKeys and tssPublicKeys to store TSS encryption keys as they are being read from disk or generated.
    • Updated the scan, generate, and verify methods to handle TSS keys. [1] [2] [3]
    • Added a new method resolveTssPrivateKey to load TSS private keys from disk.
  • platform-sdk/swirlds-platform-core/src/main/java/com/swirlds/platform/crypto/KeysAndCerts.java:

    • Updated the KeysAndCerts record to include TSS encryption keys.
    • Modified the generate and loadExistingAndCreateAgrKeyIfMissing methods to handle TSS keys. [1] [2]

Updates to dependencies and tests:

  • platform-sdk/swirlds-platform-core/src/main/java/module-info.java: Added a dependency on com.hedera.cryptography.bls.

  • platform-sdk/swirlds-platform-core/src/test/java/com/swirlds/platform/crypto/EnhancedKeyStoreLoaderTest.java:

    • Updated the keyStoreLoaderPositiveTest to include a new test case for TSS keys.
    • Added assertions to check for the presence of TSS keys.
  • Added TSS private key files for new test cases. [1] [2] [3]

Related issue(s):

Fixes #14767

Notes for reviewer:

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
@derektriley derektriley marked this pull request as draft November 25, 2024 18:59
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
Copy link

codacy-production bot commented Nov 25, 2024

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation Diff coverage
+0.01% (target: -1.00%) 83.05%
Coverage variation details
Coverable lines Covered lines Coverage
Common ancestor commit (935fdb1) 97511 63616 65.24%
Head commit (3d7c771) 97578 (+67) 63669 (+53) 65.25% (+0.01%)

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details
Coverable lines Covered lines Diff coverage
Pull request (#16780) 59 49 83.05%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Codacy stopped sending the deprecated coverage status on June 5th, 2024. Learn more

Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 79.66102% with 12 lines in your changes missing coverage. Please review.

Project coverage is 63.54%. Comparing base (935fdb1) to head (3d7c771).
Report is 6 commits behind head on develop.

Files with missing lines Patch % Lines
...wirlds/platform/crypto/EnhancedKeyStoreLoader.java 75.00% 9 Missing and 2 partials ⚠️
...java/com/swirlds/platform/crypto/KeysAndCerts.java 90.00% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             develop   #16780   +/-   ##
==========================================
  Coverage      63.53%   63.54%           
- Complexity     20370    20381   +11     
==========================================
  Files           2537     2537           
  Lines          94746    94813   +67     
  Branches        9902     9910    +8     
==========================================
+ Hits           60198    60250   +52     
- Misses         30941    30955   +14     
- Partials        3607     3608    +1     
Files with missing lines Coverage Δ
...java/com/swirlds/platform/crypto/CryptoStatic.java 56.88% <100.00%> (+1.01%) ⬆️
...java/com/swirlds/platform/crypto/KeysAndCerts.java 63.93% <90.00%> (+5.44%) ⬆️
...wirlds/platform/crypto/EnhancedKeyStoreLoader.java 70.07% <75.00%> (+0.39%) ⬆️

... and 11 files with indirect coverage changes

Impacted file tree graph

@derektriley derektriley marked this pull request as ready for review November 25, 2024 22:26
Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
final SignatureSchema SIGNATURE_SCHEMA =
SignatureSchema.create(Curve.ALT_BN128, GroupAssignment.SHORT_SIGNATURES);
if (secureRandom == null) {
return BlsKeyPair.generate(SIGNATURE_SCHEMA);
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: might be beneficial to add a debug log that the default secure random generator is being used here ?

@@ -171,6 +179,11 @@ public static KeysAndCerts generate(
agrDetRandom.setSeed(AGR_SEED);
agrKeyGen.initialize(CryptoConstants.AGR_KEY_SIZE_BITS, agrDetRandom);

tssEncryptionKeyRandom.setSeed(masterKey);
Copy link
Contributor

@thomas-swirlds-labs thomas-swirlds-labs Nov 26, 2024

Choose a reason for hiding this comment

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

Repeated code for setting seeds for different key generators. Could be refactored/extracted into separate methods (but not required 😄 ).

@@ -0,0 +1,3 @@
-----BEGIN PRIVATE KEY-----
AVS7ccxMt5screfvBWYJkUhm3SRhdkNxvsk8KcDMEXgl
-----END PRIVATE KEY-----
Copy link
Contributor

@thomas-swirlds-labs thomas-swirlds-labs Nov 26, 2024

Choose a reason for hiding this comment

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

Would add newlines for consistency, compatibility etc.

Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
@derektriley derektriley requested a review from a team as a code owner November 26, 2024 18:25
@derektriley derektriley requested a review from a team as a code owner November 26, 2024 18:25
@@ -238,16 +238,16 @@ dependencies.constraints {
api("com.google.protobuf:protoc:3.25.4")
api("io.grpc:protoc-gen-grpc-java:1.66.0")

api("com.hedera.cryptography:hedera-cryptography-pairings-api:0.1.0-SNAPSHOT") {
api("com.hedera.cryptography:hedera-cryptography-pairings-api:0.1.1-SNAPSHOT") {
Copy link
Member

Choose a reason for hiding this comment

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

You can merge back develop. This change should be merged

Signed-off-by: Derek Riley <derek.riley@swirldslabs.com>
@Test
@DisplayName("KeyStore Loader Corrupt TSS Key Test")
void keyStoreLoaderNegativeCorruptTssKey() throws IOException {
final Path keyDirectory = testDataDirectory.resolve("enhanced-invalid-case-3");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could pass in directoryName as a parameter for testing multiple versions


assertThat(loader).isNotNull();
assertThatCode(loader::migrate).doesNotThrowAnyException();
assertThatCode(loader::scan).isInstanceOf(KeyLoadingException.class);
Copy link
Contributor

Choose a reason for hiding this comment

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

optional: add custom assertion messages e.g.

assertThatCode(loader::scan)
      .as("Scan operation should throw KeyLoadingException when processing a corrupt TSS key in '%s'", keyDirectory)
      .isInstanceOf(KeyLoadingException.class);

Objects.requireNonNull(nodeId, MSG_NODE_ID_NON_NULL);
Objects.requireNonNull(nodeAlias, MSG_NODE_ALIAS_NON_NULL);

Path keyLocation = keyStoreDirectory.resolve(String.format("t-private-%s.tss", nodeAlias));
Copy link
Contributor

Choose a reason for hiding this comment

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

could validate or sanitize nodeAlias to ensure it is always safe for filenames before using it to construct the path.

*/
public static BlsKeyPair generateBlsKeyPair(@Nullable final SecureRandom secureRandom)
throws NoSuchAlgorithmException {
final SignatureSchema SIGNATURE_SCHEMA =
Copy link
Contributor

Choose a reason for hiding this comment

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

Signature_Schema is constant and could be made static for re-use to avoid recreating it for every call

Copy link
Contributor

@thomas-swirlds-labs thomas-swirlds-labs left a comment

Choose a reason for hiding this comment

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

A few minor comments otherwise LGTM. Thanks! @derektriley

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update cryptography tools to support tssEncryptionKey loading and generation.
3 participants