-
Notifications
You must be signed in to change notification settings - Fork 34
Optional parameter to choose what algorithm to seal the session #430
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Optional parameter to choose what algorithm to seal the session #430
Conversation
Greptile OverviewGreptile SummaryAdded optional
Note: The Iron algorithm uses PBKDF2 with only 1 iteration and SHA1 (inherent to Fe26.2 spec), which is cryptographically weaker than modern standards but required for cross-platform compatibility. Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant RubyAPI as Ruby API (WorkOS)
participant NextJS as Next.js App
Note over Client,NextJS: Cross-Platform Session Unsealing
NextJS->>NextJS: seal_data(session, password, algorithm: :iron)
NextJS->>Client: Set cookie with Iron-sealed session
Client->>RubyAPI: Request with Iron-sealed cookie
RubyAPI->>RubyAPI: load_sealed_session(seal_algorithm: :iron)
RubyAPI->>RubyAPI: Session.unseal_data(sealed, key, algorithm: :iron)
RubyAPI->>RubyAPI: IronSealUnseal.unseal(sealed, password)
RubyAPI->>RubyAPI: Verify HMAC, decrypt AES-256-CBC, parse JSON
RubyAPI->>RubyAPI: authenticate() - validate access token
RubyAPI->>Client: Authenticated response
Note over Client,RubyAPI: Session Refresh Flow
Client->>RubyAPI: refresh() request
RubyAPI->>RubyAPI: Session.unseal_data(algorithm: @seal_algorithm)
RubyAPI->>RubyAPI: authenticate_with_refresh_token(session_opts)
RubyAPI->>RubyAPI: seal_data(new_tokens, algorithm: :iron)
RubyAPI->>Client: New sealed session (Iron format)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 2 comments
| def derive_key(password, salt, key_length) | ||
| OpenSSL::KDF.pbkdf2_hmac( | ||
| password, | ||
| salt: salt, | ||
| iterations: 1, | ||
| length: key_length, | ||
| hash: OpenSSL::Digest.new('SHA1'), | ||
| ) | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PBKDF2 with only 1 iteration is cryptographically weak
| def derive_key(password, salt, key_length) | |
| OpenSSL::KDF.pbkdf2_hmac( | |
| password, | |
| salt: salt, | |
| iterations: 1, | |
| length: key_length, | |
| hash: OpenSSL::Digest.new('SHA1'), | |
| ) | |
| end | |
| def derive_key(password, salt, key_length) | |
| OpenSSL::KDF.pbkdf2_hmac( | |
| password, | |
| salt: salt, | |
| iterations: 100000, | |
| length: key_length, | |
| hash: OpenSSL::Digest.new('SHA256'), | |
| ) | |
| end |
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/workos/iron_seal_unseal.rb
Line: 96:104
Comment:
PBKDF2 with only 1 iteration is cryptographically weak
```suggestion
def derive_key(password, salt, key_length)
OpenSSL::KDF.pbkdf2_hmac(
password,
salt: salt,
iterations: 100000,
length: key_length,
hash: OpenSSL::Digest.new('SHA256'),
)
end
```
How can I resolve this? If you propose a fix, please make it concise.| describe '.seal_data and .unseal_data' do | ||
| it 'round-trips with default algorithm (aes_gcm)' do | ||
| data = { access_token: 'tok', user: 'u1' } | ||
| sealed = WorkOS::Session.seal_data(data, cookie_password) | ||
| expect(sealed).not_to eq(data) | ||
| unsealed = WorkOS::Session.unseal_data(sealed, cookie_password) | ||
| expect(unsealed).to eq(data) | ||
| end | ||
|
|
||
| it 'round-trips with algorithm: :iron' do | ||
| data = { access_token: 'tok', user: 'u1' } | ||
| sealed = WorkOS::Session.seal_data(data, cookie_password, algorithm: :iron) | ||
| expect(sealed).to start_with(WorkOS::IronSealUnseal::MAC_PREFIX) | ||
| unsealed = WorkOS::Session.unseal_data(sealed, cookie_password, algorithm: :iron) | ||
| expect(unsealed).to eq(data) | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing test cases for algorithm mismatches
Add tests to verify error handling when unsealing data sealed with one algorithm using a different algorithm (e.g., seal with :iron, unseal with :aes_gcm)
Prompt To Fix With AI
This is a comment left during a code review.
Path: spec/lib/workos/session_spec.rb
Line: 325:341
Comment:
Missing test cases for algorithm mismatches
Add tests to verify error handling when unsealing data sealed with one algorithm using a different algorithm (e.g., seal with `:iron`, unseal with `:aes_gcm`)
How can I resolve this? If you propose a fix, please make it concise.
Description
We ran into an issue trying to unseal sessions that were generated in next.js in our api. Added an optional algorithm parameter to unseal the sessions from different environmemts
Documentation
Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.
If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.