fix: verify that BC works with approved_only mode set#375
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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) | ⏱️ ~50 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, fallback behavior, and verification script correctness. Possibly related PRs
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 |
mkleene
changed the title
approved_only mode setapproved_only mode set
Removed unused Logger imports from Command.java
X-Test Failure Report |
pflynn-virtru
left a comment
pflynn-virtru
left a comment
There was a problem hiding this comment.
Nice to have an interop test with FIPS and non-FIPS
marythought
left a comment
marythought
left a comment
There was a problem hiding this comment.
DX Review — release / migration impact
The FIPS fixes are correct, but this is a breaking change for existing FIPS + EC deployments and the release process needs to surface that.
1. Migration note via release-please
Since this repo uses release-please, the squash-merge commit message needs a BREAKING CHANGE: footer so it appears in the generated release notes. Something like:
BREAKING CHANGE: FIPS deployments using EC key wrapping must add
`io.opentdf.platform:sdk-fips-bouncycastle` to their classpath.
Without it, HKDF key derivation fails under
`org.bouncycastle.fips.approved_only=true`.
Without this footer, the change ships as a minor fix: and FIPS consumers discover the new dependency requirement at runtime.
2. Failure mode isn't discoverable
If a FIPS consumer doesn't add the jar: HkdfResolver.get() returns null → calculateHKDF falls through to the JDK-native HmacSHA256 → same IllegalKey error as before. The error doesn't mention sdk-fips-bouncycastle or how to fix it. Consider logging a warning when approved_only=true is set but no HkdfProvider resolves — that would point people to the fix instead of leaving them with a raw BC exception.
3. Wire compatibility of ENCRYPT→WRAP cipher mode change
The AsymEncryption/AsymDecryption change from ENCRYPT_MODE/DECRYPT_MODE to WRAP_MODE/UNWRAP_MODE should be wire-compatible (OAEP produces the same bytes either way), but worth confirming in the release notes that existing TDFs encrypted with the old mode can still be decrypted — this is the kind of thing a security-conscious consumer will ask about.
|
|
4 participants
The bouncycastle FIPS provider happily does things that are not FIPS-compliant when the
org.bouncycastle.fips.approved_onlysystem property is not set totrue. The following problems were observed:ENCRYPTrather thanWRAP. We are indeed using OEAP for key wrapping so we just hadto change some cipher parameters.
WRAPandENCRYPTperform the same operation; the only differenceis in the semantics and the JCA API associated with each operation. This should not affect compatibility with other parts of the platform in any way.
approved_onlyBC would be fine using a non-FIPS truststore. Now we add an empty truststore(with an empty password) of a type that can be loaded by the BC FIPS truststore.
SHA-265/HMACwithin the HKDFimplementation triggers 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.We include the FIPS implementation of HKDF in a separate JAR due to its dependency on the bouncycastle FIPS provider. If we included it in the same JAR we would have a dependency on
bc-fips, which would conflict with usage of the regular bouncycastle provider. Since bouncycastle is popular it seems best to allow non-FIPS usage.The major change from a customer POV is that customers that want to use EC crypto in a FIPS context will now
need to pull in the
sdk-fips-bouncycastlejar and depend onbc-fipssince HKDF only got a JCA namein JDK 24 and bc-fips does not expose HKDF through a provider interface. If using
bc-fipsis not appropriatein a particular application it would be quite easy to implement an alternative JAR that implements a similar
provider pattern and uses whichever FIPS-compliant library one wishes to implement HKDF.
Also:
checks.yamlin to bash files so they can be run locally more easilycmdlineplatform integration phasejava.securityfile in its entirety so that we know exactly which providers we are using in FIPS tests