feat(sdk)!: remove bouncycastle and ayza libraries#367
Conversation
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Remove the three io.github.hakky54:ayza* dependencies and replace their TLS trust-material role with an SDK-owned TrustProvider built on provider-agnostic JCA APIs (CertificateFactory, KeyStore, TrustManagerFactory, SSLContext). This works under any registered crypto provider, including BC-FIPS, and avoids hardcoded provider names. - Add TrustProvider and package-private CompositeX509ExtendedTrustManager for combining JVM default + custom trust material. - SDKBuilder: replace SSLFactory field with SSLSocketFactory + X509TrustManager. sslFactory(SSLFactory) becomes sslFactory(SSLSocketFactory); add sslFactory(SSLSocketFactory, X509TrustManager) for callers that have a matching trust manager. sslFactoryFromDirectory / sslFactoryFromKeyStore signatures and semantics are preserved, now backed by TrustProvider internally. - TokenSource takes SSLSocketFactory directly. - Command.java --insecure path uses TrustProvider.insecure(). - SDKBuilderTest reworked to drop nl.altindag imports and use TrustProvider + standard JCA. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
📝 WalkthroughWalkthroughThis PR refactors the Java SDK to migrate from third-party cryptographic libraries (Ayza, explicit Bouncy Castle) to native JCA/JCE APIs, adds FIPS/non‑FIPS Maven profiles and CI steps, implements TrustProvider and CompositeX509ExtendedTrustManager, rewires SDK TLS to use SSLSocketFactory/X509TrustManager, migrates EC/AES helpers to JCA, and updates tests to use new utilities. ChangesCryptography library migration and TLS refactoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 docstrings
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Code Review
This pull request refactors the SDK's cryptographic and SSL handling to support FIPS compliance by transitioning from provider-specific libraries to standard Java Cryptography Architecture (JCA) APIs. Key changes include the removal of external dependencies like ayza and nl.altindag.ssl, the introduction of a TrustProvider for managing SSL contexts, and the addition of Maven profiles to manage FIPS-specific dependencies. A high-severity bug was identified in the TrustProvider class where the use of Set.of() with potentially duplicate keystore types would cause a runtime crash during initialization.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
sdk/src/main/java/io/opentdf/platform/sdk/TDF.java (1)
163-164: ⚡ Quick winDerive the split-key size from
AesGcminstead of keeping a second 32-byte contract.Line 163 now makes
TDFdepend onAesGcm.generateKey(), but this class still hardcodesGCM_KEY_SIZE = 32forpayloadKeysizing and XOR reconstruction. That duplication is easy to miss on future key-size changes and would silently desynchronize the two layers.Suggested fix
- private static final int GCM_KEY_SIZE = 32; + private static final int GCM_KEY_SIZE = AesGcm.GCM_KEY_SIZE_BITS / Byte.SIZE;🤖 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/TDF.java` around lines 163 - 164, The code duplicates the 32-byte key size: TDF uses a hardcoded GCM_KEY_SIZE = 32 while calling AesGcm.generateKey(), risking divergence; update TDF to derive the split-key size from AesGcm instead of the hardcoded constant (e.g., replace uses of GCM_KEY_SIZE for payloadKey sizing and XOR reconstruction with a value obtained from AesGcm such as AesGcm.KEY_SIZE if available, or by using AesGcm.generateKey().length/returnedKeyLength helper), and ensure all places that allocate payloadKey or perform XOR use that single derived size so key generation and reconstruction remain consistent (reference symbols: class TDF, GCM_KEY_SIZE, payloadKey, XOR reconstruction, and AesGcm.generateKey())..github/workflows/checks.yaml (1)
94-104: ⚡ Quick winIsolate the FIPS and non-FIPS Maven runs.
These two steps switch dependency profiles in the same workspace and both reuse the existing
target/trees. That makes the non-FIPSverifypartially depend on artifacts emitted by the earlier FIPS run, which can hide profile-specific regressions. Please addcleanto each profile-specific invocation or run them in separate jobs.🤖 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 @.github/workflows/checks.yaml around lines 94 - 104, The two workflow steps "Tests and enforcer (fips)" and "Tests with coverage and javadoc (non-fips)" reuse the same workspace and target/ artifacts, causing the non-FIPS mvn verify to depend on FIPS artifacts; update the Maven invocations in those steps (the lines invoking mvn --batch-mode test enforcer:enforce -P 'fips,!non-fips' -Dmaven.antrun.skip and the mvn --batch-mode verify ... -P 'coverage,non-fips,!fips') to include a clean phase (e.g., prepend clean so they become mvn --batch-mode clean test ... and mvn --batch-mode clean verify ...) or alternatively move each profile run into separate jobs to ensure isolation.
🤖 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 `@pom.xml`:
- Around line 20-22: The bc-fips.version property is pinned to 2.0.1 while
bcpkix-fips.version and bctls-fips.version are 2.1.x, which causes
incompatibility; update the bc-fips.version property to a 2.1.x release that
matches the PKIX/TLS modules (e.g., 2.1.23 or the same release series used by
bcpkix-fips/bctls-fips) and ensure the dependencyManagement entries that
reference bc-fips, bcpkix-fips, and bctls-fips all use the same 2.1.x release
set so all Bouncy Castle FIPS artifacts come from the same release.
In `@sdk/pom.xml`:
- Around line 493-517: The fips profile currently hides the bc-fips, bctls-fips,
and bcpkix-fips dependencies (profile id "fips") from downstream consumers; move
those three dependencies out of the inactive <profile id="fips"> into the main
<dependencies> section and mark each with <optional>true</optional> so SDK
consumers can opt in to FIPS provider jars (keep the same groupId/artifactId
values: org.bouncycastle:bc-fips, org.bouncycastle:bctls-fips,
org.bouncycastle:bcpkix-fips).
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 54-64: The method visibility for ECKeyPair.publicKeyFromPem was
reduced to package-private, which silently removes a public SDK entry point;
restore its visibility to public by changing the method signature back to public
(i.e., make publicKeyFromPem(...) public in class ECKeyPair) so external callers
keep working, or if removal is intentional, add explicit breaking-change
documentation and update the SDK changelog and API docs referencing
ECKeyPair.publicKeyFromPem to announce the removal.
In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java`:
- Around line 162-163: Replace the incorrect enum name usage passed to
PemTestUtils.publicKeyFromECPoint — it's calling SECP256R1.name() (which yields
"SECP256R1") but ECGenParameterSpec requires the JCA curve alias; change the
call to use SECP256R1.getCurveName() (the same form used earlier on line 67) so
the publicKeyFromECPoint invocation receives "secp256r1" instead of the
uppercase enum constant.
In `@sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java`:
- Around line 208-212: The server certificate built in SDKBuilderTest (the
HeldCertificate assigned to serverCertificate) only adds the canonical hostname
SAN; update the HeldCertificate.Builder usage in the serverCertificate creation
to include the literal string "localhost" as an additional subject alternative
name (i.e., add a SAN entry for the literal "localhost" in addition to the
existing localhost variable) so hostname verification succeeds in environments
where getCanonicalHostName() is not the literal "localhost".
---
Nitpick comments:
In @.github/workflows/checks.yaml:
- Around line 94-104: The two workflow steps "Tests and enforcer (fips)" and
"Tests with coverage and javadoc (non-fips)" reuse the same workspace and
target/ artifacts, causing the non-FIPS mvn verify to depend on FIPS artifacts;
update the Maven invocations in those steps (the lines invoking mvn --batch-mode
test enforcer:enforce -P 'fips,!non-fips' -Dmaven.antrun.skip and the mvn
--batch-mode verify ... -P 'coverage,non-fips,!fips') to include a clean phase
(e.g., prepend clean so they become mvn --batch-mode clean test ... and mvn
--batch-mode clean verify ...) or alternatively move each profile run into
separate jobs to ensure isolation.
In `@sdk/src/main/java/io/opentdf/platform/sdk/TDF.java`:
- Around line 163-164: The code duplicates the 32-byte key size: TDF uses a
hardcoded GCM_KEY_SIZE = 32 while calling AesGcm.generateKey(), risking
divergence; update TDF to derive the split-key size from AesGcm instead of the
hardcoded constant (e.g., replace uses of GCM_KEY_SIZE for payloadKey sizing and
XOR reconstruction with a value obtained from AesGcm such as AesGcm.KEY_SIZE if
available, or by using AesGcm.generateKey().length/returnedKeyLength helper),
and ensure all places that allocate payloadKey or perform XOR use that single
derived size so key generation and reconstruction remain consistent (reference
symbols: class TDF, GCM_KEY_SIZE, payloadKey, XOR reconstruction, and
AesGcm.generateKey()).
🪄 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: 1959f667-867f-4fe5-902c-1230a1063ad3
📒 Files selected for processing (17)
.github/workflows/checks.yamlcmdline/src/main/java/io/opentdf/platform/Command.javapom.xmlsdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/AesGcm.javasdk/src/main/java/io/opentdf/platform/sdk/CompositeX509ExtendedTrustManager.javasdk/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/SDKBuilder.javasdk/src/main/java/io/opentdf/platform/sdk/TDF.javasdk/src/main/java/io/opentdf/platform/sdk/TokenSource.javasdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javasdk/src/test/java/io/opentdf/platform/sdk/PemTestUtils.javasdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.javasdk/src/test/java/io/opentdf/platform/sdk/TDFTest.javasdk/src/test/resources/java.security.fips.test
| HeldCertificate serverCertificate = new HeldCertificate.Builder() | ||
| .addSubjectAlternativeName(localhost) | ||
| .commonName("CN=localhost") | ||
| .rsa2048() | ||
| .commonName("localhost") | ||
| .signedBy(rootCertificate) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "-- canonical hostname for localhost on this machine --"
python - <<'PY'
import socket
print(socket.getfqdn("localhost"))
PY
echo
echo "-- localhost endpoint literals and SAN inputs in SDKBuilderTest --"
rg -n -C1 '://localhost:|addSubjectAlternativeName|commonName\("localhost"\)' \
sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.javaRepository: opentdf/java-sdk
Length of output: 1544
🏁 Script executed:
# Get the context around the certificate building code and find the localhost variable definition
cat -n sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java | sed -n '195,225p'Repository: opentdf/java-sdk
Length of output: 1572
Include literal localhost in the certificate SANs.
These tests connect to https://localhost:..., but the certificate only includes the canonical hostname of "localhost" as a SAN. On systems where InetAddress.getByName("localhost").getCanonicalHostName() resolves to something other than the literal string "localhost" (e.g., certain Docker or CI environments), hostname verification will fail.
Suggested fix
HeldCertificate serverCertificate = new HeldCertificate.Builder()
+ .addSubjectAlternativeName("localhost")
.addSubjectAlternativeName(localhost)
.rsa2048()
.commonName("localhost")
.signedBy(rootCertificate)📝 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. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HeldCertificate serverCertificate = new HeldCertificate.Builder() | |
| .addSubjectAlternativeName(localhost) | |
| .commonName("CN=localhost") | |
| .rsa2048() | |
| .commonName("localhost") | |
| .signedBy(rootCertificate) | |
| HeldCertificate serverCertificate = new HeldCertificate.Builder() | |
| .addSubjectAlternativeName("localhost") | |
| .addSubjectAlternativeName(localhost) | |
| .rsa2048() | |
| .commonName("localhost") | |
| .signedBy(rootCertificate) |
🤖 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/SDKBuilderTest.java` around lines
208 - 212, The server certificate built in SDKBuilderTest (the HeldCertificate
assigned to serverCertificate) only adds the canonical hostname SAN; update the
HeldCertificate.Builder usage in the serverCertificate creation to include the
literal string "localhost" as an additional subject alternative name (i.e., add
a SAN entry for the literal "localhost" in addition to the existing localhost
variable) so hostname verification succeeds in environments where
getCanonicalHostName() is not the literal "localhost".
Dependency ReviewThe following issues were found:
Vulnerabilitiespom.xml
License Issuessdk/pom.xml
Denied Licenses: GPL-2.0, AGPL-1.0, AGPL-1.0-or-later, AGPL-1.0-only, AGPL-3.0, AGPL-3.0-only, AGPL-3.0-or-later, GPL-1.0, GPL-1.0+, GPL-1.0-only, GPL-1.0-or-later, CNRI-Python-GPL-Compatible, GPL-2.0+, GPL-2.0-only, GPL-2.0-or-later, GPL-2.0-with-GCC-exception, GPL-2.0-with-autoconf-exception, GPL-2.0-with-bison-exception, GPL-2.0-with-classpath-exception, GPL-2.0-with-font-exception, GPL-3.0, GPL-3.0+, GPL-3.0-only, GPL-3.0-or-later, GPL-3.0-with-GCC-exception, GPL-3.0-with-autoconf-exception, LGPL-2.0, LGPL-2.0+, LGPL-2.0-only, LGPL-2.0-or-later, LGPL-2.1, LGPL-2.1+, LGPL-2.1-only, LGPL-2.1-or-later, LGPL-3.0, LGPL-3.0+, LGPL-3.0-only, LGPL-3.0-or-later, LGPLLR, NGPL OpenSSF Scorecard
Scanned Files
|
X-Test Failure Report |
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
sdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.java (1)
95-99: ⚡ Quick winSpecify minimum TLS version instead of generic "TLS" string to prevent negotiation of deprecated protocols.
The generic
SSLContext.getInstance("TLS")(lines 96 and 211) permits negotiation of deprecated TLS 1.0/1.1. While modern JDKs disable these by default viajdk.tls.disabledAlgorithms, relying on JVM configuration is fragile. Since BC-FIPS supports"TLSv1.2"and"TLSv1.3"protocol strings, use one of these instead for stronger guarantees.🤖 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/TrustProvider.java` around lines 95 - 99, Replace the generic SSLContext.getInstance("TLS") usages with a specific, minimum protocol like "TLSv1.2" (or "TLSv1.3" if you require/guarantee support) to avoid negotiating deprecated TLS 1.0/1.1; update the call in TrustProvider.fromTrustManager (and the other SSLContext.getInstance("TLS") occurrence referenced in this review) to use SSLContext.getInstance("TLSv1.2") and ensure the rest of TrustProvider construction (sslContext.init(...), returned TrustProvider) remains unchanged.
🤖 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.
Nitpick comments:
In `@sdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.java`:
- Around line 95-99: Replace the generic SSLContext.getInstance("TLS") usages
with a specific, minimum protocol like "TLSv1.2" (or "TLSv1.3" if you
require/guarantee support) to avoid negotiating deprecated TLS 1.0/1.1; update
the call in TrustProvider.fromTrustManager (and the other
SSLContext.getInstance("TLS") occurrence referenced in this review) to use
SSLContext.getInstance("TLSv1.2") and ensure the rest of TrustProvider
construction (sslContext.init(...), returned TrustProvider) remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 348f7f1d-b626-4140-961a-d2cc299dfa3f
📒 Files selected for processing (5)
pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/AesGcm.javasdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.javasdk/src/main/java/io/opentdf/platform/sdk/TrustProvider.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- pom.xml
- sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java
- sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java
marythought
left a comment
There was a problem hiding this comment.
Overall — this is a real DX improvement, even with the breaking changes. From the consumer's seat:
- Dropping ayza is a long-term win. Removes a niche third-party type from the public surface;
SSLSocketFactory/X509TrustManagerare types every Java dev already knows. - The new
SDKBuildermethod set is well-shaped.sslFactoryFromTrustManager/sslFactoryFromDirectory/sslFactoryFromKeyStorecover the common cases; the bare/pairedsslFactory(SSLSocketFactory[, X509TrustManager])overloads handle the escape hatches; naming is consistent and the javadoc explicitly tells callers to prefer the paired form. Good guidance built into the API. - Sensible defaults —
fromDirectory/fromKeyStoreinclude JVM default cacerts automatically. The common "my certs plus system trust" case works without customers having to think about composition. - The breaking-change table in the PR description is exactly the format consumers need. Old → new in one place.
- ECKeyPair public-surface tightening is principled. The moved-to-test-only helpers were marked as "should not have been exposed" and would have broken under FIPS anyway. Better to fix now.
Three specific notes as inline comments below.
| X509TrustManager insecureTrustManager = new X509TrustManager() { | ||
| @Override public void checkClientTrusted(X509Certificate[] chain, String authType) {} | ||
| @Override public void checkServerTrusted(X509Certificate[] chain, String authType) {} | ||
| @Override public X509Certificate[] getAcceptedIssuers() { return new X509Certificate[0]; } | ||
| }; | ||
| builder.sslFactoryFromTrustManager(insecureTrustManager); |
There was a problem hiding this comment.
The insecure-trust path is now ~5 lines of boilerplate every consumer who wants local-dev "trust everything" will rewrite. The pre-ayza one-liner was SSLFactory.builder().withUnsafeTrustMaterial().build(). Worth a no-arg SDKBuilder.useInsecureSkipVerify() (or similar) that wraps this same anonymous trust-manager internally? useInsecurePlaintextConnection is already alongside it as precedent for "we provide the unsafe option explicitly because consumers will ask for it anyway."
| import java.util.Objects; | ||
| // https://www.bouncycastle.org/latest_releases.html | ||
|
|
||
| public class ECKeyPair { |
There was a problem hiding this comment.
The PR description's breaking-change table lists what's removed from ECKeyPair's public API, but doesn't say what consumers should use instead. For each removed static helper, a one-line JCA recipe in the table would save callers a JCA-docs hunt — e.g., publicKeyFromPem(pem) → KeyFactory.getInstance("EC").generatePublic(new X509EncodedKeySpec(...)), or getPEMPublicKeyFromX509Cert(...) → CertificateFactory.getInstance("X.509").generateCertificate(...).getPublicKey(). If any of the removed helpers have no public replacement (genuinely test-only), saying so explicitly avoids a search that ends in disappointment.
| * work is fulfilled by whichever {@link java.security.Provider} is registered with the JVM, | ||
| * including FIPS-mode providers. | ||
| */ | ||
| final class TrustProvider { |
There was a problem hiding this comment.
Load-bearing architectural shift here — provider-agnostic JCA + an internal trust composition abstraction, driven by FIPS-readiness and removing the BC namespace conflict with ayza. The rationale is solid but lives only in the PR description today. Worth recording in a repo ADR (e.g., adr/000X-provider-agnostic-cryptography.md) so future maintainers don't have to dig through PR history to understand why BC stopped being a compile dep and why TrustProvider exists. Could also capture the trade-offs that aren't visible in code — e.g., why TrustProvider is package-private today, and what the FIPS-readiness gaps are (the "Remaining FIPS work" list in the PR body).



This pull makes this library provider-agnostic. Users can control how cryptography is provided by configuring
their
java.securityconfiguration, as we do in the FIPS-profile tests. We add two new maven profilesfipsandnon-fips. These profiles are used solely for testing; the library itself is agnostic to which providers are usedfor cryptography.
In the
non-fipsprofile we use the normal providers configured on the JVM. The BouncyCastle provider is includedjust for some test verification.
In the
fipsprofile we include the BouncyCastle FIPS providers we need at runtime to verify that our library works with a FIPS-compliant setup.Ayza removal
I believe it's necessary to remove ayza since it loads a BouncyCastleProvider explicitly. Since the FIPS provider
occupies the same namespace loading both providers can't work.
BouncyCastle moved to test-only
Removing BouncyCastle as a compile dependency isn't strictly necessary but it results in much less rework and
eliminates a transitive dependency for consumers.
Remaining FIPS work
We still need to:
library can be made FIPS compliant
FIPS-compliant source of entropy we need to include the SUN provider to get at
/dev/rand. This is OK, sinceThe breaking API changes are as follows:
SDKBuilder.sslFactory(SSLFactory)sslFactory(SSLSocketFactory)andsslFactory(SSLSocketFactory, X509TrustManager)SDKBuilder.sslFactoryFromTrustManager(X509TrustManager)ECKeyPair(ECCurve, ECAlgorithm)constructorECKeyPair(ECCurve)ECKeyPair.publicKeyFromPemECKeyPair.privateKeyFromPem,getPEMPublicKeyFromX509Cert,publicKeyFromECPoint, staticcompressECPublickey(String)PemTestUtils)io.github.hakky54:ayza*and the runtime BC dep; addsbc-fips,bcpkix-fips,bctls-fips(via newfips/non-fipsMaven profiles)Summary by CodeRabbit
New Features
Improvements
Chores