feat(sdk)!: remove bouncycastle and implement things in terms of standard Java APIs#365
feat(sdk)!: remove bouncycastle and implement things in terms of standard Java APIs#365mkleene wants to merge 13 commits into
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR migrates the Java SDK from BouncyCastle-backed cryptography to JDK/JCE native implementations. BouncyCastle dependencies are restricted to test scope. Core ECKeyPair methods now use KeyPairGenerator, KeyAgreement, HmacSHA256, and Signature. Production services are updated to decode EC keys via KeyFactory. Tests use a new PemUtils utility for BouncyCastle compatibility in test code. ChangesBouncyCastle Removal & JDK Native Crypto Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request removes BouncyCastle as a compile-time dependency, replacing its functionality with standard Java Cryptography Architecture (JCA) APIs. The ECKeyPair class was refactored to use the standard 'EC' KeyPairGenerator, and manual implementations for PEM encoding, EC point compression, and HKDF were introduced. Review feedback suggests simplifying the ECKeyPair constructor by removing the redundant ECAlgorithm parameter and centralizing the PEM parsing logic to eliminate code duplication between KASClient and TDF.
There was a problem hiding this comment.
Code Review
This pull request refactors the SDK to remove BouncyCastle as a runtime dependency, replacing its cryptographic and PEM handling functionality with standard Java Security APIs. While the transition reduces external dependencies, the review highlights several critical issues: a missing utility class (PemUtils) will cause compilation failures, and the keySize calculation incorrectly uses DER-encoded lengths instead of the curve's field size. Additionally, the reviewer noted significant code duplication in PEM parsing across KASClient and TDF, a typo in a method name, and misleading parameter naming in the signature verification methods.
| public int keySize() { | ||
| return this.keyPair.getPrivate().getEncoded().length * 8; | ||
| } |
There was a problem hiding this comment.
The keySize() method incorrectly calculates the key size by using the length of the DER-encoded private key (PKCS#8), which includes significant overhead from headers and metadata. For Elliptic Curve keys, the key size should be the field size of the curve in bits (e.g., 256 for P-256).
| public int keySize() { | |
| return this.keyPair.getPrivate().getEncoded().length * 8; | |
| } | |
| public int keySize() { | |
| return getPublicKey().getParams().getCurve().getField().getFieldSize(); | |
| } |
There was a problem hiding this comment.
this seems legit but I'm not sure how many breaking-ish changes we want in this pull
| } | ||
| } | ||
|
|
||
| public static Boolean verifyECDSAig(byte[] digest, byte[] signature, ECPublicKey publicKey) { |
There was a problem hiding this comment.
There is a typo in the method name verifyECDSAig (missing 's'). It should be verifyECDSASig. Additionally, the parameter name digest should be renamed to data for consistency with the fact that SHA256withECDSA performs hashing on the input.
| public static Boolean verifyECDSAig(byte[] digest, byte[] signature, ECPublicKey publicKey) { | |
| public static Boolean verifyECDSASig(byte[] data, byte[] signature, ECPublicKey publicKey) { |
| byte[] der = Base64.getDecoder().decode(kasInfo.PublicKey | ||
| .replaceAll("-----[^-]+-----", "") | ||
| .replaceAll("\\s+", "")); | ||
| kasPubKey = (ECPublicKey) KeyFactory.getInstance("EC") | ||
| .generatePublic(new X509EncodedKeySpec(der)); |
There was a problem hiding this comment.
This PEM parsing logic is duplicated here and in KASClient.java. Consider centralizing this logic in ECKeyPair.java or a dedicated utility class to adhere to DRY principles.
X-Test Failure Report |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java (1)
57-78:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical blocker:
PemUtilsclass required for EC key conversion is missing.
ECKeyPairTest.javaandTDFTest.javacallPemUtilsmethods (publicKeyFromPem,privateKeyFromPem,compressECPublickey,publicKeyFromECPoint,getPEMPublicKeyFromX509Cert) that do not exist. Compilation fails with "cannot find symbol" at 17 call sites inECKeyPairTest.javaalone (lines 57, 58, 64, 67, 77, 78, 97, 100, 101, 104, 115, 141, 142, 144, 145, 158, 162).Note:
SDKBuilderTest.javaimports the external librarynl.altindag.ssl.pem.util.PemUtilsfor certificate operations, but test code needs a separatePemUtilsutility in test scope for EC key serialization.Required API:
ECPublicKey publicKeyFromPem(String pem)ECPrivateKey privateKeyFromPem(String pem)byte[] compressECPublickey(String pem)String publicKeyFromECPoint(byte[] compressed, String curveName)String getPEMPublicKeyFromX509Cert(String certPem)Create
sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.javawith these static methods to unblock the build.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java` around lines 57 - 78, Tests fail because a test-scoped PemUtils class is missing; add sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java implementing the required static methods: ECPublicKey publicKeyFromPem(String pem), ECPrivateKey privateKeyFromPem(String pem), byte[] compressECPublickey(String pem), String publicKeyFromECPoint(byte[] compressed, String curveName), and String getPEMPublicKeyFromX509Cert(String certPem). Implement each to parse PEM strings into JCA EC keys (ECPublicKey/ECPrivateKey), convert EC public keys to/from compressed point bytes using the named curve (SECP256R1), produce PEM-formatted public key strings from an EC point, and extract a PEM public key from an X.509 certificate; ensure methods use java.security / BouncyCastle utilities and match the signatures used by ECKeyPairTest and TDFTest so compilation succeeds.
🧹 Nitpick comments (3)
sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java (2)
185-194: ⚡ Quick winRecommended: extract this PEM →
ECPublicKeyblock into a small shared helper.The exact same six‑line decode (strip PEM markers, drop whitespace, Base64‑decode, build
X509EncodedKeySpec, askKeyFactory("EC")for anECPublicKey) was added in two places: here andTDF.java(lines 250–259). SincePemUtilsis being introduced for the tests anyway, consider promoting it tosdk/src/main/...and routing both call sites through it. That also gives you one place to add input validation and unit‑test it.♻️ Sketch
// in io.opentdf.platform.sdk.PemUtils (main scope) public static ECPublicKey ecPublicKeyFromPem(String pem) { try { byte[] der = Base64.getDecoder().decode( pem.replaceAll("-----[^-]+-----", "").replaceAll("\\s+", "")); return (ECPublicKey) KeyFactory.getInstance("EC") .generatePublic(new X509EncodedKeySpec(der)); } catch (NoSuchAlgorithmException | InvalidKeySpecException | IllegalArgumentException e) { throw new SDKException("error decoding EC public key", e); } }Then both
KASClient.unwrapandTDF.createECWrappedKeybecome a one‑liner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java` around lines 185 - 194, The PEM→ECPublicKey decoding block duplicated in KASClient.unwrap and TDF.createECWrappedKey should be extracted into a new sdk main class (e.g., io.opentdf.platform.sdk.PemUtils) as a method like ecPublicKeyFromPem(String pem) that strips PEM markers/whitespace, Base64-decodes and returns an ECPublicKey, throwing SDKException on failure; replace the inline code in KASClient.unwrap and TDF.createECWrappedKey with calls to PemUtils.ecPublicKeyFromPem, add basic input validation inside the helper (null/empty and valid Base64) and move the existing test helpers into sdk/src/test to unit-test the new method.
187-189: 💤 Low valueMinor: the new parsing is more lenient than
ECKeyPair.publicKeyFromPemwas.
replaceAll("-----[^-]+-----", "")silently matches nothing when the input has no PEM markers, so a raw Base64 blob (or a totally malformed string) will reachKeyFactoryand surface a confusingInvalidKeySpecExceptioninstead of a clear "not a PEM" error. The KAS response should always be PEM today, so this is low risk, but a quick guard would make failures more debuggable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java` around lines 187 - 189, The current decoding in KASClient (where kasEphemeralPublicKey is processed before Base64 decoding) is too permissive and can let raw or malformed strings reach KeyFactory; add an explicit guard that checks for PEM markers (e.g., "-----BEGIN" and "-----END") on kasEphemeralPublicKey before calling replaceAll/decoding and, if absent, throw a clear IllegalArgumentException (or a domain-specific exception) stating the KAS response was not PEM-formatted; this change should be applied at the decode site in KASClient.java so callers get a deterministic "not a PEM" error instead of an InvalidKeySpecException from KeyFactory.sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java (1)
91-110: 💤 Low valueHKDF implementation is correct for the fixed 32‑byte output, but the limitation is now hard‑coded.
The single‑block Expand (
hmac.update((byte) 0x01); return hmac.doFinal();) only works becauseL = HashLen = 32andinfois empty. The current callers (createECWrappedKey,unwrap) all rely on a 32‑byte session key, so this is functionally fine.Consider tightening the Javadoc so future callers don't assume a general HKDF — and so a reader can match the code to RFC 5869 §2.3 with
T(1) = HMAC(PRK, 0x01).📝 Optional doc tweak
/** - * Returns a HKDF key derived from the provided salt and secret - * that is 32 bytes (256 bits) long. + * Returns a 32-byte (256-bit) HKDF-SHA256 key derived from {`@code` secret} using {`@code` salt}. + * + * <p>This is a fixed-output specialization of RFC 5869: Extract with the given salt + * (zero-filled HashLen bytes if {`@code` salt} is null/empty), followed by a single + * Expand block with empty {`@code` info} and counter {`@code` 0x01}. It is not a + * general HKDF and will not produce more than 32 bytes of output. + */🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java` around lines 91 - 110, The Javadoc for calculateHKDF should explicitly state this is a constrained HKDF-Expand that returns a single 32-byte HMAC-SHA256 block (T(1) = HMAC(PRK, 0x01)) with empty info per RFC 5869 §2.3, not a general HKDF capable of arbitrary-length output; update the comment on the calculateHKDF method to document the salt substitution behavior, the fixed 32-byte output, the single-block expand formula (T(1)=HMAC(PRK, 0x01)), and the RFC reference so future callers (e.g., createECWrappedKey, unwrap) don’t assume general HKDF behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java`:
- Around line 57-78: Tests fail because a test-scoped PemUtils class is missing;
add sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java implementing the
required static methods: ECPublicKey publicKeyFromPem(String pem), ECPrivateKey
privateKeyFromPem(String pem), byte[] compressECPublickey(String pem), String
publicKeyFromECPoint(byte[] compressed, String curveName), and String
getPEMPublicKeyFromX509Cert(String certPem). Implement each to parse PEM strings
into JCA EC keys (ECPublicKey/ECPrivateKey), convert EC public keys to/from
compressed point bytes using the named curve (SECP256R1), produce PEM-formatted
public key strings from an EC point, and extract a PEM public key from an X.509
certificate; ensure methods use java.security / BouncyCastle utilities and match
the signatures used by ECKeyPairTest and TDFTest so compilation succeeds.
---
Nitpick comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 91-110: The Javadoc for calculateHKDF should explicitly state this
is a constrained HKDF-Expand that returns a single 32-byte HMAC-SHA256 block
(T(1) = HMAC(PRK, 0x01)) with empty info per RFC 5869 §2.3, not a general HKDF
capable of arbitrary-length output; update the comment on the calculateHKDF
method to document the salt substitution behavior, the fixed 32-byte output, the
single-block expand formula (T(1)=HMAC(PRK, 0x01)), and the RFC reference so
future callers (e.g., createECWrappedKey, unwrap) don’t assume general HKDF
behavior.
In `@sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java`:
- Around line 185-194: The PEM→ECPublicKey decoding block duplicated in
KASClient.unwrap and TDF.createECWrappedKey should be extracted into a new sdk
main class (e.g., io.opentdf.platform.sdk.PemUtils) as a method like
ecPublicKeyFromPem(String pem) that strips PEM markers/whitespace,
Base64-decodes and returns an ECPublicKey, throwing SDKException on failure;
replace the inline code in KASClient.unwrap and TDF.createECWrappedKey with
calls to PemUtils.ecPublicKeyFromPem, add basic input validation inside the
helper (null/empty and valid Base64) and move the existing test helpers into
sdk/src/test to unit-test the new method.
- Around line 187-189: The current decoding in KASClient (where
kasEphemeralPublicKey is processed before Base64 decoding) is too permissive and
can let raw or malformed strings reach KeyFactory; add an explicit guard that
checks for PEM markers (e.g., "-----BEGIN" and "-----END") on
kasEphemeralPublicKey before calling replaceAll/decoding and, if absent, throw a
clear IllegalArgumentException (or a domain-specific exception) stating the KAS
response was not PEM-formatted; this change should be applied at the decode site
in KASClient.java so callers get a deterministic "not a PEM" error instead of an
InvalidKeySpecException from KeyFactory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fa646f66-b51a-48ad-9304-553551f35479
📒 Files selected for processing (6)
sdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/KASClient.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javasdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java
mkleene
changed the title
X-Test Failure Report |
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
X-Test Failure Report |
X-Test Failure Report |
mkleene
changed the title
X-Test Results✅ java-v0.15.0 |
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java (1)
38-42: ⚡ Quick winDrop the JVM-wide provider registration from this test helper.
Every crypto call in this class already passes
PROVIDERexplicitly, soSecurity.addProvider(PROVIDER)is redundant and mutates global JVM state for the whole test run. That makes unrelated tests more order-dependent than they need to be.Suggested cleanup
-import java.security.Security; @@ - static { - Security.addProvider(PROVIDER); - } - private PemUtils() { }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java` around lines 38 - 42, The static block in PemUtils that calls Security.addProvider(PROVIDER) mutates JVM-global state and should be removed; leave the private static final BouncyCastleProvider PROVIDER field in place and ensure all crypto calls (those already passing PROVIDER) continue to use that constant explicitly, removing the Security.addProvider(PROVIDER) static initializer so the test helper no longer registers the provider JVM-wide.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 122-139: Rename the method verifyECDSAig to verifyECDSASig and
rename its parameter from digest to message (since both computeECDSASig and
verifyECDSASig call Signature.getInstance("SHA256withECDSA") which hashes
internally), update any internal references/tests to use the new name/parameter,
and add Javadoc to both computeECDSASig(byte[] data, ECPrivateKey privateKey)
and verifyECDSASig(byte[] message, ECPublicKey publicKey) clarifying they accept
the raw message bytes (not a precomputed digest) and that the SHA-256 hashing is
performed internally by the signature algorithm.
- Around line 54-58: The method publicKeyFromPem currently lets
IllegalArgumentException from Base64.getDecoder().decode(pemData) escape,
violating its declared throws; wrap the Base64 decoding in a try/catch that
catches IllegalArgumentException and rethrows a new InvalidKeySpecException
(including the original exception as the cause and a clear message), then
continue with KeyFactory.generatePublic as before so callers that catch
InvalidKeySpecException get the wrapped error.
---
Nitpick comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java`:
- Around line 38-42: The static block in PemUtils that calls
Security.addProvider(PROVIDER) mutates JVM-global state and should be removed;
leave the private static final BouncyCastleProvider PROVIDER field in place and
ensure all crypto calls (those already passing PROVIDER) continue to use that
constant explicitly, removing the Security.addProvider(PROVIDER) static
initializer so the test helper no longer registers the provider JVM-wide.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7e8edc45-a2fb-4168-a8f9-b2542363a8ae
📒 Files selected for processing (5)
sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/KASClient.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javasdk/src/test/java/io/opentdf/platform/sdk/PemUtils.java
🚧 Files skipped from review as they are similar to previous changes (2)
- sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java
- sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java
| static ECPublicKey publicKeyFromPem(String pem) throws InvalidKeySpecException, NoSuchAlgorithmException { | ||
| String pemData = pem.replaceAll("-----(BEGIN|END) [A-Z ]+-----", "").replaceAll("\\s", ""); | ||
| byte[] der = Base64.getDecoder().decode(pemData); | ||
| KeyFactory keyFactory = KeyFactory.getInstance("EC"); | ||
| return (ECPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(der)); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find and read the ECKeyPair.java file around the mentioned lines
fd -t f "ECKeyPair.java" sdk/src/mainRepository: opentdf/java-sdk
Length of output: 117
🏁 Script executed:
# Get the full content of ECKeyPair.java to understand the context
wc -l sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javaRepository: opentdf/java-sdk
Length of output: 121
🏁 Script executed:
# Read the file with context around lines 54-58
cat -n sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java | head -80 | tail -50Repository: opentdf/java-sdk
Length of output: 1988
🏁 Script executed:
# Search for all calls to publicKeyFromPem to see how exceptions are handled
rg "publicKeyFromPem" -A 5 -B 2Repository: opentdf/java-sdk
Length of output: 8132
Wrap Base64.getDecoder().decode() to align exception contract with callers.
Base64.getDecoder().decode(pemData) throws IllegalArgumentException when the Base64 input is malformed. Since this method declares only throws InvalidKeySpecException, NoSuchAlgorithmException, the unchecked exception escapes to call sites in TDF.java and KASClient.java that only catch the declared exceptions. This breaks the exception contract and allows unexpected runtime exceptions to leak instead of being wrapped in SDKException.
Catch IllegalArgumentException and convert it to InvalidKeySpecException to match the method's declared exception contract.
Suggested fix
static ECPublicKey publicKeyFromPem(String pem) throws InvalidKeySpecException, NoSuchAlgorithmException {
- String pemData = pem.replaceAll("-----(BEGIN|END) [A-Z ]+-----", "").replaceAll("\\s", "");
- byte[] der = Base64.getDecoder().decode(pemData);
- KeyFactory keyFactory = KeyFactory.getInstance("EC");
- return (ECPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(der));
+ try {
+ String pemData = pem.replaceAll("-----(BEGIN|END) [A-Z ]+-----", "").replaceAll("\\s", "");
+ byte[] der = Base64.getDecoder().decode(pemData);
+ KeyFactory keyFactory = KeyFactory.getInstance("EC");
+ return (ECPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(der));
+ } catch (IllegalArgumentException e) {
+ throw new InvalidKeySpecException("invalid EC public key PEM", e);
+ }
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java` around lines 54 -
58, The method publicKeyFromPem currently lets IllegalArgumentException from
Base64.getDecoder().decode(pemData) escape, violating its declared throws; wrap
the Base64 decoding in a try/catch that catches IllegalArgumentException and
rethrows a new InvalidKeySpecException (including the original exception as the
cause and a clear message), then continue with KeyFactory.generatePublic as
before so callers that catch InvalidKeySpecException get the wrapped error.
| public static byte[] computeECDSASig(byte[] data, ECPrivateKey privateKey) { | ||
| try { | ||
| Signature ecdsaSign = Signature.getInstance("SHA256withECDSA", "BC"); | ||
| Signature ecdsaSign = Signature.getInstance("SHA256withECDSA"); | ||
| ecdsaSign.initSign(privateKey); | ||
| ecdsaSign.update(digest); | ||
| ecdsaSign.update(data); | ||
| return ecdsaSign.sign(); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (NoSuchProviderException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (InvalidKeyException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (SignatureException e) { | ||
| } catch (NoSuchAlgorithmException | InvalidKeyException | SignatureException e) { | ||
| throw new RuntimeException(e); | ||
| } | ||
| } | ||
|
|
||
| public static Boolean verifyECDSAig(byte[] digest, byte[] signature, ECPublicKey publicKey) { | ||
| try { | ||
| Signature ecdsaVerify = Signature.getInstance("SHA256withECDSA", "BC"); | ||
| Signature ecdsaVerify = Signature.getInstance("SHA256withECDSA"); | ||
| ecdsaVerify.initVerify(publicKey); | ||
| ecdsaVerify.update(digest); | ||
| return ecdsaVerify.verify(signature); | ||
| } catch (NoSuchAlgorithmException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (NoSuchProviderException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (InvalidKeyException e) { | ||
| throw new RuntimeException(e); | ||
| } catch (SignatureException e) { | ||
| } catch (NoSuchAlgorithmException | InvalidKeyException | SignatureException e) { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect whether callers pass raw payloads or precomputed digests into the ECKeyPair ECDSA helpers.
rg -n --type=java -C3 '\b(computeECDSASig|verifyECDSAig)\s*\('
rg -n --type=java -C3 'MessageDigest|getInstance\("SHA-256"|digest\s*\('Repository: opentdf/java-sdk
Length of output: 8366
🏁 Script executed:
# Search for all usages across the codebase
rg -n --type=java '\b(computeECDSASig|verifyECDSAig)\b' -A2 -B2
# Check for Javadoc or comments near the method definitions
grep -n -B5 "public static.*computeECDSASig\|public static.*verifyECDSAig" sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java
# Search for any references in Manifest or TDF that might interact
rg -n --type=java 'ECKeyPair\.' sdk/src/main/java/io/opentdf/platform/sdk/Repository: opentdf/java-sdk
Length of output: 3138
🏁 Script executed:
sed -n '115,145p' sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javaRepository: opentdf/java-sdk
Length of output: 1298
Fix parameter naming and method name typo before finalizing public SDK API contract.
verifyECDSAig() has a misleading parameter name (digest) but receives raw messages—both methods use SHA256withECDSA, which hashes internally, so the parameter should be named message to match the actual behavior. Additionally, the method name itself has a typo: verifyECDSAig should be verifyECDSASig. Since these are new SDK methods with no external callers yet (only test usage with raw message bytes), correct the naming and add Javadoc clarifying the message-based contract now to prevent API stability issues later.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java` around lines 122 -
139, Rename the method verifyECDSAig to verifyECDSASig and rename its parameter
from digest to message (since both computeECDSASig and verifyECDSASig call
Signature.getInstance("SHA256withECDSA") which hashes internally), update any
internal references/tests to use the new name/parameter, and add Javadoc to both
computeECDSASig(byte[] data, ECPrivateKey privateKey) and verifyECDSASig(byte[]
message, ECPublicKey publicKey) clarifying they accept the raw message bytes
(not a precomputed digest) and that the SHA-256 hashing is performed internally
by the signature algorithm.
X-Test Results✅ java-v0.15.0 |
X-Test Results✅ java-main |
X-Test Failure Report |
1 participant
Summary by CodeRabbit
Refactor
Breaking Changes