[PATCH/puppet 2/2] Fix #7106 - last run summary obey specified owner/group/permissions
Brice Figureau
now enforce any specified mode, owner or group.
The defaults have also been changed to 0644 and the running user/group.
This allows to specify other owner/group/mode in puppet.conf:
[agent]
...
lastrunfile = /tmp/lastrun.yml { mode=0664, owner=monitoring }
...
Signed-off-by: Brice Figureau <brice-...@daysofwonder.com>
---
lib/puppet/configurer.rb | 14 +++++++++++---
lib/puppet/defaults.rb | 5 +++--
spec/integration/configurer_spec.rb | 3 +++
spec/unit/configurer_spec.rb | 17 +++++++++--------
4 files changed, 26 insertions(+), 13 deletions(-)
diff --git a/lib/puppet/configurer.rb b/lib/puppet/configurer.rb
index 9f68ca4..970811b 100644
--- a/lib/puppet/configurer.rb
+++ b/lib/puppet/configurer.rb
@@ -180,9 +180,17 @@ class Puppet::Configurer
end
def save_last_run_summary(report)
- Puppet::Util::FileLocking.writelock(Puppet[:lastrunfile], 0660) do |file|
- file.print YAML.dump(report.raw_summary)
- end
+ last_run = Puppet.settings.setting(:lastrunfile)
+ last_run.create = true # force file creation
+
+ resource = last_run.to_resource
+ resource[:content] = YAML.dump(report.raw_summary)
+
+ catalog = Puppet::Resource::Catalog.new("last_run_file")
+ catalog.add_resource(resource)
+ ral = catalog.to_ral
+ ral.host_config = false
+ ral.apply
rescue => detail
puts detail.backtrace if Puppet[:trace]
Puppet.err "Could not save last run local report: #{detail}"
diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index 2a1ded5..5a1c263 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -608,11 +608,12 @@ module Puppet
"Whether to send reports after every transaction."
],
:lastrunfile => { :default => "$statedir/last_run_summary.yaml",
- :mode => 0660,
+ :mode => 0644,
+ :allow_any_owners_groups => true,
:desc => "Where puppet agent stores the last run report summary in yaml format."
},
:lastrunreport => { :default => "$statedir/last_run_report.yaml",
- :mode => 0660,
+ :mode => 0644,
:desc => "Where puppet agent stores the last run report in yaml format."
},
:graph => [false, "Whether to create dot graph files for the different
diff --git a/spec/integration/configurer_spec.rb b/spec/integration/configurer_spec.rb
index 780c9bb..402bf7d 100755
--- a/spec/integration/configurer_spec.rb
+++ b/spec/integration/configurer_spec.rb
@@ -48,10 +48,13 @@ describe Puppet::Configurer do
report.stubs(:save)
Puppet[:lastrunfile] = tmpfile("lastrunfile")
+ Puppet.settings.setting(:lastrunfile).mode = 0666
Puppet[:report] = true
@configurer.run :catalog => @catalog, :report => report
+ File.stat(Puppet[:lastrunfile]).mode.to_s(8).should == "100666"
+
summary = nil
File.open(Puppet[:lastrunfile], "r") do |fd|
summary = YAML.load(fd.read)
diff --git a/spec/unit/configurer_spec.rb b/spec/unit/configurer_spec.rb
index a4b627c..343e1d2 100755
--- a/spec/unit/configurer_spec.rb
+++ b/spec/unit/configurer_spec.rb
@@ -297,29 +297,30 @@ describe Puppet::Configurer, "when sending a report" do
end
describe Puppet::Configurer, "when saving the summary report file" do
+ include PuppetSpec::Files
+
before do
Puppet.settings.stubs(:use).returns(true)
@configurer = Puppet::Configurer.new
- @report = stub 'report'
- @trans = stub 'transaction'
- @lastrunfd = stub 'lastrunfd'
- Puppet::Util::FileLocking.stubs(:writelock).yields(@lastrunfd)
+ @report = stub 'report', :raw_summary => {}
+
+ Puppet[:lastrunfile] = tmpfile('last_run_file')
end
- it "should write the raw summary to the lastrunfile setting value" do
- Puppet::Util::FileLocking.expects(:writelock).with(Puppet[:lastrunfile], 0660)
+ it "should write the last run file" do
@configurer.save_last_run_summary(@report)
+ FileTest.exists?(Puppet[:lastrunfile]).should be_true
end
it "should write the raw summary as yaml" do
@report.expects(:raw_summary).returns("summary")
- @lastrunfd.expects(:print).with(YAML.dump("summary"))
@configurer.save_last_run_summary(@report)
+ File.read(Puppet[:lastrunfile]).should == YAML.dump("summary")
end
it "should log but not fail if saving the last run summary fails" do
- Puppet::Util::FileLocking.expects(:writelock).raises "exception"
+ Puppet[:lastrunfile] = "/dev/null/inexistant"
Puppet.expects(:err)
lambda { @configurer.save_last_run_summary(@report) }.should_not raise_error
end
--
1.7.5.1
Brice Figureau
for any file/directory settings (both from defaults or by speficying
those in the configuration file).
This has been introduced in commit 06fcec to prevent users to
set invalid values.
But for some settings, it might be interesting to use other owners
and groups than root/service.
This patch allows individual settings to remove this restriction by
adding :allow_any_owners_groups boolean property to their defaults.
If this property is false or not present, the default behavior is
used. If this property is true, any combination of owner and groups
is allowed.
Signed-off-by: Brice Figureau <brice-...@daysofwonder.com>
---
lib/puppet/util/settings/file_setting.rb | 35 ++++++++-
spec/unit/util/settings/file_setting_spec.rb | 103 ++++++++++++++++++--------
2 files changed, 103 insertions(+), 35 deletions(-)
diff --git a/lib/puppet/util/settings/file_setting.rb b/lib/puppet/util/settings/file_setting.rb
index edbab1d..62bbca6 100644
--- a/lib/puppet/util/settings/file_setting.rb
+++ b/lib/puppet/util/settings/file_setting.rb
@@ -7,7 +7,30 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
class SettingError < StandardError; end
- attr_accessor :mode, :create
+ attr_accessor :mode, :create, :allow_any_owners_groups
+
+ def initialize(args = {})
+ # treat first allow_any_owners_groups, so that setting owner and groups
+ # can't be happen before this setting.
+ @allow_any_owners_groups = args.delete(:allow_any_owners_groups)
+ super(args)
+ end
+
+ def allowed_groups
+ @allowed_groups = (allow_any_owners_groups ? nil : AllowedGroups)
+ end
+
+ def allowed_groups=(groups)
+ @allowed_groups = groups
+ end
+
+ def allowed_owners
+ @allowed_owners = (allow_any_owners_groups ? nil : AllowedOwners)
+ end
+
+ def allowed_owners=(owners)
+ @allowed_owners = owners
+ end
# Should we create files, rather than just directories?
def create_files?
@@ -15,7 +38,7 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
end
def group=(value)
- unless AllowedGroups.include?(value)
+ if allowed_groups and not allowed_groups.include?(value)
identifying_fields = [desc,name,default].compact.join(': ')
raise SettingError, "Internal error: The :group setting for #{identifying_fields} must be 'service', not '#{value}'"
end
@@ -24,11 +47,12 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
def group
return unless @group
- @settings[:group]
+ return @settings[:group] if allowed_groups
+ @group
end
def owner=(value)
- unless AllowedOwners.include?(value)
+ if allowed_owners and not allowed_owners.include?(value)
identifying_fields = [desc,name,default].compact.join(': ')
raise SettingError, "Internal error: The :owner setting for #{identifying_fields} must be either 'root' or 'service', not '#{value}'"
end
@@ -38,7 +62,8 @@ class Puppet::Util::Settings::FileSetting < Puppet::Util::Settings::Setting
def owner
return unless @owner
return "root" if @owner == "root" or ! use_service_user?
- @settings[:user]
+ return @settings[:user] if allowed_owners
+ @owner
end
def use_service_user?
diff --git a/spec/unit/util/settings/file_setting_spec.rb b/spec/unit/util/settings/file_setting_spec.rb
index dcfb6e3..36e6bc7 100755
--- a/spec/unit/util/settings/file_setting_spec.rb
+++ b/spec/unit/util/settings/file_setting_spec.rb
@@ -38,24 +38,38 @@ describe Puppet::Util::Settings::FileSetting do
end
describe "when setting the owner" do
- it "should allow the file to be owned by root" do
- root_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "root", :desc => "a setting") }
- root_owner.should_not raise_error
- end
+ describe "and using the default allowed owners" do
+ it "should allow the file to be owned by root" do
+ root_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "root", :desc => "a setting") }
+ root_owner.should_not raise_error
+ end
- it "should allow the file to be owned by the service user" do
- service_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "service", :desc => "a setting") }
- service_owner.should_not raise_error
- end
+ it "should allow the file to be owned by the service user" do
+ service_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "service", :desc => "a setting") }
+ service_owner.should_not raise_error
+ end
- it "should allow the ownership of the file to be unspecified" do
- no_owner = lambda { FileSetting.new(:settings => mock("settings"), :desc => "a setting") }
- no_owner.should_not raise_error
+ it "should allow the ownership of the file to be unspecified" do
+ no_owner = lambda { FileSetting.new(:settings => mock("settings"), :desc => "a setting") }
+ no_owner.should_not raise_error
+ end
+
+ it "should not allow other owners" do
+ invalid_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "invalid", :desc => "a setting") }
+ invalid_owner.should raise_error(FileSetting::SettingError)
+ end
end
- it "should not allow other owners" do
- invalid_owner = lambda { FileSetting.new(:settings => mock("settings"), :owner => "invalid", :desc => "a setting") }
- invalid_owner.should raise_error(FileSetting::SettingError)
+ describe "and allowing any owners" do
+ it "should allow the ownership of the file to be unspecified" do
+ no_owner = lambda { FileSetting.new(:settings => mock("settings"), :allow_any_owners_groups => true, :desc => "a setting") }
+ no_owner.should_not raise_error
+ end
+
+ it "should allow any owners" do
+ any_owner = lambda { FileSetting.new(:settings => mock("settings"), :allow_any_owners_groups => true, :owner => "any", :desc => "a setting") }
+ any_owner.should_not raise_error(FileSetting::SettingError)
+ end
end
end
@@ -86,22 +100,44 @@ describe Puppet::Util::Settings::FileSetting do
it "should be nil when the owner is unspecified" do
FileSetting.new(:settings => mock("settings"), :desc => "a setting").owner.should be_nil
end
+
+ it "should be the owner when specified and allowing any owners" do
+ settings = mock("settings")
+
+ setting = FileSetting.new(:settings => settings, :owner => "owner", :allow_any_owners_groups => true, :desc => "a setting")
+ setting.expects(:use_service_user?).returns true
+ setting.owner.should == "owner"
+ end
end
describe "when setting the group" do
- it "should allow the group to be service" do
- service_group = lambda { FileSetting.new(:settings => mock("settings"), :group => "service", :desc => "a setting") }
- service_group.should_not raise_error
- end
+ describe "and using the default allowed owners" do
+ it "should allow the group to be service" do
+ service_group = lambda { FileSetting.new(:settings => mock("settings"), :group => "service", :desc => "a setting") }
+ service_group.should_not raise_error
+ end
- it "should allow the group to be unspecified" do
- no_group = lambda { FileSetting.new(:settings => mock("settings"), :desc => "a setting") }
- no_group.should_not raise_error
+ it "should allow the group to be unspecified" do
+ no_group = lambda { FileSetting.new(:settings => mock("settings"), :desc => "a setting") }
+ no_group.should_not raise_error
+ end
+
+ it "should not allow invalid groups" do
+ invalid_group = lambda { FileSetting.new(:settings => mock("settings"), :group => "invalid", :desc => "a setting") }
+ invalid_group.should raise_error(FileSetting::SettingError)
+ end
end
- it "should not allow invalid groups" do
- invalid_group = lambda { FileSetting.new(:settings => mock("settings"), :group => "invalid", :desc => "a setting") }
- invalid_group.should raise_error(FileSetting::SettingError)
+ describe "and allowing any groups" do
+ it "should allow the group to be unspecified" do
+ no_group = lambda { FileSetting.new(:settings => mock("settings"), :allow_any_owners_groups => true, :desc => "a setting") }
+ no_group.should_not raise_error
+ end
+
+ it "should allow any groups" do
+ any_group = lambda { FileSetting.new(:settings => mock("settings"), :allow_any_owners_groups => true, :group => "any", :desc => "a setting") }
+ any_group.should_not raise_error(FileSetting::SettingError)
+ end
end
end
@@ -114,6 +150,13 @@ describe Puppet::Util::Settings::FileSetting do
it "should be nil when the group is unspecified" do
FileSetting.new(:settings => mock("settings"), :desc => "a setting").group.should be_nil
end
+
+ it "should be the specified group when allowing any groups" do
+ settings = mock("settings")
+
+ setting = FileSetting.new(:settings => settings, :group => "group", :allow_any_owners_groups => true, :desc => "a setting")
+ setting.group.should == "group"
+ end
end
it "should be able to be converted into a resource" do
--
1.7.5.1
Luke Kanies
> It is impossible to set an owner/group different than root or service
> for any file/directory settings (both from defaults or by speficying
> those in the configuration file).
> This has been introduced in commit 06fcec to prevent users to
> set invalid values.
> But for some settings, it might be interesting to use other owners
> and groups than root/service.
>
> This patch allows individual settings to remove this restriction by
> adding :allow_any_owners_groups boolean property to their defaults.
> If this property is false or not present, the default behavior is
> used. If this property is true, any combination of owner and groups
> is allowed.
What's the motivation for this?
The reason I switched this in the first place is that we never actually set anything to any values other than 'root' and the system user, whatever it was.
Do you have settings that specifically need to be another value?
> --
> You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
> To post to this group, send email to puppe...@googlegroups.com.
> To unsubscribe from this group, send email to puppet-dev+...@googlegroups.com.
> For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
>
--
In our civilization, and under our republican form of government,
intelligence is so highly honored that it is rewarded by exemption from
the cares of office. --Ambrose Bierce
---------------------------------------------------------------------
Luke Kanies -|- http://puppetlabs.com -|- http://about.me/lak
Brice Figureau
> On May 28, 2011, at 8:11 AM, Brice Figureau wrote:
>
> > It is impossible to set an owner/group different than root or service
> > for any file/directory settings (both from defaults or by speficying
> > those in the configuration file).
> > This has been introduced in commit 06fcec to prevent users to
> > set invalid values.
> > But for some settings, it might be interesting to use other owners
> > and groups than root/service.
> >
> > This patch allows individual settings to remove this restriction by
> > adding :allow_any_owners_groups boolean property to their defaults.
> > If this property is false or not present, the default behavior is
> > used. If this property is true, any combination of owner and groups
> > is allowed.
>
> What's the motivation for this?
>
> The reason I switched this in the first place is that we never
> actually set anything to any values other than 'root' and the system
> user, whatever it was.
>
> Do you have settings that specifically need to be another value?
Check the second patch in the serie.
We introduced the lastrunfile settings that points to a file containing
a summary of the last puppet agent run. This is the perfect file to be
consummed by mcollective mc puppetd or any monitoring system.
Unfortunately, my original version of this feature created a 0660
root:root file, making this feature a little bit unuseful.
Second problem, despite being a file setting, using the "{mode =
0644, ...}" syntax has no effect on this file (the FileSetting system
only runs when the agent starts).
So I wanted to fix this specific problem and at the same time support
file settings owner/group change through the configuration file. It
proved to not be possible because the only owner/group we support is
root and the service user.
So instead of adding specific settings like "lastrunfileowner" or
"lastrunfilegroup", I decided to instead fix our FileSetting
implementation and have a way to relax the AllowedOwners/Groups. Note
that I was careful to relax those constraints only on the settings that
needed it (namely the one I cared about: lastrunfile). The other
settings are unaffected and if you try to make your cacert.pem file
owner to be "nobody" that won't work as you designed it.
I can backtrack and remove this attempt at fixing the problem. The only
remaining solution I'll have for a useful lastrunfile is to make it 0644
(the good thing is that the patch is then simple).
--
Brice Figureau
Follow the latest Puppet Community evolutions on www.planetpuppet.org!
Luke Kanies
So are we not writing it with the correct modes, then?
The Settings class provides a method for making files, I think, that handles all of the modes correctly and such; maybe the "right" answer is to fix the writing of the summary to use that method?
> So I wanted to fix this specific problem and at the same time support
> file settings owner/group change through the configuration file. It
> proved to not be possible because the only owner/group we support is
> root and the service user.
>
> So instead of adding specific settings like "lastrunfileowner" or
> "lastrunfilegroup", I decided to instead fix our FileSetting
> implementation and have a way to relax the AllowedOwners/Groups. Note
> that I was careful to relax those constraints only on the settings that
> needed it (namely the one I cared about: lastrunfile). The other
> settings are unaffected and if you try to make your cacert.pem file
> owner to be "nobody" that won't work as you designed it.
>
> I can backtrack and remove this attempt at fixing the problem. The only
> remaining solution I'll have for a useful lastrunfile is to make it 0644
> (the good thing is that the patch is then simple).
I guess my only real concern about the patch is that it enables something that I'm afraid will be a one-off. If we're very confident this won't be the only example, then I'm fine, but I'd prefer to avoid it if possible.
--
I take my children everywhere, but they always find their way
back home. --Robert Orben
Brice Figureau
As of now the mode is always 0660, despite what can be configured.
> The Settings class provides a method for making files, I think, that
> handles all of the modes correctly and such; maybe the "right" answer
> is to fix the writing of the summary to use that method?
Which is what I did in the second patch. But I wanted to go further and
allow the user to set a different owner/group, which proved to be
impossible, hence the first patch in the serie :)
> > So I wanted to fix this specific problem and at the same time support
> > file settings owner/group change through the configuration file. It
> > proved to not be possible because the only owner/group we support is
> > root and the service user.
> >
> > So instead of adding specific settings like "lastrunfileowner" or
> > "lastrunfilegroup", I decided to instead fix our FileSetting
> > implementation and have a way to relax the AllowedOwners/Groups. Note
> > that I was careful to relax those constraints only on the settings that
> > needed it (namely the one I cared about: lastrunfile). The other
> > settings are unaffected and if you try to make your cacert.pem file
> > owner to be "nobody" that won't work as you designed it.
> >
> > I can backtrack and remove this attempt at fixing the problem. The only
> > remaining solution I'll have for a useful lastrunfile is to make it 0644
> > (the good thing is that the patch is then simple).
>
> I guess my only real concern about the patch is that it enables
> something that I'm afraid will be a one-off. If we're very confident
> this won't be the only example, then I'm fine, but I'd prefer to avoid
> it if possible.
I understand your concern. I'd argue with a tongue-in-cheekesque "build
it and they will come" :-D
I'll drop the first patch from the serie, that will still allow to set
the mode, which would certainly be enough for most cases.
Luke Kanies
It sounds like the code that writes the summary should be modified to pay attention to the mode when writing.
>> The Settings class provides a method for making files, I think, that
>> handles all of the modes correctly and such; maybe the "right" answer
>> is to fix the writing of the summary to use that method?
>
> Which is what I did in the second patch. But I wanted to go further and
> allow the user to set a different owner/group, which proved to be
> impossible, hence the first patch in the serie :)
Ah, I see.
>>> So I wanted to fix this specific problem and at the same time support
>>> file settings owner/group change through the configuration file. It
>>> proved to not be possible because the only owner/group we support is
>>> root and the service user.
>>>
>>> So instead of adding specific settings like "lastrunfileowner" or
>>> "lastrunfilegroup", I decided to instead fix our FileSetting
>>> implementation and have a way to relax the AllowedOwners/Groups. Note
>>> that I was careful to relax those constraints only on the settings that
>>> needed it (namely the one I cared about: lastrunfile). The other
>>> settings are unaffected and if you try to make your cacert.pem file
>>> owner to be "nobody" that won't work as you designed it.
>>>
>>> I can backtrack and remove this attempt at fixing the problem. The only
>>> remaining solution I'll have for a useful lastrunfile is to make it 0644
>>> (the good thing is that the patch is then simple).
>>
>> I guess my only real concern about the patch is that it enables
>> something that I'm afraid will be a one-off. If we're very confident
>> this won't be the only example, then I'm fine, but I'd prefer to avoid
>> it if possible.
>
> I understand your concern. I'd argue with a tongue-in-cheekesque "build
> it and they will come" :-D
>
> I'll drop the first patch from the serie, that will still allow to set
> the mode, which would certainly be enough for most cases.
Thanks. If we see more than one example of this, we can revisit then.
--
Somebody has to do something, and it's just incredibly pathetic that it
has to be us. --Jerry Garcia