threads / patch / 30101

patchMake http-backend REMOTE_USER configurable

Subject: [PATCH] Make http-backend REMOTE_USER configurable

## tl;dr

8 messages between Mar 29, 2012 and Mar 30, 2012. Diffs are folded; open one to read it.

replies: 7people: 3as markdown or json

William Strecker-Kellogg· Mar 29, 2012, 19:58 UTC · lore

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(-)
Show changes to 2 files +21 −6

Documentation/git-http-backend.txt, http-backend.c

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
Junio C Hamano· Mar 29, 2012, 22:02 UTC · re: William Strecker-Kellogg · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

William Strecker-Kellogg <willsk@bnl.gov> writes:
Show 8 quoted lines
> 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.

Jeff King· Mar 29, 2012, 22:22 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

On Thu, Mar 29, 2012 at 03:02:52PM -0700, Junio C Hamano wrote:
Show 25 quoted lines
> 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
Jeff King· Mar 29, 2012, 22:26 UTC · re: Jeff King · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

On Thu, Mar 29, 2012 at 06:22:30PM -0400, Jeff King wrote:
Show 7 quoted lines
> 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:
---
Show changes to http-backend.c +12 −9
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,
Junio C Hamano· Mar 30, 2012, 01:52 UTC · re: Jeff King · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

Jeff King <peff@peff.net> writes:
Show 11 quoted lines
> 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?
William Strecker-Kellogg· Mar 30, 2012, 03:24 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

On 03/29/2012 09:52 PM, Junio C Hamano wrote:
Show 22 quoted lines
> 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.
Show 7 quoted lines
> 
> 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?
> 
Jeff King· Mar 30, 2012, 07:01 UTC · re: Junio C Hamano · lore

Re: [PATCH] Make http-backend REMOTE_USER configurable

On Thu, Mar 29, 2012 at 06:52:35PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> 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(-)
Show changes to 3 files +37 −13

http-backend.c, t/lib-httpd/apache.conf, t/t5541-http-push.sh

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

← back to recent threads