Skip to content

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

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

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

Conversation

@alnr
Copy link
Copy Markdown
Contributor

@alnr alnr commented Aug 10, 2023

  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
View reviewed changes
Copy link
Copy Markdown
Member

@aeneasr aeneasr left a comment

Choose a reason for hiding this comment

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

LGTM

@aeneasr
Copy link
Copy Markdown
Member

aeneasr commented Aug 11, 2023

test fail

@alnr alnr force-pushed the fix-500-invalid-jwks branch from 0ff8849 to 13f3ec2 Compare August 11, 2023 13:09
@codecov
Copy link
Copy Markdown

codecov bot commented Aug 11, 2023

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
Copy link
Copy Markdown
Contributor Author

alnr commented Aug 11, 2023

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

Reviewers

@aeneasr aeneasr aeneasr left review comments

Assignees

@alnr alnr

Labels

bug Something is not working.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@alnr @aeneasr