Skip to content

Commit

Permalink
fixes theforeman#3725, theforeman#4167 - root password validations, r…
Browse files Browse the repository at this point in the history
…emove default password
  • Loading branch information
stbenjam authored and Dominic Cleal committed Feb 5, 2014
1 parent f7d5a22 commit c4bfd47
Show file tree
Hide file tree
Showing 12 changed files with 75 additions and 35 deletions.
10 changes: 4 additions & 6 deletions app/models/concerns/host_common.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions app/models/host/managed.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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) }
Expand Down Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/setting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion app/models/setting/provisioning.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
15 changes: 15 additions & 0 deletions test/fixtures/hosts.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ one:
compute_resource: one
location: location1
organization: organization1
root_pass: xybxa6JUkz63w

two:
type: Host::Managed
Expand All @@ -32,6 +33,7 @@ two:
puppet_proxy: puppetmaster
compute_resource: one
location: location1
root_pass: xybxa6JUkz63w

otherfullhost:
type: Host::Managed
Expand All @@ -46,6 +48,7 @@ otherfullhost:
subnet: one
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

anotherfullhost:
type: Host::Managed
Expand All @@ -60,6 +63,7 @@ anotherfullhost:
subnet: one
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

myfullhost:
type: Host::Managed
Expand All @@ -73,6 +77,7 @@ myfullhost:
domain: mydomain
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

redhat:
type: Host::Managed
Expand All @@ -89,6 +94,7 @@ redhat:
subnet: one
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

ubuntu:
type: Host::Managed
Expand All @@ -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:
Expand All @@ -123,6 +130,7 @@ ubuntu2:
subnet: one
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

minimal:
type: Host::Managed
Expand All @@ -137,6 +145,7 @@ minimal:
puppet_proxy: puppetmaster
domain: useless
compute_resource: one
root_pass: xybxa6JUkz63w

sol10host:
type: Host::Managed
Expand All @@ -152,6 +161,7 @@ sol10host:
domain: yourdomain
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

suse:
type: Host::Managed
Expand All @@ -168,6 +178,7 @@ suse:
subnet: one
puppet_proxy: puppetmaster
compute_resource: one
root_pass: xybxa6JUkz63w

dhcp:
type: Host::Managed
Expand All @@ -184,6 +195,7 @@ dhcp:
puppet_ca_proxy: puppetmaster
managed: true
compute_resource: one
root_pass: xybxa6JUkz63w

sp_dhcp:
type: Host::Managed
Expand All @@ -199,6 +211,7 @@ sp_dhcp:
puppet_proxy: puppetmaster
managed: true
compute_resource: one
root_pass: xybxa6JUkz63w

db_host:
type: Host::Managed
Expand All @@ -215,6 +228,7 @@ db_host:
managed: true
compute_resource: one
hostgroup: db
root_pass: xybxa6JUkz63w

owned_by_restricted:
type: Host::Managed
Expand All @@ -234,3 +248,4 @@ owned_by_restricted:
organization: organization1
owner: restricted
owner_type: User
root_pass: xybxa6JUkz63w
4 changes: 2 additions & 2 deletions test/fixtures/settings.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/functional/api/v1/hosts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion test/functional/api/v2/hosts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
3 changes: 2 additions & 1 deletion test/functional/hosts_controller_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion test/unit/host_observer_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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?
Expand Down
57 changes: 39 additions & 18 deletions test/unit/host_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion test/unit/parameter_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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"]

Expand Down

0 comments on commit c4bfd47

Please sign in to comment.