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

On Thu, Apr 28, 2016 at 6:49 AM, Jeff King [off-list ref] wrote:

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

quoted

quoted

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

quoted

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