[PATCH/resend] CVS Server: Support reading base and roots from environment

[PATCH/resend] CVS Server: Support reading base and roots from environment

[Phil Miller]

The Gitosis single-account Git/ssh hosting system runs git commands
through git-shell after confirming that the connecting user is
authorized to access the requested repository. This works well for
upload-pack and receive-pack, which take a repository argument through
git-shell. This doesn't work so well for `cvs server', which is passed
through literally, with no arguments. Allowing arguments risks
sneaking in `--export-all', so that restriction should be maintained.

Despite that, passing a list of repository roots is necessary for
per-user access control by the hosting software, and passing a base
path improves usability without weakening security. Thus,
git-cvsserver needs to come up with these values at runtime by some
other means. Since git-shell preserves the environment for other
purposes, the environment can carry these arguments as well.

Thus, modify git-cvsserver to read $GIT_CVSSERVER_{BASE_PATH,ROOTS} in
the absence of equivalent command line arguments.

Signed-off-by: Phil Miller <mille121@illinois.edu>
---
 git-cvsserver.perl |   21 ++++++++++++++++++++-
 1 files changed, 20 insertions(+), 1 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 6dc45f5..9e58d2a 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -104,6 +104,7 @@ $log->info("--------------- STARTING -----------------");
 my $usage =
     "Usage: git cvsserver [options] [pserver|server] [<directory> ...]\n".
     "    --base-path <path>  : Prepend to requested CVSROOT\n".
+    "                          Can be read from GIT_CVSSERVER_BASE_PATH\n".
     "    --strict-paths      : Don't allow recursing into subdirectories\n".
     "    --export-all        : Don't check for gitcvs.enabled in config\n".
     "    --version, -V       : Print version information and exit\n".
@@ -111,7 +112,8 @@ my $usage =
     "\n".
     "<directory> ... is a list of allowed directories. If no directories\n".
     "are given, all are allowed. This is an additional restriction, gitcvs\n".
-    "access still needs to be enabled by the gitcvs.enabled config option.\n";
+    "access still needs to be enabled by the gitcvs.enabled config option.\n".
+    "Alternately, these directories may be specified in
GIT_CVSSERVER_ROOTS.\n";

 my @opts = ( 'help|h|H', 'version|V',
 	     'base-path=s', 'strict-paths', 'export-all' );
@@ -148,6 +150,23 @@ if ($state->{'export-all'} &&
!@{$state->{allowed_roots}}) {
     die "--export-all can only be used together with an explicit whitelist\n";
 }

+# Environment handling for running under git-shell
+if ($ENV{GIT_CVSSERVER_BASE_PATH}) {
+    if ($state->{'base-path'}) {
+	die "Cannot specify base path both ways.\n";
+    }
+    my $base_path = $ENV{GIT_CVSSERVER_BASE_PATH};
+    $state->{'base-path'} = $base_path;
+    $log->debug("Picked up base path '$base_path' from environment.\n");
+}
+if ($ENV{GIT_CVSSERVER_ROOTS}) {
+    if (@{$state->{allowed_roots}}) {
+	die "Cannot specify roots both ways: @ARGV\n";
+    }
+    my @allowed_root = split(',', $ENV{GIT_CVSSERVER_ROOTS});
+    $state->{allowed_roots} = [ @allowed_root ];
+}
+
 # if we are called with a pserver argument,
 # deal with the authentication cat before entering the
 # main loop
-- 
1.5.6.3

Re: [PATCH/resend] CVS Server: Support reading base and roots from environment

[Junio C Hamano]

Phil Miller <mille121@illinois.edu> writes:

> The Gitosis single-account Git/ssh hosting system runs git commands
> through git-shell after confirming that the connecting user is
> authorized to access the requested repository. This works well for
> upload-pack and receive-pack, which take a repository argument through
> git-shell. This doesn't work so well for `cvs server', which is passed
> through literally, with no arguments. Allowing arguments risks
> sneaking in `--export-all', so that restriction should be maintained.
>
> Despite that, passing a list of repository roots is necessary for
> per-user access control by the hosting software, and passing a base
> path improves usability without weakening security. Thus,
> git-cvsserver needs to come up with these values at runtime by some
> other means. Since git-shell preserves the environment for other
> purposes, the environment can carry these arguments as well.
>
> Thus, modify git-cvsserver to read $GIT_CVSSERVER_{BASE_PATH,ROOTS} in
> the absence of equivalent command line arguments.
>
> Signed-off-by: Phil Miller <mille121@illinois.edu>
> ---

Thanks.

Any comments from cvsserver users?

>  git-cvsserver.perl |   21 ++++++++++++++++++++-
>  1 files changed, 20 insertions(+), 1 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index 6dc45f5..9e58d2a 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -104,6 +104,7 @@ $log->info("--------------- STARTING -----------------");
>  my $usage =
>      "Usage: git cvsserver [options] [pserver|server] [<directory> ...]\n".
>      "    --base-path <path>  : Prepend to requested CVSROOT\n".
> +    "                          Can be read from GIT_CVSSERVER_BASE_PATH\n".
>      "    --strict-paths      : Don't allow recursing into subdirectories\n".
>      "    --export-all        : Don't check for gitcvs.enabled in config\n".
>      "    --version, -V       : Print version information and exit\n".
> @@ -111,7 +112,8 @@ my $usage =
>      "\n".
>      "<directory> ... is a list of allowed directories. If no directories\n".
>      "are given, all are allowed. This is an additional restriction, gitcvs\n".
> -    "access still needs to be enabled by the gitcvs.enabled config option.\n";
> +    "access still needs to be enabled by the gitcvs.enabled config option.\n".
> +    "Alternately, these directories may be specified in
> GIT_CVSSERVER_ROOTS.\n";

When you introduce a single variable holding multiple values, you need to
document how to cram the values into it.  Maybe such a detail isn't
necessary in this usage text, but definitely in the documentation.

Documentation/git-cvsserver.txt needs to be updated to describe the
features added by this patch.

By the way, this patch is line-wrapped.  Here, and...

>  my @opts = ( 'help|h|H', 'version|V',
>  	     'base-path=s', 'strict-paths', 'export-all' );
> @@ -148,6 +150,23 @@ if ($state->{'export-all'} &&
> !@{$state->{allowed_roots}}) {

... also here.

>      die "--export-all can only be used together with an explicit whitelist\n";
>  }
>
> +# Environment handling for running under git-shell
> +if ($ENV{GIT_CVSSERVER_BASE_PATH}) {

It probably is more kosher to say

	if (exists $ENV{...})

as the base_path _could_ be '0' that evaluates to false.  When the path
does not begin with a '/', what will it be relative to?  Do we want to
document it (not a rhetorical question)?

> +    if ($state->{'base-path'}) {
> +	die "Cannot specify base path both ways.\n";
> +    }
> +    my $base_path = $ENV{GIT_CVSSERVER_BASE_PATH};
> +    $state->{'base-path'} = $base_path;
> +    $log->debug("Picked up base path '$base_path' from environment.\n");
> +}
> +if ($ENV{GIT_CVSSERVER_ROOTS}) {
> +    if (@{$state->{allowed_roots}}) {
> +	die "Cannot specify roots both ways: @ARGV\n";
> +    }
> +    my @allowed_root = split(',', $ENV{GIT_CVSSERVER_ROOTS});
> +    $state->{allowed_roots} = [ @allowed_root ];

Even though a comma is probably rare in pathname components, I do not know
if this is good.

How much thought went into choosing comma for this purpose?  Is there a
particular reason you chose ',' as the separator?  That should be
documented in the commit log message.

Logical alternative choices are "split at whitespace" (mimics the way how
command line argument splitting works) and "colon" (mimics the way how
$PATH is split into component paths).

If a pathname component with whitespaces (Windows and Macs?) is not an
issue, I would imagine "split at whitespace" is much more natural way to
handle this, but probably many people have "My Programs" and such.

Especially because it is hard, if not impossible, to have a pathname
component that contains a colon on Windows, I suspect that a colon is much
rare compared to whitespaces and commas in the name of people's
directories and files.  It might be better to split at colon like $PATH is
handled than using a comma, if you are not going to give any escape
mechanism to .

Re: [PATCH/resend] CVS Server: Support reading base and roots from environment

[Nanako Shiraishi]

Junio, could you tell us what happened to this thread?

Phil Miller's patch to help gitosis installation.  No response from
git-cvsserver users.

Re: [PATCH/resend] CVS Server: Support reading base and roots from environment

[Phil Miller]

On Wed, Dec 30, 2009 at 07:41, Nanako Shiraishi <nanako3@lavabit.com> wrote:
> Junio, could you tell us what happened to this thread?
>
> Phil Miller's patch to help gitosis installation.  No response from
> git-cvsserver users.

I never resent with modifications as Junio requested. I've been busy
with some end-of-year close-out stuff, and actually wanted to
dramatically simplify part of the patch before the resend. I'll do
that shortly.

Since Gitosis wants to do repository-level access control before
running any Git commands, I figured out a CVSROOT format that would
let it do this. Given that, there's only one repository that an
invocation of git-cvsserver should be allowed to access, and so the
issue of "list separator" and so forth is moot. The resultant CVS
command line looks something like

cvs -d ":ext;CVS_SERVER=cvs
'/path/to/module':charmgit/path/to/module.git" co -d module

Note that I have had to modify Gitosis to make this, and various other
things, work. I had wanted to clean up my work before announcing it,
but if it would help you now, it is publicly available from github
now:

git://github.com/PhilMiller/gitosis.git

The documentation hasn't been updated, but the recent history makes as
least some usage clear. I'll send more details in a follow-up.

Phil

[PATCH/resend] CVS Server: Support reading base and roots from environment

[Phil Miller]

The Gitosis single-account Git/ssh hosting system runs git commands
through git-shell after confirming that the connecting user is
authorized to access the requested repository. This works well for
upload-pack and receive-pack, which take a repository argument through
git-shell. This doesn't work so well for `cvs server', which is passed
through literally, with no arguments. Allowing arguments risks
sneaking in `--export-all', so that restriction should be maintained.

Despite that, passing a repository root is necessary for per-user
access control by the hosting software, and passing a base path
improves usability without weakening security. Thus, git-cvsserver
needs to come up with these values at runtime by some other
means. Since git-shell preserves the environment for other purposes,
the environment can carry these arguments as well.

Thus, modify git-cvsserver to read $GIT_CVSSERVER_{BASE_PATH,ROOT} in
the absence of equivalent command line arguments.

Signed-off-by: Phil Miller <mille121@illinois.edu>
---
I believe this revision addresses all of your comments on the first submission.

Your comment about cramming multiple values into one environment variable made
me realize that more than one simply was unnecessary complexity, since gitosis
expects to authenticate access to a single repository anyway.

I've not documented what GIT_CVSSERVER_BASE_PATH is relative to, because it
behaves identically to the --base-path command line argument. Documenting
what that is relative to is a separate issue.

 Documentation/git-cvsserver.txt |   15 +++++++++++++++
 git-cvsserver.perl              |   22 +++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index 99a7c14..fbab295 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -277,6 +277,21 @@ In `dbdriver` and `dbuser` you can use the following variables:
 	If no name can be determined, the
 	numeric uid is used.
 
+ENVIRONMENT
+-----------
+
+These variables obviate the need for command-line options in some
+circumstances, allowing easier restricted usage through git-shell.
+
+GIT_CVSSERVER_BASE_PATH takes the place of the argument to --base-path.
+
+GIT_CVSSERVER_ROOT specifies a single-directory whitelist. The
+repository must still be configured to allow access through
+git-cvsserver, as described above.
+
+When these environment variables are set, the corresponding
+command-line arguments may not be used.
+
 Eclipse CVS Client Notes
 ------------------------
 
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 6dc45f5..f5b57b9 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -104,6 +104,7 @@ $log->info("--------------- STARTING -----------------");
 my $usage =
     "Usage: git cvsserver [options] [pserver|server] [<directory> ...]\n".
     "    --base-path <path>  : Prepend to requested CVSROOT\n".
+    "                          Can be read from GIT_CVSSERVER_BASE_PATH\n".
     "    --strict-paths      : Don't allow recursing into subdirectories\n".
     "    --export-all        : Don't check for gitcvs.enabled in config\n".
     "    --version, -V       : Print version information and exit\n".
@@ -111,7 +112,8 @@ my $usage =
     "\n".
     "<directory> ... is a list of allowed directories. If no directories\n".
     "are given, all are allowed. This is an additional restriction, gitcvs\n".
-    "access still needs to be enabled by the gitcvs.enabled config option.\n";
+    "access still needs to be enabled by the gitcvs.enabled config option.\n".
+    "Alternately, one directory may be specified in GIT_CVSSERVER_ROOT.\n";
 
 my @opts = ( 'help|h|H', 'version|V',
 	     'base-path=s', 'strict-paths', 'export-all' );
@@ -148,6 +150,24 @@ if ($state->{'export-all'} && !@{$state->{allowed_roots}}) {
     die "--export-all can only be used together with an explicit whitelist\n";
 }
 
+# Environment handling for running under git-shell
+if (exists $ENV{GIT_CVSSERVER_BASE_PATH}) {
+    if ($state->{'base-path'}) {
+	die "Cannot specify base path both ways.\n";
+    }
+    my $base_path = $ENV{GIT_CVSSERVER_BASE_PATH};
+    $state->{'base-path'} = $base_path;
+    $log->debug("Picked up base path '$base_path' from environment.\n");
+}
+if (exists $ENV{GIT_CVSSERVER_ROOT}) {
+    if (@{$state->{allowed_roots}}) {
+	die "Cannot specify roots both ways: @ARGV\n";
+    }
+    my $allowed_root = $ENV{GIT_CVSSERVER_ROOT};
+    $state->{allowed_roots} = [ $allowed_root ];
+    $log->debug("Picked up allowed root '$allowed_root' from environment.\n");
+}
+
 # if we are called with a pserver argument,
 # deal with the authentication cat before entering the
 # main loop
-- 
debian.1.6.6_rc2.1.7.gc3ed7

Re: [PATCH/resend] CVS Server: Support reading base and roots from environment

[Junio C Hamano]

Phil Miller <mille121@illinois.edu> writes:

> I never resent with modifications as Junio requested. I've been busy
> with some end-of-year close-out stuff, and actually wanted to
> dramatically simplify part of the patch before the resend. I'll do
> that shortly.
> ...
> The documentation hasn't been updated, but the recent history makes as
> least some usage clear. I'll send more details in a follow-up.

Thanks Phil for status updates (and thanks Nana for prodding).

Re: [PATCH/resend] CVS Server: Support reading base and roots from environment

[Phil Miller]

The Gitosis single-account Git/ssh hosting system runs git commands
through git-shell after confirming that the connecting user is
authorized to access the requested repository. This works well for
upload-pack and receive-pack, which take a repository argument through
git-shell. This doesn't work so well for `cvs server', which is passed
through literally, with no arguments. Allowing arguments risks
sneaking in `--export-all', so that restriction should be maintained.

Despite that, passing a repository root is necessary for per-user
access control by the hosting software, and passing a base path
improves usability without weakening security. Thus, git-cvsserver
needs to come up with these values at runtime by some other
means. Since git-shell preserves the environment for other purposes,
the environment can carry these arguments as well.

Thus, modify git-cvsserver to read $GIT_CVSSERVER_{BASE_PATH,ROOT} in
the absence of equivalent command line arguments.

Signed-off-by: Phil Miller <mille121@illinois.edu>
---
I believe this revision addresses all of your comments on the first submission.

Your comment about cramming multiple values into one environment variable made
me realize that more than one simply was unnecessary complexity, since gitosis
expects to authenticate access to a single repository anyway.

I've not documented what GIT_CVSSERVER_BASE_PATH is relative to, because it
behaves identically to the --base-path command line argument. Documenting
what that is relative to is a separate issue.

 Documentation/git-cvsserver.txt |   15 +++++++++++++++
 git-cvsserver.perl              |   22 +++++++++++++++++++++-
 2 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/Documentation/git-cvsserver.txt b/Documentation/git-cvsserver.txt
index 99a7c14..fbab295 100644
--- a/Documentation/git-cvsserver.txt
+++ b/Documentation/git-cvsserver.txt
@@ -277,6 +277,21 @@ In `dbdriver` and `dbuser` you can use the
following variables:
 	If no name can be determined, the
 	numeric uid is used.

+ENVIRONMENT
+-----------
+
+These variables obviate the need for command-line options in some
+circumstances, allowing easier restricted usage through git-shell.
+
+GIT_CVSSERVER_BASE_PATH takes the place of the argument to --base-path.
+
+GIT_CVSSERVER_ROOT specifies a single-directory whitelist. The
+repository must still be configured to allow access through
+git-cvsserver, as described above.
+
+When these environment variables are set, the corresponding
+command-line arguments may not be used.
+
 Eclipse CVS Client Notes
 ------------------------

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 6dc45f5..f5b57b9 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -104,6 +104,7 @@ $log->info("--------------- STARTING -----------------");
 my $usage =
     "Usage: git cvsserver [options] [pserver|server] [<directory> ...]\n".
     "    --base-path <path>  : Prepend to requested CVSROOT\n".
+    "                          Can be read from GIT_CVSSERVER_BASE_PATH\n".
     "    --strict-paths      : Don't allow recursing into subdirectories\n".
     "    --export-all        : Don't check for gitcvs.enabled in config\n".
     "    --version, -V       : Print version information and exit\n".
@@ -111,7 +112,8 @@ my $usage =
     "\n".
     "<directory> ... is a list of allowed directories. If no directories\n".
     "are given, all are allowed. This is an additional restriction, gitcvs\n".
-    "access still needs to be enabled by the gitcvs.enabled config option.\n";
+    "access still needs to be enabled by the gitcvs.enabled config option.\n".
+    "Alternately, one directory may be specified in GIT_CVSSERVER_ROOT.\n";

 my @opts = ( 'help|h|H', 'version|V',
 	     'base-path=s', 'strict-paths', 'export-all' );
@@ -148,6 +150,24 @@ if ($state->{'export-all'} &&
!@{$state->{allowed_roots}}) {
     die "--export-all can only be used together with an explicit whitelist\n";
 }

+# Environment handling for running under git-shell
+if (exists $ENV{GIT_CVSSERVER_BASE_PATH}) {
+    if ($state->{'base-path'}) {
+	die "Cannot specify base path both ways.\n";
+    }
+    my $base_path = $ENV{GIT_CVSSERVER_BASE_PATH};
+    $state->{'base-path'} = $base_path;
+    $log->debug("Picked up base path '$base_path' from environment.\n");
+}
+if (exists $ENV{GIT_CVSSERVER_ROOT}) {
+    if (@{$state->{allowed_roots}}) {
+	die "Cannot specify roots both ways: @ARGV\n";
+    }
+    my $allowed_root = $ENV{GIT_CVSSERVER_ROOT};
+    $state->{allowed_roots} = [ $allowed_root ];
+    $log->debug("Picked up allowed root '$allowed_root' from environment.\n");
+}
+
 # if we are called with a pserver argument,
 # deal with the authentication cat before entering the
 # main loop
-- 
debian.1.6.6_rc2.1.7.gc3ed7