There should be a way to allow the root user to see diffs of readable-by-root-only files in rhncfg-actions.

--- Additional comment from Vincent Danen on 2012-11-06 12:11:56 EST ---

So a few thoughts on how to fix this:

- do not log root-owned/non-world-readable diffs to the log file
- _do_ log the diffs to the Satellite server (possibly configurable, but the Satellite server should be protected with authorization to view this, as well as authentication, so it's very different than a local log file and especially a world-readable one)
- allow rhncfg-client to diff files that are owned root/non-world-readable if euid==0 (e.g. if root is running the program interactively (not a root process started by another service like a cron job), show the diff on stdout
- implement a (configurable) blacklist and populate it with things like /etc/shadow right off the start (perhaps using paths and/or globs so as to prevent things like /etc/pki/[private_cert_directory] from being diffed as well)... but make it configurable so that _if_ the client wants to shoot themself in the foot, they have to work at doing it (remove files/paths from the blacklist)

I think if we can do that, then we should be able to still have it secure, but flexible enough that it will meet multiple use-cases.  And if we never output diffs of these protected files to the local log, then regardless of permissions we never have to worry about sensitive info being leaked (because root will be able to see the contents of the files, allowing an interactive diff, by root, should be allowed).

--- Additional comment from Stephen Herr on 2012-11-09 16:26:14 EST ---

I have a patch ready, if we decide to go this route, that adds an option to the "rhncfg-actions diff" command that forces the diff of secure files.

Adding -d or --diff-secure-files would bring back the old behavior of always showing the diff of secure files, whereas the default behavior would be the new, securer way.

The rational here is that only root user can run rhncfg-config, and the root user can view any file he wants to anyway, so we can let him decide what is best for his environment.

Outstanding questions / issues:
1) It is not explicit in the code that you must be root to run rhncfg-config, but it won't work unless you are. I should probably make this more explicit and throw a nice error if a non-root user attempts to run it.

2) I could also add a config setting that allows the admin to set --diff-secure-files permanently.

Is this the road we want to take?

--- Additional comment from Jan Pazdziora on 2012-11-10 11:47:45 EST ---

(In reply to comment #6)
> I have a patch ready, if we decide to go this route, that adds an option to
> the "rhncfg-actions diff" command that forces the diff of secure files.
> 
> Adding -d or --diff-secure-files would bring back the old behavior of always
> showing the diff of secure files, whereas the default behavior would be the
> new, securer way.

If we go the road of having the root to decide something, the default probably should be the original behaviour, seeing how many people were affected.

--- Additional comment from Stephen Herr on 2012-11-12 08:53:49 EST ---

(In reply to comment #7)
> (In reply to comment #6)
> > I have a patch ready, if we decide to go this route, that adds an option to
> > the "rhncfg-actions diff" command that forces the diff of secure files.
> > 
> > Adding -d or --diff-secure-files would bring back the old behavior of always
> > showing the diff of secure files, whereas the default behavior would be the
> > new, securer way.
> 
> If we go the road of having the root to decide something, the default
> probably should be the original behaviour, seeing how many people were
> affected.

That is fine with me. What we cannot do is just revert the changes to bug 824707 and log the differences to a world-readable file like before. But the log file is not world-readable now, and besides we can easily make the output of rhncfg-actions diff useful by default without affecting what gets logged.

--- Additional comment from Jan Pazdziora on 2012-11-12 11:38:53 EST ---

By the way, why is logging the diff and reporting tied together? Can't we report without logging?

--- Additional comment from Vincent Danen on 2012-11-13 12:27:46 EST ---

I'm of the frame of mind that we should keep the current secure default the default, and this option of allowing root to do the diffs of secure files as something an admin has to enable (to allow for the previous less-secure actions).

We've already made this the default, now we're adding an option to allow it again.  I think the fact that we've done so is sufficient.  We should err on the side of greater security and let it be the responsibility of the administrator to lessen the security of the system by allowing this.

Note that I say "less secure" rather than insecure, but we should default to as secure as possible because people won't tighten security unless they're paranoid, but they _will_ relax it if it impacts them, so we should default to the most secure setting.

--- Additional comment from Stephen Herr on 2012-11-15 16:55:48 EST ---

(In reply to comment #9)
> By the way, why is logging the diff and reporting tied together? Can't we
> report without logging?

Unless I'm mistaken we never log the output of 'rhncfg-actions diff', that only ever goes to console. Conversely, the client does it's once-daily pull of config files, the diffs get logged and sent to Satellite, but never sent to the console (which makes sense since there is no user to see it). Both end up calling the same method, but yes we could have one display the diff and the other not. However, if we are defaulting to "more secure" then we probably want it to not display the diff in both cases, the only exception being if the admin forces the diff.

Is that how we want to move ahead then?