-
Notifications
You must be signed in to change notification settings - Fork 43
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
Add validation of iss claim parameter #3552
Conversation
83631bd
to
9d2c2dd
Compare
issuer_url: http://keycloak:8080 # Use http://localhost:8081 instead for running minder outside of docker compose | ||
issuer_url: http://localhost:8081 | ||
issuer_claim: http://localhost:8081/realms/stacklok |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still want to have the default be the all-in-docker setup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can set this whichever way -- we override the config value with a flag in docker-compose for the all-in-docker setup.
@@ -87,11 +87,19 @@ var serveCmd = &cobra.Command{ | |||
} | |||
|
|||
// Identity | |||
jwksUrl, err := cfg.Identity.Server.Path("realms/stacklok/protocol/openid-connect/certs") | |||
// TODO: cfg.Identity.Server.IssuerUrl _should_ be a URL to an issuer that has an |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this comment is accurate. Maybe it's better to rename the issuerUrl
to hostnameUrl
. We intentionally use the hostname for deriving the user deletion URL as well, which is also Keycloak specific.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh... so, in an ideal world, the URL that Keycloak includes in the iss
claim would be reachable by the minder
server and then we could use the standard OpenID JWKS discovery.
Unfortunately, the current docker-compose setup produces an iss
of http://localhost:8081/realms/stacklok
, but the minder
container can only reach Keycloak via keycloak:8080
. This isn't a problem in staging or production, where Minder can reach the external interface, and we just happen to fetch some keys from an entirely different URL and use them to validate the JWT.
Let me think on this, and see if I can come up with a better way to handle this.
And yes, we should rename issuer_url
to identity_host
or identity_url
, but I didn't want to gratuitously break existing configuration (including our own).
That also reminds me that I need to set these fields in staging and production before we push this code there...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These fields are now set in production.
Summary
Per https://openid.net/specs/openid-connect-basic-1_0.html#IDTokenValidation:
In this case, we're not implementing discovery, and using a server-configured JWKS URL, but we should still validate the iss URL.
Fixes #3542
Change Type
Mark the type of change your PR introduces:
Testing
Unit tests and manual login test, which revealed that:
issuer_url
parameter, which is actually a hostname, not theiss
parameter. In particular, it doesn't include the/realms/stacklok
part, which is instead part of the URLs we build in Minder.issuer_url
tohttp://localhost:8081
for development (to match what's in the JWT), Minder in docker-compose is unable to reach Keycloak, because each is in a different network namespace./admin
API endpoints on the public internet.All of this is pointed me to splitting the identity into two bits -- a keycloak server URL for the admin API endpoints, and an openID issuer URL.
Review Checklist: