-
Notifications
You must be signed in to change notification settings - Fork 35
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,7 +46,8 @@ database: | |
|
||
identity: | ||
server: | ||
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 | ||
Comment on lines
-49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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. |
||
client_id: minder-server | ||
client_secret: secret | ||
audience: minder | ||
|
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
tohostnameUrl
. 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 theminder
server and then we could use the standard OpenID JWKS discovery.Unfortunately, the current docker-compose setup produces an
iss
ofhttp://localhost:8081/realms/stacklok
, but theminder
container can only reach Keycloak viakeycloak: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
toidentity_host
oridentity_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.