From 8e44af162bdcd9161db775bdf3abbbb55f91682c Mon Sep 17 00:00:00 2001 From: Lars Kellogg-Stedman Date: Wed, 13 Jun 2018 11:26:01 -0400 Subject: [PATCH] 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 Change-Id: I200011e2e0ffd01a2aa26df8a03f03151eb64150 --- manifests/federation.pp | 31 +++++++++++ manifests/federation/mellon.pp | 33 +++++++----- manifests/federation/openidc.pp | 52 +++++++++++++------ .../federation/openidc_httpd_configuration.pp | 7 +-- .../keystone_federation_openidc_spec.rb | 47 ++++++++++------- spec/classes/keystone_federation_spec.rb | 36 +++++++++++++ templates/openidc.conf.erb | 18 +++++-- 7 files changed, 167 insertions(+), 57 deletions(-) create mode 100644 manifests/federation.pp create mode 100644 spec/classes/keystone_federation_spec.rb diff --git a/manifests/federation.pp b/manifests/federation.pp new file mode 100644 index 000000000..69a9470c7 --- /dev/null +++ b/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; + } + } +} diff --git a/manifests/federation/mellon.pp b/manifests/federation/mellon.pp index 32a1aad78..69f40a4e1 100644 --- a/manifests/federation/mellon.pp +++ b/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){ - fail('No trusted dashboard specified, please add at least one.') + if($trusted_dashboards){ + 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),','); } } diff --git a/manifests/federation/openidc.pp b/manifests/federation/openidc.pp index 38fd6a13e..537a39531 100644 --- a/manifests/federation/openidc.pp +++ b/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 -# module. -# (Optional) Defaults to 'keystone.auth.plugins.mapped.Mapped' (string value) +# This value is no longer used. # 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 ) { - fail('Methods should contain openidc as one of the auth methods.') + if !('openid' in $methods ) { + fail('Methods should contain openid as one of the auth methods.') } - validate_bool($admin_port) - validate_bool($main_port) + validate_legacy(Boolean, 'validate_bool', $admin_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_endpoint, + keystone_endpoint => $_keystone_admin_url, } } - if $main_port { + if $main_port and $_keystone_public_url { keystone::federation::openidc_httpd_configuration{ 'main': - port => $::keystone::public_port, - keystone_endpoint => $::keystone::public_endpoint, + keystone_endpoint => $_keystone_public_url, } } - } diff --git a/manifests/federation/openidc_httpd_configuration.pp b/manifests/federation/openidc_httpd_configuration.pp index de415db10..eaea7c69d 100644 --- a/manifests/federation/openidc_httpd_configuration.pp +++ b/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, diff --git a/spec/classes/keystone_federation_openidc_spec.rb b/spec/classes/keystone_federation_openidc_spec.rb index db9a3878a..47e1d7cae 100644 --- a/spec/classes/keystone_federation_openidc_spec.rb +++ b/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 - - class { 'keystone::wsgi::apache': } + include 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') - it_raises 'a Puppet::Error', /The external method should be dropped to avoid any interference with openidc/ + params.merge!(:methods => 'external, password, token, oauth1, openid') + 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 - is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openidc') - is_expected.to contain_keystone_config('auth/openidc').with_ensure('absent') + it 'should have basic params for openidc in Keystone configuration' do + is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openid') + 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 - is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openidc') - is_expected.to contain_keystone_config('auth/openidc').with_ensure('absent') + it 'should have basic params for openidc in Keystone configuration' do + is_expected.to contain_keystone_config('auth/methods').with_value('password, token, openid') + 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 diff --git a/spec/classes/keystone_federation_spec.rb b/spec/classes/keystone_federation_spec.rb new file mode 100644 index 000000000..ab4251086 --- /dev/null +++ b/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 diff --git a/templates/openidc.conf.erb b/templates/openidc.conf.erb index e5fe33a6f..79fd8fa87 100644 --- a/templates/openidc.conf.erb +++ b/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 + # line + /protocols/openidc/auth"> + AuthType oauth20 + Require valid-user + + + # 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" + + AuthType "openid-connect" Require valid-user - OIDCRedirectURI "<%= @keystone_endpoint-%>/v3/auth/OS-FEDERATION/identity_providers/<%= scope['keystone::federation::openidc::idp_name']-%>/protocols/openidc/websso/redirect" - + /protocols/openidc/websso"> AuthType "openid-connect" Require valid-user