[PATCH 0/3] Implement authorization via external program

[PATCH 0/3] Implement authorization via external program

[valentin.haenel]

We needed to support querying gitolite from cgit to authorize authenticated
users and display only those repositories they have access to. In our case the
authentication is handled by apache and the user name is stored in an
environment variable. The first patch implements reading that variable. The
second patch allows querying an external helper. And the third adds an example
script to contrib/ showing how gitolite might be queried.

I would appreciate a ruthless review, since this is my first time doing any cgi
work and I am not too familiar with C.

Carlos Aguado Sanchez (1):
  Helper script to interface to gitolite

Valentin Haenel (2):
  Add config option user-envvar
  Add ability to authorize viewing a repository

 cgit.c                |   34 ++++++++++++++++++++++++++++++++++
 cgit.h                |    3 +++
 cgitrc.5.txt          |   12 ++++++++++++
 contrib/gl-check-user |   18 ++++++++++++++++++
 4 files changed, 67 insertions(+)
 create mode 100755 contrib/gl-check-user

-- 
1.7.9.5

[PATCH 1/3] Add config option user-envvar

[valentin.haenel]

When cgit sits on a backend server and relies on a set of
front-ends to do authentication, it will read the username
from an environment variable defined by this option.

In this way, one can safely use any forwarded HTTP header
and not only the expected REMOTE_USER variable set by the
CGI standard.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   10 ++++++++++
 cgit.h       |    2 ++
 cgitrc.5.txt |    6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/cgit.c b/cgit.c
index 1ec02e74ac..101954e12a 100644
--- a/cgit.c
+++ b/cgit.c
@@ -121,6 +121,8 @@ void config_cb(const char *name, const char *value)
 		repo_config(ctx.repo, name + 5, value);
 	else if (!strcmp(name, "readme"))
 		ctx.cfg.readme = xstrdup(value);
+	else if (!strcmp(name, "user-envvar"))
+		ctx.cfg.user_envvar = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -370,6 +372,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.user_envvar = "REMOTE_USER";
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
 	ctx->env.https = xstrdupn(getenv("HTTPS"));
@@ -814,6 +817,13 @@ int main(int argc, const char **argv)
 	ctx.repo = NULL;
 	http_parse_querystring(ctx.qry.raw, querystring_cb);
 
+	/*
+	 * Get the username of an authenticated user. It will get
+	 * from the environment variable defined by the user-header
+	 * option (defaults to REMOTE_USER)
+	 */
+	ctx.env.remote_user = xstrdupn(getenv(ctx.cfg.user_envvar));
+
 	/* If virtual-root isn't specified in cgitrc, lets pretend
 	 * that virtual-root equals SCRIPT_NAME, minus any possibly
 	 * trailing slashes.
diff --git a/cgit.h b/cgit.h
index 79ba7adffe..369dd8af8b 100644
--- a/cgit.h
+++ b/cgit.h
@@ -165,6 +165,7 @@ struct cgit_query {
 
 struct cgit_config {
 	char *agefile;
+	char *user_envvar;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
@@ -262,6 +263,7 @@ struct cgit_environment {
 	char *script_name;
 	char *server_name;
 	char *server_port;
+	char *remote_user;
 };
 
 struct cgit_context {
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 902fff3e66..cd7327a4b3 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -386,6 +386,12 @@ strict-export::
 	repositories to match those exported by git-daemon. This option MUST come
 	before 'scan-path'.
 
+user-envvar::
+	Environment variable to read the user name from in a CGI environment. By
+	default, CGI exports it with the REMOTE_USER variable. This parameter can
+	be adjusted to a custom variable (e.g. any HTTP header forwarded by an
+	external authentication engine like HTTP_X_FORWARDED_USER)
+
 virtual-root::
 	Url which, if specified, will be used as root for all cgit links. It
 	will also cause cgit to generate 'virtual urls', i.e. urls like
-- 
1.7.9.5

[PATCH 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

This provides a mechanism for cgit to query an external program for repository
authorisation, e.g. gitolite. An additional config variable 'authorization'
contains the path and name of the executable to use.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   24 ++++++++++++++++++++++++
 cgit.h       |    1 +
 cgitrc.5.txt |    6 ++++++
 3 files changed, 31 insertions(+)

diff --git a/cgit.c b/cgit.c
index 101954e12a..f06528215d 100644
--- a/cgit.c
+++ b/cgit.c
@@ -123,6 +123,8 @@ void config_cb(const char *name, const char *value)
 		ctx.cfg.readme = xstrdup(value);
 	else if (!strcmp(name, "user-envvar"))
 		ctx.cfg.user_envvar = xstrdup(value);
+	else if (!strcmp(name, "authorization"))
+		ctx.cfg.authorization = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -372,6 +374,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.authorization = "/bin/true";
 	ctx->cfg.user_envvar = "REMOTE_USER";
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
@@ -545,6 +548,27 @@ static void process_request(void *cbdata)
 		return;
 	}
 
+	/* Here we do the authorization check.
+	 *
+	 * TODO
+	 * ----
+	 *  * figure out if this is the correct place to do the check
+	 *  * check that the authorization script exists and is executable
+	 *
+	 */
+	if (ctx->repo && system(fmt("%s %s %s",
+					ctx->cfg.authorization,
+					ctx->repo->name,
+					ctx->env.remote_user)) != 0) {
+		cgit_print_http_headers(ctx);
+		cgit_print_docstart(ctx);
+		cgit_print_pageheader(ctx);
+		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
+					ctx->repo->name, ctx->env.remote_user));
+		cgit_print_docend();
+		exit(1);
+	}
+
 	if (ctx->repo && prepare_repo_cmd(ctx))
 		return;
 
diff --git a/cgit.h b/cgit.h
index 369dd8af8b..43f4f562f3 100644
--- a/cgit.h
+++ b/cgit.h
@@ -166,6 +166,7 @@ struct cgit_query {
 struct cgit_config {
 	char *agefile;
 	char *user_envvar;
+	char *authorization;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index cd7327a4b3..d864289502 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -40,6 +40,12 @@ agefile::
 	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
 	hh:mm:ss". Default value: "info/web/last-modified".
 
+authorization::
+	Specifies a path to authorization executable. The executable must take two
+	arguments: "<repository> <user> [<permissions>]". The permission is the
+	type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
+	common use case is to call gitolite through this machinery.
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "MACRO EXPANSION".
-- 
1.7.9.5

[PATCH 2/3] Add ability to authorize viewing a repository using

[valentin.haenel]

This provides a mechanism for cgit to query an external program for repository
authorisation. An additional config variable 'authorization' is
---
 cgit.c       |   24 ++++++++++++++++++++++++
 cgit.h       |    1 +
 cgitrc.5.txt |    6 ++++++
 3 files changed, 31 insertions(+)

diff --git a/cgit.c b/cgit.c
index 101954e12a..f06528215d 100644
--- a/cgit.c
+++ b/cgit.c
@@ -123,6 +123,8 @@ void config_cb(const char *name, const char *value)
 		ctx.cfg.readme = xstrdup(value);
 	else if (!strcmp(name, "user-envvar"))
 		ctx.cfg.user_envvar = xstrdup(value);
+	else if (!strcmp(name, "authorization"))
+		ctx.cfg.authorization = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -372,6 +374,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.authorization = "/bin/true";
 	ctx->cfg.user_envvar = "REMOTE_USER";
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
@@ -545,6 +548,27 @@ static void process_request(void *cbdata)
 		return;
 	}
 
+	/* Here we do the authorization check.
+	 *
+	 * TODO
+	 * ----
+	 *  * figure out if this is the correct place to do the check
+	 *  * check that the authorization script exists and is executable
+	 *
+	 */
+	if (ctx->repo && system(fmt("%s %s %s",
+					ctx->cfg.authorization,
+					ctx->repo->name,
+					ctx->env.remote_user)) != 0) {
+		cgit_print_http_headers(ctx);
+		cgit_print_docstart(ctx);
+		cgit_print_pageheader(ctx);
+		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
+					ctx->repo->name, ctx->env.remote_user));
+		cgit_print_docend();
+		exit(1);
+	}
+
 	if (ctx->repo && prepare_repo_cmd(ctx))
 		return;
 
diff --git a/cgit.h b/cgit.h
index 369dd8af8b..43f4f562f3 100644
--- a/cgit.h
+++ b/cgit.h
@@ -166,6 +166,7 @@ struct cgit_query {
 struct cgit_config {
 	char *agefile;
 	char *user_envvar;
+	char *authorization;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index cd7327a4b3..d864289502 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -40,6 +40,12 @@ agefile::
 	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
 	hh:mm:ss". Default value: "info/web/last-modified".
 
+authorization::
+	Specifies a path to authorization executable. The executable must take two
+	arguments: "<repository> <user> [<permissions>]". The permission is the
+	type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
+	common use case is to call gitolite through this machinery.
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "MACRO EXPANSION".
-- 
1.7.9.5

[PATCH 3/3] Helper script to interface to gitolite

[valentin.haenel]

From: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>


Signed-off-by: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 contrib/gl-check-user |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100755 contrib/gl-check-user

diff --git a/contrib/gl-check-user b/contrib/gl-check-user
new file mode 100755
index 0000000000..2f9a331e00
--- /dev/null
+++ b/contrib/gl-check-user
@@ -0,0 +1,18 @@
+#!/bin/sh
+# Wrapper around gitolite to perform
+# repository authentication from a
+# CGI environment
+prog="/usr/local/bin/gitolite"
+
+# Repository to check access against
+# Strip the trailing .git if one is
+# present
+export REPO=${1%%.git}
+export REMOTE_USER=${2}
+export PERM=${3-"R"}
+# HTTPD will not set some essential
+# variables expexted by gitolite
+# Set them here (EUID expected final)
+export HOME=`getent passwd $(id -n -u) | cut -d":" -f 6`
+
+exec $prog access -q ${REPO} ${REMOTE_USER} ${PERM}
-- 
1.7.9.5

[PATCH 2/3] Add ability to authorize viewing a repository using

[valentin.haenel]

You can ignore this one, sorry.

* Valentin Haenel <valentin.haenel at gmx.de> [2012-10-16]:
> This provides a mechanism for cgit to query an external program for repository
> authorisation. An additional config variable 'authorization' is
> ---
>  cgit.c       |   24 ++++++++++++++++++++++++
>  cgit.h       |    1 +
>  cgitrc.5.txt |    6 ++++++
>  3 files changed, 31 insertions(+)
> 
> diff --git a/cgit.c b/cgit.c
> index 101954e12a..f06528215d 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -123,6 +123,8 @@ void config_cb(const char *name, const char *value)
>  		ctx.cfg.readme = xstrdup(value);
>  	else if (!strcmp(name, "user-envvar"))
>  		ctx.cfg.user_envvar = xstrdup(value);
> +	else if (!strcmp(name, "authorization"))
> +		ctx.cfg.authorization = xstrdup(value);
>  	else if (!strcmp(name, "root-title"))
>  		ctx.cfg.root_title = xstrdup(value);
>  	else if (!strcmp(name, "root-desc"))
> @@ -372,6 +374,7 @@ static void prepare_context(struct cgit_context *ctx)
>  	ctx->cfg.summary_tags = 10;
>  	ctx->cfg.max_atom_items = 10;
>  	ctx->cfg.ssdiff = 0;
> +	ctx->cfg.authorization = "/bin/true";
>  	ctx->cfg.user_envvar = "REMOTE_USER";
>  	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
>  	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
> @@ -545,6 +548,27 @@ static void process_request(void *cbdata)
>  		return;
>  	}
>  
> +	/* Here we do the authorization check.
> +	 *
> +	 * TODO
> +	 * ----
> +	 *  * figure out if this is the correct place to do the check
> +	 *  * check that the authorization script exists and is executable
> +	 *
> +	 */
> +	if (ctx->repo && system(fmt("%s %s %s",
> +					ctx->cfg.authorization,
> +					ctx->repo->name,
> +					ctx->env.remote_user)) != 0) {
> +		cgit_print_http_headers(ctx);
> +		cgit_print_docstart(ctx);
> +		cgit_print_pageheader(ctx);
> +		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
> +					ctx->repo->name, ctx->env.remote_user));
> +		cgit_print_docend();
> +		exit(1);
> +	}
> +
>  	if (ctx->repo && prepare_repo_cmd(ctx))
>  		return;
>  
> diff --git a/cgit.h b/cgit.h
> index 369dd8af8b..43f4f562f3 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -166,6 +166,7 @@ struct cgit_query {
>  struct cgit_config {
>  	char *agefile;
>  	char *user_envvar;
> +	char *authorization;
>  	char *cache_root;
>  	char *clone_prefix;
>  	char *clone_url;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index cd7327a4b3..d864289502 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -40,6 +40,12 @@ agefile::
>  	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
>  	hh:mm:ss". Default value: "info/web/last-modified".
>  
> +authorization::
> +	Specifies a path to authorization executable. The executable must take two
> +	arguments: "<repository> <user> [<permissions>]". The permission is the
> +	type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
> +	common use case is to call gitolite through this machinery.
> +
>  cache-root::
>  	Path used to store the cgit cache entries. Default value:
>  	"/var/cache/cgit". See also: "MACRO EXPANSION".
> -- 
> 1.7.9.5
> 

[PATCH 2/3] Add ability to authorize viewing a repository

[Jason]

On Tue, Oct 16, 2012 at 11:15 AM, Valentin Haenel
<valentin.haenel at gmx.de> wrote:
>         ctx->cfg.ssdiff = 0;
> +       ctx->cfg.authorization = "/bin/true";

This should be set to null by default.

Also, choose a more specific variable name than "authorization".


>
> +       /* Here we do the authorization check.
> +        *
> +        * TODO
> +        * ----
> +        *  * figure out if this is the correct place to do the check
> +        *  * check that the authorization script exists and is executable
> +        *
> +        */

It's best to submit code that's been completed, not with glaring
TODOs, whose fate of completion is unknown.


> +       if (ctx->repo && system(fmt("%s %s %s",
> +                                       ctx->cfg.authorization,
> +                                       ctx->repo->name,
> +                                       ctx->env.remote_user)) != 0) {


Considering the null change above, this should also be changed to  "if
(ctx..authorization && !system(...)) return;


> +authorization::
> +       Specifies a path to authorization executable. The executable must take two
> +       arguments: "<repository> <user> [<permissions>]". The permission is the
> +       type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
> +       common use case is to call gitolite through this machinery.
> +

Remove reference to gitolite. Add default value.



In general, the idea of having this is nice, but the implementation
needs to be polished.

[PATCH 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

Hi Jason,

thanks for your review.

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-16]:
> On Tue, Oct 16, 2012 at 11:15 AM, Valentin Haenel
> <valentin.haenel at gmx.de> wrote:
> >         ctx->cfg.ssdiff = 0;
> > +       ctx->cfg.authorization = "/bin/true";
> 
> This should be set to null by default.
> 
> Also, choose a more specific variable name than "authorization".

Perhaps you have some ideas, the two that come to mind here are:

* authz-exec
* authorization-executable

> > +       /* Here we do the authorization check.
> > +        *
> > +        * TODO
> > +        * ----
> > +        *  * figure out if this is the correct place to do the check
> > +        *  * check that the authorization script exists and is executable
> > +        *
> > +        */
> 
> It's best to submit code that's been completed, not with glaring
> TODOs, whose fate of completion is unknown.

I left them in because I couldn't resolve them on my own. Especially the
question about this being the right place. Would appreciate some
feedback on this.

Regarding the non-existence of the script, AFAICT 'system' will hand
over execution to a command processor which will return non-zero exit
status if the executable does not exist. Not sure what to do here. An
admin might misspell the executable name and get confused because he
thinks his authorization isn't working properly.

> > +       if (ctx->repo && system(fmt("%s %s %s",
> > +                                       ctx->cfg.authorization,
> > +                                       ctx->repo->name,
> > +                                       ctx->env.remote_user)) != 0) {
> 
> 
> Considering the null change above, this should also be changed to  "if
> (ctx..authorization && !system(...)) return;

OK.

> > +authorization::
> > +       Specifies a path to authorization executable. The executable must take two
> > +       arguments: "<repository> <user> [<permissions>]". The permission is the
> > +       type of permission we wish to check for, e.g. "R" or "RW" or "RW+". A
> > +       common use case is to call gitolite through this machinery.
> > +
> 
> Remove reference to gitolite. Add default value.

OK.

> In general, the idea of having this is nice, but the implementation
> needs to be polished.

Sure, I can fix any issues that are raised by people on this list. I'll
take care of the ones I OKd now and wait for more feedback on the
others before sending a 'v2' of this patch stack.

V-

[PATCHv2 1/3] Add config option user-envvar

[valentin.haenel]

When cgit sits on a backend server and relies on a set of
front-ends to do authentication, it will read the username
from an environment variable defined by this option.

In this way, one can safely use any forwarded HTTP header
and not only the expected REMOTE_USER variable set by the
CGI standard.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   10 ++++++++++
 cgit.h       |    2 ++
 cgitrc.5.txt |    6 ++++++
 3 files changed, 18 insertions(+)

diff --git a/cgit.c b/cgit.c
index a97ed69653..92e35ae958 100644
--- a/cgit.c
+++ b/cgit.c
@@ -126,6 +126,8 @@ void config_cb(const char *name, const char *value)
 		repo_config(ctx.repo, name + 5, value);
 	else if (!strcmp(name, "readme"))
 		ctx.cfg.readme = xstrdup(value);
+	else if (!strcmp(name, "user-envvar"))
+		ctx.cfg.user_envvar = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -379,6 +381,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.user_envvar = "REMOTE_USER";
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
 	ctx->env.https = xstrdupn(getenv("HTTPS"));
@@ -823,6 +826,13 @@ int main(int argc, const char **argv)
 	ctx.repo = NULL;
 	http_parse_querystring(ctx.qry.raw, querystring_cb);
 
+	/*
+	 * Get the username of an authenticated user. It will get
+	 * from the environment variable defined by the user-header
+	 * option (defaults to REMOTE_USER)
+	 */
+	ctx.env.remote_user = xstrdupn(getenv(ctx.cfg.user_envvar));
+
 	/* If virtual-root isn't specified in cgitrc, lets pretend
 	 * that virtual-root equals SCRIPT_NAME, minus any possibly
 	 * trailing slashes.
diff --git a/cgit.h b/cgit.h
index 7a99135710..016baa8e7d 100644
--- a/cgit.h
+++ b/cgit.h
@@ -166,6 +166,7 @@ struct cgit_query {
 
 struct cgit_config {
 	char *agefile;
+	char *user_envvar;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
@@ -263,6 +264,7 @@ struct cgit_environment {
 	char *script_name;
 	char *server_name;
 	char *server_port;
+	char *remote_user;
 };
 
 struct cgit_context {
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 7d01fcde58..7a479d1d84 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -389,6 +389,12 @@ strict-export::
 	repositories to match those exported by git-daemon. This option MUST come
 	before 'scan-path'.
 
+user-envvar::
+	Environment variable to read the user name from in a CGI environment. By
+	default, CGI exports it with the REMOTE_USER variable. This parameter can
+	be adjusted to a custom variable (e.g. any HTTP header forwarded by an
+	external authentication engine like HTTP_X_FORWARDED_USER)
+
 virtual-root::
 	Url which, if specified, will be used as root for all cgit links. It
 	will also cause cgit to generate 'virtual urls', i.e. urls like
-- 
1.7.9.5

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

This provides a mechanism for cgit to query an external program for repository
authorisation, e.g. gitolite. An additional config variable 'authorization'
contains the path and name of the executable to use.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   22 ++++++++++++++++++++++
 cgit.h       |    1 +
 cgitrc.5.txt |    6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/cgit.c b/cgit.c
index 92e35ae958..d62c1a748d 100644
--- a/cgit.c
+++ b/cgit.c
@@ -128,6 +128,8 @@ void config_cb(const char *name, const char *value)
 		ctx.cfg.readme = xstrdup(value);
 	else if (!strcmp(name, "user-envvar"))
 		ctx.cfg.user_envvar = xstrdup(value);
+	else if (!strcmp(name, "authz-exec"))
+		ctx.cfg.authz_exec = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -381,6 +383,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.authz_exec = NULL;
 	ctx->cfg.user_envvar = "REMOTE_USER";
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
@@ -554,6 +557,25 @@ static void process_request(void *cbdata)
 		return;
 	}
 
+	/* Here we do the authorization check.
+	 *
+	 * TODO figure out if this is the correct place to do the check
+	 *
+	 */
+	if (ctx->cfg.authz_exec && ctx->repo &&
+					system(fmt("%s %s %s",
+					ctx->cfg.authz_exec,
+					ctx->repo->name,
+					ctx->env.remote_user)) != 0) {
+		cgit_print_http_headers(ctx);
+		cgit_print_docstart(ctx);
+		cgit_print_pageheader(ctx);
+		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
+					ctx->repo->name, ctx->env.remote_user));
+		cgit_print_docend();
+		return;
+	}
+
 	if (ctx->repo && prepare_repo_cmd(ctx))
 		return;
 
diff --git a/cgit.h b/cgit.h
index 016baa8e7d..14fc2fb777 100644
--- a/cgit.h
+++ b/cgit.h
@@ -167,6 +167,7 @@ struct cgit_query {
 struct cgit_config {
 	char *agefile;
 	char *user_envvar;
+	char *authz_exec;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 7a479d1d84..79625cf055 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -40,6 +40,12 @@ agefile::
 	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
 	hh:mm:ss". Default value: "info/web/last-modified".
 
+authorization::
+	Specifies a path to authorization executable. The executable must take two
+	arguments: "<repository> <user> [<permissions>]". The permission is the
+	type of permission we wish to check for, e.g. "R" or "RW" or "RW+". Default
+	value: none.
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "MACRO EXPANSION".
-- 
1.7.9.5

[PATCHv2 3/3] Helper script to interface to gitolite

[valentin.haenel]

From: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>

Signed-off-by: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 contrib/gl-check-user |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100755 contrib/gl-check-user

diff --git a/contrib/gl-check-user b/contrib/gl-check-user
new file mode 100755
index 0000000000..2f9a331e00
--- /dev/null
+++ b/contrib/gl-check-user
@@ -0,0 +1,18 @@
+#!/bin/sh
+# Wrapper around gitolite to perform
+# repository authentication from a
+# CGI environment
+prog="/usr/local/bin/gitolite"
+
+# Repository to check access against
+# Strip the trailing .git if one is
+# present
+export REPO=${1%%.git}
+export REMOTE_USER=${2}
+export PERM=${3-"R"}
+# HTTPD will not set some essential
+# variables expexted by gitolite
+# Set them here (EUID expected final)
+export HOME=`getent passwd $(id -n -u) | cut -d":" -f 6`
+
+exec $prog access -q ${REPO} ${REMOTE_USER} ${PERM}
-- 
1.7.9.5

[PATCHv2 1/3] Add config option user-envvar

[mathstuf]

On Mon, Oct 22, 2012 at 08:29:16 GMT, Valentin Haenel wrote:
> When cgit sits on a backend server and relies on a set of
> front-ends to do authentication, it will read the username
> from an environment variable defined by this option.
>
> In this way, one can safely use any forwarded HTTP header
> and not only the expected REMOTE_USER variable set by the
> CGI standard.
>
> Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
> ---
>  cgit.c       |   10 ++++++++++
>  cgit.h       |    2 ++
>  cgitrc.5.txt |    6 ++++++
>  3 files changed, 18 insertions(+)
>
> diff --git a/cgit.c b/cgit.c
> index a97ed69653..92e35ae958 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -126,6 +126,8 @@ void config_cb(const char *name, const char *value)
>  		repo_config(ctx.repo, name + 5, value);
>  	else if (!strcmp(name, "readme"))
>  		ctx.cfg.readme = xstrdup(value);
> +	else if (!strcmp(name, "user-envvar"))
> +		ctx.cfg.user_envvar = xstrdup(value);
>  	else if (!strcmp(name, "root-title"))
>  		ctx.cfg.root_title = xstrdup(value);
>  	else if (!strcmp(name, "root-desc"))
> @@ -379,6 +381,7 @@ static void prepare_context(struct cgit_context *ctx)
>  	ctx->cfg.summary_tags = 10;
>  	ctx->cfg.max_atom_items = 10;
>  	ctx->cfg.ssdiff = 0;
> +	ctx->cfg.user_envvar = "REMOTE_USER";

Use xstrdupn here. It can't be free'd if it's static like this.

>  	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
>  	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
>  	ctx->env.https = xstrdupn(getenv("HTTPS"));
> @@ -823,6 +826,13 @@ int main(int argc, const char **argv)
>  	ctx.repo = NULL;
>  	http_parse_querystring(ctx.qry.raw, querystring_cb);
>  
> +	/*
> +	 * Get the username of an authenticated user. It will get
> +	 * from the environment variable defined by the user-header
> +	 * option (defaults to REMOTE_USER)
> +	 */
> +	ctx.env.remote_user = xstrdupn(getenv(ctx.cfg.user_envvar));
> +
>  	/* If virtual-root isn't specified in cgitrc, lets pretend
>  	 * that virtual-root equals SCRIPT_NAME, minus any possibly
>  	 * trailing slashes.
> diff --git a/cgit.h b/cgit.h
> index 7a99135710..016baa8e7d 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -166,6 +166,7 @@ struct cgit_query {
>  
>  struct cgit_config {
>  	char *agefile;
> +	char *user_envvar;

It should be free'd where the rest of these are free'd. I don't see that
here.

>  	char *cache_root;
>  	char *clone_prefix;
>  	char *clone_url;
> @@ -263,6 +264,7 @@ struct cgit_environment {
>  	char *script_name;
>  	char *server_name;
>  	char *server_port;
> +	char *remote_user;

Same here.

-- Ben

[PATCHv2 3/3] Helper script to interface to gitolite

[mathstuf]

On Mon, Oct 22, 2012 at 08:29:18 GMT, Valentin Haenel wrote:
> @@ -0,0 +1,18 @@
> +#!/bin/sh
> +# Wrapper around gitolite to perform
> +# repository authentication from a
> +# CGI environment
> +prog="/usr/local/bin/gitolite"
> +
> +# Repository to check access against
> +# Strip the trailing .git if one is
> +# present
> +export REPO=${1%%.git}
> +export REMOTE_USER=${2}
> +export PERM=${3-"R"}
> +# HTTPD will not set some essential
> +# variables expexted by gitolite
> +# Set them here (EUID expected final)
> +export HOME=`getent passwd $(id -n -u) | cut -d":" -f 6`
> +
> +exec $prog access -q ${REPO} ${REMOTE_USER} ${PERM}

Quoting highly recommended around the variables (I suspect other shell
scripts need a once over for this, but let's not add more instances :)
).

-- Ben

[PATCHv2 2/3] Add ability to authorize viewing a repository

[mathstuf]

On Mon, Oct 22, 2012 at 08:29:17 GMT, Valentin Haenel wrote:
> @@ -554,6 +557,25 @@ static void process_request(void *cbdata)
>  		return;
>  	}
>  
> +	/* Here we do the authorization check.
> +	 *
> +	 * TODO figure out if this is the correct place to do the check
> +	 *
> +	 */
> +	if (ctx->cfg.authz_exec && ctx->repo &&
> +					system(fmt("%s %s %s",

Single quote the arguments to the executable. This is ripe for code
execution (remote_user is under attacker's control).

> +					ctx->cfg.authz_exec,
> +					ctx->repo->name,
> +					ctx->env.remote_user)) != 0) {
> +		cgit_print_http_headers(ctx);
> +		cgit_print_docstart(ctx);
> +		cgit_print_pageheader(ctx);
> +		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
> +					ctx->repo->name, ctx->env.remote_user));
> +		cgit_print_docend();
> +		return;
> +	}
> +
>  	if (ctx->repo && prepare_repo_cmd(ctx))
>  		return;
>  
> diff --git a/cgit.h b/cgit.h
> index 016baa8e7d..14fc2fb777 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -167,6 +167,7 @@ struct cgit_query {
>  struct cgit_config {
>  	char *agefile;
>  	char *user_envvar;
> +	char *authz_exec;

Where is this free'd?

-- Ben

[PATCH 1/3] Add config option user-envvar

[Jason]

On Tue, Oct 16, 2012 at 3:15 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
>
> When cgit sits on a backend server and relies on a set of
> front-ends to do authentication, it will read the username
> from an environment variable defined by this option.
>
> In this way, one can safely use any forwarded HTTP header
> and not only the expected REMOTE_USER variable set by the
> CGI standard.


Why is this necessary at all? Won't helper programs be given the full
environment of the parent program (cgit<--cgi server), and so it can
be up to the helper script to determine the username by getting the
env var itself? The book keeping inside cgit in this patch seems
wasteful.

[PATCHv2 2/3] Add ability to authorize viewing a repository

[Jason]

On Mon, Oct 22, 2012 at 2:29 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> This provides a mechanism for cgit to query an external program for repository
> authorisation, e.g. gitolite. An additional config variable 'authorization'
> contains the path and name of the executable to use.

The config var is called 'authorization'. The struct var is called 'authz_exec'.

Ugly. Let's unify these into one config variable / struct variable
called "authorization_helper".

[PATCHv2 2/3] Add ability to authorize viewing a repository

[Jason]

On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
> Single quote the arguments to the executable. This is ripe for code
> execution (remote_user is under attacker's control).

Was going to mention this myself, but you beat me too it. Dead on.
Correctamundo.

Please double double tripe triple check your code before submitting things.


While we're on the topic, is "system" the best way to be calling this?
Since an auth helper gets called for each and every request, it seems
like it'd be cleanest if we could just fork/exec/wait ourselves,
passing the options in to execv. This way we don't have to fire up a
shell interpreter each time.

[PATCHv2 2/3] Add ability to authorize viewing a repository

[Jason]

On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
>> +             cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
>> +                                     ctx->repo->name, ctx->env.remote_user));

XSS.

[PATCHv2 2/3] Add ability to authorize viewing a repository

[mathstuf]

On Sun, Oct 28, 2012 at 01:16:36 GMT, Jason A. Donenfeld wrote:
> On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
>> Single quote the arguments to the executable. This is ripe for code
>> execution (remote_user is under attacker's control).
>
> Was going to mention this myself, but you beat me too it. Dead on.
> Correctamundo.
>
> Please double double tripe triple check your code before submitting things.

Working on the uzbl shell scripts (where we require POSIX compatibility
as well) has made these problems stick out like sore thumbs :) . Alas,
secure shell coding isn't the default mode for many programmers :( .
I'll look at the other shell code in cgit this weekend.

> While we're on the topic, is "system" the best way to be calling this?
> Since an auth helper gets called for each and every request, it seems
> like it'd be cleanest if we could just fork/exec/wait ourselves,
> passing the options in to execv. This way we don't have to fire up a
> shell interpreter each time.

Well, that would require that we either shell-split the option (messy
and error prone in and of itself) or effectively require trampoline
scripts for everything due to enforced arguments. Though...something
taking these arguments naturally is unlikely. +1 for fork/exec here.

--Ben

[PATCHv2 2/3] Add ability to authorize viewing a repository

[Jason]

On Sat, Oct 27, 2012 at 7:29 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
> Well, that would require that we either shell-split the option (messy
> and error prone in and of itself) or effectively require trampoline
> scripts for everything due to enforced arguments. Though...something
> taking these arguments naturally is unlikely. +1 for fork/exec here.

In light of the prior suggestion of leaving the env var choice to the
helper script, a trampoline is gonna be necessary no matter what. For
resource intensive things, just write it in C.

On the topic of 'enforced arguments', it might be cleaner to actually
pass everything we need as environment variables (set within the fork
before the exec). That way scripts don't have to messily deal with
positional arguments.

[PATCHv2 1/3] Add config option user-envvar

[valentin.haenel]

* Ben Boeckel <mathstuf at gmail.com> [2012-10-28]:
> On Mon, Oct 22, 2012 at 08:29:16 GMT, Valentin Haenel wrote:
> > When cgit sits on a backend server and relies on a set of
> > front-ends to do authentication, it will read the username
> > from an environment variable defined by this option.
> >
> > In this way, one can safely use any forwarded HTTP header
> > and not only the expected REMOTE_USER variable set by the
> > CGI standard.
> >
> > Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
> > ---
> >  cgit.c       |   10 ++++++++++
> >  cgit.h       |    2 ++
> >  cgitrc.5.txt |    6 ++++++
> >  3 files changed, 18 insertions(+)
> >
> > diff --git a/cgit.c b/cgit.c
> > index a97ed69653..92e35ae958 100644
> > --- a/cgit.c
> > +++ b/cgit.c
> > @@ -126,6 +126,8 @@ void config_cb(const char *name, const char *value)
> >  		repo_config(ctx.repo, name + 5, value);
> >  	else if (!strcmp(name, "readme"))
> >  		ctx.cfg.readme = xstrdup(value);
> > +	else if (!strcmp(name, "user-envvar"))
> > +		ctx.cfg.user_envvar = xstrdup(value);
> >  	else if (!strcmp(name, "root-title"))
> >  		ctx.cfg.root_title = xstrdup(value);
> >  	else if (!strcmp(name, "root-desc"))
> > @@ -379,6 +381,7 @@ static void prepare_context(struct cgit_context *ctx)
> >  	ctx->cfg.summary_tags = 10;
> >  	ctx->cfg.max_atom_items = 10;
> >  	ctx->cfg.ssdiff = 0;
> > +	ctx->cfg.user_envvar = "REMOTE_USER";
> 
> Use xstrdupn here. It can't be free'd if it's static like this.

Check.

> >  	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
> >  	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
> >  	ctx->env.https = xstrdupn(getenv("HTTPS"));
> > @@ -823,6 +826,13 @@ int main(int argc, const char **argv)
> >  	ctx.repo = NULL;
> >  	http_parse_querystring(ctx.qry.raw, querystring_cb);
> >  
> > +	/*
> > +	 * Get the username of an authenticated user. It will get
> > +	 * from the environment variable defined by the user-header
> > +	 * option (defaults to REMOTE_USER)
> > +	 */
> > +	ctx.env.remote_user = xstrdupn(getenv(ctx.cfg.user_envvar));
> > +
> >  	/* If virtual-root isn't specified in cgitrc, lets pretend
> >  	 * that virtual-root equals SCRIPT_NAME, minus any possibly
> >  	 * trailing slashes.
> > diff --git a/cgit.h b/cgit.h
> > index 7a99135710..016baa8e7d 100644
> > --- a/cgit.h
> > +++ b/cgit.h
> > @@ -166,6 +166,7 @@ struct cgit_query {
> >  
> >  struct cgit_config {
> >  	char *agefile;
> > +	char *user_envvar;
> 
> It should be free'd where the rest of these are free'd. I don't see that
> here.
> 
> >  	char *cache_root;
> >  	char *clone_prefix;
> >  	char *clone_url;
> > @@ -263,6 +264,7 @@ struct cgit_environment {
> >  	char *script_name;
> >  	char *server_name;
> >  	char *server_port;
> > +	char *remote_user;
> 
> Same here.

Forgive me if I am mistaken, but I don't see any of those free'd
anywhere.

V-

[PATCHv2 3/3] Helper script to interface to gitolite

[valentin.haenel]

* Ben Boeckel <mathstuf at gmail.com> [2012-10-28]:
> On Mon, Oct 22, 2012 at 08:29:18 GMT, Valentin Haenel wrote:
> > @@ -0,0 +1,18 @@
> > +#!/bin/sh
> > +# Wrapper around gitolite to perform
> > +# repository authentication from a
> > +# CGI environment
> > +prog="/usr/local/bin/gitolite"
> > +
> > +# Repository to check access against
> > +# Strip the trailing .git if one is
> > +# present
> > +export REPO=${1%%.git}
> > +export REMOTE_USER=${2}
> > +export PERM=${3-"R"}
> > +# HTTPD will not set some essential
> > +# variables expexted by gitolite
> > +# Set them here (EUID expected final)
> > +export HOME=`getent passwd $(id -n -u) | cut -d":" -f 6`
> > +
> > +exec $prog access -q ${REPO} ${REMOTE_USER} ${PERM}
> 
> Quoting highly recommended around the variables (I suspect other shell
> scripts need a once over for this, but let's not add more instances :)
> ).

Check.

V-

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-28]:
> On Mon, Oct 22, 2012 at 2:29 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> > This provides a mechanism for cgit to query an external program for repository
> > authorisation, e.g. gitolite. An additional config variable 'authorization'
> > contains the path and name of the executable to use.
> 
> The config var is called 'authorization'. The struct var is called 'authz_exec'.
> 
> Ugly. Let's unify these into one config variable / struct variable
> called "authorization_helper".

Check.

V-

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-28]:
> On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
> > Single quote the arguments to the executable. This is ripe for code
> > execution (remote_user is under attacker's control).
> 
> Was going to mention this myself, but you beat me too it. Dead on.
> Correctamundo.
> 
> Please double double tripe triple check your code before submitting things.

I added the single quotes as suggested. When I looked at the code
initially, I was reasoning that the remote_user is set by the
authentication part, in our case this is Apache, which in turn asks
LDAP. Furthermore, Apache sets the remote_user and forward to cgit only
if the user is actually a valid user. So my assumption was, that
remote_user is not under the attackers control.

I guess I need some more help to understand why I am mistaken about
this. Is it the case that the assumption fails, if an attacker can
inject something into LDAP he may be able to pass through apache
successfully and then have his exploit, which is in remote_user, be
executed on the machine which is running cgit?

V-

[PATCH 1/3] Add config option user-envvar

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-28]:
> On Tue, Oct 16, 2012 at 3:15 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> >
> > When cgit sits on a backend server and relies on a set of
> > front-ends to do authentication, it will read the username
> > from an environment variable defined by this option.
> >
> > In this way, one can safely use any forwarded HTTP header
> > and not only the expected REMOTE_USER variable set by the
> > CGI standard.
> 
> 
> Why is this necessary at all? Won't helper programs be given the full
> environment of the parent program (cgit<--cgi server), and so it can
> be up to the helper script to determine the username by getting the
> env var itself? The book keeping inside cgit in this patch seems
> wasteful.

The only reason to fetch the remote_user would be to echo back an
appropriate error message.

V-

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-28]:
> On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
> >> +             cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
> >> +                                     ctx->repo->name, ctx->env.remote_user));
> 
> XSS.

Would it be enough to use 'html_txt' from html.c:

http://git.zx2c4.com/cgit/tree/html.c#n92

to prevent this?

V-

[PATCHv2 2/3] Add ability to authorize viewing a repository

[mathstuf]

On Mon, Oct 29, 2012 at 10:43:47 +0100, Valentin Haenel wrote:
> I added the single quotes as suggested. When I looked at the code
> initially, I was reasoning that the remote_user is set by the
> authentication part, in our case this is Apache, which in turn asks
> LDAP. Furthermore, Apache sets the remote_user and forward to cgit only
> if the user is actually a valid user. So my assumption was, that
> remote_user is not under the attackers control.
> 
> I guess I need some more help to understand why I am mistaken about
> this. Is it the case that the assumption fails, if an attacker can
> inject something into LDAP he may be able to pass through apache
> successfully and then have his exploit, which is in remote_user, be
> executed on the machine which is running cgit?

Okay, that makes it sound to me as if it makes sense to have a setting
for the variable.

We should quote it since the provenance is outside of cgit. If
preventing code exection is a couple of characters added on our side,
it'll mean we don't have to worry about bugs in apache, nginx, lighttpd,
slapd, and every other piece of software which might be involved.
Remember, we need only give attackers one hole and we're done. Even if
"that's impossible" is the gut reaction, that's a target attackers can
knowningly try to thread arbitrary strings through towards.

It also means we're guarded against obscure webserver setups which is
always a good thing (http://git.example.org/~foobar giving the user's
view being turned into a user's view is possible).

--Ben

[PATCHv2 1/3] Add config option user-envvar

[mathstuf]

On Mon, Oct 29, 2012 at 10:22:18 +0100, Valentin Haenel wrote:
> * Ben Boeckel <mathstuf at gmail.com> [2012-10-28]:
> > On Mon, Oct 22, 2012 at 08:29:16 GMT, Valentin Haenel wrote:
> > > @@ -166,6 +166,7 @@ struct cgit_query {
> > >  
> > >  struct cgit_config {
> > >  	char *agefile;
> > > +	char *user_envvar;
> > 
> > It should be free'd where the rest of these are free'd. I don't see that
> > here.
> > 
> > >  	char *cache_root;
> > >  	char *clone_prefix;
> > >  	char *clone_url;
> > > @@ -263,6 +264,7 @@ struct cgit_environment {
> > >  	char *script_name;
> > >  	char *server_name;
> > >  	char *server_port;
> > > +	char *remote_user;
> > 
> > Same here.
> 
> Forgive me if I am mistaken, but I don't see any of those free'd
> anywhere.

Hrm. That's...unfortunate. But likely a different patch if it's missing
altogether.

--Ben

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

* Ben Boeckel <mathstuf at gmail.com> [2012-10-29]:
> On Mon, Oct 29, 2012 at 10:43:47 +0100, Valentin Haenel wrote:
> > I added the single quotes as suggested. When I looked at the code
> > initially, I was reasoning that the remote_user is set by the
> > authentication part, in our case this is Apache, which in turn asks
> > LDAP. Furthermore, Apache sets the remote_user and forward to cgit only
> > if the user is actually a valid user. So my assumption was, that
> > remote_user is not under the attackers control.
> > 
> > I guess I need some more help to understand why I am mistaken about
> > this. Is it the case that the assumption fails, if an attacker can
> > inject something into LDAP he may be able to pass through apache
> > successfully and then have his exploit, which is in remote_user, be
> > executed on the machine which is running cgit?
> 
> Okay, that makes it sound to me as if it makes sense to have a setting
> for the variable.
> 
> We should quote it since the provenance is outside of cgit. If
> preventing code exection is a couple of characters added on our side,
> it'll mean we don't have to worry about bugs in apache, nginx, lighttpd,
> slapd, and every other piece of software which might be involved.
> Remember, we need only give attackers one hole and we're done. Even if
> "that's impossible" is the gut reaction, that's a target attackers can
> knowningly try to thread arbitrary strings through towards.
> 
> It also means we're guarded against obscure webserver setups which is
> always a good thing (http://git.example.org/~foobar giving the user's
> view being turned into a user's view is possible).

Yeah, better safe than sorry. :-D

V-

[PATCHv2 1/3] Add config option user-envvar

[valentin.haenel]

* Ben Boeckel <mathstuf at gmail.com> [2012-10-29]:
> On Mon, Oct 29, 2012 at 10:22:18 +0100, Valentin Haenel wrote:
> > * Ben Boeckel <mathstuf at gmail.com> [2012-10-28]:
> > > On Mon, Oct 22, 2012 at 08:29:16 GMT, Valentin Haenel wrote:
> > > > @@ -166,6 +166,7 @@ struct cgit_query {
> > > >  
> > > >  struct cgit_config {
> > > >  	char *agefile;
> > > > +	char *user_envvar;
> > > 
> > > It should be free'd where the rest of these are free'd. I don't see that
> > > here.
> > > 
> > > >  	char *cache_root;
> > > >  	char *clone_prefix;
> > > >  	char *clone_url;
> > > > @@ -263,6 +264,7 @@ struct cgit_environment {
> > > >  	char *script_name;
> > > >  	char *server_name;
> > > >  	char *server_port;
> > > > +	char *remote_user;
> > > 
> > > Same here.
> > 
> > Forgive me if I am mistaken, but I don't see any of those free'd
> > anywhere.
> 
> Hrm. That's...unfortunate. But likely a different patch if it's missing
> altogether.

I think the reasoning is: it's a CGI, so it gets called for every request
and then exits. So maybe, there is no need to free anything, because the
program is short lived?

V-

[PATCHv2 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

* Valentin Haenel <valentin.haenel at gmx.de> [2012-10-29]:
> * Jason A. Donenfeld <Jason at zx2c4.com> [2012-10-28]:
> > On Sat, Oct 27, 2012 at 7:00 PM, Ben Boeckel <mathstuf at gmail.com> wrote:
> > >> +             cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
> > >> +                                     ctx->repo->name, ctx->env.remote_user));
> > 
> > XSS.
> 
> Would it be enough to use 'html_txt' from html.c:
> 
> http://git.zx2c4.com/cgit/tree/html.c#n92
> 
> to prevent this?

After further investigation, I discovered that 'cgit_print_error' does
'html_txt' to do the escaping:

http://git.zx2c4.com/cgit/tree/ui-shared.c#n30'

V-

[PATCHv3 0/3] Implement authorization via external program (v3)

[valentin.haenel]

This is v3 of this pacth stack. Including many suggestions from the
mailinglist where possible.

The two remaining questions I would like to get some more feedback on are:

* Is this the right place to perform the check (see TODO in code)?

* Do we want to use system or fork/exec/wait when calling the external helper?


Carlos Aguado Sanchez (1):
  Helper script to interface to gitolite

Valentin Haenel (2):
  Add config option user-envvar
  Add ability to authorize viewing a repository

 cgit.c                |   32 ++++++++++++++++++++++++++++++++
 cgit.h                |    3 +++
 cgitrc.5.txt          |   13 +++++++++++++
 contrib/gl-check-user |   18 ++++++++++++++++++
 4 files changed, 66 insertions(+)
 create mode 100755 contrib/gl-check-user

-- 
1.7.9.5

[PATCHv3 1/3] Add config option user-envvar

[valentin.haenel]

When cgit sits on a backend server and relies on a set of
front-ends to do authentication, it will read the username
from an environment variable defined by this option.

In this way, one can safely use any forwarded HTTP header
and not only the expected REMOTE_USER variable set by the
CGI standard.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   10 ++++++++++
 cgit.h       |    2 ++
 cgitrc.5.txt |    7 +++++++
 3 files changed, 19 insertions(+)

diff --git a/cgit.c b/cgit.c
index a97ed69653..5a8ae97a13 100644
--- a/cgit.c
+++ b/cgit.c
@@ -126,6 +126,8 @@ void config_cb(const char *name, const char *value)
 		repo_config(ctx.repo, name + 5, value);
 	else if (!strcmp(name, "readme"))
 		ctx.cfg.readme = xstrdup(value);
+	else if (!strcmp(name, "user-envvar"))
+		ctx.cfg.user_envvar = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -379,6 +381,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.user_envvar = xstrdupn("REMOTE_USER");
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
 	ctx->env.https = xstrdupn(getenv("HTTPS"));
@@ -823,6 +826,13 @@ int main(int argc, const char **argv)
 	ctx.repo = NULL;
 	http_parse_querystring(ctx.qry.raw, querystring_cb);
 
+	/*
+	 * Get the username of an authenticated user. It will get
+	 * from the environment variable defined by the user-header
+	 * option (defaults to REMOTE_USER)
+	 */
+	ctx.env.remote_user = xstrdupn(getenv(ctx.cfg.user_envvar));
+
 	/* If virtual-root isn't specified in cgitrc, lets pretend
 	 * that virtual-root equals SCRIPT_NAME, minus any possibly
 	 * trailing slashes.
diff --git a/cgit.h b/cgit.h
index 7a99135710..016baa8e7d 100644
--- a/cgit.h
+++ b/cgit.h
@@ -166,6 +166,7 @@ struct cgit_query {
 
 struct cgit_config {
 	char *agefile;
+	char *user_envvar;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
@@ -263,6 +264,7 @@ struct cgit_environment {
 	char *script_name;
 	char *server_name;
 	char *server_port;
+	char *remote_user;
 };
 
 struct cgit_context {
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 5887cee9a8..d2fca9fb9c 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -389,6 +389,13 @@ strict-export::
 	repositories to match those exported by git-daemon. This option MUST come
 	before 'scan-path'.
 
+user-envvar::
+	Environment variable to read the user name from in a CGI environment. By
+	default, CGI exports it with the REMOTE_USER variable. This parameter can
+	be adjusted to a custom variable (e.g. any HTTP header forwarded by an
+	external authentication engine like HTTP_X_FORWARDED_USER). Default value:
+	"REMOTE_USER".
+
 virtual-root::
 	Url which, if specified, will be used as root for all cgit links. It
 	will also cause cgit to generate 'virtual urls', i.e. urls like
-- 
1.7.9.5

[PATCHv3 2/3] Add ability to authorize viewing a repository

[valentin.haenel]

This provides a mechanism for cgit to query an external program for repository
authorisation, e.g. gitolite. An additional config variable 'authorization'
contains the path and name of the executable to use.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   22 ++++++++++++++++++++++
 cgit.h       |    1 +
 cgitrc.5.txt |    6 ++++++
 3 files changed, 29 insertions(+)

diff --git a/cgit.c b/cgit.c
index 5a8ae97a13..1bf1935b88 100644
--- a/cgit.c
+++ b/cgit.c
@@ -128,6 +128,8 @@ void config_cb(const char *name, const char *value)
 		ctx.cfg.readme = xstrdup(value);
 	else if (!strcmp(name, "user-envvar"))
 		ctx.cfg.user_envvar = xstrdup(value);
+	else if (!strcmp(name, "authz-exec"))
+		ctx.cfg.authorization_helper = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -382,6 +384,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
 	ctx->cfg.user_envvar = xstrdupn("REMOTE_USER");
+	ctx->cfg.authorization_helper = NULL;
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
 	ctx->env.https = xstrdupn(getenv("HTTPS"));
@@ -554,6 +557,25 @@ static void process_request(void *cbdata)
 		return;
 	}
 
+	/* Here we do the authorization check.
+	 *
+	 * TODO figure out if this is the correct place to do the check
+	 *
+	 */
+	if (ctx->cfg.authorization_helper && ctx->repo &&
+					system(fmt("'%s' '%s' '%s'",
+					ctx->cfg.authorization_helper,
+					ctx->repo->name,
+					ctx->env.remote_user)) != 0) {
+		cgit_print_http_headers(ctx);
+		cgit_print_docstart(ctx);
+		cgit_print_pageheader(ctx);
+		cgit_print_error(fmt("Authorization failed for repo: '%s' and user: '%s'",
+					ctx->repo->name, ctx->env.remote_user));
+		cgit_print_docend();
+		return;
+	}
+
 	if (ctx->repo && prepare_repo_cmd(ctx))
 		return;
 
diff --git a/cgit.h b/cgit.h
index 016baa8e7d..48888af0d3 100644
--- a/cgit.h
+++ b/cgit.h
@@ -167,6 +167,7 @@ struct cgit_query {
 struct cgit_config {
 	char *agefile;
 	char *user_envvar;
+	char *authorization_helper;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index d2fca9fb9c..fa51fe6a6e 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -40,6 +40,12 @@ agefile::
 	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
 	hh:mm:ss". Default value: "info/web/last-modified".
 
+authorization_helper::
+	Specifies a path to authorization executable. The executable must take two
+	arguments: "<repository> <user> [<permissions>]". The permission is the
+	type of permission we wish to check for, e.g. "R" or "RW" or "RW+". Default
+	value: none.
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "MACRO EXPANSION".
-- 
1.7.9.5

[PATCHv3 3/3] Helper script to interface to gitolite

[valentin.haenel]

From: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>

Signed-off-by: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 contrib/gl-check-user |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100755 contrib/gl-check-user

diff --git a/contrib/gl-check-user b/contrib/gl-check-user
new file mode 100755
index 0000000000..33c326043c
--- /dev/null
+++ b/contrib/gl-check-user
@@ -0,0 +1,18 @@
+#!/bin/sh
+# Wrapper around gitolite to perform
+# repository authentication from a
+# CGI environment
+prog="/usr/local/bin/gitolite"
+
+# Repository to check access against
+# Strip the trailing .git if one is
+# present
+export REPO=${1%%.git}
+export REMOTE_USER=${2}
+export PERM=${3-"R"}
+# HTTPD will not set some essential
+# variables expexted by gitolite
+# Set them here (EUID expected final)
+export HOME=$( getent passwd $(id -n -u) | cut -d":" -f 6 )
+
+exec $prog access -q "${REPO}" "${REMOTE_USER}" "${PERM}"
-- 
1.7.9.5

[PATCHv3 0/3] Implement authorization via external program (v3)

[mathstuf]

On Tue, Oct 30, 2012 at 11:11:27 +0100, Valentin Haenel wrote:
> * Do we want to use system or fork/exec/wait when calling the external helper?

IMO, it should be used.

--Ben

[PATCHv3 3/3] Helper script to interface to gitolite

[mathstuf]

On Tue, Oct 30, 2012 at 11:11:30 +0100, Valentin Haenel wrote:
> +export REPO=${1%%.git}
> +export REMOTE_USER=${2}
> +export PERM=${3-"R"}

Quote these expansions as well.

--Ben

[PATCHv3 1/3] Add config option user-envvar

[Jason]

On Tue, Oct 30, 2012 at 4:11 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> When cgit sits on a backend server and relies on a set of
> front-ends to do authentication, it will read the username
> from an environment variable defined by this option.
>
> In this way, one can safely use any forwarded HTTP header
> and not only the expected REMOTE_USER variable set by the
> CGI standard.
>

As I wrote in another email, this isn't necessary, and I don't dig the
extra book keeping. The helper script can manage all this using normal
environment variables that are inherited.

In fact, as I also explained before, instead of positional arguments,
everything can be passed in env vars. In fact, quite a few CGIT_* env
vars are already defined.

[PATCHv3 2/3] Add ability to authorize viewing a repository

[Jason]

On Tue, Oct 30, 2012 at 4:11 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> +       else if (!strcmp(name, "authz-exec"))
> +               ctx.cfg.authorization_helper = xstrdup(value);

The way it should work is:

Config var: authorization-helper
Code var: authorization_helper

[PATCHv3 0/3] Implement authorization via external program (v3)

[Jason]

On Tue, Oct 30, 2012 at 4:11 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> * Do we want to use system or fork/exec/wait when calling the external helper?

Fork/exec/wait, as previously mentioned.

[PATCHv4 0/2] Authorize viewing a repository

[valentin.haenel]

This is the fourth iteration of the patches to invoke an external helper for
authorizing viewing of repositories. The significant changes are that now, no
book-keeping of the REMOTE_USER or equivalent environment variable is done
within cgit and the authorization helper must instead obtain this from the
environment. Also, the authorization helper is no longer invoked via 'system',
but instead using a 'fork/exec/wait' construct. Lastly, the external helper is
no longer given any arguments, but must get the repository to authorize for,
from the environment.  To this end, the same "CGIT_*" environment variables
that are available to the filters are handed over. The cgit man page entry for
the authorization helper has been updated to describe the environment available
to the helper and the expected exit code.

The example for interfacing with gitolite has now got adequately quoted
variables and has been updated to reflect the changes in how the external
helper should be invoked.

Carlos Aguado Sanchez (1):
  Helper script to interface to gitolite

Valentin Haenel (1):
  Add ability to authorize viewing a repository

 cgit.c                |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 cgit.h                |    1 +
 cgitrc.5.txt          |    6 ++++++
 contrib/gl-check-user |   20 ++++++++++++++++++++
 4 files changed, 73 insertions(+)
 create mode 100755 contrib/gl-check-user

-- 
1.7.9.5

[PATCHv4 1/2] Add ability to authorize viewing a repository

[valentin.haenel]

This provides a mechanism for cgit to query an external program for repository
authorisation, e.g. gitolite. An additional config variable 'authorization-helper'
contains the path and name of the executable to use.

Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 cgit.c       |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 cgit.h       |    1 +
 cgitrc.5.txt |    6 ++++++
 3 files changed, 53 insertions(+)

diff --git a/cgit.c b/cgit.c
index a97ed69653..dbce4c2e1c 100644
--- a/cgit.c
+++ b/cgit.c
@@ -126,6 +126,8 @@ void config_cb(const char *name, const char *value)
 		repo_config(ctx.repo, name + 5, value);
 	else if (!strcmp(name, "readme"))
 		ctx.cfg.readme = xstrdup(value);
+	else if (!strcmp(name, "authorization-helper"))
+		ctx.cfg.authorization_helper = xstrdup(value);
 	else if (!strcmp(name, "root-title"))
 		ctx.cfg.root_title = xstrdup(value);
 	else if (!strcmp(name, "root-desc"))
@@ -379,6 +381,7 @@ static void prepare_context(struct cgit_context *ctx)
 	ctx->cfg.summary_tags = 10;
 	ctx->cfg.max_atom_items = 10;
 	ctx->cfg.ssdiff = 0;
+	ctx->cfg.authorization_helper = NULL;
 	ctx->env.cgit_config = xstrdupn(getenv("CGIT_CONFIG"));
 	ctx->env.http_host = xstrdupn(getenv("HTTP_HOST"));
 	ctx->env.https = xstrdupn(getenv("HTTPS"));
@@ -513,6 +516,40 @@ static int prepare_repo_cmd(struct cgit_context *ctx)
 	return 0;
 }
 
+/* Run an external authorization helper via fork/exec/wait.
+ *
+ * Returns the exit code of the authorization helper if this exited
+ * successfully (0 will be success) and -1 in case of any other errors. If
+ * there is no authorization helper configured, it will return 0 too.
+ *
+ * */
+static int run_authorization_helper(struct cgit_context *ctx)
+{
+	if (ctx->cfg.authorization_helper == NULL)
+		return 0;
+	int wait_ret;
+	pid_t pid = fork();
+	switch (pid) {
+		case -1:  /* fork failed */
+			return -1;
+		case 0:   /* child */
+			execl(ctx->cfg.authorization_helper, ctx->cfg.authorization_helper, NULL);
+			/* exec failed */
+			exit(-1);
+		default:  /* parent */
+			if (waitpid(pid, &wait_ret, 0) < 0) {
+				return -1; /* waitpid failed */
+			} else if (WIFSIGNALED(wait_ret)) {
+				return -1; /* helper terminated by signal */
+			} else if (WIFEXITED(wait_ret)) {
+				/* helper exited normally */
+				int status = WEXITSTATUS(wait_ret);
+				return status;
+			}
+	}
+	return -1;
+}
+
 static void process_request(void *cbdata)
 {
 	struct cgit_context *ctx = cbdata;
@@ -554,6 +591,15 @@ static void process_request(void *cbdata)
 	if (ctx->repo && prepare_repo_cmd(ctx))
 		return;
 
+	if (ctx->repo && run_authorization_helper(ctx) != 0){
+		cgit_print_http_headers(ctx);
+		cgit_print_docstart(ctx);
+		cgit_print_pageheader(ctx);
+		cgit_print_error(fmt("Authorization failed"));
+		cgit_print_docend();
+		return;
+	}
+
 	if (cmd->want_layout) {
 		cgit_print_http_headers(ctx);
 		cgit_print_docstart(ctx);
diff --git a/cgit.h b/cgit.h
index 7a99135710..5c95c46502 100644
--- a/cgit.h
+++ b/cgit.h
@@ -166,6 +166,7 @@ struct cgit_query {
 
 struct cgit_config {
 	char *agefile;
+	char *authorization_helper;
 	char *cache_root;
 	char *clone_prefix;
 	char *clone_url;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 5887cee9a8..45ea1fe248 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -40,6 +40,12 @@ agefile::
 	function in libgit. Recommended timestamp-format is "yyyy-mm-dd
 	hh:mm:ss". Default value: "info/web/last-modified".
 
+authorization-helper::
+	Specifies a path to authorization executable. The executable is given the
+	full environment from the webserver, including all "CGIT_*" variables
+	available for filters. The executable is expected to return "0" on
+	authorization success. Default value: none. See also: "FILTER API"
+
 cache-root::
 	Path used to store the cgit cache entries. Default value:
 	"/var/cache/cgit". See also: "MACRO EXPANSION".
-- 
1.7.9.5

[PATCHv4 2/2] Helper script to interface to gitolite

[valentin.haenel]

From: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>

Signed-off-by: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
---
 contrib/gl-check-user |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)
 create mode 100755 contrib/gl-check-user

diff --git a/contrib/gl-check-user b/contrib/gl-check-user
new file mode 100755
index 0000000000..45eb95fead
--- /dev/null
+++ b/contrib/gl-check-user
@@ -0,0 +1,20 @@
+#!/bin/sh
+# Wrapper around gitolite to perform
+# repository authentication from a
+# CGI environment
+prog="/usr/local/bin/gitolite"
+
+# HTTPD will not set some essential
+# variables expexted by gitolite
+# Set them here (EUID expected final)
+
+export REPO="${CGIT_REPO_URL%.git}"
+# Get the user from webserver environment.
+# May be either REMOTE_USER or HTTP_X_FORWARDED_USER
+export REMOTE_USER="${HTTP_X_FORWARDED_USER}"
+# Looking for read permission from gitolite
+export PERM="R"
+# Gitolite needs homedir set
+export HOME="$( getent passwd $(id -n -u) | cut -d":" -f 6 )"
+
+exec "$prog" access -q "${REPO}" "${REMOTE_USER}" "${PERM}"
-- 
1.7.9.5

[PATCHv4 2/2] Helper script to interface to gitolite

[jamie.couture]

On Wed, Oct 31, 2012 at 07:52:36PM +0100, Valentin Haenel wrote:
> From: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
> 
> Signed-off-by: Carlos Aguado Sanchez <carlos.aguado at epfl.ch>
> Signed-off-by: Valentin Haenel <valentin.haenel at gmx.de>
> ---
>  contrib/gl-check-user |   20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
>  create mode 100755 contrib/gl-check-user
> 
> diff --git a/contrib/gl-check-user b/contrib/gl-check-user
> new file mode 100755
> index 0000000000..45eb95fead
> --- /dev/null
> +++ b/contrib/gl-check-user
> @@ -0,0 +1,20 @@
> +#!/bin/sh
> +# Wrapper around gitolite to perform
> +# repository authentication from a
> +# CGI environment
> +prog="/usr/local/bin/gitolite"
What about users that have installed gitolite via their distro's
package manager, as opposed to local install?

I do not think the script should assume to know where gitolite lives.

This might be agnostic:
prog="$(which gitolite)"

But that could lead to PATH abuse; potential security problems.

Maybe obtaining the value from another environment variable, say in
your web server's virtual host:

SetEnv GITOLITE_PROG /path/to/gitolite

---

prog="${GITOLITE_PROG}"


Not sure how much of a crutch that is, but I wouldn't want the helper
script to assume where gitolite lives; and if any distro were to
include these scripts people might want it to work out of the box.

> +
> +# HTTPD will not set some essential
> +# variables expexted by gitolite
> +# Set them here (EUID expected final)
> +
> +export REPO="${CGIT_REPO_URL%.git}"
> +# Get the user from webserver environment.
> +# May be either REMOTE_USER or HTTP_X_FORWARDED_USER
> +export REMOTE_USER="${HTTP_X_FORWARDED_USER}"
> +# Looking for read permission from gitolite
> +export PERM="R"
> +# Gitolite needs homedir set
> +export HOME="$( getent passwd $(id -n -u) | cut -d":" -f 6 )"
> +
> +exec "$prog" access -q "${REPO}" "${REMOTE_USER}" "${PERM}"
> -- 
> 1.7.9.5
> 
> 
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit

[PATCHv4 2/2] Helper script to interface to gitolite

[mathstuf]

On Wed, Oct 31, 2012 at 23:03:01 -0400, Jamie Couture wrote:
> What about users that have installed gitolite via their distro's
> package manager, as opposed to local install?

FreeBSD installs to /usr/local, so the path is fine for it.

> I do not think the script should assume to know where gitolite lives.
> 
> This might be agnostic:
> prog="$(which gitolite)"

It could also be configured with $PREFIX as part of the build. Any
hard-coded path is going to be wrong somewhere, so I think this might be
for distros to decide (does cgit even install contrib/ stuff by
default?). However...

> But that could lead to PATH abuse; potential security problems.

if you've injected something into $PATH as the user running the CGI
program, that's likely a system setup misconfiguration and I doubt
everything down that rabbit hole could be protected against ("Oops,
you're using an Apache setup vulnerable to BEAST; let's mitigate...").
If attackers are setting PATH somehow, there are other issues beyond
cgit's scope. Since cgit doesn't (and if it does, that probably needs to
not happen) touch PATH, it therefore shouldn't be responsible for
securing it.

> Maybe obtaining the value from another environment variable, say in
> your web server's virtual host:
> 
> SetEnv GITOLITE_PROG /path/to/gitolite
> 
> ---
> 
> prog="${GITOLITE_PROG}"

Assuming cgit doesn't touch PATH, this is just as valid an attack target
if PATH is an attack vector. Nothing is gained by making another
environment variable.

> Not sure how much of a crutch that is, but I wouldn't want the helper
> script to assume where gitolite lives; and if any distro were to
> include these scripts people might want it to work out of the box.

The solutions I see are:

  - hook it up to the build system (if contrib is already a part of it);
  - use `which`; or
  - ignore it and just have downstreams patch it for the system paths.

I'm in favor of the second one because it's the simplest solution. (I
don't think another patchset would be required if this is the only
thing; a commit on top of these should be fine; --amend should be fine
as well).

--Ben

[PATCHv4 2/2] Helper script to interface to gitolite

[Jason]

1. If PATH is controlled by an attacker, it's already game over, regardless
of this script.
2. Using `which` doesn't make sense, since in a shell script you just call
it by the name, and then it searches path.
3. Gitolite is frequently installed just in a home directory, in the case
of shared hosting, not globally in /usr or /usr/local.
4. So, the best way is just to call gitolite by typing "gitolite"

[PATCHv4 2/2] Helper script to interface to gitolite

[mathstuf]

On Wed, Oct 31, 2012 at 22:20:51 -0600, Jason A. Donenfeld wrote:
> 1. If PATH is controlled by an attacker, it's already game over, regardless
> of this script.
> 2. Using `which` doesn't make sense, since in a shell script you just call
> it by the name, and then it searches path.
> 3. Gitolite is frequently installed just in a home directory, in the case
> of shared hosting, not globally in /usr or /usr/local.
> 4. So, the best way is just to call gitolite by typing "gitolite"

Ah, yeah, `which` isn't necessary. I should get to bed...it's been a
long day.

--Ben

[PATCHv4 2/2] Helper script to interface to gitolite

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-11-01]:
> 1. If PATH is controlled by an attacker, it's already game over, regardless
> of this script.
> 2. Using `which` doesn't make sense, since in a shell script you just call
> it by the name, and then it searches path.
> 3. Gitolite is frequently installed just in a home directory, in the case
> of shared hosting, not globally in /usr or /usr/local.
> 4. So, the best way is just to call gitolite by typing "gitolite"

The intention of the script is to be an example of how things *could* be
done. Depending on how your setup is configured, you need to patch this
script anyway. For example: the REMOTE_USER environment variable must be
matched with how you authenticate in your webserver. Therefore I don't
see any value in trying to make the script as generic as possible. I
could, of course replace the "${prog}" with just gitolite if that's what
people prefer.

V-

[PATCHv4 0/2] Authorize viewing a repository

[valentin.haenel]

* Valentin Haenel <valentin.haenel at gmx.de> [2012-10-31]:
> This is the fourth iteration of the patches to invoke an external helper for
> authorizing viewing of repositories. The significant changes are that now, no
> book-keeping of the REMOTE_USER or equivalent environment variable is done
> within cgit and the authorization helper must instead obtain this from the
> environment. Also, the authorization helper is no longer invoked via 'system',
> but instead using a 'fork/exec/wait' construct. Lastly, the external helper is
> no longer given any arguments, but must get the repository to authorize for,
> from the environment.  To this end, the same "CGIT_*" environment variables
> that are available to the filters are handed over. The cgit man page entry for
> the authorization helper has been updated to describe the environment available
> to the helper and the expected exit code.

One idea I had last night, was to rename 'authorization-helper' into
'authorization-filter'. Although it doesn't take anything on stdin and
returns nothing on stdout, the fact that it has the set of "CGIT_*" env
vars available does make it somewhat like a filter.

Opinions?

V-

[PATCHv4 0/2] Authorize viewing a repository

[Jason]

On Thu, Nov 1, 2012 at 4:40 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> Opinions?

Except it's not a filter. The documentation can just say
"authorization-helper has access to the variables of the Filter API.
See below. Bla bla."

If you don't think of it as a "helper", either, I guess we could call
it an "authorization-authorizer". Or something. Agent? Helper seems
pretty uncontroversial to me.

[PATCHv4 2/2] Helper script to interface to gitolite

[Jason]

On Thu, Nov 1, 2012 at 2:58 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> The intention of the script is to be an example of how things *could* be
> done. Depending on how your setup is configured, you need to patch this
> script anyway.

True that. You definitely don't want to go down the road of writing
auto-detecting shellscript kludges.

[PATCHv4 0/2] Authorize viewing a repository

[valentin.haenel]

* Jason A. Donenfeld <Jason at zx2c4.com> [2012-11-01]:
> On Thu, Nov 1, 2012 at 4:40 AM, Valentin Haenel <valentin.haenel at gmx.de> wrote:
> > Opinions?
> 
> Except it's not a filter. The documentation can just say
> "authorization-helper has access to the variables of the Filter API.
> See below. Bla bla."

Yeah, it does.

> If you don't think of it as a "helper", either, I guess we could call
> it an "authorization-authorizer". Or something. Agent? Helper seems
> pretty uncontroversial to me.

Oki.

V-