Fixes #33811 - Fixing inheriting of root password for Hosts #8887

domitea · 2021-10-29T19:24:13Z

Hosts need passwords, especially the root one for the provisioning. With the bigger amounts of hosts, hostgroups are used for handle and easily modify these attributes for hosts. Root password has special place in this chain but it isn't to great as it looks. When you're change the hostgroup for host, the root password became intanct. It makes sence for host that has own password defined but not for host that use hostgroup's password. So this PR tries to fix it.

theforeman-bot · 2021-10-29T19:24:15Z

Issues: #33811

domitea · 2021-11-02T14:07:51Z

Testing if the host group related tests are really fixed. The PR is about passwords so it's on the table to careful.

And as I learned earlier, the green sign at local dev setup doesn't automatically mean green button here.

lzap · 2021-11-04T08:08:06Z

app/models/host/managed.rb

@@ -684,12 +684,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_root_pass(Setting[:root_pass])&.first


I do not have time for full review, but what immediately strikes me is that previously this was just a getter, now this modifies state of the model. In the method both root_pass= and grub_pass= methods are called.

I don't think that's the case. The method crypt_root_pass just uses local variables root_pass and grub_pass, then returns them in the array. From this array, we take the first value (root_pass hash for the given input) and use only that as a result of root_pass method.

What causes the confusion is imho the fact, that crypt_root_pass always gives you hashes for both root and grub passwords. IMHO it should accept the parameter saying which type the consumer is interested in.

lzap · 2021-11-04T08:12:41Z

Can you describe the solution and how are you trying to achieve the result?

Also I am missing a test that would simulate what the user complains about, the patch just modifies existing tests. I read that as its making a change for existing behavior.

Don't get me wrong but this feature is a can of worms really. We need to be extra careful for this one.

ares

So I can confirm the behavior works as this.

The root_pass is never stored on the host or host group level unless explicitly set. Therefore, even after you modify the host group of the host, the host.root_pass will inherit the value from the host group. If not value is set for the host group, it still takes the value from the settings. The root_pass method now also correctly returns crypted version of the password.

Previously, we saved the root_pass to the host, meaning if you changed the HG which had a different root pw, the host would still have the old root pw even after the rebuild. While I see some logic behind that, it was not expected by the users. Also previously it didn't crypt the password if we used the one from Settings, this is also now fixed.

Things to improve:

There's a mistake in the root_pass_check though
the crypt_root_pass looks a bit cryptic (pun intended). Since it's based on the old code, it tries to deal with the brub_pass too. However we often need it for the root_pass only. I think it should accept a parameter which password the callee is actually interested in.
the test that covers the HG change (as @lzap pointed out) should be added
the test for root_pass_check with HG nesting should also be added

ares · 2021-11-05T14:03:15Z

app/models/concerns/host_common.rb

+  end
+
+  def root_pass_check
+    self[:root_pass].present?


This does not work reliably for host group, host group that is nested, does not have a root password set directly but inherits it from parent would result in false. The root pass component then says it applies global setting. In reality, the root_pass method that is used in host/managed.rb (line 686) from the hostgroup supports the inheritance from parents.

ares · 2021-11-05T14:12:04Z

app/models/host/managed.rb

@@ -684,12 +684,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_root_pass(Setting[:root_pass])&.first


I don't think that's the case. The method crypt_root_pass just uses local variables root_pass and grub_pass, then returns them in the array. From this array, we take the first value (root_pass hash for the given input) and use only that as a result of root_pass method.

What causes the confusion is imho the fact, that crypt_root_pass always gives you hashes for both root and grub passwords. IMHO it should accept the parameter saying which type the consumer is interested in.

ezr-ondrej

Most of my comments are not blockers, but please take a look at them and let me know what you think. Only real blocker is the double present? as Boolean#present? will lead to unexpected results.

ezr-ondrej · 2021-11-18T16:47:17Z

app/models/host/managed.rb

-    return N_("host") if self[:root_pass].present?
-    return N_("hostgroup") if hostgroup.try(:root_pass).present?
+    return N_("host") if root_pass_check
+    return N_("hostgroup") if hostgroup.try(:root_pass_check).present?


there is double present now. Probably not what we want.

ezr-ondrej · 2021-11-18T16:47:42Z

app/models/host/managed.rb

@@ -1034,4 +1034,8 @@ def validate_association_taxonomy(association_name)
  def compute_resource_in_taxonomy
    validate_association_taxonomy(:compute_resource)
  end
+
+  def root_pass_check


Why do we have method now?

ezr-ondrej · 2021-11-18T16:50:38Z

app/models/concerns/host_common.rb

-        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 save_crypted_root_pass


Why did we renamed the method? We are still just setting the password, we are not saving it in this method, are we?

I think crypt_host_passwords would describe the method better.

ezr-ondrej · 2021-11-18T16:53:23Z

app/models/hostgroup.rb

+    return true if nested_root_pw
+    self[:root_pass].present?


could we swap these?
nested_root_pw is quite inefficient method and we can skip it if self[:root_pass]

ezr-ondrej · 2021-11-18T16:56:54Z

app/models/concerns/host_common.rb

+
+  def crypt_pass(unencrypted_pass, pass_kind)
+    return unless unencrypted_pass.present?
+    raise "Correct type of password is not used. You can use only :root or :grub" if [:root, :grub].exclude?(pass_kind)


It is bit hard to read and decode what does it mean.

Suggested change

raise "Correct type of password is not used. You can use only :root or :grub" if [:root, :grub].exclude?(pass_kind)

raise "Incorrect type of password. Only one of :root, :grub is supported" unless [:root, :grub].include?(pass_kind)

ezr-ondrej · 2021-11-18T16:59:53Z

app/models/hostgroup.rb

@@ -251,6 +251,11 @@ def render_template(template:, **params)
    template.render(host: self, **params)
  end

+  def root_pass_check


Could that be root_pass_present? so it is more clear?
In case we decide to keep the method, which I believe might be overkill given it's used once.

ares

I see another review has been added meanwhile, so just leaving a comment for one code construction

ares · 2021-11-11T12:28:14Z

app/models/concerns/host_common.rb

+                              unencrypted_pass.starts_with?("$")
+                            end
+
+    if is_actually_encrypted


how about

# 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 'Unsupported pass type' end

That would be imho slightly more readable and safer in case someone uses the method in a wrong way

ezr-ondrej

Apart of one nit, looks correct to me 👍

ezr-ondrej · 2021-11-23T11:38:44Z

app/models/host/managed.rb

@@ -658,13 +658,13 @@ def provider
  # no need to store anything in the db if the password is our default
  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]
+    return hostgroup.try(:root_pass) if hostgroup.try(:root_pass)


Why did we removed present? here?

I removed it because there was a present? and ignored the method where is used. On the second look, it should be there.

Hosts need passwords, especially the root one for the provisioning. With the bigger amounts of hosts, hostgroups are used for handle and easily modify these attributes for hosts. Root password has special place in this chain but it isn't to great as it looks. When you're change the hostgroup for host, the root password became intanct. It makes sence for host that has own password defined but not for host that use hostgroup's password. So this PR tries to fix it.

domitea · 2021-11-23T13:11:38Z

All looks green, so I think it's ready for merge. What do you think, @ezr-ondrej ?

ezr-ondrej

Thanks @domitea ! 👍

All concerns were addressed.

ezr-ondrej · 2021-11-24T19:58:14Z

Thanks @ares and @lzap for the reviews! :)

theforeman-bot added Needs testing Not yet reviewed labels Oct 29, 2021

domitea force-pushed the fix_33811 branch 2 times, most recently from b329002 to 12c39dc Compare November 2, 2021 14:05

domitea force-pushed the fix_33811 branch 3 times, most recently from 0654815 to 8f323c9 Compare November 4, 2021 06:33

lzap reviewed Nov 4, 2021

View reviewed changes

ares previously requested changes Nov 5, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Not yet reviewed labels Nov 5, 2021

domitea force-pushed the fix_33811 branch from 8f323c9 to abf1cb6 Compare November 9, 2021 13:42

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 9, 2021

domitea force-pushed the fix_33811 branch 3 times, most recently from b1fb5e2 to 7a9b12f Compare November 9, 2021 15:49

ezr-ondrej assigned ares and lzap Nov 10, 2021

ezr-ondrej requested changes Nov 18, 2021

View reviewed changes

theforeman-bot added Waiting on contributor and removed Needs re-review labels Nov 18, 2021

ares reviewed Nov 19, 2021

View reviewed changes

domitea force-pushed the fix_33811 branch from 7a9b12f to f550e2b Compare November 22, 2021 08:58

theforeman-bot added Needs re-review and removed Waiting on contributor labels Nov 22, 2021

domitea force-pushed the fix_33811 branch from f550e2b to 45755ed Compare November 23, 2021 10:44

ezr-ondrej reviewed Nov 23, 2021

View reviewed changes

domitea force-pushed the fix_33811 branch from 45755ed to 123c89f Compare November 23, 2021 12:03

ezr-ondrej approved these changes Nov 24, 2021

View reviewed changes

theforeman-bot removed the Needs re-review label Nov 24, 2021

ezr-ondrej merged commit 60d53c2 into theforeman:develop Nov 24, 2021

ezr-ondrej mentioned this pull request Jan 19, 2022

Fixes #33732 - Make sure base64 encoding is done when creating a host via API #8878

Merged

m-bucher mentioned this pull request Jan 13, 2023

Fixes #35942 - pass from settings not encrypted #9583

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #33811 - Fixing inheriting of root password for Hosts #8887

Fixes #33811 - Fixing inheriting of root password for Hosts #8887

domitea commented Oct 29, 2021

theforeman-bot commented Oct 29, 2021

domitea commented Nov 2, 2021 •

edited

lzap Nov 4, 2021

ares Nov 5, 2021

lzap commented Nov 4, 2021

ares left a comment

ares Nov 5, 2021

ares Nov 5, 2021

ezr-ondrej left a comment

ezr-ondrej Nov 18, 2021

ezr-ondrej Nov 18, 2021

ezr-ondrej Nov 18, 2021

ezr-ondrej Nov 18, 2021

ezr-ondrej Nov 18, 2021

ezr-ondrej Nov 18, 2021

ares left a comment

ares Nov 11, 2021

ezr-ondrej left a comment

ezr-ondrej Nov 23, 2021

domitea Nov 23, 2021

domitea commented Nov 23, 2021

ezr-ondrej left a comment

ezr-ondrej commented Nov 24, 2021

	raise "Correct type of password is not used. You can use only :root or :grub" if [:root, :grub].exclude?(pass_kind)
	raise "Incorrect type of password. Only one of :root, :grub is supported" unless [:root, :grub].include?(pass_kind)

Fixes #33811 - Fixing inheriting of root password for Hosts #8887

Fixes #33811 - Fixing inheriting of root password for Hosts #8887

Conversation

domitea commented Oct 29, 2021

theforeman-bot commented Oct 29, 2021

domitea commented Nov 2, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzap commented Nov 4, 2021

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ares left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ezr-ondrej left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

domitea commented Nov 23, 2021

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej commented Nov 24, 2021

domitea commented Nov 2, 2021 •

edited