Merge pull request #13 from ekinanp/MODULES-7789-again

(MODULES-7789) Port over the PUP-9217 changes

5 files changed, 450 insertions, 0 deletions

diff --git a/spec/acceptance/tests/resource/cron/should_fail_to_write_a_nonexistent_users_crontab_spec.rb b/spec/acceptance/tests/resource/cron/should_fail_to_write_a_nonexistent_users_crontab_spec.rb
new file mode 100644
index 0000000..40fbbd9
--- /dev/null
+++ b/spec/acceptance/tests/resource/cron/should_fail_to_write_a_nonexistent_users_crontab_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper_acceptance'
+
+RSpec.context 'when Puppet attempts to write the crontab of a nonexistent user' do
+  let(:nonexistent_username) { "pl#{rand(999_999).to_i}" }
+
+  before(:each) do
+    step 'Ensure that the nonexistent user does not exist' do
+      compatible_agents.each do |agent|
+        user_absent(agent, nonexistent_username)
+      end
+    end
+  end
+
+  compatible_agents.each do |agent|
+    it "should fail on #{agent}" do
+      manifest = cron_manifest('second_entry', command: 'ls', user: nonexistent_username)
+      apply_manifest_on(agent, manifest, expect_failures: true)
+    end
+  end
+end
diff --git a/spec/acceptance/tests/resource/cron/should_not_overwrite_crontab_file_on_file_read_error_spec.rb b/spec/acceptance/tests/resource/cron/should_not_overwrite_crontab_file_on_file_read_error_spec.rb
new file mode 100644
index 0000000..6a5b2c6
--- /dev/null
+++ b/spec/acceptance/tests/resource/cron/should_not_overwrite_crontab_file_on_file_read_error_spec.rb
@@ -0,0 +1,133 @@
+require 'spec_helper_acceptance'
+
+RSpec.context 'when Puppet cannot read a crontab file' do
+  # This test only makes sense for agents that shipped with PUP-9217's
+  # changes, so we do not want to run it on older agents.
+  def older_agent?(agent)
+    puppet_version = Gem::Version.new(on(agent, puppet('--version')).stdout.chomp)
+    minimum_puppet_version = if puppet_version < Gem::Version.new('6.0.0')
+                               Gem::Version.new('5.5.9')
+                             else
+                               Gem::Version.new('6.0.5')
+                             end
+
+    puppet_version < minimum_puppet_version
+  end
+
+  let(:username) { "pl#{rand(999_999).to_i}" }
+  let(:crontab_contents) { '6 6 6 6 6 /usr/bin/true' }
+
+  before(:each) do
+    compatible_agents.each do |agent|
+      next if older_agent?(agent)
+
+      step "Create the user on #{agent}" do
+        user_present(agent, username)
+      end
+
+      step "Set the user's crontab on #{agent}" do
+        run_cron_on(agent, :add, username, crontab_contents)
+        assert_matching_arrays([crontab_contents], crontab_entries_of(agent, username), "Could not set the user's crontab for testing")
+      end
+    end
+  end
+
+  after(:each) do
+    compatible_agents.each do |agent|
+      next if older_agent?(agent)
+
+      step "Teardown -- Erase the user on #{agent}" do
+        run_cron_on(agent, :remove, username)
+        user_absent(agent, username)
+      end
+    end
+  end
+
+  compatible_agents.each do |agent|
+    it "should not overwrite it on #{agent}" do
+      if older_agent?(agent)
+        skip('Skipping this test since we are on an older agent that does not have the PUP-9217 changes')
+      end
+
+      crontab_exe = nil
+      step 'Find the crontab executable' do
+        crontab_exe = on(agent, 'which crontab').stdout.chomp
+      end
+
+      stub_crontab_bin_dir = nil
+      stub_crontab_exe = nil
+      step 'Create the stub crontab executable that triggers the read error' do
+        stub_crontab_bin_dir = agent.tmpdir('stub_crontab_bin_dir')
+        on(agent, "chown #{username} #{stub_crontab_bin_dir}")
+
+        stub_crontab_exe = "#{stub_crontab_bin_dir}/crontab"
+        stub_crontab_exe_script = <<-SCRIPT
+  #!/usr/bin/env bash
+  exit 1
+  SCRIPT
+        create_remote_file(agent, stub_crontab_exe, stub_crontab_exe_script)
+        on(agent, "chown #{username} #{stub_crontab_exe}")
+        on(agent, "chmod a+x #{stub_crontab_exe}")
+      end
+
+      path_env_var = nil
+      step 'Get the value of the PATH environment variable' do
+        path_env_var = on(agent, 'echo $PATH').stdout.chomp
+      end
+
+      step "(PUP-9217) Attempt to overwrite the user's crontab file" do
+        # This manifest reproduces the issue in PUP-9217. Here's how:
+        #   1. When Puppet attempts to realize Cron['first_entry'], it will prefetch
+        #   all of the present cron entries on the system. To do this, it will execute
+        #   the crontab command. Since we prepend stub_crontab_bindir to PATH prior to
+        #   executing Puppet, executing the crontab command will really execute
+        #   stub_crontab_exe, which returns an exit code of 1. This triggers our
+        #   read error.
+        #
+        #   2. Puppet will attempt to write Cron['first_entry'] onto disk. However, it will
+        #   fail to do so because it will still execute stub_crontab_exe to perform the
+        #   write. Thus, Cron['first_entry'] will fail its evaluation.
+        #
+        #   3. Next, Puppet will modify stub_crontab_exe to now execute the actual crontab
+        #   command so that any subsequent calls to crontab will succeed. Note that under
+        #   the hood, Puppet will still be executing stub_crontab_exe.
+        #
+        #   4. Finally, Puppet will attempt to realize Cron['second_entry']. It will skip
+        #   the prefetch step, instead proceeding to directly write Cron['second_entry']
+        #   to disk. But note that since the prefetch failed in (1), Puppet will proceed
+        #   to overwrite our user's crontab file so that it will contain Cron['first_entry']
+        #   and Cron['second_entry'] (Cron['first_entry'] is there because Puppet maintains
+        #   each crontab file's entries in memory so that when it writes one entry to disk,
+        #   it will write all of them).
+        manifest = [
+          cron_manifest('first_entry', command: 'ls', user: username),
+          file_manifest(stub_crontab_exe, content: "#!/usr/bin/env bash\n#{crontab_exe} $@"),
+          cron_manifest('second_entry', command: 'ls', user: username),
+        ].join("\n\n")
+        manifest_file = agent.tmpfile('crontab_overwrite_manifest')
+        create_remote_file(agent, manifest_file, manifest)
+
+        # We need to run a script here instead of a command because:
+        #   * We need to cd into a directory owned by our user. Otherwise, bash will
+        #   fail to execute stub_crontab_exe on AIX and Solaris because we run crontab
+        #   as the given user, and the given user does not have access to Puppet's cwd.
+        #
+        #   * We also need to pass-in our PATH to Puppet since it contains stub_crontab_bin_dir.
+        apply_crontab_overwrite_manifest = agent.tmpfile('apply_crontab_overwrite_manifest')
+        script = <<-SCRIPT
+  #!/usr/bin/env bash
+
+  cd #{stub_crontab_bin_dir} && puppet apply #{manifest_file}
+  SCRIPT
+        create_remote_file(agent, apply_crontab_overwrite_manifest, script)
+        on(agent, "chmod a+x #{apply_crontab_overwrite_manifest}")
+
+        on(agent, "bash #{apply_crontab_overwrite_manifest}", environment: { PATH: "#{stub_crontab_bin_dir}:#{path_env_var}" })
+      end
+
+      step "(PUP-9217) Verify that Puppet does not overwrite the user's crontab file when it fails to read it" do
+        assert_matching_arrays([crontab_contents], crontab_entries_of(agent, username), "Puppet overwrote the user's crontab file even though it failed to read it")
+      end
+    end
+  end
+end
diff --git a/spec/acceptance/tests/resource/cron/should_only_fail_associated_resources_on_file_read_error_spec.rb b/spec/acceptance/tests/resource/cron/should_only_fail_associated_resources_on_file_read_error_spec.rb
new file mode 100644
index 0000000..1846523
--- /dev/null
+++ b/spec/acceptance/tests/resource/cron/should_only_fail_associated_resources_on_file_read_error_spec.rb
@@ -0,0 +1,136 @@
+require 'spec_helper_acceptance'
+
+RSpec.context 'when Puppet cannot read a crontab file' do
+  # This test only makes sense for agents that shipped with PUP-9217's
+  # changes, so we do not want to run it on older agents.
+  def older_agent?(agent)
+    puppet_version = Gem::Version.new(on(agent, puppet('--version')).stdout.chomp)
+    minimum_puppet_version = if puppet_version < Gem::Version.new('6.0.0')
+                               Gem::Version.new('5.5.9')
+                             else
+                               Gem::Version.new('6.0.5')
+                             end
+
+    puppet_version < minimum_puppet_version
+  end
+
+  let(:username) { "pl#{rand(999_999).to_i}" }
+  let(:failed_username) { "pl#{rand(999_999).to_i}" }
+
+  before(:each) do
+    compatible_agents.each do |agent|
+      next if older_agent?(agent)
+
+      step "Create the users on #{agent}" do
+        user_present(agent, username)
+        user_present(agent, failed_username)
+      end
+    end
+  end
+
+  after(:each) do
+    compatible_agents.each do |agent|
+      next if older_agent?(agent)
+
+      step "Teardown -- Erase the users on #{agent}" do
+        run_cron_on(agent, :remove, username)
+        user_absent(agent, username)
+
+        user_absent(agent, failed_username)
+      end
+    end
+  end
+
+  compatible_agents.each do |agent|
+    it "should only fail the associated resources on #{agent}" do
+      if older_agent?(agent)
+        skip('Skipping this test since we are on an older agent that does not have the PUP-9217 changes')
+      end
+
+      crontab_exe = nil
+      step 'Find the crontab executable' do
+        crontab_exe = on(agent, 'which crontab').stdout.chomp
+      end
+
+      stub_crontab_bin_dir = nil
+      stub_crontab_exe = nil
+      step 'Create the stub crontab executable that triggers the read error for the failed user' do
+        stub_crontab_bin_dir = agent.tmpdir('stub_crontab_bin_dir')
+        stub_crontab_exe = "#{stub_crontab_bin_dir}/crontab"
+
+        # On Linux and OSX, we read a user's crontab by running crontab -u <username>,
+        # where the crontab command is run as root. However on AIX/Solaris, we read a
+        # user's crontab by running the crontab command as that user. Thus our mock
+        # crontab executable needs to check if we're reading our failed user's crontab
+        # (Linux and OSX) OR running crontab as our failed user (AIX and Solaris) before
+        # triggering the FileReadError
+        stub_crontab_exe_script = <<-SCRIPT
+#!/usr/bin/env bash
+ if [[ "$@" =~ #{failed_username} || "`id`" =~ #{failed_username} ]]; then
+  echo "Mocking a FileReadError for the #{failed_username} user's crontab!"
+  exit 1
+fi
+ #{crontab_exe} $@
+SCRIPT
+
+        create_remote_file(agent, stub_crontab_exe, stub_crontab_exe_script)
+        on(agent, "chmod 777 #{stub_crontab_bin_dir}")
+        on(agent, "chmod 777 #{stub_crontab_exe}")
+      end
+
+      path_env_var = nil
+      step 'Get the value of the PATH environment variable' do
+        path_env_var = on(agent, 'echo $PATH').stdout.chomp
+      end
+
+      puppet_result = nil
+      step 'Add some cron entries with Puppet' do
+        # We delete our mock crontab executable here to ensure that Cron[second_entry]'s
+        # evaluation fails because of the FileReadError raised in the prefetch
+        # step. Otherwise, Cron[second_entry]'s evaluation will fail at the write step
+        # because Puppet would still be invoking our mock crontab executable, which would
+        # pass the test on an agent that swallows FileReadErrors in the cron provider's
+        # prefetch step.
+        manifest = [
+          cron_manifest('first_entry', command: 'ls', user: username),
+          file_manifest(stub_crontab_exe, ensure: :absent),
+          cron_manifest('second_entry', command: 'ls', user: failed_username),
+        ].join("\n\n")
+        manifest_file = agent.tmpfile('crontab_overwrite_manifest')
+        create_remote_file(agent, manifest_file, manifest)
+
+        # We need to run a script here instead of a command because:
+        #   * We need to cd into a directory that our user can access. Otherwise, bash will
+        #   fail to execute stub_crontab_exe on AIX and Solaris because we run crontab
+        #   as the given user, and the given user does not have access to Puppet's cwd.
+        #
+        #   * We also need to pass-in our PATH to Puppet since it contains stub_crontab_bin_dir.
+        apply_crontab_overwrite_manifest = agent.tmpfile('apply_crontab_overwrite_manifest')
+        script = <<-SCRIPT
+  #!/usr/bin/env bash
+  cd #{stub_crontab_bin_dir} && puppet apply #{manifest_file}
+  SCRIPT
+        create_remote_file(agent, apply_crontab_overwrite_manifest, script)
+        on(agent, "chmod a+x #{apply_crontab_overwrite_manifest}")
+
+        puppet_result = on(agent, "bash #{apply_crontab_overwrite_manifest}", environment: { PATH: "#{stub_crontab_bin_dir}:#{path_env_var}" })
+      end
+
+      step 'Verify that Puppet fails a Cron resource associated with an unreadable crontab file' do
+        assert_match(%r{Cron.*second_entry}, puppet_result.stderr, 'Puppet does not fail a Cron resource associated with an unreadable crontab file')
+      end
+
+      step 'Verify that Puppet does not fail a Cron resource associated with a readable crontab file' do
+        assert_no_match(%r{Cron.*first_entry}, puppet_result.stderr, 'Puppet fails a Cron resource associated with a readable crontab file')
+      end
+
+      step 'Verify that Puppet successfully evaluates a Cron resource associated with a readable crontab file' do
+        assert_match(%r{Cron.*first_entry}, puppet_result.stdout, 'Puppet fails to evaluate a Cron resource associated with a readable crontab file')
+      end
+
+      step 'Verify that Puppet did update the readable crontab file with the Cron resource' do
+        assert_matching_arrays(['* * * * * ls'], crontab_entries_of(agent, username), 'Puppet fails to update a readable crontab file with the specified Cron entry')
+      end
+    end
+  end
+end
diff --git a/spec/acceptance/tests/resource/cron/should_write_an_originally_unauthorized_users_crontab_if_puppet_authorizes_them_spec.rb b/spec/acceptance/tests/resource/cron/should_write_an_originally_unauthorized_users_crontab_if_puppet_authorizes_them_spec.rb
new file mode 100644
index 0000000..834cac7
--- /dev/null
+++ b/spec/acceptance/tests/resource/cron/should_write_an_originally_unauthorized_users_crontab_if_puppet_authorizes_them_spec.rb
@@ -0,0 +1,102 @@
+require 'spec_helper_acceptance'
+
+RSpec.context 'when Puppet authorizes a previously unauthorized user to use crontab' do
+  let(:username) { "pl#{rand(999_999).to_i}" }
+  let(:unauthorized_username) { "pl#{rand(999_999).to_i}" }
+
+  let(:cron_deny_path) do
+    {}
+  end
+  let(:cron_deny_original_contents) do
+    {}
+  end
+
+  before(:each) do
+    step 'Ensure that the test users exist' do
+      compatible_agents.each do |agent|
+        user_present(agent, username)
+        user_present(agent, unauthorized_username)
+      end
+    end
+
+    step 'Get the current cron.deny contents for each agent' do
+      compatible_agents.each do |agent|
+        case agent['platform']
+        when %r{aix}
+          cron_deny_path[agent] = '/var/adm/cron/cron.deny'
+        when %r{solaris}
+          cron_deny_path[agent] = '/etc/cron.d/cron.deny'
+        else
+          fail_test "Cannot figure out the path of the cron.deny file for the #{agent['platform']} platform"
+        end
+
+        cron_deny_original_contents[agent] = on(agent, "cat #{cron_deny_path[agent]}").stdout
+      end
+    end
+  end
+
+  after(:each) do
+    step 'Teardown -- Erase the test users on the agents' do
+      compatible_agents.each do |agent|
+        run_cron_on(agent, :remove, username)
+        user_absent(agent, username)
+
+        run_cron_on(agent, :remove, unauthorized_username)
+        user_absent(agent, unauthorized_username)
+      end
+    end
+
+    # It is possible for the "before" hook to raise an exception while the
+    # cron.deny contents are being computed, so only execute the next step
+    # if we've managed to compute the cron.deny contents for each agent
+    next unless cron_deny_original_contents.keys.size == compatible_agents.size
+
+    step "Teardown -- Reset each agent's cron.deny contents" do
+      compatible_agents.each do |agent|
+        apply_manifest_on(agent, file_manifest(cron_deny_path[agent], ensure: :present, content: cron_deny_original_contents[agent]))
+      end
+    end
+  end
+
+  compatible_agents.each do |agent|
+    is_aix_or_solaris_agent = agent['platform'].include?('aix') || agent['platform'].include?('solaris')
+
+    it "should write that user's crontab on #{agent}", if: is_aix_or_solaris_agent do
+      step 'Add the unauthorized user to the cron.deny file' do
+        on(agent, "echo #{unauthorized_username} >> #{cron_deny_path[agent]}")
+      end
+
+      step 'Verify that the unauthorized user was added to the cron.deny file' do
+        cron_deny_contents = on(agent, "cat #{cron_deny_path[agent]}").stdout
+        assert_match(%r{^#{unauthorized_username}$}, cron_deny_contents, 'Failed to add the unauthorized user to the cron.deny file')
+      end
+
+      step "Modify the unauthorized user's crontab with Puppet" do
+        # The scenario we're testing here is:
+        #   * An unrelated cron resource triggers the prefetch step, which will also
+        #   prefetch the crontab of our unauthorized user. The latter prefetch should
+        #   fail, instead returning an empty crontab file.
+        #
+        #   * Puppet authorizes our unauthorized user by removing them from the cron.deny
+        #   file.
+        #
+        #   * A cron resource linked to our (originally) unauthorized user should now be able
+        #   to write to that user's crontab file (assuming it requires the resource updating
+        #   the cron.deny file)
+        #
+        # The following manifest replicates the above scenario. Note that we test this specific
+        # scenario to ensure that the changes in PUP-9217 enforce backwards compatibility.
+        manifest = [
+          cron_manifest('first_entry', command: 'ls', user: username),
+          file_manifest(cron_deny_path[agent], ensure: :present, content: cron_deny_original_contents[agent]),
+          cron_manifest('second_entry', command: 'ls', user: unauthorized_username),
+        ].join("\n\n")
+        apply_manifest_on(agent, manifest)
+      end
+
+      step "Verify that Puppet did modify the unauthorized user's crontab" do
+        assert_matching_arrays(['* * * * * ls'], crontab_entries_of(agent, unauthorized_username), "Puppet did not modify the unauthorized user's crontab file")
+      end
+    end
+  end
+end
diff --git a/spec/acceptance/tests/resource/cron/should_write_new_users_crontab_after_puppet_creates_them_spec.rb b/spec/acceptance/tests/resource/cron/should_write_new_users_crontab_after_puppet_creates_them_spec.rb
new file mode 100644
index 0000000..12506ad
--- /dev/null
+++ b/spec/acceptance/tests/resource/cron/should_write_new_users_crontab_after_puppet_creates_them_spec.rb
@@ -0,0 +1,59 @@
+require 'spec_helper_acceptance'
+
+RSpec.context 'when Puppet creates a user in the middle of its run' do
+  let(:known_username) { "pl#{rand(999_999).to_i}" }
+  let(:new_username) { "pl#{rand(999_999).to_i}" }
+
+  before(:each) do
+    step 'Ensure that the known user exists on all of the agents' do
+      compatible_agents.each do |agent|
+        user_present(agent, known_username)
+      end
+    end
+
+    step 'Ensure that the new user does not exist on all of the agents' do
+      compatible_agents.each do |agent|
+        user_absent(agent, new_username)
+      end
+    end
+  end
+
+  after(:each) do
+    step 'Teardown -- Ensure that both users are deleted on all of the agents' do
+      compatible_agents.each do |agent|
+        run_cron_on(agent, :remove, known_username)
+        user_absent(agent, known_username)
+
+        run_cron_on(agent, :remove, new_username)
+        user_absent(agent, new_username)
+      end
+    end
+  end
+
+  compatible_agents.each do |agent|
+    it "should be able to write their crontab on #{agent}" do
+      puppet_result = nil
+      step "Create the new user, and the known + new user's crontab entries with Puppet" do
+        # Placing Cron[first_entry] before creating the new user
+        # triggers a prefetch of all the Cron resources on the
+        # system. This lets us test that the prefetch step marks
+        # the crontab of an unknown user as empty instead of marking
+        # it as a failure.
+        manifest = [
+          cron_manifest('first_entry', command: 'ls', user: known_username),
+          user_manifest(new_username, ensure: :present),
+          cron_manifest('second_entry', command: 'ls', user: new_username),
+        ].join("\n\n")
+        puppet_result = apply_manifest_on(agent, manifest)
+      end
+
+      step 'Verify that Puppet successfully evaluates a Cron resource associated with the new user' do
+        assert_match(%r{Cron.*second_entry}, puppet_result.stdout, 'Puppet fails to evaluate a Cron resource associated with a new user')
+      end
+
+      step "Verify that Puppet did create the new user's crontab file" do
+        assert_matching_arrays(['* * * * * ls'], crontab_entries_of(agent, new_username), "Puppet did not create the new user's crontab file")
+      end
+    end
+  end
+end