Fixes #33811 - Inheritance of root password for Hosts

Hostgroups are used for handle and easily modify passwords for Hosts. But prior this change we stored the password on create time to the Host even if it was inherited from Hostgroup.
When you've changed Hostgroup for Host, the root password was intanct and thus it used password from the old Hostgroup.
This change it so the password is stored on the Host only if it is set directly, otherwise it's stored on the Hostgroup and always read from there.
This also fixes the issue, that the password was returned unencrypted if it was comming from global setting.

app/models/concerns/host_common.rb

@@ -40,7 +40,7 @@ module HostCommon
     # See "def lookup_values_attributes=" under, for the implementation of accepts_nested_attributes_for :lookup_values
     accepts_nested_attributes_for :lookup_values
 
-    before_save :check_puppet_ca_proxy_is_required?, :crypt_root_pass
+    before_save :check_puppet_ca_proxy_is_required?, :crypt_passwords
     before_save :set_lookup_value_matcher
 
     # Replacement of accepts_nested_attributes_for :lookup_values,
@@ -151,30 +151,31 @@ def image_file
     super || default_image_file
   end
 
-  def crypt_root_pass
-    # hosts will always copy and crypt the password from parents when saved, but hostgroups should
-    # only crypt if the attribute is stored, else will stay blank and inherit
-    unencrypted_pass = if is_a?(Hostgroup)
-                         self[:root_pass]
-                       else
-                         root_pass
-                       end
-
-    if unencrypted_pass.present?
-      is_actually_encrypted = if (operatingsystem.try(:password_hash) == "Base64" || operatingsystem.try(:password_hash) == "Base64-Windows")
-                                password_base64_encrypted?
-                              elsif PasswordCrypt.crypt_gnu_compatible?
-                                unencrypted_pass.match('^\$\d+\$.+\$.+')
-                              else
-                                unencrypted_pass.starts_with?("$")
-                              end
-
-      if is_actually_encrypted
-        self.root_pass = self.grub_pass = unencrypted_pass
-      else
-        self.root_pass = operatingsystem.nil? ? PasswordCrypt.passw_crypt(unencrypted_pass) : PasswordCrypt.passw_crypt(unencrypted_pass, operatingsystem.password_hash)
-        self.grub_pass = PasswordCrypt.grub2_passw_crypt(unencrypted_pass)
-      end
+  def crypt_passwords
+    self.root_pass = crypt_pass(self[:root_pass], :root)
+    self.grub_pass = crypt_pass(self[:grub_pass] || self[:root_pass], :grub)
+  end
+
+  def crypt_pass(unencrypted_pass, pass_kind)
+    return unless unencrypted_pass.present?
+    is_actually_encrypted = if operatingsystem.try(:password_hash) == "Base64" || operatingsystem.try(:password_hash) == "Base64-Windows"
+                              password_base64_encrypted?
+                            elsif PasswordCrypt.crypt_gnu_compatible?
+                              unencrypted_pass.match('^\$\d+\$.+\$.+')
+                            else
+                              unencrypted_pass.starts_with?("$")
+                            end
+
+    # Grub_pass and root_pass are the same, so return the right pass is correct everytime
+    return unencrypted_pass if is_actually_encrypted
+
+    case pass_kind
+    when :root
+      operatingsystem.nil? ? PasswordCrypt.passw_crypt(unencrypted_pass) : PasswordCrypt.passw_crypt(unencrypted_pass, operatingsystem.password_hash)
+    when :grub
+      PasswordCrypt.grub2_passw_crypt(unencrypted_pass)
+    else
+      raise "Incorrect type of password. Only one of :root, :grub is supported"
     end
   end
 

app/models/host/managed.rb

@@ -659,12 +659,12 @@ def provider
   def root_pass
     return self[:root_pass] if self[:root_pass].present?
     return hostgroup.try(:root_pass) if hostgroup.try(:root_pass).present?
-    Setting[:root_pass]
+    crypt_pass(Setting[:root_pass], :root)
   end
 
   def root_pass_source
-    return N_("host") if self[:root_pass].present?
-    return N_("hostgroup") if hostgroup.try(:root_pass).present?
+    return N_("host") if root_pass_present?
+    return N_("hostgroup") if hostgroup.try(:root_pass_present?)
     return N_("global setting") if Setting[:root_pass].present?
     nil
   end
@@ -1009,4 +1009,8 @@ def validate_association_taxonomy(association_name)
   def compute_resource_in_taxonomy
     validate_association_taxonomy(:compute_resource)
   end
+
+  def root_pass_present?
+    self[:root_pass].present?
+  end
 end

app/models/hostgroup.rb

@@ -197,7 +197,7 @@ def root_pass
     return self[:root_pass] if self[:root_pass].present?
     npw = nested_root_pw
     return npw if npw.present?
-    Setting[:root_pass]
+    crypt_pass(Setting[:root_pass], :root)
   end
 
   def explicit_pxe_loader
@@ -251,6 +251,11 @@ def render_template(template:, **params)
     template.render(host: self, **params)
   end
 
+  def root_pass_present?
+    return true if self[:root_pass].present?
+    nested_root_pw
+  end
+
   protected
 
   def lookup_value_match

test/controllers/api/v2/hosts_controller_test.rb

@@ -707,7 +707,8 @@ def test_rebuild_tftp_config
       facts['foreman_hostgroup'] = hostgroup.title
       post :facts, params: { :name => hostname, :facts => facts }
       assert_response :success
-      assert_equal hostgroup.root_pass, Host.find_by(:name => hostname).root_pass
+      host_pass = as_admin { Host.find_by(:name => hostname).root_pass }
+      assert_equal hostgroup.root_pass, host_pass
     end
 
     test 'when ":restrict_registered_smart_proxies" is false, HTTP requests should be able to import facts' do

test/models/host_test.rb

@@ -968,7 +968,7 @@ def teardown
     h.save!
     assert h.root_pass.present?
     assert_equal h.hostgroup.root_pass, h.root_pass
-    assert_equal h.hostgroup.root_pass, h.read_attribute(:root_pass), 'should copy root_pass to host unmodified'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host unmodified'
   end
 
   test "should use hostgroup base64-windows root password without reencoding" do
@@ -984,7 +984,7 @@ def teardown
     h.save!
     assert h.root_pass.present?
     assert_equal h.hostgroup.root_pass, h.root_pass
-    assert_equal h.hostgroup.root_pass, h.read_attribute(:root_pass), 'should copy root_pass to host unmodified'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host unmodified'
   end
 
   test "should use hostgroup root password" do
@@ -995,7 +995,7 @@ def teardown
     assert h.save
     assert h.root_pass.present?
     assert_equal h.hostgroup.root_pass, h.root_pass
-    assert_equal h.hostgroup.root_pass, h.read_attribute(:root_pass), 'should copy root_pass to host'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host unmodified'
   end
 
   test "should use a nested hostgroup parent root password" do
@@ -1011,7 +1011,7 @@ def teardown
     assert h.save
     assert h.root_pass.present?
     assert_equal p.root_pass, h.root_pass
-    assert_equal p.root_pass, h.read_attribute(:root_pass), 'should copy root_pass to host'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host unmodified'
   end
 
   test "should use settings root password" do
@@ -1021,7 +1021,88 @@ def teardown
     assert h.save
     assert h.root_pass.present?
     assert_equal Setting[:root_pass], h.root_pass
-    assert_equal Setting[:root_pass], h.read_attribute(:root_pass), 'should copy root_pass to host'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host'
+  end
+
+  test "should change password when hostgroup is changed" do
+    first_password = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    second_password = "$2$newone$hCkak1kaJPQILNmYbUXhD1"
+
+    h1 = FactoryBot.create(:hostgroup, :root_pass => first_password)
+    h2 = FactoryBot.create(:hostgroup, :root_pass => second_password)
+
+    host = FactoryBot.create(:host, :managed, :hostgroup => h1, :root_pass => nil)
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal first_password, host.root_pass
+    assert_equal host.root_pass_source, 'hostgroup'
+
+    host.hostgroup = h2
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal second_password, host.root_pass
+    assert_equal host.root_pass_source, 'hostgroup'
+  end
+
+  test "should change password when hostgroup is changed to hostgroup without password" do
+    first_password = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    Setting[:root_pass] = "$2$newone$hCkak1kaJPQILNmYbUXhD1"
+
+    h1 = FactoryBot.create(:hostgroup, :root_pass => first_password)
+    h2 = FactoryBot.create(:hostgroup, :root_pass => nil)
+
+    host = FactoryBot.create(:host, :managed, :hostgroup => h1, :root_pass => nil)
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal first_password, host.root_pass
+    assert_equal host.root_pass_source, 'hostgroup'
+
+    host.hostgroup = h2
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal Setting[:root_pass], host.root_pass
+    assert_equal host.root_pass_source, 'global setting'
+  end
+
+  test "should change password when hostgroup without password is changed to hostgroup with password" do
+    Setting[:root_pass] = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    second_password = "$2$newone$hCkak1kaJPQILNmYbUXhD1"
+
+    h1 = FactoryBot.create(:hostgroup, :root_pass => nil)
+    h2 = FactoryBot.create(:hostgroup, :root_pass => second_password)
+
+    host = FactoryBot.create(:host, :managed, :hostgroup => h1, :root_pass => nil)
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal Setting[:root_pass], host.root_pass
+    assert_equal host.root_pass_source, 'global setting'
+
+    host.hostgroup = h2
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal second_password, host.root_pass
+    assert_equal host.root_pass_source, 'hostgroup'
+  end
+
+  test "should change password when hostgroup without password is changed to hostgroup with password and parent" do
+    Setting[:root_pass] = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    second_password = "$2$newone$hCkak1kaJPQILNmYbUXhD1"
+
+    h1 = FactoryBot.create(:hostgroup, :root_pass => nil)
+    parent_h2 = FactoryBot.create(:hostgroup, :root_pass => second_password)
+    h2 = FactoryBot.create(:hostgroup, :parent => parent_h2, :root_pass => nil)
+
+    host = FactoryBot.create(:host, :managed, :hostgroup => h1, :root_pass => nil)
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal Setting[:root_pass], host.root_pass
+    assert_equal host.root_pass_source, 'global setting'
+
+    host.hostgroup = h2
+    assert host.save
+    assert host.root_pass.present?
+    assert_equal second_password, host.root_pass
+    assert_equal host.root_pass_source, 'hostgroup'
   end
 
   test "should use settings root password when hostgroup has empty root password" do
@@ -1033,7 +1114,7 @@ def teardown
     assert_valid h
     assert h.root_pass.present?
     assert_equal Setting[:root_pass], h.root_pass
-    assert_equal Setting[:root_pass], h.read_attribute(:root_pass), 'should copy root_pass to host'
+    assert_nil h.read_attribute(:root_pass), 'should not copy root_pass to host unmodified'
   end
 
   test "should validate pxe loader when provided" do

test/models/hostgroup_test.rb

@@ -223,6 +223,8 @@ class HostgroupTest < ActiveSupport::TestCase
 
   test "root_pass inherited from settings if blank" do
     Setting[:root_pass] = '12345678'
+    PasswordCrypt.expects(:crypt_gnu_compatible?).at_least_once.returns(true)
+    PasswordCrypt.expects(:passw_crypt).with(Setting[:root_pass]).at_least_once.returns(Setting[:root_pass])
     hostgroup = FactoryBot.build(:hostgroup, :root_pass => '')
     assert_equal '12345678', hostgroup.root_pass
     hostgroup.save!
@@ -231,6 +233,8 @@ class HostgroupTest < ActiveSupport::TestCase
 
   test "root_pass inherited from settings if group and parent are blank" do
     Setting[:root_pass] = '12345678'
+    PasswordCrypt.expects(:crypt_gnu_compatible?).at_least_once.returns(true)
+    PasswordCrypt.expects(:passw_crypt).with(Setting[:root_pass]).at_least_once.returns(Setting[:root_pass])
     parent = FactoryBot.create(:hostgroup, :root_pass => '')
     hostgroup = FactoryBot.build(:hostgroup, :parent => parent, :root_pass => '')
     assert_equal '12345678', hostgroup.root_pass

test/unit/foreman/renderer/scope/macros/host_template_test.rb

@@ -184,6 +184,7 @@ def host_info
     test 'grub_pass helper returns the grub password if enabled' do
       @scope.instance_variable_set('@host', host)
       FactoryBot.create(:parameter, name: 'encrypt_grub', value: 'true')
+      assert host.save
       assert_equal "--iscrypted --password=#{host.grub_pass}", @scope.grub_pass
     end