On Thu, Oct 21, 2010 at 11:16 PM, Erik Faye-Lund <kusmabite@xxxxxxxxx> wrote: > On Mon, Oct 18, 2010 at 6:31 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote: >> Just to throw an idea out: you can also do something like >> >> #ifndef NO_POSIX_GOODIES >> struct credentials { >> }; >> #else >> struct credentials { >> struct passwd *pass; >> gid_t gid; >> } >> #endif >> >> and pass a pointer to credentials around. >> > > Yes, but that structure still needs to be filled somehow. I'm not sure > how this solves anything, really. Isn't it essentially another way of > wrapping an ifdef around the parameters inside main() (at least when > I've inlined serve() into main())? > >> #ifndef HAVE_POSIX_GOODIES >> static int drop_privileges(...) >> { >> return error("--user and --group not supported on this platform"); >> } >> #endif >> static int drop_privileges(...) >> { >> ... >> do >> something >> ... >> } >> #endif >> >> would make serve() look like >> >> static int serve(...) >> { >> int socknum, *socklist; >> >> ... setup socket ... >> >> if (want to drop privileges) { >> if (drop_privileges(...)) >> return -1; >> } >> >> return service_loop(socknum, socklist); >> } >> >> which should be quite readable even to a person only interested in the >> !HAVE_POSIX_GOODIES case imho. With some code rearrangement it could >> be made nicer. Now compare: >> >> static int serve(...) >> { >> int socknum, *socklist; >> >> ... setup socket ... >> >> #ifdef HAVE_POSIX_GOODIES >> ... >> do >> things >> ... >> #endif >> >> return service_loop(socknum, socklist); >> } >> >> Just my two cents. Sorry I do not have something more substantive to >> say. >> > > You're leaving out the troublesome part, namely the glue between "if > (user_name)" in main(), and the "want to drop privileges"-stuff in > serve(). > > I could do a "struct credentials *cred = NULL;" in main(), and assign > that inside "if (user_name)". But that'd leave a warning about > unreachable code in drop_privileges(), no? > OK, I did another stab at this, and this is the best I could come up with right now, what do you think? diff --git a/Makefile b/Makefile index 46034bf..53986b1 100644 --- a/Makefile +++ b/Makefile @@ -401,6 +401,7 @@ EXTRA_PROGRAMS = # ... and all the rest that could be moved out of bindir to gitexecdir PROGRAMS += $(EXTRA_PROGRAMS) +PROGRAM_OBJS += daemon.o PROGRAM_OBJS += fast-import.o PROGRAM_OBJS += imap-send.o PROGRAM_OBJS += shell.o @@ -1066,7 +1067,6 @@ ifeq ($(uname_S),Windows) NO_SVN_TESTS = YesPlease NO_PERL_MAKEMAKER = YesPlease RUNTIME_PREFIX = YesPlease - NO_POSIX_ONLY_PROGRAMS = YesPlease NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease NO_NSEC = YesPlease USE_WIN32_MMAP = YesPlease @@ -1077,6 +1077,7 @@ ifeq ($(uname_S),Windows) NO_CURL = YesPlease NO_PYTHON = YesPlease BLK_SHA1 = YesPlease + NO_POSIX_GOODIES = UnfortunatelyYes NATIVE_CRLF = YesPlease CC = compat/vcbuild/scripts/clink.pl @@ -1119,7 +1120,6 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_SVN_TESTS = YesPlease NO_PERL_MAKEMAKER = YesPlease RUNTIME_PREFIX = YesPlease - NO_POSIX_ONLY_PROGRAMS = YesPlease NO_ST_BLOCKS_IN_STRUCT_STAT = YesPlease NO_NSEC = YesPlease USE_WIN32_MMAP = YesPlease @@ -1130,6 +1130,9 @@ ifneq (,$(findstring MINGW,$(uname_S))) NO_PYTHON = YesPlease BLK_SHA1 = YesPlease ETAGS_TARGET = ETAGS + NO_INET_PTON = YesPlease + NO_INET_NTOP = YesPlease + NO_POSIX_GOODIES = UnfortunatelyYes COMPAT_CFLAGS += -D__USE_MINGW_ACCESS -DNOGDI -Icompat -Icompat/fnmatch -Icompat/win32 COMPAT_CFLAGS += -DSTRIP_EXTENSION=\".exe\" COMPAT_OBJS += compat/mingw.o compat/fnmatch/fnmatch.o compat/winansi.o \ @@ -1249,9 +1252,6 @@ ifdef ZLIB_PATH endif EXTLIBS += -lz -ifndef NO_POSIX_ONLY_PROGRAMS - PROGRAM_OBJS += daemon.o -endif ifndef NO_OPENSSL OPENSSL_LIBSSL = -lssl ifdef OPENSSLDIR @@ -1419,6 +1419,10 @@ ifdef NO_DEFLATE_BOUND BASIC_CFLAGS += -DNO_DEFLATE_BOUND endif +ifdef NO_POSIX_GOODIES + BASIC_CFLAGS += -DNO_POSIX_GOODIES +endif + ifdef BLK_SHA1 SHA1_HEADER = "block-sha1/sha1.h" LIB_OBJS += block-sha1/sha1.o diff --git a/daemon.c b/daemon.c index b7f3874..20ae9b4 100644 --- a/daemon.c +++ b/daemon.c @@ -26,7 +26,9 @@ static const char daemon_usage[] = " [--reuseaddr] [--pid-file=file]\n" " [--[enable|disable|allow-override|forbid-override]=service]\n" " [--inetd | [--listen=host_or_ipaddr] [--port=n]\n" +#ifndef NO_POSIX_GOODIES " [--detach] [--user=user [--group=group]]\n" +#endif " [directory...]"; /* List of acceptable pathname prefixes */ @@ -938,6 +940,33 @@ static void sanitize_stdfds(void) close(fd); } +#ifdef NO_POSIX_GOODIES + +struct credentials; +static void drop_privileges(struct credentials *cred) +{ + /* nothing */ +} + +static void daemonize(void) +{ + die("--detach not supported on this platform"); +} + +#else + +struct credentials { + struct passwd *pass; + gid_t gid; +}; + +static void drop_privileges(struct credentials *cred) +{ + if (cred && initgroups(cred->pass->pw_name, cred->gid) || + setgid (cred->gid) || setuid(cred->pass->pw_uid)) + die("cannot drop privileges"); +} + static void daemonize(void) { switch (fork()) { @@ -955,6 +984,7 @@ static void daemonize(void) close(2); sanitize_stdfds(); } +#endif static void store_pid(const char *path) { @@ -965,7 +995,7 @@ static void store_pid(const char *path) die_errno("failed to write pid file '%s'", path); } -static int serve(struct string_list *listen_addr, int listen_port, struct passwd *pass, gid_t gid) +static int serve(struct string_list *listen_addr, int listen_port, struct credentials *cred) { struct socketlist socklist = { NULL, 0, 0 }; @@ -974,10 +1004,7 @@ static int serve(struct string_list *listen_addr, int listen_port, struct passwd die("unable to allocate any listen sockets on port %u", listen_port); - if (pass && gid && - (initgroups(pass->pw_name, gid) || setgid (gid) || - setuid(pass->pw_uid))) - die("cannot drop privileges"); + drop_privileges(cred); return service_loop(&socklist); } @@ -989,9 +1016,7 @@ int main(int argc, char **argv) int serve_mode = 0, inetd_mode = 0; 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; + struct credentials *cred = NULL; int i; git_extract_argv0_path(argv[0]); @@ -1079,6 +1104,7 @@ int main(int argc, char **argv) pid_file = arg + 11; continue; } +#ifndef NO_POSIX_GOODIES if (!strcmp(arg, "--detach")) { detach = 1; log_syslog = 1; @@ -1092,6 +1118,12 @@ int main(int argc, char **argv) group_name = arg + 8; continue; } +#else + /* avoid warnings */ + (void)user_name; + (void)group_name; + (void)detach; +#endif if (!prefixcmp(arg, "--enable=")) { enable_service(arg + 9, 1); continue; @@ -1126,32 +1158,37 @@ int main(int argc, char **argv) /* avoid splitting a message in the middle */ setvbuf(stderr, NULL, _IOFBF, 4096); - if (inetd_mode && (detach || group_name || user_name)) - die("--detach, --user and --group are incompatible with --inetd"); - if (inetd_mode && (listen_port || (listen_addr.nr > 0))) die("--listen= and --port= are incompatible with --inetd"); else if (listen_port == 0) listen_port = DEFAULT_GIT_PORT; +#ifndef NO_POSIX_GOODIES + if (inetd_mode && (detach || group_name || user_name)) + die("--detach, --user and --group are incompatible with --inetd"); + if (group_name && !user_name) die("--group supplied without --user"); if (user_name) { - pass = getpwnam(user_name); - if (!pass) + struct credentials c; + cred = &c; + + c->pass = getpwnam(user_name); + if (!c->pass) die("user not found - %s", user_name); if (!group_name) - gid = pass->pw_gid; + c->gid = pass->pw_gid; else { - group = getgrnam(group_name); + struct group *group = getgrnam(group_name); if (!group) die("group not found - %s", group_name); - gid = group->gr_gid; + c->gid = group->gr_gid; } } +#endif if (strict_paths && (!ok_paths || !*ok_paths)) die("option --strict-paths requires a whitelist"); @@ -1185,5 +1222,5 @@ int main(int argc, char **argv) cld_argv[argc] = "--serve"; cld_argv[argc+1] = NULL; - return serve(&listen_addr, listen_port, pass, gid); + return serve(&listen_addr, listen_port, cred); } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html