-
Notifications
You must be signed in to change notification settings - Fork 5.5k
fix: Change encoding from HMAC to AES when setting secret key #26487
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: master
Are you sure you want to change the base?
fix: Change encoding from HMAC to AES when setting secret key #26487
Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideUpdates RefreshTokensConfig to interpret provided Base64-encoded secret keys as raw AES keys instead of HMAC-SHA keys, addressing failures in refresh token secret key property. Class diagram for updated RefreshTokensConfig secret key handlingclassDiagram
class RefreshTokensConfig {
-SecretKey secretKey
+RefreshTokensConfig setSecretKey(String key)
}
class SecretKeySpec {
+SecretKeySpec(byte[] key, String algorithm)
}
RefreshTokensConfig --> SecretKeySpec : uses for AES key
Flow diagram for secret key encoding change from HMAC to AESflowchart TD
A["Base64-encoded key input"] --> B["Decode key bytes"]
B --> C["Create SecretKeySpec with AES algorithm"]
C --> D["Assign to secretKey property"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
| } | ||
|
|
||
| secretKey = Keys.hmacShaKeyFor(Decoders.BASE64.decode(key)); | ||
| secretKey = new SecretKeySpec(Decoders.BASE64.decode(key), "AES"); |
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.
The corresponding Trino PR had a test, is it applicable here, and if so could you add it? trinodb/trino@3c687c3
yhwang
left a comment
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.
Just left a minor question, and the changes look good to me
presto-main/src/main/java/com/facebook/presto/server/security/oauth2/JweTokenSerializer.java
Show resolved
Hide resolved
yhwang
left a comment
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.
/LGTM
81df6d6 to
02d3235
Compare
02d3235 to
c6be714
Compare
Description
This is required to stop failures on the refresh token secret key property
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.