-
Notifications
You must be signed in to change notification settings - Fork 271
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
Implement all Keycloak settings #1203
base: master
Are you sure you want to change the base?
Conversation
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.
This would be very nice. Sadly that (and the naming thing) is all I can say before reading on oidc again
This introduces a single parameter to select the type of authentication.
Today the instructions for users are to set various settings in the UI after enabling Keycloak, but that's not great. Especially since some values are known or even must be set to a certain value. This creates settings files so the user is unable to change the values in the UI. The result is also a shorter instructions in the manual.
Still not complete, but I handled a few more cases that were obviously wrong while changing the parameter to |
':login_delegation_logout_url' => "${foreman::foreman_url}/users/extlogout", | ||
# TODO: parameters or obtain from ${oidc_issuer}/.well-known/openid-configuration | ||
':oidc_algorithm' => 'RS256', | ||
':oidc_audience' => ["${foreman::servername}-foreman-openidc"], |
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 would parameterize oidc_audience
so that the value can be overwritten. This allows people who have a certain naming standards for their keycloak clients to keep using them.
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 agree, which is why there's a TODO above it.
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.
Apologies! Completely over looked those
':oidc_algorithm' => 'RS256', | ||
':oidc_audience' => ["${foreman::servername}-foreman-openidc"], | ||
':oidc_issuer' => $oidc_issuer, | ||
':oidc_jwks_url' => "${oidc_issuer}/protocol/openid-connect/certs", |
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 would also parameterize oidc_jwks_url
so that the value can be overwritten. If you use non standard paths or keycloak updates their path, as they have been known for doing already you will have no option to overwrite this.
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 was still wondering how to best deal with this. IMHO this is also covered by the TODO above it that this isn't the final form.
One thought I had was to have keycloak
as one opinionated option and one oidc
as a generic one. Not sure if that makes sense or not.
Note that the OIDC standard says there should be a .well-known/openid-configuration
endpoint. Quoting https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderConfig
OpenID Providers supporting Discovery MUST make a JSON document available at the path formed by concatenating the string /.well-known/openid-configuration to the Issuer.
In particular, it should have the userinfo_endpoint
metadata. That points to https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata which list jwks_uri
. In other words, you should always be able to look this up. If you look at how Apache is configured by keycloak-httpd-client-install it's using OIDCProviderMetadataURL {{ keycloak_server_url }}/realms/{{ keycloak_realm }}/.well-known/openid-configuration
. That's what the TODO refers to. If we have the metadata URL then you can read:
oidc_algorithm
fromid_token_signing_alg_values_supported
odic_issuer
fromissuer
oidc_jwks_url
fromjwks_uri
That only leaves oidc_audience
.
Now I wonder what should read these values. Should we modify Foreman itself to do this? That feels more correct than having the installer retrieve these values and store it.
This is inspired by #1201. The goal is to refactor ODIC auth to become completely managed by the installer without additional manual steps afterwards. It's still very incomplete, but I'm pushing this so it's at least visible to others.