Patch: Add after_worker_ready configuration option

Patch: Add after_worker_ready configuration option

[Jeremy Evans]

This adds a hook that is called after the application has
been loaded by the worker process, directly before it starts
accepting requests.  This hook is necessary if your application
needs to get gain access to resources during initialization,
and then drop privileges before serving requests.

This is especially useful in conjunction with chroot support
so the app can load all the normal ruby libraries it needs
to function, and then chroot before accepting requests.

If you are preloading the app, it's possible to drop privileges
or chroot in after_fork, but if you are not preloading the app,
there is not currently a properly place to handle this, hence
the reason for this feature.
---
 lib/unicorn/configurator.rb | 17 ++++++++++++++++-
 lib/unicorn/http_server.rb  |  4 ++--
 2 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/lib/unicorn/configurator.rb b/lib/unicorn/configurator.rb
index 5bad925..6df5a30 100644
--- a/lib/unicorn/configurator.rb
+++ b/lib/unicorn/configurator.rb
@@ -49,6 +49,9 @@ class Unicorn::Configurator
           server.logger.error(m)
         end
       },
+    :after_worker_ready => lambda { |server, worker|
+        server.logger.info("worker=#{worker.nr} ready")
+      },
     :pid => nil,
     :preload_app => false,
     :check_client_connection => false,
@@ -162,7 +165,7 @@ def after_fork(*args, &block)
   # sets after_worker_exit hook to a given block.  This block will be called
   # by the master process after a worker exits:
   #
-  #  after_fork do |server,worker,status|
+  #  after_worker_exit do |server,worker,status|
   #    # status is a Process::Status instance for the exited worker process
   #    unless status.success?
   #      server.logger.error("worker process failure: #{status.inspect}")
@@ -172,6 +175,18 @@ def after_worker_exit(*args, &block)
     set_hook(:after_worker_exit, block_given? ? block : args[0], 3)
   end
 
+  # sets after_worker_ready hook to a given block.  This block will be called
+  # by a worker process after it has been fully loaded, directly before it
+  # starts responding to requests:
+  #
+  #  after_worker_ready do |server,worker|
+  #    server.logger.info("worker #{worker.nr} ready, dropping privileges")
+  #    worker.user('username', 'groupname')
+  #  end
+  def after_worker_ready(*args, &block)
+    set_hook(:after_worker_ready, block_given? ? block : args[0])
+  end
+
   # sets before_fork got be a given Proc object.  This Proc
   # object will be called by the master process before forking
   # each worker.
diff --git a/lib/unicorn/http_server.rb b/lib/unicorn/http_server.rb
index c2086cb..ef897ad 100644
--- a/lib/unicorn/http_server.rb
+++ b/lib/unicorn/http_server.rb
@@ -15,7 +15,7 @@ class Unicorn::HttpServer
                 :before_fork, :after_fork, :before_exec,
                 :listener_opts, :preload_app,
                 :orig_app, :config, :ready_pipe, :user
-  attr_writer   :after_worker_exit
+  attr_writer   :after_worker_exit, :after_worker_ready
 
   attr_reader :pid, :logger
   include Unicorn::SocketHelper
@@ -644,7 +644,7 @@ def worker_loop(worker)
     trap(:USR1) { nr = -65536 }
 
     ready = readers.dup
-    @logger.info "worker=#{worker.nr} ready"
+    @after_worker_ready.call(self, worker)
 
     begin
       nr < 0 and reopen_worker_logs(worker.nr)
-- 
2.11.0

Re: Patch: Add after_worker_ready configuration option

[Eric Wong]

Jeremy Evans <code@jeremyevans.net> wrote:
> This adds a hook that is called after the application has
> been loaded by the worker process, directly before it starts
> accepting requests.  This hook is necessary if your application
> needs to get gain access to resources during initialization,
> and then drop privileges before serving requests.
> 
> This is especially useful in conjunction with chroot support
> so the app can load all the normal ruby libraries it needs
> to function, and then chroot before accepting requests.
> 
> If you are preloading the app, it's possible to drop privileges
> or chroot in after_fork, but if you are not preloading the app,
> there is not currently a properly place to handle this, hence
> the reason for this feature.

This means using Unicorn::Worker#user directly,
and not Unicorn::Configurator#user.  OK...

Also, were you planning to send a v2 for chroot support?
I'm fine with either.

> --- a/lib/unicorn/configurator.rb
> +++ b/lib/unicorn/configurator.rb

> @@ -162,7 +165,7 @@ def after_fork(*args, &block)
>    # sets after_worker_exit hook to a given block.  This block will be called
>    # by the master process after a worker exits:
>    #
> -  #  after_fork do |server,worker,status|
> +  #  after_worker_exit do |server,worker,status|
>    #    # status is a Process::Status instance for the exited worker process
>    #    unless status.success?
>    #      server.logger.error("worker process failure: #{status.inspect}")

This looks like an unrelated hunk which belongs in a separate
patch.  I can squeeze in a separate fix for this (credited to
you) or you can send a separate patch.

> @@ -172,6 +175,18 @@ def after_worker_exit(*args, &block)
>      set_hook(:after_worker_exit, block_given? ? block : args[0], 3)
>    end
>  
> +  # sets after_worker_ready hook to a given block.  This block will be called
> +  # by a worker process after it has been fully loaded, directly before it
> +  # starts responding to requests:
> +  #
> +  #  after_worker_ready do |server,worker|
> +  #    server.logger.info("worker #{worker.nr} ready, dropping privileges")
> +  #    worker.user('username', 'groupname')
> +  #  end

The documentation should probably say something along the lines
of:

    Do not use Unicorn::Configurator#user if you rely on
    changing users in the after_worker_ready hook.

And maybe corresponding documentation for Unicorn::Configurator#user,
too.

Anyways, I'm somewhat inclined to accept this as well, but will
think about it a bit, too.

One problem I have with this change is that it requires users
read more documentation and know more caveats to use.

Adding to that, Unicorn::Configurator#user has been favored
(documentation-wise) over Unicorn::Worker#user in the past;
so it's a bit of a reversal(*)

Based on the series of changes you're making, I'm sensing you're
working on some complex system which might need more security
than a typical app.

I know you use OpenBSD, and I don't know much about the init
system or how deployment works, there.

Nowadays, it seems more common things (binding sockets,
switching users, chrooting, etc..) is handled by init systems
(at least systemd); and having that functionality in unicorn
would mean redundant bloat.

It would be useful to me and folks who won't use these features
to understand how and why they're used.  That can help justify
the (admittedly small) memory overhead which comes with it.

Thanks.


(*) Fwiw, I wasn't thrilled to add user switching to unicorn back
    in 2009/2010, as it should not need to run privileged ports.
    So I'm wondering if there can be some of that which could be
    trimmed out someday or made optional, somehow.

Re: Patch: Add after_worker_ready configuration option

[Eric Wong]

Eric Wong <e@80x24.org> wrote:
> Also, were you planning to send a v2 for chroot support?

+ Or did you want me to squash the explicit path change in?

> I'm fine with either.

Re: Patch: Add after_worker_ready configuration option

[Jeremy Evans]

On 02/23 02:32, Eric Wong wrote:
> Jeremy Evans <code@jeremyevans.net> wrote:
> > This adds a hook that is called after the application has
> > been loaded by the worker process, directly before it starts
> > accepting requests.  This hook is necessary if your application
> > needs to get gain access to resources during initialization,
> > and then drop privileges before serving requests.
> > 
> > This is especially useful in conjunction with chroot support
> > so the app can load all the normal ruby libraries it needs
> > to function, and then chroot before accepting requests.
> > 
> > If you are preloading the app, it's possible to drop privileges
> > or chroot in after_fork, but if you are not preloading the app,
> > there is not currently a properly place to handle this, hence
> > the reason for this feature.
> 
> This means using Unicorn::Worker#user directly,
> and not Unicorn::Configurator#user.  OK...

Correct.  We could add configuration support to specify when to call
Worker#user, but that seems like overkill.
 
> Also, were you planning to send a v2 for chroot support?
> I'm fine with either.

Sorry about that, I will send a v2 for chroot support tomorrow.

> 
> > --- a/lib/unicorn/configurator.rb
> > +++ b/lib/unicorn/configurator.rb
> 
> > @@ -162,7 +165,7 @@ def after_fork(*args, &block)
> >    # sets after_worker_exit hook to a given block.  This block will be called
> >    # by the master process after a worker exits:
> >    #
> > -  #  after_fork do |server,worker,status|
> > +  #  after_worker_exit do |server,worker,status|
> >    #    # status is a Process::Status instance for the exited worker process
> >    #    unless status.success?
> >    #      server.logger.error("worker process failure: #{status.inspect}")
> 
> This looks like an unrelated hunk which belongs in a separate
> patch.  I can squeeze in a separate fix for this (credited to
> you) or you can send a separate patch.
 
Correct, this was an unrelated bug in my last patch.  I'll separate this
hunk into a separate patch.

> > @@ -172,6 +175,18 @@ def after_worker_exit(*args, &block)
> >      set_hook(:after_worker_exit, block_given? ? block : args[0], 3)
> >    end
> >  
> > +  # sets after_worker_ready hook to a given block.  This block will be called
> > +  # by a worker process after it has been fully loaded, directly before it
> > +  # starts responding to requests:
> > +  #
> > +  #  after_worker_ready do |server,worker|
> > +  #    server.logger.info("worker #{worker.nr} ready, dropping privileges")
> > +  #    worker.user('username', 'groupname')
> > +  #  end
> 
> The documentation should probably say something along the lines
> of:
> 
>     Do not use Unicorn::Configurator#user if you rely on
>     changing users in the after_worker_ready hook.
> 
> And maybe corresponding documentation for Unicorn::Configurator#user,
> too.

OK, I'll update the documentation to be more complete.

> Anyways, I'm somewhat inclined to accept this as well, but will
> think about it a bit, too.
> 
> One problem I have with this change is that it requires users
> read more documentation and know more caveats to use.
 
It's unfortunate, but I think it's necessary in this case. Unicorn's
default behavior works for most users, but this makes it so you
don't have to override HttpServer#init_worker_process to get similar
functionality.

> Adding to that, Unicorn::Configurator#user has been favored
> (documentation-wise) over Unicorn::Worker#user in the past;
> so it's a bit of a reversal(*)

I think Configurator#user still makes sense for most users.  As the
documentation mentions, directly calling Worker#user should only
be done if Configurator#user is not sufficient.

> Based on the series of changes you're making, I'm sensing you're
> working on some complex system which might need more security
> than a typical app.

These patches are related to all applications using Unicorn that I run
on OpenBSD (about 20 separate applications).  They are designed to
to reduce the attack surface if an attacker is able to find a
remote code execution vulnerability in one of the applications.

In addition to chroot(2), after chrooting, these applications also
use pledge(2), which is an OpenBSD-specific system call filter
(similar in principle to seccomp(2) in Linux).

> I know you use OpenBSD, and I don't know much about the init
> system or how deployment works, there.
 
It's a very simple sh(1) based-init system, similar to traditional Unix.
Most OpenBSD system daemons will start as root, chroot to a specific
directory, drop privileges (possibly separate privileges in separate
daemon processes connected via IPC), and finally use pledge(2) to limit
what each process can actually use in terms of system calls.

> Nowadays, it seems more common things (binding sockets,
> switching users, chrooting, etc..) is handled by init systems
> (at least systemd); and having that functionality in unicorn
> would mean redundant bloat.

I believe only Linux uses a systemd-like approach, and while most
Linux systems use systemd these days, usage is not universal
(notable exceptions are Slackware, Void, and Gentoo).  But my
knowledge of other operating systems is not very broad.

> It would be useful to me and folks who won't use these features
> to understand how and why they're used.  That can help justify
> the (admittedly small) memory overhead which comes with it.

True.  I'm sorry the commit messages didn't provide a more thorough
explanation.  I will try to improve that in the future.

> (*) Fwiw, I wasn't thrilled to add user switching to unicorn back
>     in 2009/2010, as it should not need to run privileged ports.

I think had you not added it, there wouldn't be a need for a chroot
patch, since otherwise one would assume any user that wanted to
drop privileges would just run the necessary code themselves.

One thing to keep in mind is that if you did not add the user switching
code, developers who need it would implement it themselves, and would
likely miss things like initgroups.

>     So I'm wondering if there can be some of that which could be
>     trimmed out someday or made optional, somehow.

I'm not sure how open you are to a plugin-like system for Unicorn, where
you can have separate files for separate features, and users can choose
which features the want to use.  This limits memory usage so that you
don't pay a memory cost for features you don't use.  This is the
approach used by Sequel, Roda, and Rodauth (other libraries I maintain),
and if you are interested in a similar system for Unicorn, I would be
happy to work on one.

I do have a couple of additional patches I plan to send, but I think it
may be better to ask here first.

One is that SIGUSR1 handling doesn't work well with chroot if the log
file is outside of the chroot, since the worker process may not have
access to reopen the file.  One simple fix is to reopen the logs in
the master process, then send SIGQUIT instead of SIGUSR1 to the child
processes.  Are you open to a patch that does that?

The second feature is more exotic security feature called fork+exec.
The current issue with unicorn's direct forking of children is that
forked children share the memory layout of the server.  This is good
from a memory usage standpoint due to CoW memory.  However, if there
is a memory vulnerability in the unicorn application, this makes it
easier to exploit, since if the a unicorn worker process crashes, the
master process will fork a child with pretty much the same memory
layout.  This allows address space discovery attacks.  Using fork+exec,
each child process would have a unique address space for ASLR, on
most Unix-like systems (Linux, MacOS X, NetBSD, OpenBSD, Solaris).
There are additional security advantages from fork+exec on OpenBSD, but
all Unix-like systems that use ASLR can benefit from the unique
memory layout per process.

Thanks,
Jeremy

Re: Patch: Add after_worker_ready configuration option

[Eric Wong]

Jeremy Evans <code@jeremyevans.net> wrote:

<snip> acknowledging everything above.

> On 02/23 02:32, Eric Wong wrote:
> > Jeremy Evans <code@jeremyevans.net> wrote:
> > Anyways, I'm somewhat inclined to accept this as well, but will
> > think about it a bit, too.
> > 
> > One problem I have with this change is that it requires users
> > read more documentation and know more caveats to use.
>  
> It's unfortunate, but I think it's necessary in this case. Unicorn's
> default behavior works for most users, but this makes it so you
> don't have to override HttpServer#init_worker_process to get similar
> functionality.

Yeah.  Unfortunately, with every option comes more
support/maintenance overhead.  So I'm trying to make sure we've
explored every possible avenue before adding this new option.

<snip>

> > (*) Fwiw, I wasn't thrilled to add user switching to unicorn back
> >     in 2009/2010, as it should not need to run privileged ports.
> 
> I think had you not added it, there wouldn't be a need for a chroot
> patch, since otherwise one would assume any user that wanted to
> drop privileges would just run the necessary code themselves.
> 
> One thing to keep in mind is that if you did not add the user switching
> code, developers who need it would implement it themselves, and would
> likely miss things like initgroups.

Heh.  As I was mentioning above, every feature can come with
new, unintended consequences :>  But yeah, maybe push things
up to Ruby stdlib, or systemd...

> >     So I'm wondering if there can be some of that which could be
> >     trimmed out someday or made optional, somehow.
> 
> I'm not sure how open you are to a plugin-like system for Unicorn, where
> you can have separate files for separate features, and users can choose
> which features the want to use.  This limits memory usage so that you
> don't pay a memory cost for features you don't use.  This is the
> approach used by Sequel, Roda, and Rodauth (other libraries I maintain),
> and if you are interested in a similar system for Unicorn, I would be
> happy to work on one.

Not really.  As I've stated many times in the past, unicorn is
just one of many servers available for Rack users; so I've been
against people building too many dependencies on unicorn, that
comes at the expense of other servers.

What I had in mind was having a shim process which does socket
setup, user switching, etc. before exec-ing unicorn in cases
where a systemd-like spawner is not used.  But it's probably too
small to make an impact on real apps anyways, since Ruby still
has tons of methods which never get called, either.

> I do have a couple of additional patches I plan to send, but I think it
> may be better to ask here first.
> 
> One is that SIGUSR1 handling doesn't work well with chroot if the log
> file is outside of the chroot, since the worker process may not have
> access to reopen the file.  One simple fix is to reopen the logs in
> the master process, then send SIGQUIT instead of SIGUSR1 to the child
> processes.  Are you open to a patch that does that?

Right now, the worker exits with some log spew if reopening
logs fails, and the worker respawns anyways, correct?

Perhaps that mapping can be made automatic if chroot is
enabled.  I wonder if that's too much magic...
Or the log spew could be suppressed when the error
is ENOENT and chroot is enabled.

The sysadmin could also send SIGHUP to reload everything
and respawn all children.

So, I'm wondering if using SIGHUP is too much of a burden, or if
automatic USR1 => QUIT mapping for chroot users is too magical.

(trying to avoid adding more options + documentation, as always :)

> The second feature is more exotic security feature called fork+exec.
> The current issue with unicorn's direct forking of children is that
> forked children share the memory layout of the server.  This is good
> from a memory usage standpoint due to CoW memory.  However, if there
> is a memory vulnerability in the unicorn application, this makes it
> easier to exploit, since if the a unicorn worker process crashes, the
> master process will fork a child with pretty much the same memory
> layout.  This allows address space discovery attacks.  Using fork+exec,
> each child process would have a unique address space for ASLR, on
> most Unix-like systems (Linux, MacOS X, NetBSD, OpenBSD, Solaris).
> There are additional security advantages from fork+exec on OpenBSD, but
> all Unix-like systems that use ASLR can benefit from the unique
> memory layout per process.

It depends how intrusive the implementation is...

Is this merely calling exec in the worker?

I'm not sure if your use of "fork+exec" is merely the two
well-known syscalls combined, or if there's something else;
I'll assume the former, since that's enough to have a
unique address space for Ruby bytecode.

Can this be done in an after_fork/after_worker_ready hook?

Socket inheritance can be done the same way it handles USR2
upgrades and systemd socket inheritance.

Re: Patch: Add after_worker_ready configuration option

[Jeremy Evans]

On 02/23 09:23, Eric Wong wrote:
> Jeremy Evans <code@jeremyevans.net> wrote:
> > On 02/23 02:32, Eric Wong wrote:
> > > Jeremy Evans <code@jeremyevans.net> wrote:
> > > Anyways, I'm somewhat inclined to accept this as well, but will
> > > think about it a bit, too.
> > > 
> > > One problem I have with this change is that it requires users
> > > read more documentation and know more caveats to use.
> >  
> > It's unfortunate, but I think it's necessary in this case. Unicorn's
> > default behavior works for most users, but this makes it so you
> > don't have to override HttpServer#init_worker_process to get similar
> > functionality.
> 
> Yeah.  Unfortunately, with every option comes more
> support/maintenance overhead.  So I'm trying to make sure we've
> explored every possible avenue before adding this new option.

I agree.  If I knew of a way to get this functionality without
overriding a private method, I would use it.  I'm not sure there is a
way currently, which I why I worked on the patch.

> > >     So I'm wondering if there can be some of that which could be
> > >     trimmed out someday or made optional, somehow.
> > 
> > I'm not sure how open you are to a plugin-like system for Unicorn, where
> > you can have separate files for separate features, and users can choose
> > which features the want to use.  This limits memory usage so that you
> > don't pay a memory cost for features you don't use.  This is the
> > approach used by Sequel, Roda, and Rodauth (other libraries I maintain),
> > and if you are interested in a similar system for Unicorn, I would be
> > happy to work on one.
> 
> Not really.  As I've stated many times in the past, unicorn is
> just one of many servers available for Rack users; so I've been
> against people building too many dependencies on unicorn, that
> comes at the expense of other servers.
 
True, but there are features that only unicorn provides.  If you want
those features, you have to use unicorn anyway. 

> What I had in mind was having a shim process which does socket
> setup, user switching, etc. before exec-ing unicorn in cases
> where a systemd-like spawner is not used.  But it's probably too
> small to make an impact on real apps anyways, since Ruby still
> has tons of methods which never get called, either.

I could see that decreasing memory usage slightly, but I agree it's
unlikely to make a significant impact.

> > I do have a couple of additional patches I plan to send, but I think it
> > may be better to ask here first.
> > 
> > One is that SIGUSR1 handling doesn't work well with chroot if the log
> > file is outside of the chroot, since the worker process may not have
> > access to reopen the file.  One simple fix is to reopen the logs in
> > the master process, then send SIGQUIT instead of SIGUSR1 to the child
> > processes.  Are you open to a patch that does that?
> 
> Right now, the worker exits with some log spew if reopening
> logs fails, and the worker respawns anyways, correct?
> 
> Perhaps that mapping can be made automatic if chroot is
> enabled.  I wonder if that's too much magic...
> Or the log spew could be suppressed when the error
> is ENOENT and chroot is enabled.
> 
> The sysadmin could also send SIGHUP to reload everything
> and respawn all children.
> 
> So, I'm wondering if using SIGHUP is too much of a burden, or if
> automatic USR1 => QUIT mapping for chroot users is too magical.

SIGHUP is actually fine.  I didn't realize it reopened log files
in the master process, but apparently it does, so I can just use
SIGHUP instead of SIGUSR1.

> > The second feature is more exotic security feature called fork+exec.
> > The current issue with unicorn's direct forking of children is that
> > forked children share the memory layout of the server.  This is good
> > from a memory usage standpoint due to CoW memory.  However, if there
> > is a memory vulnerability in the unicorn application, this makes it
> > easier to exploit, since if the a unicorn worker process crashes, the
> > master process will fork a child with pretty much the same memory
> > layout.  This allows address space discovery attacks.  Using fork+exec,
> > each child process would have a unique address space for ASLR, on
> > most Unix-like systems (Linux, MacOS X, NetBSD, OpenBSD, Solaris).
> > There are additional security advantages from fork+exec on OpenBSD, but
> > all Unix-like systems that use ASLR can benefit from the unique
> > memory layout per process.
> 
> It depends how intrusive the implementation is...

That I don't know yet.  I haven't implemented fork+exec before, so I'm
not sure how many changes are needed. I probably won't be able to work
on this right away.

> Is this merely calling exec in the worker?
> 
> I'm not sure if your use of "fork+exec" is merely the two
> well-known syscalls combined, or if there's something else;
> I'll assume the former, since that's enough to have a
> unique address space for Ruby bytecode.
> 
> Can this be done in an after_fork/after_worker_ready hook?
> 
> Socket inheritance can be done the same way it handles USR2
> upgrades and systemd socket inheritance.

I expect in addition to socket inheritance, we will need to handle pipe
inheritance for the @to_io and @master pipes in the worker.

Thanks,
Jeremy