git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] Added support for dropping privileges to git-daemon.
@ 2006-08-19 12:27 Tilman Sauerbeck
  2006-08-19 13:23 ` Marco Costalba
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Tilman Sauerbeck @ 2006-08-19 12:27 UTC (permalink / raw)
  To: git

Signed-off-by: Tilman Sauerbeck <tilman@code-monkey.de>
---

 I'm not sure how useful this is. I'd like to start git-daemon as root,
 so it can store it's PID in /var/run, but I don't want it to keep root
 privileges. My git repos are world readable, so for serving them, root
 privileges aren't needed at all.

 What do you think?


 Documentation/git-daemon.txt |    8 +++++++-
 daemon.c                     |   39 ++++++++++++++++++++++++++++++++++++++-
 2 files changed, 45 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 0f7d274..a8d75d9 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all]
              [--timeout=n] [--init-timeout=n] [--strict-paths]
              [--base-path=path] [--user-path | --user-path=path]
-	     [--reuseaddr] [--detach] [--pid-file=file] [directory...]
+	     [--reuseaddr] [--detach] [--pid-file=file]
+	     [--user=u] [--group=g] [directory...]
 
 DESCRIPTION
 -----------
@@ -93,6 +94,11 @@ OPTIONS
 --pid-file=file::
 	Save the process id in 'file'.
 
+--user=u::
+--group=g::
+	If both options are given, `git-daemon` will change it's uid and gid to
+	the ones of 'u' and 'g' before entering the server loop.
+
 <directory>::
 	A directory to add to the whitelist of allowed directories. Unless
 	--strict-paths is specified this will also include subdirectories
diff --git a/daemon.c b/daemon.c
index 012936f..78658c1 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;
 
 static const char daemon_usage[] =
 "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n"
 "           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
 "           [--base-path=path] [--user-path | --user-path=path]\n"
-"           [--reuseaddr] [--detach] [--pid-file=file] [directory...]";
+"           [--reuseaddr] [--detach] [--pid-file=file]\n"
+"           [--user=u] [--group=g] [directory...]";
 
 /* List of acceptable pathname prefixes */
 static char **ok_paths;
@@ -701,6 +706,24 @@ static void store_pid(const char *path)
 	fclose(f);
 }
 
+static void drop_privileges()
+{
+	struct passwd *p;
+	struct group *g;
+
+	p = getpwnam (user);
+	if (!p)
+		die("user not found - %s", user);
+
+	g = getgrnam (group);
+	if (!g)
+		die("group not found - %s", group);
+
+	if (initgroups (p->pw_name, g->gr_gid) || setgid (g->gr_gid) ||
+	    setuid (p->pw_uid))
+		die("cannot drop privileges");
+}
+
 static int serve(int port)
 {
 	int socknum, *socklist;
@@ -709,6 +732,9 @@ static int serve(int port)
 	if (socknum == 0)
 		die("unable to allocate any listen sockets on port %u", port);
 
+	if (user && group)
+		drop_privileges();
+
 	return service_loop(socknum, socklist);
 }
 
@@ -791,6 +817,14 @@ int main(int argc, char **argv)
 			log_syslog = 1;
 			continue;
 		}
+		if (!strncmp(arg, "--user=", 7)) {
+			user = arg + 7;
+			continue;
+		}
+		if (!strncmp(arg, "--group=", 8)) {
+			group = arg + 8;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
@@ -802,6 +836,9 @@ int main(int argc, char **argv)
 		usage(daemon_usage);
 	}
 
+	if (!user ^ !group)
+		die("either set both user and group or none of them");
+
 	if (log_syslog) {
 		openlog("git-daemon", 0, LOG_DAEMON);
 		set_die_routine(daemon_die);
-- 
1.4.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 17:25 ` Mitchell Blank Jr
@ 2006-08-19 12:27   ` Tilman Sauerbeck
  2006-08-22  6:38     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Tilman Sauerbeck @ 2006-08-19 12:27 UTC (permalink / raw)
  To: git

Signed-off-by: Tilman Sauerbeck <tilman@code-monkey.de>
---

 My idea was to keep the code simple :)

 Anyway, this patch has the code you proposed.

 Documentation/git-daemon.txt |   11 +++++++++-
 daemon.c                     |   45 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 0f7d274..3c1bc85 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all]
              [--timeout=n] [--init-timeout=n] [--strict-paths]
              [--base-path=path] [--user-path | --user-path=path]
-	     [--reuseaddr] [--detach] [--pid-file=file] [directory...]
+	     [--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
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;
 
 static const char daemon_usage[] =
 "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n"
 "           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
 "           [--base-path=path] [--user-path | --user-path=path]\n"
-"           [--reuseaddr] [--detach] [--pid-file=file] [directory...]";
+"           [--reuseaddr] [--detach] [--pid-file=file]\n"
+"           [--user=u] [--group=g] [directory...]";
 
 /* List of acceptable pathname prefixes */
 static char **ok_paths;
@@ -701,6 +706,30 @@ static void store_pid(const char *path)
 	fclose(f);
 }
 
+static void drop_privileges()
+{
+	struct passwd *p;
+	struct group *g;
+	gid_t gid;
+
+	p = getpwnam (user);
+	if (!p)
+		die("user not found - %s", user);
+
+	if (!group)
+		gid = p->pw_gid;
+	else {
+		g = getgrnam (group);
+		if (!g)
+			die("group not found - %s", group);
+
+		gid = g->gr_gid;
+	}
+
+	if (initgroups (p->pw_name, gid) || setgid (gid) || setuid (p->pw_uid))
+		die("cannot drop privileges");
+}
+
 static int serve(int port)
 {
 	int socknum, *socklist;
@@ -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);
 }
 
@@ -791,6 +823,14 @@ int main(int argc, char **argv)
 			log_syslog = 1;
 			continue;
 		}
+		if (!strncmp(arg, "--user=", 7)) {
+			user = arg + 7;
+			continue;
+		}
+		if (!strncmp(arg, "--group=", 8)) {
+			group = arg + 8;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
@@ -802,6 +842,9 @@ int main(int argc, char **argv)
 		usage(daemon_usage);
 	}
 
+	if (group && !user)
+		die("--group supplied without --user");
+
 	if (log_syslog) {
 		openlog("git-daemon", 0, LOG_DAEMON);
 		set_die_routine(daemon_die);
-- 
1.4.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 12:27 [PATCH] Added support for dropping privileges to git-daemon Tilman Sauerbeck
@ 2006-08-19 13:23 ` Marco Costalba
  2006-08-19 13:29   ` Tilman Sauerbeck
  2006-08-19 13:32 ` Johannes Schindelin
  2006-08-19 17:25 ` Mitchell Blank Jr
  2 siblings, 1 reply; 13+ messages in thread
From: Marco Costalba @ 2006-08-19 13:23 UTC (permalink / raw)
  To: Tilman Sauerbeck; +Cc: git

>
> +       if (!user ^ !group)
> +               die("either set both user and group or none of them");
> +
>

Just a  question. Why simply not

       if (user ^ group)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 13:23 ` Marco Costalba
@ 2006-08-19 13:29   ` Tilman Sauerbeck
  2006-08-19 15:19     ` Marco Costalba
  0 siblings, 1 reply; 13+ messages in thread
From: Tilman Sauerbeck @ 2006-08-19 13:29 UTC (permalink / raw)
  To: git

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

Marco Costalba [2006-08-19 15:23]:
> >
> >+       if (!user ^ !group)
> >+               die("either set both user and group or none of them");
> >+
> >
> 
> Just a  question. Why simply not
> 
>       if (user ^ group)

Because gcc doesn't like that:
 error: invalid operands to binary ^

Regards,
Tilman

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 12:27 [PATCH] Added support for dropping privileges to git-daemon Tilman Sauerbeck
  2006-08-19 13:23 ` Marco Costalba
@ 2006-08-19 13:32 ` Johannes Schindelin
  2006-08-19 17:25 ` Mitchell Blank Jr
  2 siblings, 0 replies; 13+ messages in thread
From: Johannes Schindelin @ 2006-08-19 13:32 UTC (permalink / raw)
  To: Tilman Sauerbeck; +Cc: git

Hi,

On Sat, 19 Aug 2006, Tilman Sauerbeck wrote:

>  What do you think?

I think it is a good addition. Note that most people will probably use 
inetd instead, though.

> +	     [--user=u] [--group=g] [directory...]

Since you enforce both --user and --group, this should read

	 [--user=u --group=g]

Ciao,
Dscho

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 13:29   ` Tilman Sauerbeck
@ 2006-08-19 15:19     ` Marco Costalba
  2006-08-19 15:22       ` Marco Costalba
  2006-08-19 17:15       ` Mitchell Blank Jr
  0 siblings, 2 replies; 13+ messages in thread
From: Marco Costalba @ 2006-08-19 15:19 UTC (permalink / raw)
  To: git

On 8/19/06, Tilman Sauerbeck <tilman@code-monkey.de> wrote:
> Marco Costalba [2006-08-19 15:23]:
> > >
> > >+       if (!user ^ !group)
> > >+               die("either set both user and group or none of them");
> > >+
> > >
> >
> > Just a  question. Why simply not
> >
> >       if (user ^ group)
>
> Because gcc doesn't like that:
>  error: invalid operands to binary ^
>

Interesting!

Indeed gcc it's right, operator ^ it's a bitwise operator, not a
logical one. So operands must be booleans (or numerical types in case
of C).

The trick is that operator '!' performs an implicit conversion to
integral on the 'user' and 'group' pointers.

BTW the following (very ugly) works.

         if ((int)user ^ (int)group)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 15:19     ` Marco Costalba
@ 2006-08-19 15:22       ` Marco Costalba
  2006-08-19 17:15       ` Mitchell Blank Jr
  1 sibling, 0 replies; 13+ messages in thread
From: Marco Costalba @ 2006-08-19 15:22 UTC (permalink / raw)
  To: git

> logical one. So operands must be booleans (or numerical types in case
> of C).

Operands must be numerical types of course, not booleans!!

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 15:19     ` Marco Costalba
  2006-08-19 15:22       ` Marco Costalba
@ 2006-08-19 17:15       ` Mitchell Blank Jr
  1 sibling, 0 replies; 13+ messages in thread
From: Mitchell Blank Jr @ 2006-08-19 17:15 UTC (permalink / raw)
  To: Marco Costalba; +Cc: git

Marco Costalba wrote:
> >> >+       if (!user ^ !group)
> >> >+               die("either set both user and group or none of them");
> 
> BTW the following (very ugly) works.
> 
>         if ((int)user ^ (int)group)

No it doesn't.  Besides being a dangerous cast (no guarantee that a pointer
will fit in an "int") your code is basically just a fancy way of saying

	if (user != group)

Which is definitely NOT what is intended.  "user" and "group" are pointers --
unless they're both NULL we expect them to have different values.  The
original code is equivalent to:

	if ((user == NULL) != (group == NULL))

which is what is actually intended.

-Mitch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 12:27 [PATCH] Added support for dropping privileges to git-daemon Tilman Sauerbeck
  2006-08-19 13:23 ` Marco Costalba
  2006-08-19 13:32 ` Johannes Schindelin
@ 2006-08-19 17:25 ` Mitchell Blank Jr
  2006-08-19 12:27   ` Tilman Sauerbeck
  2 siblings, 1 reply; 13+ messages in thread
From: Mitchell Blank Jr @ 2006-08-19 17:25 UTC (permalink / raw)
  To: Tilman Sauerbeck; +Cc: git

Tilman Sauerbeck wrote:
> +	if (user && group)
> +		drop_privileges();

It seems "if (user)" would be sufficient here.

> +	if (!user ^ !group)
> +		die("either set both user and group or none of them");

For simplicities sake I'd actually suggest allowing group==NULL.  So
change this test to be

	if (group && !user)
		die("--group supplied without --user");

Then in drop_privileges() do something like:

	struct passwd *p;
	gid_t gid;

	p = getpwnam(user);
	if (!p)
		die("user not found - %s", user);
	if (group == NULL)
		gid = p->pw_gid;
	else {
		struct group *g = getgrnam(group);
		if (!g)
			die("group not found - %s", group);
		gid = g->gr_gid;
	}

Since usually you want to use the same gid that is normally associated
with that pid, this just makes things a little easier on the user

-Mitch

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-19 12:27   ` Tilman Sauerbeck
@ 2006-08-22  6:38     ` Junio C Hamano
  2006-08-22 17:37       ` Tilman Sauerbeck
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2006-08-22  6:38 UTC (permalink / raw)
  To: Tilman Sauerbeck; +Cc: git

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

> +	     [--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? 

> 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.

> +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.

> @@ -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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-22  6:38     ` Junio C Hamano
@ 2006-08-22 17:37       ` Tilman Sauerbeck
  2006-08-22 23:40         ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Tilman Sauerbeck @ 2006-08-22 17:37 UTC (permalink / raw)
  To: git

Signed-off-by: Tilman Sauerbeck <tilman@code-monkey.de>
---

 This one got the changes you proposed.

 Not sure whether the documentation is good enough, I'm not that good at
 writing tech docs in english :(


 Documentation/git-daemon.txt |   18 ++++++++++++++++
 daemon.c                     |   46 ++++++++++++++++++++++++++++++++++++++----
 2 files changed, 59 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 0f7d274..b591895 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -11,7 +11,8 @@ SYNOPSIS
 'git-daemon' [--verbose] [--syslog] [--inetd | --port=n] [--export-all]
              [--timeout=n] [--init-timeout=n] [--strict-paths]
              [--base-path=path] [--user-path | --user-path=path]
-	     [--reuseaddr] [--detach] [--pid-file=file] [directory...]
+	     [--reuseaddr] [--detach] [--pid-file=file]
+	     [--user=user [--group=group]] [directory...]
 
 DESCRIPTION
 -----------
@@ -93,6 +94,21 @@ OPTIONS
 --pid-file=file::
 	Save the process id in 'file'.
 
+--user=user::
+--group=group::
+	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 'user'. If `group` is specified,
+	the gid is set to the one of 'group', otherwise, the default gid
+	of 'user' is used.
+
+	Both `group` and `user` need to be passed as the name of the resp of
+	the group, ie you'll get unexpected results if you pass an uid/a gid.
+
+	Note that you probably don't want to use these options if you run
+	git-daemon in inetd mode, since inetd can do the privilege dropping
+	for you.
+
 <directory>::
 	A directory to add to the whitelist of allowed directories. Unless
 	--strict-paths is specified this will also include subdirectories
diff --git a/daemon.c b/daemon.c
index 012936f..4e94210 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"
@@ -19,7 +21,8 @@ static const char daemon_usage[] =
 "git-daemon [--verbose] [--syslog] [--inetd | --port=n] [--export-all]\n"
 "           [--timeout=n] [--init-timeout=n] [--strict-paths]\n"
 "           [--base-path=path] [--user-path | --user-path=path]\n"
-"           [--reuseaddr] [--detach] [--pid-file=file] [directory...]";
+"           [--reuseaddr] [--detach] [--pid-file=file]\n"
+"           [--user=user [[--group=group]] [directory...]";
 
 /* List of acceptable pathname prefixes */
 static char **ok_paths;
@@ -701,7 +704,7 @@ static void store_pid(const char *path)
 	fclose(f);
 }
 
-static int serve(int port)
+static int serve(int port, struct passwd *pass, gid_t gid)
 {
 	int socknum, *socklist;
 
@@ -709,6 +712,11 @@ static int serve(int port)
 	if (socknum == 0)
 		die("unable to allocate any listen sockets on port %u", port);
 
+	if (pass && gid &&
+	    (initgroups(pass->pw_name, gid) || setgid (gid) ||
+	     setuid(pass->pw_uid)))
+		die("cannot drop privileges");
+
 	return service_loop(socknum, socklist);
 }
 
@@ -716,8 +724,11 @@ int main(int argc, char **argv)
 {
 	int port = DEFAULT_GIT_PORT;
 	int inetd_mode = 0;
-	const char *pid_file = NULL;
+	const char *pid_file = NULL, *user_name = NULL, *group_name = NULL;
 	int detach = 0;
+	struct passwd *pass = NULL;
+	struct group *group;
+	gid_t gid = 0;
 	int i;
 
 	/* Without this we cannot rely on waitpid() to tell
@@ -791,6 +802,14 @@ int main(int argc, char **argv)
 			log_syslog = 1;
 			continue;
 		}
+		if (!strncmp(arg, "--user=", 7)) {
+			user_name = arg + 7;
+			continue;
+		}
+		if (!strncmp(arg, "--group=", 8)) {
+			group_name = arg + 8;
+			continue;
+		}
 		if (!strcmp(arg, "--")) {
 			ok_paths = &argv[i+1];
 			break;
@@ -802,6 +821,25 @@ int main(int argc, char **argv)
 		usage(daemon_usage);
 	}
 
+	if (group_name && !user_name)
+		die("--group supplied without --user");
+
+	if (user_name) {
+		pass = getpwnam(user_name);
+		if (!pass)
+			die("user not found - %s", user_name);
+
+		if (!group_name)
+			gid = pass->pw_gid;
+		else {
+			group = getgrnam(group_name);
+			if (!group)
+				die("group not found - %s", group_name);
+
+			gid = group->gr_gid;
+		}
+	}
+
 	if (log_syslog) {
 		openlog("git-daemon", 0, LOG_DAEMON);
 		set_die_routine(daemon_die);
@@ -831,5 +869,5 @@ int main(int argc, char **argv)
 	if (pid_file)
 		store_pid(pid_file);
 
-	return serve(port);
+	return serve(port, pass, gid);
 }
-- 
1.4.2

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-22 17:37       ` Tilman Sauerbeck
@ 2006-08-22 23:40         ` Junio C Hamano
  2006-08-23 16:45           ` Tilman Sauerbeck
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2006-08-22 23:40 UTC (permalink / raw)
  To: Tilman Sauerbeck; +Cc: git

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

> @@ -93,6 +94,21 @@ OPTIONS
>  --pid-file=file::
>  	Save the process id in 'file'.
>  
> +--user=user::
> +--group=group::

Probably

	--user=user, --group=group::

Also check for asciidoc formatting please; it's rather picky.

> +	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 'user'. If `group` is specified,
> +	the gid is set to the one of 'group', otherwise, the default gid
> +	of 'user' is used.

Funny whitespaces all over the place...

What is the pw_gid stored in struct passwd for the user?
getgroups(2) gives supplementary group IDs, so perhaps it is
called primary group ID?

> +	Both `group` and `user` need to be passed as the name of the resp of
> +	the group, ie you'll get unexpected results if you pass an uid/a gid.

Gaah, but that is probably OK.  I'd explicitly say they are
always interpreted as name and never numeric.  An alternative
would be to be nice and when getpwnam() and/or getgrnam()
returns NULL try to interpret them as numeric, which might help
a small dedicated server installation that does not have any
/etc/passwd or /etc/group file ;-) but I do not think that would
be worth the confusion.

> +	Note that you probably don't want to use these options if you run
> +	git-daemon in inetd mode, since inetd can do the privilege dropping
> +	for you.

Gaah again.  These options do not have any effect (other than
sanity checking) on the inetd_mode codepath, so instead of
saying this in the documentation I would suggest specifying
these options an error under --inetd.

Something like this on top of your patch perhaps.

diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
index 8f850fa..17619a3 100644
--- a/Documentation/git-daemon.txt
+++ b/Documentation/git-daemon.txt
@@ -94,20 +94,16 @@ OPTIONS
 --pid-file=file::
 	Save the process id in 'file'.
 
---user=user::
---group=group::
-	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 'user'. If `group` is specified,
-	the gid is set to the one of 'group', otherwise, the default gid
-	of 'user' is used.
-
-	Both `group` and `user` need to be passed as the name of the resp of
-	the group, ie you'll get unexpected results if you pass an uid/a gid.
-
-	Note that you probably don't want to use these options if you run
-	git-daemon in inetd mode, since inetd can do the privilege dropping
-	for you.
+--user=user, --group=group::
+	Change daemon's uid and gid before entering the service loop.
+	When only `--user` is given without `--group`, the
+	primary group ID for the user is used.  The values of
+	the option are given to `getpwnam(3)` and `getgrnam(3)`
+	and numeric IDs are not supported.
++
+Giving these options is an error when used with `--inetd`; use
+the facility of inet daemon to achieve the same before spawning
+`git-daemon` if needed.
 
 <directory>::
 	A directory to add to the whitelist of allowed directories. Unless
diff --git a/daemon.c b/daemon.c
index 4e94210..dd3915a 100644
--- a/daemon.c
+++ b/daemon.c
@@ -821,6 +821,9 @@ int main(int argc, char **argv)
 		usage(daemon_usage);
 	}
 
+	if (inetd_mode && (group_name || user_name))
+		die("--user and --group are incompatible with --inetd");
+
 	if (group_name && !user_name)
 		die("--group supplied without --user");
 

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] Added support for dropping privileges to git-daemon.
  2006-08-22 23:40         ` Junio C Hamano
@ 2006-08-23 16:45           ` Tilman Sauerbeck
  0 siblings, 0 replies; 13+ messages in thread
From: Tilman Sauerbeck @ 2006-08-23 16:45 UTC (permalink / raw)
  To: git

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

Junio C Hamano [2006-08-22 16:40]:
> Tilman Sauerbeck <tilman@code-monkey.de> writes:
> 
> > +	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 'user'. If `group` is specified,
> > +	the gid is set to the one of 'group', otherwise, the default gid
> > +	of 'user' is used.
> 
> Funny whitespaces all over the place...

There's exactly _one_ stray \t in the middle of a line. Not quite what
I'd call "all over the place' ;)
I should probably tell vim to hilight space errors in txt files, too.

> Gaah again.  These options do not have any effect (other than
> sanity checking) on the inetd_mode codepath, so instead of
> saying this in the documentation I would suggest specifying
> these options an error under --inetd.

Good point.

Regards,
Tilman

-- 
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing on usenet and in e-mail?

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2006-08-23 16:45 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-08-19 12:27 [PATCH] Added support for dropping privileges to git-daemon Tilman Sauerbeck
2006-08-19 13:23 ` Marco Costalba
2006-08-19 13:29   ` Tilman Sauerbeck
2006-08-19 15:19     ` Marco Costalba
2006-08-19 15:22       ` Marco Costalba
2006-08-19 17:15       ` Mitchell Blank Jr
2006-08-19 13:32 ` Johannes Schindelin
2006-08-19 17:25 ` Mitchell Blank Jr
2006-08-19 12:27   ` Tilman Sauerbeck
2006-08-22  6:38     ` Junio C Hamano
2006-08-22 17:37       ` Tilman Sauerbeck
2006-08-22 23:40         ` Junio C Hamano
2006-08-23 16:45           ` Tilman Sauerbeck

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 NNTP newsgroup(s).