Conversation
ekohl
left a comment
There was a problem hiding this comment.
Exposing these makes sense to me. In hindsight I'm not happy with how we (I) have implemented Keycloak integration. It relies a lot on keycloak-httpd-client-install and that's problematic. This lower level "expose the parameters" approach is IMHO better, but I have some implementation notes.
I'd prefer to avoid exposing those authorize_login_delegation. At first I thought about a more specific Boolean $oidc, but prefer a more generic auth parameter like Enum['internal', 'ipa', 'keycloak', 'oidc'] $auth = 'internal'. We have places where we could then use a case statement to handle it. This makes it obvious to users they need to select a single auth mechanism.
This is where we handle external auth on the Foreman side:
puppet-foreman/manifests/config.pp
Lines 188 to 283 in dbfdc94
And this is where we configure Apache:
puppet-foreman/manifests/config/apache.pp
Lines 239 to 264 in dbfdc94
As you can see, this part relies on
keycloak-http-client-install but I'd prefer to configure Apache's mod_auth_openidc ourselves and I think the parameters you expose can do that. You can do so by passing oidc parameters to the vhost: https://github.com/puppetlabs/puppetlabs-apache/blob/97449e4a0a9f395a2765a3100336070e8298f1dc/manifests/vhost.pp#L1649-L1653 and see https://github.com/puppetlabs/puppetlabs-apache/blob/main/types/oidcsettings.pp for the specific types. You'd probably add them very similar to vhost_https_internal_options.
In #1172 I started some refactoring in the auth area that you might find interesting as well. In general I want to move to EPP templates instead of ERB but haven't fully pursued it.
Lastly, I would really like to see some tests before we merge this, but I can imagine you first want to get the design right to avoid rewriting those all the time.
| <% if scope.lookupvar('foreman::authorize_login_delegation') -%> | ||
| # The following values are used for oidc authentication | ||
| :authorize_login_delegation: <%= scope.lookupvar("foreman::authorize_login_delegation") %> | ||
| :authorize_login_delegation_auth_source_user_autocreate: <%= scope.lookupvar("foreman::authorize_login_delegation_auth_source_user_autocreate") %> | ||
| :login_delegation_logout_url: <%= scope.lookupvar("foreman::login_delegation_logout_url") %> | ||
| :oidc_jwks_url: <%= scope.lookupvar("foreman::oidc_jwks_url") %> | ||
| :oidc_audience: <%= scope.lookupvar("foreman::oidc_audience") %> | ||
| :oidc_issuer: <%= scope.lookupvar("foreman::oidc_issuer") %> | ||
| :oidc_algorithm: <%= scope.lookupvar("foreman::oidc_algorithm") %> | ||
|
|
||
| <% end -%> |
There was a problem hiding this comment.
I'd prefer to have this as a separate fragment. Possibly reuse the existing one: https://github.com/theforeman/puppet-foreman/blob/master/templates/settings-external-auth.yaml.erb
If it helps, https://github.com/latchset/keycloak-httpd-client-install/blob/master/templates/oidc_httpd.conf is what's used today in keycloak-httpd-client-install. Some of those values ( |
|
cc @adamruzicka this might interest you |
|
For context, this would make the sections https://docs.theforeman.org/nightly/Configuring_User_Authentication/index-katello.html#configuring-a-foreman-client-to-provide-foreman_web_UI-authentication-with-keycloak_keycloak-wildfly and https://docs.theforeman.org/nightly/Configuring_User_Authentication/index-katello.html#configuring-a-foreman-client-to-provide-hammer-cli-authentication-with-keycloak_keycloak-wildfly obsolete by adding the options to the installer command. Bonus value of putting it in the YAML files is that users can't override them at all, even if they wanted to. |
|
An incomplete version of what I suggested is in #1203. |
This PR adds a oidc parameters, to allow enabling oidc authentication with for example keycloak completely from config as code. The parameters get added to settings.yml config.