Fixes #28436 - Add keycloak support #779

ekohl · 2019-12-05T17:55:53Z

This is a draft since it hasn't been tested end to end. It's here to allow end-to-end testing using forklift.

theforeman/forklift#1065 is a related PR to install a Keycloak server.

rabajaj0509

@ekohl I am also trying to use this PR as an opportunity to learn and contribute to the puppet-foreman repo.

Few questions in the comments below.

manifests/settings.pp

rabajaj0509 · 2019-12-05T20:21:59Z

manifests/config/apache.pp

+    #
+    # keycloak-httpd-client-install --app-name ${keycloak_app_name} --keycloak-server-url $KEYCLOAK_URL --keycloak-admin-username $KEYCLOAK_USER --keycloak-realm ${keycloak_realm} --keycloak-admin-realm master --keycloak-auth-role root-admin --client-type openidc --client-hostname ${servername} --protected-locations /users/extlogin
+    #
+    # If $suburi is used, --location-root should also be passed in


Not sure what $suburi is and where/how it can we used. Can you share some insights?

Not sure how much it's used, but if foreman_url is passed in as https://foreman.example.com/foreman then $suburi will be set. Probably ok to ignore it for now.

manifests/settings.pp

ekohl · 2019-12-16T14:59:18Z

Updated to not configure the settings and require the user to do so.

rabajaj0509 · 2019-12-16T15:23:03Z

manifests/config/apache.pp

+  } elsif $keycloak {
+    apache::mod { 'auth_openidc':
+      package => 'mod_auth_openidc',
+    }


Should we also install keycloak-httpd-client-install package too?

I was debating that, but decided against it. I think the instruction should be:

yum install keycloak-httpd-client-install

keycloak-httpd-client-install ....

foreman-installer --foreman-keycloak true --foreman-keycloak-realm ...

In theory the installer could run the install command, but then we need to pass credentials and I think that's something we probably don't want.

ntkathole · 2019-12-23T04:19:13Z

@ekohl can you please rebase?

stderr_lines:
  - From https://github.com/theforeman/puppet-foreman
  - ' * [new ref]         refs/pull/779/head -> pr'
  stdout: |-
    Auto-merging spec/classes/foreman_spec.rb
    Auto-merging manifests/init.pp
    Auto-merging manifests/config/apache.pp
    CONFLICT (content): Merge conflict in manifests/config/apache.pp
    Automatic merge failed; fix conflicts and then commit the result.
  stdout_lines: <omitted>

rabajaj0509 · 2020-01-04T12:51:45Z

@ekohl

I think this should good once the conflict is solved, can you please solve the conflicts and rebase?

ekohl · 2020-01-06T17:00:09Z

Rebased to fix the merge conflict.

ntkathole · 2020-01-07T08:42:01Z

@ekohl @rahulbajaj0509 I am not able to test this PR as installer is failing with issue mentioned at theforeman/puppet-certs#266 (comment) I tried to revert the PR changes but still installer is failing with Failed to call refresh: Proxy centos7-katello-devel.nkathole.example.com cannot be retrieved: unknown error (response 503)

ekohl · 2020-01-22T17:05:18Z

@ekohl @rahulbajaj0509 I am not able to test this PR as installer is failing with issue mentioned at theforeman/puppet-certs#266 (comment) I tried to revert the PR changes but still installer is failing with Failed to call refresh: Proxy centos7-katello-devel.nkathole.example.com cannot be retrieved: unknown error (response 503)

That's unrelated, but should be resolved in the latest nightly packages.

ehelms · 2020-02-11T12:25:47Z

@ekohl Seems fairly straight forward of a change, if you circle back to the tests I am happy to merge

ehelms · 2020-02-14T00:21:24Z

manifests/config/apache.pp

@@ -83,6 +83,9 @@
  Boolean $ipa_authentication = $::foreman::ipa_authentication,
  Hash[String, Any] $http_vhost_options = {},
  Hash[String, Any] $https_vhost_options = {},
+  Boolean $keycloak = $foreman::keycloak,
+  String[1] $keycloak_app_name = $foreman::keycloak_app_name,
+  String[1] $keycloak_realm = $foreman::keycloak_realm,


Do these need to be $:: prefixed?

Technically no and I'd also like to remove that prefix everywhere else. Just haven't gotten around to it. It was needed to make Puppet 3 and Puppet 4 behave in the same way but luckily those dark days are behind us. In this case I'm going to add them to stay consistent with the rest of the file.

ehelms · 2020-02-19T12:52:55Z

Needs a rebase

ehelms · 2020-02-26T14:22:51Z

spec/classes/foreman_config_apache_spec.rb

+          let(:params) { super().merge(keycloak: true) }
+
+          it { should compile.with_all_deps }
+          it { should contain_apache_mod('auth_openidc') }


This needs an additional underscore apache__mod

ehelms · 2020-02-26T14:25:05Z

I see a dependency cycle error as well in the tests but I am less clear on how to resolve that one.

This adds support for keycloak when using the keycloak-httpd-client-install command. A more native integration would rely on the oidc support in puppetlabs-apache, but that's unreleased. It drops the dependency chaining for the passenger package because it was leading to dependency cycles and I don't see a reason for the explicit chaining.

ehelms

Failures look like the same Debian failures we've been seeing

theforeman-bot added Needs testing Not yet reviewed labels Dec 5, 2019

rabajaj0509 reviewed Dec 5, 2019

View reviewed changes

ekohl mentioned this pull request Dec 12, 2019

Fixes #26312 - fix default taxonomy for external users theforeman/foreman#7219

Closed

rabajaj0509 reviewed Dec 16, 2019

View reviewed changes

manifests/settings.pp Outdated Show resolved Hide resolved

ekohl force-pushed the 28436-keycloak branch from 89e9ab6 to 0a966e8 Compare December 16, 2019 14:59

rabajaj0509 reviewed Dec 16, 2019

View reviewed changes

ekohl force-pushed the 28436-keycloak branch from 0a966e8 to b31019e Compare January 6, 2020 16:59

ekohl marked this pull request as ready for review January 6, 2020 16:59

ekohl mentioned this pull request Jan 13, 2020

Ensure pulp-server package present before qpid client certs created theforeman/puppet-certs#270

Closed

ekohl force-pushed the 28436-keycloak branch from b31019e to 4abb017 Compare February 12, 2020 14:38

ehelms reviewed Feb 14, 2020

View reviewed changes

ekohl force-pushed the 28436-keycloak branch 2 times, most recently from 850220b to c5d7cdb Compare February 14, 2020 12:43

ekohl force-pushed the 28436-keycloak branch from c5d7cdb to b39013c Compare February 25, 2020 15:50

ehelms reviewed Feb 26, 2020

View reviewed changes

ekohl force-pushed the 28436-keycloak branch from b39013c to 52052e4 Compare February 27, 2020 17:18

ehelms approved these changes Feb 28, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label Feb 28, 2020

ehelms merged commit 0e86214 into theforeman:master Feb 28, 2020

ekohl deleted the 28436-keycloak branch March 13, 2020 18:12

ekohl added the Enhancement label Apr 28, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #28436 - Add keycloak support #779

Fixes #28436 - Add keycloak support #779

ekohl commented Dec 5, 2019

rabajaj0509 left a comment

rabajaj0509 Dec 5, 2019

ekohl Dec 6, 2019

ekohl commented Dec 16, 2019

rabajaj0509 Dec 16, 2019

ekohl Dec 16, 2019

ntkathole commented Dec 23, 2019

rabajaj0509 commented Jan 4, 2020

ekohl commented Jan 6, 2020

ntkathole commented Jan 7, 2020

ekohl commented Jan 22, 2020

ehelms commented Feb 11, 2020

ehelms Feb 14, 2020

ekohl Feb 14, 2020

ehelms commented Feb 19, 2020

ehelms Feb 26, 2020

ehelms commented Feb 26, 2020

ehelms left a comment

Fixes #28436 - Add keycloak support #779

Fixes #28436 - Add keycloak support #779

Conversation

ekohl commented Dec 5, 2019

rabajaj0509 left a comment

Choose a reason for hiding this comment

rabajaj0509 Dec 5, 2019

Choose a reason for hiding this comment

ekohl Dec 6, 2019

Choose a reason for hiding this comment

ekohl commented Dec 16, 2019

rabajaj0509 Dec 16, 2019

Choose a reason for hiding this comment

ekohl Dec 16, 2019

Choose a reason for hiding this comment

ntkathole commented Dec 23, 2019

rabajaj0509 commented Jan 4, 2020

ekohl commented Jan 6, 2020

ntkathole commented Jan 7, 2020

ekohl commented Jan 22, 2020

ehelms commented Feb 11, 2020

ehelms Feb 14, 2020

Choose a reason for hiding this comment

ekohl Feb 14, 2020

Choose a reason for hiding this comment

ehelms commented Feb 19, 2020

ehelms Feb 26, 2020

Choose a reason for hiding this comment

ehelms commented Feb 26, 2020

ehelms left a comment

Choose a reason for hiding this comment