threads / patch / 21690

patchCVS Server: Support reading base and roots from environment

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

## tl;dr

7 messages between Nov 20, 2009 and Dec 30, 2009. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Phil Miller· Nov 20, 2009, 16:05 UTC · lore

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(-)

Show changes to git-cvsserver.perl +20 −1

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

Junio C Hamano· Nov 20, 2009, 22:42 UTC · re: Phil Miller · lore

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

Phil Miller <mille121@illinois.edu> writes:

Show 20 quoted lines

> 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?

Show 23 quoted lines

>  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.

Show 5 quoted lines

>      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)?

Show 13 quoted lines

> +    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 .

Nanako Shiraishi· Dec 30, 2009, 13:41 UTC · re: Junio C Hamano · lore

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

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

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

Phil Miller· Dec 30, 2009, 17:12 UTC · re: Nanako Shiraishi · lore

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

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

Junio C Hamano· Dec 30, 2009, 19:49 UTC · re: Phil Miller · lore

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

Phil Miller <mille121@illinois.edu> writes:

Show 7 quoted lines

> 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).

Phil Miller· Dec 30, 2009, 20:12 UTC · re: Junio C Hamano · lore

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

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(-)

Show changes to 2 files +36 −1

Documentation/git-cvsserver.txt, git-cvsserver.perl

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

Phil Miller· Dec 30, 2009, 19:35 UTC · re: Junio C Hamano · lore

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.

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(-)

Show changes to 2 files +36 −1

Documentation/git-cvsserver.txt, git-cvsserver.perl

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