Re: [PATCH 2/2] http, imap-send: stop using CURLOPT_VERBOSE

From: Jeff King <peff@peff.net>
Date: May 13, 2020, 04:50 UTC
Message-ID: <20200513045001.GA65110@coredump.intra.peff.net>
In-Reply-To: <xmqqwo5gwv7z.fsf@gitster.c.googlers.com>

On Tue, May 12, 2020 at 05:10:24PM -0700, Junio C Hamano wrote:

Show 21 quoted lines

> > On 2020-05-11 at 17:43:10, Jonathan Tan wrote:
> >> Whenever GIT_CURL_VERBOSE is set, teach Git to behave as if
> >> GIT_TRACE_CURL=1 and GIT_TRACE_CURL_NO_DATA=1 is set, instead of setting
> >> CURLOPT_VERBOSE.
> >> 
> >> This is to prevent inadvertent revelation of sensitive data. In
> >> particular, GIT_CURL_VERBOSE redacts neither the "Authorization" header
> >> nor any cookies specified by GIT_REDACT_COOKIES.
> >
> > I actually use GIT_CURL_VERBOSE to debug authentication problems from
> > time to time, so I'd like to keep an option to produce full, unredacted
> > output.  Since everyone uses HTTPS, it's not possible to perform this
> > debugging using a tool like Wireshark unless you use a MITM CA cert,
> > which seems excessive.
> 
> Hmm, that is a valid concern.  Introducing yet another environment
> feels a bit yucky, but something like GIT_NO_REDACT that disables
> any redacting, not limited to curl but in all codepaths, might turn
> out to be a useful escape hatch.
> 
> Opinions?

Having an environment variable was my first thought, as well. I do think it's key that the default be to redact. That makes life slightly harder for people debugging auth problems, but prevents people from accidentally leaking private info.

Regarding the name:

  - should it be under GIT_TRACE_CURL_* to make its impact clear? Or do
    we imagine it might eventually be applied elsewhere?

  - doing GIT_TRACE_REDACT would get rid of the negative (and it could
    just default to "true")

-Peff