Allow crontab resource to read crontab at user specified paths. #2328

miah · 2017-11-22T00:52:03Z

This PR adds support for reading crontab files at user specified paths as requested in #1461.

Example usage:

describe crontab('/etc/cron.d/some_crontab') do
  its('commands') { should include '/path/to/some/script' }
end

adamleff

Hi @miah! I love that we're enhancing this resource. However, I think some incorrect assumptions were made regarding the format of non-user crontab files and how they can be read in. Perhaps I tested on a different OS than you did, so I'm happy to be wrong about this as well.

adamleff · 2017-11-27T16:23:39Z

lib/resources/crontab.rb

+      @destination.nil? ? 'crontab for current user' : path_or_user
+    end
+
+    def path_or_user


We should probably make this a private method.

adamleff · 2017-11-27T16:24:58Z

lib/resources/crontab.rb

@@ -73,7 +77,7 @@ def parse_crontab_line(l)
    end

    def crontab_cmd
-      @user.nil? ? 'crontab -l' : "crontab -l -u #{@user}"
+      @destination.nil? ? 'crontab -l' : "crontab -l -u #{@destination}"


I don't think this works... -u specifically takes a user. On a stock Ubuntu 16.04 box, which has a /etc/cron.d/mdadm file, crontab -l -u /etc/cron.d/mdadm fails. I think you just need to read in the file.

adamleff · 2017-11-27T16:25:52Z

lib/resources/crontab.rb

      @params = read_crontab

      return skip_resource 'The `crontab` resource is not supported on your OS.' unless inspec.os.unix?
    end

    def read_crontab
-      inspec.command(crontab_cmd).stdout.lines.map { |l| parse_crontab_line(l) }.compact
+      ct = path? ? inspec.file(@destination).content : inspec.command(crontab_cmd).stdout
+      ct.lines.map { |l| parse_crontab_line(l) }.compact


Non-user crontabs, such as those in /etc/cron.d follow a different format. Specifically, before the command, there is another field for the user via which the command should be run. We'd need to adapt the resource to handle that new format for non-user crontabs.

adamleff · 2017-11-27T16:27:13Z

test/unit/mock/files/crondotd

@@ -0,0 +1,10 @@
+#


I think we need to provide a better mock file that has users in it. Here's the contents of the /etc/cron.d/mdadm file from the Ubuntu vagrant box I just spun up:

# # cron.d/mdadm -- schedules periodic redundancy checks of MD devices # # Copyright © martin f. krafft <[email protected]> # distributed under the terms of the Artistic Licence 2.0 # # By default, run at 00:57 on every Sunday, but do nothing unless the day of # the month is less than or equal to 7. Thus, only run on the first Sunday of # each month. crontab(5) sucks, unfortunately, in this regard; therefore this # hack (see #380425). 57 0 * * 0 root if [ -x /usr/share/mdadm/checkarray ] && [ $(date +\%d) -le 7 ]; then /usr/share/mdadm/checkarray --cron --all --idle --quiet; fi

Thanks, my precise64 doesn't have this; even after I installed mdadm. =)

chris-rock · 2017-11-27T16:40:04Z

lib/resources/crontab.rb

    "

    attr_reader :params

    include CommentParser

-    def initialize(user = nil)
-      @user   = user
+    def initialize(destination = nil)


I am a little bit unsure about the overloading of the destination. It can be a user name or a path. Wouldn't it be cleaner to use something like:
describe crontab({path: '/etc/cron.d/some_crontab'}) and describe crontab({user: 'chris'})

Yes will do!

miah · 2017-12-01T08:38:12Z

This PR updates the crontab resource to parse system and user crontab files. To parse a system crontab file pass a {path: '/some/path'} to the resource. The resource still accepts a user passed in as a string describe crontab('root'). You may now match against the user field.

describe crontab({user: 'root'}) do
  its('commands') { should include '/path/to/some/script' }
  its('user') { should be 'root' }
end

describe crontab({path: '/etc/cron.d/some_crontab'}) do
  its('commands') { should include '/path/to/some/script' }
end

adamleff

@miah nice work! A couple of clean-up items and I think this will be ready to go.

adamleff · 2017-12-01T12:46:38Z

lib/resources/crontab.rb


 require 'utils/parser'
 require 'utils/filter'

+# rubocop:disable Metrics/ClassLength


To be consistent with the way we disable in other files, can you please move this to the class definition itself?

class Crontab < Inspec.resource(1) # rubocop:disable Metrics/ClassLength

adamleff · 2017-12-01T12:47:25Z

lib/resources/crontab.rb

 module Inspec::Resources
  class Crontab < Inspec.resource(1)
    name 'crontab'
    desc 'Use the crontab InSpec audit resource to test the contents of the crontab for a given user which contains information about scheduled tasks owned by that user.'
    example "
-      describe crontab('root') do
+      describe crontab({user: 'root'}) do


The { } braces aren't necessary, Ruby will happily see that you're passing in a hash if you use the 2.0-and-greater syntax like you're doing:

describe crontab(user: 'root') do

adamleff · 2017-12-01T12:47:39Z

lib/resources/crontab.rb

@@ -25,51 +25,35 @@ class Crontab < Inspec.resource(1)
      describe crontab.where { command =~ /a partial command string/ } do
        its('entries.length') { should cmp 1 }
      end
+
+      describe crontab({path: '/etc/cron.d/some_crontab'}) do


Same here, unnecessary { } characters :)

adamleff · 2017-12-01T12:50:50Z

lib/resources/crontab.rb

-    def initialize(user = nil)
-      @user   = user
+    def initialize(opts = {})
+      Hash[opts.map { |k, v| [k.to_sym, v] }] if opts.respond_to?(:fetch)


We're doing a check to see if it's a Hash on each of these 3 lines, so can we consolidate it so it reads a bit more clearly?

if opts.respond_to?(:fetch) # user supplied a Hash containing options @user = opts[:user] @path = opts[:path] else # user passed in a string, and the prior behavior only supported # user crontabs, so assume they're supplying a user. @user = opts @path = nil end

adamleff · 2017-12-01T14:14:28Z

lib/resources/crontab.rb

      @params = read_crontab

      return skip_resource 'The `crontab` resource is not supported on your OS.' unless inspec.os.unix?
    end

    def read_crontab
-      inspec.command(crontab_cmd).stdout.lines.map { |l| parse_crontab_line(l) }.compact
+      ct = @path.nil? ? inspec.command(crontab_cmd).stdout : inspec.file(@path).content


We do the @path.nil? check a few times. What do you think about adding a helper method or to?

def is_system_crontab? !@path.nil? end def is_user_crontab? !@user.nil? end

We could use that here, in the to_s method, and in parse_crontab_line.

adamleff · 2017-12-01T14:19:28Z

lib/resources/crontab.rb

+    def initialize(opts = {})
+      Hash[opts.map { |k, v| [k.to_sym, v] }] if opts.respond_to?(:fetch)
+      @user = opts.respond_to?(:fetch) ? opts.fetch(:user, nil) : opts
+      @path = opts.fetch(:path, nil) if opts.respond_to?(:fetch)
      @params = read_crontab

      return skip_resource 'The `crontab` resource is not supported on your OS.' unless inspec.os.unix?


While you're in here, can you change this to raise an exception?

raise Inspec::Exceptions::ResourceSkipped, 'The `crontab` resource is not supported on your OS.' unless inspec.os.unix?

And we should also add an exception if the user didn't supply us enough information (i.e. they gave us a hash that contains neither user or path):

raise Inspec::Exceptions::ResourceFailed, "A user or path must be supplied" if @user.nil? && @path.nil?

miah · 2017-12-05T22:39:19Z

Hi,

I have addressed that list of clean up items. Please let me know if there is anything else I should consider. =)

adamleff

Alright, @miah! This looks great. Conditionally approved pending green tests after a rebase once we square away the unrelated Train issue.

a user and return the correct string.

…hen either calling the crontab command or reading the file directly, so break that out into a path? method.

…n the crontab command. Signed-off-by: Miah Johnson <[email protected]>

Signed-off-by: Miah Johnson <[email protected]>

@user

Change initialize default argument to create a hash by default, though it is still possible to pass in a 'user' string argument. @user gets set with the argument value unless its a hash, in which case it tries to set the value of the user key, otherwise it becomes nil. @file gets set with the value of the path key, unless it doesn't exist in which case it becomes nil. All hash keys are symbolized to ensure consistent access. Signed-off-by: Miah Johnson <[email protected]>

@destination

file. path? was removed as we're not overloading a @destination variable anymore. Signed-off-by: Miah Johnson <[email protected]>

Signed-off-by: Miah Johnson <[email protected]>

We have three possible cases, current user, other user, or file path. This accounts for all of them. Signed-off-by: Miah Johnson <[email protected]>

Signed-off-by: Miah Johnson <[email protected]>

@Weekly

parse_system_crontab Because a command in a crontab file could have spaces we must parse user and system crontabs differently. When we parse user crontabs the user field will either be nil, or the requested user. Both user and path parsers handle special strings (@Yearly, @Weekly, etc). And also account for position of user in these files (or adds it in user case) Signed-off-by: Miah Johnson <[email protected]>

Signed-off-by: Miah Johnson <[email protected]>

module. Remove unnecessary braces. Add is_system_crontab? and is_user_crontab helper methods and use them. Add tests to see if error conditions are raised when the resource is invoked with missing parameters (user, or path), and on a unsupported os. Change initialize to group all hash functions together and raise errors when user and path is unset. Also raise errors on unsupported operating systems. Change order of ternary and use is_system_crontab? rather than @path.nil? Signed-off-by: Miah Johnson <[email protected]>

arlimus

What a wonderful improvement and update, thank you so much @miah !! 😄

Kudos Adam for your guidance as well :)

arlimus · 2017-12-07T12:46:53Z

lib/resources/crontab.rb

@@ -25,51 +24,40 @@ class Crontab < Inspec.resource(1)
      describe crontab.where { command =~ /a partial command string/ } do
        its('entries.length') { should cmp 1 }
      end
+
+      describe crontab(path: '/etc/cron.d/some_crontab') do


love the new options specification!

miah requested a review from a team as a code owner November 22, 2017 00:52

miah force-pushed the miah/1461 branch from c575267 to 994edb9 Compare November 22, 2017 06:17

adamleff suggested changes Nov 27, 2017

View reviewed changes

chris-rock reviewed Nov 27, 2017

View reviewed changes

miah force-pushed the miah/1461 branch from 60d84fc to 20dd715 Compare November 30, 2017 23:18

adamleff suggested changes Dec 1, 2017

View reviewed changes

miah force-pushed the miah/1461 branch from 99107f1 to 1647a1e Compare December 5, 2017 22:05

adamleff approved these changes Dec 6, 2017

View reviewed changes

miah added 20 commits December 6, 2017 12:19

add a emulated /etc/cron.d/crondotd file to the mocking system.

ce2864a

test that we handle incoming paths correctly by rendering to_s.

754e5be

We take in both users and a path, so lets call that destination.

6f0ea68

To make the test pass we'll determine if we are dealing with a path or

a63e052

a user and return the correct string.

we will need the ability to determine if we are dealing with a path w…

d367126

…hen either calling the crontab command or reading the file directly, so break that out into a path? method.

remove author field.

00a3c89

test contents of our crondotd file.

d977931

we have to explicitly make @destination a String to use include?.

21e2320

when we get a path we use inspec.file to get conents, otherwise we ru…

f5c4afe

…n the crontab command. Signed-off-by: Miah Johnson <[email protected]>

Add documentation for example usage with file path.

0883e9c

Signed-off-by: Miah Johnson <[email protected]>

Make path? and path_or_user private methods

30fcdc6

Signed-off-by: Miah Johnson <[email protected]>

Add missing username filed to crondotd mock file

8f03362

Signed-off-by: Miah Johnson <[email protected]>

Pass argument as a hash when testing file paths

f7dfb5f

Signed-off-by: Miah Johnson <[email protected]>

Expected results should include usernames when testing file paths

9e7dc12

Signed-off-by: Miah Johnson <[email protected]>

Add special string @yearly test to crondotd mock file

fae2079

Signed-off-by: Miah Johnson <[email protected]>

Add user to existing cron tests

05b5800

Signed-off-by: Miah Johnson <[email protected]>

Rubocop says I need spaces after/before curly brackets

06b6b41

Signed-off-by: Miah Johnson <[email protected]>

Add user to crondotd file tests and add @Yearly test

a44f17a

Signed-off-by: Miah Johnson <[email protected]>

Check if @path is nil to determine if we run crontab command or parse

fd65426

file. path? was removed as we're not overloading a @destination variable anymore. Signed-off-by: Miah Johnson <[email protected]>

miah added 8 commits December 6, 2017 12:19

if @user is nil assume current user otherwise crontab for @user

1a44890

Signed-off-by: Miah Johnson <[email protected]>

Change to complete if rather than ternary.

68e84fc

We have three possible cases, current user, other user, or file path. This accounts for all of them. Signed-off-by: Miah Johnson <[email protected]>

Add user to the crontab FilterTable

b5c82a9

Signed-off-by: Miah Johnson <[email protected]>

Remove path? and path_or_user

b8b8560

Signed-off-by: Miah Johnson <[email protected]>

Update examples with user: and path:

5a237da

Signed-off-by: Miah Johnson <[email protected]>

Add spaces after : in example docs

99dab38

Signed-off-by: Miah Johnson <[email protected]>

Disable rubocop ClassLength check

f5f378e

Signed-off-by: Miah Johnson <[email protected]>

miah force-pushed the miah/1461 branch 2 times, most recently from 2239aa9 to 858fd30 Compare December 6, 2017 20:33

miah force-pushed the miah/1461 branch from 858fd30 to 808f096 Compare December 6, 2017 20:58

adamleff added the Type: Enhancement Improves an existing feature label Dec 7, 2017

arlimus approved these changes Dec 7, 2017

View reviewed changes

arlimus merged commit e33f495 into master Dec 7, 2017

arlimus deleted the miah/1461 branch December 7, 2017 12:50

tduffield added Habitat: Skip Build labels Dec 7, 2017

zenspider added Expeditor: Skip Habitat Expeditor: Skip Omnibus and removed Habitat: Skip Build labels Nov 15, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow crontab resource to read crontab at user specified paths. #2328

Allow crontab resource to read crontab at user specified paths. #2328

miah commented Nov 22, 2017

adamleff left a comment

adamleff Nov 27, 2017

adamleff Nov 27, 2017

adamleff Nov 27, 2017

adamleff Nov 27, 2017

miah Nov 27, 2017

chris-rock Nov 27, 2017

miah Nov 27, 2017

miah commented Dec 1, 2017

adamleff left a comment

adamleff Dec 1, 2017

adamleff Dec 1, 2017

adamleff Dec 1, 2017

adamleff Dec 1, 2017

adamleff Dec 1, 2017

adamleff Dec 1, 2017

miah commented Dec 5, 2017

adamleff left a comment

arlimus left a comment

arlimus Dec 7, 2017

Allow crontab resource to read crontab at user specified paths. #2328

Allow crontab resource to read crontab at user specified paths. #2328

Conversation

miah commented Nov 22, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miah commented Dec 1, 2017

adamleff left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miah commented Dec 5, 2017

adamleff left a comment

Choose a reason for hiding this comment

arlimus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment