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

Keycloak implementation of SPI #5

Merged
merged 31 commits into from
Sep 10, 2024
Merged

Keycloak implementation of SPI #5

merged 31 commits into from
Sep 10, 2024

Conversation

djmcaleese
Copy link
Member

@djmcaleese djmcaleese commented Apr 19, 2024

Note: Due to updates being done in the schema, the Cognito implementation must be updated as well. Due to the scope of this issue and me (@inFocus7) going on PTO soon, it will likely not be updated as part of this PR.


Adds a second implementation of the SPI, this one for Keycloak.

Since Keycloak supports User Managed Access (UMA), this implementation represents API products as resources and authorisation to them is granted to clients via permissions. This allows a standardised approach to gaining access to these resources through UMA, which differs from the scope-based approach used by the Cognito implementation and therefore requires a different enforcement method (which is provided in the documentation).

It would still be possible to implement permissions via scopes if customers wished to, since they would own the ultimate implementation of the SPI, but the approach taken here shows an implementation aligned to UMA should that be preferred.

Tested with Keycloak 24.0.3.

@inFocus7 inFocus7 changed the title Keycloak implementation of SPI [WIP, DNM] Keycloak implementation of SPI Sep 3, 2024
@inFocus7 inFocus7 changed the title [WIP, DNM] Keycloak implementation of SPI Keycloak implementation of SPI Sep 6, 2024
Copy link
Contributor

@SirNexus SirNexus left a comment

Choose a reason for hiding this comment

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

Looks good, just some comments/suggestions

docs/usage.md Show resolved Hide resolved
docs/configuring-gloo-platform.md Outdated Show resolved Hide resolved
docs/configuring-gloo-platform.md Outdated Show resolved Hide resolved
docs/configuring-gloo-platform.md Outdated Show resolved Hide resolved
docs/configuring-gloo-platform.md Outdated Show resolved Hide resolved
internal/keycloak/server/handler.go Outdated Show resolved Hide resolved
internal/keycloak/server/server.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jmhbh jmhbh left a comment

Choose a reason for hiding this comment

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

mostly nits that we can follow up in the future

{
"sub": "s2ai3kk4j6vfun6po9747bi1g",
"token_use": "access",
"scope": "access/catstronauts-api tracks-rest-api",
Copy link
Contributor

Choose a reason for hiding this comment

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

access/tracks-rest-api

In Gloo Gateway, your RouteTable defines your API Product. Below is an example of such an API Product for the Tracks API:

```yaml
apiVersion: networking.gloo.solo.io/v2
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add a TODO here to update this to use the GGv2 API instead of the GME API

Copy link
Contributor

Choose a reason for hiding this comment

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

#11

Finally, we can apply the ExtAuthPolicy to our ApiProduct route(s) that performs JWT validation using the Cognito’s JSON Web Key Set (JWKS), which contains the public keys used to verify the validity of the token, and applies to OPA policy to perform Authorization checks:

```yaml
apiVersion: security.policy.gloo.solo.io/v2
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above we should use route route options and the AuthConfig API instead of ExtauthPolicy

@inFocus7 inFocus7 marked this pull request as draft September 9, 2024 19:13
@inFocus7 inFocus7 marked this pull request as ready for review September 10, 2024 15:30
@inFocus7 inFocus7 merged commit c227bca into main Sep 10, 2024
4 checks passed
@djmcaleese djmcaleese deleted the declan/keycloak branch September 13, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants