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