Skip to content

🐛 bug: Fix FIPS-140 compliance for EncryptCookie middleware#3955

Merged
ReneWerner87 merged 3 commits into
mainfrom
update-encryptmiddleware-to-use-newgcmwithrandomnonce
Dec 21, 2025
Merged

🐛 bug: Fix FIPS-140 compliance for EncryptCookie middleware#3955
ReneWerner87 merged 3 commits into
mainfrom
update-encryptmiddleware-to-use-newgcmwithrandomnonce

Conversation

@gaby
Copy link
Copy Markdown
Member

@gaby gaby commented Dec 20, 2025

Summary

  • switch encryptcookie to AES-GCM with random nonces using cipher.NewGCMWithRandomNonce
  • keep ciphertext length validation aligned with the middleware decrypt path and its test coverage

Fixes #3953

Note:

  • Backporting this to v2 requires golang 1.25

Copilot AI review requested due to automatic review settings December 20, 2025 13:45
@gaby gaby requested a review from a team as a code owner December 20, 2025 13:45
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Dec 20, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Replaces manual AES-GCM nonce handling in the encryptcookie middleware: uses cipher.NewGCMWithRandomNonce instead of cipher.NewGCM, removes manual nonce generation/IO usage, and updates ciphertext-length validation to use gcm.Overhead().

Changes

Cohort / File(s) Summary
GCM nonce handling update
middleware/encryptcookie/utils.go
Replaced cipher.NewGCM(block) with cipher.NewGCMWithRandomNonce(block); removed manual nonce creation/reading and passing to gcm.Seal/gcm.Open; adjusted ciphertext length checks to use gcm.Overhead(); removed io import.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

  • Verify correct use of NewGCMWithRandomNonce in FIPS modes.
  • Confirm ciphertext length/overhead validation matches intended semantics.
  • Sanity-check absence of explicit nonce storage/transmission where previously expected.

Possibly related PRs

Suggested reviewers

  • sixcolors
  • ReneWerner87
  • efectn

Poem

🐰
I hopped through code with careful paw,
Replaced the nonce—the rules I saw.
Now randomness hides what once was passed,
Cookies safe, and crashes past.
Hop, hop, secure and fast!

Pre-merge checks and finishing touches

❌ Failed checks (1 inconclusive)
Check name Status Explanation Resolution
Description check ❓ Inconclusive The PR description provides a concise summary of changes and links to the relevant issue, but lacks details on testing, documentation updates, and does not follow the provided template structure with checklist items. Complete the PR description by filling in the template sections: mark relevant change type checkboxes, add testing details, confirm documentation updates, and verify all checklist items are addressed.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly and specifically identifies the main change: fixing FIPS-140 compliance in the EncryptCookie middleware, which is the primary objective of the changeset.
Linked Issues check ✅ Passed The changes directly address issue #3953 by replacing cipher.NewGCM with cipher.NewGCMWithRandomNonce, eliminating the FIPS-140 compliance crash when running in FIPS-only mode.
Out of Scope Changes check ✅ Passed All changes are narrowly scoped to replacing the GCM cipher initialization with the FIPS-compliant variant and adjusting validation accordingly, with no unrelated modifications detected.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch update-encryptmiddleware-to-use-newgcmwithrandomnonce

📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4b72bbc and cc379a1.

📒 Files selected for processing (1)
  • middleware/encryptcookie/utils.go (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • middleware/encryptcookie/utils.go
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
  • GitHub Check: Compare
  • GitHub Check: unit (1.25.x, macos-latest)
  • GitHub Check: unit (1.25.x, windows-latest)
  • GitHub Check: repeated
  • GitHub Check: lint

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Summary of Changes

Hello @gaby, 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 significantly enhances the security of the cookie encryption middleware by implementing cryptographically secure random nonces for AES-GCM. By transitioning to cipher.NewGCMWithRandomNonce, the system now automatically manages nonce generation and embedding, streamlining the encryption and decryption processes while bolstering the confidentiality and integrity of sensitive cookie data. This change addresses a potential vulnerability and aligns the implementation with best practices for modern cryptography.

Highlights

  • Enhanced Security: The cookie encryption middleware now utilizes cipher.NewGCMWithRandomNonce for AES-GCM, ensuring cryptographically secure random nonce generation for all encrypted cookies.
  • Simplified Nonce Management: The encryption and decryption functions have been refactored to remove manual nonce generation, prepending, and extraction, as NewGCMWithRandomNonce handles these operations internally.
  • Updated Validation Logic: Ciphertext length validation in the DecryptCookie function has been adjusted to use gcm.Overhead(), which correctly accounts for the authentication tag and embedded nonce, aligning with the new encryption mechanism.

🧠 New Feature in Public Preview: You can now enable Memory 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. 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.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@ReneWerner87 ReneWerner87 added this to v3 Dec 20, 2025
@ReneWerner87 ReneWerner87 added this to the v3 milestone Dec 20, 2025
@gaby gaby changed the title 🐛 bug: Use random nonce for encrypt cookie middleware 🐛 bug: Fix FIPS-140 compliance for EncryptCookie middleware Dec 20, 2025
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request correctly refactors the cookie encryption middleware to use AES-GCM with random nonces by switching to cipher.NewGCMWithRandomNonce. This is a significant security improvement that prevents nonce reuse vulnerabilities and simplifies the code by removing manual nonce generation. The changes to both EncryptCookie and DecryptCookie functions are implemented correctly. A notable improvement is the more robust ciphertext length validation in DecryptCookie, which fixes a subtle bug in the previous implementation. Overall, these changes enhance security and correctness, and the implementation is excellent.

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 20, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.58%. Comparing base (6808448) to head (cc379a1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3955      +/-   ##
==========================================
- Coverage   91.62%   91.58%   -0.05%     
==========================================
  Files         119      119              
  Lines       10190    10185       -5     
==========================================
- Hits         9337     9328       -9     
- Misses        540      544       +4     
  Partials      313      313              
Flag Coverage Δ
unittests 91.58% <100.00%> (-0.05%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR modernizes the encrypt cookie middleware by switching from manual nonce generation to using cipher.NewGCMWithRandomNonce, which appears to be a new Go standard library feature for AES-GCM encryption with automatic random nonce management.

Key Changes

  • Replaced manual nonce generation with cipher.NewGCMWithRandomNonce for both encryption and decryption
  • Removed the io package import as io.ReadFull is no longer needed
  • Updated the ciphertext length validation from NonceSize() to Overhead()

Comment thread middleware/encryptcookie/utils.go Outdated
gaby and others added 2 commits December 20, 2025 12:25
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@gaby gaby requested a review from Copilot December 21, 2025 16:17
@gaby
Copy link
Copy Markdown
Member Author

gaby commented Dec 21, 2025

/gemini review

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request effectively addresses FIPS-140 compliance for the EncryptCookie middleware by transitioning to cipher.NewGCMWithRandomNonce. This change correctly enforces the use of random nonces for each encryption, a critical security practice that prevents nonce reuse vulnerabilities. The implementation correctly adapts the Seal and Open calls for the new AEAD interface. Additionally, the change improves the robustness of the decryption process by correcting the ciphertext length validation, which now properly accounts for both the nonce size and GCM overhead. This prevents potential panics with malformed inputs and provides more accurate errors. Overall, these changes significantly enhance the security and reliability of the cookie encryption mechanism.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

@ReneWerner87 ReneWerner87 merged commit a331406 into main Dec 21, 2025
24 of 25 checks passed
@ReneWerner87 ReneWerner87 deleted the update-encryptmiddleware-to-use-newgcmwithrandomnonce branch December 21, 2025 17:58
@github-project-automation github-project-automation Bot moved this to Done in v3 Dec 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

🐛 [Bug]: EncryptCookie Middleware Crashes in FIPS-140 Mode

4 participants