logfile created with root:root ownership when "log_file" config option specified

logfile created with root:root ownership when "log_file" config option specified

[Karol Mroz]

[-- Attachment #1: Type: text/plain, Size: 2148 bytes --]

Greetings Ceph Developers,

I recently discovered that when explicitly setting "log_file" for radosgw in
ceph.conf, the logfile was created with root:root ownership. Initially I thought
that this was a RGW issue, which prompted me to open: http://tracker.ceph.com/issues/14613

Tracing this a bit further, I believe it to be a more general problem with when the
LogObs observer is invoked.

The LogObs observer is first invoked from global_pre_init() by way of:

global_pre_init():
...
  conf->apply_changes(NULL)
...

Which then invokes:

md_config_t::_apply_changes()
{
...
  // Make any pending observer callbacks
  for (rev_obs_map_t::const_iterator r = robs.begin(); r != robs.end(); ++r) {
    md_config_obs_t *obs = r->first;
    obs->handle_conf_change(this, r->second);
  }
...
}

Which in turn fires the LogObs observer code:

class LogObs : public md_config_obs_t {
...
void handle_conf_change()
...
  if (changed.count("log_file")) {
    log->set_log_file(conf->log_file);
    log->reopen_log_file();
  }
...

When "log_file" is specified in ceph.conf, Log::reopen_log_file() is called and
the logfile is open()'d. However, at this point in time, the daemon is still running
as root, and thus the logfile is created with root:root ownership.

A quick workaround is to manually not set "log_file" in ceph.conf. The default
logfile is then created later on in global_init(), by way of:

global_init()
...
  g_conf->call_all_observers();
...

This is called _after_ the correct permissions have been set.

From a quick glance, removing changed.count("log_file"...) from
LogObs::handle_conf_changes() would skip over the early logfile creation in this observer.
A more invasive option would be to remove conf->apply_changes() from global_pre_init(),
thus delaying running the observers for the first time until after the permissions drop.
However, I suspect there may be valid reason why conf->apply_changes() is needed in
global_pre_init()? As I'm not familiar with the nuances here, a comment from someone more
familiar with this would be appreciated.

-- 
Regards,
Karol

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: logfile created with root:root ownership when "log_file" config option specified

[Karol Mroz]

[-- Attachment #1: Type: text/plain, Size: 1155 bytes --]

On Wed, Feb 03, 2016 at 03:17:27PM -0800, Karol Mroz wrote:
[...]
> From a quick glance, removing changed.count("log_file"...) from
> LogObs::handle_conf_changes() would skip over the early logfile creation in this observer.
> A more invasive option would be to remove conf->apply_changes() from global_pre_init(),
> thus delaying running the observers for the first time until after the permissions drop.
> However, I suspect there may be valid reason why conf->apply_changes() is needed in
> global_pre_init()? As I'm not familiar with the nuances here, a comment from someone more
> familiar with this would be appreciated.

Removing the changed.count("log_file") section in LogObs::handle_conf_changes() I think would kill
the possibility of changing the logfile via injectargs. I'd say that's a no-go.

The more I look at it, the more I think running conf->handle_changes(NULL) from global_pre_init()
is not needed. Upon returning to global_init(), we invoke g_conf->call_all_observers() which does
meta variable expansion and runs _all_ observers. This should be enough... or am I oversimplifying
things? :D

-- 
Regards,
Karol

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: logfile created with root:root ownership when "log_file" config option specified

[Sage Weil]

On Fri, 5 Feb 2016, Karol Mroz wrote:
> On Wed, Feb 03, 2016 at 03:17:27PM -0800, Karol Mroz wrote:
> [...]
> > From a quick glance, removing changed.count("log_file"...) from
> > LogObs::handle_conf_changes() would skip over the early logfile creation in this observer.
> > A more invasive option would be to remove conf->apply_changes() from global_pre_init(),
> > thus delaying running the observers for the first time until after the permissions drop.
> > However, I suspect there may be valid reason why conf->apply_changes() is needed in
> > global_pre_init()? As I'm not familiar with the nuances here, a comment from someone more
> > familiar with this would be appreciated.
> 
> Removing the changed.count("log_file") section in LogObs::handle_conf_changes() I think would kill
> the possibility of changing the logfile via injectargs. I'd say that's a no-go.
> 
> The more I look at it, the more I think running conf->handle_changes(NULL) from global_pre_init()
> is not needed. Upon returning to global_init(), we invoke g_conf->call_all_observers() which does
> meta variable expansion and runs _all_ observers. This should be enough... or am I oversimplifying
> things? :D

That sounds like the right thing to me.  These are dark and crufty parts 
of the code, though, so we'll have to make the change and see what 
breaks..

There are only 2 global_pre_init callers: global_init (the next method 
down), and ceph-conf.  Presumably this was split to make ceph-conf happy.  
So this is primarily a matter of looking through global_init to make sure 
nothing depends on metavariable expansion.  The only thing htat catches my 
eye is g_conf->run_dir.  We can probably move that below 
call_all_observers?  Or, put the apply_changes just after we drop 
privileges.

Take a look at

	https://github.com/ceph/ceph/pull/7545

I think this does the trick, and passes my quick smoke test...

sage

Re: logfile created with root:root ownership when "log_file" config option specified

[Karol Mroz]

[-- Attachment #1: Type: text/plain, Size: 1451 bytes --]

On Fri, Feb 05, 2016 at 09:02:58PM -0500, Sage Weil wrote:
[...]
> > The more I look at it, the more I think running conf->handle_changes(NULL) from global_pre_init()
> > is not needed. Upon returning to global_init(), we invoke g_conf->call_all_observers() which does
> > meta variable expansion and runs _all_ observers. This should be enough... or am I oversimplifying
> > things? :D
> 
> That sounds like the right thing to me.  These are dark and crufty parts 
> of the code, though, so we'll have to make the change and see what 
> breaks..
> 
> There are only 2 global_pre_init callers: global_init (the next method 
> down), and ceph-conf.  Presumably this was split to make ceph-conf happy.  
> So this is primarily a matter of looking through global_init to make sure 
> nothing depends on metavariable expansion.  The only thing htat catches my 
> eye is g_conf->run_dir.  We can probably move that below 
> call_all_observers?  Or, put the apply_changes just after we drop 
> privileges.
> 
> Take a look at
> 
> 	https://github.com/ceph/ceph/pull/7545
> 
> I think this does the trick, and passes my quick smoke test...

Yes, this looks good.

One thing... before the logfile is ultimately created, there are several potential calls
to dout(). Is it possible these calls would be lost? Or is there some caching that goes on?
I've not looked too deeply at the logging code.

Thanks!

-- 
Regards,
Karol

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]