Re: [PATCH v2] fetch-pack: redact packfile urls in traces

It seems I sent my original reply only to the github PR. Sorry for the
confusion:

On Mon, Oct 11, 2021 at 1:39 PM Junio C Hamano [off-list ref] wrote:

"Ivan Frade via GitGitGadget" [off-list ref] writes:

quoted

From: Ivan Frade <redacted>

...

It of course is a different matter if the explained idea is
agreeable, though ;-).  Hiding the entire packet, based on the "it
might be in some setups" seems a bit too much.

Is it often the case that the whole URI is sensitive, or perhaps
leading "<scheme>://<host>/pack-<abc>.pack" part is not sensitive at
all, and what follows after that "public" part has some "nonce"
material that makes it sensitive?

In the specific case I am working on, the path of the URL is an
encrypted string that shouldn't be completely exposed (exposing part
of it would be fine). In general, I think we can assume that
<scheme>://<host>/ are always "public" but the path could be
sensitive.

We could redact only the path (<scheme>://<host>/REDACTED), or even a
fixed length of the URL? (<scheme>://<host>/pack-<xxREDACTED).

In the next patch version I go with redacting the path.

quoted

Changes since v1:

...

 Please write such material below the three-dash line.

Done

And there is no need to duplicate the log message here ;-)

Done

So "original_options" is used to save away the reader->options so
that it can be restored before returning to our caller?

OK (it may be more common in this codebase to call such a variable
"saved_X", though).

In the latest iteration, the option is enabled for all sections and
there is no need to set/unset the flag.

quoted

+     grep "clone< <redacted>" log

This checks only that "redacted" string appears, but what the theme
of the change really cares about is different, no?  You want to
ensure that no sensitive substring of the URI appears in the log.

Imagine somebody breaking the redact logic by making it prepend that
string to the payload, instead of replacing the payload with that
string---this test will not catch such a regression.

Now the tests verify the expected packfile-uri full line is in the log.

Thanks,

Ivan