Skip to content
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

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

Merged
merged 6 commits into from
Aug 11, 2023

Conversation

alnr
Copy link
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
Copy link
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
Member

aeneasr commented Aug 11, 2023

test fail

@codecov
Copy link

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
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
Labels
bug Something is not working.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants