From e273f4c64df2abfa5e6da50b9d467ca56c6e7ba9 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 21 Aug 2024 13:17:39 -0400 Subject: [PATCH 1/3] In specs, make httpd user_attrs common --- spec/models/authenticator/httpd_spec.rb | 35 ++++++++----------------- 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 1a7afa7ef36..fd865374857 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -172,6 +172,17 @@ def authenticate } end + let(:user_attrs) do + { + :username => "testuser", + :fullname => "Test User", + :firstname => "Alice", + :lastname => "Aardvark", + :email => "testuser@example.com", + :domain => "example.com" + } + end + context "with user details" do let!(:alice) { FactoryBot.create(:user, :userid => 'alice') } @@ -707,14 +718,6 @@ def authenticate let(:headers) do super().merge('X-Remote-User-Groups' => 'wibble@fqdn,bubble@fqdn') end - let(:user_attrs) do - {:username => "testuser", - :fullname => "Test User", - :firstname => "Alice", - :lastname => "Aardvark", - :email => "testuser@example.com", - :domain => "example.com"} - end it "handles a comma separated grouplist" do expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble@fqdn", "bubble@fqdn"]) @@ -727,14 +730,6 @@ def authenticate let(:headers) do super().merge('X-Remote-User-Groups' => CGI.escape('spécial_char@fqdn:moré@fqdn')) end - let(:user_attrs) do - {:username => "testuser", - :fullname => "Test User", - :firstname => "Alice", - :lastname => "Aardvark", - :email => "testuser@example.com", - :domain => "example.com"} - end it "handles group names with escaped special characters" do expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["spécial_char@fqdn", "moré@fqdn"]) @@ -755,14 +750,6 @@ def authenticate 'X-Remote-User-Groups' => nil, } end - let(:user_attrs) do - {:username => "testuser", - :fullname => "Test User", - :firstname => "Alice", - :lastname => "Aardvark", - :email => "testuser@example.com", - :domain => "example.com"} - end it "handles nil group names" do expect(subject).to receive(:find_external_identity).with(username, user_attrs, []) From cb4ef414900c467cbd3854bbff142354b04e579b Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 4 Sep 2024 11:32:11 -0400 Subject: [PATCH 2/3] CP4AIOPS-3113 Introduce Settings.authentication.http_group_delimiter This allows users to configure their LDAP group delimiter Currently we are flexible and allow a , : or ; But if a group has that value, then it causes issues. The use case is to allow : in the group name for Liberty. This allows us to specify a , as the delimiter for that case. --- app/models/authenticator/httpd.rb | 8 +++++++- config/settings.yml | 1 + spec/models/authenticator/httpd_spec.rb | 19 ++++++++++++++++--- 3 files changed, 24 insertions(+), 4 deletions(-) diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index 2f0362ffad0..b81bed98f46 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -4,6 +4,11 @@ def self.proper_name 'External httpd' end + # @return group delimiter defined in settings + def self.group_delimiter + ::Settings.authentication.group_delimiter.presence + end + def authorize_queue(username, request, options, *_args) log_auth_debug("authorize_queue(username=#{username}, options=#{options})") @@ -146,7 +151,8 @@ def user_details_from_headers(username, request) user_headers.each { |k, v| log_auth_debug(" %-24{key} = \"%{val}\"" % {:key => k, :val => v}) } end - groups = CGI.unescape(user_headers['X-REMOTE-USER-GROUPS'] || '').split(/[;:,]/) + delimiter = self.class.group_delimiter || /[;:,]/ + groups = CGI.unescape(user_headers['X-REMOTE-USER-GROUPS'] || '').split(delimiter) user_attrs = {:username => username, :fullname => user_headers['X-REMOTE-USER-FULLNAME'], :firstname => user_headers['X-REMOTE-USER-FIRSTNAME'], diff --git a/config/settings.yml b/config/settings.yml index 0c360dc95e6..b2cfef4882d 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -16,6 +16,7 @@ :ldaphost: :ldapport: '389' :mode: database + :group_delimiter: :max_failed_login_attempts: 3 :locked_account_timeout: 2.minutes :search_timeout: 30 diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index fd865374857..4ccb3ad5f91 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -1,5 +1,5 @@ RSpec.describe Authenticator::Httpd do - subject { Authenticator::Httpd.new(config) } + subject { Authenticator::Httpd.new(Settings.authentication.to_hash) } let(:config) { {:httpd_role => false} } let(:request) do @@ -21,6 +21,7 @@ # dummy_password_for_external_auth hook runs, and it needs to ask # Authenticator#uses_stored_password? whether it's allowed to do anything. + stub_settings_merge(:authentication => config) allow(User).to receive(:authenticator).and_return(subject) EvmSpecHelper.local_miq_server @@ -713,18 +714,30 @@ def authenticate end end - context "using a comma separated group list" do + context "using default delimiters" do let(:config) { {:httpd_role => true} } let(:headers) do super().merge('X-Remote-User-Groups' => 'wibble@fqdn,bubble@fqdn') end - it "handles a comma separated grouplist" do + it "parses group names" do expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble@fqdn", "bubble@fqdn"]) authenticate end end + context "using custom delimiter in settings" do + let(:config) { {:httpd_role => true, :group_delimiter => ","} } + let(:headers) do + super().merge('X-Remote-User-Groups' => 'wibble:wobble@fqdn,hobble;bubble@fqdn') + end + + it "parses group names that contain characters from the default delimiters (:)" do + expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble:wobble@fqdn", "hobble;bubble@fqdn"]) + authenticate + end + end + context "when group names have escaped special characters" do let(:config) { {:httpd_role => true} } let(:headers) do From c6766b81ec985bd55015805af84df24bea7df638 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 4 Sep 2024 10:01:09 -0400 Subject: [PATCH 3/3] Use X-REMOTE-USER-GROUP-DELIMITER for parsing ldap groups Different configurations want a : ; or , as the delimiter This puts the delimiter definition into apache config, which is the system that knows the configuration better than others This requires different appliance and appliance_console to install files that specify the delimiters. hence the bump in appliance_console version --- Gemfile | 2 +- app/models/authenticator/httpd.rb | 3 +- spec/models/authenticator/httpd_spec.rb | 65 +++++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) diff --git a/Gemfile b/Gemfile index 5c716436cce..6e26c4bc037 100644 --- a/Gemfile +++ b/Gemfile @@ -290,7 +290,7 @@ end group :appliance, :optional => true do gem "irb", "=1.4.1", :require => false # Locked to same version as the installed RPM rubygem-irb-1.4.1-142.module_el9+787+b20bfeee.noarch so that we don't bundle our own - gem "manageiq-appliance_console", "~>9.1", ">=9.1.1", :require => false + gem "manageiq-appliance_console", "~>10.0", :require => false gem "rdoc", :require => false # Needed for rails console end diff --git a/app/models/authenticator/httpd.rb b/app/models/authenticator/httpd.rb index b81bed98f46..9dbd83d6910 100644 --- a/app/models/authenticator/httpd.rb +++ b/app/models/authenticator/httpd.rb @@ -151,7 +151,7 @@ def user_details_from_headers(username, request) user_headers.each { |k, v| log_auth_debug(" %-24{key} = \"%{val}\"" % {:key => k, :val => v}) } end - delimiter = self.class.group_delimiter || /[;:,]/ + delimiter = self.class.group_delimiter || user_headers['X-REMOTE-USER-GROUP-DELIMITER'].presence || /[;:,]/ groups = CGI.unescape(user_headers['X-REMOTE-USER-GROUPS'] || '').split(delimiter) user_attrs = {:username => username, :fullname => user_headers['X-REMOTE-USER-FULLNAME'], @@ -172,6 +172,7 @@ def user_details_from_headers(username, request) X-REMOTE-USER-EMAIL X-REMOTE-USER-DOMAIN X-REMOTE-USER-GROUPS + X-REMOTE-USER-GROUP-DELIMITER ].each_with_object({}) do |k, h| h[k] = request.headers[k]&.force_encoding("UTF-8") end.delete_nils diff --git a/spec/models/authenticator/httpd_spec.rb b/spec/models/authenticator/httpd_spec.rb index 4ccb3ad5f91..af496e4e08f 100644 --- a/spec/models/authenticator/httpd_spec.rb +++ b/spec/models/authenticator/httpd_spec.rb @@ -738,6 +738,50 @@ def authenticate end end + context "using custom delimiter in settings (despite a header)" do + let(:config) { {:httpd_role => true, :group_delimiter => "|"} } + let(:headers) do + super().merge('X-Remote-User-Groups' => 'wibble,wobble@fqdn|bubble@fqdn', 'X-Remote-User-Group-Delimiter' => ",") + end + let(:user_attrs) do + { + :username => "testuser", + :fullname => "Test User", + :firstname => "Alice", + :lastname => "Aardvark", + :email => "testuser@example.com", + :domain => "example.com" + } + end + + it "parses group names that contain characters from header" do + expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble,wobble@fqdn", "bubble@fqdn"]) + authenticate + end + end + + context "using header delimiter" do + let(:config) { {:httpd_role => true} } + let(:headers) do + super().merge('X-Remote-User-Groups' => 'wibble:wobble@fqdn,bubble@fqdn', 'X-Remote-User-Group-Delimiter' => ",") + end + let(:user_attrs) do + { + :username => "testuser", + :fullname => "Test User", + :firstname => "Alice", + :lastname => "Aardvark", + :email => "testuser@example.com", + :domain => "example.com" + } + end + + it "parses group names that contain characters from default delimiter" do + expect(subject).to receive(:find_external_identity).with(username, user_attrs, ["wibble:wobble@fqdn", "bubble@fqdn"]) + authenticate + end + end + context "when group names have escaped special characters" do let(:config) { {:httpd_role => true} } let(:headers) do @@ -769,6 +813,27 @@ def authenticate authenticate end end + + context "when there are no group names (but a group header)" do + let(:config) { {:httpd_role => true} } + let(:headers) do + { + 'X-Remote-User' => 'testuser', + 'X-Remote-User-FullName' => 'Test User', + 'X-Remote-User-FirstName' => 'Alice', + 'X-Remote-User-LastName' => 'Aardvark', + 'X-Remote-User-Email' => 'testuser@example.com', + 'X-Remote-User-Domain' => 'example.com', + 'X-Remote-User-Groups' => nil, + 'X-Remote-User-Group-Delimiter' => nil + } + end + + it "handles nil group names" do + expect(subject).to receive(:find_external_identity).with(username, user_attrs, []) + authenticate + end + end end end end