diff options
author | Josh Cooper <josh@puppet.com> | 2019-01-09 15:18:22 -0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-01-09 15:18:22 -0800 |
commit | 389ebeeca247d2d437c620310b8bef1b1bfdead2 (patch) | |
tree | c970bd1e0b18b44a79b118daca6772a2e2ed651e /spec | |
parent | 352a38fdeda0c9b0c181f1e9bcf7bf40c2f62749 (diff) | |
parent | c0068fc6535289f7828ba42ad8610914a57604f1 (diff) | |
download | puppet-cron_core-389ebeeca247d2d437c620310b8bef1b1bfdead2.tar.gz puppet-cron_core-389ebeeca247d2d437c620310b8bef1b1bfdead2.tar.bz2 |
Merge pull request #13 from ekinanp/MODULES-7789-again
(MODULES-7789) Port over the PUP-9217 changes
Diffstat (limited to 'spec')
6 files changed, 515 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 diff --git a/spec/spec_helper_acceptance.rb b/spec/spec_helper_acceptance.rb index e4f0182..57c5b21 100644 --- a/spec/spec_helper_acceptance.rb +++ b/spec/spec_helper_acceptance.rb @@ -4,6 +4,34 @@ require 'beaker/puppet_install_helper' $LOAD_PATH << File.join(__dir__, 'acceptance/lib') +# TODO: This should be added to Beaker +def assert_matching_arrays(expected, actual, message = '') + assert_equal(expected.sort, actual.sort, message) +end + +# TODO: Remove the wrappers to user_present +# and user_absent if Beaker::Host's user_present +# and user_absent functions are fixed to work with +# Arista (EOS). + +def user_present(host, username) + case host['platform'] + when %r{eos} + on(host, "useradd #{username}") + else + host.user_present(username) + end +end + +def user_absent(host, username) + case host['platform'] + when %r{eos} + on(host, "userdel #{username}", acceptable_exit_codes: [0, 1]) + else + host.user_absent(username) + end +end + def beaker_opts { debug: true, trace: true, expect_failures: true, acceptable_exit_codes: (0...256) } # { expect_failures: true, acceptable_exit_codes: (0...256) } @@ -29,6 +57,43 @@ def setup(agent, o = {}) package {'cron': name=> $cron, ensure=>present, })) end +# Returns all of the lines that correspond to crontab entries +# on the agent. For simplicity, these are any lines that do +# not begin with '#'. +def crontab_entries_of(host, username) + crontab_contents = run_cron_on(host, :list, username).stdout.strip + crontab_contents.lines.map(&:chomp).reject { |line| line =~ %r{^#} } +end + +def resource_manifest(resource, title, params = {}) + params_str = params.map { |param, value| + # This is not quite correct for all parameter values, + # but it is good enough for most purposes. + value_str = value.to_s + value_str = "\"#{value_str}\"" if value.is_a?(String) + + " #{param} => #{value_str}" + }.join(",\n") + + <<-MANIFEST + #{resource} { '#{title}': + #{params_str} +} + MANIFEST +end + +def file_manifest(path, params = {}) + resource_manifest('file', path, params) +end + +def cron_manifest(entry_name, params = {}) + resource_manifest('cron', entry_name, params) +end + +def user_manifest(username, params = {}) + resource_manifest('user', username, params) +end + RSpec.configure do |c| c.before :suite do unless ENV['BEAKER_provision'] == 'no' |