fix: verify that bc works with approved_only mode set#375
Conversation
📝 WalkthroughWalkthroughThis PR adds FIPS-140 compliance by introducing a pluggable HKDF service provider interface (SPI), a BouncyCastle FIPS implementation, asymmetric key-wrapping semantics, and supporting build/test infrastructure for FIPS-enforced test runs, CLI verification, and matrix-based CI/CD workflows. ChangesFIPS-Approved Encryption and Pluggable HKDF Provider
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The PR introduces multiple functional layers: a pluggable SPI for HKDF, a new FIPS module with implementation and tests, core asymmetric crypto changes from encrypt/decrypt to key-wrapping semantics, FIPS configuration across multiple POM files and test resources, and CI/CD matrix workflow changes. While individual layers are well-scoped, the interdependencies and crypto semantic changes demand careful review of key wrapping logic, FIPS provider registration, and fallback behavior. Possibly related PRs
Suggested reviewers
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 introduces FipsProviderVerificationTest.java to verify that the FIPS security provider configuration is correctly loaded. A review comment points out that the system property org.bouncycastle.fips.approved_only is not configured in the fips profile in sdk/pom.xml, which would cause the test to be silently skipped, and provides a configuration fix.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Switch AsymEncryption from ENCRYPT_MODE+doFinal to WRAP_MODE+wrap, and AsymDecryption from DECRYPT_MODE+doFinal to UNWRAP_MODE+unwrap, treating the key material as AES SecretKeySpec for FIPS compatibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
X-Test Failure Report✅ java@v0.15.0-v0.16.0 |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
Removed static logger initialization from TDFReader class.
X-Test Failure Report |
X-Test Failure Report |
X-Test Failure Report |
Removed unnecessary blank lines for cleaner code.
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.
Actionable comments posted: 3
🧹 Nitpick comments (4)
sdk-fips-bouncycastle/pom.xml (1)
35-40: ⚡ Quick winCentralize AssertJ version in parent dependencyManagement.
The AssertJ version
3.27.7is hardcoded here and also appears hardcoded insdk/pom.xml:180. For consistency and easier maintenance, define it in the parent POM'sdependencyManagementsection (similar to howbc-fips, JUnit, and other dependencies are managed).🤖 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-fips-bouncycastle/pom.xml` around lines 35 - 40, The AssertJ version is hardcoded for org.assertj:assertj-core in this module; remove the <version>3.27.7</version> here and instead add a managed version entry in the parent POM's dependencyManagement (e.g., add an <dependencyManagement><dependencies><dependency> with groupId org.assertj, artifactId assertj-core and the version 3.27.7). Then update any other POMs that currently hardcode assertj-core (such as the sdk/pom.xml entry) to rely on the parent-managed version by omitting the <version> element and keeping only the dependency coordinates.sdk-fips-bouncycastle/src/test/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProviderTest.java (1)
9-33: ⚡ Quick winConsider adding edge case tests for robustness.
The current tests cover the happy path well, but consider adding tests for edge cases to improve robustness:
- Null salt (RFC 5869 specifies zero-filled buffer substitution)
- Empty salt
- Null secret (expected behavior: throw or handle gracefully?)
- Empty secret
- Known test vectors from RFC 5869 for cross-validation
🤖 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-fips-bouncycastle/src/test/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProviderTest.java` around lines 9 - 33, Add unit tests in BouncyCastleFipsHkdfProviderTest that exercise edge cases for BouncyCastleFipsHkdfProvider.computeHKDF: verify behavior when salt is null (expect provider to treat it as an RFC-5869 zero-filled salt), when salt is empty, when secret is null (assert expected exception or documented handling), when secret is empty, and add at least one known RFC-5869 test vector to validate output against a canonical value; use the existing test structure/method names (computeHKDF_returns32Bytes, computeHKDF_isDeterministic) as examples for naming and assertions..github/scripts/verify_assertions_rs256.sh (1)
23-23: 💤 Low valueInconsistent argument quoting style across verification scripts.
This script uses
--with-assertions "$SIGNED_ASSERTIONS_RS256"(space-separated, line 23) whileverify_assertions_basic.shandverify_assertions_hs256.shuse--with-assertions="$ASSERTIONS"(equals-separated, no space). Both forms work in Bash, but consistency would improve maintainability.🤖 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/scripts/verify_assertions_rs256.sh at line 23, Change the inconsistent argument quoting in the RS256 script: replace the space-separated option form --with-assertions "$SIGNED_ASSERTIONS_RS256" with the equals form --with-assertions="$SIGNED_ASSERTIONS_RS256" to match verify_assertions_basic.sh and verify_assertions_hs256.sh; update the line in .github/scripts/verify_assertions_rs256.sh so the option uses the equals sign with the quoted variable.sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java (1)
119-125: 💤 Low valueConsider the overhead of debug logging on every HKDF call.
The provider resolution and debug logging occur on every invocation of
calculateHKDF. WhileHkdfResolver.get()returns a cached singleton, the null check and debug log at lines 121-123 execute on every call. If HKDF derivation is performance-sensitive or called frequently, consider whether debug-level logging here adds measurable overhead.🤖 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 119 - 125, The debug check and provider resolution are executed on every calculateHKDF call; to avoid per-call overhead, resolve and cache the HkdfProvider once (via HkdfResolver.get()) and perform the log.debug(...) only when the provider is initially obtained or changes rather than inside every calculateHKDF invocation; update calculateHKDF to use the cached provider (HkdfProvider) and remove the repeated log.isDebugEnabled() check on the hot path, or cache log.isDebugEnabled() in a static/instance boolean so only provider.computeHKDF(salt, secret) runs on each call.
🤖 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 @.github/workflows/checks.yaml:
- Line 96: Fix the duplicated word in the comment string "install the the
sdk-fips-bouncycastle jar so that FIPS mode tests work" by changing it to
"install the sdk-fips-bouncycastle jar so that FIPS mode tests work"; locate and
update the comment in .github/workflows/checks.yaml (the comment containing
"install the the sdk-fips-bouncycastle jar") so the duplicated "the" is removed.
In
`@sdk-fips-bouncycastle/src/main/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProvider.java`:
- Around line 14-18: BouncyCastleFipsHkdfProvider.computeHKDF currently forwards
salt/secret directly to FipsKDF; make its behavior match the JDK fallback by
normalizing salt==null or salt.length==0 to a 32-byte zero array before calling
.withSalt(...), and explicitly validate secret (e.g., throw a clear
NPE/IllegalArgumentException or document acceptance) so
HkdfProvider.computeHKDF, BouncyCastleFipsHkdfProvider.computeHKDF and
ECKeyPair.calculateHKDF have consistent null/empty handling; update computeHKDF
to perform the salt normalization and secret null-check prior to calling
.build(secret).
In `@sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java`:
- Line 7: Remove the unused import of DisabledIfSystemProperty from the
ECKeyPairTest class: locate the import statement referencing
DisabledIfSystemProperty and delete it so the class ECKeyPairTest no longer
contains an unused import; run the test compile to verify there are no remaining
unused-import warnings.
---
Nitpick comments:
In @.github/scripts/verify_assertions_rs256.sh:
- Line 23: Change the inconsistent argument quoting in the RS256 script: replace
the space-separated option form --with-assertions "$SIGNED_ASSERTIONS_RS256"
with the equals form --with-assertions="$SIGNED_ASSERTIONS_RS256" to match
verify_assertions_basic.sh and verify_assertions_hs256.sh; update the line in
.github/scripts/verify_assertions_rs256.sh so the option uses the equals sign
with the quoted variable.
In `@sdk-fips-bouncycastle/pom.xml`:
- Around line 35-40: The AssertJ version is hardcoded for
org.assertj:assertj-core in this module; remove the <version>3.27.7</version>
here and instead add a managed version entry in the parent POM's
dependencyManagement (e.g., add an
<dependencyManagement><dependencies><dependency> with groupId org.assertj,
artifactId assertj-core and the version 3.27.7). Then update any other POMs that
currently hardcode assertj-core (such as the sdk/pom.xml entry) to rely on the
parent-managed version by omitting the <version> element and keeping only the
dependency coordinates.
In
`@sdk-fips-bouncycastle/src/test/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProviderTest.java`:
- Around line 9-33: Add unit tests in BouncyCastleFipsHkdfProviderTest that
exercise edge cases for BouncyCastleFipsHkdfProvider.computeHKDF: verify
behavior when salt is null (expect provider to treat it as an RFC-5869
zero-filled salt), when salt is empty, when secret is null (assert expected
exception or documented handling), when secret is empty, and add at least one
known RFC-5869 test vector to validate output against a canonical value; use the
existing test structure/method names (computeHKDF_returns32Bytes,
computeHKDF_isDeterministic) as examples for naming and assertions.
In `@sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java`:
- Around line 119-125: The debug check and provider resolution are executed on
every calculateHKDF call; to avoid per-call overhead, resolve and cache the
HkdfProvider once (via HkdfResolver.get()) and perform the log.debug(...) only
when the provider is initially obtained or changes rather than inside every
calculateHKDF invocation; update calculateHKDF to use the cached provider
(HkdfProvider) and remove the repeated log.isDebugEnabled() check on the hot
path, or cache log.isDebugEnabled() in a static/instance boolean so only
provider.computeHKDF(salt, secret) runs on each call.
🪄 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: c41c71a4-8315-4746-ae6d-c490b8931c6f
📒 Files selected for processing (24)
.github/scripts/verify_assertions_basic.sh.github/scripts/verify_assertions_hs256.sh.github/scripts/verify_assertions_rs256.sh.github/scripts/verify_cmdline_roundtrip.sh.github/workflows/checks.yamlcmdline/pom.xmlcmdline/src/main/java/io/opentdf/platform/Command.javacmdline/src/main/resources/log4j2.xmlpom.xmlsdk-fips-bouncycastle/pom.xmlsdk-fips-bouncycastle/src/main/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProvider.javasdk-fips-bouncycastle/src/main/resources/META-INF/services/io.opentdf.platform.sdk.HkdfProvidersdk-fips-bouncycastle/src/test/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProviderTest.javasdk/pom.xmlsdk/src/main/java/io/opentdf/platform/sdk/AsymDecryption.javasdk/src/main/java/io/opentdf/platform/sdk/AsymEncryption.javasdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javasdk/src/main/java/io/opentdf/platform/sdk/HkdfProvider.javasdk/src/main/java/io/opentdf/platform/sdk/HkdfResolver.javasdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javasdk/src/test/java/io/opentdf/platform/sdk/FipsProviderVerificationTest.javasdk/src/test/resources/empty-fips-truststore.bcfkssdk/src/test/resources/empty-fips-truststore.p12sdk/src/test/resources/java.security.fips.test
| - name: Tests and enforcer (fips) | ||
| run: mvn --batch-mode test enforcer:enforce -P 'fips,!non-fips' -Dmaven.antrun.skip | ||
| run: | | ||
| # install the the sdk-fips-bouncycastle jar so that FIPS mode tests work |
There was a problem hiding this comment.
Fix typo in comment.
The comment contains a duplicated word: "install the the sdk-fips-bouncycastle jar" should be "install the sdk-fips-bouncycastle jar".
📝 Proposed fix
- # install the the sdk-fips-bouncycastle jar so that FIPS mode tests work
+ # install the sdk-fips-bouncycastle jar so that FIPS mode tests work📝 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.
| # install the the sdk-fips-bouncycastle jar so that FIPS mode tests work | |
| # install the sdk-fips-bouncycastle jar so that FIPS mode tests work |
🤖 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 at line 96, Fix the duplicated word in the
comment string "install the the sdk-fips-bouncycastle jar so that FIPS mode
tests work" by changing it to "install the sdk-fips-bouncycastle jar so that
FIPS mode tests work"; locate and update the comment in
.github/workflows/checks.yaml (the comment containing "install the the
sdk-fips-bouncycastle jar") so the duplicated "the" is removed.
| public byte[] computeHKDF(byte[] salt, byte[] secret) { | ||
| var key = FipsKDF.HKDF_KEY_BUILDER | ||
| .withPrf(FipsKDF.AgreementKDFPRF.SHA256_HMAC) | ||
| .withSalt(salt) | ||
| .build(secret); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check HkdfProvider interface for null-handling documentation or validation patterns
# Search for HkdfProvider interface definition and javadoc
rg -A 10 -B 2 'interface HkdfProvider' sdk/src/main/java/
# Check if other HKDF call sites in ECKeyPair handle null
rg -A 3 -B 3 'calculateHKDF' sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.javaRepository: opentdf/java-sdk
Length of output: 1202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show HkdfProvider javadoc + method signature
sed -n '1,120p' sdk/src/main/java/io/opentdf/platform/sdk/HkdfProvider.java
# Show ECKeyPair.calculateHKDF (and any null checks around it)
rg -n "calculateHKDF\\(" sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java
sed -n '1,220p' sdk/src/main/java/io/opentdf/platform/sdk/ECKeyPair.java
# Show computeHKDF implementation in the FIPS provider
sed -n '1,120p' sdk-fips-bouncycastle/src/main/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProvider.javaRepository: opentdf/java-sdk
Length of output: 9432
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all call sites of calculateHKDF
rg -n "calculateHKDF\\(" -S .
# Look for any explicit null checks or defaulting for salt/secret at call sites
rg -n "calculateHKDF\\(.*null|null.*\\)" -S .
# Check HkdfResolver (in case it wraps/validates inputs)
rg -n "class HkdfResolver|interface HkdfResolver|HkdfResolver" sdk/src/main/java/io/opentdf/platform/sdk -SRepository: opentdf/java-sdk
Length of output: 41640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where GLOBAL_KEY_SALT is defined (and its value)
rg -n "GLOBAL_KEY_SALT" sdk/src/main/java/io/opentdf/platform/sdk -S
rg -n "GLOBAL_KEY_SALT" sdk/src/test/java/io/opentdf/platform/sdk -S
# Inspect TDF/KASClient around HKDF usage and the constant definition context
sed -n '200,320p' sdk/src/main/java/io/opentdf/platform/sdk/TDF.java
sed -n '140,240p' sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java
# Check tests for null salt/secret behavior
rg -n "calculateHKDF\\(null|computeHKDF\\(null|salt.*null|secret.*null" sdk/src/test/java/io/opentdf/platform/sdk -S
sed -n '1,220p' sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javaRepository: opentdf/java-sdk
Length of output: 10374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect HkdfResolver
sed -n '1,160p' sdk/src/main/java/io/opentdf/platform/sdk/HkdfResolver.java
# Check whether there are any tests for null salt/secret in HKDF
rg -n "calculateHKDF\\(.*null|computeHKDF\\(.*null" sdk/src/test/java/io/opentdf/platform/sdk -S
# Inspect ECKeyPairTest around HKDF
sed -n '1,240p' sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.javaRepository: opentdf/java-sdk
Length of output: 770
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect ECKeyPairTest around HKDF
sed -n '1,240p' sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java
# Search for any HKDF null-input tests (allow empty result)
rg -n "computeHKDF\\(|calculateHKDF\\(" sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java sdk/src/test/java/io/opentdf/platform/sdk/TDFTest.java sdk/src/test/java/io/opentdf/platform/sdk/KASClientTest.java || true
rg -n "calculateHKDF\\(\\s*(null|\\{)\\s*|computeHKDF\\(\\s*(null|\\{)\\s*" sdk/src/test/java/io/opentdf/platform/sdk -S || true
# Also inspect the provider tests (if any)
fd -e java sdk/src/test/java/io/opentdf/platform/sdk | rg -n "Fips|Bouncy|Hkdf|Provider" || trueRepository: opentdf/java-sdk
Length of output: 9597
Align HKDF null-handling contract across JDK fallback and FIPS provider
ECKeyPair.calculateHKDF supports salt == null (it replaces null/empty salt with 32 zero bytes) when using the JDK fallback, but when an HkdfProvider is resolved it forwards salt/secret directly to HkdfProvider.computeHKDF(...). BouncyCastleFipsHkdfProvider.computeHKDF (lines 14-18) passes salt into .withSalt(salt) and secret into .build(secret) without any null checks, and HkdfProvider’s interface Javadoc doesn’t specify null expectations. Update the FIPS provider (or the resolver/wrapper) to match the fallback behavior for salt, and validate/document secret null-handling so FIPS and non-FIPS modes behave consistently.
🤖 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-fips-bouncycastle/src/main/java/io/opentdf/platform/sdk/fips/bouncycastle/BouncyCastleFipsHkdfProvider.java`
around lines 14 - 18, BouncyCastleFipsHkdfProvider.computeHKDF currently
forwards salt/secret directly to FipsKDF; make its behavior match the JDK
fallback by normalizing salt==null or salt.length==0 to a 32-byte zero array
before calling .withSalt(...), and explicitly validate secret (e.g., throw a
clear NPE/IllegalArgumentException or document acceptance) so
HkdfProvider.computeHKDF, BouncyCastleFipsHkdfProvider.computeHKDF and
ECKeyPair.calculateHKDF have consistent null/empty handling; update computeHKDF
to perform the salt normalization and secret null-check prior to calling
.build(secret).
| import java.security.interfaces.ECPublicKey; | ||
|
|
||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.condition.DisabledIfSystemProperty; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if DisabledIfSystemProperty is actually used in the test file
rg -n 'DisabledIfSystemProperty' sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java -C 3Repository: opentdf/java-sdk
Length of output: 288
Remove unused DisabledIfSystemProperty import in ECKeyPairTest
DisabledIfSystemProperty only appears as an import (sdk/src/test/java/io/opentdf/platform/sdk/ECKeyPairTest.java:7) and is not referenced anywhere else in the test class.
🤖 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 7,
Remove the unused import of DisabledIfSystemProperty from the ECKeyPairTest
class: locate the import statement referencing DisabledIfSystemProperty and
delete it so the class ECKeyPairTest no longer contains an unused import; run
the test compile to verify there are no remaining unused-import warnings.
|
mkleene
changed the title
approved_only mode set
1 participant
The bouncycastle FIPS provider can do things that do not strictly conform to FIPS when the
org.bouncycastle.fips.approved_onlysystem property is not set totrue.Two things that failed under this stricter regime were
ENCRYPTrather thanWRAP. We are indeed using OEAP for key wrapping so we just hadto change some cipher parameters.
SHA-265/HMACtriggers the error:ECKeyPairTest.createSymmetricKeysWithOtherCurves:129 » IllegalKey Key size for HMAC must be at least 112 bits in approved mode: SHA-256/HMAC. By using a FIPS compliant provider that implements HKDF directly we bypass this error.Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests