* [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* 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 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 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 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
* [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 ` 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).