update support for openidc in puppet-keystone

The existing openidc support in puppet-keystone was incomplete and
would result in invalid Apache configurations. This commit updates
the openidc federation to work with modern Keystone and abstracts out
some common parameters for use in other federated identity modules.

Co-Authored-By: Nathan Kinder <nkinder@redhat.com>
Change-Id: I200011e2e0ffd01a2aa26df8a03f03151eb64150

manifests/federation.pp

@@ -0,0 +1,31 @@
+# == class: keystone::federation
+#
+# == Parameters
+#
+# [*trusted_dashboards*]
+#   (optional) URL list of trusted horizon servers.
+#   This setting ensures that keystone only sends token data back to trusted
+#   servers. This is performed as a precaution, specifically to prevent man-in-
+#   the-middle (MITM) attacks.
+#   Defaults to undef
+#
+# [*remote_id_attribute*]
+#   (optional) Value to be used to obtain the entity ID of the Identity
+#   Provider from the environment.
+#
+class keystone::federation (
+  $trusted_dashboards  = undef,
+  $remote_id_attribute = undef,
+) {
+  include ::keystone::deps
+
+  keystone_config {
+    'federation/trusted_dashboard': value  => any2array($trusted_dashboards);
+  }
+
+  if $remote_id_attribute {
+    keystone_config {
+      'federation/remote_id_attribute': value => $remote_id_attribute;
+    }
+  }
+}

manifests/federation/mellon.pp

@@ -4,7 +4,8 @@
 #
 # [*methods*]
 #  A list of methods used for authentication separated by comma or an array.
-#  The allowed values are: 'external', 'password', 'token', 'oauth1', 'saml2'
+#  The allowed values are: 'external', 'password', 'token', 'oauth1', 'saml2',
+#  and 'openid'
 #  (Required) (string or array value).
 #  Note: The external value should be dropped to avoid problems.
 #
@@ -45,13 +46,6 @@
 #   (optional) Wheater or not to enable Web Single Sign-On (SSO)
 #   Defaults to false
 #
-# [*trusted_dashboards*]
-#   (optional) URL list of trusted horizon servers.
-#   This setting ensures that keystone only sends token data back to trusted
-#   servers. This is performed as a precaution, specifically to prevent man-in-
-#   the-middle (MITM) attacks.
-#   Defaults to undef
-#
 # === DEPRECATED
 #
 # [*module_plugin*]
@@ -59,6 +53,15 @@
 #  module.
 #  (Optional) Defaults to 'keystone.auth.plugins.mapped.Mapped' (string value)
 #
+# [*trusted_dashboards*]
+#   (optional) URL list of trusted horizon servers.
+#   This setting ensures that keystone only sends token data back to trusted
+#   servers. This is performed as a precaution, specifically to prevent man-in-
+#   the-middle (MITM) attacks.
+#   It is recommended to use the keystone::federation class to set the
+#   trusted_dashboards configuration instead of this parameter.
+#   Defaults to undef
+#
 class keystone::federation::mellon (
   $methods,
   $idp_name,
@@ -68,8 +71,8 @@ class keystone::federation::mellon (
   $template_order     = 331,
   $package_ensure     = present,
   $enable_websso      = false,
-  $trusted_dashboards = undef,
   # DEPRECATED
+  $trusted_dashboards = undef,
   $module_plugin      = undef,
 ) {
 
@@ -77,6 +80,11 @@ class keystone::federation::mellon (
   include ::keystone::deps
   include ::keystone::params
 
+  if ($trusted_dashboards) {
+    warning("keystone::federation::mellon::trusted_dashboards is deprecated \
+in Stein and will be removed in future releases")
+  }
+
   # Note: if puppet-apache modify these values, this needs to be updated
   if $template_order <= 330 or $template_order >= 999 {
     fail('The template order should be greater than 330 and less than 999.')
@@ -105,12 +113,13 @@ Apache + Mellon SP setups, where a REMOTE_USER env variable is always set, even
   }
 
   if($enable_websso){
-    if( !trusted_dashboards){
+    if($trusted_dashboards){
-      fail('No trusted dashboard specified, please add at least one.')
+      keystone_config {
+        'federation/trusted_dashboard': value => join(any2array($trusted_dashboards),',');
+      }
     }
     keystone_config {
       'mapped/remote_id_attribute': value => 'MELLON_IDP';
-      'federation/trusted_dashboard': value => join(any2array($trusted_dashboards),',');
     }
   }
 

manifests/federation/openidc.pp

@@ -4,7 +4,8 @@
 #
 # [*methods*]
 #  A list of methods used for authentication separated by comma or an array.
-#  The allowed values are: 'external', 'password', 'token', 'oauth1', 'saml2'
+#  The allowed values are: 'external', 'password', 'token', 'oauth1', 'saml2',
+#  and 'openid'
 #  (Required) (string or array value).
 #  Note: The external value should be dropped to avoid problems.
 #
@@ -34,6 +35,10 @@
 #  (Optional) String value.
 #  Defaults to 'id_token'
 #
+# [*remote_id_attribute*]
+#  (optional) Value to be used to obtain the entity ID of the Identity
+#  Provider from the environment.
+#
 # [*admin_port*]
 #  A boolean value to ensure that you want to configure openidc Federation
 #  using Keystone VirtualHost on port 35357.
@@ -59,12 +64,16 @@
 #   accepts latest or specific versions.
 #   Defaults to present.
 #
+# [*keystone_public_url*]
+#   (optional) URL to keystone public endpoint.
+#
+# [*keystone_admin_url*]
+#    (optional) URL to keystone admin endpoint.
+#
 # === DEPRECATED
 #
 # [*module_plugin*]
-#  The plugin for authentication acording to the choice made with protocol and
+#  This value is no longer used.
-#  module.
-#  (Optional) Defaults to 'keystone.auth.plugins.mapped.Mapped' (string value)
 #
 class keystone::federation::openidc (
   $methods,
@@ -74,10 +83,13 @@ class keystone::federation::openidc (
   $openidc_client_secret,
   $openidc_crypto_passphrase   = 'openstack',
   $openidc_response_type       = 'id_token',
+  $remote_id_attribute         = undef,
   $admin_port                  = false,
   $main_port                   = true,
   $template_order              = 331,
   $package_ensure              = present,
+  $keystone_public_url         = undef,
+  $keystone_admin_url          = undef,
   # DEPRECATED
   $module_plugin               = undef,
 ) {
@@ -86,21 +98,24 @@ class keystone::federation::openidc (
   include ::keystone::deps
   include ::keystone::params
 
+  $_keystone_public_url = pick($keystone_public_url, $::keystone::public_endpoint)
+  $_keystone_admin_url = pick($keystone_admin_url, $::keystone::admin_endpoint)
+
   # Note: if puppet-apache modify these values, this needs to be updated
   if $template_order <= 330 or $template_order >= 999 {
     fail('The template order should be greater than 330 and less than 999.')
   }
 
   if ('external' in $methods ) {
-    fail('The external method should be dropped to avoid any interference with openidc')
+    fail('The external method should be dropped to avoid any interference with openid.')
   }
 
-  if !('openidc' in $methods ) {
+  if !('openid' in $methods ) {
-    fail('Methods should contain openidc as one of the auth methods.')
+    fail('Methods should contain openid as one of the auth methods.')
   }
 
-  validate_bool($admin_port)
+  validate_legacy(Boolean, 'validate_bool', $admin_port)
-  validate_bool($main_port)
+  validate_legacy(Boolean, 'validate_bool', $main_port)
 
   if( !$admin_port and !$main_port){
     fail('No VirtualHost port to configure, please choose at least one.')
@@ -108,7 +123,13 @@ class keystone::federation::openidc (
 
   keystone_config {
     'auth/methods': value  => join(any2array($methods),',');
-    'auth/openidc': ensure => absent;
+    'auth/openid':   ensure => absent;
+  }
+
+  if $remote_id_attribute {
+    keystone_config {
+      'openid/remote_id_attribute': value => $remote_id_attribute;
+    }
   }
 
   ensure_packages([$::keystone::params::openidc_package_name], {
@@ -116,18 +137,15 @@ class keystone::federation::openidc (
     tag    => 'keystone-support-package',
   })
 
-  if $admin_port {
+  if $admin_port and $_keystone_admin_url {
     keystone::federation::openidc_httpd_configuration{ 'admin':
-      port              => $::keystone::admin_port,
+      keystone_endpoint => $_keystone_admin_url,
-      keystone_endpoint => $::keystone::admin_endpoint,
     }
   }
 
-  if $main_port {
+  if $main_port and $_keystone_public_url {
     keystone::federation::openidc_httpd_configuration{ 'main':
-      port              => $::keystone::public_port,
+      keystone_endpoint => $_keystone_public_url,
-      keystone_endpoint => $::keystone::public_endpoint,
     }
   }
-
 }

manifests/federation/openidc_httpd_configuration.pp

@@ -2,20 +2,15 @@
 #
 # == Parameters
 #
-# [*port*]
-#  The port number to configure OpenIDC federated authentication on
-#  (Required) String value.
-#
 # [*keystone_endpoint*]
 #  The keystone endpoint to use when configuring the OpenIDC redirect back
 #  to keystone
 #  (Required) String value.
 #
 define keystone::federation::openidc_httpd_configuration (
-  $port              = undef,
   $keystone_endpoint = undef
 ) {
-  concat::fragment { "configure_openidc_on_port_${port}":
+  concat::fragment { "configure_openidc_on_${title}":
     target  => "${keystone::wsgi::apache::priority}-keystone_wsgi_${title}.conf",
     content => template('keystone/openidc.conf.erb'),
     order   => $keystone::federation::openidc::template_order,

spec/classes/keystone_federation_openidc_spec.rb

@@ -6,17 +6,16 @@ describe 'keystone::federation::openidc' do
     <<-EOS
     class { 'keystone':
       admin_token => 'service_token',
-      admin_password => 'special_password',
+      public_endpoint => 'http://os.example.com:5000',
+      admin_endpoint => 'http://os.example.com:35357',
     }
 
-    include apache
+    include keystone::wsgi::apache
-
-    class { 'keystone::wsgi::apache': }
     EOS
   end
 
   let :params do
-    { :methods => 'password, token, openidc',
+    { :methods => 'password, token, openid',
       :idp_name => 'myidp',
       :openidc_provider_metadata_url => 'https://accounts.google.com/.well-known/openid-configuration',
       :openidc_client_id => 'openid_client_id',
@@ -27,13 +26,13 @@ describe 'keystone::federation::openidc' do
 
   context 'with invalid params' do
     before do
-      params.merge!(:methods => 'external, password, token, oauth1')
+      params.merge!(:methods => 'external, password, token, oauth1, openid')
-      it_raises 'a Puppet::Error', /The external method should be dropped to avoid any interference with openidc/
+      it_raises 'a Puppet::Error', /The external method should be dropped to avoid any interference with openid/
     end
 
     before do
       params.merge!(:methods => 'password, token, oauth1')
-      it_raises 'a Puppet::Error', /Methods should contain openidc as one of the auth methods./
+      it_raises 'a Puppet::Error', /Methods should contain openid as one of the auth methods./
     end
 
     before do
@@ -73,12 +72,12 @@ describe 'keystone::federation::openidc' do
     end
 
     context 'with only required parameters' do
-      it 'should have basic params for mellon in Keystone configuration' do
+      it 'should have basic params for openidc in Keystone configuration' do
-        is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openidc')
+        is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openid')
-        is_expected.to contain_keystone_config('auth/openidc').with_ensure('absent')
+        is_expected.to contain_keystone_config('auth/openid').with_ensure('absent')
       end
 
-      it { is_expected.to contain_concat__fragment('configure_openidc_on_port_5000').with({
+      it { is_expected.to contain_concat__fragment('configure_openidc_on_main').with({
         :target => "10-keystone_wsgi_main.conf",
         :order  => params[:template_order],
       })}
@@ -91,23 +90,35 @@ describe 'keystone::federation::openidc' do
         })
       end
 
-      it 'should have basic params for mellon in Keystone configuration' do
+      it 'should have basic params for openidc in Keystone configuration' do
-        is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openidc')
+        is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openid')
-        is_expected.to contain_keystone_config('auth/openidc').with_ensure('absent')
+        is_expected.to contain_keystone_config('auth/openid').with_ensure('absent')
       end
 
-      it { is_expected.to contain_concat__fragment('configure_openidc_on_port_5000').with({
+      it { is_expected.to contain_concat__fragment('configure_openidc_on_main').with({
         :target => "10-keystone_wsgi_main.conf",
         :order  => params[:template_order],
       })}
 
-      it { is_expected.to contain_concat__fragment('configure_openidc_on_port_35357').with({
+      it { is_expected.to contain_concat__fragment('configure_openidc_on_admin').with({
         :target => "10-keystone_wsgi_admin.conf",
         :order  => params[:template_order],
       })}
     end
 
-    it { is_expected.to contain_package(platform_parameters[:openidc_package_name]) }
+    context 'with remote id attribute' do
+      before do
+        params.merge!({
+          :remote_id_attribute => 'myremoteid',
+        })
+      end
 
+      it 'should set remote id attribute in Keystone configuration' do
+        is_expected.to contain_keystone_config('openid/remote_id_attribute').with_value('myremoteid')
+      end
+
+    end
+
+    it { is_expected.to contain_package(platform_parameters[:openidc_package_name]) }
   end
 end

spec/classes/keystone_federation_spec.rb

@@ -0,0 +1,36 @@
+require 'spec_helper'
+
+describe 'keystone::federation' do
+
+  let(:pre_condition) do
+    <<-EOS
+    class { 'keystone':
+      admin_token => 'service_token',
+      admin_password => 'special_password',
+    }
+    EOS
+  end
+
+  let :params do
+    { :trusted_dashboards => ['http://dashboard.example.com'],
+      :remote_id_attribute => 'test_attribute',
+     }
+  end
+
+  on_supported_os({
+  }).each do |os,facts|
+    let (:facts) do
+      facts.merge!(OSDefaults.get_facts({}))
+    end
+
+    context 'with optional parameters' do
+      it 'should set federation/trusted_dashboard' do
+        is_expected.to contain_keystone_config('federation/trusted_dashboard').with_value(['http://dashboard.example.com'])
+      end
+
+      it 'should set federation/remote_id_attribute' do
+        is_expected.to contain_keystone_config('federation/remote_id_attribute').with_value('test_attribute')
+      end
+    end
+  end
+end

templates/openidc.conf.erb

@@ -7,14 +7,24 @@
   OIDCClientSecret "<%= scope['keystone::federation::openidc::openidc_client_secret']-%>"
   OIDCCryptoPassphrase "<%= scope['keystone::federation::openidc::openidc_crypto_passphrase']-%>"
 
-  OIDCRedirectURI "<%= @keystone_endpoint-%>/v3/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/auth/redirect"
+  # The following directives are required to support openidc from the command
-  <LocationMatch /v3/OS-FEDERATION/identity_providers/.*?/protocols/openidc/auth>
+  # line
+  <Location ~ "/v3/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/auth">
+      AuthType oauth20
+      Require valid-user
+  </Location>
+
+  # The following directives are necessary to support websso from Horizon
+  # (Per https://docs.openstack.org/keystone/pike/advanced-topics/federation/websso.html)
+  OIDCRedirectURI "<%= @keystone_endpoint-%>/v3/auth/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/websso"
+  OIDCRedirectURI "<%= @keystone_endpoint-%>/v3/auth/OS-FEDERATION/websso"
+
+  <LocationMatch "/v3/auth/OS-FEDERATION/websso/openidc">
       AuthType "openid-connect"
       Require valid-user
   </LocationMatch>
 
-  OIDCRedirectURI "<%= @keystone_endpoint-%>/v3/auth/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/websso/redirect"
+  <LocationMatch "/v3/auth/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/websso">
-  <LocationMatch /v3/auth/OS-FEDERATION/identity_providers/.*?/protocols/openidc/websso>
       AuthType "openid-connect"
       Require valid-user
   </LocationMatch>