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

Improve error handling for JWKS with missing alg #3853

Closed
andyvajda opened this issue Sep 19, 2023 · 1 comment · Fixed by #3922
Closed

Improve error handling for JWKS with missing alg #3853

andyvajda opened this issue Sep 19, 2023 · 1 comment · Fixed by #3922
Assignees

Comments

@andyvajda
Copy link

Describe the bug
When a JWKS url is used in the router authentication config that includes keys without an "alg" field, none of the keys will be imported. Instead one, or both of the following errors will be thrown:

ERROR: could not get url
ERROR: could not create JWKS from url content

To Reproduce
Steps to reproduce the behavior:
Configure router with JWKS

Expected behavior
Error response indicates why JWKS was unable to load.
Possibly skip loading the offending keys while loading the others.

@flyboarder

garypen added a commit that referenced this issue Sep 27, 2023
There are JWKS out there which contain keys which have algorithms which
aren't supported by the router. We exclude any such known keys from
processing to allow the rest of the JWKS to be used.

Currently we exclude alg of ES512

fixes: #3853
@garypen
Copy link
Contributor

garypen commented Sep 28, 2023

I don't think the problem is because an alg field is missing from the JWKS entries, but because an alg field is specified which isn't supported by our underlying jsonwebtoken crate. For example: ES512

I've gone with an approach, #3922, which simply removes entries with unsupported algorithms from the JWKS set when we retrieve it.

You can still use the algorithms configuration field to further restrict the set of algorithms if you wish.

garypen added a commit that referenced this issue Sep 29, 2023
If you have a JWKS which contains a key which has an algorithm (alg)
which the router doesn't recognise, then the entire JWKS is disregarded.
This is unsatisfactory, since there are likely to be many other keys in
the JWKS which the router could use.

We have changed the JWKS processing logic so that we remove entries with
an unrecognised algorithm from the list of available keys. We print a
warning with the name of the algorithm for each removed entry.

fixes: #3853

<!-- start metadata -->
---

**Checklist**

Complete the checklist (and note appropriate exceptions) before the PR
is marked ready-for-review.

- [x] Changes are compatible[^1]
- [x] Documentation[^2] completed
- [x] Performance impact assessed and acceptable
- Tests added and passing[^3]
    - [x] Unit Tests
    - [x] Manual Tests

**Exceptions**

I manually tested with a set of keys which are known to contain alg
ES512 entries. I'll add some unit tests before I merge the PR if this
approach is agreed to be acceptable.

**Notes**

[^1]: It may be appropriate to bring upcoming changes to the attention
of other (impacted) groups. Please endeavour to do this before seeking
PR approval. The mechanism for doing this will vary considerably, so use
your judgement as to how and when to do this.
[^2]: Configuration is an important part of many changes. Where
applicable please try to document configuration examples.
[^3]: Tick whichever testing boxes are applicable. If you are adding
Manual Tests, please document the manual testing (extensively) in the
Exceptions.
This was referenced Oct 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants