Fix JWE encryption with user provided secret key #18015
Conversation
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
2 times, most recently
from
7febda2 to
e0d4f71
Compare
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
from
e0d4f71 to
ef63c62
Compare
s2lomon
left a comment
s2lomon
left a comment
There was a problem hiding this comment.
I like this changes, but I was wondering whether we haven't decided that we prefer to not allow using weaker keys, where your pr adds support for this. Apart from that - lgtm.
core/trino-main/src/main/java/io/trino/server/security/oauth2/TokenPairSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/security/oauth2/JweTokenSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/oauth2/TestJweTokenSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/oauth2/TestJweTokenSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/test/java/io/trino/server/security/oauth2/TestJweTokenSerializer.java
Outdated
Show resolved
Hide resolved
core/trino-main/src/main/java/io/trino/server/security/oauth2/JweTokenSerializer.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
from
b5b3802 to
ab842c8
Compare
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
from
ab842c8 to
b1d8bb2
Compare
core/trino-main/src/main/java/io/trino/server/security/oauth2/JweTokenSerializer.java
Outdated
Show resolved
Hide resolved
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
from
b1d8bb2 to
8988a41
Compare
kokosing
left a comment
kokosing
left a comment
There was a problem hiding this comment.
Refactors should be before actual change. That way you don't have add a thing (hashCode/equals) in one commit and then in the second commit remove it.
|
@kokosing sure, I'll reorder commits |
This verifies key length and sets correct encryption method and JWE algorithm. Tests are added to ensure that the roundtrips with the user provided keys are covered and working as expected.
wendigo
force-pushed
the
serafin/fix-refresh-tokens
branch
from
8988a41 to
e156e01
Compare
|
@colebow yes. It fixes the ability to provide secret that’s used as symmetric key for JWE token encryption. 16, 24, 32 bytes keys are supported (which translates to 128, 178 and 256 bit encryption of the JWE token) |
|
User provided secret is important because on every cluster restart new key will be autogenerated when user is not providing it’s own. Then all of the sessions will be invalidated. |
|
@wendigo - we came across an issue with current implementation. Has anyone come across that issue? Is it a good idea to separate out these tokens into their own cookies? |
|
@sajjoseph This won't be a backward compatible change. Maybe splitting them across 4096 ranges is a solution? Adding more cookies and combining them when reading? |
|
@sajjoseph fixed |
|
Awesome! Thanks. This will help us to use the OAuth Refresh token flow. I will try it out and let you know the results. |
6 participants
Description
Additional context and related issues
Release notes
( ) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text: