Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Junio C Hamano]

Suggested reading:

  http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html

I am wondering if we are better off applying something along the lines of
this patch, so that with the default configuration, users can notice if
their upstream unexpectedly rewound their branches.

It would produce

	[remote]
        	url = git://.../git.git/
                fetch = refs/heads/*:refs/remotes/origin/*

upon cloning from my repository, and your "git fetch" will fail because
the pu (proposed updates) branch is constantly unwinding, but that can be
easily fixed with


	[remote]
        	url = git://.../git.git/
                fetch = refs/heads/*:refs/remotes/origin/*
                fetch = +refs/heads/pu:refs/remotes/origin/pu

as the explicit refspec trumps the wildcard one.

 builtin/remote.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index f2a9c26..081fbbf 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -116,11 +116,11 @@ static int add_branch(const char *key, const char *branchname,
 		const char *remotename, int mirror, struct strbuf *tmp)
 {
 	strbuf_reset(tmp);
-	strbuf_addch(tmp, '+');
-	if (mirror)
+	if (mirror) {
+		strbuf_addch(tmp, '+');
 		strbuf_addf(tmp, "refs/%s:refs/%s",
 				branchname, branchname);
-	else
+	} else
 		strbuf_addf(tmp, "refs/heads/%s:refs/remotes/%s/%s",
 				branchname, remotename, branchname);
 	return git_config_set_multivar(key, tmp->buf, "^$", 0);

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Suggested reading:
>
>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html
>
> I am wondering if we are better off applying something along the lines of
> this patch, so that with the default configuration, users can notice if
> their upstream unexpectedly rewound their branches.
>
> It would produce
>
> 	[remote]
>         	url = git://.../git.git/
>                 fetch = refs/heads/*:refs/remotes/origin/*
>
> upon cloning from my repository, and your "git fetch" will fail because
> the pu (proposed updates) branch is constantly unwinding, but that can be
> easily fixed with
>
>
> 	[remote]
>         	url = git://.../git.git/
>                 fetch = refs/heads/*:refs/remotes/origin/*
>                 fetch = +refs/heads/pu:refs/remotes/origin/pu
>
> as the explicit refspec trumps the wildcard one.

It appears that we have a glitch somewhere in the implementation. We
should make the explicit refspec trump the wildcarded ones.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Shawn Pearce]

On Thu, Sep 1, 2011 at 11:39, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Suggested reading:
>>
>>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html
>>
>> I am wondering if we are better off applying something along the lines of
>> this patch, so that with the default configuration, users can notice if
>> their upstream unexpectedly rewound their branches.
>>
>> It would produce
>>
>>       [remote]
>>               url = git://.../git.git/
>>                 fetch = refs/heads/*:refs/remotes/origin/*
>>
>> upon cloning from my repository, and your "git fetch" will fail because
>> the pu (proposed updates) branch is constantly unwinding, but that can be
>> easily fixed with
>>
>>
>>       [remote]
>>               url = git://.../git.git/
>>                 fetch = refs/heads/*:refs/remotes/origin/*
>>                 fetch = +refs/heads/pu:refs/remotes/origin/pu
>>
>> as the explicit refspec trumps the wildcard one.
>
> It appears that we have a glitch somewhere in the implementation. We
> should make the explicit refspec trump the wildcarded ones.

This is a great idea. :-)

-- 
Shawn.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 01.09.2011 20:25:
> Suggested reading:
> 
>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html
> 
> I am wondering if we are better off applying something along the lines of
> this patch, so that with the default configuration, users can notice if
> their upstream unexpectedly rewound their branches.
> 
> It would produce
> 
> 	[remote]
>         	url = git://.../git.git/
>                 fetch = refs/heads/*:refs/remotes/origin/*
> 
> upon cloning from my repository, and your "git fetch" will fail because
> the pu (proposed updates) branch is constantly unwinding, but that can be
> easily fixed with
> 
> 
> 	[remote]
>         	url = git://.../git.git/
>                 fetch = refs/heads/*:refs/remotes/origin/*
>                 fetch = +refs/heads/pu:refs/remotes/origin/pu
> 
> as the explicit refspec trumps the wildcard one.
> 
>  builtin/remote.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

My first thought was "Oh no", even though I saw it coming since I read
your blog entry. But I realized that it was probably due to the usual
inertia when habits have to change.

Thinking more about it, we try to encourage a workflow where locally
history may be rewritten a lot, and distribution points fast-forward
only. We have defaults and settings to discourage (pushes to checked out
branches and) non-ff pushes, for example. So I think the above change is
pretty much in line with that reasoning.

The kernel.org problems gave git some pretty good PR even without that
change, but with it we have an even stronger stop put in. On the other
hand, adding "+" to the config for "pu" (and peff...) isn't that much of
an issue, though we might also want "fetch -f" as an override - I guess
we have that already.

Plus fetch with fsck, yes.

To

"I don't need backups, I let others clone."

add

"I don't need intrusion detection, I let others fetch."

;)

Michael

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Matthieu Moy]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> Thinking more about it, we try to encourage a workflow where locally
> history may be rewritten a lot, and distribution points fast-forward
> only. We have defaults and settings to discourage (pushes to checked out
> branches and) non-ff pushes, for example. So I think the above change is
> pretty much in line with that reasoning.

Agreed. It's not only a security thing, it's also about
teaching/encourraging workflows.

By asking users to explicitely say "yes, I know, this branch can be
rewond", we also ask them to think about it before making a mistake.

That said, enabling the check by default may also become painful. I'd
vote for a configuration option, defaulting to the current behavior for
now. Then we can try living with it for a while and see how painful it
is.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Shawn Pearce]

On Thu, Sep 1, 2011 at 12:35, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
> By asking users to explicitely say "yes, I know, this branch can be
> rewond", we also ask them to think about it before making a mistake.
>
> That said, enabling the check by default may also become painful. I'd
> vote for a configuration option, defaulting to the current behavior for
> now. Then we can try living with it for a while and see how painful it
> is.

I suspect the vast majority of branches in the wild do not rewind
under normal conditions. Users who work against branches that rewind
(e.g. those of us basing on a topic in pu) are already sophisticated
enough with Git to understand what the fetch error would mean and fix
it.

IMHO, just change the default in clone, and better, add a warning to
fetch if that default pattern is still in the configuration file. Let
the user either remove the wildcarded force fetch spec, or add a new
configuration variable to his remote block to silence the warning.

-- 
Shawn.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Matthieu Moy]

Shawn Pearce <spearce@spearce.org> writes:

> On Thu, Sep 1, 2011 at 12:35, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
>> By asking users to explicitely say "yes, I know, this branch can be
>> rewond", we also ask them to think about it before making a mistake.
>>
>> That said, enabling the check by default may also become painful. I'd
>> vote for a configuration option, defaulting to the current behavior for
>> now. Then we can try living with it for a while and see how painful it
>> is.
>
> I suspect the vast majority of branches in the wild do not rewind
> under normal conditions. Users who work against branches that rewind
> (e.g. those of us basing on a topic in pu)

Err, I don't think it's about people basing their work on pu, but rather
about anybody fetching from pu, i.e. everybody calling "git fetch" or
"git pull" in their clone.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Thu, Sep 01, 2011 at 11:25:31AM -0700, Junio C Hamano wrote:

> Suggested reading:
> 
>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html
> 
> I am wondering if we are better off applying something along the lines of
> this patch, so that with the default configuration, users can notice if
> their upstream unexpectedly rewound their branches.

Hmm. This feels like it's subtly changing the meaning of
refs/remotes/$remote/*.

Right now, I think of it as a local cache for whatever the remote side
has. In other words, a way of separating the network-fetching parts of
the workflow from the local parts. And in that sense, it is perfectly
reasonable to overwrite with what the other side has, whether they
rewind or not, because we are just representing what they have. And
since we keep a reflog, it's not as if the previous state is lost to us
locally.

But with this change, we are making a policy judgement about what to
fetch. And as you noticed, it means that users need to start telling git
about their policy (e.g., mentioning in the refspecs that pu can rewind)
in order to keep fetch working.

So I consider that a downside, because it's extra work for the user[1].
What are the upsides?

Is this about preventing workflow-related mistakes where people
accidentally merge in rebased commits, creating annoying shadow
histories?

Is it about preventing malicious rewinds from infecting downstream
repositories?

If the former, then I suspect we need to give more guidance to the user
than saying "reject, non-fast-forward". What then? Should they "fetch
-f"?  Or "pull --rebase" (actually, how do they even fetch the branch
now for such a pull --rebase)? Or talk out-of-band to the repo owner?

If the latter, then I think we should be specific about the attack
scenarios, and what happens with and without this config. And if it's a
security precaution, what cases doesn't it cover (e.g., initial clone is
still vulnerable, as is a one-off pull. As are are malicious insertion
attacks that don't involve rewinding).

And then we can weigh the upsides and the downsides.

-Peff

[1] What I really don't like is that cloning git.git is no longer:

      git clone git://git.kernel.org/pub/scm/git/git.git

    which is a minimal as it can be, but becomes:

      git clone git://git.kernel.org/pub/scm/git/git.git
      cd git
      git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu

    It's not that my fingers are too tired to do all that typing, but
    rather that the first set of instructions is very easy to explain,
    and the second one is full of magic and head-scratching about why
    git isn't handling this magic itself.

    It would be considerably nicer if the server had some way of saying
    "I expect this branch to be rewound". Which has been discussed off
    and on over the years, as I recall.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Johannes Sixt]

Am 9/2/2011 2:00, schrieb Jeff King:
> Right now, I think of it as a local cache for whatever the remote side
> has. In other words, a way of separating the network-fetching parts of
> the workflow from the local parts.

This is also my interpretation. For this reason, I don't think a change is
necessary.

> So I consider that a downside, because it's extra work for the user[1].
> What are the upsides?
> 
> Is this about preventing workflow-related mistakes where people
> accidentally merge in rebased commits, creating annoying shadow
> histories?
> 
> Is it about preventing malicious rewinds from infecting downstream
> repositories?

All good questions to ask.

> [1] What I really don't like is that cloning git.git is no longer:
> 
>       git clone git://git.kernel.org/pub/scm/git/git.git
> 
>     which is a minimal as it can be, but becomes:
> 
>       git clone git://git.kernel.org/pub/scm/git/git.git
>       cd git
>       git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu
> 
>     It's not that my fingers are too tired to do all that typing, but
>     rather that the first set of instructions is very easy to explain,
>     and the second one is full of magic and head-scratching about why
>     git isn't handling this magic itself.

Absolutely.

>     It would be considerably nicer if the server had some way of saying
>     "I expect this branch to be rewound". Which has been discussed off
>     and on over the years, as I recall.

So, if such a feature were available, wouldn't it be nicer if the initial
clone set up the refspec like this:

  [remote "origin"]
        url = git://git.kernel.org/pub/scm/git/git.git
        fetch = +refs/heads/*:refs/remotes/origin/*
        fetch = refs/heads/maint:refs/remotes/origin/maint
        fetch = refs/heads/master:refs/remotes/origin/master

i.e., the non-wildcard refspec are about which branches are *not* expected
to be rewound rather than the other way around.

-- Hannes

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Fri, Sep 02, 2011 at 09:00:55AM +0200, Johannes Sixt wrote:

> >     It would be considerably nicer if the server had some way of saying
> >     "I expect this branch to be rewound". Which has been discussed off
> >     and on over the years, as I recall.
> 
> So, if such a feature were available, wouldn't it be nicer if the initial
> clone set up the refspec like this:
> 
>   [remote "origin"]
>         url = git://git.kernel.org/pub/scm/git/git.git
>         fetch = +refs/heads/*:refs/remotes/origin/*
>         fetch = refs/heads/maint:refs/remotes/origin/maint
>         fetch = refs/heads/master:refs/remotes/origin/master
> 
> i.e., the non-wildcard refspec are about which branches are *not* expected
> to be rewound rather than the other way around.

I don't see the advantage one way or the other. Doesn't it just amount
to what the default will be? And isn't "not rewind" generally the more
common, and hence a better default?

Or are you saying that for backwards compatibility, it would be better
to end up with a refspec more like what we have now? That I can see the
advantage of.

-Peff

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Michael J Gruber]

Jeff King venit, vidit, dixit 02.09.2011 02:00:
> On Thu, Sep 01, 2011 at 11:25:31AM -0700, Junio C Hamano wrote:
> 
>> Suggested reading:
>>
>>   http://git-blame.blogspot.com/2011/08/how-to-inject-malicious-commit-to-git.html
>>
>> I am wondering if we are better off applying something along the lines of
>> this patch, so that with the default configuration, users can notice if
>> their upstream unexpectedly rewound their branches.
> 
> Hmm. This feels like it's subtly changing the meaning of
> refs/remotes/$remote/*.
> 
> Right now, I think of it as a local cache for whatever the remote side
> has. In other words, a way of separating the network-fetching parts of
> the workflow from the local parts. And in that sense, it is perfectly
> reasonable to overwrite with what the other side has, whether they
> rewind or not, because we are just representing what they have. And
> since we keep a reflog, it's not as if the previous state is lost to us
> locally.
> 
> But with this change, we are making a policy judgement about what to
> fetch. And as you noticed, it means that users need to start telling git
> about their policy (e.g., mentioning in the refspecs that pu can rewind)
> in order to keep fetch working.
> 
> So I consider that a downside, because it's extra work for the user[1].
> What are the upsides?
> 
> Is this about preventing workflow-related mistakes where people
> accidentally merge in rebased commits, creating annoying shadow
> histories?
> 
> Is it about preventing malicious rewinds from infecting downstream
> repositories?
> 
> If the former, then I suspect we need to give more guidance to the user
> than saying "reject, non-fast-forward". What then? Should they "fetch
> -f"?  Or "pull --rebase" (actually, how do they even fetch the branch
> now for such a pull --rebase)? Or talk out-of-band to the repo owner?
> 
> If the latter, then I think we should be specific about the attack
> scenarios, and what happens with and without this config. And if it's a
> security precaution, what cases doesn't it cover (e.g., initial clone is
> still vulnerable, as is a one-off pull. As are are malicious insertion
> attacks that don't involve rewinding).
> 
> And then we can weigh the upsides and the downsides.
> 
> -Peff
> 
> [1] What I really don't like is that cloning git.git is no longer:
> 
>       git clone git://git.kernel.org/pub/scm/git/git.git
> 
>     which is a minimal as it can be, but becomes:
> 
>       git clone git://git.kernel.org/pub/scm/git/git.git
>       cd git
>       git config --add remote.origin.fetch +refs/heads/pu:refs/remotes/origin/pu
> 
>     It's not that my fingers are too tired to do all that typing, but
>     rather that the first set of instructions is very easy to explain,
>     and the second one is full of magic and head-scratching about why
>     git isn't handling this magic itself.
> 
>     It would be considerably nicer if the server had some way of saying
>     "I expect this branch to be rewound". Which has been discussed off
>     and on over the years, as I recall.

Hmm, that sounds like the often requested feature to have part of the
config distributed by "clone" as well, possibly after displaying it and
getting user confirmation.

Michael

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Fri, Sep 02, 2011 at 09:42:49AM +0200, Michael J Gruber wrote:

> >     It would be considerably nicer if the server had some way of saying
> >     "I expect this branch to be rewound". Which has been discussed off
> >     and on over the years, as I recall.
> 
> Hmm, that sounds like the often requested feature to have part of the
> config distributed by "clone" as well, possibly after displaying it and
> getting user confirmation.

Yeah, if distributed config existed, that would be a good place to put
this information.

And I personally thing a limited form[1] of distributed config is a
sensible idea, but I'm not sure everybody else agrees.

-Peff

[1] My idea of "limited" would be an allow-known-good list of harmless
config keys which we would respect when they came from the remote, with
the option for the user to whitelist or blacklist more keys if they
wanted.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> [1] My idea of "limited" would be an allow-known-good list of harmless
> config keys which we would respect when they came from the remote, with
> the option for the user to whitelist or blacklist more keys if they
> wanted.

It coincides with my idea too, but it might be a very limited set. For
example, there may be a good "suggested by upstream" default for LHS of
fetch refspecs (e.g. somebody may have 47 topics published but majority of
people are expected to follow only his "master" branch), but it is up to
the user of that suggestion what the RHS would be.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Fri, Sep 02, 2011 at 09:14:15AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > [1] My idea of "limited" would be an allow-known-good list of harmless
> > config keys which we would respect when they came from the remote, with
> > the option for the user to whitelist or blacklist more keys if they
> > wanted.
> 
> It coincides with my idea too, but it might be a very limited set. For
> example, there may be a good "suggested by upstream" default for LHS of
> fetch refspecs (e.g. somebody may have 47 topics published but majority of
> people are expected to follow only his "master" branch), but it is up to
> the user of that suggestion what the RHS would be.

Yeah. That leads to synthesizing local keys based on what remote keys
say. Which is pretty straightforward if you are just fetching the
remote's config during clone, and then copying or creating local keys
based on that in your own .git/config (e.g., by creating full refspecs
with upstream's idea of the LHS, and our idea of the RHS).

But it becomes hard to keep your local config in sync with updates on
the remote end. When the remote now adds "next" to the list of
interesting branches, by what mechanism do you fix up your local config?
Certainly we wouldn't want to rewrite the local config without
consulting the user, because they may have reviewed or modified it since
it was created.

One possible solution is that the local config could dynamically depend
on the remote config. E.g., the fetch refspec has something like a
wildcard that matches only the branches that the remote provides to us
via some "interesting branches" config key. Then it's OK for git to
update the "interesting branches" key from the remote. Either the user
is OK with respecting that (because they have left the wildcard in
place), or not (because they have changed the refspec not to use that
wildcard).

I do worry that could quickly get complex, and people would start
wanting a Turing-complete config language. :)

-Peff

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> It coincides with my idea too, but it might be a very limited set. For
>> example, there may be a good "suggested by upstream" default for LHS of
>> fetch refspecs (e.g. somebody may have 47 topics published but majority of
>> people are expected to follow only his "master" branch), but it is up to
>> the user of that suggestion what the RHS would be.
>
> Yeah. That leads to synthesizing local keys based on what remote keys
> say. Which is pretty straightforward if you are just fetching the
> remote's config during clone, and then copying or creating local keys
> based on that in your own .git/config (e.g., by creating full refspecs
> with upstream's idea of the LHS, and our idea of the RHS).
> ...
> I do worry that could quickly get complex, and people would start
> wanting a Turing-complete config language. :)

My real point was that more often than not the settings of configuration
variables are inherently per repository not per project, and even though
we may hear people want shared configs, possibly in-tree, distributed as
part of the projects, such a set-up would not be very useful, before you
even consider merging updates but just taking them as suggested initial
values. The choice to take, ignore, or tweak the suggestions all depend
on the semantics of each variable, and it becomes more so once you start
talking about merging updates.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Shawn Pearce]

On Fri, Sep 2, 2011 at 09:25, Jeff King <peff@peff.net> wrote:
> On Fri, Sep 02, 2011 at 09:14:15AM -0700, Junio C Hamano wrote:
>>
>> It coincides with my idea too, but it might be a very limited set. For
>> example, there may be a good "suggested by upstream" default for LHS of
>> fetch refspecs (e.g. somebody may have 47 topics published but majority of
>> people are expected to follow only his "master" branch), but it is up to
>> the user of that suggestion what the RHS would be.
...
> One possible solution is that the local config could dynamically depend
> on the remote config. E.g., the fetch refspec has something like a
> wildcard that matches only the branches that the remote provides to us
> via some "interesting branches" config key. Then it's OK for git to
> update the "interesting branches" key from the remote. Either the user
> is OK with respecting that (because they have left the wildcard in
> place), or not (because they have changed the refspec not to use that
> wildcard).
>
> I do worry that could quickly get complex, and people would start
> wanting a Turing-complete config language. :)

What are we trying to do here?

I had some thought that dropping the "+" might prevent a remote
repository from being fetched from if it was rewound by an evil
attacker that now controls it. Unfortunately that attack is a
pointless one. Which makes this change to remove the "+" from fetch
specs also pointless.

If the attacker knows Git clients always fetch rewinds, he might be
tempted to rewrite some part of history and serve his modified history
of events to clients. But the repository owner (if using a private
per-user repository model like the Linux kernel developers use) would
notice on their next push, and sound the alarm that her repository has
been damaged and should not be trusted.

If on the other hand Git clients never fetch rewinds, the attacker
would just add a new commit to the tip of the history, and serve that.
Again, the repository owner would notice on their next push, and
notify people the repository is not to be trusted.

Either way, the "+" in the fetch spec has no impact on the attack. The
default just changes the attacker's choices slightly.


Maybe instead of getting a project policy from the server, we observe
the server's behavior over time from the client's reflog. If every
update to "maint" that _I_ have observed has always been a
fast-forward, a rewind on that branch should be a lot more verbose in
the fetch output than "force update". That is pretty easy to observe
from the reflog too, its just a scan of the records and either
matching the message, or checking the merge status of the old-new
pairs listed in the record. We don't even need to read the entire log
on every fetch, we could cache this data.

The main reason to alert the user that a branch rewound is to give
them a chance to correct their client if they need to. If a branch
normally doesn't rewind (e.g. next) but then suddenly does (e.g.
release cycle), but I haven't used this client in 3 weeks, its nice to
give me more of a "HEY STUPID FIX YOUR TOPICS" warning than just the
little quiet "force update" we give.

-- 
Shawn.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Mon, Sep 05, 2011 at 11:15:26AM -0700, Shawn O. Pearce wrote:

> > One possible solution is that the local config could dynamically depend
> > on the remote config. E.g., the fetch refspec has something like a
> [...]
> 
> What are we trying to do here?

We veered way off topic into the idea of generally pulling config from a
remote. This was just one specific example of how it could be used, and
what kinds of complications that might entail.

> If the attacker knows Git clients always fetch rewinds, he might be
> tempted to rewrite some part of history and serve his modified history
> of events to clients. But the repository owner (if using a private
> per-user repository model like the Linux kernel developers use) would
> notice on their next push, and sound the alarm that her repository has
> been damaged and should not be trusted.
> 
> If on the other hand Git clients never fetch rewinds, the attacker
> would just add a new commit to the tip of the history, and serve that.
> Again, the repository owner would notice on their next push, and
> notify people the repository is not to be trusted.
> 
> Either way, the "+" in the fetch spec has no impact on the attack. The
> default just changes the attacker's choices slightly.

Exactly. This is what I was hinting at in my original email in this
thread. My gut feeling is that it's not useful as a security measure,
but I was trying to challenge people to prove me wrong by showing a case
where the attacker can't just trivially modify his attack to get the
same result.

> Maybe instead of getting a project policy from the server, we observe
> the server's behavior over time from the client's reflog. If every
> update to "maint" that _I_ have observed has always been a
> fast-forward, a rewind on that branch should be a lot more verbose in
> the fetch output than "force update". That is pretty easy to observe
> from the reflog too, its just a scan of the records and either
> matching the message, or checking the merge status of the old-new
> pairs listed in the record. We don't even need to read the entire log
> on every fetch, we could cache this data.

Hmm. That would probably work most of the time in practice. But it seems
like it would be quite confusing when the heuristic is wrong (e.g.,
Junio rewinds next once every few months, and other than that, it always
fast forwards). On the other hand, if the failure mode of the heuristic
is only a slightly bigger warning, then it's not that big a deal.

> The main reason to alert the user that a branch rewound is to give
> them a chance to correct their client if they need to. If a branch
> normally doesn't rewind (e.g. next) but then suddenly does (e.g.
> release cycle), but I haven't used this client in 3 weeks, its nice to
> give me more of a "HEY STUPID FIX YOUR TOPICS" warning than just the
> little quiet "force update" we give.

Sure. I'm totally open to the idea of making the non-fast-forward
warning more obvious. Suggestions for wording (though I am tempted by
"HEY STUPID" above ;) )?

-Peff

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Shawn Pearce]

On Mon, Sep 5, 2011 at 13:47, Jeff King <peff@peff.net> wrote:
> On Mon, Sep 05, 2011 at 11:15:26AM -0700, Shawn O. Pearce wrote:
>> Maybe instead of getting a project policy from the server, we observe
>> the server's behavior over time from the client's reflog.
>
> Hmm. That would probably work most of the time in practice. But it seems
> like it would be quite confusing when the heuristic is wrong (e.g.,
> Junio rewinds next once every few months, and other than that, it always
> fast forwards). On the other hand, if the failure mode of the heuristic
> is only a slightly bigger warning, then it's not that big a deal.

Right. Its probably a bigger failure not to warn than to warn here too.

>> The main reason to alert the user that a branch rewound is to give
>> them a chance to correct their client if they need to. If a branch
>> normally doesn't rewind (e.g. next) but then suddenly does (e.g.
>> release cycle), but I haven't used this client in 3 weeks, its nice to
>> give me more of a "HEY STUPID FIX YOUR TOPICS" warning than just the
>> little quiet "force update" we give.
>
> Sure. I'm totally open to the idea of making the non-fast-forward
> warning more obvious. Suggestions for wording (though I am tempted by
> "HEY STUPID" above ;) )?

I'm not suggesting all non-fast-forward should issue a bigger warning.
pu updates daily with a non-fast-forward. That isn't useful.

But if the local reflog hints that this reference almost never does a
non-fast-forward, and then it does, that should be a big warning.

-- 
Shawn.

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Jeff King]

On Mon, Sep 05, 2011 at 01:53:42PM -0700, Shawn O. Pearce wrote:

> > Sure. I'm totally open to the idea of making the non-fast-forward
> > warning more obvious. Suggestions for wording (though I am tempted by
> > "HEY STUPID" above ;) )?
> 
> I'm not suggesting all non-fast-forward should issue a bigger warning.
> pu updates daily with a non-fast-forward. That isn't useful.
> 
> But if the local reflog hints that this reference almost never does a
> non-fast-forward, and then it does, that should be a big warning.

Right. What I mean is, what should the bigger warning look like?

Also, you suggested caching to avoid looking through the whole reflog
each time. I think you could probably just sample the last 10 or so
reflog entries to get an idea.

-Peff

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Shawn Pearce]

On Mon, Sep 5, 2011 at 13:57, Jeff King <peff@peff.net> wrote:
> On Mon, Sep 05, 2011 at 01:53:42PM -0700, Shawn O. Pearce wrote:
>
>> > Sure. I'm totally open to the idea of making the non-fast-forward
>> > warning more obvious. Suggestions for wording (though I am tempted by
>> > "HEY STUPID" above ;) )?
>>
>> I'm not suggesting all non-fast-forward should issue a bigger warning.
>> pu updates daily with a non-fast-forward. That isn't useful.
>>
>> But if the local reflog hints that this reference almost never does a
>> non-fast-forward, and then it does, that should be a big warning.
>
> Right. What I mean is, what should the bigger warning look like?

Its a bikeshed. I refuse to paint bikesheds. :-)

> Also, you suggested caching to avoid looking through the whole reflog
> each time. I think you could probably just sample the last 10 or so
> reflog entries to get an idea.

Good point. 10 or so last records might be representative of the
branch's recent behavior, which is all that matters to the user who
wants this warning.

-- 
Shawn.

[RFC/PATCH] fetch: bigger forced-update warnings

[Jeff King]

On Mon, Sep 05, 2011 at 02:14:57PM -0700, Shawn O. Pearce wrote:

> > Right. What I mean is, what should the bigger warning look like?
> 
> Its a bikeshed. I refuse to paint bikesheds. :-)

Hmph. Somebody has to write the patch. :P

> > Also, you suggested caching to avoid looking through the whole reflog
> > each time. I think you could probably just sample the last 10 or so
> > reflog entries to get an idea.
> 
> Good point. 10 or so last records might be representative of the
> branch's recent behavior, which is all that matters to the user who
> wants this warning.

Actually, because recent ones are near the end, it's much easier to say
"look at the last 4096 bytes of reflogs" rather than "look at exactly
10". For our purposes, it's about the same (actually 4096 is probably
more like 18-20, depending on the exact size of each entry. But it's a
page, so it's probably reasonable).

-- >8 --
Subject: fetch: bigger forced-update warnings

The default fetch refspec allows forced-updates. We already
print "forced update" in the status table, but it's easy to
miss. Let's make the warning a little more prominent.

Some branches are expected to rewind, so the prominent
warning would be annoying. However, git doesn't know what
the expectation is for a particular branch. We can have it
guess by peeking at the lost couple of reflog entries. If we
see all fast forwards, then a new forced-update is probably
noteworthy. If we see something that force-updates all the
time, it's probably boring and not worth displaying the big
warning (we keep the status table "forced update" note, of
course).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/fetch.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index 93c9938..93bfefa 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -208,6 +208,34 @@ static struct ref *get_ref_map(struct transport *transport,
 	return ref_map;
 }
 
+struct update_counts {
+	unsigned fastforward;
+	unsigned forced;
+};
+
+static int count_updates(unsigned char *osha1, unsigned char *nsha1,
+			 const char *email, unsigned long timestamp, int tz,
+			 const char *message, void *data)
+{
+	struct update_counts *uc = data;
+	/* We could check the ancestry of osha1 and nsha1, but this is way
+	 * cheaper */
+	if (!prefixcmp(message, "fetch: fast-forward"))
+		uc->fastforward++;
+	else if (!prefixcmp(message, "fetch: forced-update\n"))
+		uc->forced++;
+	return 0;
+}
+
+static int forced_update_is_uncommon(const char *ref)
+{
+	struct update_counts uc;
+	memset(&uc, 0, sizeof(&uc));
+	if (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)
+		for_each_reflog_ent(ref, count_updates, &uc);
+	return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */
+}
+
 #define STORE_REF_ERROR_OTHER 1
 #define STORE_REF_ERROR_DF_CONFLICT 2
 
@@ -239,7 +267,8 @@ static int s_update_ref(const char *action,
 
 static int update_local_ref(struct ref *ref,
 			    const char *remote,
-			    char *display)
+			    char *display,
+			    int *uncommon_forced_update)
 {
 	struct commit *current = NULL, *updated;
 	enum object_type type;
@@ -336,6 +365,8 @@ static int update_local_ref(struct ref *ref,
 			TRANSPORT_SUMMARY_WIDTH, quickref, REFCOL_WIDTH, remote,
 			pretty_ref,
 			r ? _("unable to update local ref") : _("forced update"));
+		if (!r && forced_update_is_uncommon(ref->name))
+			*uncommon_forced_update = 1;
 		return r;
 	} else {
 		sprintf(display, "! %-*s %-*s -> %s  %s",
@@ -355,6 +386,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 	const char *what, *kind;
 	struct ref *rm;
 	char *url, *filename = dry_run ? "/dev/null" : git_path("FETCH_HEAD");
+	int uncommon_forced_update = 0;
 
 	fp = fopen(filename, "a");
 	if (!fp)
@@ -428,7 +460,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		fputc('\n', fp);
 
 		if (ref) {
-			rc |= update_local_ref(ref, what, note);
+			rc |= update_local_ref(ref, what, note,
+					       &uncommon_forced_update);
 			free(ref);
 		} else
 			sprintf(note, "* %-*s %-*s -> FETCH_HEAD",
@@ -450,6 +483,8 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,
 		error(_("some local refs could not be updated; try running\n"
 		      " 'git remote prune %s' to remove any old, conflicting "
 		      "branches"), remote_name);
+	if (uncommon_forced_update)
+		warning("HEY STUPID FIX YOUR TOPICS");
 	return rc;
 }
 
-- 
1.7.6.10.g62f04

Re: [RFC/PATCH] fetch: bigger forced-update warnings

[Shawn Pearce]

On Wed, Sep 7, 2011 at 14:20, Jeff King <peff@peff.net> wrote:
> +       if (uncommon_forced_update)
> +               warning("HEY STUPID FIX YOUR TOPICS");

<action>
  <type>paint</type>
  <object>bikeshed</object>
  <why>because-i-can</why>

How about:

  warning("!!! REMOTE BRANCH REWOUND HISTORY !!!");
  warning("    Check status report for branches that rewound.");

</action>

-- 
Shawn.

Re: [RFC/PATCH] fetch: bigger forced-update warnings

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Subject: fetch: bigger forced-update warnings
>
> The default fetch refspec allows forced-updates. We already
> print "forced update" in the status table, but it's easy to
> miss. Let's make the warning a little more prominent.
>
> Some branches are expected to rewind, so the prominent
> warning would be annoying. However, git doesn't know what
> the expectation is for a particular branch. We can have it
> guess by peeking at the lost couple of reflog entries. If we

s/lost/last/

> see all fast forwards, then a new forced-update is probably
> noteworthy. If we see something that force-updates all the
> time, it's probably boring and not worth displaying the big
> warning (we keep the status table "forced update" note, of
> course).
>
> Signed-off-by: Jeff King <peff@peff.net>

This is slightly offtopic, but I have been wondering if this approach do
the right thing for "git pull". Wouldn't the underlying "git fetch" give a
warning, and then the calling "git pull" go ahead and make a merge,
scrolling the warning away with the merge/update summary diffstat? That
would be a larger change if "git pull" needs to stash away the warning
message, do its thing and then spit out the warning later.

> +static int forced_update_is_uncommon(const char *ref)
> +{
> +	struct update_counts uc;
> +	memset(&uc, 0, sizeof(&uc));
> +	if (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)
> +		for_each_reflog_ent(ref, count_updates, &uc);
> +	return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */
> +}

Looks sensible.

Re: [RFC/PATCH] fetch: bigger forced-update warnings

[Jeff King]

On Wed, Sep 07, 2011 at 02:53:32PM -0700, Junio C Hamano wrote:

> > Some branches are expected to rewind, so the prominent
> > warning would be annoying. However, git doesn't know what
> > the expectation is for a particular branch. We can have it
> > guess by peeking at the lost couple of reflog entries. If we
> 
> s/lost/last/

Oops, thanks.

> This is slightly offtopic, but I have been wondering if this approach do
> the right thing for "git pull". Wouldn't the underlying "git fetch" give a
> warning, and then the calling "git pull" go ahead and make a merge,
> scrolling the warning away with the merge/update summary diffstat? That
> would be a larger change if "git pull" needs to stash away the warning
> message, do its thing and then spit out the warning later.

I think this particular warning has nothing to do with git-pull. But
rather, that we should _always_ abort a pull with a forced-update.
Because the only sane things to do there are:

  1. Stop and look around, and see if you should be doing a "git reset"
     first.

or

  2. "git pull --rebase"

But proceeding with the pull just seems like a disaster. So it is not
about "this usually fast forwards, but isn't now, so let's make the
warning bigger". It is more about noticing that it is a forced-update at
all. Maybe that's what you meant by "off-topic". :)

> > +static int forced_update_is_uncommon(const char *ref)
> > +{
> > +	struct update_counts uc;
> > +	memset(&uc, 0, sizeof(&uc));
> > +	if (for_each_recent_reflog_ent(ref, count_updates, 4096, &uc) < 0)
> > +		for_each_reflog_ent(ref, count_updates, &uc);
> > +	return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */
> > +}
> 
> Looks sensible.

Now we just need to paint the shed. :)

-Peff

Re: [RFC/PATCH] fetch: bigger forced-update warnings

[Thomas Rast]

Jeff King wrote:
> On Mon, Sep 05, 2011 at 02:14:57PM -0700, Shawn O. Pearce wrote:
> 
> > > Right. What I mean is, what should the bigger warning look like?
> > 
> > Its a bikeshed. I refuse to paint bikesheds. :-)
[...]
> +	if (uncommon_forced_update)
> +		warning("HEY STUPID FIX YOUR TOPICS");

Whatever comes out of the bikeshedding, I'm going to keep a patch
locally that refreshes the mental picture of Shawn shouting that!

That being said, I think there should be a multiline warning pointing
the user at the "recovering from upstream rebase" section in
git-rebase(1).  At least by default with an advice.* setting to
disable it.

> +       if (!prefixcmp(message, "fetch: fast-forward"))
> +               uc->fastforward++;
> +       else if (!prefixcmp(message, "fetch: forced-update\n"))
> +               uc->forced++;

That doesn't work: fetch puts the whole command line there.
E.g.

  git fetch altgit
  --> fetch altgit: fast-forward

  git fetch altgit next:refs/remotes/next
  --> fetch altgit next:remotes/altgit/next: fast-forward

There's also a minor subtlety here that I had to double-check first:
the message for a branch creation is 'storing head', so the later
check

> +       return uc.fastforward && uc.forced <= 1; /* 1 for the one we just did */

never triggers at the second fetch.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Matthieu Moy]

Shawn Pearce <spearce@spearce.org> writes:

> Again, the repository owner would notice on their next push, and
> notify people the repository is not to be trusted.

For simple attack, yes. But if the server is compromised, you can't
trust it anymore to error out on non-fast-forward. I don't think it
would be very complex to write a modified Git server that would come
back to the official history before a push, and re-introduce faulty
commits right after. pushers wouldn't notice, and fetchers would get
compromised history.

OTOH, non-fast-forward fetches can be reliably detected client-side, and
I like being able to think "whatever the server does, I don't care
because I'm using Git".

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: Dropping '+' from fetch = +refs/heads/*:refs/remotes/origin/*?

[Michael J Gruber]

Matthieu Moy venit, vidit, dixit 06.09.2011 09:39:
> Shawn Pearce <spearce@spearce.org> writes:
> 
>> Again, the repository owner would notice on their next push, and
>> notify people the repository is not to be trusted.
> 
> For simple attack, yes. But if the server is compromised, you can't
> trust it anymore to error out on non-fast-forward. I don't think it
> would be very complex to write a modified Git server that would come
> back to the official history before a push, and re-introduce faulty
> commits right after. pushers wouldn't notice, and fetchers would get
> compromised history.

Exactly. Even on fetch, it could serve different histories depending on
the ip so that the pusher does not notice when fetching from the same ip.

> OTOH, non-fast-forward fetches can be reliably detected client-side, and
> I like being able to think "whatever the server does, I don't care
> because I'm using Git".

reflog based warnings should provide a sane default. We could amend the
update hook for those who want to allow/deny specific branches to
rewind, or make receiveDeny... branch.-specific, if the multiple
refspecs approach is too complicated.

Michael