Omniscient crontab provider?

[Felix Frank]

Hi,

I've been looking at the redmine ticket in [1], which basically asks for
the ability to purge crontab entries from *all* users' crontabs. As is,
puppet will purge only from such crontab files that contain entries that
are explicitly managed.

There is a similar request for ssh_authorized_keys, which is just about
at the top of the highly voted issues (see [2]).

For crontab, I cobbled together a patch (see [1]), but I'm loath to go
so far as to add tests and open a pull request, because I'm not sure
that the idea of changing the crontab provider in this way is sound. The
approach is simply to add each account found in /etc/passwd to the
target list of the provider.

For the purge=>true use case, parsing all available crontabs honors the
principle of least surprise. In other cases though, the opposite may be
true (but then, I can't think of any examples off the top of my head).
Point in case, there are unit tests that break when applying the patch:

1) Puppet::Type::Cron::ProviderCrontab.instances on linux should be
empty if user has no crontab
2) Puppet::Type::Cron::ProviderCrontab.instances on linux should be
empty if user is not present

I'm not sure how vital it is to keep those around (i.e., what breaks
along with these assumptions?)

A similar change can be made to the SSH key provider, which I briefly
considered, but that incurred a lot more breakage. Will revisit that
later, depending on where this discussion leads.

Sorry for the long read and thanks in advance for any feedback.

Cheers,
Felix

[1] https://projects.puppetlabs.com/issues/11383
[2] https://projects.puppetlabs.com/issues/1581

[Andy Parker]

On Wed, Nov 20, 2013 at 3:09 PM, Felix Frank <Felix...@alumni.tu-berlin.de> wrote:

Hi,

I've been looking at the redmine ticket in [1], which basically asks for
the ability to purge crontab entries from *all* users' crontabs. As is,
puppet will purge only from such crontab files that contain entries that
are explicitly managed.

I can understand the desire, but it seems like a very dangerous thing, maybe making it available but not immediately present would be a good way of doing it? We have the resources type that could be extended or there could be another crontab type created to purge.

I'm not certain about either of these, but thought I'd chime in with those suggestions :)

 

--
You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to puppet-dev+...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/puppet-dev/528D412D.6090402%40Alumni.TU-Berlin.de.
For more options, visit https://groups.google.com/groups/opt_out.

--

Andrew Parker

an...@puppetlabs.com

Freenode: zaphod42

Twitter: @aparker42

Software Developer

Join us at PuppetConf 2014, September 23-24 in San Francisco

[John Bollinger]

On Thursday, November 21, 2013 10:15:52 AM UTC-6, Andy Parker wrote:

On Wed, Nov 20, 2013 at 3:09 PM, Felix Frank <Felix...@alumni.tu-berlin.de> wrote:

Hi,

I've been looking at the redmine ticket in [1], which basically asks for
the ability to purge crontab entries from *all* users' crontabs. As is,
puppet will purge only from such crontab files that contain entries that
are explicitly managed.

I can understand the desire, but it seems like a very dangerous thing, maybe making it available but not immediately present would be a good way of doing it? We have the resources type that could be extended or there could be another crontab type created to purge.

I'm not certain about either of these, but thought I'd chime in with those suggestions :)

 

There is a similar request for ssh_authorized_keys, which is just about
at the top of the highly voted issues (see [2]).

There are indeed similarities here with Ssh_authorized_key, but also some important differences.  Consider that Ssh_authorized_key can manage any file as a keystore (see the 'target' parameter).  As such, it is flatly impossible for Puppet to reliably determine from which files it must purge keys to zap them all without collateral damage.  Puppet could be conservative by only purging from files having the standard name, but if Ssh_authorized_key.target has any use in the first place then a conservative purge would miss some keys in some environments.  I think it would be worse to provide a key purging feature that doesn't do the full job than to omit key purging altogether.

I think a more feasible reference model for key purging is the one provided by recursively-managed Files.  Essentially, there is a container (a File resource) that establishes the scope of the purge.  There is not currently a suitable container type for authorized keys, but one could certainly be created, say Ssh_authorized_keyfile or Ssh_authorized_keys.

 

For crontab, I cobbled together a patch (see [1]), but I'm loath to go
so far as to add tests and open a pull request, because I'm not sure
that the idea of changing the crontab provider in this way is sound. The
approach is simply to add each account found in /etc/passwd to the
target list of the provider.

What about systems that use other user databases, either instead of or in addition to /etc/password?  For example, LDAP or NIS or even winbind?

 

For the purge=>true use case, parsing all available crontabs honors the
principle of least surprise. In other cases though, the opposite may be
true (but then, I can't think of any examples off the top of my head).

Part of the issue there is that the Cron resource's 'target' parameter is effectively part of a Cron resource's identifier.  Puppet must take the target into account as it matches existing resources to declared ones to make either purging or ordinary Cron resource management work correctly.

On second thought, though, I guess match failures are not harmful for purging; they just turn resource updates into (remove, add) pairs in that case.  And they may not be a new phenomenon for managed Cron resources.

 

Point in case, there are unit tests that break when applying the patch:

  1) Puppet::Type::Cron::ProviderCrontab.instances on linux should be
empty if user has no crontab
  2) Puppet::Type::Cron::ProviderCrontab.instances on linux should be
empty if user is not present

I'm not sure how vital it is to keep those around (i.e., what breaks
along with these assumptions?)

I'm not sure whether anything breaks, but surely those particular tests fly directly in the face of the change described.  It may be that they simply are rendered incorrect.  If the change is acceptable then the corresponding rules under the revised behavior would be more like:

1) On Linux, if a user has no crontab then Puppet::Type::Cron::ProviderCrontab.instances should contain no instances having that user as their target
2) The target of every element of Puppet::Type::Cron::ProviderCrontab.instances should be an existing system user.


John

[Felix Frank]

Hi John,

thanks for your elaborate-as-always insights and comments.

Ere we digress too much, I saw fit to move the ssh centric bits to a new
thread over on the users' list.

> For crontab, I cobbled together a patch (see [1]), but I'm
> loath to go
> so far as to add tests and open a pull request, because I'm
> not sure
> that the idea of changing the crontab provider in this way is
> sound. The
> approach is simply to add each account found in /etc/passwd to the
> target list of the provider.
>
>
>
> What about systems that use other user databases, either instead of or
> in addition to /etc/password? For example, LDAP or NIS or even winbind?

That's an excellent point. I see two ways to deal with this.

1. "Somehow" take advantage of the available slew of user providers,
most (all?) of which support enumerating the available users. We would
probably need to generate user resources. To get from there to cron
resources, the user type would likely need to generate additional (cron)
resources.
Stuffing the transaction with otherwise useless (and likely unwanted)
user resources seems wasteful and dangerous, though.
I don't see the cron type making direct use of the user providers to get
at the list of users.

2. Add logic to enumerate actual crontabs. This would be somewhat ugly,
because it's quite different from the current paradigm of "work with
usernames only and access the crontab files through the crontab interface".

Alternatives? Thoughts? (This apple appears more rotten the deeper we cut.)

>
>
> For the purge=>true use case, parsing all available crontabs
> honors the
> principle of least surprise. In other cases though, the
> opposite may be
> true (but then, I can't think of any examples off the top of
> my head).
>
>
>
> Part of the issue there is that the Cron resource's 'target' parameter
> is effectively part of a Cron resource's identifier. Puppet must take
> the target into account as it matches existing resources to declared
> ones to make either purging or ordinary Cron resource management work
> correctly.
>
> On second thought, though, I guess match failures are not harmful for
> purging; they just turn resource updates into (remove, add) pairs in
> that case. And they may not be a new phenomenon for managed Cron
> resources.

That would likely work out fine indeed.

Since crontabs are manipulated through crontab(1) anyway, it seems
unlikely that we will be able to purge without determining *some*
username to a given crontab entry, and the chances for getting that name
wrong seem slim to me.

> Point in case, there are unit tests that break when applying
> the patch:
>
> 1) Puppet::Type::Cron::ProviderCrontab.instances on linux
> should be
> empty if user has no crontab
> 2) Puppet::Type::Cron::ProviderCrontab.instances on linux
> should be
> empty if user is not present
>
> I'm not sure how vital it is to keep those around (i.e., what
> breaks
> along with these assumptions?)
>
>
>
> I'm not sure whether anything breaks, but surely those particular
> tests fly directly in the face of the change described. It may be
> that they simply are rendered incorrect. If the change is acceptable
> then the corresponding rules under the revised behavior would be more
> like:
>
> 1) On Linux, if a user has no crontab then
> Puppet::Type::Cron::ProviderCrontab.instances should contain no
> instances having that user as their target
> 2) The target of every element of
> Puppet::Type::Cron::ProviderCrontab.instances should be an existing
> system user.

Well, yes, that would make sense.

Concerning the question how vital those tests might be: I dug around the
history a bit. Turns out those tests are fairly recent. Stefan Schulte
added them during the liberation of the BSD special schedule syntax.
(Good times.)

Stefan, any thoughts on repurposing these if we decide to go through
with this? Barring a response, my gut says that at least this part
likely poses no problems.

Best regards,
Felix

[Martin Langhoff]

On Wed, Nov 20, 2013 at 6:09 PM, Felix Frank
<Felix...@alumni.tu-berlin.de> wrote:
> I've been looking at the redmine ticket in [1], which basically asks for
> the ability to purge crontab entries from *all* users' crontabs. As is,

[ Please excuse the perhaps grumpy sounding tone. I am mainly
surprised/confounded :-) ]

Why all the bikeshedding on using the crontab facility, when
/etc/cron.d works like a charm?

If puppet is to have a better way to handle cronjobs, the IMHO clearly
sane approach would be files under /etc/cron.d . There puppet can
assume complete control of a given file, control the name of the file,
etc.

Two easy approaches are evident: install one file per cronjob defined,
or manage one big file that piles up all the puppet-controlled
cronjobs for that host. I am sure those options can be expanded and
refined to handle pretty much any situation we throw at it.

Having built systems over many years -- I cannot imagine a way of
handling cronjobs through the "crontab" facility that doesn't end up
being rather brittle eating data occasionally.

I will also note that this is a different case from authorized keys,
and that taking inspiration from the authorized keys handling code
might be a bad idea:

- authorized_keys isn't giving us a convenient "conf.d" directory structure

- authorized_keys _can_ hold old/stale/redundant entries sanely, one
rarely prunes it, where old/invalid cronjobs are an immediate source
of trouble

- authorized keys can hold duplicate entries without breaking, and
duplicate entries are safe to prune. Duplicated cronjobs are both a
valid configuration, and very bad news if not intended.

IOWs, it is an altogether different kind of fish. Maybe model it on
something that has conf.d style support (
https://forge.puppetlabs.com/puppetlabs/apache ? )

The infra I manage currently has all its cronjobs handled by puppet as
files under /etc/cron.d. It would naturally be more elegant to be able
to define the cronjob in a .pp file, and have Puppet handle the file
itself.

But messing with crontab... options like "purge" that remove data
blindly ... dragons be there...



m
--
martin....@gmail.com
- ask interesting questions
- don't get distracted with shiny stuff - working code first
~ http://docs.moodle.org/en/User:Martin_Langhoff

[Felix Frank]

Hi,

On 11/22/2013 01:48 AM, Martin Langhoff wrote:
> [ Please excuse the perhaps grumpy sounding tone. I am mainly
> surprised/confounded :-) ]

That's cool, I think we should be allowed to be grumpuses (grumpi?) here :-)

> Why all the bikeshedding on using the crontab facility, when
> /etc/cron.d works like a charm?

> If puppet is to have a better way to handle cronjobs, the IMHO clearly
> sane approach would be files under /etc/cron.d . There puppet can
> assume complete control of a given file, control the name of the file,
> etc.
>
> Two easy approaches are evident: install one file per cronjob defined,
> or manage one big file that piles up all the puppet-controlled
> cronjobs for that host. I am sure those options can be expanded and
> refined to handle pretty much any situation we throw at it.
>
> Having built systems over many years -- I cannot imagine a way of
> handling cronjobs through the "crontab" facility that doesn't end up
> being rather brittle eating data occasionally.

The whole cron.d thing is https://projects.puppetlabs.com/issues/746 and
I have started working on that, but it's far from becoming useful.

We won't be able to deprecate the crontab provider any time soon, I believe.

> I will also note that this is a different case from authorized keys,
> and that taking inspiration from the authorized keys handling code
> might be a bad idea:

Burrowing code is not at all what we were purporting, anyway.

> - authorized_keys isn't giving us a convenient "conf.d" directory structure
>
> - authorized_keys _can_ hold old/stale/redundant entries sanely, one
> rarely prunes it, where old/invalid cronjobs are an immediate source
> of trouble

I disagree about cronjobs implying greater problems. Pruning unknown ssh
keys is likely a security concern for some users.

> - authorized keys can hold duplicate entries without breaking, and
> duplicate entries are safe to prune. Duplicated cronjobs are both a
> valid configuration, and very bad news if not intended.

For parsedfile providers, pruning duplicate entries is pretty hard,
actually (or so I found).

I'm not sure to which you are referring specifically, a) a resource that
appears multiple times but is not managed at all or b) a resource that
duplicates a managed resource.

> IOWs, it is an altogether different kind of fish. Maybe model it on
> something that has conf.d style support (
> https://forge.puppetlabs.com/puppetlabs/apache ? )

I don't think that really pertains here. As a matter of fact, the only
type in that module is a2mod and its providers rely on apache2ctl, which
is in spirit much more akin to the crontab provider than a future cron.d
provider.

Purging resources that represent "files in one well known directory"
will be exceedingly easy, I think, because it's trivial to define a
flawless .instances provider method.

> The infra I manage currently has all its cronjobs handled by puppet as
> files under /etc/cron.d. It would naturally be more elegant to be able
> to define the cronjob in a .pp file, and have Puppet handle the file
> itself.
>
> But messing with crontab... options like "purge" that remove data
> blindly ... dragons be there...

Well, that's a question of philosophy, I think.

Personally, I think we should allow users to shoot themselves in their
respective feet to their hearts content, if some of them think the
facility for doing so can be worthwile. In the specific case of purging
lines from crontab files, I can even see potential benefits myself (by
the way, that discussion is more closely related to issue #3220),
although I'm wary of doing it to all possible crontab files on a system
(hence this RFC thread).

When all is said and done, I will gladly bury the crontab provider, then
exhume it, burn it and scatter its ashes throughout the source tree. It
will be awesome.
However, we first need a solid cron.d provider implementation, and even
then, there must be quite a deprecation period, because the old provider
is likely seeing pretty widespread use.

Cheers,
Felix

[Martin Langhoff]

On Fri, Nov 22, 2013 at 5:34 AM, Felix Frank
<felix...@alumni.tu-berlin.de> wrote:
> That's cool, I think we should be allowed to be grumpuses (grumpi?) here :-)

:-)

> The whole cron.d thing is https://projects.puppetlabs.com/issues/746 and
> I have started working on that, but it's far from becoming useful.
>
> We won't be able to deprecate the crontab provider any time soon, I believe.

Why not focus on a cron.d provider that is really solid, and then
start the crontab provider deprecation process (mediated by whatever
timeframe is appropriate)?

> Personally, I think we should allow users to shoot themselves in their
> respective feet to their hearts content, if some of them think the

> facility for doing so can be worthwhile.

Well, that is valid... only if the worth of the dangerous feature is
solid. IOWs, assume a few users will misunderstand or just plain not
read the docs and set it without meaning to.

I guess I cannot see the value of purging entries _that puppet did not
install in the first place_. I mean, there are lots of odd jobs that I
get puppet to perform by getting it to execute a bit of shell code,
and "nuke all crontab data for this user" seems to fit :-)

"Self contained" is a good goal, if an enhancement will deal with
puppet removing properly entries that puppet installed, then it seems
right.

> When all is said and done, I will gladly bury the crontab provider, then
> exhume it, burn it and scatter its ashes throughout the source tree. It
> will be awesome.

I'll be the one serving champagne...

cheers,

[John Bollinger]

On Friday, November 22, 2013 7:25:29 AM UTC-6, Martin Langhoff wrote:

On Fri, Nov 22, 2013 at 5:34 AM, Felix Frank
<felix...@alumni.tu-berlin.de> wrote:
> That's cool, I think we should be allowed to be grumpuses (grumpi?) here :-)

But not wumpuses.  All wumpuses must be hunted down.  :-)

 

:-)

> The whole cron.d thing is https://projects.puppetlabs.com/issues/746 and
> I have started working on that, but it's far from becoming useful.
>
> We won't be able to deprecate the crontab provider any time soon, I believe.

Why not focus on a cron.d provider that is really solid, and then
start the crontab provider deprecation process (mediated by whatever
timeframe is appropriate)?

Do all relevant cron implementations support cron.d?  Solaris?  BSD / OSX?  All Linux distros that matter?  Etc?  I don't know, but I am doubtful.

Moreover, it would be highly desirable to enable Puppet effectively to manage cron jobs that it did not create itself.  The work that Felix has been doing goes in that direction (hurrah!).  To the extent that that is a target, however, crontab support will be required as long as cron implementations support crontabs, and I don't foresee that going away any time soon, if ever.

 

> Personally, I think we should allow users to shoot themselves in their
> respective feet to their hearts content, if some of them think the
> facility for doing so can be worthwhile.

Well, that is valid... only if the worth of the dangerous feature is
solid. IOWs, assume a few users will misunderstand or just plain not
read the docs and set it without meaning to.

I guess I cannot see the value of purging entries _that puppet did not
install in the first place_. I mean, there are lots of odd jobs that I
get puppet to perform by getting it to execute a bit of shell code,
and "nuke all crontab data for this user" seems to fit :-)

I don't understand that.  I mean, I know that one can work around Puppet's limitations as to what cron jobs it can and can't manage, but inasmuch as it is stipulated that Puppet's built-in cron support is not sufficient, I don't understand why it should not seen as valuable to enhance Puppet's cron support.

Even if the only enhancement is support for global purge, that would provide something that you can only approximate with Exec, unless possibly you use a lot more than just "a bit" of shell code.  That is, you cannot easily and reliably avoid your Exec removing cron jobs that are under management.  Yes, Puppet might then recreate such jobs in the same run, but the result is not equivalent if any other resource listens for events from those Cron resources.
 

John

[Felix Frank]

On 11/25/2013 05:29 PM, John Bollinger wrote:
>
> Do all relevant cron implementations support cron.d? Solaris? BSD /
> OSX? All Linux distros that matter? Etc? I don't know, but I am
> doubtful.

That's a good point in favor of the continued fixing of crontab provider
bugs.

> I guess I cannot see the value of purging entries _that puppet did
> not
> install in the first place_. I mean, there are lots of odd jobs
> that I
> get puppet to perform by getting it to execute a bit of shell code,
> and "nuke all crontab data for this user" seems to fit :-)
>
>
>
> I don't understand that. I mean, I know that one can work around
> Puppet's limitations as to what cron jobs it can and can't manage, but
> inasmuch as it is stipulated that Puppet's built-in cron support is
> not sufficient, I don't understand why it should not seen as valuable
> to enhance Puppet's cron support.
>
> Even if the only enhancement is support for global purge, that would
> provide something that you can only approximate with Exec, unless
> possibly you use a lot more than just "a bit" of shell code. That is,
> you cannot easily and reliably avoid your Exec removing cron jobs that
> are under management. Yes, Puppet might then recreate such jobs in
> the same run, but the result is not equivalent if any other resource
> listens for events from those Cron resources.

I'm glad this discussion is not taking place on the users list, because
the workarounds proposed here are dangerous in their own way, as
manifested in this report: https://projects.puppetlabs.com/issues/10063

As an aside: Is Nigel Kersten on a longer (or permanent) leave of
absence? Whom can I troll with this kind of issue now?

On 11/21/2013 05:15 PM, Andy Parker wrote:
> I can understand the desire, but it seems like a very dangerous thing,
> maybe making it available but not immediately present would be a good
> way of doing it? We have the resources type that could be extended or
> there could be another crontab type created to purge.
>
> I'm not certain about either of these, but thought I'd chime in with
> those suggestions :)

Thanks, Andy. I wasn't really specific - apologies. The purge mechanism
that users expect does in fact rely on the resources type. Users want to

resources { "cron": purge => true }

and (on systems that default to the crontab provider, i.e. currently all
*nixes) expect crontabs to be cleared, all of them. Yes, it's dangerous,
but may be acceptable to some (e.g., I have long been operating with a
similar semantics for sudoers entries, which is potentially even more
dangerous).

> > What about systems that use other user databases, either instead of or
> > in addition to /etc/password? For example, LDAP or NIS or even winbind?

...

> 2. Add logic to enumerate actual crontabs. This would be somewhat ugly,
> because it's quite different from the current paradigm of "work with
> usernames only and access the crontab files through the crontab interface".

I've put some more thought into this and I think this general approach
should be pursued after all.

My initial sketch is flawed in two ways:
1. As John pointed out, Etc.passwd cannot reliably expected to yield all
applicable users.
2. Blindly adding targets to a Parsedfile provider feels like a dirty hack.

Instead, I will go ahead and try to allow the crontab provider to
facilitate the generation of additional cron resources by scanning the
file system. This will require some platform specific code for detecting
crontab files depending on the environment. I feel that this would be an
acceptable thing to do, because a similar logic already selects the
correct crontab (1) interface to use.

Thanks for all your feedback so far.

Cheers,
Felix

[Nigel Kersten]

On Sat, Nov 30, 2013 at 12:32 PM, Felix Frank <Felix...@alumni.tu-berlin.de> wrote:

I'm glad this discussion is not taking place on the users list, because
the workarounds proposed here are dangerous in their own way, as
manifested in this report: https://projects.puppetlabs.com/issues/10063

As an aside: Is Nigel Kersten on a longer (or permanent) leave of
absence? Whom can I troll with this kind of issue now?

I'm not on a leave of absence, but I'm not triaging these sorts of issues anymore.

Thanks for the feedback in the ticket, it does seem clear we should reject it as stated.

[John Bollinger]

On Saturday, November 30, 2013 2:32:22 PM UTC-6, Felix Frank wrote:

On 11/25/2013 05:29 PM, John Bollinger wrote:
>
> Do all relevant cron implementations support cron.d?  Solaris?  BSD /
> OSX?  All Linux distros that matter?  Etc?  I don't know, but I am
> doubtful.

That's a good point in favor of the continued fixing of crontab provider
bugs.

Here's another that I just discovered: crontab is specified by POSIX, but the mechanism by which crontabbed commands are executed (traditionally the cron daemon) is not.  Compliant implementations are required to provide a mechanism to execute commands from crontab entries as scheduled, but details not inherent in the crontab specifications are unspecified.  /etc/cron.d falls into that "unspecified" area, but more to the point, 'crontab' is the standard interface for scheduling periodic jobs.

> > What about systems that use other user databases, either instead of or
> > in addition to /etc/password?  For example, LDAP or NIS or even winbind?
...
> 2. Add logic to enumerate actual crontabs. This would be somewhat ugly,
> because it's quite different from the current paradigm of "work with
> usernames only and access the crontab files through the crontab interface".

I've put some more thought into this and I think this general approach
should be pursued after all.

My initial sketch is flawed in two ways:
1. As John pointed out, Etc.passwd cannot reliably expected to yield all
applicable users.
2. Blindly adding targets to a Parsedfile provider feels like a dirty hack.

Instead, I will go ahead and try to allow the crontab provider to
facilitate the generation of additional cron resources by scanning the
file system. This will require some platform specific code for detecting
crontab files depending on the environment. I feel that this would be an
acceptable thing to do, because a similar logic already selects the
correct crontab (1) interface to use.

I think that is consistent with Puppet's general approach.

For what it's worth, POSIX specifies that crontabs are stored in /var/spool, though it does not appear to specify exactly where therein, or what file names are used.

John

[Felix Frank]

On 12/03/2013 09:40 PM, John Bollinger wrote:
>
> For what it's worth, POSIX specifies that crontabs are stored in
> /var/spool, though it does not appear to specify exactly where
> therein, or what file names are used.
>

I based the initial implementation on this list:
http://www.cyberciti.biz/faq/where-is-the-crontab-file/
Only Debian is being tested. I'm inclined to just put this out into the
wild and wait for bug reports indicating potential deviations.

As a heads up, code is here:
https://github.com/ffrank/puppet/tree/ticket/master/11383-read-all-crontabs
It's based on the branch with the #3220 fix which is pending, therefor
no PR yet. Besides, I'm having a hard time thinking of good spec tests.
There will likely be a pretty heavily stubbed unit test.

Cheers,
Felix