From ccdd043ab309ca382dc949612d7efe3562adf5c5 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Thu, 18 Aug 2011 10:30:29 -0700 Subject: (#8662) Break circular feature dependency The root feature was being evaluated prior to the microsoft_windows feature being defined. On Windows, this had the side-effect of calling Process.uid prior to the win32 sys/admin gem being loaded. And the default ruby implementation of Process.uid always returns 0, which caused Puppet.features.root? to always return true. This commit reorders the syslog, posix, microsoft_windows, and root features due to dependencies among them. This ensures the microsoft_windows feature is defined prior to evaluating the root feature. As a result of this commit, the SUIDManager now calls the win32 sys/admin version of the Process.uid method, which returns the RID component of the user's SID. As this is not 0, Puppet.features.root? will now always return false on Windows. A future commit will fix this, so that Puppet.feature.root? only returns true when running as the Windows-equivalent of root. --- lib/puppet/feature/base.rb | 54 ++++++++++++++++++++++++---------------------- 1 file changed, 28 insertions(+), 26 deletions(-) diff --git a/lib/puppet/feature/base.rb b/lib/puppet/feature/base.rb index 2eddadb7a..cecc1b9ad 100644 --- a/lib/puppet/feature/base.rb +++ b/lib/puppet/feature/base.rb @@ -2,6 +2,34 @@ require 'puppet/util/feature' # Add the simple features, all in one file. +# Order is important as some features depend on others + +# We have a syslog implementation +Puppet.features.add(:syslog, :libs => ["syslog"]) + +# We can use POSIX user functions +Puppet.features.add(:posix) do + require 'etc' + Etc.getpwuid(0) != nil && Puppet.features.syslog? +end + +# We can use Microsoft Windows functions +Puppet.features.add(:microsoft_windows) do + begin + require 'sys/admin' + require 'win32/process' + require 'win32/dir' + require 'win32/service' + require 'win32ole' + require 'win32/api' + true + rescue LoadError => err + warn "Cannot run on Microsoft Windows without the sys-admin, win32-process, win32-dir & win32-service gems: #{err}" unless Puppet.features.posix? + end +end + +raise Puppet::Error,"Cannot determine basic system flavour" unless Puppet.features.posix? or Puppet.features.microsoft_windows? + # We've got LDAP available. Puppet.features.add(:ldap, :libs => ["ldap"]) @@ -30,32 +58,6 @@ Puppet.features.add(:rrd, :libs => ["RRD"]) # We have OpenSSL Puppet.features.add(:openssl, :libs => ["openssl"]) -# We have a syslog implementation -Puppet.features.add(:syslog, :libs => ["syslog"]) - -# We can use POSIX user functions -Puppet.features.add(:posix) do - require 'etc' - Etc.getpwuid(0) != nil && Puppet.features.syslog? -end - -# We can use Microsoft Windows functions -Puppet.features.add(:microsoft_windows) do - begin - require 'sys/admin' - require 'win32/process' - require 'win32/dir' - require 'win32/service' - require 'win32ole' - require 'win32/api' - true - rescue LoadError => err - warn "Cannot run on Microsoft Windows without the sys-admin, win32-process, win32-dir & win32-service gems: #{err}" unless Puppet.features.posix? - end -end - -raise Puppet::Error,"Cannot determine basic system flavour" unless Puppet.features.posix? or Puppet.features.microsoft_windows? - # We have CouchDB Puppet.features.add(:couchdb, :libs => ["couchrest"]) -- cgit From 2ac87905708ddbc44d212e10e34d72cad09e3271 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Thu, 18 Aug 2011 10:34:18 -0700 Subject: (#8662) Fix Puppet.features.root? on Windows This commit changes Puppet::Util::SUIDManager.root? (and Puppet.features.root?) to only return true if the user is running with elevated privileges (granted via UAC). If this check fails because elevated privileges are not supported, e.g. pre-Vista, then we fall back to checking if the user is a member of the builtin Administrators group. This means if you are logged in as Administrator on 2008, Puppet.features.root? will return false, unless you are explicitly running puppet as an administrator, e.g. runas /user:Administrator "puppet apply manifest.pp" This commit also adds tests to ensure SUIDManager.asuser is a no-op on Windows, since Windows does not (easily) support switching user contexts without providing a password. --- lib/puppet/util/suidmanager.rb | 15 ++++++- spec/unit/util/suidmanager_spec.rb | 89 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 103 insertions(+), 1 deletion(-) diff --git a/lib/puppet/util/suidmanager.rb b/lib/puppet/util/suidmanager.rb index 697bce111..d2772002e 100644 --- a/lib/puppet/util/suidmanager.rb +++ b/lib/puppet/util/suidmanager.rb @@ -37,7 +37,20 @@ module Puppet::Util::SUIDManager module_function :groups= def self.root? - Process.uid == 0 + return Process.uid == 0 unless Puppet.features.microsoft_windows? + + require 'sys/admin' + require 'win32/security' + + # if Vista or later, check for unrestricted process token + begin + return Win32::Security.elevated_security? + rescue Win32::Security::Error => e + raise e unless e.to_s =~ /Incorrect function/i + end + + group = Sys::Admin.get_group("Administrators", :sid => Win32::Security::SID::BuiltinAdministrators) + group and group.members.index(Sys::Admin.get_login) != nil end # Runs block setting uid and gid if provided then restoring original ids diff --git a/spec/unit/util/suidmanager_spec.rb b/spec/unit/util/suidmanager_spec.rb index abfe3f723..474d0b2a2 100755 --- a/spec/unit/util/suidmanager_spec.rb +++ b/spec/unit/util/suidmanager_spec.rb @@ -66,6 +66,14 @@ describe Puppet::Util::SUIDManager do xids.should be_empty end + + it "should not get or set euid/egid on Windows" do + Puppet.features.stubs(:microsoft_windows?).returns true + + Puppet::Util::SUIDManager.asuser(user[:uid], user[:gid]) {} + + xids.should be_empty + end end describe "#change_group" do @@ -195,6 +203,15 @@ describe Puppet::Util::SUIDManager do xids.should be_empty end + + it "should not get or set euid/egid on Windows" do + Puppet.features.stubs(:microsoft_windows?).returns true + Kernel.expects(:system).with('blah') + + Puppet::Util::SUIDManager.system('blah', user[:uid], user[:gid]) + + xids.should be_empty + end end describe "with #run_and_capture" do @@ -210,4 +227,76 @@ describe Puppet::Util::SUIDManager do end end end + + describe "#root?" do + describe "on POSIX systems" do + before :each do + Puppet.features.stubs(:posix?).returns(true) + Puppet.features.stubs(:microsoft_windows?).returns(false) + end + + it "should be root if uid is 0" do + Process.stubs(:uid).returns(0) + + Puppet::Util::SUIDManager.should be_root + end + + it "should not be root if uid is not 0" do + Process.stubs(:uid).returns(1) + + Puppet::Util::SUIDManager.should_not be_root + end + end + + describe "on Microsoft Windows", :if => Puppet.features.microsoft_windows? do + describe "2003 without UAC" do + it "should be root if user is a member of the Administrators group" do + Win32::Security.stubs(:elevated_security?).raises(Win32::Security::Error, "Incorrect function.") + Sys::Admin.stubs(:get_login).returns("Administrator") + Sys::Group.stubs(:members).returns(%w[Administrator]) + + Puppet::Util::SUIDManager.should be_root + end + + it "should not be root if the process is running as Guest" do + Win32::Security.stubs(:elevated_security?).raises(Win32::Security::Error, "Incorrect function.") + Sys::Admin.stubs(:get_login).returns("Guest") + Sys::Group.stubs(:members).returns([]) + + Puppet::Util::SUIDManager.should_not be_root + end + + it "should raise an exception if the process fails to open the process token" do + Win32::Security.stubs(:elevated_security?).raises(Win32::Security::Error, "Access denied.") + Sys::Admin.stubs(:get_login).returns("Administrator") + Sys::Group.expects(:members).never + + lambda { Puppet::Util::SUIDManager.should raise_error(Win32::Security::Error, /Access denied./) } + end + end + + describe "2008 with UAC" do + it "should be root if user is running with elevated privileges" do + Win32::Security.stubs(:elevated_security?).returns(true) + Sys::Admin.expects(:get_login).never + + Puppet::Util::SUIDManager.should be_root + end + + it "should not be root if user is not running with elevated privileges" do + Win32::Security.stubs(:elevated_security?).returns(false) + Sys::Admin.expects(:get_login).never + + Puppet::Util::SUIDManager.should_not be_root + end + + it "should raise an exception if the process fails to open the process token" do + Win32::Security.stubs(:elevated_security?).raises(Win32::Security::Error, "Access denied.") + Sys::Admin.expects(:get_login).never + + lambda { Puppet::Util::SUIDManager.should raise_error(Win32::Security::Error, /Access denied./) } + end + end + end + end end -- cgit From 47058abc0c5647d59b0dd21181e67dbfdd908292 Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Thu, 18 Aug 2011 10:35:50 -0700 Subject: (#8662) Skip user and group resources when applying settings on Windows When running as root, puppet will generate a catalog from its settings to create the various directories, e.g. var, ssl. If mkusers is true and a setting implements owner and/or group methods, then puppet will automatically add user and group resources to the catalog (provided the user name is not root and the group names are not root or wheel). This functionality will not be supported on Windows, and so this step is skipped. --- lib/puppet/util/settings.rb | 1 + spec/unit/util/settings_spec.rb | 19 +++++++++++++++++++ 2 files changed, 20 insertions(+) diff --git a/lib/puppet/util/settings.rb b/lib/puppet/util/settings.rb index caaf61b7b..3039a7b0a 100644 --- a/lib/puppet/util/settings.rb +++ b/lib/puppet/util/settings.rb @@ -780,6 +780,7 @@ if @config.include?(:run_mode) # Create the transportable objects for users and groups. def add_user_resources(catalog, sections) return unless Puppet.features.root? + return if Puppet.features.microsoft_windows? return unless self[:mkusers] @config.each do |name, setting| diff --git a/spec/unit/util/settings_spec.rb b/spec/unit/util/settings_spec.rb index 76f229c0f..69c117f28 100755 --- a/spec/unit/util/settings_spec.rb +++ b/spec/unit/util/settings_spec.rb @@ -720,9 +720,28 @@ describe Puppet::Util::Settings do @settings.to_catalog end + describe "on Microsoft Windows" do + before :each do + Puppet.features.stubs(:root?).returns true + Puppet.features.stubs(:microsoft_windows?).returns true + + @settings.setdefaults :foo, :mkusers => [true, "e"], :user => ["suser", "doc"], :group => ["sgroup", "doc"] + @settings.setdefaults :other, :otherdir => {:default => "/otherdir", :desc => "a", :owner => "service", :group => "service"} + + @catalog = @settings.to_catalog + end + + it "it should not add users and groups to the catalog" do + @catalog.resource(:user, "suser").should be_nil + @catalog.resource(:group, "sgroup").should be_nil + end + end + describe "when adding users and groups to the catalog" do before do Puppet.features.stubs(:root?).returns true + Puppet.features.stubs(:microsoft_windows?).returns false + @settings.setdefaults :foo, :mkusers => [true, "e"], :user => ["suser", "doc"], :group => ["sgroup", "doc"] @settings.setdefaults :other, :otherdir => {:default => "/otherdir", :desc => "a", :owner => "service", :group => "service"} -- cgit From 0f207a8ea61205ba6b47e8da429ab3887b3cda4d Mon Sep 17 00:00:00 2001 From: Josh Cooper Date: Mon, 22 Aug 2011 09:26:33 -0700 Subject: (#8662) Don't manage internal file permissions on Windows When running as root, puppet will by default manage internal file permissions for file-related settings. However, ruby does not support chown/chgrp functionality on Windows, so puppet will fail to run (puppet apply generates an exception while trying to set the owner, etc). This commit disables internal file permissions handling on Windows until we add support for chown (at least) as part of the larger file type effort on Windows. --- lib/puppet/util/settings/file_setting.rb | 3 ++- spec/unit/util/settings/file_setting_spec.rb | 22 ++++++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/lib/puppet/util/settings/file_setting.rb b/lib/puppet/util/settings/file_setting.rb index 0fa65d846..f02a0c547 100644 --- a/lib/puppet/util/settings/file_setting.rb +++ b/lib/puppet/util/settings/file_setting.rb @@ -93,7 +93,8 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting if Puppet[:manage_internal_file_permissions] resource[:mode] = self.mode if self.mode - if Puppet.features.root? + # REMIND fails on Windows because chown/chgrp functionality not supported yet + if Puppet.features.root? and !Puppet.features.microsoft_windows? resource[:owner] = self.owner if self.owner resource[:group] = self.group if self.group end diff --git a/spec/unit/util/settings/file_setting_spec.rb b/spec/unit/util/settings/file_setting_spec.rb index 01d891f08..6344cf1b5 100755 --- a/spec/unit/util/settings/file_setting_spec.rb +++ b/spec/unit/util/settings/file_setting_spec.rb @@ -189,6 +189,8 @@ describe Puppet::Util::Settings::FileSetting do it "should set the owner if running as root and the owner is provided" do Puppet.features.expects(:root?).returns true + Puppet.features.stubs(:microsoft_windows?).returns false + @file.stubs(:owner).returns "foo" @file.to_resource[:owner].should == "foo" end @@ -203,6 +205,8 @@ describe Puppet::Util::Settings::FileSetting do it "should set the group if running as root and the group is provided" do Puppet.features.expects(:root?).returns true + Puppet.features.stubs(:microsoft_windows?).returns false + @file.stubs(:group).returns "foo" @file.to_resource[:group].should == "foo" end @@ -218,16 +222,34 @@ describe Puppet::Util::Settings::FileSetting do it "should not set owner if not running as root" do Puppet.features.expects(:root?).returns false + Puppet.features.stubs(:microsoft_windows?).returns false @file.stubs(:owner).returns "foo" @file.to_resource[:owner].should be_nil end it "should not set group if not running as root" do Puppet.features.expects(:root?).returns false + Puppet.features.stubs(:microsoft_windows?).returns false @file.stubs(:group).returns "foo" @file.to_resource[:group].should be_nil end + describe "on Microsoft Windows systems" do + before :each do + Puppet.features.stubs(:microsoft_windows?).returns true + end + + it "should not set owner" do + @file.stubs(:owner).returns "foo" + @file.to_resource[:owner].should be_nil + end + + it "should not set group" do + @file.stubs(:group).returns "foo" + @file.to_resource[:group].should be_nil + end + end + it "should set :ensure to the file type" do @file.expects(:type).returns :directory @file.to_resource[:ensure].should == :directory -- cgit