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

From: Jeff King <peff@peff.net>
Date: Apr 28, 2016, 13:49 UTC
Message-ID: <20160428134953.GB25364@sigill.intra.peff.net>
In-Reply-To: <alpine.DEB.2.20.1604281405540.2896@virtualbox>

On Thu, Apr 28, 2016 at 02:19:37PM +0200, Johannes Schindelin wrote:

Show 22 quoted lines

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

Show 14 quoted lines

> 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