git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/3] wincred: ignore unknown lines (do not die)

From
Jeff King <peff@peff.net>
Date
Sep 22, 2022, 22:09 UTC
Message-ID
<YyzdFBaiWgCJzMWX@coredump.intra.peff.net>
In-Reply-To
<xmqqbkr7xg28.fsf@gitster.g>
On Thu, Sep 22, 2022 at 02:19:43PM -0700, Junio C Hamano wrote:
Show 26 quoted lines
> > It is the expectation that credential helpers be liberal in what they
> > accept and conservative in what they return, to allow for future growth
> > and evolution of the protocol/interaction.
> 
> That is nice in principle, and the updated code below may work well
> with existing "other side of the connection" (codepaths in "git"
> that asks credential API to talk to the helpers), but I am not sure
> if this is always a safe thing to do.
> 
> When we gain a new "command" in the protocol, if we just read it
> without understanding it, would we open ourselves to a risk of
> breaking the protocol communication, worst of which may be to
> deadlock?  A new command, when received by a more recent helper that
> understands how to react to it, may _require_ it to write more than
> "username" and "password" back to "git" from get_credential(), for
> example, but the helper with this patch alone, while not complaining
> about seeing such a new and unknown command, would not know how to
> compute and write that third thing other than "username" and
> "password"---would the other side who issued that new command get
> stuck waiting for us to return the third thing?  Worse yet, the new
> command may expect us to read further in get_credential()
> (e.g. maybe they will give us a challenge, which may need to be used
> when yielding the "username" and "password" things), but because we
> do not even know we need to read, their attempt to write to us may
> get them stuck, and that is when we are expecting to write to them,
> easily leading to a deadlock, no?

This open-endedness was an intentional part of the original credential-helper design. Helpers are always "best effort", and it is OK if they ignore a request, or return a partial result. If the sending side really wants to extend the protocol in a way that the other side doesn't act at all (say, they "username" to be used _only_ if the helper also understands a new "foobar" key), then they should be adding the new fields as an atomic unit. I.e., "foobar", "foobar-userame", and so on. And then existing helpers which don't understand the new feature will just ignore it totally.

In practice, I think it's not that big a deal either way, because the setup of these helpers is under the control of the user. So if you really want to use "foobar" but a helper doesn't support it, then you'd just remove that helper from your config.

-Peff
Previous: Junio C HamanoNext: Matthew John Cheetham via GitGitGadget
Message 4 of 8 in “Correct credential helper discrepancies handling input”
  1. 0/3 Correct credential helper discrepancies handling inputMatthew John Cheetham via GitGitGadget, Sep 22, 2022
  2. 1/3 wincred: ignore unknown lines (do not die)Matthew John Cheetham via GitGitGadget, Sep 22, 2022
  3. Junio C HamanoSep 22, 2022
  4. Jeff KingSep 22, 2022
  5. 2/3 netrc: ignore unknown lines (do not die)Matthew John Cheetham via GitGitGadget, Sep 22, 2022
  6. 3/3 osxkeychain: clarify that we ignore unknown linesMatthew John Cheetham via GitGitGadget, Sep 22, 2022
  7. Derrick StoleeSep 22, 2022
  8. Jeff KingSep 22, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.