what are the chances of a 'pre-upload' hook?

what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

(...and/or a post-upload hook)

Has this ever come up?

-- 
Sitaram

Re: what are the chances of a 'pre-upload' hook?

[Martin Fick]

Sitaram Chamarty <sitaramc@gmail.com> wrote:

>(...and/or a post-upload hook)
>
>Has this ever come up?
>

 Yes, previously it was seen as problematic (potentially too slow), but we are now open to having one.  Patches welcome.  :)

Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum

Re: what are the chances of a 'pre-upload' hook?

[Martin Fick]

>Martin Fick <mfick@codeaurora.org> wrote:
>Sitaram Chamarty <sitaramc@gmail.com> wrote:
>
>>(...and/or a post-upload hook)
>>
>>Has this ever come up?
>
>Yes, previously it was seen as problematic (potentially too slow), but
>we are now open to having one.  Patches welcome.  :)


Doh sorry, I thought this was a Gerrit question!  (Wrong mailing list)

Employee of Qualcomm Innovation Center,Inc. which is a member of Code Aurora Forum

Re: what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

On Fri, Nov 25, 2011 at 8:46 AM, Sitaram Chamarty <sitaramc@gmail.com> wrote:
> (...and/or a post-upload hook)
>
> Has this ever come up?

Sorry for the google-fu fail and for replying to my own post.
http://git.661346.n2.nabble.com/Removal-of-post-upload-hook-td4394312.html
is the longest thread I (later) found.

The quick summary, in the words of Jeff (second post in that link) is:
"Because [upload]-pack runs as the user who is [fetching], not as the
repository owner. So by convincing you to [fetch from] my repository
in a multi-user environment, I convince you to run some arbitrary code
of mine."

My contention (today) is:

  - you're already taking that risk for push
  - so this would add a new risk only for people who fetch but don't push
  - which, I submit, is a very small (if not almost empty) set of people

I may be wrong but I imagine shared environments are those where
almost everyone will push at least once in a while.  It's a closed
group of people, probably all developers, etc etc etc...

Thanks for listening.

Re: what are the chances of a 'pre-upload' hook?

[Andreas Ericsson]

On 11/25/2011 05:13 AM, Sitaram Chamarty wrote:
> On Fri, Nov 25, 2011 at 8:46 AM, Sitaram Chamarty<sitaramc@gmail.com>  wrote:
>> (...and/or a post-upload hook)
>>
>> Has this ever come up?
> 
> Sorry for the google-fu fail and for replying to my own post.
> http://git.661346.n2.nabble.com/Removal-of-post-upload-hook-td4394312.html
> is the longest thread I (later) found.
> 
> The quick summary, in the words of Jeff (second post in that link) is:
> "Because [upload]-pack runs as the user who is [fetching], not as the
> repository owner. So by convincing you to [fetch from] my repository
> in a multi-user environment, I convince you to run some arbitrary code
> of mine."
> 
> My contention (today) is:
> 
>    - you're already taking that risk for push
>    - so this would add a new risk only for people who fetch but don't push
>    - which, I submit, is a very small (if not almost empty) set of people
> 

People who fetch but don't push is, by far, the vast majority of git users.
Think of everyone fetching from any public software repository without
having write access to it. If you think of git.git and linux.git alone
I think it's safe to assume the number of "fetch-no-push" outnumber the
"push-and-whatnot" group by some quarter million to one.

> I may be wrong but I imagine shared environments are those where
> almost everyone will push at least once in a while.  It's a closed
> group of people, probably all developers, etc etc etc...
> 

Not really. We fetch from each other quite a lot at work, and from
each others semi-public repositories on a shared server where we've
all got accounts (ie, write access), but we very, very rarely push
into each others repositories. The sharepoint is the "official" repo
on the repo-server, which the buildbots gets its code from and where
everything to be released, maintained or handled in some way in the
future resides.

Anyways. Shooting down the arguments *against* pre-upload hooks are
quite silly if it's not combined with some fresh arguments *for* such
a hook.

So... What usecase do you envision where you'd need one?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.

Re: what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

On Fri, Nov 25, 2011 at 6:39 PM, Andreas Ericsson <ae@op5.se> wrote:

> People who fetch but don't push is, by far, the vast majority of git users.
> Think of everyone fetching from any public software repository without
> having write access to it. If you think of git.git and linux.git alone
> I think it's safe to assume the number of "fetch-no-push" outnumber the
> "push-and-whatnot" group by some quarter million to one.

But in those environments the person pulling does not even have an ID
so how is he at risk with the hook running?

>> I may be wrong but I imagine shared environments are those where
>> almost everyone will push at least once in a while.  It's a closed
>> group of people, probably all developers, etc etc etc...
>>
>
> Not really. We fetch from each other quite a lot at work, and from
> each others semi-public repositories on a shared server where we've
> all got accounts (ie, write access), but we very, very rarely push
> into each others repositories. The sharepoint is the "official" repo
> on the repo-server, which the buildbots gets its code from and where
> everything to be released, maintained or handled in some way in the
> future resides.

Yes, and this is the only situation where it does have the issue.  I'm
just not sure how common this is.

It's fine if you tell me I'm wrong and that this *is* still very
common.  I'll back off.

But everyone seems to be bringing in github and public repos as part
of the argument, and I don't see how they're relevant to the original
security issue of the guy who pulls having his account compromised.

> Anyways. Shooting down the arguments *against* pre-upload hooks are
> quite silly if it's not combined with some fresh arguments *for* such
> a hook.
>
> So... What usecase do you envision where you'd need one?

I'm writing a caching proxy that helps with bandwidth issues when too
many people in a bad-WAN site want to clone some huge repo from its
canonical site.  The only one I found by googling fiddles with the git
protocol itself, and I hate doing things like that.

Ignoring all the details, the pre-upload hook would have checked some
conditions and fired off a fetch from the remote site if those checks
passed.

It's easy enough to do it from cron but it would have been more
elegant this way.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Fri, Nov 25, 2011 at 09:43:14AM +0530, Sitaram Chamarty wrote:

> The quick summary, in the words of Jeff (second post in that link) is:
> "Because [upload]-pack runs as the user who is [fetching], not as the
> repository owner. So by convincing you to [fetch from] my repository
> in a multi-user environment, I convince you to run some arbitrary code
> of mine."
> 
> My contention (today) is:
> 
>   - you're already taking that risk for push
>   - so this would add a new risk only for people who fetch but don't push
>   - which, I submit, is a very small (if not almost empty) set of people
> 
> I may be wrong but I imagine shared environments are those where
> almost everyone will push at least once in a while.  It's a closed
> group of people, probably all developers, etc etc etc...

One thing to note is that this is _only_ an issue on systems where the
user running upload-pack (or receive-pack, for push) is not the same as
the user who owns the hooks directory. So basically two situations:

  1. Alice and Bob are developers on a multi-user system with ssh
     access. Alice will run "git fetch ~bob/project.git" or "git fetch
     alice@server:~bob/project.git". She executes Bob's hooks as
     herself on the server.

  2. Somebody runs a git:// server, locked down under a "git" user (or a
     smart-http server, under a "www" account).  If users of the service
     can write their own hooks, they will run as the "git" user.

But there are many situations that don't fall under this umbrella. In
(1), maybe Alice and Bob decide that they trust each other enough, and
the hook is more important than the security issue. In (2), users of the
service might not be able to write their own hooks (this is the case for
GitHub, and I assume also for Gerrit).

There should be a way for people who aren't in one of those dangerous
situations to be able to use a hook. The important thing is to set the
defaults in such a way that the people who _are_ in that situation
aren't left in a vulnerable situation.

The easiest way would be something like a "trust remote hooks"
environment variable, off by default. Admins in situation (2) could set
it for their git-daemon (or webserver, or whatever, or
gitolite-over-ssh), once they decided it was safe.

It could also help with (1), but I think you would need to take special
steps to propagate the variable in the "git fetch server:project.git"
case.

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The easiest way would be something like a "trust remote hooks"
> environment variable, off by default. Admins in situation (2) could set
> it for their git-daemon (or webserver, or whatever, or
> gitolite-over-ssh), once they decided it was safe.

Alice and Bob may work on the same project, and they may want to trust
each other as participants of that project, but that does not mean Alice
wants to give Bob a blanket access to places she owns that are not shared
with the project members (e.g. her $HOME directory), so I am afraid "trust
remote hooks" is not a workable solution for the casual sharing on sites
that do not fall into either of your two classes.

The real reason why the upload-hook violates the expectation of the users
is because it would run as the user who fetches, I think. If it ran with
the intersection of capabilities of the owner of the repository and the
user who is fetching, I suspect that we would not have to worry about it.

What would happen if we allowed some hooks to run only when the process is
running under a group-id that can write into the repository?  When Alice
fetches from the repository, it would still run as her and would have an
access to her $HOME, so this won't really work yet, but I am wondering if
there is a workable alternative along this line.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sat, Nov 26, 2011 at 02:34:53PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The easiest way would be something like a "trust remote hooks"
> > environment variable, off by default. Admins in situation (2) could set
> > it for their git-daemon (or webserver, or whatever, or
> > gitolite-over-ssh), once they decided it was safe.
> 
> Alice and Bob may work on the same project, and they may want to trust
> each other as participants of that project, but that does not mean Alice
> wants to give Bob a blanket access to places she owns that are not shared
> with the project members (e.g. her $HOME directory), so I am afraid "trust
> remote hooks" is not a workable solution for the casual sharing on sites
> that do not fall into either of your two classes.

Of course. My point isn't that such a feature would cover all cases, but
that it would give people a tool to make that decision for themselves,
instead of blanket-forbidding it.

> The real reason why the upload-hook violates the expectation of the users
> is because it would run as the user who fetches, I think. If it ran with
> the intersection of capabilities of the owner of the repository and the
> user who is fetching, I suspect that we would not have to worry about it.

Sure. And I think we would probably trust automatically a hook that is
owned by the executing uid. Or possibly root, though that could be
surprising in NFS root-squashed environments.

> What would happen if we allowed some hooks to run only when the process is
> running under a group-id that can write into the repository?  When Alice
> fetches from the repository, it would still run as her and would have an
> access to her $HOME, so this won't really work yet, but I am wondering if
> there is a workable alternative along this line.

I don't think so. It comes down to this: Alice is executing arbitrary
code from Bob's repository, which Bob (and maybe others) have access to
write. This is giving Bob permission to run arbitrary code as Alice's
user.

Checking things like group access doesn't matter. If Alice is in the
group, too, it doesn't make a difference; Bob can still write the files,
and Alice is still running the code under her UID.

I think the only solutions are:

  1. Alice accepts that she can trust Bob enough to run his arbitrary
     code.

  2. We use some technique to run the code as the repo owner's UID.

The usual methods for doing (2) are:

  a. setuid, though doing it right can be quite tricky (e.g., cleansing
     environment, file descriptors, etc). And it requires root
     cooperation.

  b. running a server with a socket as Bob, and triggering the hook code
     from the server

Bob could run a specialized server for (b) that listens on a unix socket
and triggers his hook. But why? Why not just do the whole thing over
git-daemon or smart http, which already exist?

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Bob could run a specialized server for (b) that listens on a unix socket
> and triggers his hook. But why? Why not just do the whole thing over
> git-daemon or smart http, which already exist?

If that "whole thing" is "to allow an arbitrary code to run anywhere as
incoming user", I would apply the "why?" to a different part of the
statemennt. Why allow running an arbitrary code at all?

Running things as Bob with setuid is not a solution, either.

A pre-anything hook wants to see if the accessing user, not the owner of
the repository, can or cannot do something to the repository and decide
what to do.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sat, Nov 26, 2011 at 03:13:38PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Bob could run a specialized server for (b) that listens on a unix socket
> > and triggers his hook. But why? Why not just do the whole thing over
> > git-daemon or smart http, which already exist?
> 
> If that "whole thing" is "to allow an arbitrary code to run anywhere as
> incoming user", I would apply the "why?" to a different part of the
> statemennt. Why allow running an arbitrary code at all?

Because there are useful hooks that Bob wants to run when somebody
fetches or pushes to/from his repository?

> A pre-anything hook wants to see if the accessing user, not the owner of
> the repository, can or cannot do something to the repository and decide
> what to do.

It depends on what you want your hook to do. I thought Sitaram's
use-case was putting something like "git fetch upstream" into a hook
that runs before upload-pack, to create a transparent proxy. That has
nothing to do with the accessing user.

At GitHub, we run a pre-upload-pack hook to keep statistics. That has
nothing to do with the accessing user. We have to patch git to provide
the hook.

And even if you _did_ want to know something about the accessing user,
that doesn't mean you necessarily can or want to be running code as
them. If I'm running a smart-http server, I might have verified the user
already via cookie, client cert, or basic auth. That information could
be passed down to a pre-upload-pack hook where it could make a decision.

But we don't _have_ a pre-upload-pack hook, because it can be dangerous
in some situations. My point is to make it available, give it safe
semantics by default, and let people who are running daemon-like service
(i.e., where the admin controls the daemon and arbitrary users can't
write into the hooks directory) use it by setting an environment
variable, rather than patching git.

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sat, Nov 26, 2011 at 03:47:09PM -0800, Junio C Hamano wrote:

> > My point is to make it available, give it safe
> > semantics by default, and let people who are running daemon-like service
> > (i.e., where the admin controls the daemon and arbitrary users can't
> > write into the hooks directory) use it by setting an environment
> > variable, rather than patching git.
> 
> I think we re on the same page on that point, and this thread is to find
> such a safe default and safe semantics when enabled.
> 
> Unfortunately neither your "trusted" switch nor check the gid of repository
> is that safe thing (sane default part is easy; do not allow it by default).

Sorry, why is the trusted switch not a sane thing? By turning it on, you
are saying "it's OK to run arbitrary code from the repo as the current
user". It's _exactly_ what some people are going to want to do[1],
regardless of any other heuristics.

Sure, maybe it's giving people rope to hang themselves with, but I don't
see a problem with that as long as the issues are clearly laid out in
the documentation.

-Peff

[1] An alternate and even more flexible form is to not just say "it's OK
to run hooks", but to say "run this particular hook as a
pre-upload-hook" without any regard for what's in $GIT_DIR/hooks. It is
a superset of the other form, because of course the hook you tell it
to run can be "sh $GIT_DIR/hooks/pre-upload-pack".

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sat, Nov 26, 2011 at 03:57:40PM -0800, Junio C Hamano wrote:

> Did I say anything about saNe?. I was talking about saFe.

Fine. But that doesn't change my point: the purpose of such a feature is
to tell git "do _not_ be safe; I have decided already for you whether it
is OK to do this".

> > By turning it on, you
> > are saying "it's OK to run arbitrary code from the repo as the current
> > user".
> 
> The problem I have with it is that you are saying much more than that.
> ... as the current user ANYWHERE on the machine.

Just because it is passed through the environment does not mean you need
to have it set all the time. There is nothing wrong with:

  GIT_ALLOW_UNTRUSTED_HOOKS=true git fetch ~bob/repo.git

We can even spell it:

  git --allow-untrusted-hooks fetch ~bob/repo.git

but it should probably still end up as an environment variable to make
it through to the remote side (you could also tack it on to the
upload-pack command line; that wouldn't make it across git:// or http://
connections, but those are irrelevant here anyway).

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Just because it is passed through the environment does not mean you need
> to have it set all the time. There is nothing wrong with:
>
>   GIT_ALLOW_UNTRUSTED_HOOKS=true git fetch ~bob/repo.git
>
> We can even spell it:
>
>   git --allow-untrusted-hooks fetch ~bob/repo.git
>
> but it should probably still end up as an environment variable to make
> it through to the remote side (you could also tack it on to the
> upload-pack command line; that wouldn't make it across git:// or http://
> connections, but those are irrelevant here anyway).

I actually like the idea of allowing pre-upload-pack hook on git:// and
possibly http:// only. git-daemon can tell the upload-pack that it is OK
to run the hook, and the hook can do the things that only the daemon can
do, never touching what the original requestor would but the repository
owner would not have an access to. Normal file:// and ssh:// transfer
should not be needed for GitHub's stats or Sitaram's proxy usage, and we
should be able to unconditionally disable the hook when transfer is going
over these protocols, no?

One scenario I do not want to see is this. Suppose there is a company that
runs more than one project, and one of the projects is so secret that only
engineers working on the project have access to its repository, while all
people, including the project-A engineers, in the company have access to
other repositories. Further suppose that people involved in some or many
of the general repositories want to do something before a fetch from them
happens. They will use the pre-upload-hook to implement it if we make it
available, and their new-hire training course will tell their engineers to
set the GIT_ALLOW_UNTRUSTED_HOOKS. Perhaps the /etc/skel/profile the
company gives the new-hires already defines it.

And somebody who controls one of the general repositories installs a
pre-upload-hook that borrows credential of a project-A engineer who
happens to fetch from it to access repositories of the project-A.

Imagine how the blame game that will ensue goes after materials from
project-A is exposed. Of course, the owner of the rogue hook will get the
first blame, but I do not think Git will come unscathed out of it. At
least we will be blamed for adding an inherently unsafe mechanism.

Re: what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

On Sun, Nov 27, 2011 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Just because it is passed through the environment does not mean you need
>> to have it set all the time. There is nothing wrong with:
>>
>>   GIT_ALLOW_UNTRUSTED_HOOKS=true git fetch ~bob/repo.git
>>
>> We can even spell it:
>>
>>   git --allow-untrusted-hooks fetch ~bob/repo.git
>>
>> but it should probably still end up as an environment variable to make
>> it through to the remote side (you could also tack it on to the
>> upload-pack command line; that wouldn't make it across git:// or http://
>> connections, but those are irrelevant here anyway).
>
> I actually like the idea of allowing pre-upload-pack hook on git:// and
> possibly http:// only. git-daemon can tell the upload-pack that it is OK
> to run the hook, and the hook can do the things that only the daemon can
> do, never touching what the original requestor would but the repository
> owner would not have an access to. Normal file:// and ssh:// transfer
> should not be needed for GitHub's stats or Sitaram's proxy usage, and we
> should be able to unconditionally disable the hook when transfer is going
> over these protocols, no?
>
> One scenario I do not want to see is this. Suppose there is a company that
> runs more than one project, and one of the projects is so secret that only
> engineers working on the project have access to its repository, while all
> people, including the project-A engineers, in the company have access to
> other repositories. Further suppose that people involved in some or many
> of the general repositories want to do something before a fetch from them
> happens. They will use the pre-upload-hook to implement it if we make it
> available, and their new-hire training course will tell their engineers to
> set the GIT_ALLOW_UNTRUSTED_HOOKS. Perhaps the /etc/skel/profile the
> company gives the new-hires already defines it.
>
> And somebody who controls one of the general repositories installs a
> pre-upload-hook that borrows credential of a project-A engineer who
> happens to fetch from it to access repositories of the project-A.
>
> Imagine how the blame game that will ensue goes after materials from
> project-A is exposed. Of course, the owner of the rogue hook will get the
> first blame, but I do not think Git will come unscathed out of it. At
> least we will be blamed for adding an inherently unsafe mechanism.
>

I'm sorry I started this discussion.  I worked around it, though it's
a bit kludgy, so maybe time to drop the debate.

-- 
Sitaram

Re: what are the chances of a 'pre-upload' hook?

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

>> I actually like the idea of allowing pre-upload-pack hook on git:// and
>> possibly http:// only....
>>
>> One scenario I do not want to see is this. Suppose ...
>
> I'm sorry I started this discussion.  I worked around it, though it's
> a bit kludgy, so maybe time to drop the debate.

I do not want you to feel sorry, and I do not understand why you feel that
way.

I think a reasonable and safe way to trigger an action in response to a
request to fetch from a repository _is_ a sensible thing to wish for. So
far, we established that we cannot just simply add pre-upload hook back in
and be done with it, as that is not a safe way. We learned something.

Jeff may be right that any approach based on hooks cannot be made totally
safe. But the discussion can lead to a workable alternative. The "enable
the hook only on git:// and http:// and no other" approach might or might
not be such a workable alternative. The "try talking to a service process
via named pipe, instead of spawning a hook" might or might not be such a
workable alternative. Other possibilities may be there to be explored.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sun, Nov 27, 2011 at 10:41:39PM -0800, Junio C Hamano wrote:

> >> I actually like the idea of allowing pre-upload-pack hook on git:// and
> >> possibly http:// only....
> >>
> >> One scenario I do not want to see is this. Suppose ...
> >
> > I'm sorry I started this discussion.  I worked around it, though it's
> > a bit kludgy, so maybe time to drop the debate.
> 
> I do not want you to feel sorry, and I do not understand why you feel that
> way.

Agreed. This is a topic that has come up several times before for other
users (GitHub and Gentoo come to mind), and which I've given some
thought to, which is why I'm writing so many words on the subject.

As for your kludge, I took a peek at gitpod. I actually think
intercepting the call to upload-pack is a reasonably sane approach,
since you really don't care what the client says, and just want to run
before anything else (including ref advertisement) happens. It would be
much nicer if git-daemon could take a parameter[1] to an alternate
upload-pack path so you didn't have to play PATH games. That would be a
simple patch with no security ramifications, I think.

-Peff

[1] I mean a command-line parameter or environment varable. Reading it
    from the repo config _does_ have security implications.

Re: what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

On Mon, Nov 28, 2011 at 1:31 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Nov 27, 2011 at 10:41:39PM -0800, Junio C Hamano wrote:
>
>> >> I actually like the idea of allowing pre-upload-pack hook on git:// and
>> >> possibly http:// only....
>> >>
>> >> One scenario I do not want to see is this. Suppose ...
>> >
>> > I'm sorry I started this discussion.  I worked around it, though it's
>> > a bit kludgy, so maybe time to drop the debate.
>>
>> I do not want you to feel sorry, and I do not understand why you feel that
>> way.
>
> Agreed. This is a topic that has come up several times before for other
> users (GitHub and Gentoo come to mind), and which I've given some
> thought to, which is why I'm writing so many words on the subject.

I can see that you have, and I appreciate that.

I was vaguely feeling guilty about *causing* all that extra effort,
because I think it is a bit of a waste of time.  I sincerely believe
there is *no* place for shared setups [1] these days, however common
they may be.  In fact, if I didn't already believe so, your last few
emails would have convinced me!

I'm amazed to learn in this thread that people apparently even *push*
in such setups!

[1] not just "upload-pack runs as invoking user" but also other
settings like liberal umask, (which you mentioned) and so on.

> As for your kludge, I took a peek at gitpod. I actually think
> intercepting the call to upload-pack is a reasonably sane approach,

Thanks.  I wasn't happy at first but now I'm fine with it.

-- 
Sitaram

Re: what are the chances of a 'pre-upload' hook?

[Sitaram Chamarty]

On Mon, Nov 28, 2011 at 12:11 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> writes:
>
>>> I actually like the idea of allowing pre-upload-pack hook on git:// and
>>> possibly http:// only....
>>>
>>> One scenario I do not want to see is this. Suppose ...
>>
>> I'm sorry I started this discussion.  I worked around it, though it's
>> a bit kludgy, so maybe time to drop the debate.
>
> I do not want you to feel sorry, and I do not understand why you feel that
> way.

Because I did not think it was so complicated...? :-)

> I think a reasonable and safe way to trigger an action in response to a
> request to fetch from a repository _is_ a sensible thing to wish for. So
> far, we established that we cannot just simply add pre-upload hook back in
> and be done with it, as that is not a safe way. We learned something.

> Jeff may be right that any approach based on hooks cannot be made totally
> safe. But the discussion can lead to a workable alternative. The "enable
> the hook only on git:// and http:// and no other" approach might or might
> not be such a workable alternative. The "try talking to a service process
> via named pipe, instead of spawning a hook" might or might not be such a
> workable alternative. Other possibilities may be there to be explored.

There are only 2 cases: git-upload-pack runs as invoking user, or it
runs as some common user/repo owner.

I see pre-upload hooks for case 1 as being hard/impossible to do,
while case 2 is trivial (just check if the hook file owner == UID of
the git-upload-pack process).

Yes, this means pre-upload won't work identically in *all* setups.
But as you said somewhere: perfect is the enemy of good.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Mon, Nov 28, 2011 at 01:47:44PM +0530, Sitaram Chamarty wrote:

> > Jeff may be right that any approach based on hooks cannot be made totally
> > safe. But the discussion can lead to a workable alternative. The "enable
> > the hook only on git:// and http:// and no other" approach might or might
> > not be such a workable alternative. The "try talking to a service process
> > via named pipe, instead of spawning a hook" might or might not be such a
> > workable alternative. Other possibilities may be there to be explored.
> 
> There are only 2 cases: git-upload-pack runs as invoking user, or it
> runs as some common user/repo owner.

I think it is slightly more complicated. You may have a common user, but
that user may or may not trust the contents of the repo on disk (e.g.,
a system daemon serving repos for arbitrary users).

> I see pre-upload hooks for case 1 as being hard/impossible to do,
> while case 2 is trivial (just check if the hook file owner == UID of
> the git-upload-pack process).

The UID check doesn't quite tell you what you want. If I am in a shared
repo situation with a liberal umask and I write a hook, it will have my
UID. But it may have group-write permissions. Other people in my group
can write arbitrary contents to the file, but it will retain my UID, and
git's check will think the code is OK to run.

> Yes, this means pre-upload won't work identically in *all* setups.
> But as you said somewhere: perfect is the enemy of good.

Yes, I think we have to accept inconsistency if we have them at all.
Because such hooks are dangerous in some setups, and OK in others. So no
matter what the rule for determining danger, they will sometimes run and
sometimes not.

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> It depends on what you want your hook to do. I thought Sitaram's
> use-case was putting something like "git fetch upstream" into a hook
> that runs before upload-pack, to create a transparent proxy. That has
> nothing to do with the accessing user.
>
> At GitHub, we run a pre-upload-pack hook to keep statistics. That has
> nothing to do with the accessing user. We have to patch git to provide
> the hook.

I personally have a huge problem with shipping an inherently unsafe
mechanism that is disabled by default even if it is marked with big red
letters saying that it is unsafe and should not be enabled casually. It
makes it up to the system administrator to decide what is casual and what
is not, but when end users are get harmed by a clueless administrator's
decision, the repercussion will come back to the Git software, not to the
clueless administrator who enabled an unsafe mechanism in an environment
where it was not designed to support.

It may merely be a matter of perception. After all, the pre-upload-pack
hook I did in a8563ec (upload-pack: add a trigger for post-upload-pack
hook, 2009-08-26) in response to GitHub's stat request was so trivial that
even clueless admins could trojan it back into the current upload-pack
source to use in their own site, without letting their users know that
their fetch request may be opening their account to a random hook
script. So in that sense, it might not be worth my effort to fight for Git
users' safety to find a better solution and instead go with the big red
letter approach which is easy. I know perfect is an enemy of good, but
when I do not think an easy Quick-and-Dirty solution is even less than
good, I need to point out that not having less than bad is better than
having it.

If the mechanism to notify the external machinery (i.e. counting accesses,
learning the true destination of a new fetch request and have the fetcher
wait while the real fetch goes to the origin site) were not via a hook
that runs as the fetcher but were via something else that runs as the
owner of that external service (i.e. counting accesses, maintaining the
proxy object pool), I wouldn't have trouble with the proposal.

For example, upload-pack could write the details of the original request
to a named pipe to ask the "service" process listening on the other end,
and wait for its response.

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Sat, Nov 26, 2011 at 11:51:29PM -0800, Junio C Hamano wrote:

> I personally have a huge problem with shipping an inherently unsafe
> mechanism that is disabled by default even if it is marked with big red
> letters saying that it is unsafe and should not be enabled casually. It
> makes it up to the system administrator to decide what is casual and what
> is not, but when end users are get harmed by a clueless administrator's
> decision, the repercussion will come back to the Git software, not to the
> clueless administrator who enabled an unsafe mechanism in an environment
> where it was not designed to support.

I agree. If we can avoid adding any feature which is even potentially
dangerous, it's good to explore alternatives. At the same time, the
subject of remote fetch hooks has come up before, and I don't think it's
going away. It would be nice to reach more resolution this time.

I also have mild concerns about the current receive-pack hooks. The
thought so far has been "well, you're less likely push into a bad
person's repository". Which is probably true. But it seems like a flaky
thing to base security on, especially in multi-user environments with
shared repositories.

> If the mechanism to notify the external machinery (i.e. counting accesses,
> learning the true destination of a new fetch request and have the fetcher
> wait while the real fetch goes to the origin site) were not via a hook
> that runs as the fetcher but were via something else that runs as the
> owner of that external service (i.e. counting accesses, maintaining the
> proxy object pool), I wouldn't have trouble with the proposal.
> 
> For example, upload-pack could write the details of the original request
> to a named pipe to ask the "service" process listening on the other end,
> and wait for its response.

Right, that was the "socket" solution I mentioned earlier. The problem
is that it's a pain to set up. If you really want to have a daemon
running and handling git tasks, then you might as well just have the
whole upload-pack/receive-pack side accessible over a socket (e.g., just
run git-daemon if you don't care about authentication, and
git-over-https if you do).

If you really care about ssh authentication and keeping track of
individual unix users, then one possibility is to run git-daemon
listening on a unix socket, use SO_PEERCRED to get the user on the other
end, and then process everything as a single git user.

For example, imagine Bob and Alice are in the "research" group, and use
a shared central repo. Bob wants to push:

  1. Bob runs "git push" on his workstation. Git on the workstation
     connects to "ssh bob@server git-receive-pack $repo".

  2. git-receive-pack on the server runs as Bob. It notices that
     $repo/daemon-socket exists, and runs in "shim" mode, connecting to
     the socket and proxying data over the socket.

  3. git-daemon, running as the "research" user, listens on the other
     end of the socket (and potentially sockets for other repos). It
     sees the connection and serves it. The daemon already knows which
     repo to serve because it knows which socket Bob connected to. And
     it knows it's OK for Bob to access it, because he was able to get
     to the socket via filesystem permissions.

     The daemon can use getsockopt(SO_PEERCRED) to find out that Bob is
     the user, and probably set an environment variable. It can freely
     run any hooks, because the hooks are owned by the "research" user,
     and that's the user the daemon runs as. Hooks can trust the
     environment from the daemon, and can use Bob's identity for further
     authorization checks if they want to. IOW, we are roughly at the
     same place you would get by connecting over git-over-https.

This naturally extends to "Bob fetches" or "Bob runs his push from the
server".  As an added bonus over the usual shared-central-repo model,
malicious users can't just ignore hooks (in the usual shared-repo model,
everybody can write directly to the object db and refs, so hooks are
somewhat advisory).

I suspect people who really care about this use case are already running
git-over-https, possibly with something like gerrit doing some policy
enforcement. I also suspect there are a lot of corporate environments
where they just have a big shared repo, set core.sharedRepository, and
don't worry too much about users hurting each other (in most dev
environments I've seen, people are perfectly willing to fetch and run
make without looking at the diff, so it's sort of a moot point).

People in the latter half would be one of the potential audiences for
the "screw it, just run hooks from the remote repo" flag.

> I actually like the idea of allowing pre-upload-pack hook on git:// and
> possibly http:// only. git-daemon can tell the upload-pack that it is OK
> to run the hook, and the hook can do the things that only the daemon can
> do, never touching what the original requestor would but the repository
> owner would not have an access to.

That's not necessarily safe. Think about a site like kernel.org. The
administrator is the one running the daemon, on behalf of all of the
users. But my impression is that pre-August, developers had shell access
to their own repos and could write their own hook files. So if
git-daemon runs hooks, then any repo owner could run arbitrary code as
the git-daemon, including killing the running daemon and running their
own trojan.

So you really need some way for the admin to say "it's OK, hooks are
locked down and it's safe for them to run". Which again, I was thinking
of as an environment variable.

However, I think for the daemon case, the other alternative I mentioned
earlier is a bit more elegant: an environment variable which doesn't say
"respect the hooks in the repo", but rather "run this _particular_
program as the pre-upload-pack hook".  And then the admin doesn't have
to care what's in the repos. The daemon runs some particular program
every time a fetch happens.

It's tempting to say "can't they just replace git-upload-pack with a
shim that does what they want and then runs upload-pack?"[1]. But I
think the answer is no, for the same reason we have hooks at all: they
might want to operate under a lock, or use some data produced by
upload-pack, or make a decision that blocks the fetch.

[1] Actually, I think Sitaram's case may not be a pre-upload-pack hook,
    but rather the simple "run before upload-pack" case. Because a
    pre-upload-pack would probably feed the set of requested objects to
    the hook. But he will want to fetch from the upstream before even
    advertising any refs to the client.

> One scenario I do not want to see is this. Suppose there is a company that
> runs more than one project, and one of the projects is so secret that only
> engineers working on the project have access to its repository, while all
> people, including the project-A engineers, in the company have access to
> other repositories. Further suppose that people involved in some or many
> of the general repositories want to do something before a fetch from them
> happens. They will use the pre-upload-hook to implement it if we make it
> available, and their new-hire training course will tell their engineers to
> set the GIT_ALLOW_UNTRUSTED_HOOKS. Perhaps the /etc/skel/profile the
> company gives the new-hires already defines it.

Right. This is a failure of the training course and/or the admin who
wrote the default profile, and is exactly the thing that would be
mentioned in the red flags in the docs. I agree there is a risk of
bone-headed admins, though.

This, btw, is more or less the case I am concerned about _now_ with
receive-pack. With current git, if any of those general repositories is
a shared central repo, then project A is similarly vulnerable to anyone
who can write the hook.

-Peff

Re: what are the chances of a 'pre-upload' hook?

[Jeff King]

On Mon, Nov 28, 2011 at 02:51:07AM -0500, Jeff King wrote:

> > I actually like the idea of allowing pre-upload-pack hook on git:// and
> > possibly http:// only. git-daemon can tell the upload-pack that it is OK
> > to run the hook, and the hook can do the things that only the daemon can
> > do, never touching what the original requestor would but the repository
> > owner would not have an access to.
> 
> That's not necessarily safe. Think about a site like kernel.org. The
> administrator is the one running the daemon, on behalf of all of the
> users. But my impression is that pre-August, developers had shell access
> to their own repos and could write their own hook files. So if
> git-daemon runs hooks, then any repo owner could run arbitrary code as
> the git-daemon, including killing the running daemon and running their
> own trojan.

Actually, depending on how kernel.org (or other similar sites) run the
daemon, this might be an issue even without further patches. By default,
git-daemon lets the enable/disable flag for any service be overridden by
the repo config. So something like this:

  # make an evil repo; imagine this is done by a user on a shared
  # hosting site which allows shell access.
  git init --bare evil &&
  cat >evil/hooks/update <<\EOF &&
  #!/bin/sh
  echo >&2 "executing arbitrary code as `id`"
  EOF
  git --git-dir=evil config daemon.receivepack true &&
  touch evil/git-daemon-export-ok &&
  chmod -R 777 evil

  # now serve the repo as a daemon running as some other
  # user. This simulates the admin of a shared git hosting
  # site running git-daemon.
  sudo su -c 'git daemon --verbose --base-path="$PWD"' nobody

and then push from anywhere:

  $ git push --all git://localhost/evil
  ...
  remote: executing arbitrary code as uid=65534(nobody) gid=65534(nogroup) groups=65534(nogroup)

You can avoid this by setting --forbid-override=receivepack; I wonder if
that should be the default.

-Peff