Investigating issues with ECDSA_SECP256k1 #53
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe updates add custom logging and expand cryptographic functionalities within the SDK for Java and Kotlin applications. This involves integrating SLF4J and Logback, adding new cryptographic algorithms, and including comprehensive tests for these features. Improvements to logging include the introduction of a custom logger through Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
Unit Test Results 53 files ±0 53 suites ±0 7s ⏱️ ±0s For more details on these failures, see this check. Results for commit 59feb70. ± Comparison against base commit 797007a. ♻️ This comment has been updated with latest results. |
Integration Test Results0 files - 7 0 suites - 7 0s ⏱️ -27s Results for commit 38f19fb. ± Comparison against base commit a51dee8. This pull request removes 33 tests.♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Outside diff range and nitpick comments (1)
src/main/kotlin/org/onflow/flow/sdk/models.kt (1)
85-86: Add documentation for new hash algorithmsKECCAK256andKMAC128.It would be beneficial to add documentation comments explaining the use cases and specifics of the new hash algorithms
KECCAK256andKMAC128.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- README.md (1 hunks)
- build.gradle.kts (1 hunks)
- src/main/kotlin/org/onflow/flow/sdk/LoggerProvider.kt (1 hunks)
- src/main/kotlin/org/onflow/flow/sdk/SdkConfig.kt (1 hunks)
- src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (3 hunks)
- src/main/kotlin/org/onflow/flow/sdk/models.kt (3 hunks)
- src/main/kotlin/resources/logback.xml (1 hunks)
- src/test/kotlin/org/onflow/flow/sdk/LoggerProviderTest.kt (1 hunks)
- src/test/kotlin/org/onflow/flow/sdk/SdkConfigTest.kt (1 hunks)
- src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt (4 hunks)
- src/test/kotlin/org/onflow/flow/sdk/models/HashAlgorithmTest.kt (1 hunks)
Files skipped from review due to trivial changes (3)
- src/main/kotlin/org/onflow/flow/sdk/SdkConfig.kt
- src/main/kotlin/resources/logback.xml
- src/test/kotlin/org/onflow/flow/sdk/LoggerProviderTest.kt
Additional context used
LanguageTool
README.md
[style] ~8-~8: For conciseness, consider replacing this expression with an adverb.
Context: ... to interact with the Flow blockchain. At the moment, this SDK includes the following featur...
[style] ~210-~210: Style-wise, it’s not ideal to insert an adverb (‘automatically’) in the middle of an infinitive construction (‘to create’). Try moving the adverb to avoid split infinitives.
Context: ...ount credentials -@FlowTestAccount- used to automatically create an account in the emulator and inject a `TestAccou...
[misspelling] ~218-~218: Although the phrase ‘are welcomed’ is grammatically correct, possibly, the meaning is not correct in this context. Did you mean “welcome”?
Context: ...very early phase; all contributions are welcomed. Read the [contributing guide](https:/...
Markdownlint
README.md
111-111: Expected: h3; Actual: h4
Heading levels should only increment by one level at a time
229-229: Expected: dash; Actual: asterisk
Unordered list style
230-230: Expected: dash; Actual: asterisk
Unordered list style
231-231: Expected: dash; Actual: asterisk
Unordered list style
232-232: Expected: dash; Actual: asterisk
Unordered list style
233-233: Expected: dash; Actual: asterisk
Unordered list style
230-230: Expected: 2; Actual: 3
Unordered list indentation
231-231: Expected: 2; Actual: 3
Unordered list indentation
233-233: Expected: 2; Actual: 3
Unordered list indentation
3-3: Expected: 0 or 2; Actual: 1
Trailing spaces
10-10: Expected: 0 or 2; Actual: 1
Trailing spaces
18-18: Expected: 0 or 2; Actual: 1
Trailing spaces
231-231: Expected: 0 or 2; Actual: 1
Trailing spaces
234-234: Expected: 0 or 2; Actual: 1
Trailing spaces
235-235: Expected: 0 or 2; Actual: 5
Trailing spaces
235-235: Expected: 1; Actual: 2
Multiple consecutive blank lines
236-236: Expected: 1; Actual: 3
Multiple consecutive blank lines
141-141: null
Fenced code blocks should be surrounded by blank lines
163-163: null
Fenced code blocks should be surrounded by blank lines
10-10: null
Lists should be surrounded by blank lines
229-229: null
Lists should be surrounded by blank lines
Additional comments not posted (8)
src/main/kotlin/org/onflow/flow/sdk/LoggerProvider.kt (2)
7-7: Initialization of the logger with a default name "DefaultLogger" is appropriate and follows standard practices.
9-13: The implementation of the getter and setter for the logger is correct and provides necessary flexibility for changing the logger at runtime.src/test/kotlin/org/onflow/flow/sdk/SdkConfigTest.kt (1)
7-14: The testtestSetLoggercorrectly verifies the functionality of setting a custom logger throughSdkConfig. It uses appropriate assertions to ensure that the logger is set correctly.src/test/kotlin/org/onflow/flow/sdk/models/HashAlgorithmTest.kt (1)
22-23: The tests forKECCAK256andKMAC128correctly verify the mapping from Cadence indices toHashAlgorithmvalues. This ensures that the enum mappings are accurate and functional.build.gradle.kts (1)
65-66: The addition ofslf4j-apiandlogback-classicdependencies is appropriate for enabling logging capabilities within the SDK. These dependencies are correctly configured in the implementation scope.src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt (1)
66-71: The tests forECDSA_SECP256k1correctly verify the signing and normalization of signatures. These tests are crucial for ensuring the cryptographic integrity and functionality of the SDK.Also applies to: 95-110, 130-136
README.md (1)
89-130: The added documentation on integrating a custom logger with the SDK is comprehensive and clear. It provides valuable guidance for developers on how to customize the logging functionality of the SDK.src/main/kotlin/org/onflow/flow/sdk/models.kt (1)
60-60: Ensure theidforECDSA_SECP256k1is consistent with standards.Verification successful
The
idvalues forECDSA_SECP256k1(3 and 2) appear to be used consistently across the codebase. However, to ensure these values are correct and align with the standards, we need to verify their correctness.
The
idvalues forECDSA_SECP256k1(3 and 2) are used consistently in the codebase, particularly in theSignatureAlgorithmTest.ktfile, where they are tested for correctness. This indicates that the values are correctly implemented and align with the expected standards.
SignatureAlgorithm.fromCode(3)returnsSignatureAlgorithm.ECDSA_SECP256k1.SignatureAlgorithm.fromCadenceIndex(2)returnsSignatureAlgorithm.ECDSA_SECP256k1.These tests confirm that the
idvalues are correctly mapped and used.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the `id` for `ECDSA_SECP256k1` across different standards. # Test: Search for the `id` usage. Expect: Consistent usage across the codebase. rg --type kotlin $'ECDSA_SECP256k1'Length of output: 1984
Script:
#!/bin/bash # Description: Verify the correctness of the `id` values for `ECDSA_SECP256k1`. # Test: Search for the `id` values (3 and 2) usage in the codebase. rg --type kotlin '3' | rg 'SignatureAlgorithm' rg --type kotlin '2' | rg 'SignatureAlgorithm'Length of output: 5934
| val signature: ByteArray | ||
| val ecdsaSign: Signature | ||
|
|
||
| if (privateKey.key.algorithm == "EC" && privateKey.key is ECPrivateKey) { | ||
| // Use Bouncy Castle for secp256k1 | ||
| ecdsaSign = Signature.getInstance("ECDSA", "BC") | ||
| } else { | ||
| // Use default provider for other algorithms | ||
| ecdsaSign = Signature.getInstance(hashAlgo.id) | ||
| } | ||
|
|
||
| ecdsaSign.initSign(privateKey.key) | ||
| ecdsaSign.update(bytes) | ||
|
|
||
| val signature = ecdsaSign.sign() | ||
| if (hashAlgo == HashAlgorithm.KECCAK256 || hashAlgo == HashAlgorithm.KMAC128) { | ||
| // Handle Keccak-256 and KMAC128 separately | ||
| val hash = hasher.hash(bytes) | ||
| val noneEcdsaSign = Signature.getInstance("NONEwithECDSA", "BC") | ||
| noneEcdsaSign.initSign(privateKey.key) | ||
| noneEcdsaSign.update(hash) | ||
| signature = noneEcdsaSign.sign() | ||
| } else { | ||
| // Handle other algorithms with valid IDs | ||
| ecdsaSign.update(bytes) | ||
| signature = ecdsaSign.sign() | ||
| } | ||
|
|
There was a problem hiding this comment.
Refactor to simplify the conditional logic in the signing process.
- if (privateKey.key.algorithm == "EC" && privateKey.key is ECPrivateKey) {
- ecdsaSign = Signature.getInstance("ECDSA", "BC")
- } else {
- ecdsaSign = Signature.getInstance(hashAlgo.id)
- }
+ ecdsaSign = Signature.getInstance(privateKey.key.algorithm, "BC")This change simplifies the conditional logic by directly using the algorithm specified in the private key.
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.
| val signature: ByteArray | |
| val ecdsaSign: Signature | |
| if (privateKey.key.algorithm == "EC" && privateKey.key is ECPrivateKey) { | |
| // Use Bouncy Castle for secp256k1 | |
| ecdsaSign = Signature.getInstance("ECDSA", "BC") | |
| } else { | |
| // Use default provider for other algorithms | |
| ecdsaSign = Signature.getInstance(hashAlgo.id) | |
| } | |
| ecdsaSign.initSign(privateKey.key) | |
| ecdsaSign.update(bytes) | |
| val signature = ecdsaSign.sign() | |
| if (hashAlgo == HashAlgorithm.KECCAK256 || hashAlgo == HashAlgorithm.KMAC128) { | |
| // Handle Keccak-256 and KMAC128 separately | |
| val hash = hasher.hash(bytes) | |
| val noneEcdsaSign = Signature.getInstance("NONEwithECDSA", "BC") | |
| noneEcdsaSign.initSign(privateKey.key) | |
| noneEcdsaSign.update(hash) | |
| signature = noneEcdsaSign.sign() | |
| } else { | |
| // Handle other algorithms with valid IDs | |
| ecdsaSign.update(bytes) | |
| signature = ecdsaSign.sign() | |
| } | |
| val signature: ByteArray | |
| val ecdsaSign: Signature | |
| ecdsaSign = Signature.getInstance(privateKey.key.algorithm, "BC") | |
| ecdsaSign.initSign(privateKey.key) | |
| if (hashAlgo == HashAlgorithm.KECCAK256 || hashAlgo == HashAlgorithm.KMAC128) { | |
| // Handle Keccak-256 and KMAC128 separately | |
| val hash = hasher.hash(bytes) | |
| val noneEcdsaSign = Signature.getInstance("NONEwithECDSA", "BC") | |
| noneEcdsaSign.initSign(privateKey.key) | |
| noneEcdsaSign.update(hash) | |
| signature = noneEcdsaSign.sign() | |
| } else { | |
| // Handle other algorithms with valid IDs | |
| ecdsaSign.update(bytes) | |
| signature = ecdsaSign.sign() | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 3
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (3 hunks)
- src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt (1 hunks)
- src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt (4 hunks)
Additional context used
Learnings (2)
src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (1)
Learnt from: lealobanov PR: onflow/flow-jvm-sdk#52 File: src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt:116-122 Timestamp: 2024-07-04T12:45:26.335Z Learning: The "Sanity check KMAC128" test in `CryptoTest.kt` covers various key and customizer combinations and verifies the hash output against known good values.src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt (1)
Learnt from: lealobanov PR: onflow/flow-jvm-sdk#52 File: src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt:109-114 Timestamp: 2024-07-04T12:44:06.833Z Learning: The "Sanity check Keccak_256" test in `CryptoTest.kt` verifies the correctness of the Keccak-256 hash output against known good values.
Gitleaks
src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt
163-163: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
281-281: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Additional comments not posted (5)
src/test/kotlin/org/onflow/flow/sdk/ScriptTest.kt (2)
88-96: Verify the relocation of themainfunction.The
mainfunction has been moved inside the script block. Ensure that this change is consistent with the rest of the codebase and does not introduce any issues.Verification successful
Verified the relocation of the
mainfunction.The
mainfunction has been correctly moved inside the script block inScriptTest.kt. All occurrences ofpub fun mainwithinScriptTest.ktare within script blocks, maintaining consistency.
src/test/kotlin/org/onflow/flow/sdk/ScriptTest.ktScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify consistency of the `main` function relocation. # Test: Search for the usage of `main` function inside script blocks. Expect: All occurrences should be inside script blocks. rg --type cadence $'pub fun main'Length of output: 66
Script:
#!/bin/bash # Description: Verify consistency of the `main` function relocation. # Test: Search for the usage of `main` function inside Kotlin files. Expect: All occurrences should be inside script blocks. rg --type kotlin 'pub fun main'Length of output: 1392
Script:
#!/bin/bash # Description: Verify that all occurrences of `pub fun main` in `ScriptTest.kt` are within script blocks. # Extract and display 5 lines before and after each occurrence of `pub fun main` in `ScriptTest.kt` to verify the context. rg --type kotlin 'pub fun main' -C 5 src/test/kotlin/org/onflow/flow/sdk/ScriptTest.ktLength of output: 1198
74-96: Ensure consistency with struct naming conventions and initialization.The
TestClassstruct's constructor has been renamed toinit. Ensure that this change is consistent with the rest of the codebase and adheres to the naming conventions for initializers in Cadence scripts.src/main/kotlin/org/onflow/flow/sdk/crypto/Crypto.kt (1)
201-208: Ensure proper finalization ofKMAChashing.The
doFinalmethod properly finalizes theKMAChashing. Ensure that the output size is correctly handled.src/test/kotlin/org/onflow/flow/sdk/crypto/CryptoTest.kt (2)
210-217: Ensure comprehensive test coverage forKECCAK256.The tests for
KECCAK256should cover all relevant cases, including edge cases.
220-251: Ensure comprehensive test coverage forKMAC128.The tests for
KMAC128should cover all relevant cases, including edge cases.
|
Thanks for rebasing the changes to the hashing branch 👌🏼 |
|
@tarakby , yes the related issue is: the-nft-company/flow-jvm-sdk#24 which links to 2 more follow-up issues. Will follow up on latest comments for hashing PR tomorrow as well. Thanks! |
| @@ -60,6 +63,13 @@ internal class CryptoTest { | |||
| assertNotNull(signer) | |||
| } | |||
|
|
|||
| @Test | |||
| fun `Get signer for ECDSA_SECP256k1`() { | |||
There was a problem hiding this comment.
While we are looking at the signer class I think it makes sense to improve the quality of these tests because they follow the same paradigm as the old hasher tests... will have a look at sanity checks Go SDK is currently using
|
I think #71 covered the same issues addressed by this current PR, so it should be fine to close this PR. |
Investigating reported GH issues with ECDSA_SECP256k1
For contributor use:
masterbranchFiles changedin the Github PR explorerSummary by CodeRabbit
New Features
SdkConfigto allow custom logger integration.KECCAK256andKMAC128.Documentation
Tests
Enhancements
HashAlgorithmto includeKECCAK256andKMAC128.