feat(sdk)!: remove the hardcoded BouncyCastle provider#364
feat(sdk)!: remove the hardcoded BouncyCastle provider#364
BouncyCastle provider#364Conversation
Merges main (v0.15.0) into the fips branch.
Conflict resolution:
- ECKeyPair.java: Combined main's structure (ECCurve enum, KeyPair field,
new methods like getPEMPublicKeyFromX509Cert) with fips's FIPS-compatible
crypto changes (no explicit BouncyCastle provider, standard Java interfaces
for ECPublicKey/ECPrivateKey, KeyFactory.getInstance("EC")).
- ECKeyPairTest.java: Kept fips's removal of extractPemPubKeyFromX509 test
(which used BC-specific .getQ()), incorporated main's new
createSymmetricKeysWithOtherCurves test, and enabled publicKeyFromECPoint
assertion using ECCurve.getCurveName().
- NanoTDF.java: Accepted main's deletion since all NanoTDF dependencies
(ECCMode, NanoTDFType, etc.) were also removed in main.
Signed-off-by: GitHub <noreply@github.com>
Co-authored-by: mkleene <262667+mkleene@users.noreply.github.com>
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR migrates OpenTDF SDK cryptographic operations from explicit BouncyCastle provider qualification to provider-agnostic JCA/JCE lookups. BouncyCastle provider registration is centralized in the CLI command and a JUnit test extension, while all key generation, ECDH, ECDSA, and PEM conversion operations now use unqualified JCA APIs. ChangesProvider-Agnostic Cryptography Migration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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 |
mkleene
force-pushed
the
remove-hardcoded-provider
branch
from
fb346eb to
df4682d
Compare
X-Test Failure Report |
![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 refactors the platform's security handling by removing external SSL utility dependencies and introducing a custom TrustProvider based on standard JCA APIs. ECKeyPair is updated to use standard Java security interfaces, reducing direct reliance on Bouncy Castle provider-specific classes. Key feedback includes a critical missing class (CompositeX509ExtendedTrustManager) that will cause compilation errors, the use of internal Bouncy Castle SPI classes instead of standard KeyPairGenerator APIs, and a deprecated OkHttp method usage in SDKBuilder.
I am having trouble creating individual review comments. Click here to see my feedback.
sdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.java (245)
The class CompositeX509ExtendedTrustManager is referenced here but does not appear to be defined in this file or provided in the PR. This will cause a compilation error. Please ensure all necessary utility classes are included in the changes.
sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java (12)
This import of an internal Bouncy Castle SPI class should be removed. The goal of this PR is to move towards provider-agnostic JCA APIs. Using implementation classes directly defeats this purpose and can lead to runtime issues if the provider is not registered or if internal APIs change.
sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java (50-54)
Instead of using the internal Bouncy Castle SPI class and provider-specific algorithm names like "ECDH" or "ECDSA" for key pair generation, use the standard JCA algorithm name "EC" with the KeyPairGenerator API. This ensures better compatibility across different security providers.
generator = KeyPairGenerator.getInstance("EC");sdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.java (130)
The iteration over keystore types contains potential duplicates. KeyStore.getDefaultType() is often "PKCS12" on modern JVMs, leading to redundant attempts. Consider using a LinkedHashSet to maintain order while ensuring uniqueness.
for (String type : new java.util.LinkedHashSet<>(java.util.Arrays.asList(KeyStore.getDefaultType(), "JKS", "PKCS12"))) {
sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java (413)
Calling sslSocketFactory(SSLSocketFactory) without an accompanying X509TrustManager is deprecated in OkHttp 4.x. It relies on reflection to find a trust manager, which is fragile on modern JDKs. Since TrustProvider can provide a trust manager, it should be used here if available, or a default one should be explicitly provided.
X-Test Failure Report |
X-Test Results |
mkleene
changed the title
BouncyCastle provider
mkleene
changed the title
BouncyCastle providerBouncyCastle provider
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/main/java/io/opentdf/platform/sdk/ECKeyPair.java (1)
46-56:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
KeyPairGenerator.getInstance("ECDH"/"ECDSA")are BouncyCastle-specific aliases; use standard JCA algorithm"EC"insteadThe standard JCA specification defines
KeyPairGeneratorandKeyFactoryfor algorithm"EC", not"ECDH"or"ECDSA". Using"ECDH"or"ECDSA"as aKeyPairGeneratoralgorithm name is a BouncyCastle-specific alias that will throwNoSuchAlgorithmExceptionwhenever the security provider is not explicitly registered (viaSecurity.addProvider()).The SDK instantiates
ECKeyPairfrom core API classes (KASClient.java:124,TDF.java:246), not exclusively from the CLI. SDK consumers using this library without the CLI bootstrap will encounter runtime failures. Additionally,CryptoUtils.java:49in the same codebase already demonstrates the correct pattern:KeyPairGenerator.getInstance("EC"). TheECGenParameterSpecon line 57 already encodes the target curve, making the algorithm distinction ingetInstanceredundant.🐛 Proposed fix
- if (algorithm == ECAlgorithm.ECDH) { - generator = KeyPairGenerator.getInstance(ECAlgorithm.ECDH.name()); - } else { - generator = KeyPairGenerator.getInstance(ECAlgorithm.ECDSA.name()); - } + generator = KeyPairGenerator.getInstance("EC");🤖 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 46 - 56, In ECKeyPair, the conditional KeyPairGenerator.getInstance(ECAlgorithm.ECDH.name()/ECDSA.name()) is using BouncyCastle-specific aliases and can fail; change it to always call KeyPairGenerator.getInstance("EC") (remove the algorithm==ECAlgorithm.ECDH check) and continue to set the curve via ECGenParameterSpec as already done; update any related exception handling in the ECKeyPair constructor so a NoSuchAlgorithmException still wraps/propagates as before.
🧹 Nitpick comments (2)
sdk/src/test/java/io/opentdf/platform/sdk/CryptoProviderSetupExtension.java (1)
10-16: ⚡ Quick winInstance-level
synchronized+ null-check does not guarantee global once-only BC registrationJUnit 5's service-loader auto-detection can create a fresh
CryptoProviderSetupExtensioninstance per test class. Each new instance starts withsecurityProvider == null, soSecurity.addProvider()is invoked once per test class, not once per JVM. Thesynchronizedkeyword guards concurrent access to this particular instance, which provides no protection across separate instances.This is functionally harmless because
Security.addProvider()is idempotent (it returns-1when the provider is already registered and does not throw), but the guard logic does not achieve its apparent intent.If a true once-per-JVM guarantee is desired, use a static field or the
ExtensionContextroot store:♻️ Option A – static flag (simplest)
public class CryptoProviderSetupExtension implements BeforeAllCallback { - private BouncyCastleProvider securityProvider; + private static final AtomicBoolean registered = new AtomicBoolean(false); `@Override` - public synchronized void beforeAll(ExtensionContext extensionContext) throws Exception { - if (this.securityProvider == null) { - Security.addProvider(this.securityProvider = new BouncyCastleProvider()); - } + public void beforeAll(ExtensionContext extensionContext) { + if (registered.compareAndSet(false, true)) { + Security.addProvider(new BouncyCastleProvider()); + } } }🤖 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/CryptoProviderSetupExtension.java` around lines 10 - 16, The beforeAll method in CryptoProviderSetupExtension uses an instance-level synchronized and null-check on the instance field securityProvider, which does not guarantee a once-per-JVM BouncyCastle registration because JUnit may create multiple extension instances; change the guard to a JVM-wide one by making the provider flag static (e.g., a static volatile BouncyCastleProvider or static boolean registered) or store/check a marker in the ExtensionContext root store, then call Security.addProvider(...) only when that static/root-store marker indicates it has not yet been registered; update references in beforeAll and the securityProvider field accordingly.sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java (1)
56-56: 💤 Low valueRemove debug
System.out.printlnstatements from tests.Lines 56, 57, 70, and 71 print key material to stdout. This adds noise to test output and leaks key bytes to logs; none of these prints are assertions.
🧹 Proposed cleanup
- System.out.println(keypairAPublicKey); System.out.println(keypairAPrivateKey);🤖 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` at line 56, Remove the debug System.out.println calls that print key material in the ECKeyPairTest; specifically delete the printlns referencing variables keypairAPublicKey, keypairAPrivateKey, keypairBPublicKey, and keypairBPrivateKey in the ECKeyPairTest class, or replace them with meaningful assertions if you intended to verify values—do not print key bytes to stdout.
🤖 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/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 46-56: In ECKeyPair, the conditional
KeyPairGenerator.getInstance(ECAlgorithm.ECDH.name()/ECDSA.name()) is using
BouncyCastle-specific aliases and can fail; change it to always call
KeyPairGenerator.getInstance("EC") (remove the algorithm==ECAlgorithm.ECDH
check) and continue to set the curve via ECGenParameterSpec as already done;
update any related exception handling in the ECKeyPair constructor so a
NoSuchAlgorithmException still wraps/propagates as before.
---
Nitpick comments:
In `@sdk/src/test/java/io/opentdf/platform/sdk/CryptoProviderSetupExtension.java`:
- Around line 10-16: The beforeAll method in CryptoProviderSetupExtension uses
an instance-level synchronized and null-check on the instance field
securityProvider, which does not guarantee a once-per-JVM BouncyCastle
registration because JUnit may create multiple extension instances; change the
guard to a JVM-wide one by making the provider flag static (e.g., a static
volatile BouncyCastleProvider or static boolean registered) or store/check a
marker in the ExtensionContext root store, then call Security.addProvider(...)
only when that static/root-store marker indicates it has not yet been
registered; update references in beforeAll and the securityProvider field
accordingly.
In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java`:
- Line 56: Remove the debug System.out.println calls that print key material in
the ECKeyPairTest; specifically delete the printlns referencing variables
keypairAPublicKey, keypairAPrivateKey, keypairBPublicKey, and keypairBPrivateKey
in the ECKeyPairTest class, or replace them with meaningful assertions if you
intended to verify values—do not print key bytes to stdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f7af437f-f1ea-4293-8a22-70b1ab4dc371
📒 Files selected for processing (7)
cmdline/src/main/java/io/opentdf/platform/Command.javasdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/test/java/io/opentdf/platform/sdk/CryptoProviderSetupExtension.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javasdk/src/test/resources/META-INF/services/org.junit.jupiter.api.extension.Extensionsdk/src/test/resources/junit-platform.properties
|
X-Test Results |
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
DX / docs review
Thanks for taking this on — letting consumers bring their own provider is the right call for FIPS. A few things I'd like to see addressed before merge, mostly around making the breaking change discoverable for downstream users.
1. PR description needs a migration note
Right now the body is just CodeRabbit's auto-summary. Because Release Please auto-generates the CHANGELOG from the PR title and links back to this PR, the description is the migration doc downstream consumers will land on when they hit the ⚠ BREAKING CHANGES entry in release notes. It needs a hand-written "What breaks / How to migrate" section. Suggested structure:
- What's changing: SDK no longer auto-registers
BouncyCastleProvider. - Why: FIPS compatibility — consumers can now plug in their own provider.
- Action required: Register BC (or your preferred provider) in your application's bootstrap, e.g.:
See
static { Security.addProvider(new BouncyCastleProvider()); }
cmdline/src/main/java/io/opentdf/platform/Command.javafor the reference pattern.
2. There are public API removals beyond provider registration
A few changes go further than the title suggests, and I think they deserve explicit callouts in the migration note:
ECKeyPair.getPEMPublicKeyFromX509Cert(String)is deleted entirely. Is this intentional? If anything external depends on it, we should either restore it (using the new BC-classes-without-provider pattern) or document the removal + replacement.publicKeyFromPem/privateKeyFromPemnow returnjava.security.interfaces.ECPublicKey/ECPrivateKeyinstead of the BC interfaces. Any caller invoking BC-specific methods like.getQ()will fail to compile — the deletedextractPemPubKeyFromX509test is the canonical example of this pattern.compressECPublickey(String)no longer throwsNoSuchProviderException. Minor, but a checked-exception signature change.
3. README "Cryptography Library" section needs updating
The current wording hedges:
Note: When using this SDK, it may be necessary to register the Bouncy Castle Provider as follows...
That was accurate when the SDK auto-registered. After this PR it's misleading. I'd update it to:
- State clearly that the SDK no longer auto-registers a provider.
- Spell out when registration is required vs. optional, and what the JDK default supports.
- Mention the FIPS motivation so FIPS users know they can plug in their own provider.
- Point at
Command.javaas a reference pattern.
I'd also verify the Quick Start example in the README still runs end-to-end on a stock JDK after this change. If it doesn't, the example needs the registration block added so first-time users aren't met with a runtime failure.
4. opentdf.io docs follow-up
Not in scope for this PR, but I'll audit these for samples that import org.bouncycastle.jce.interfaces.ECPublicKey or call getPEMPublicKeyFromX509Cert, and update them once this lands:
- https://opentdf.io/sdks/tdf (Java tab)
- https://opentdf.io/sdks/policy (Java tab)
- https://opentdf.io/quickstart (Java sections)
- https://opentdf.io/llms.txt
What I liked
The test infrastructure (CryptoProviderSetupExtension + META-INF/services + junit.jupiter.extensions.autodetection.enabled) is a clean way to keep BC available for tests without coupling the SDK to it. Nice contributor DX.
I'm not sure what the right pattern is for the provider. It seems like telling them they can bring their own provider is wrong because we use bouncycastle internally, e.g. this. We need a static binding to bouncycastle but at that point we should just load the provider for them, I think, since if we are statically bound to one provider and they pass us the other then we are in trouble. Maybe it's better to just depend on the bouncycastle classes explicitly and not go through the provider interfaces? I wonder if the right thing to do is just list bouncycastle as Ideally there would be a way to parse the PEM stuff in a way that is unrelated to FIPS. We can't use https://openjdk.org/jeps/524 until java 26, which kind of seems too much to me. I also think I made a mistake in opening this without removing the |
Summary by CodeRabbit
Refactor
Tests