[PATCH] Make http-backend REMOTE_USER configurable

[PATCH] Make http-backend REMOTE_USER configurable

[William Strecker-Kellogg]

The http-backend looks at $REMOTE_USER and sets $GIT_COMMITTER_NAME to
that for use in the hooks. At our site we have a third party
authentication module for our proxy (Shibboleth) which sets an alternative
environment variable that our backend sees instead of REMOTE USER.

This patch adds the config option http.remoteuser which changes what
environment variable is inspected by the http-backend code (it defaults
to REMOTE_USER).

Reported-by: Jason Smith <smithj4@bnl.gov>
Tested-by: William Strecker-Kellogg <willsk@bnl.gov>
Signed-off-by: William Strecker-Kellogg <willsk@bnl.gov>
---

I do not know if anyone else may find this useful, but FWIW, this works
for us. It may even be a good idea to add the ability to overwrite other
default CGI environment variables, although I can't think of any other
ones at the moment.


 Documentation/git-http-backend.txt |   14 ++++++++++----
 http-backend.c                     |   13 +++++++++++--
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-http-backend.txt b/Documentation/git-http-backend.txt
index f4e0741..b320a45 100644
--- a/Documentation/git-http-backend.txt
+++ b/Documentation/git-http-backend.txt
@@ -42,6 +42,10 @@ http.getanyfile::
 	It is enabled by default, but a repository can disable it
 	by setting this configuration item to `false`.
 
+http.remoteuser::
+	This setting, if present, changes what environment variable is inspected
+	to determine the remote user (the default is "REMOTE_USER").
+
 http.uploadpack::
 	This serves 'git fetch-pack' and 'git ls-remote' clients.
 	It is enabled by default, but a repository can disable it
@@ -175,10 +179,12 @@ The GIT_HTTP_EXPORT_ALL environmental variable may be passed to
 'git-http-backend' to bypass the check for the "git-daemon-export-ok"
 file in each repository before allowing export of that repository.
 
-The backend process sets GIT_COMMITTER_NAME to '$REMOTE_USER' and
-GIT_COMMITTER_EMAIL to '$\{REMOTE_USER}@http.$\{REMOTE_ADDR\}',
-ensuring that any reflogs created by 'git-receive-pack' contain some
-identifying information of the remote user who performed the push.
+The backend process sets GIT_COMMITTER_NAME to '$REMOTE_USER' (or
+if http.remoteuser is present it sets it to $\{http.remoteuser\})
+and GIT_COMMITTER_EMAIL to '$\{REMOTE_USER}@http.$\{REMOTE_ADDR\}',
+ensuring that any reflogs created by 'git-receive-pack' contain
+some identifying information of the remote user who performed the
+push.
 
 All CGI environment variables are available to each of the hooks
 invoked by the 'git-receive-pack'.
diff --git a/http-backend.c b/http-backend.c
index 869d515..69756f0 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -11,6 +11,7 @@
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
 static const char last_modified[] = "Last-Modified";
+static char *remoteuser = "REMOTE_USER";
 static int getanyfile = 1;
 
 static struct string_list *query_params;
@@ -225,6 +226,14 @@ static int http_config(const char *var, const char *value, void *cb)
 		return 0;
 	}
 
+	if(!strcmp(var, "http.remoteuser")) {
+		char *tmp;
+		if(git_config_string(&tmp, var, value) == 0) {
+			remoteuser = tmp;
+		}
+		return 0;
+	}
+
 	if (!prefixcmp(var, "http.")) {
 		int i;
 
@@ -261,7 +270,7 @@ static struct rpc_service *select_service(const char *name)
 		forbidden("Unsupported service: '%s'", name);
 
 	if (svc->enabled < 0) {
-		const char *user = getenv("REMOTE_USER");
+		const char *user = getenv(remoteuser);
 		svc->enabled = (user && *user) ? 1 : 0;
 	}
 	if (!svc->enabled)
@@ -315,7 +324,7 @@ done:
 static void run_service(const char **argv)
 {
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
-	const char *user = getenv("REMOTE_USER");
+	const char *user = getenv(remoteuser);
 	const char *host = getenv("REMOTE_ADDR");
 	char *env[3];
 	struct strbuf buf = STRBUF_INIT;
-- 
1.7.7.6

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Junio C Hamano]

William Strecker-Kellogg <willsk@bnl.gov> writes:

> The http-backend looks at $REMOTE_USER and sets $GIT_COMMITTER_NAME to
> that for use in the hooks. At our site we have a third party
> authentication module for our proxy (Shibboleth) which sets an alternative
> environment variable that our backend sees instead of REMOTE USER.
>
> This patch adds the config option http.remoteuser which changes what
> environment variable is inspected by the http-backend code (it defaults
> to REMOTE_USER).

What is the chain of systems that pass the authenticated ident down to
this CGI program?  Can another part of that chain stuff the value of
SHIBBOLETH_USER (or whatever) to REMOTE_USER before running it?

As a design, I am not convinced this is a good change.  

What if the next person wants to interoperate with an authentication
system that passes the same information via a mechanism different from
environment variables?  This change does not help him at all, as it is
still married to "the information has to come from an environment
variable" limitation.

What if an authentication system can supply more appropriate committer
ident information other than just the uesrname part?

If it were a patch to teach the CGI program a new command line parameter,
e.g. --log-as="C O Mitter <committer@example.xz>", it may have made a lot
more sense, though.

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Jeff King]

On Thu, Mar 29, 2012 at 03:02:52PM -0700, Junio C Hamano wrote:

> William Strecker-Kellogg <willsk@bnl.gov> writes:
> 
> > The http-backend looks at $REMOTE_USER and sets $GIT_COMMITTER_NAME to
> > that for use in the hooks. At our site we have a third party
> > authentication module for our proxy (Shibboleth) which sets an alternative
> > environment variable that our backend sees instead of REMOTE USER.
> >
> > This patch adds the config option http.remoteuser which changes what
> > environment variable is inspected by the http-backend code (it defaults
> > to REMOTE_USER).
> 
> What is the chain of systems that pass the authenticated ident down to
> this CGI program?  Can another part of that chain stuff the value of
> SHIBBOLETH_USER (or whatever) to REMOTE_USER before running it?
> 
> As a design, I am not convinced this is a good change.
> 
> What if the next person wants to interoperate with an authentication
> system that passes the same information via a mechanism different from
> environment variables?  This change does not help him at all, as it is
> still married to "the information has to come from an environment
> variable" limitation.
> 
> What if an authentication system can supply more appropriate committer
> ident information other than just the uesrname part?

I agree. It seems like one could just wrap http-backend in a script like
this:

  #!/bin/sh
  REMOTE_USER=$SHIBBOLETH_USER
  exec git http-backend "$@"

and that leaves way more flexibility. I think an even better thing would
be for http-backend to leave GIT_COMMITTER_* alone if it exists; that is
the usual well-known interface for setting such things. And then you
could specify a detailed committer name and email if you want, or leave
them blank to pull from $REMOTE_USER as we do now. As it is now, even if
you specify GIT_COMMITTER_EMAIL, it gets overwritten with
"$REMOTE_USER@http.$REMOTE_ADDR".

Just today, we were looking at a similar patch for GitHub (we keep
reflogs on all pushes, and we want to put useful information about the
pusher into the reflog). William's patch would not be flexible enough
for what we want to do, but setting GIT_COMMITTER_* would be easy (we
are also stuffing more information into the reflog message, but that is
a separate issue).

-Peff

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Jeff King]

On Thu, Mar 29, 2012 at 06:22:30PM -0400, Jeff King wrote:

> I think an even better thing would be for http-backend to leave
> GIT_COMMITTER_* alone if it exists; that is the usual well-known
> interface for setting such things. And then you could specify a
> detailed committer name and email if you want, or leave them blank to
> pull from $REMOTE_USER as we do now. As it is now, even if you specify
> GIT_COMMITTER_EMAIL, it gets overwritten with
> "$REMOTE_USER@http.$REMOTE_ADDR".

That patch would look something like this:

---
diff --git a/http-backend.c b/http-backend.c
index 869d515..aa892e6 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -318,7 +318,7 @@ static void run_service(const char **argv)
 	const char *user = getenv("REMOTE_USER");
 	const char *host = getenv("REMOTE_ADDR");
 	char *env[3];
-	struct strbuf buf = STRBUF_INIT;
+	int num_env = 0;
 	int gzipped_request = 0;
 	struct child_process cld;
 
@@ -332,13 +332,17 @@ static void run_service(const char **argv)
 	if (!host || !*host)
 		host = "(none)";
 
-	memset(&env, 0, sizeof(env));
-	strbuf_addf(&buf, "GIT_COMMITTER_NAME=%s", user);
-	env[0] = strbuf_detach(&buf, NULL);
-
-	strbuf_addf(&buf, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
-	env[1] = strbuf_detach(&buf, NULL);
-	env[2] = NULL;
+	if (!getenv("GIT_COMMITTER_NAME")) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "GIT_COMMITTER_NAME=%s", user);
+		env[num_env++] = strbuf_detach(&buf, NULL);
+	}
+	if (!getenv("GIT_COMMITTER_EMAIL")) {
+		struct strbuf buf = STRBUF_INIT;
+		strbuf_addf(&buf, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
+		env[num_env++] = strbuf_detach(&buf, NULL);
+	}
+	env[num_env] = NULL;
 
 	memset(&cld, 0, sizeof(cld));
 	cld.argv = argv;
@@ -359,7 +363,6 @@ static void run_service(const char **argv)
 		exit(1);
 	free(env[0]);
 	free(env[1]);
-	strbuf_release(&buf);
 }
 
 static int show_text_ref(const char *name, const unsigned char *sha1,

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Mar 29, 2012 at 06:22:30PM -0400, Jeff King wrote:
>
>> I think an even better thing would be for http-backend to leave
>> GIT_COMMITTER_* alone if it exists; that is the usual well-known
>> interface for setting such things. And then you could specify a
>> detailed committer name and email if you want, or leave them blank to
>> pull from $REMOTE_USER as we do now. As it is now, even if you specify
>> GIT_COMMITTER_EMAIL, it gets overwritten with
>> "$REMOTE_USER@http.$REMOTE_ADDR".
>
> That patch would look something like this:

It would regress for somebody who is running the CGI program while
exporting these environment variables pointing at himself and relying on
the fact that these are canceled by REMOTE_USER/ADDR (perhaps a web-based
editor can write into some repository and commits made by that editor
takes the ident information from COMMITTER variables, while another part
of the webserver takes a push by spawning the http backend???).

Which is very unlikely.

If somebody else comes up with a valid scenario to show why this patch is
a bad idea, I'd stand corrected but at the same time I'd be very surprised.

But I think this is the right thing to do, even though it is not related
to the issue William wanted to address with his patch.

Care to sign it off?

Re: [PATCH] Make http-backend REMOTE_USER configurable

[William Strecker-Kellogg]

On 03/29/2012 09:52 PM, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> On Thu, Mar 29, 2012 at 06:22:30PM -0400, Jeff King wrote:
>>
>>> I think an even better thing would be for http-backend to leave
>>> GIT_COMMITTER_* alone if it exists; that is the usual well-known
>>> interface for setting such things. And then you could specify a
>>> detailed committer name and email if you want, or leave them blank to
>>> pull from $REMOTE_USER as we do now. As it is now, even if you specify
>>> GIT_COMMITTER_EMAIL, it gets overwritten with
>>> "$REMOTE_USER@http.$REMOTE_ADDR".
>>
>> That patch would look something like this:
> 
> It would regress for somebody who is running the CGI program while
> exporting these environment variables pointing at himself and relying on
> the fact that these are canceled by REMOTE_USER/ADDR (perhaps a web-based
> editor can write into some repository and commits made by that editor
> takes the ident information from COMMITTER variables, while another part
> of the webserver takes a push by spawning the http backend???).
> 
> Which is very unlikely.

Agreed.

> 
> If somebody else comes up with a valid scenario to show why this patch is
> a bad idea, I'd stand corrected but at the same time I'd be very surprised.
> 
> But I think this is the right thing to do, even though it is not related
> to the issue William wanted to address with his patch.
> 

The reason we're interested in this is validation -- who can push to our
production puppet branch is determined by a post-update hook.  A wrapper
around http-backend like Jeff's combined with his patch would accomplish
what we need just fine.

> Care to sign it off?
> 

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Jeff King]

On Thu, Mar 29, 2012 at 06:52:35PM -0700, Junio C Hamano wrote:

> It would regress for somebody who is running the CGI program while
> exporting these environment variables pointing at himself and relying on
> the fact that these are canceled by REMOTE_USER/ADDR (perhaps a web-based
> editor can write into some repository and commits made by that editor
> takes the ident information from COMMITTER variables, while another part
> of the webserver takes a push by spawning the http backend???).
> 
> Which is very unlikely.

Yeah, I agree that is not worth worrying too much about.

> But I think this is the right thing to do, even though it is not related
> to the issue William wanted to address with his patch.
> 
> Care to sign it off?

Updated patch is below. In addition to a commit message and signoff, I
tweaked two things:

  1. The original I posted failed to update the free() calls at the end
     of the function (and since we now have a variable number of env
     variables, we can't just free all elements). I ended up just
     converting this to use argv_array, which handles the memory
     management for us and is way shorter and easier to read.

  2. I added tests, both for the default behavior and for the new one.
     The "127.0.0.1" is hard-coded in the expected output (but comes
     from apache's setting of REMOTE_ADDR). I think this is OK. The
     lib-httpd setup code always listens on 127.0.0.1, so short of some
     insane loopback routing setup, the REMOTE_ADDR should also be
     127.0.0.1.

-- >8 --
Subject: http-backend: respect existing GIT_COMMITTER_* variables

The http-backend program sets default GIT_COMMITTER_NAME and
GIT_COMMITTER_EMAIL variables based on the REMOTE_USER and
REMOTE_ADDR variables provided by the webserver. However, it
unconditionally overwrites any existing GIT_COMMITTER
variables, which may have been customized by site-specific
code in the webserver (or in a script wrapping http-backend).

Let's leave those variables intact if they already exist,
assuming that any such configuration was intentional. There
is a slight chance of a regression if somebody has set
GIT_COMMITTER_* for the entire webserver, not intending it
to leak through http-backend. We could protect against this
by passing the information in alternate variables.  However,
it seems unlikely that anyone will care about that
regression, and there is value in the simplicity of using
the common variable names that are used elsewhere in git.

While we're tweaking the environment-handling in
http-backend, let's switch it to use argv_array to handle
the list of variables. That makes the memory management much
simpler.

Signed-off-by: Jeff King <peff@peff.net>
---
 http-backend.c          |   22 +++++++++-------------
 t/lib-httpd/apache.conf |    7 +++++++
 t/t5541-http-push.sh    |   21 +++++++++++++++++++++
 3 files changed, 37 insertions(+), 13 deletions(-)

diff --git a/http-backend.c b/http-backend.c
index 869d515..f50e77f 100644
--- a/http-backend.c
+++ b/http-backend.c
@@ -7,6 +7,7 @@
 #include "run-command.h"
 #include "string-list.h"
 #include "url.h"
+#include "argv-array.h"
 
 static const char content_type[] = "Content-Type";
 static const char content_length[] = "Content-Length";
@@ -317,8 +318,7 @@ static void run_service(const char **argv)
 	const char *encoding = getenv("HTTP_CONTENT_ENCODING");
 	const char *user = getenv("REMOTE_USER");
 	const char *host = getenv("REMOTE_ADDR");
-	char *env[3];
-	struct strbuf buf = STRBUF_INIT;
+	struct argv_array env = ARGV_ARRAY_INIT;
 	int gzipped_request = 0;
 	struct child_process cld;
 
@@ -332,17 +332,15 @@ static void run_service(const char **argv)
 	if (!host || !*host)
 		host = "(none)";
 
-	memset(&env, 0, sizeof(env));
-	strbuf_addf(&buf, "GIT_COMMITTER_NAME=%s", user);
-	env[0] = strbuf_detach(&buf, NULL);
-
-	strbuf_addf(&buf, "GIT_COMMITTER_EMAIL=%s@http.%s", user, host);
-	env[1] = strbuf_detach(&buf, NULL);
-	env[2] = NULL;
+	if (!getenv("GIT_COMMITTER_NAME"))
+		argv_array_pushf(&env, "GIT_COMMITTER_NAME=%s", user);
+	if (!getenv("GIT_COMMITTER_EMAIL"))
+		argv_array_pushf(&env, "GIT_COMMITTER_EMAIL=%s@http.%s",
+				 user, host);
 
 	memset(&cld, 0, sizeof(cld));
 	cld.argv = argv;
-	cld.env = (const char *const *)env;
+	cld.env = env.argv;
 	if (gzipped_request)
 		cld.in = -1;
 	cld.git_cmd = 1;
@@ -357,9 +355,7 @@ static void run_service(const char **argv)
 
 	if (finish_command(&cld))
 		exit(1);
-	free(env[0]);
-	free(env[1]);
-	strbuf_release(&buf);
+	argv_array_clear(&env);
 }
 
 static int show_text_ref(const char *name, const unsigned char *sha1,
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 3c12b05..de3762e 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -52,8 +52,15 @@ Alias /auth/ www/auth/
 <Location /smart_noexport/>
 	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
 </Location>
+<Location /smart_custom_env/>
+	SetEnv GIT_EXEC_PATH ${GIT_EXEC_PATH}
+	SetEnv GIT_HTTP_EXPORT_ALL
+	SetEnv GIT_COMMITTER_NAME "Custom User"
+	SetEnv GIT_COMMITTER_EMAIL custom@example.com
+</Location>
 ScriptAlias /smart/ ${GIT_EXEC_PATH}/git-http-backend/
 ScriptAlias /smart_noexport/ ${GIT_EXEC_PATH}/git-http-backend/
+ScriptAlias /smart_custom_env/ ${GIT_EXEC_PATH}/git-http-backend/
 <Directory ${GIT_EXEC_PATH}>
 	Options None
 </Directory>
diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
index cc6f081..d7964c7 100755
--- a/t/t5541-http-push.sh
+++ b/t/t5541-http-push.sh
@@ -30,6 +30,7 @@ test_expect_success 'setup remote repository' '
 	git clone --bare test_repo test_repo.git &&
 	cd test_repo.git &&
 	git config http.receivepack true &&
+	git config core.logallrefupdates true &&
 	ORIG_HEAD=$(git rev-parse --verify HEAD) &&
 	cd - &&
 	mv test_repo.git "$HTTPD_DOCUMENT_ROOT_PATH"
@@ -222,5 +223,25 @@ test_expect_success TTY 'quiet push' '
 	test_cmp /dev/null output
 '
 
+test_expect_success 'http push gives sane defaults to reflog' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit reflog-test &&
+	git push "$HTTPD_URL"/smart/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -g -1 --format="%gn <%ge>" >actual &&
+	echo "anonymous <anonymous@http.127.0.0.1>" >expect &&
+	test_cmp expect actual
+'
+
+test_expect_success 'http push respects GIT_COMMITTER_* in reflog' '
+	cd "$ROOT_PATH"/test_repo_clone &&
+	test_commit custom-reflog-test &&
+	git push "$HTTPD_URL"/smart_custom_env/test_repo.git &&
+	git --git-dir="$HTTPD_DOCUMENT_ROOT_PATH/test_repo.git" \
+		log -g -1 --format="%gn <%ge>" >actual &&
+	echo "Custom User <custom@example.com>" >expect &&
+	test_cmp expect actual
+'
+
 stop_httpd
 test_done
-- 
1.7.9.5.7.g11b89

Re: [PATCH] Make http-backend REMOTE_USER configurable

[Junio C Hamano]

Thanks.