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

From: Ivan Frade <ifrade@google.com>
Date: Oct 26, 2021, 19:32 UTC
Message-ID: <CANQMx9W_Zt+qy3sppx1qGdf6S9gMSEp_7jjV4hc_aeyR62syrQ@mail.gmail.com>
In-Reply-To: <xmqqczobb8jd.fsf@gitster.g>

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 <gitster@pobox.com> wrote:

>
> "Ivan Frade via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Ivan Frade <ifrade@google.com>

...

Show 9 quoted lines

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

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

Show 5 quoted lines

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

Show 9 quoted lines

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