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

Jeff King [off-list ref] 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.

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

Jeff King [off-list ref] writes:

quoted

[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

On Fri, Sep 2, 2011 at 09:25, Jeff King [off-list ref] wrote:

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

quoted

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.

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

quoted

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

On Mon, Sep 5, 2011 at 13:47, Jeff King [off-list ref] wrote:

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

quoted

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.

quoted

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.

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

quoted

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

On Mon, Sep 5, 2011 at 13:57, Jeff King [off-list ref] wrote:

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

quoted

quoted

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.

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

quoted

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

quoted

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

On Wed, Sep 7, 2011 at 14:20, Jeff King [off-list ref] 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.

Jeff King wrote:

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

quoted

quoted

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