Re: [PATCH] Added support for dropping privileges to git-daemon.

From: Junio C Hamano <junkio@cox.net>
Date: Aug 22, 2006, 06:38 UTC
Message-ID: <7vhd05s2b5.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <1156079371.28098@hammerfest>

Tilman Sauerbeck <tilman@code-monkey.de> writes:

Show 20 quoted lines

> +	     [--reuseaddr] [--detach] [--pid-file=file]
> +	     [--user=u [--group=g]] [directory...]
>  
>  DESCRIPTION
>  -----------
> @@ -93,6 +94,14 @@ OPTIONS
>  --pid-file=file::
>  	Save the process id in 'file'.
>  
> +--user=u::
> +--group=g::
> +	These two options may be used to make `git-daemon` change its uid and
> +	gid	before entering the server loop.
> +	The uid that's used is the one of 'u'. If `group` is specified,
> +	the gid is set to the one of 'g', otherwise, the default gid
> +	of 'u' is used.
> +
>  <directory>::
>  	A directory to add to the whitelist of allowed directories. Unless
>  	--strict-paths is specified this will also include subdirectories

I'd prefer to spell <u> and <g> out, and clarify the description that the parameter parsing code would do getpwent() for the user, and the user does not have to supply numeric user and group ids, and what happens if numeric ids are given.

How well does this interact with --inetd (inetd_mode)? Do we want to have some notes in the documentation on that topic?

Show 19 quoted lines

> diff --git a/daemon.c b/daemon.c
> index 012936f..70be10f 100644
> --- a/daemon.c
> +++ b/daemon.c
> @@ -7,6 +7,8 @@ #include <netdb.h>
>  #include <netinet/in.h>
>  #include <arpa/inet.h>
>  #include <syslog.h>
> +#include <pwd.h>
> +#include <grp.h>
>  #include "pkt-line.h"
>  #include "cache.h"
>  #include "exec_cmd.h"
> @@ -14,12 +16,15 @@ #include "exec_cmd.h"
>  static int log_syslog;
>  static int verbose;
>  static int reuseaddr;
> +static const char *user;
> +static const char *group;

Do these have to be file-level global?

Do we want to do getpwnam/getgrnam lookup in drop_privileges()? "Gaaaah, no such username/groupname!" would be needed anyway, and I have a feeling that it might be better done once immediately after argument parsing. Then you can pass whatever is needed for initgroups/setgid/setuid down as parameters to serve() and drop_privileges().

Do we want to use initgroups(), which is _BSD_SOURCE? I guess it is perhaps Ok.

Show 7 quoted lines

> +static void drop_privileges()
> +{
> +	struct passwd *p;
> +	struct group *g;
> +	gid_t gid;
> +
> +	p = getpwnam (user);

No space between function name and open parenthesis, please.

Show 9 quoted lines

> @@ -709,6 +738,9 @@ static int serve(int port)
>  	if (socknum == 0)
>  		die("unable to allocate any listen sockets on port %u", port);
>  
> +	if (user)
> +		drop_privileges();
> +
>  	return service_loop(socknum, socklist);
>  }

Makes one wonder why it checks only user and not group, and then makes one realize that the argument parsing code enforces that group is never supplied without user, which leaves funny taste in the mouth, but that is Ok, I guess.