Lokasi ngalangkungan proxy:   [ UP ]  
[Ngawartoskeun bug]   [Panyetelan cookie]                
Skip to content

fix: reject invalid JWKS in client configuration / dependency cleanup and bump#3603

Merged
aeneasr merged 6 commits into
masterfrom
fix-500-invalid-jwks
Aug 11, 2023
Merged

fix: reject invalid JWKS in client configuration / dependency cleanup and bump#3603
aeneasr merged 6 commits into
masterfrom
fix-500-invalid-jwks

Conversation

@alnr

@alnr alnr commented Aug 10, 2023

Copy link
Copy Markdown
Contributor
  1. bumped dependencies, cleaned up go.mod, and fixed some lint issues.
  2. fixed two issues of missing context passing, leading to missing timeouts/cancelation and gaps in tracing
  3. fixed an issue where we would accept a malformed JWKS in the OAuth client configuration (on creation or update). Once in the database, it can't be correctly deserialzed anymore. That bricks the complete GET /admin/clients endpoint.

Users encountering this issue are advised to fix up offending JWKS keys in their database manually. A good start point for a query is

select id, jwks from hydra_client where jwks <> '{}';

From there, inspect the jwks column and scan for invalid keys. Those should be replaced by {}.

@alnr alnr requested a review from aeneasr as a code owner August 10, 2023 12:00
@alnr alnr self-assigned this Aug 10, 2023
@alnr alnr added the bug Something is not working. label Aug 10, 2023
@alnr alnr force-pushed the fix-500-invalid-jwks branch from fa39be5 to 0ff8849 Compare August 10, 2023 13:38
aeneasr
aeneasr previously approved these changes Aug 11, 2023

@aeneasr aeneasr left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@aeneasr

aeneasr commented Aug 11, 2023

Copy link
Copy Markdown
Member

test fail

@codecov

codecov Bot commented Aug 11, 2023

Copy link
Copy Markdown

Codecov Report

Merging #3603 (bf51c63) into master (5704640) will decrease coverage by 0.06%.
The diff coverage is 58.33%.

❗ Current head bf51c63 differs from pull request most recent head 13f3ec2. Consider uploading reports for the commit 13f3ec2 to get more accurate results

@@            Coverage Diff             @@
##           master    #3603      +/-   ##
==========================================
- Coverage   76.29%   76.24%   -0.06%     
==========================================
  Files         132      132              
  Lines        9974     9930      -44     
==========================================
- Hits         7610     7571      -39     
+ Misses       1845     1842       -3     
+ Partials      519      517       -2     
Files Changed Coverage Δ
jwk/handler.go 68.68% <0.00%> (-0.71%) ⬇️
persistence/sql/persister_nonce.go 89.47% <ø> (+8.52%) ⬆️
client/handler.go 78.44% <50.00%> (ø)
consent/strategy_default.go 68.89% <70.00%> (+0.14%) ⬆️
client/validator.go 69.91% <75.00%> (+0.09%) ⬆️

... and 8 files with indirect coverage changes

@alnr

alnr commented Aug 11, 2023

Copy link
Copy Markdown
Contributor Author

test fail

fixed, pls check out @aeneasr

@aeneasr aeneasr merged commit 1d73d83 into master Aug 11, 2023
@aeneasr aeneasr deleted the fix-500-invalid-jwks branch August 11, 2023 14:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something is not working.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants