Looking over Red Hat bug #495096¹, I'm wondering in what cases the 1777 mode for $rundir is needed. As far as I can tell from reading the code and commit messages, it's used to avoid requiring a puppet user and group, as well as for the tests?
In the Fedora/EPEL packages, I think we'd be better off avoiding setting 1777, since we know that we'll have a puppet user/group and keeping the permissions as tight as we can helps everyone sleep a little better.
But I'm not really sure I understand all of the reasons and use cases where 1777 makes sense, so I'd like to learn more before submitting any real patches. I was thinking of something like this perhaps (which is based on the 0.24.x branch, just for testing package patches):
diff --git c/lib/puppet/defaults.rb w/lib/puppet/defaults.rb index e36dd70..2f6c1ff 100644 --- c/lib/puppet/defaults.rb +++ w/lib/puppet/defaults.rb @@ -39,14 +39,25 @@ module Puppet logopts = ["$vardir/log", "The Puppet log directory."] end setdefaults(:main, :logdir => logopts) - + # This name hackery is necessary so that the rundir is set reasonably during # unit tests. - if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(self.name) - rundir = "/var/run/puppet" + if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(File.basename($0)) + runopts = { + :default => "/var/run/puppet", + :mode => 0755, + :owner => "$user", + :group => "$group", + :desc => "Where Puppet PID files are kept." + } else - rundir = "$vardir/run" + runopts = { + :default => "$vardir/run", + :mode => 01777, + :desc => "Where Puppet PID files are kept." + } end + setdefaults(:main, :rundir => runopts)
self.setdefaults(:main, :trace => [false, "Whether to print stack traces on some errors"], @@ -66,11 +77,6 @@ module Puppet :owner => "root", :desc => "Where SSL certificates are kept." }, - :rundir => { - :default => rundir, - :mode => 01777, - :desc => "Where Puppet PID files are kept." - }, :genconfig => [false, "Whether to just print a configuration to stdout and exit. Only makes sense when used interactively. Takes into account arguments specified
I ran the spec task with this and didn't notice any failures related to the changes, but then, I did it on a system where the puppet user exists (though I ran the spec task as a normal user).
I probably just lack a clue, but I couldn't figure out when self.name would ever match puppetd or puppetmasterd in the if Process.uid == 0 test. That's why I used the ugly File.basename($0) part.
If anyone can help clue me in on what is wrong with this idea and/or the specific diff above, I'd appreciate it.
-- Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Whenever you find yourself on the side of the majority, it is time to pause and reflect. -- Mark Twain
On May 31, 2009, at 11:29 AM, Todd Zullinger wrote:
> Looking over Red Hat bug #495096¹, I'm wondering in what cases the > 1777 mode for $rundir is needed. As far as I can tell from reading > the code and commit messages, it's used to avoid requiring a puppet > user and group, as well as for the tests?
> In the Fedora/EPEL packages, I think we'd be better off avoiding > setting 1777, since we know that we'll have a puppet user/group and > keeping the permissions as tight as we can helps everyone sleep a > little better.
> But I'm not really sure I understand all of the reasons and use cases > where 1777 makes sense, so I'd like to learn more before submitting > any real patches. I was thinking of something like this perhaps > (which is based on the 0.24.x branch, just for testing package > patches):
Essentially, yes, it's used to make it so we don't require the 'puppet' user and group on the client.
I know that RHEL et al include them in the package, but e.g., gems don't have them. I've accidentally done things that require them, and it's always filed very quickly as a bug, so it's clear that people at least aren't expecting it.
We could see about changing the requirements so that even the client need a puppet user and group.
> diff --git c/lib/puppet/defaults.rb w/lib/puppet/defaults.rb > index e36dd70..2f6c1ff 100644 > --- c/lib/puppet/defaults.rb > +++ w/lib/puppet/defaults.rb > @@ -39,14 +39,25 @@ module Puppet > logopts = ["$vardir/log", "The Puppet log directory."] > end > setdefaults(:main, :logdir => logopts) > - > + > # This name hackery is necessary so that the rundir is set > reasonably during > # unit tests. > - if Process.uid == 0 and %w{puppetd puppetmasterd}.include? > (self.name) > - rundir = "/var/run/puppet" > + if Process.uid == 0 and %w{puppetd puppetmasterd}.include? > (File.basename($0)) > + runopts = { > + :default => "/var/run/puppet", > + :mode => 0755, > + :owner => "$user", > + :group => "$group", > + :desc => "Where Puppet PID files are kept." > + } > else > - rundir = "$vardir/run" > + runopts = { > + :default => "$vardir/run", > + :mode => 01777, > + :desc => "Where Puppet PID files are kept." > + } > end > + setdefaults(:main, :rundir => runopts)
At this point, I'd only consider this if it had a special case for those platforms or situations where you could guarantee that the user/ group were there. Either that, or have this patch applied by those packaging systems. Long term, yeah, it makes more sense to see about making the user/group required.
> self.setdefaults(:main, > :trace => [false, "Whether to print stack traces on some > errors"], > @@ -66,11 +77,6 @@ module Puppet > :owner => "root", > :desc => "Where SSL certificates are kept." > }, > - :rundir => { > - :default => rundir, > - :mode => 01777, > - :desc => "Where Puppet PID files are kept." > - }, > :genconfig => [false, > "Whether to just print a configuration to stdout and > exit. Only makes > sense when used interactively. Takes into account > arguments specified
> I ran the spec task with this and didn't notice any failures related > to the changes, but then, I did it on a system where the puppet user > exists (though I ran the spec task as a normal user).
> I probably just lack a clue, but I couldn't figure out when self.name > would ever match puppetd or puppetmasterd in the if Process.uid == 0 > test. That's why I used the ugly File.basename($0) part.
> If anyone can help clue me in on what is wrong with this idea and/or > the specific diff above, I'd appreciate it.
> -- > Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > Whenever you find yourself on the side of the majority, it is time to > pause and reflect. > -- Mark Twain
-- Measure with a micrometer. Mark with chalk. Cut with an axe. --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com
Luke Kanies wrote: > Essentially, yes, it's used to make it so we don't require the > 'puppet' user and group on the client.
> I know that RHEL et al include them in the package, but e.g., gems > don't have them. I've accidentally done things that require them, > and it's always filed very quickly as a bug, so it's clear that > people at least aren't expecting it.
> We could see about changing the requirements so that even the client > need a puppet user and group.
Alternately, would it be possible and not too much overhead to check for the existence of $user and $group in defaults.rb and only set 1777 if not present?
(Not that I'm in the camp who would oppose just requiring $user and $group to exist.)
> At this point, I'd only consider this if it had a special case for > those platforms or situations where you could guarantee that the > user/ group were there. Either that, or have this patch applied by > those packaging systems. Long term, yeah, it makes more sense to > see about making the user/group required.
It's not a big deal if the Fedora/EPEL packages patch this out, if that ends up being the best thing to do for now.
Thanks for the explanation. :)
-- Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Never do today that which will become someone else's responsibility tomorrow.
On May 31, 2009, at 6:42 PM, Todd Zullinger wrote:
> Luke Kanies wrote: >> Essentially, yes, it's used to make it so we don't require the >> 'puppet' user and group on the client.
>> I know that RHEL et al include them in the package, but e.g., gems >> don't have them. I've accidentally done things that require them, >> and it's always filed very quickly as a bug, so it's clear that >> people at least aren't expecting it.
>> We could see about changing the requirements so that even the client >> need a puppet user and group.
> Alternately, would it be possible and not too much overhead to check > for the existence of $user and $group in defaults.rb and only set 1777 > if not present?
They're always in defaults.rb, they're just not always present on the machine.
It wouldn't be all that much overhead to check if they're present on the machine, but it could possibly get a bit self-referential, since it'd make the most sense to just use a 'user' and 'group' instance. I expect Settings could relatively easily be extended to tell you whether the user and group exist, since it knows how to create them and everything.
> (Not that I'm in the camp who would oppose just requiring $user and > $group to exist.)
>> At this point, I'd only consider this if it had a special case for >> those platforms or situations where you could guarantee that the >> user/ group were there. Either that, or have this patch applied by >> those packaging systems. Long term, yeah, it makes more sense to >> see about making the user/group required.
> It's not a big deal if the Fedora/EPEL packages patch this out, if > that ends up being the best thing to do for now.
> Thanks for the explanation. :)
No problem.
-- SCSI is *not* magic. There are fundamental technical reasons why it is necessary to sacrifice a young goat to your SCSI chain now and then. --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com
> On May 31, 2009, at 6:42 PM, Todd Zullinger wrote: >> Alternately, would it be possible and not too much overhead to check >> for the existence of $user and $group in defaults.rb and only set 1777 >> if not present?
> They're always in defaults.rb, they're just not always present on the > machine.
Yeah, that's what I meant. ;)
> It wouldn't be all that much overhead to check if they're present on > the machine, but it could possibly get a bit self-referential, since > it'd make the most sense to just use a 'user' and 'group' instance. I > expect Settings could relatively easily be extended to tell you > whether the user and group exist, since it knows how to create them > and everything.
Doing that properly is likely not within my Ruby limited skill set. But perhaps setting a more restrictive mode when running popped or puppetmasterd as root would be a reasonable step for now? After looking at the code a bit more, I think 'self.name' should be 'name' in the line:
if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(self.name)
Otherwise, I've never seen self.name contain anything but Puppet. If I'm not just missing the point entirely, would making that change and then setting the rundir mode appropriately in that if/else work well for the unit tests and folks without a puppet user/group?
# This name hackery is necessary so that the rundir is set reasonably during # unit tests. - if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(self.name) + if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(name) rundir = "/var/run/puppet" + rundirmode = 0755 else rundir = "$vardir/run" + rundirmode = 01777 end
-- Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ Deliberation, n.: The act of examining one's bread to determine which side it is buttered on. -- Ambrose Bierce, "The Devil's Dictionary"
>> On May 31, 2009, at 6:42 PM, Todd Zullinger wrote:
>>> Alternately, would it be possible and not too much overhead to check
>>> for the existence of $user and $group in defaults.rb and only set
>>> 1777
>>> if not present?
>> They're always in defaults.rb, they're just not always present on the
>> machine.
> Yeah, that's what I meant. ;)
>> It wouldn't be all that much overhead to check if they're present on
>> the machine, but it could possibly get a bit self-referential, since
>> it'd make the most sense to just use a 'user' and 'group'
>> instance. I
>> expect Settings could relatively easily be extended to tell you
>> whether the user and group exist, since it knows how to create them
>> and everything.
> Doing that properly is likely not within my Ruby limited skill set.
> But perhaps setting a more restrictive mode when running popped or
> puppetmasterd as root would be a reasonable step for now?
The problem is that you then have puppetmasterd (which never runs as
root) warring with puppetd (which always does).
The only real way to do it is to check to see if the puppet user
exists, and behave differently then.
I'd settle for a shell-out hack or something, I guess, but this is
inherently messy and self-referential, like I said.
> After
> looking at the code a bit more, I think 'self.name' should be 'name'
> in the line:
> if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(self.name)
Ok.
> Otherwise, I've never seen self.name contain anything but Puppet. If
> I'm not just missing the point entirely, would making that change
> and then
> setting the rundir mode appropriately in that if/else work well for
> the unit tests and folks without a puppet user/group?
It's all about the problem of running puppetmasterd and puppetd on the
same host. If you resolve that, then essentially everything else goes
away.
> # This name hackery is necessary so that the rundir is set
> reasonably during
> # unit tests.
> - if Process.uid == 0 and %w{puppetd puppetmasterd}.include? > (self.name)
> + if Process.uid == 0 and %w{puppetd puppetmasterd}.include?(name)
> rundir = "/var/run/puppet"
> + rundirmode = 0755
> else
> rundir = "$vardir/run"
> + rundirmode = 01777
> end
> -- > Todd OpenPGP -> KeyID: 0xBEAF0CE3 | URL: www.pobox.com/~tmz/pgp > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> Deliberation, n.:
> The act of examining one's bread to determine which side it is
> buttered on.
> -- Ambrose Bierce, "The Devil's Dictionary"
-- I went to a restaurant that serves "breakfast at anytime". So I
ordered French Toast during the Renaissance. -- Stephen Wright
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com
> On Jun 6, 2009, at 4:40 PM, Todd Zullinger wrote:
In the case of these downstream packages, setting a mode of 0755, ownership puppet:puppet on the RPM level would suffice for all of our branches. It enables the puppet user (puppetmasterd) to write (and the puppet user is available on the system due to packaging), as well as the root user (puppetd).
I'm very much interested to continue down the road of checking for a puppet user/group to exist on the machine on which puppet is deployed, but I'm afraid I'm too unfamiliar with Luke's vision on how things are supposed to work to come up with a proper patch.
Am I understanding correctly that;
- We can make $vardir/run mode 0755 if the puppet user exists, - We can make $vardir/run mode 0775 if (only) the puppet group exists, - puppetd (running as user root) can always write to $vardir/puppet and so does not require any additional permissions, and - puppetmasterd requires 01777 when no puppet user or group exists.
> On 06/10/2009 09:56 PM, Luke Kanies wrote: >> On Jun 6, 2009, at 4:40 PM, Todd Zullinger wrote:
> In the case of these downstream packages, setting a mode of 0755, > ownership puppet:puppet on the RPM level would suffice for all of our > branches. It enables the puppet user (puppetmasterd) to write (and the > puppet user is available on the system due to packaging), as well as > the > root user (puppetd).
> I'm very much interested to continue down the road of checking for a > puppet user/group to exist on the machine on which puppet is deployed, > but I'm afraid I'm too unfamiliar with Luke's vision on how things are > supposed to work to come up with a proper patch.
It's not so much that I have a vision as that there are just practical considerations.
> Am I understanding correctly that;
> - We can make $vardir/run mode 0755 if the puppet user exists, > - We can make $vardir/run mode 0775 if (only) the puppet group exists, > - puppetd (running as user root) can always write to $vardir/puppet > and > so does not require any additional permissions, and > - puppetmasterd requires 01777 when no puppet user or group exists.
> Is this the logic we're after? Thanks in advance,
Essentially yes, although puppetmasterd essentially requires read/ write access, not 1777 specifically.
-- Silence is a text easy to misread. -- A. A. Attanasio, 'The Eagle and the Sword' --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com
