feat(sdk): replace ayza libraries with TrustProvider on JCA#366
feat(sdk): replace ayza libraries with TrustProvider on JCA#366mkleene wants to merge 17 commits into
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>
📝 WalkthroughWalkthroughThis PR migrates TLS and cryptography infrastructure from the external ChangesSSL/TLS Provider Migration to JCA
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
![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 replaces the external SSLFactory dependency with a native JCA-based implementation using the new TrustProvider and CompositeX509ExtendedTrustManager classes. The SDKBuilder, TokenSource, and associated tests have been updated to work with standard SSLSocketFactory and X509TrustManager objects. Reviewers suggested using InputStream.readAllBytes() for better performance, deduplicating keystore types during loading, and ensuring that X509TrustManager instances are provided alongside socket factories to avoid deprecated OkHttp methods and potential validation issues.
| private static byte[] readAll(InputStream in) throws IOException { | ||
| java.io.ByteArrayOutputStream out = new java.io.ByteArrayOutputStream(); | ||
| byte[] buf = new byte[8192]; | ||
| int n; | ||
| while ((n = in.read(buf)) >= 0) { | ||
| out.write(buf, 0, n); | ||
| } | ||
| return out.toByteArray(); | ||
| } |
There was a problem hiding this comment.
The readAll method is redundant as InputStream.readAllBytes() is available since Java 9. Since the project targets Java 11, it's better to use the built-in method for better performance and readability.
private static byte[] readAll(InputStream in) throws IOException {
return in.readAllBytes();
}| // registered fulfills the request. | ||
| byte[] bytes = readAll(in); | ||
| KeyStoreException last = null; | ||
| for (String type : new String[]{KeyStore.getDefaultType(), "JKS", "PKCS12"}) { |
There was a problem hiding this comment.
The loop iterates over a fixed array that likely contains duplicate values (e.g., KeyStore.getDefaultType() is often "PKCS12" on modern JVMs). This leads to redundant attempts to load the keystore if the first attempt fails.
| for (String type : new String[]{KeyStore.getDefaultType(), "JKS", "PKCS12"}) { | |
| for (String type : new java.util.LinkedHashSet<>(java.util.Arrays.asList(KeyStore.getDefaultType(), "JKS", "PKCS12"))) { |
| // Caller supplied an SSLSocketFactory without a matching trust manager (e.g. via | ||
| // sslFactory(SSLSocketFactory)). Falls back to OkHttp's reflection-based platform | ||
| // default trust manager — only the SSLSocketFactory governs the actual handshake. | ||
| httpClient.sslSocketFactory(sslSocketFactory); | ||
| } |
There was a problem hiding this comment.
The httpClient.sslSocketFactory(SSLSocketFactory) method is deprecated in OkHttp 4.x. It relies on reflection to find a matching X509TrustManager, which can fail on modern JVMs with strict encapsulation or on Android. Since the SDK now provides a TrustProvider that returns both the socket factory and the trust manager, it's highly recommended to always provide both to OkHttp. For the sslFactory(SSLSocketFactory) overload, consider documenting that it may fail on some platforms or require a matching trust manager.
| * @param sslSocketFactory Optional SSLSocketFactory for token endpoint requests | ||
| */ | ||
| public TokenSource(ClientAuthentication clientAuth, RSAKey rsaKey, URI tokenEndpointURI, AuthorizationGrant authzGrant, SSLFactory sslFactory) { | ||
| public TokenSource(ClientAuthentication clientAuth, RSAKey rsaKey, URI tokenEndpointURI, AuthorizationGrant authzGrant, SSLSocketFactory sslSocketFactory) { |
There was a problem hiding this comment.
The TokenSource constructor now takes an SSLSocketFactory but not the matching X509TrustManager. Similar to the OkHttp client in SDKBuilder, the underlying HTTP client used by the Nimbus OAuth2 SDK may require the trust manager for proper certificate validation and platform compatibility. Consider passing the X509TrustManager to TokenSource as well.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| try { | ||
| SSLContext ctx = SSLContext.getInstance("TLS"); | ||
| X509ExtendedTrustManager trustAll = new InsecureTrustManager(); | ||
| ctx.init(new KeyManager[0], new TrustManager[]{trustAll}, new SecureRandom()); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java (3)
86-102: 💤 Low valueConsider not silently clearing
trustManagerin the single-arg overload.
sslFactory(SSLSocketFactory)resetstrustManagertonull. If a caller first invokessslFactoryFromDirectory(...)(which sets both fields) and later overrides only the socket factory, the previously paired trust manager is silently dropped, causing OkHttp to fall back to its deprecated reflection-based default trust manager (Line 411-413 comment). This is currently documented in Javadoc, but a paired clear is easy to overlook. Either keep the field unchanged or assert/log when discarding a previously set trust manager.🤖 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/SDKBuilder.java` around lines 86 - 102, The single-arg SDKBuilder.sslFactory(SSLSocketFactory) currently clears the existing trustManager (this.trustManager = null), which can silently drop a previously paired trust manager set by sslFactoryFromDirectory(...) or the two-arg sslFactory(..., X509TrustManager); change this behavior to either leave this.trustManager unchanged when a new SSLSocketFactory is supplied or add an explicit safeguard: if a non-null trustManager is already present, log/warn or throw an IllegalStateException before clearing it so callers are made aware; update SDKBuilder.sslFactory(SSLSocketFactory) to implement one of these two options and adjust Javadoc accordingly.
419-425: ⚡ Quick winMethod name
getSslFactory()is misleading.It now returns
SSLSocketFactory, not anSSLFactory. Consider renaming togetSslSocketFactory()for consistency with the new types and with the package-private getter naming used byTrustProvider. Both getters are package-private, so the rename is non-breaking outside the package.♻️ Suggested rename
- SSLSocketFactory getSslFactory() { + SSLSocketFactory getSslSocketFactory() { return this.sslSocketFactory; }(Update internal callers in this package accordingly.)
🤖 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/SDKBuilder.java` around lines 419 - 425, Rename the misleading package-private getter getSslFactory() to getSslSocketFactory() to reflect that it returns an SSLSocketFactory; update all internal callers in the package (e.g., places that reference SDKBuilder.getSslFactory()) to use getSslSocketFactory(); keep the existing getTrustManager() name consistent with TrustProvider usage.
396-417: 💤 Low valueNarrow the deprecation suppression scope.
Only the single-argument
httpClient.sslSocketFactory(sslSocketFactory)call at line 413 is deprecated in OkHttp 4.12.0. The method-level@SuppressWarnings("deprecation")suppresses warnings for the entire method unnecessarily. Move the annotation to a local block or a separate helper method to keep the warning surface meaningful.🤖 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/SDKBuilder.java` around lines 396 - 417, The method getHttpClient currently has a broad `@SuppressWarnings`("deprecation") that hides all deprecation warnings; narrow it by removing the method-level annotation and apply `@SuppressWarnings`("deprecation") only around the single deprecated call httpClient.sslSocketFactory(sslSocketFactory) (e.g., enclose that branch in a small local block with the annotation) or extract that fallback into a small helper method (e.g., sslSocketFactoryFallback(SSLSocketFactory)) annotated with the suppression; keep the existing logic using trustManager, sslSocketFactory, usePlainText and httpClient as-is.cmdline/src/main/java/io/opentdf/platform/Command.java (1)
264-267: ⚡ Quick winUse the two-arg
sslFactoryoverload for--insecureto keep behavior consistent.The single-arg overload at line 266 calls
builder.sslFactory(TrustProvider.insecure().getSslSocketFactory()), which sets theSDKBuilder's trust manager tonull. This causesSDKBuilder.getHttpClient()to fall back to OkHttp's deprecated reflection-based default trust manager (per the Javadoc at SDKBuilder lines 411–413). SinceTrustProvider.insecure()also exposesgetTrustManager(), pass both the socket factory and trust manager to keep cert handling consistent and avoid the deprecated fallback path.♻️ Suggested change
- // Trust all certificates - builder.sslFactory(TrustProvider.insecure().getSslSocketFactory()); + // Trust all certificates + TrustProvider insecure = TrustProvider.insecure(); + builder.sslFactory(insecure.getSslSocketFactory(), insecure.getTrustManager());🤖 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 `@cmdline/src/main/java/io/opentdf/platform/Command.java` around lines 264 - 267, The current call builder.sslFactory(TrustProvider.insecure().getSslSocketFactory()) uses the single-arg overload which leaves the trust manager null and triggers SDKBuilder.getHttpClient() to fall back to the deprecated default trust manager; update the code to call the two-arg overload and pass both the SSLSocketFactory and its TrustManager from TrustProvider.insecure() (or store TrustProvider.insecure() in a local variable and pass its getSslSocketFactory() and getTrustManager()), so the builder.sslFactory(...) invocation supplies both socket factory and trust manager and avoids the deprecated fallback.sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java (1)
396-399: ⚡ Quick winRemove the swallowing
catch (Exception e)block.
sdkServicesSetupalready declaresthrows Exception, so any thrown exception should propagate with its original stack trace. Wrapping it incatch (Exception e) { assertThat(e).isNull(); }replaces the real failure (and its line/stack) with a generic "expected null but was …" assertion error, which makes debugging considerably harder. Note that AssertJ failures inside thetryareAssertionErrors (subclass ofError, notException), so they already bypass this catch — the only thing this block changes is the diagnostic quality on real exceptions.♻️ Proposed change
assertThat(kasDPoPHeader.get()).isNotNull(); assertThat(kasAuthHeader.get()).isEqualTo("DPoP hereisthetoken"); - } catch (Exception e) { - assertThat(e).isNull(); - } - finally { + } finally { if (platformServicesServer != null) { platformServicesServer.shutdownNow(); }🤖 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 396 - 399, Remove the swallowing catch block that catches Exception in the test around sdkServicesSetup; since sdkServicesSetup declares throws Exception, let exceptions propagate instead of catching them with "catch (Exception e) { assertThat(e).isNull(); }"—delete that catch clause so any thrown Exception surfaces with its original stack trace (leave the try/finally or convert to a plain method body as appropriate) and retain any finally cleanup code.sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java (2)
100-106: 💤 Low valueDrop the duplicate compression call.
compressedKeyis computed twice with the samepubKeyargument on lines 101 and 104. The first value is only printed before being immediately overwritten with an identical result — looks like leftover refactor scaffolding.♻️ Proposed cleanup
ECPublicKey publicKey = PemTestUtils.publicKeyFromPem(pubKey); byte[] compressedKey = PemTestUtils.compressECPublickey(pubKey); System.out.println(Arrays.toString(compressedKey)); - - compressedKey = PemTestUtils.compressECPublickey(pubKey); - System.out.println(Arrays.toString(compressedKey)); System.out.println(compressedKey.length);🤖 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 100 - 106, Remove the duplicate compression call and redundant prints: only call PemTestUtils.compressECPublickey(pubKey) once, assign it to compressedKey, and print the result and length once; adjust the ECPublicKey publicKey = PemTestUtils.publicKeyFromPem(pubKey) section to use the single compressedKey and remove the extra System.out.println(Arrays.toString(compressedKey)) and duplicate assignment to compressedKey to avoid overwriting the value.
167-180: 💤 Low valueConsider asserting at least one curve was tested.
While the
isSupported()flag is hardcoded per enum value (3 out of 4 curves are supported), adding an assertion that the loop body executed at least once would provide defensive coverage against accidental enum changes that might disable all curves.🤖 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 167 - 180, The test testECDSA currently loops ECCurve.values() and skips unsupported curves via ECCurve.isSupported() but doesn't assert that any curve was actually exercised; add a counter or boolean flag inside ECKeyPairTest.testECDSA to track that at least one supported curve ran (increment when entering the loop body or on successful verify) and at the end assertThat(counter > 0) or assertTrue(flag) so the test fails if no curves are supported, referencing ECCurve.isSupported(), ECKeyPair, computeECDSASig and verifyECDSAig to locate the loop to modify.sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java (1)
171-171: 💤 Low valueConsider introducing a dedicated HMAC key generator to avoid semantic coupling with AES.
Line 171 (and also 633, 665) uses
AesGcm.generateKey()to produce raw key bytes for HS256 (HMAC-SHA256) viaAssertionConfig.AssertionKey(). While the current implementation returns 256 bits (32 bytes)—sufficient for HS256—the practice couples HMAC test fixtures to an AES-specific helper, creating maintainability confusion. A dedicated HMAC key generation method (or a clearly documented contract specifying minimum key length for shared use) would improve clarity without requiring changes to the current tests.🤖 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/TDFTest.java` at line 171, Replace use of AesGcm.generateKey() for HMAC test keys with a dedicated HMAC key generator to avoid semantic coupling: add or use a method like HmacKeyGenerator.generateKey() (or AssertionConfig.createHmacKey()) that returns 256-bit raw bytes and update tests calling AesGcm.generateKey() (e.g., in TDFTest where AssertionConfig.AssertionKey(...) is invoked) to call the new HMAC generator instead; ensure the new generator documents the required minimum length for HS256 and returns the same byte length as the current AES helper so test behavior remains unchanged.sdk/src/test/java.security.test (1)
5-8: ⚡ Quick winThe default non-FIPS lane is still pinned to Bouncy Castle providers.
Lines 6-7 mean the default test run exercises
TrustProviderandECKeyPairagainst BC/BCJSSE, not the plain Java 11 provider stack. If the goal of this PR is provider-agnostic JCA compatibility, add a stock-JDK provider file or a dedicated CI lane for it.🤖 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.security.test` around lines 5 - 8, The default test config is pinned to BouncyCastle via the security.provider entries, so update the test provider setup to exercise the stock JDK provider stack (or add a dedicated CI lane) instead of BC: remove or disable the lines registering org.bouncycastle.jce.provider.BouncyCastleProvider and org.bouncycastle.jsse.provider.BouncyCastleJsseProvider in the test provider file and replace them with a stock-JDK provider file (or CI job) so TrustProvider and ECKeyPair are validated against the plain Java 11 providers rather than BC/BCJSSE.
🤖 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/pom.xml`:
- Around line 467-472: The maven-surefire-plugin configuration is overwriting
the JaCoCo-injected argLine, preventing coverage; update the surefire <argLine>
in the plugin to preserve and append to the existing value (i.e., include the
existing ${argLine} placeholder and then add the
-Djava.security.properties=${test.java.security.file} piece) so JaCoCo's
prepare-agent can inject its agent args while still setting the security
properties.
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 54-58: The method publicKeyFromPem currently calls
Base64.getDecoder().decode(pemData) which may throw an unchecked
IllegalArgumentException for malformed input; update publicKeyFromPem to catch
IllegalArgumentException around the Base64 decoding and rethrow it as a checked
InvalidKeySpecException (preserving the original exception as the cause) so the
method's declared exceptions cover malformed PEM input and callers receive a
consistent checked error; locate the Base64 decoding call in publicKeyFromPem
and wrap it with a try/catch that converts IllegalArgumentException into new
InvalidKeySpecException(..., e).
In `@sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java`:
- Around line 79-102: The public overload
SDKBuilder.sslFactory(nl.altindag.ssl.SSLFactory) was removed which is a
breaking API change; update the release artifacts to reflect this by documenting
the removal in the CHANGELOG/release notes, add a clear migration note in the
SDKBuilder Javadoc near the remaining sslFactory(SSLSocketFactory) and
sslFactory(SSLSocketFactory, X509TrustManager) methods that explains the old
overload was removed and how to migrate, and bump the major version per semver
to signal the breaking change.
- Around line 124-135: The sslFactoryFromKeyStore method currently replaces a
null keystorePassword with "".toCharArray(), which forces KeyStore.load() to
treat it as an empty password and breaks compatibility; update
SDKBuilder.sslFactoryFromKeyStore to pass the original keystorePassword through
to TrustProvider.fromKeyStore (i.e., pass null when null) so
TrustProvider.fromKeyStore (and ultimately KeyStore.load()) can handle null
correctly; locate and change the ternary that builds the char[] password and
ensure no other code in sslFactoryFromKeyStore converts null to an empty char[].
In `@sdk/src/test/java.security.fips.test`:
- Around line 4-8: Change the securerandom.strongAlgorithms entry to use
BCFIPS's DRBG instead of SUN so SecureRandom.getInstanceStrong() and
FIPS-sensitive paths (e.g., AesGcm.encrypt()) use the BCFIPS RNG; update the
property securerandom.strongAlgorithms from "DRBG:SUN" to "DRBG:BCFIPS"
(ensuring the BouncyCastle FIPS provider entries remain present) so the FIPS
profile exercises the BCFIPS DRBG implementation.
---
Nitpick comments:
In `@cmdline/src/main/java/io/opentdf/platform/Command.java`:
- Around line 264-267: The current call
builder.sslFactory(TrustProvider.insecure().getSslSocketFactory()) uses the
single-arg overload which leaves the trust manager null and triggers
SDKBuilder.getHttpClient() to fall back to the deprecated default trust manager;
update the code to call the two-arg overload and pass both the SSLSocketFactory
and its TrustManager from TrustProvider.insecure() (or store
TrustProvider.insecure() in a local variable and pass its getSslSocketFactory()
and getTrustManager()), so the builder.sslFactory(...) invocation supplies both
socket factory and trust manager and avoids the deprecated fallback.
In `@sdk/src/main/java/io/opentdf/platform/sdk/SDKBuilder.java`:
- Around line 86-102: The single-arg SDKBuilder.sslFactory(SSLSocketFactory)
currently clears the existing trustManager (this.trustManager = null), which can
silently drop a previously paired trust manager set by
sslFactoryFromDirectory(...) or the two-arg sslFactory(..., X509TrustManager);
change this behavior to either leave this.trustManager unchanged when a new
SSLSocketFactory is supplied or add an explicit safeguard: if a non-null
trustManager is already present, log/warn or throw an IllegalStateException
before clearing it so callers are made aware; update
SDKBuilder.sslFactory(SSLSocketFactory) to implement one of these two options
and adjust Javadoc accordingly.
- Around line 419-425: Rename the misleading package-private getter
getSslFactory() to getSslSocketFactory() to reflect that it returns an
SSLSocketFactory; update all internal callers in the package (e.g., places that
reference SDKBuilder.getSslFactory()) to use getSslSocketFactory(); keep the
existing getTrustManager() name consistent with TrustProvider usage.
- Around line 396-417: The method getHttpClient currently has a broad
`@SuppressWarnings`("deprecation") that hides all deprecation warnings; narrow it
by removing the method-level annotation and apply
`@SuppressWarnings`("deprecation") only around the single deprecated call
httpClient.sslSocketFactory(sslSocketFactory) (e.g., enclose that branch in a
small local block with the annotation) or extract that fallback into a small
helper method (e.g., sslSocketFactoryFallback(SSLSocketFactory)) annotated with
the suppression; keep the existing logic using trustManager, sslSocketFactory,
usePlainText and httpClient as-is.
In `@sdk/src/test/java.security.test`:
- Around line 5-8: The default test config is pinned to BouncyCastle via the
security.provider entries, so update the test provider setup to exercise the
stock JDK provider stack (or add a dedicated CI lane) instead of BC: remove or
disable the lines registering org.bouncycastle.jce.provider.BouncyCastleProvider
and org.bouncycastle.jsse.provider.BouncyCastleJsseProvider in the test provider
file and replace them with a stock-JDK provider file (or CI job) so
TrustProvider and ECKeyPair are validated against the plain Java 11 providers
rather than BC/BCJSSE.
In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java`:
- Around line 100-106: Remove the duplicate compression call and redundant
prints: only call PemTestUtils.compressECPublickey(pubKey) once, assign it to
compressedKey, and print the result and length once; adjust the ECPublicKey
publicKey = PemTestUtils.publicKeyFromPem(pubKey) section to use the single
compressedKey and remove the extra
System.out.println(Arrays.toString(compressedKey)) and duplicate assignment to
compressedKey to avoid overwriting the value.
- Around line 167-180: The test testECDSA currently loops ECCurve.values() and
skips unsupported curves via ECCurve.isSupported() but doesn't assert that any
curve was actually exercised; add a counter or boolean flag inside
ECKeyPairTest.testECDSA to track that at least one supported curve ran
(increment when entering the loop body or on successful verify) and at the end
assertThat(counter > 0) or assertTrue(flag) so the test fails if no curves are
supported, referencing ECCurve.isSupported(), ECKeyPair, computeECDSASig and
verifyECDSAig to locate the loop to modify.
In `@sdk/src/test/java/io/opentdf/platform/sdk/SDKBuilderTest.java`:
- Around line 396-399: Remove the swallowing catch block that catches Exception
in the test around sdkServicesSetup; since sdkServicesSetup declares throws
Exception, let exceptions propagate instead of catching them with "catch
(Exception e) { assertThat(e).isNull(); }"—delete that catch clause so any
thrown Exception surfaces with its original stack trace (leave the try/finally
or convert to a plain method body as appropriate) and retain any finally cleanup
code.
In `@sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java`:
- Line 171: Replace use of AesGcm.generateKey() for HMAC test keys with a
dedicated HMAC key generator to avoid semantic coupling: add or use a method
like HmacKeyGenerator.generateKey() (or AssertionConfig.createHmacKey()) that
returns 256-bit raw bytes and update tests calling AesGcm.generateKey() (e.g.,
in TDFTest where AssertionConfig.AssertionKey(...) is invoked) to call the new
HMAC generator instead; ensure the new generator documents the required minimum
length for HS256 and returns the same byte length as the current AES helper so
test behavior remains unchanged.
🪄 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: 3dab7a35-bb89-4710-9659-7b13a17854ee
📒 Files selected for processing (17)
cmdline/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.security.fips.testsdk/src/test/java.security.testsdk/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.java
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-surefire-plugin</artifactId> | ||
| <configuration> | ||
| <argLine>-Djava.security.properties=${test.java.security.file}</argLine> | ||
| </configuration> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the specific lines in sdk/pom.xml
head -n 480 sdk/pom.xml | tail -n 30Repository: opentdf/java-sdk
Length of output: 1159
🏁 Script executed:
# Check for JaCoCo configuration in root pom.xml
grep -n "jacoco\|prepare-agent" pom.xml -A 5 -B 2Repository: opentdf/java-sdk
Length of output: 2443
🏁 Script executed:
# Search for all argLine definitions in pom.xml files
rg "argLine" -n pom.xml sdk/pom.xml cmdline/pom.xml examples/pom.xml 2>/dev/null || rg "argLine" -nRepository: opentdf/java-sdk
Length of output: 169
Preserve the existing Surefire argLine instead of replacing it.
The hardcoded argLine at line 471 overwrites the argLine property injected by JaCoCo's prepare-agent execution, preventing coverage data collection when the test profile is active.
Suggested fix
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<configuration>
- <argLine>-Djava.security.properties=${test.java.security.file}</argLine>
+ <argLine>@{argLine} -Djava.security.properties=${test.java.security.file}</argLine>
</configuration>
</plugin>📝 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.
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-surefire-plugin</artifactId> | |
| <configuration> | |
| <argLine>-Djava.security.properties=${test.java.security.file}</argLine> | |
| </configuration> | |
| <plugin> | |
| <groupId>org.apache.maven.plugins</groupId> | |
| <artifactId>maven-surefire-plugin</artifactId> | |
| <configuration> | |
| <argLine>@{argLine} -Djava.security.properties=${test.java.security.file}</argLine> | |
| </configuration> |
🤖 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/pom.xml` around lines 467 - 472, The maven-surefire-plugin configuration
is overwriting the JaCoCo-injected argLine, preventing coverage; update the
surefire <argLine> in the plugin to preserve and append to the existing value
(i.e., include the existing ${argLine} placeholder and then add the
-Djava.security.properties=${test.java.security.file} piece) so JaCoCo's
prepare-agent can inject its agent args while still setting the security
properties.
| 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 examine the ECKeyPair.java file
find . -name "ECKeyPair.java" -type fRepository: opentdf/java-sdk
Length of output: 119
🌐 Web query:
Java Base64.Decoder.decode() exception behavior malformed input IllegalArgumentException
💡 Result:
Java’s Base64.Decoder.decode(...) throws java.lang.IllegalArgumentException when the input is not in a valid Base64 scheme, and it may have already written some decoded bytes before the exception is thrown. What “malformed input” triggers 1) Wrong or mismatched padding at the end - The decoder accepts ‘=’ as the end of the encoded byte data, but if a padding character is present in the final quantum then the correct number of ‘=’ characters must be present; otherwise IllegalArgumentException is thrown during decoding [1]. 2) Characters not in the decoder’s Base64 alphabet - The basic decoder rejects data containing characters outside the Base64 alphabet [2]. Examples reported include “Illegal base64 character...” for characters like underscore when using the wrong alphabet/decoder [3], and URL/line-break related issues in practice [4][5]. 3) Incorrect final 4-byte ending unit (general structural malformation) - In general, invalid Base64 encoding scheme data causes IllegalArgumentException [1]. Real examples show messages like “Input byte array has wrong 4-byte ending unit” and are associated with decoding incomplete/incorrectly chunked input (e.g., decoding a full buffer rather than only the bytesRead, or using a buffer size not aligned with Base64’s 4-character groups) [6]. Side effects / partial output - For the array-to-array decode methods, Oracle’s Javadoc explicitly states: “If the input byte array is not in valid Base64 encoding scheme then some bytes may have been written to the output byte array before IllegalargumentException is thrown.” [1] - For the no-output-buffer (returns a new byte[]) decode methods, the same Javadoc section indicates IllegalArgumentException is thrown when src is not valid Base64 [1]. (The “position will not be advanced” wording is specifically mentioned for ByteBuffer decode variants.) [1] Buffered/streaming behavior - For Base64 stream decoding, the Javadoc notes that read methods of the returned InputStream will throw IOException when reading bytes that cannot be decoded. (So the exception type differs: IOException for stream reads; IllegalArgumentException for direct decode methods.) [1] Practical takeaways for IllegalArgumentException handling - Catch IllegalArgumentException around Base64.getDecoder/getUrlDecoder/getMimeDecoder.decode(...) when the input may be malformed [1]. - If you’re decoding data that may contain line breaks, ensure you’re using the appropriate decoder (e.g., MIME decoder) or strip/normalize whitespace/line separators before decoding—line breaks are a common cause of “illegal base64 character” style failures in practice [5]. - If decoding in chunks from an InputStream, decode only the exact bytes read (and/or use a Base64-decoding wrapper stream), because Base64 requires groups aligned to 4 characters/bytes quanta; otherwise you can get “wrong ending unit” errors [6].
Citations:
- 1: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/util/Base64.Decoder.html
- 2: https://docs.oracle.com/javase/8/docs/api/java/util/Base64.html
- 3: https://www.prostdev.com/post/understanding-the-illegal-base64-character-error-java-groovy-and-mule-4-dw-2-0
- 4: https://stackoverflow.com/questions/68019059/java-lang-illegalargumentexception-illegal-base64-character-a
- 5: IllegalArgumentException on x-www-form-urlencoded POST with Amazon Lambda HTTP extension quarkusio/quarkus#21576
- 6: https://stackoverflow.com/questions/32858758/illegalargumentexception-using-java8-base64-decoder
🏁 Script executed:
# Examine the ECKeyPair.java file, particularly around lines 54-58
cat -n ./sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java | sed -n '50,65p'Repository: opentdf/java-sdk
Length of output: 799
🏁 Script executed:
# Also check the broader context: method signature, imports, and usage patterns
cat -n ./sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java | head -80Repository: opentdf/java-sdk
Length of output: 3206
🏁 Script executed:
# Check where publicKeyFromPem is called within the file
rg "publicKeyFromPem" ./sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java -n -B 2 -A 2Repository: opentdf/java-sdk
Length of output: 351
🏁 Script executed:
# Also check if there are any calls to publicKeyFromPem from outside this file
rg "publicKeyFromPem" ./sdk/src/main/ -n --type javaRepository: opentdf/java-sdk
Length of output: 507
Wrap Base64 decoding to align with declared exception contract.
Line 56 can throw unchecked IllegalArgumentException from Base64.Decoder when the PEM input is malformed (e.g., invalid Base64 after header removal). Since this method receives untrusted external data from KAS responses and declares only checked exceptions, invalid input will escape as an undeclared unchecked exception, violating the method's 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);
+ byte[] der;
+ try {
+ der = Base64.getDecoder().decode(pemData);
+ } catch (IllegalArgumentException e) {
+ throw new InvalidKeySpecException("invalid PEM public key", e);
+ }
KeyFactory keyFactory = KeyFactory.getInstance("EC");
return (ECPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(der));
}📝 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.
| 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)); | |
| static ECPublicKey publicKeyFromPem(String pem) throws InvalidKeySpecException, NoSuchAlgorithmException { | |
| String pemData = pem.replaceAll("-----(BEGIN|END) [A-Z ]+-----", "").replaceAll("\\s", ""); | |
| byte[] der; | |
| try { | |
| der = Base64.getDecoder().decode(pemData); | |
| } catch (IllegalArgumentException e) { | |
| throw new InvalidKeySpecException("invalid PEM public key", e); | |
| } | |
| KeyFactory keyFactory = KeyFactory.getInstance("EC"); | |
| return (ECPublicKey) keyFactory.generatePublic(new X509EncodedKeySpec(der)); | |
| } |
🤖 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 calls
Base64.getDecoder().decode(pemData) which may throw an unchecked
IllegalArgumentException for malformed input; update publicKeyFromPem to catch
IllegalArgumentException around the Base64 decoding and rethrow it as a checked
InvalidKeySpecException (preserving the original exception as the cause) so the
method's declared exceptions cover malformed PEM input and callers receive a
consistent checked error; locate the Base64 decoding call in publicKeyFromPem
and wrap it with a try/catch that converts IllegalArgumentException into new
InvalidKeySpecException(..., e).
| /** | ||
| * Configure the SDK to use the supplied {@link SSLSocketFactory} for outbound TLS connections. | ||
| * Callers using this overload bring their own pre-built socket factory; cert-chain trust | ||
| * material is whatever the supplied factory was built with. For full PKIX validation under a | ||
| * matching {@link X509TrustManager}, use {@link #sslFactoryFromDirectory(String)} or | ||
| * {@link #sslFactoryFromKeyStore(String, String)} which build both via {@link TrustProvider}. | ||
| */ | ||
| public SDKBuilder sslFactory(SSLSocketFactory sslSocketFactory) { | ||
| this.sslSocketFactory = sslSocketFactory; | ||
| this.trustManager = null; | ||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * Configure the SDK to use the supplied {@link SSLSocketFactory} together with a matching | ||
| * {@link X509TrustManager}. The trust manager is used by OkHttp for certificate pinning and | ||
| * cleartext-fallback decisions; supply this overload when the caller has a trust manager that | ||
| * matches the socket factory's trust material (e.g. both built from a {@link TrustProvider}). | ||
| */ | ||
| public SDKBuilder sslFactory(SSLSocketFactory sslSocketFactory, X509TrustManager trustManager) { | ||
| this.sslSocketFactory = sslSocketFactory; | ||
| this.trustManager = trustManager; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
Public API: removed sslFactory(SSLFactory) overload is a breaking change.
Per coding guidelines, public SDK APIs should be stable and additive where possible. Removing public SDKBuilder sslFactory(nl.altindag.ssl.SSLFactory) (per the AI summary) breaks any downstream caller. Since the type is from a now-removed dependency, a same-name shim is impossible, but consider:
- Documenting this as a breaking change in release notes/CHANGELOG, and
- Bumping the major version per semver.
As per coding guidelines: "keep public SDK APIs stable and additive where possible".
🤖 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/SDKBuilder.java` around lines 79 -
102, The public overload SDKBuilder.sslFactory(nl.altindag.ssl.SSLFactory) was
removed which is a breaking API change; update the release artifacts to reflect
this by documenting the removal in the CHANGELOG/release notes, add a clear
migration note in the SDKBuilder Javadoc near the remaining
sslFactory(SSLSocketFactory) and sslFactory(SSLSocketFactory, X509TrustManager)
methods that explains the old overload was removed and how to migrate, and bump
the major version per semver to signal the breaking change.
| public SDKBuilder sslFactoryFromKeyStore(String keystorePath, String keystorePassword) { | ||
| this.sslFactory = SSLFactory.builder().withDefaultTrustMaterial().withSystemTrustMaterial() | ||
| .withTrustMaterial(Path.of(keystorePath), | ||
| keystorePassword == null ? "".toCharArray() : keystorePassword.toCharArray()) | ||
| .build(); | ||
| try { | ||
| TrustProvider provider = TrustProvider.fromKeyStore( | ||
| Path.of(keystorePath), | ||
| keystorePassword == null ? "".toCharArray() : keystorePassword.toCharArray()); | ||
| this.sslSocketFactory = provider.getSslSocketFactory(); | ||
| this.trustManager = provider.getTrustManager(); | ||
| } catch (IOException | java.security.GeneralSecurityException e) { | ||
| throw new SDKException("failed to load keystore from " + keystorePath, e); | ||
| } | ||
| return this; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect TrustProvider.fromKeyStore implementation to verify null-password handling.
fd -t f 'TrustProvider.java' | xargs -I{} sed -n '1,200p' {}Repository: opentdf/java-sdk
Length of output: 7986
Fix null keystore password handling to preserve backward compatibility.
The code converts null keystorePassword to "".toCharArray(), but KeyStore.load() treats these differently: null skips integrity verification (allowing keystores created without a password), while an empty password attempts to verify with an empty passphrase. This breaks backward compatibility with keystores that have no integrity password.
Suggested change
- TrustProvider provider = TrustProvider.fromKeyStore(
- Path.of(keystorePath),
- keystorePassword == null ? "".toCharArray() : keystorePassword.toCharArray());
+ TrustProvider provider = TrustProvider.fromKeyStore(
+ Path.of(keystorePath),
+ keystorePassword == null ? null : keystorePassword.toCharArray());TrustProvider.fromKeyStore() accepts null and forwards it to KeyStore.load(), which handles it correctly.
📝 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.
| public SDKBuilder sslFactoryFromKeyStore(String keystorePath, String keystorePassword) { | |
| this.sslFactory = SSLFactory.builder().withDefaultTrustMaterial().withSystemTrustMaterial() | |
| .withTrustMaterial(Path.of(keystorePath), | |
| keystorePassword == null ? "".toCharArray() : keystorePassword.toCharArray()) | |
| .build(); | |
| try { | |
| TrustProvider provider = TrustProvider.fromKeyStore( | |
| Path.of(keystorePath), | |
| keystorePassword == null ? "".toCharArray() : keystorePassword.toCharArray()); | |
| this.sslSocketFactory = provider.getSslSocketFactory(); | |
| this.trustManager = provider.getTrustManager(); | |
| } catch (IOException | java.security.GeneralSecurityException e) { | |
| throw new SDKException("failed to load keystore from " + keystorePath, e); | |
| } | |
| return this; | |
| } | |
| public SDKBuilder sslFactoryFromKeyStore(String keystorePath, String keystorePassword) { | |
| try { | |
| TrustProvider provider = TrustProvider.fromKeyStore( | |
| Path.of(keystorePath), | |
| keystorePassword == null ? null : keystorePassword.toCharArray()); | |
| this.sslSocketFactory = provider.getSslSocketFactory(); | |
| this.trustManager = provider.getTrustManager(); | |
| } catch (IOException | java.security.GeneralSecurityException e) { | |
| throw new SDKException("failed to load keystore from " + keystorePath, e); | |
| } | |
| return this; | |
| } |
🤖 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/SDKBuilder.java` around lines 124 -
135, The sslFactoryFromKeyStore method currently replaces a null
keystorePassword with "".toCharArray(), which forces KeyStore.load() to treat it
as an empty password and breaks compatibility; update
SDKBuilder.sslFactoryFromKeyStore to pass the original keystorePassword through
to TrustProvider.fromKeyStore (i.e., pass null when null) so
TrustProvider.fromKeyStore (and ultimately KeyStore.load()) can handle null
correctly; locate and change the ternary that builds the char[] password and
ensure no other code in sslFactoryFromKeyStore converts null to an empty char[].
| securerandom.strongAlgorithms=DRBG:SUN | ||
|
|
||
| security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider | ||
| security.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider fips:BCFIPS | ||
| security.provider.3=SUN |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "java.security.fips.test" -type fRepository: opentdf/java-sdk
Length of output: 99
🏁 Script executed:
find . -name "AesGcm.java" -type fRepository: opentdf/java-sdk
Length of output: 116
🏁 Script executed:
find sdk/src/test -name "*java.security*" -type fRepository: opentdf/java-sdk
Length of output: 129
🏁 Script executed:
cat -n sdk/src/test/java.security.fips.testRepository: opentdf/java-sdk
Length of output: 735
🏁 Script executed:
cat -n sdk/src/main/java/io/opentdf/platform/sdk/AesGcm.javaRepository: opentdf/java-sdk
Length of output: 9488
🏁 Script executed:
rg -i "bcfips|bouncycastle" --type md --type txt --type java -A 2 -B 2 | head -50Repository: opentdf/java-sdk
Length of output: 1678
🏁 Script executed:
rg "securerandom" -A 2 -B 2Repository: opentdf/java-sdk
Length of output: 403
🏁 Script executed:
find . -name "*.java" -type f -exec grep -l "getInstanceStrong" {} \;Repository: opentdf/java-sdk
Length of output: 116
🏁 Script executed:
cat -n sdk/src/test/java.security.testRepository: opentdf/java-sdk
Length of output: 451
🏁 Script executed:
find . -name "pom.xml" -o -name "build.gradle" | xargs grep -l "bouncycastle\|bcfips" 2>/dev/nullRepository: opentdf/java-sdk
Length of output: 84
🏁 Script executed:
rg "DRBG" -A 3 -B 3Repository: opentdf/java-sdk
Length of output: 602
🏁 Script executed:
cat -n sdk/pom.xml | grep -A 5 -B 5 "bcfips\|bouncycastle"Repository: opentdf/java-sdk
Length of output: 2133
🏁 Script executed:
rg "BouncyCastleFipsProvider|BCFIPS" -A 5 -B 2Repository: opentdf/java-sdk
Length of output: 707
🏁 Script executed:
rg "SecureRandom" -t java -B 3 -A 3 | head -40Repository: opentdf/java-sdk
Length of output: 3780
🌐 Web query:
BCFIPS BouncyCastle DRBG configuration securerandom.strongAlgorithms
💡 Result:
In Bouncy Castle FIPS (BCFIPS), the DRBG-based SecureRandom is configured primarily through the BouncyCastleFipsProvider constructor or java.security provider definition using a configuration string starting with "C:" (e.g., "C:DEFRND[SHA512];ENABLE{ALL};" for SHA-512 DRBG, "C:HYBRID;ENABLE{ALL};" for hybrid entropy pool mode).[1][2][3][4] The provider's default is a prediction-resistant SHA-512 DRBG if no CryptoServicesRegistrar.setSecureRandom is set.[1][2] The java.security property securerandom.strongAlgorithms specifies the entropy source for seeding the DRBG, as BCFIPS is a software provider requiring external entropy (e.g., NativePRNGBlocking:SUN).[5][6][7] Setting DEFAULT:BCFIPS causes stack overflow; use NativePRNGBlocking:SUN (FIPS-safe currently, but monitor SP 800-90B compliance).[5] Examples include DRBG(SHA512):BCFIPS or ENTROPY:BCRNG with BC JENT provider.[3][8] For FIPS-approved mode, ensure high security strength (e.g., 256 bits) via DRBG config and strong entropy source to avoid errors like "FIPS SecureRandom security strength not as high as required".[8][7]
Citations:
- 1: https://downloads.bouncycastle.org/fips-java/docs/bc-fips-2.0.0-javadoc/org/bouncycastle/jcajce/provider/BouncyCastleFipsProvider.html
- 2: https://downloads.bouncycastle.org/fips-java/docs/bc-fips-2.1.0-javadoc/org/bouncycastle/jcajce/provider/BouncyCastleFipsProvider.html
- 3: https://downloads.bouncycastle.org/fips-java/docs/BC-FJA-UserGuide-2.0.0.pdf
- 4: https://downloads.bouncycastle.org/fips-java/docs/BC-FJA-UserGuide-2.1.0.pdf
- 5: BCFIPS 2.0 StackOverflowError when loading com.sun.jna.Native bcgit/bc-java#1800
- 6: https://docs.oracle.com/en/java/javase/26/docs/api/java.base/java/security/SecureRandom.html
- 7: https://downloads.bouncycastle.org/fips-java/docs/bc-fips-2.1.0-javadoc/org/bouncycastle/crypto/fips/FipsDRBG.Base.html
- 8: BC Approved mode on java-8 throws “FIPS SecureRandom security strength not as high as required for operation: DH” bcgit/bc-java#1897
Change securerandom.strongAlgorithms to use BCFIPS DRBG, not SUN.
Line 4 specifies DRBG:SUN, which forces SecureRandom.getInstanceStrong() to source randomness from the SUN provider. This means AesGcm.encrypt() and other paths using strong randomness are exercised with a non-FIPS RNG even when the fips profile is active. The FIPS profile should use BCFIPS's DRBG instead to properly validate FIPS-approved randomness paths.
🤖 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.security.fips.test` around lines 4 - 8, Change the
securerandom.strongAlgorithms entry to use BCFIPS's DRBG instead of SUN so
SecureRandom.getInstanceStrong() and FIPS-sensitive paths (e.g.,
AesGcm.encrypt()) use the BCFIPS RNG; update the property
securerandom.strongAlgorithms from "DRBG:SUN" to "DRBG:BCFIPS" (ensuring the
BouncyCastle FIPS provider entries remain present) so the FIPS profile exercises
the BCFIPS DRBG implementation.
2 participants
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.
Summary by CodeRabbit
Release Notes
New Features
Improvements