unicorn Ruby/Rack server user+dev discussion/patches/pulls/bugs/help
 help / color / mirror / code / Atom feed
From: Eric Wong <e@80x24.org>
To: Jeremy Evans <code@jeremyevans.net>
Cc: unicorn-public@bogomips.org
Subject: Re: Patch: Add after_worker_ready configuration option
Date: Thu, 23 Feb 2017 02:32:21 +0000	[thread overview]
Message-ID: <20170223023221.GB15864@starla> (raw)
In-Reply-To: <20170223004911.GD81807@jeremyevans.local>

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

    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,

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.


(*) 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.

  reply	other threads:[~2017-02-23  2:32 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-02-23  0:49 Patch: Add after_worker_ready configuration option Jeremy Evans
2017-02-23  2:32 ` Eric Wong [this message]
2017-02-23  2:34   ` Eric Wong
2017-02-23  3:43   ` Jeremy Evans
2017-02-23  9:23     ` Eric Wong
2017-02-23 20:00       ` Jeremy Evans

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information: https://yhbt.net/unicorn/

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170223023221.GB15864@starla \
    --to=e@80x24.org \
    --cc=code@jeremyevans.net \
    --cc=unicorn-public@bogomips.org \


* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).