perf(oidc): remove get user by ID from jwt profile grant by muhlemmer · Pull Request #8580 · zitadel/zitadel

muhlemmer · 2024-09-09T13:39:57Z

Which Problems Are Solved

Improve performance by removing a GetUserByID call. The call also executed a Trigger on projections, which significantly impacted concurrent requests.

How the Problems Are Solved

Token creation needs information from the user, such as the resource owner and access token type.

For client credentials this is solved in a single search. By getting the user by username (client_id), the user details and secret were obtained in a single query. After that verification and token creation can proceed. For JWT profile it is a bit more complex. We didn't know anything about the user until after JWT verification.
The verification did a query for the AuthN key and after that we did a GetUserByID to get remaining details.

This change uses a joined query when the OIDC library calls the GetKeyByIDAndClientID method on the token storage. The found user details are set to the verifieer object and returned after verification is completed.
It is safe because the jwtProfileKeyStorage is a single-use object as a wrapper around query.Queries.
This way getting the public key and user details are obtained in a single query.

Additional Changes

Correctly set the client_id field with machine's username.

Additional Context

Related to: Machine account authentications performance #8352

vercel · 2024-09-09T13:40:02Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 11, 2024 8:45am

github-actions · 2024-09-09T13:40:10Z

Thanks for your contribution @muhlemmer! 🎉

Please make sure you tick the following checkboxes before marking this Pull Request (PR) as ready for review:

I am happy with the code
Documentations and examples are up-to-date
Logical behavior changes are tested automatically
No debug or dead code
My code has no repetitions
The PR title adheres to the conventional commit format
The example texts in the PR description are replaced.
If there are any open TODOs or follow-ups, they are described in issues and link to this PR
If there are deviations from a user stories acceptance criteria or design, they are agreed upon with the PO and documented.

codecov · 2024-09-09T13:46:19Z

Codecov Report

Attention: Patch coverage is 97.95918% with 1 line in your changes missing coverage. Please review.

Project coverage is 63.87%. Comparing base (3aba942) to head (6f82260).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/api/oidc/token_jwt_profile.go	94.44%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #8580       +/-   ##
===========================================
+ Coverage   47.63%   63.87%   +16.24%     
===========================================
  Files        1424     1492       +68     
  Lines      118060   120681     +2621     
===========================================
+ Hits        56237    77091    +20854     
+ Misses      58798    39369    -19429     
- Partials     3025     4221     +1196

Flag	Coverage Δ
core-integration-tests-postgres	`37.26% <89.79%> (?)`
core-unit-tests	`47.64% <36.73%> (+<0.01%)`	⬆️

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.

muhlemmer · 2024-09-10T09:19:00Z

Local load test on main branch

     checks...............................: 100.00% ✓ 131116     ✗ 0    
     data_received........................: 87 MB   435 kB/s
     data_sent............................: 56 MB   281 kB/s
     http_req_blocked.....................: min=942ns    avg=2.64µs  max=465.48µs p(50)=2.27µs  p(95)=4.18µs   p(99)=6.81µs  
     http_req_connecting..................: min=0s       avg=74ns    max=412.83µs p(50)=0s      p(95)=0s       p(99)=0s      
     http_req_duration....................: min=1.01ms   avg=44.84ms max=996.83ms p(50)=16.17ms p(95)=91.47ms  p(99)=100.04ms
       { expected_response:true }.........: min=1.01ms   avg=44.84ms max=996.83ms p(50)=16.17ms p(95)=91.47ms  p(99)=100.04ms
     http_req_failed......................: 0.00%   ✓ 0          ✗ 87452
     http_req_receiving...................: min=14.93µs  avg=42.45µs max=3.77ms   p(50)=38.41µs p(95)=68.41µs  p(99)=98.17µs 
     http_req_sending.....................: min=6.05µs   avg=15.2µs  max=695.93µs p(50)=13.9µs  p(95)=24.06µs  p(99)=36.39µs 
     http_req_tls_handshaking.............: min=0s       avg=0s      max=0s       p(50)=0s      p(95)=0s       p(99)=0s      
     http_req_waiting.....................: min=943.78µs avg=44.78ms max=996.75ms p(50)=16.11ms p(95)=91.41ms  p(99)=99.97ms 
     http_reqs............................: 87452   435.741697/s
     iteration_duration...................: min=38.92ms  avg=91.57ms max=1s       p(50)=89.23ms p(95)=100.91ms p(99)=112.15ms
     iterations...........................: 43681   217.64663/s
     login_ui_enter_login_name_duration...: min=29.97ms  avg=29.97ms max=29.97ms  p(50)=29.97ms p(95)=29.97ms  p(99)=29.97ms 
     login_ui_enter_password_duration.....: min=1.01ms   avg=1.01ms  max=1.01ms   p(50)=1.01ms  p(95)=1.01ms   p(99)=1.01ms  
     login_ui_init_login_duration.........: min=13.49ms  avg=13.49ms max=13.49ms  p(50)=13.49ms p(95)=13.49ms  p(99)=13.49ms 
     login_ui_token_duration..............: min=15.12ms  avg=15.12ms max=15.12ms  p(50)=15.12ms p(95)=15.12ms  p(99)=15.12ms 
     oidc_token_duration..................: min=22.87ms  avg=86.25ms max=996.83ms p(50)=84.08ms p(95)=95.11ms  p(99)=105.47ms
     oidc_user_info_duration..............: min=1.44ms   avg=3.46ms  max=16.03ms  p(50)=3.43ms  p(95)=5.04ms   p(99)=6.33ms  
     org_create_org_duration..............: min=16.69ms  avg=16.69ms max=16.69ms  p(50)=16.69ms p(95)=16.69ms  p(99)=16.69ms 
     user_add_machine_key_duration........: min=10.98ms  avg=36.67ms max=62.38ms  p(50)=36.69ms p(95)=59.51ms  p(99)=61.81ms 
     user_create_machine_duration.........: min=16.59ms  avg=56.21ms max=101.04ms p(50)=53.35ms p(95)=96.38ms  p(99)=100.11ms
     vus..................................: 20      min=20       max=20 
     vus_max..............................: 20      min=20       max=20 


running (3m20.7s), 00/20 VUs, 43681 complete and 0 interrupted iterations
default ✓ [======================================] 20 VUs  3m20s

muhlemmer · 2024-09-10T09:28:02Z

On this branch

     checks...............................: 100.00% ✓ 353761      ✗ 0     
     data_received........................: 233 MB  1.2 MB/s
     data_sent............................: 152 MB  756 kB/s
     http_req_blocked.....................: min=872ns    avg=3.29µs   max=4.06ms   p(50)=2.92µs   p(95)=4.43µs   p(99)=7.58µs  
     http_req_connecting..................: min=0s       avg=28ns     max=516.06µs p(50)=0s       p(95)=0s       p(99)=0s      
     http_req_duration....................: min=827.37µs avg=15.69ms  max=611.98ms p(50)=14.42ms  p(95)=26.59ms  p(99)=34.13ms 
       { expected_response:true }.........: min=827.37µs avg=15.69ms  max=611.98ms p(50)=14.42ms  p(95)=26.59ms  p(99)=34.13ms 
     http_req_failed......................: 0.00%   ✓ 0           ✗ 235882
     http_req_receiving...................: min=12.83µs  avg=50.92µs  max=6.31ms   p(50)=41.54µs  p(95)=73.14µs  p(99)=189.45µs
     http_req_sending.....................: min=5.66µs   avg=18.72µs  max=16.24ms  p(50)=15.8µs   p(95)=25.09µs  p(99)=40.81µs 
     http_req_tls_handshaking.............: min=0s       avg=0s       max=0s       p(50)=0s       p(95)=0s       p(99)=0s      
     http_req_waiting.....................: min=760.54µs avg=15.62ms  max=611.93ms p(50)=14.35ms  p(95)=26.51ms  p(99)=34.02ms 
     http_reqs............................: 235882  1175.417592/s
     iteration_duration...................: min=15.12ms  avg=33.91ms  max=620.77ms p(50)=32.79ms  p(95)=44.82ms  p(99)=112.93ms
     iterations...........................: 117896  587.484558/s
     login_ui_enter_login_name_duration...: min=41.4ms   avg=41.4ms   max=41.4ms   p(50)=41.4ms   p(95)=41.4ms   p(99)=41.4ms  
     login_ui_enter_password_duration.....: min=827.37µs avg=827.37µs max=827.37µs p(50)=827.37µs p(95)=827.37µs p(99)=827.37µs
     login_ui_init_login_duration.........: min=16.19ms  avg=16.19ms  max=16.19ms  p(50)=16.19ms  p(95)=16.19ms  p(99)=16.19ms 
     login_ui_token_duration..............: min=19.78ms  avg=19.78ms  max=19.78ms  p(50)=19.78ms  p(95)=19.78ms  p(99)=19.78ms 
     oidc_token_duration..................: min=8.39ms   avg=20.62ms  max=611.98ms p(50)=18.92ms  p(95)=28.85ms  p(99)=103.51ms
     oidc_user_info_duration..............: min=2.61ms   avg=10.74ms  max=75.58ms  p(50)=10.03ms  p(95)=18.45ms  p(99)=22.68ms 
     org_create_org_duration..............: min=17.26ms  avg=17.26ms  max=17.26ms  p(50)=17.26ms  p(95)=17.26ms  p(99)=17.26ms 
     user_add_machine_key_duration........: min=10.77ms  avg=37.77ms  max=64.98ms  p(50)=35.62ms  p(95)=62.95ms  p(99)=64.57ms 
     user_create_machine_duration.........: min=21.41ms  avg=59.12ms  max=98ms     p(50)=57.77ms  p(95)=95.48ms  p(99)=97.5ms  
     vus..................................: 20      min=20        max=20  
     vus_max..............................: 20      min=20        max=20  


running (3m20.7s), 00/20 VUs, 117896 complete and 0 interrupted iterations
default ✓ [======================================] 20 VUs  3m20s

That's a 2.6x improvement. Note that at this stage k6, zitadel and postgres are competing for resources on my system. A load test in a deployment might give better numbers.

adlerhurst · 2024-09-11T08:44:26Z

just saw the benchmarks great job 🥳

fforootd · 2024-09-11T08:58:53Z

Oh wow, great job!

# Which Problems Are Solved Calls with tokens issued through JWT Profile or Client Credentials Grants were no longer possible and threw a "could not read projectid by clientid (AUTH-GHpw2)" error. ZITADEL checks the allowed origins of an application and load its projectID into the context on any API call. Tokens from service accounts did not contain any clientID and therefore never did that check. But due to a change in #8580, were the service user id was set as client_id in the OIDC session to fix the introspection response (#8590). # How the Problems Are Solved - Check if the project and origin were retrieved and only then check the origins # Additional Changes None. # Additional Context - closes #8676 - relates to #8580 (released on 2.62.0) - relates to #8590

# Which Problems Are Solved Calls with tokens issued through JWT Profile or Client Credentials Grants were no longer possible and threw a "could not read projectid by clientid (AUTH-GHpw2)" error. ZITADEL checks the allowed origins of an application and load its projectID into the context on any API call. Tokens from service accounts did not contain any clientID and therefore never did that check. But due to a change in #8580, were the service user id was set as client_id in the OIDC session to fix the introspection response (#8590). # How the Problems Are Solved - Check if the project and origin were retrieved and only then check the origins # Additional Changes None. # Additional Context - closes #8676 - relates to #8580 (released on 2.62.0) - relates to #8590 (cherry picked from commit c347e75)

…8704) # Which Problems Are Solved Calls with tokens issued through JWT Profile or Client Credentials Grants were no longer possible and threw a "could not read projectid by clientid (AUTH-GHpw2)" error. ZITADEL checks the allowed origins of an application and load its projectID into the context on any API call. Tokens from service accounts did not contain any clientID and therefore never did that check. But due to a change in zitadel#8580, were the service user id was set as client_id in the OIDC session to fix the introspection response (zitadel#8590). # How the Problems Are Solved - Check if the project and origin were retrieved and only then check the origins # Additional Changes None. # Additional Context - closes zitadel#8676 - relates to zitadel#8580 (released on 2.62.0) - relates to zitadel#8590 (cherry picked from commit dd1b2a6)

perf(oidc): remove get user by ID from jwt profile grant

e1cd7c0

muhlemmer requested a review from adlerhurst September 10, 2024 09:28

muhlemmer marked this pull request as ready for review September 10, 2024 09:28

adlerhurst approved these changes Sep 10, 2024

View reviewed changes

Merge branch 'main' into perf-jwtProfile-user

565b975

vercel Bot deployed to Preview September 11, 2024 08:04 View deployment

Merge branch 'main' into perf-jwtProfile-user

6f82260

vercel Bot deployed to Preview September 11, 2024 08:45 View deployment

muhlemmer merged commit 58a7eb1 into main Sep 11, 2024

muhlemmer deleted the perf-jwtProfile-user branch September 11, 2024 09:04

muhlemmer mentioned this pull request Sep 11, 2024

[Bug]: Introspection Response is missing client_id #8590

Closed

2 tasks done

livio-a mentioned this pull request Oct 1, 2024

fix: ignore projectID and origin check for service accounts #8704

Merged

Navigation Menu

Repository navigation

perf(oidc): remove get user by ID from jwt profile grant - #8580

perf(oidc): remove get user by ID from jwt profile grant#8580
muhlemmer merged 3 commits into
mainfrom
perf-jwtProfile-user

muhlemmer commented Sep 9, 2024

Uh oh!

vercel Bot commented Sep 9, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented Sep 9, 2024 •

edited by muhlemmer

Loading

Uh oh!

codecov Bot commented Sep 9, 2024 •

edited

Loading

Uh oh!

muhlemmer commented Sep 10, 2024

Uh oh!

muhlemmer commented Sep 10, 2024

Uh oh!

adlerhurst commented Sep 11, 2024

Uh oh!

fforootd commented Sep 11, 2024

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

muhlemmer commented Sep 9, 2024

Which Problems Are Solved

How the Problems Are Solved

Additional Changes

Additional Context

Uh oh!

vercel Bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Sep 9, 2024 • edited by muhlemmer Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Thanks for your contribution @muhlemmer! 🎉

Uh oh!

codecov Bot commented Sep 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

muhlemmer commented Sep 10, 2024

Uh oh!

muhlemmer commented Sep 10, 2024

Uh oh!

adlerhurst commented Sep 11, 2024

Uh oh!

fforootd commented Sep 11, 2024

Uh oh!

This branch was successfully deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vercel Bot commented Sep 9, 2024 •

edited

Loading

github-actions Bot commented Sep 9, 2024 •

edited by muhlemmer

Loading

codecov Bot commented Sep 9, 2024 •

edited

Loading