Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(#14283) Fix suntab filetype when run as normal user #830

Closed
wants to merge 2 commits into from

Conversation

stschulte
Copy link
Contributor

Running puppet as an unpriviledged user to manage its own crontab caused
two problems under solaris:

The uid argument was passed to the execute method to set the correct uid
before executing the crontab command. The execute method has a default
argumenthash with the key :failonfail set to true. Because the suntab
filetype passed it's own hash, failonfail was nil so if crontab
completes with a non-zero exit code no error will be raised.

This is bad because running execute with :uid not only sets the correct
userid in the child process but also tries to run Process.initgroups
to set the correct supplementary groups of that user. This call will most
likely fail as a non priviledged user causing the whole child process to fail.

The fix now makes sure that failonfail is passed to the execute method
so failures can be handled correctly and uid is only passed if necessary
(which is not the case when the target user in the crontab resource is
the same user that is running puppet).

@barn
Copy link
Contributor

barn commented Jun 1, 2012

Thank you for writing this fix, though I fear I may have mislead you with my ticket. The issue was present when running as root in agent mode. My puppet apply usage was just the easiest way of showing this.

I shall test it on the Sol11 box at Puppet next week though and report back.

@kelseyhightower
Copy link

@stschulte Can you rebase this one 2.7.x? @barn where you able to test this on a Sol11 box?

Apply the create/do_stuff/delete cylce of a temp file that is explained
in tempfile.rb to the suntab and aixtab filetype. This way the temporary
file should always be removed.

The name of the temporary file was changed so it will be easier to stub
this single call in tests - without stubbing tempfile creations in other
places (like inside Puppet::Util.execute) at the same time.
@stschulte
Copy link
Contributor Author

@kelseyhightower rebased on current 2.7.x

Running puppet as an unpriviledged user to manage its own crontab caused
two problems under solaris:

The uid argument was passed to the execute method to set the correct uid
before executing the crontab command. The execute method has a default
argumenthash with the key :failonfail set to true. Because the suntab
filetype passed it's own hash, failonfail was nil so if crontab
completes with a non-zero exit code no error will be raised.

This is bad because running execute with :uid not only sets the correct
userid in the child process but also tries to run `Process.initgroups`
to set the correct supplementary groups of that user. This call will most
likely fail as a non priviledged user causing the whole child process to fail.

The fix now makes sure that failonfail is passed to the execute method
so failures can be handled correctly and uid is only passed if necessary
(which is not the case when the target user in the crontab resource is
the same user that is running puppet).
@joshcooper
Copy link
Contributor

@stschulte I pulled in your commits and added some others around exception backtraces and cleaning up whitespace. See https://github.com/joshcooper/puppet/tree/ticket/2.7.x/14283-filetype-uid While doing that, I think some of the legacy code is unnecessary, but I don't have access to an aix system to test it out.

  1. The aixtab type seems to have the same issue that you fixed (it's not calling execute with failonfail)
  2. On line https://github.com/joshcooper/puppet/blob/ticket/2.7.x/14283-filetype-uid/lib/puppet/util/filetype.rb#L209 we're checking the output to determine if the crontab command failed or not. Likely this was due to the execute method not raising an exception on non-zero exit status, which you fixed. So I think we can remove line 209 and my comment above it.
  3. On line https://github.com/joshcooper/puppet/blob/ticket/2.7.x/14283-filetype-uid/lib/puppet/util/filetype.rb#L261, we're doing the same thing, checking the output. This line can probably be removed also (assuming aix crontab returns non-zero on error). But then if that's the case, I don't think there's any difference between aixtab and suntab types.

Can you confirm whether those lines are unnecessary, and also whether the suntab and aixtab providers can be merged.

@stschulte
Copy link
Contributor Author

@joshcooper I cannot say anything about aix but I checked Solaris at $WORK today and

"can't open your crontab"

does indeed return with a non zero exitcode. But I think we should move the

return "" if output.include?("can't open your crontab")

in the ensure block because "can't open your crontab" is raised when "the user can access the crontab command but no crontab file exists" [1] so the user should not see a prefetching failed message in this case I guess.

@stschulte
Copy link
Contributor Author

@joshcooper
Copy link
Contributor

@stschulte Yes, thanks, I had missed the significance of the "no crontab file exists" part. I've updated the branch and will merge once the 2.7.x build failures are resolved.

@joshcooper
Copy link
Contributor

Filed a new PR as #1025

@joshcooper joshcooper closed this Aug 13, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants