Re: [PATCH v5 2/2] submodule: pass on http.extraheader config settings

[Jacob Keller <jacob.keller@gmail.com>]

On Thu, Apr 28, 2016 at 6:49 AM, Jeff King <peff@peff.net> wrote:
> On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:
>
>> > Should we consider just white-listing all of "http.*"?
>> >
>> > That would help other cases which have come up, like:
>> >
>> >   http://thread.gmane.org/gmane.comp.version-control.git/264840
>> >
>> > which wants to turn off http.sslverify. That would mean it turns off for
>> > every submodule, too, but if you want to be choosy about your http
>> > variables, you should be using the "http.$URL.sslverify" form, to only
>> > affect specific servers (whether they are in submodules or not).
>>
>> I considered that, and thought that it might be dangerous, what with me
>> not vetting carefully which http.* variables are safe to pass on to the
>> submodules' update and which are not.
>>
>> So I had a look now, and the most prominent potential problem is the
>> http.cookieFile setting, which could be reused all of a sudden if we
>> made my patch more general.
>>
>> But then, we are talking about the code that filters what gets passed via
>> the *command-line*. And to be quite honest, I am not sure that we should
>> actually filter out *any* of these settings.
>
> The intent of the whitelist (from my recollection of the discussion) is
> to filter out config that must be repo-specific. E.g., core.worktree or
> core.bare should definitely _not_ be passed to a submodule.
>
> I don't know if there are others. We started with a whitelist because it
> was the smallest and safest change away from the status quo. A blacklist
> would also work, with the risk that we might let through nonsense in
> some cases (but only if the user triggers us to do so).
>
>> The commit message that introduced this particular filtering has this
>> rationale to let only credential.* through:
>>
>>     GIT_CONFIG_PARAMETERS is special, and we actually do want to
>>     preserve these settings. However, we do not want to preserve all
>>     configuration as many things should be left specific to the parent
>>     project.
>>
>>     Add a git submodule--helper function, sanitize-config, which shall be
>>     used to sanitize GIT_CONFIG_PARAMETERS, removing all key/value pairs
>>     except a small subset that are known to be safe and necessary.
>>
>> Dunno. I tried to err on the side of caution... But this sounds maybe a
>> bit *too* cautious?
>
> So if we all agree that the sanitizing is really about preventing
> repo-specific variables from leaking, and not any kind of security
> boundary, I think we should generally be pretty liberal in whitelisting
> things.
>
> I can certainly come up with a pathological case where using it as a
> security boundary may have some practical use, but in general I think it
> is mostly getting in the way of what users are trying to do.
>
> -Peff

I think I prefer a blacklist approach, since it reduces the need for
future changes, since most cases will either not put config on the
environment or (based on feedback on the mailing list and bug reports)
the user will believe it should be applied.

A black list which only removed configurations we know are harmful
would be easier to maintain but risks new additions forgetting to do
so. A whitelist means we only fix things as they come up but also
means we aren't "breaking" anything that works today, where as a
blacklist could break something that works today.

Thanks,
Jake