fixes theforeman#3725, theforeman#4167 - root password validations, r…

…emove default password

app/models/concerns/host_common.rb

@@ -17,7 +17,8 @@ module HostCommon
     belongs_to :subnet
     belongs_to :compute_profile
 
-    before_save :check_puppet_ca_proxy_is_required?
+    before_save :check_puppet_ca_proxy_is_required?, :crypt_root_pass
+
     has_many :lookup_values, :finder_sql => Proc.new { LookupValue.where('lookup_values.match' => lookup_value_match).to_sql }, :dependent => :destroy
     # See "def lookup_values_attributes=" under, for the implementation of accepts_nested_attributes_for :lookup_values
     accepts_nested_attributes_for :lookup_values
@@ -92,11 +93,8 @@ def image_file
     super || default_image_file
   end
 
-  # make sure we store an encrypted copy of the password in the database
-  # this password can be use as is in a unix system
-  def root_pass=(pass)
-    p = pass.empty? ? nil : (pass.starts_with?('$') ? pass : pass.crypt("$1$#{SecureRandom.base64(6)}"))
-    write_attribute(:root_pass, p)
+  def crypt_root_pass
+    self.root_pass = root_pass.empty? ? nil : (root_pass.starts_with?('$') ? root_pass : root_pass.crypt("$1$#{SecureRandom.base64(6)}"))
   end
 
   def param_true? name

app/models/host/managed.rb

@@ -133,7 +133,9 @@ class Jail < ::Safemode::Jail
     validates :mac, :uniqueness => true, :format => {:with => Net::Validations::MAC_REGEXP}, :unless => Proc.new { |host| host.compute? or !host.managed }
     validates :architecture_id, :operatingsystem_id, :domain_id, :presence => true, :if => Proc.new {|host| host.managed}
     validates :mac, :presence => true, :unless => Proc.new { |host| host.compute? or !host.managed }
-    validates :root_pass, :length => {:minimum => 8, :message => _('should be 8 characters or more')}
+    validates :root_pass, :length => {:minimum => 8, :message => _('should be 8 characters or more')},
+                          :presence => {:message => N_('should not be blank - consider setting a global or host group default')},
+                          :unless => Proc.new { |host| !host.managed or capabilities.include?(:image) }
     validates :ip, :format => {:with => Net::Validations::IP_REGEXP}, :if => Proc.new { |host| host.require_ip_validation? }
     validates :ptable_id, :presence => {:message => N_("cant be blank unless a custom partition has been defined")},
                           :if => Proc.new { |host| host.managed and host.disk.empty? and not defined?(Rake) and capabilities.include?(:build) }
@@ -608,7 +610,7 @@ def provider
 
   # no need to store anything in the db if the password is our default
   def root_pass
-    read_attribute(:root_pass) || hostgroup.try(:root_pass) || Setting[:root_pass]
+    read_attribute(:root_pass).blank? ? (hostgroup.try(:root_pass) || Setting[:root_pass]) : read_attribute(:root_pass)
   end
 
   def clone

app/models/setting.rb

@@ -5,7 +5,7 @@ class Setting < ActiveRecord::Base
   TYPES= %w{ integer boolean hash array string }
   FROZEN_ATTRS = %w{ name category }
   NONZERO_ATTRS = %w{ puppet_interval idle_timeout entries_per_page max_trend }
-  BLANK_ATTRS = %w{ trusted_puppetmaster_hosts login_delegation_logout_url authorize_login_delegation_auth_source_user_autocreate }
+  BLANK_ATTRS = %w{ trusted_puppetmaster_hosts login_delegation_logout_url authorize_login_delegation_auth_source_user_autocreate root_pass }
   URI_ATTRS = %w{ foreman_url unattended_url }
 
   class UriValidator < ActiveModel::EachValidator

app/models/setting/provisioning.rb

@@ -11,7 +11,7 @@ def self.load_defaults
 
     self.transaction do
       [
-        self.set('root_pass', N_("Default encrypted root password on provisioned hosts (default is 123123)"), "$1$default$hCkak1kaJPQILNmYbUXhD0"),
+        self.set('root_pass', N_("Default encrypted root password on provisioned hosts"), nil),
         self.set('unattended_url', N_("The URL that hosts will retrieve templates from during build (normally http as many installers don't support https)"), "http://#{Facter.fqdn}"),
         self.set('safemode_render', N_("Enable safe mode config templates rendering (recommended)"), true),
         self.set('ssl_certificate', N_("SSL Certificate path that Foreman would use to communicate with its proxies"), ssl_cert),

test/fixtures/hosts.yml

@@ -16,6 +16,7 @@ one:
   compute_resource: one
   location: location1
   organization: organization1
+  root_pass: xybxa6JUkz63w
 
 two:
   type: Host::Managed
@@ -32,6 +33,7 @@ two:
   puppet_proxy: puppetmaster
   compute_resource: one
   location: location1
+  root_pass: xybxa6JUkz63w
 
 otherfullhost:
   type: Host::Managed
@@ -46,6 +48,7 @@ otherfullhost:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 anotherfullhost:
   type: Host::Managed
@@ -60,6 +63,7 @@ anotherfullhost:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 myfullhost:
   type: Host::Managed
@@ -73,6 +77,7 @@ myfullhost:
   domain: mydomain
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 redhat:
   type: Host::Managed
@@ -89,6 +94,7 @@ redhat:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 ubuntu:
   type: Host::Managed
@@ -105,6 +111,7 @@ ubuntu:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 # Need a host with both build=true and managed=true for some tests
 ubuntu2:
@@ -123,6 +130,7 @@ ubuntu2:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 minimal:
   type: Host::Managed
@@ -137,6 +145,7 @@ minimal:
   puppet_proxy: puppetmaster
   domain: useless
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 sol10host:
   type: Host::Managed
@@ -152,6 +161,7 @@ sol10host:
   domain: yourdomain
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 suse:
   type: Host::Managed
@@ -168,6 +178,7 @@ suse:
   subnet: one
   puppet_proxy: puppetmaster
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 dhcp:
   type: Host::Managed
@@ -184,6 +195,7 @@ dhcp:
   puppet_ca_proxy: puppetmaster
   managed: true
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 sp_dhcp:
   type: Host::Managed
@@ -199,6 +211,7 @@ sp_dhcp:
   puppet_proxy: puppetmaster
   managed: true
   compute_resource: one
+  root_pass: xybxa6JUkz63w
 
 db_host:
   type: Host::Managed
@@ -215,6 +228,7 @@ db_host:
   managed: true
   compute_resource: one
   hostgroup: db
+  root_pass: xybxa6JUkz63w
 
 owned_by_restricted:
   type: Host::Managed
@@ -234,3 +248,4 @@ owned_by_restricted:
   organization: organization1
   owner: restricted
   owner_type: User
+  root_pass: xybxa6JUkz63w

test/fixtures/settings.yml

@@ -12,8 +12,8 @@ attributes2:
 attributes3:
   name: root_pass
   category: Setting::Provisioning
-  default: xybxa6JUkz63w
-  description: Default root password on provisioned hosts default is 123123
+  default: "--- \n"
+  description: Default root password on provisioned hosts
 attributes4:
   name: safemode_render
   category: Setting::Provisioning

test/functional/api/v1/hosts_controller_test.rb

@@ -11,7 +11,8 @@ def valid_attrs
       :architecture_id     => Architecture.find_by_name('x86_64').id,
       :operatingsystem_id  => Operatingsystem.find_by_name('Redhat').id,
       :puppet_proxy_id     => smart_proxies(:one).id,
-      :compute_resource_id => compute_resources(:one).id
+      :compute_resource_id => compute_resources(:one).id,
+      :root_pass           => "xybxa6JUkz63w"
     }
   end
 

test/functional/api/v2/hosts_controller_test.rb

@@ -11,7 +11,8 @@ def valid_attrs
       :architecture_id     => Architecture.find_by_name('x86_64').id,
       :operatingsystem_id  => Operatingsystem.find_by_name('Redhat').id,
       :puppet_proxy_id     => smart_proxies(:one).id,
-      :compute_resource_id => compute_resources(:one).id
+      :compute_resource_id => compute_resources(:one).id,
+      :root_pass           => "xybxa6JUkz63w"
     }
   end
 

test/functional/hosts_controller_test.rb

@@ -50,7 +50,8 @@ def test_create_valid
           :environment_id => environments(:production).id,
           :subnet_id => subnets(:one).id,
           :disk => "empty partition",
-          :puppet_proxy_id => smart_proxies(:puppetmaster).id
+          :puppet_proxy_id => smart_proxies(:puppetmaster).id,
+          :root_pass           => "xybxa6JUkz63w"
         }
       }, set_session_user
     end

test/unit/host_observer_test.rb

@@ -24,7 +24,7 @@ class HostObserverTest < ActiveSupport::TestCase
       host = Host.create! :name => "foo", :mac => "aabbeeddccff", :ip => "2.3.4.244", :managed => true,
         :build => true, :architecture => architectures(:x86_64), :environment => Environment.first, :puppet_proxy_id => smart_proxies(:one).id,
         :domain => Domain.first, :operatingsystem => operatingsystems(:redhat), :subnet => subnets(:one),
-        :url_options => {:host => 'foreman', :protocol => "http://"}
+        :root_pass => "xybxa6JUkz63w", :url_options => {:host => 'foreman', :protocol => "http://"}
     end
 
     assert host.token.try(:value).present?

test/unit/host_test.rb

@@ -199,6 +199,18 @@ class HostTest < ActiveSupport::TestCase
     assert_nil host
   end
 
+  test "should not save if root password is undefined when the host is managed" do
+    host = Host.new :name => "myfullhost", :managed => true
+    assert !host.valid?
+    assert host.errors[:root_pass].any?
+  end
+
+  test "should save if root password is undefined when the compute resource is image capable" do
+    host = Host.new :name => "myfullhost", :managed => true, :compute_resource_id => compute_resources(:openstack).id
+    host.valid?
+    refute host.errors[:root_pass].any?
+  end
+
   test "should not save if neither ptable or disk are defined when the host is managed" do
     if unattended?
       host = Host.create :name => "myfullhost", :mac => "aabbecddeeff", :ip => "2.4.4.03",
@@ -242,7 +254,7 @@ class HostTest < ActiveSupport::TestCase
     host = Host.new :name => "myfullhost", :mac => "aabbecddeeff", :ip => "2.3.4.03",
       :domain => domains(:mydomain), :operatingsystem => operatingsystems(:redhat), :subnet => subnets(:one), :puppet_proxy => smart_proxies(:puppetmaster),
       :subnet => subnets(:one), :architecture => architectures(:x86_64), :environment => environments(:production), :managed => true,
-      :owner_type => "User"
+      :owner_type => "User", :root_pass => "xybxa6JUkz63w"
     assert host.valid?
   end
 
@@ -614,7 +626,7 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
   end
 
   test "host os attributes must be associated with the host os" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     h.managed = true
     h.architecture = architectures(:sparc)
     assert !h.os.architectures.include?(h.arch)
@@ -623,7 +635,7 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
   end
 
   test "host puppet classes must belong to the host environment" do
-    h = hosts(:redhat)
+    h = hosts(:one)
 
     pc = puppetclasses(:three)
     h.puppetclasses << pc
@@ -642,44 +654,52 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
     assert_equal ["#{pc} does not belong to the #{h.environment} environment"], h.errors[:puppetclasses]
   end
 
+  test "should not allow short root passwords for managed host" do
+    h = hosts(:one)
+    h.root_pass = "2short"
+    h.valid?
+    assert h.errors[:root_pass].include?("should be 8 characters or more")
+  end
+
   test "should allow to save root pw" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     pw = h.root_pass
-    h.root_pass = "token"
+    h.root_pass = "12345678"
     h.hostgroup = nil
-    assert h.save
+    assert h.save!
     assert_not_equal pw, h.root_pass
   end
 
   test "should allow to revert to default root pw" do
-    h = hosts(:redhat)
-    h.root_pass = "token"
-    assert h.save
-    h.root_pass = ""
+    Setting[:root_pass] = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    h = hosts(:one)
+    h.root_pass = "xybxa6JUkz63w"
     assert h.save
+    h.root_pass = nil
+    assert h.save!
     assert_equal h.root_pass, Setting.root_pass
   end
 
   test "should generate a random salt when saving root pw" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     pw = h.root_pass
-    h.root_pass = "token"
     h.hostgroup = nil
-    assert h.save
+    h.root_pass = "xybxa6JUkz63w"
+    assert h.save!
     first = h.root_pass
 
     # Check it's a $.$....$...... enhanced style password
     assert_equal 4, first.split('$').count
     assert first.split('$')[2].size >= 8
 
     # Check it changes
-    h.root_pass = "token"
+    h.root_pass = "12345678"
     assert h.save
     assert_not_equal first.split('$')[2], h.root_pass.split('$')[2]
   end
 
   test "should pass through existing salt when saving root pw" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     pass = "$1$jmUiJ3NW$bT6CdeWZ3a6gIOio5qW0f1"
     h.root_pass = pass
     h.hostgroup = nil
@@ -688,7 +708,7 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
   end
 
   test "should use hostgroup root password" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     h.root_pass = nil
     h.hostgroup = hostgroups(:common)
     assert h.save
@@ -697,7 +717,7 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
   end
 
   test "should use a nested hostgroup parent root password" do
-    h = hosts(:redhat)
+    h = hosts(:one)
     h.root_pass = nil
     h.hostgroup = hg = hostgroups(:common)
     assert h.save
@@ -709,7 +729,8 @@ class Host::Valid < Host::Base ; belongs_to :domain ; end
   end
 
   test "should use settings root password" do
-    h = hosts(:redhat)
+    Setting[:root_pass] = "$1$default$hCkak1kaJPQILNmYbUXhD0"
+    h = hosts(:one)
     h.root_pass = nil
     h.hostgroup = nil
     assert h.save

test/unit/parameter_test.rb

@@ -22,7 +22,8 @@ class ParameterTest < ActiveSupport::TestCase
     hostgroup = Hostgroup.find_or_create_by_name "Common"
     host = Host.create :name => "myfullhost", :mac => "aabbecddeeff", :ip => "123.05.02.03",
     :domain => domain , :operatingsystem => Operatingsystem.first, :hostgroup => hostgroup,
-    :architecture => Architecture.first, :environment => Environment.first, :disk => "empty partition"
+    :architecture => Architecture.first, :environment => Environment.first, :disk => "empty partition",
+    :root_pass => "xybxa6JUkz63w"
 
     assert_equal "cat", host.host_params["animal"]