fix(core): Uses strict FIPS AES-GCM#3507
Conversation
Signed-off-by: strantalis <strantalis@virtru.com>
📝 WalkthroughWalkthroughThis PR migrates AES-GCM encryption/decryption to use Go's cipher.NewGCMWithRandomNonce (embedded nonces), removes caller-managed IV encryption APIs, updates asymmetric wrappers and protected-key validation, and adapts tests to the new sealed-output format. ChangesAES-GCM random nonce migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request updates the OpenTDF AES-GCM cryptographic helpers to align with strict FIPS 140-3 requirements by migrating to Go's internal random-nonce AEAD. The changes enforce safer defaults by restricting caller-managed IVs and non-standard tag sizes, while maintaining backward compatibility for standard TDF data and EC wrapping. These modifications ensure the platform remains compliant when running in restricted FIPS-only environments. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. In FIPS we trust to keep the key, With random nonces, safe and free. No caller IVs, no custom tag, Just standard crypto, never lag. Footnotes
|
![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 transitions the ocrypto package to use cipher.NewGCMWithRandomNonce for AES-GCM, enforcing internally generated nonces for FIPS compliance and deprecating caller-managed IV encryption. The review identifies a critical bug in EncryptInPlace where aliasing the destination buffer with the plaintext causes data corruption due to the prepended nonce. Further feedback recommends strengthening ciphertext length validation in Decrypt and optimizing DecryptWithIVAndTagSize by using the standard cipher.NewGCM to avoid unnecessary memory allocations during decryption.
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@lib/ocrypto/aes_gcm_test.go`:
- Around line 91-119: The test TestCreateAESGcm_EncryptInPlace currently never
hits the in-place code path because append([]byte{}, plainText...) yields
cap==len; add an additional test case that constructs a pre-sized buffer (e.g.
buf := make([]byte, len(plainText),
len(plainText)+GcmStandardNonceSize+aes.BlockSize)), copy( buf, plainText ),
then call aesGcm.EncryptInPlace(buf) so gcm.Seal uses the existing backing array
and the in-place code path (and potential alias.InexactOverlap) is exercised;
keep the existing non-in-place case for coverage and assert nonce length,
ciphertext length, and successful DecryptWithIVAndTagSize on the in-place result
just like the current assertions.
In `@lib/ocrypto/aes_gcm.go`:
- Around line 87-98: The len(data) < GcmStandardNonceSize guard in
AesGcm.DecryptWithTagSize duplicates the same check performed in AesGcm.Decrypt
(and is therefore dead); either remove that check from DecryptWithTagSize, or
replace it with a stricter pre-check (len(data) < GcmStandardNonceSize +
aes.BlockSize) so clearly truncated inputs return ErrInvalidCiphertext instead
of allowing gcm.Open to produce a wrapped error. Update DecryptWithTagSize
accordingly and keep references to Decrypt, GcmStandardNonceSize, aes.BlockSize,
ErrInvalidCiphertext, and gcm.Open in your reasoning while editing.
- Around line 48-57: EncryptInPlace currently passes a slice backed by the
caller's plaintext (data[:0]) as the Seal destination which can create an
inexact-overlap panic when the plaintext has spare capacity; change
EncryptInPlace (method on AesGcm) to allocate a fresh destination buffer (e.g.
dst := make([]byte, 0, GcmStandardNonceSize+len(data)+gcm.Overhead())) and call
gcm.Seal(dst, nil, data, nil) instead of gcm.Seal(data[:0], ...), then extract
nonce and cipherText from the returned sealed slice as before
(sealed[:GcmStandardNonceSize], sealed[GcmStandardNonceSize:]) to avoid
overlapping input/output.
In `@lib/ocrypto/asym_decryption.go`:
- Around line 180-192: Move the cheap input-length precondition check so it runs
before constructing the AEAD: check len(data) <
GcmStandardNonceSize+aes.BlockSize at the top of the block (before calling
cipher.NewGCMWithRandomNonce(block)), return the same "ciphertext too short"
error if true, and only then call cipher.NewGCMWithRandomNonce(block) and use
gcm.Open; this preserves behavior but avoids constructing the AEAD for obviously
malformed inputs (see GcmStandardNonceSize, aes.BlockSize,
cipher.NewGCMWithRandomNonce, and gcm.Open).
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ecf36cc8-ecde-4300-8b90-409af9d22b2b
📒 Files selected for processing (4)
lib/ocrypto/aes_gcm.golib/ocrypto/aes_gcm_test.golib/ocrypto/asym_decryption.golib/ocrypto/asym_encryption.go
Signed-off-by: strantalis <strantalis@virtru.com>
strantalis
changed the title
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: strantalis <strantalis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Signed-off-by: strantalis <strantalis@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
dmihalcik-virtru
changed the title
2 participants
Summary
Jira
Testing
go test ./...fromlib/ocryptogo test ./internal/security/...fromservicego test ./...fromsdkgo test ./...fromotdfctlgolangci-lint runfromlib/ocryptoGOFIPS140=v1.0.0 GODEBUG=fips140=only go test github.com/opentdf/platform/lib/ocrypto -run 'TestCreateAesGcm|TestNewAESGCM|TestDecrypt|TestAESProtectedKey'Notes
Summary by CodeRabbit
Refactor
Bug Fixes
Tests