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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]