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

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.