[RFC] git-format-patch: default to --from to avoid spoofed mails?

When git-format-patch formats a patch authored by someone other than yourself, it defaults to filling in the "From:" field of the email from the commit author. If you explicitly pass the --from option, git-format-patch will instead use your own committer identity as the "From:", and then put a "From:" line at the top of the body if the commit author differs. (git-am know to use that as the commit author when applying.)

While git-send-email knows how to change the patch mails to use your own address as "From:" and add a "From:" line to the body for the author, any other tool used to send emails doesn't do that. I've seen more than a few mails sent to various mailing lists and patch review tools with a spoofed "From:" field pointing to the commit author, typically without the knowledge of the author, which can lead to interesting surprises.

I'd like to propose changing the default behavior of git-format-patch to --from (and adding a --from-author option to override, and perhaps a config setting). This will not change the output *except* when formatting patches authored by someone else. git-am and git-send-email both handle the --from format without any issues.

Before I write such a patch: does anyone see a problem with such a change?

- Josh Triplett

Josh Triplett <josh@joshtriplett.org> writes:

I see this in "format-patch --help":

           Note that this option is only useful if you are actually
           sending the emails and want to identify yourself as the
           sender, but retain the original author (and git am will
           correctly pick up the in-body header).  Note also that
           git send-email already handles this transformation for
           you, and this option should not be used if you are
           feeding the result to git send-email.

The first one says "only useful", but it seems what it really means is "it becomes no-op if you are sending your own patch anyway". So that one does not worry me. What is most worrysome is the latter half of the last sentence. Is it really "should not be", or is it merely "use of this option is just a waste of time, as you would get exactly the same result anyway"? If it is the latter, that is fine.

One thing I absolutely do not want to see is people to start repeating their own ident on in-body "From: " header when they send their own patch. That would waste everybody's time pointing out "You do not have to do that, it merely adds noise". As long as you can guarantee that your change won't increase the rate of that, I am fine with the proposal.

Thanks.

On Thu, Jul 28, 2016 at 02:37:04PM -0700, Junio C Hamano wrote:

It does what you want, and omits the in-body header when it would be redundant.

I think the original reason I did not make "--from" the default is that I was worried about breaking consumers which do not know how to handle in-body headers. "git am" knows how to handle them, but if you have a one-off script that parses only the mail headers, it will start claiming you as the author of every patch.

E.g., if you do:

  git format-patch -o output ...
  grep -hm1 ^From: output/*

right now that gets you a list of patch authors. With "--from", it would return your name N times.

That's obviously a toy, but I wonder if people have scripts which behave similarly.

Another way to think about it is that "--from" is a no-brainer when you really are going to email the patches (and that's why it is has always been the default behavior in git-send-email). But if you _aren't_ going to mail the patches, retaining the original headers is more convenient. It's not clear to me how many non-mail users of format-patch there are (certainly rebase is one of them, but because it uses "am" on the receiving side, I think everything should Just Work).

-Peff

Jeff King <peff@peff.net> writes:

OK, then I would no longer be worried about that one.

That's a fair concern.

So going back to Josh's original problem description:

    While git-send-email knows how to change the patch mails to use your own
    address as "From:" and add a "From:" line to the body for the author,
    any other tool used to send emails doesn't do that.

I wonder how these "any other tool" (that reads the format-patch output, i.e. mbox file with one mail per file each, and sends each as a piece of e-mail, without paying attention who you, the tool's user, are and blindly send them with the original "From:" and other headers intact in the header part of the message) are used in the wild to send patch submissions. /usr/bin/mail or /usr/bin/Mail would not be among them, as I suspect they would place everything in the body part, and the would do so without stripping the "From " line that exists before each e-mail message.

On Thu, Jul 28, 2016 at 03:14:48PM -0700, Junio C Hamano wrote:

mutt -H would be one example; I regularly use that to send mails. (It'll override "From:" if it doesn't know the address in it, which loses the author information entirely; it'll work fine with the --from format.) git-imap-send would be another example; its behavior would vary by mail client. Both of those should always work fine with a mail produced via --from; they'll just ignore the in-body "From:" and send the mail. They'd tend to do the wrong thing with a mail produced without using --from though.

I don't know what people who end up sending From-spoofed mails to LKML are using, but I've seen such mails regularly. I also get occasional blowback from someone who sent such mails including patches I authored with my address spoofed as "From:". And I've also seen someone flamed for sending patches to a mailing list for review with spoofed "From:" addresses.

I can think of aesthetic reasons to want the non-"--from" format (for instance, sticking patch files into a non-git-based tool like quilt or a distribution packaging system, and not wanting your own email address included), but I can't think of any tool that would produce incorrect results if handed the --from format. That seems like an argument for switching the default, and adding a --from-author option or similar to get the current output.

On Thu, Jul 28, 2016 at 04:53:16PM -0700, Josh Triplett wrote:

I covered most of my view elsewhere in the thread, but I want to be clear that I also do not know of any widespread tool that would have a problem. I am mostly worried about breaking people's private scripts.

-Peff

On Thu, Jul 28, 2016 at 03:14:48PM -0700, Junio C Hamano wrote:

I cannot speak for everybody, of course, but the reason I implemented "--from" is because my workflow is basically:

  git format-patch --from --stdout @{u}..HEAD >mbox
  mutt -f mbox

and then I use mutt's "resend" command to send each one. Mutt uses the "From" header written by format-patch as the default (and so I would have to manually move the headers around if not for "--from").

The commands above are wrapped in a script, so I have no problem remembering to type "--from", but I can see how it would be irritating for general use. I would go so far as to say that any time the patches are going to be mailed, that "--from" is the right thing to do (because otherwise you are relying on your MUA to avoid impersonating the original author).

The question in my mind is whether people actually use format-patch for things besides emailing, and if the final destination is something other than "git am". It is a handy format because it is the least-lossy way to move commits around external to git itself. That's why "rebase" used it originally. If the final destination is "am" (as it is for rebase), then in-body headers are OK, because we know it understands those. If not, then it's a regression.

I think on the whole that defaulting to "--from" would help more people than hurt them, but if we do believe there are scripts that would be regressed, it probably needs a deprecation period.

-Peff

On Thu, Jul 28, 2016 at 08:16:19PM -0400, Jeff King wrote:

I don't think it's likely that there are scripts that would be regressed (and I think it's likely that there are scripts that would be progressed), but I'd also have no objection to a deprecation period.

I just confirmed that with the default changed, --no-from works to return to the current behavior, so we don't need a new option. And --no-from has worked for a long time, so scripts won't need to care if they're working with an old version of git.

I can provide a patch implementing a new config option to set the format-patch --from default ("false" for --no-from, "true" for --from, or a string value for --from=value).

Do you think this needs the kind of very noisy deprecation period that push.default had, where anyone without the git-config option set gets a warning to stderr? Or do you think it would suffice to provide a warning in the release notes for a while and then change the default?

On Thu, Jul 28, 2016 at 07:08:02PM -0700, Josh Triplett wrote:

I'm on the fence, so I'd leave the final decision on whether to deal with deprecation to you (who will write the patch) and Junio (as the maintainer).

I'd be surprised if the config option is actually useful to anybody in practice (the distinction is not "this my preference" as much as "in what context am I calling the program?"). It is a convenient way to do deprecations (introduce an option, then flip the default, leaving the option as an escape hatch for anybody who has been broken), though.

IMHO the noisy deprecations haven't really been all that beneficial. Even after the length push.default one, I think we still had people who had skipped enough versions to miss it, and quite a few people became confused or annoyed by the spew of text.

OTOH, I'm not sure most people read the release notes carefully, either. It would be nice if we could make the change and count on people to notice it in 'next' or even 'master' before the release, but empirically that does not happen.

So I dunno. If we consider the risk minor, perhaps a mention in the release notes for version X, and then the change in X+1 would be fine. That gives some opportunity for release-note readers to pipe up. It's not foolproof, but it would give us more confidence.

-Peff

On Fri, Jul 29, 2016 at 06:58:00PM -0400, Jeff King wrote:

OK, see the plan proposed below then.

It also allows people to change their local default in advance of git changing the default.

I would propose the following then:

- I'll write a patch adding a config option format.from, along with
  documentation, without changing the default.
- The release notes for the version of git introducing that config
  option should mention, prominently, the plan to change the default in
  a future version of git.
- A subsequent release can change the default.  No major rush to do
  this.

Does that sound reasonable?

- Josh Triplett

On Fri, Jul 29, 2016 at 09:50:55PM -0700, Josh Triplett wrote:

That sounds fine to me.

I do have to admit that after reading through the "format.*" section of git-config(1), there is quite a bit that is configurable in it. So perhaps we do not need to be as careful about behavior changes as I thought.

So if you wanted to squish steps 2 and 3 together, that would also be OK by me.

-Peff

On Sat, Jul 30, 2016 at 01:47:42AM -0400, Jeff King wrote:

I'll send two separate patches, and I'll leave it up to Junio whether to apply the second one right away or wait a release.

As discussed, this patch series allows transitioning the default behavior of format-patch to --from, to avoid spoofing mails when formatting commits not written by the user.

The first patch introduces the format.from option to set the default value of format-patch --from. This patch doesn't change the default behavior of format-patch, so it can go in without any transition.

The second patch changes the default to --from. If you'd like to delay this patch for a release and mention the planned change in the release notes, let me know and I'll provide text for the release notes; if you don't think this needs a transition period, you can go ahead and apply the second patch.

Josh Triplett (2):
  format-patch: Add a config option format.from to set the default for --from
  format-patch: Default to --from

 Documentation/config.txt               | 10 ++++-
 builtin/log.c                          | 14 +++++-
 contrib/completion/git-completion.bash |  1 +-
 t/t4014-format-patch.sh                | 68 +++++++++++++++++++++++++--
 4 files changed, 89 insertions(+), 4 deletions(-)

Jeff King <peff@peff.net> writes:

To me, too.

I am not sure how the first sentence (which I agree with; a random user can have quite a different behaviour configured when the command is run without any option) leads to the conclusion in the second sentence. The user can break assumptions made by a tool that reads format-patch output by tweaking his config but at least he knows that he changed the configuration, i.e. the breakage can be explained and attributed to his own action. The change in the default is somewhat different.

When we _know_ we are going to be changing the default, we should forewarn in previous releases (in release notes, and perhaps we would want to have a runtime warning when the user formats others' changes without having format.from explicitly set to either true or false).

So the second step can be delayed and does not have to be done for the release that includes the first change. But I am not sure how "there are many format.* configuration" leads to "we just announce that we changed the default and tell peole there is a new knob to retain the original behaviour".

On Mon, Aug 01, 2016 at 10:35:22AM -0700, Junio C Hamano wrote:

I half-agree. Config that causes unpredictable behavior can break somebody else's script that you are running. If you say "oh, I guess I shouldn't set that config" and move on with your life, then the config hasn't really hurt anybody. If you complain to the script author that their script is broken, and insist that they pass the --no-foo option, then the script writer does not care much whether it was a config option or a change of default.

-Peff

PS We also changed the default pager behavior for format-patch recently,
   though I can't actually think of any script regressions that would
   cause, since it only kicks in when output is going to the tty.

Jeff King <peff@peff.net> writes:

Hmph, I never considered that POV but you are right.

I am not sure if that argues that we have to be even more careful not to add non-essential configuration variable, or that we can afford to be much less careful when changing the default, though. A little bit of both, I guess.

On Thu, Jul 28, 2016 at 05:56:03PM -0400, Jeff King wrote:

I've seen various people use git format-patch to produce a patch file to email around, or a patch file to commit to a patches directory in a distribution package. I don't think any such tools would work incorrectly with the --from output, though for purely aesthetic reasons, I can imagine not wanting to include your own email address when not sending the patch by email.

On Thu, Jul 28, 2016 at 02:37:04PM -0700, Junio C Hamano wrote:

As far as I can tell, it's the latter. git send-email can do this same transformation, but handles mails that already have the transformation done to them without any issue.

git format-patch with --from *only* adds an in-body "From:" if the commit author differs from the local committer identity. So, as far as I can tell, the only scenario that would produce additional in-body "From:" headers here would be if someone had failed to configure their git identity, and manually set the author for their own commits. (In which case, they'd also have a broken "From:" in any cover letter they generated.)

So, it seems exceedingly unlikely to me that this would result in unnecessary in-body "From:" headers.

- Josh Triplett

Josh Triplett <josh@joshtriplett.org> writes:

Yup, Peff elsewhere on the thread gave the same conclusion, and after reading the codepaths involved again, I agree.