Skip to content

Commit

Permalink
Add support for multiple IDP URLs (#2500)
Browse files Browse the repository at this point in the history
IMPORTANT: Merging this PR without resolving the "alg" question below.
The "alg" question will potentially be resolved by a follow-up PR.

- Fix #912

This modifies the experimental JWT authentication support so that
multiple JWKS urls are now supported.

The implementation is draft; pending a review and feedback with product.

In addition to adding multiple JWKS we need to resolve the following
issue:

### Not all JWKS entries contain "alg".

Currently, if the router can't find "alg" in the JWKS. Then it will fail
the request. The JWKS spec notes that "alg" is optional, but if we don't
know what the "alg" is, then we can't decode the JWT.

Here are some alternatives:
-  Preserve the existing behaviour (i.e. fail if "alg" isn't specified)
- Look for "alg" in the JWT header and use that value if not found in
JWKSs
-  Allow "alg" (per IDP) to be specified in configuration either as
    -  A fallback or
    -  An override

Which of the above would users prefer?

**Checklist**

Complete the checklist (and note appropriate exceptions) before a final
PR is raised.

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

**Exceptions**

The change is to experimental configuration and not compatible.

**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.
- please raise a separate issue to automate the test and label it (or
ask for it to be labeled) as `manual test`
  • Loading branch information
garypen authored Jan 31, 2023
1 parent bbec627 commit 3ee3d4b
Show file tree
Hide file tree
Showing 6 changed files with 199 additions and 117 deletions.
19 changes: 19 additions & 0 deletions NEXT_CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -261,3 +261,22 @@ By [@SimonSapin](https://github.com/SimonSapin) in https://github.com/apollograp
The `orbiter::test::test_visit_args` tests were failing in the event that `APOLLO_TELEMETRY_DISABLED` was set, however this is now corrected.

By [@bryncooke](https://github.com/bryncooke) in https://github.com/apollographql/router/pull/2488

## 🥼 Experimental

### JWT authentication ([Issue #912](https://github.com/apollographql/router/issues/912))

As a result of UX feedback, we are modifying the experimental JWT configuration. The `jwks_url` parameter is renamed to `jwks_urls` and now expects to receive an array of URLs, rather than a single URL.

Here's a typical sample configuration fragment:

```yaml
authentication:
experimental:
jwt:
jwks_urls:
- https://dev-zzp5enui.us.auth0.com/.well-known/jwks.json
```

By [@garypen](https://github.com/garypen) in https://github.com/apollographql/router/pull/2500

Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ expression: "&schema"
"description": "The JWT configuration",
"type": "object",
"required": [
"jwks_url"
"jwks_urls"
],
"properties": {
"cooldown": {
Expand All @@ -44,9 +44,12 @@ expression: "&schema"
"default": "Bearer",
"type": "string"
},
"jwks_url": {
"description": "Retrieve our JWK Set from here",
"type": "string"
"jwks_urls": {
"description": "Retrieve our JWK Sets from these locations",
"type": "array",
"items": {
"type": "string"
}
}
}
}
Expand Down
Loading

0 comments on commit 3ee3d4b

Please sign in to comment.