[prev in list] [next in list] [prev in thread] [next in thread] 

List:       git
Subject:    Re: [PATCH] Introduce a filter-path argument to git-daemon, for doing
From:       Johan_Sørensen <johan () johansorensen ! com>
Date:       2009-03-12 19:06:25
Message-ID: 9e0f31700903121206m3adbabacra655c5d340365f43 () mail ! gmail ! com
[Download RAW message or body]

On Thu, Mar 12, 2009 at 12:29 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:

(all my comments below refer to my latest patch)

> More importantly, you might want to point out the security concerns of
> running a script with the full permissions of git-daemon.  (AFAICT from
> your patch you are not dropping any privileges at any point.)

Do you really think this is needed? It doesn't seem like running the
hook scripts does anything more than trusting the script author and
permissions of the hook scripts (?). I see the path-filter script
exactly the same way, with the exception of having to double-check the
user supplied path the script receives.

> Which brings me to another idea: we have quite a few places in Git where
> we use regular expressions.  Would they not be enough for your use case?

Hmm, please do elaborate on your idea. If you mean being able to
supply a bunch of regexp mappings when starting the daemon then it
wouldn't cut it for me; I'm in need of something more dynamic.

>> diff --git a/Documentation/git-daemon.txt b/Documentation/git-daemon.txt
>> index 36f00ae..efd1687 100644
>> --- a/Documentation/git-daemon.txt
>> +++ b/Documentation/git-daemon.txt
>> @@ -71,6 +72,18 @@ OPTIONS
[snip]
> Please keep the lines shorter than 81 characters.

I believe the longest line I've added in the docs is 77.

> But there is more: what about concurrent accesses?

The external path-filter script is run from the execute(), which is
forked+exec'ed for each incoming connection to the daemon, so that
would mean a concurrency of one in that child-process, unless I've
missed something in the code path?

>> +     read(filter_cmd.out, result, sizeof(result) - 1);
>
> No error checking?
>
> BTW we do have strbuf_read(), which would solve your "static char *"
> problem nicely.

I'm using strbuf_read() now, but this being my very first git patch, I
may have misunderstood the api slightly?

> And your code would be even easier to read if your
> run_path_filter_script() would never return NULL, but the unchanged path
> instead.

Done.

Thanks for giving my patch the run-through. I'm still curious to hear
what people think about the idea in general though!

>
> Ciao,
> Dscho
>

Cheers,
JS
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
[prev in list] [next in list] [prev in thread] [next in thread]