Skip to content

Ensuring that cookie sessions are reusable across nodes#2746

Merged
vietj merged 4 commits intomasterfrom
feature/cookie-store-fix
May 30, 2025
Merged

Ensuring that cookie sessions are reusable across nodes#2746
vietj merged 4 commits intomasterfrom
feature/cookie-store-fix

Conversation

@pmlopes
Copy link
Copy Markdown
Member

@pmlopes pmlopes commented May 22, 2025

Motivation:

Fix #2745

This PR changes the underlying algorithm for security reasons and fixes the test that IVs are stored in the cookie so they can be decripted on other nodes, making the statement that sessions can survive a server crash correct.

Regarding the algorithm:

Feature AES/CBC AES/GCM
Confidentiality Yes Yes
Data Integrity No (needs separate MAC) Yes (built-in authentication tag)
Padding Requirement Required (e.g., PKCS5Padding) Not required
Performance Slower (sequential encryption) Faster (parallelizable)
IV Requirements Unique IV required Unique IV critical (security failure if reused)
Security Against Vulnerable to padding oracle attacks Resists tampering attacks
Authentication Tag No Yes (128-bit tag by default)
Recommended By NIST No Yes
Use Cases Legacy systems, compatibility Modern apps, TLS 1.3, cloud security

With this update we move away from AES/CBC to the recommended (by NIST) AES/GCM

@pmlopes pmlopes requested a review from vietj May 23, 2025 10:34
@pmlopes
Copy link
Copy Markdown
Member Author

pmlopes commented May 23, 2025

@vietj please pick the right version for this issue.

  • API is backwards compatible, there is a new factory method and the existing one remains but ignores the last argument.
  • behavior has changed BUT the original issue was that cookies weren't interop between vert.x instances. This means that the change does not affect the previous behavior due to existing bug anyway.

If we want to be strict, then this is a 5.1.0, however, due to the security upgrade and bug fix, it may go on a 5.0.x release either.

@vietj
Copy link
Copy Markdown
Contributor

vietj commented May 23, 2025

I think it is fine to be in 5.0.x, please notice that now the master branch is 5.1.0-SNAPSHOT so it needs backport to the new 5.0 branch

@tsegismont
Copy link
Copy Markdown
Member

@pmlopes it seems there are formatting issues in tests.

About the deprecated method: I assume an existing cookie couldn't be processed after the upgrade, correct? In this case, it might be worth adding a note in the breaking changes page after your merge the PR.

Thanks for this contrib!

@pmlopes
Copy link
Copy Markdown
Member Author

pmlopes commented May 28, 2025

Notes added: https://github.com/vert-x3/wiki/wiki/5.1.0-Deprecations-and-breaking-changes

@vietj vietj added this to the 5.1.0 milestone May 28, 2025
@vietj
Copy link
Copy Markdown
Contributor

vietj commented May 28, 2025

@pmlopes I only need it for 5.0.1

@vietj vietj merged commit cb09b9f into master May 30, 2025
10 of 13 checks passed
@vietj vietj deleted the feature/cookie-store-fix branch May 30, 2025 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants