Re: [PATCH v3 1/5] Documentation: define protected configuration

From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <derrickstolee@github.com>
Cc: Glen Choo via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org, Taylor Blau <me@ttaylorr.com>,
	"brian m. carlson" <sandals@crustytoothpaste.net>,
	Emily Shaffer <emilyshaffer@google.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v3 1/5] Documentation: define protected configuration
Date: Thu, 02 Jun 2022 09:53:58 -0700	[thread overview]
Message-ID: <xmqq35gnovkp.fsf@gitster.g> (raw)
In-Reply-To: <2ed00ece-f8bb-c84c-0684-494692a71da2@github.com> (Derrick Stolee's message of "Thu, 2 Jun 2022 08:42:30 -0400")

Derrick Stolee <derrickstolee@github.com> writes:

>> It would make sense to give a rationale behind the seemingly
>> arbitrary choice of what is and what is not "protected".  Not
>> necessarily in the glossary, but in the proposed log message of the
>> commit that makes the decision.  The rationale must help readers to
>> be able to answer the following questions.
>> 
>>  - The system level is "protected" because?  Is it because we do not
>>    even try to protect ourselves from those who can write anywhere
>>    in /etc/ or other system directories?
>> 
>>  - The per-user config is "protected" because?  Is it because our
>>    primary interest in "protection" is to protect individual users
>>    from landmines laid in the filesystem by other users, and those
>>    who can already write into $HOME are not we try to guard against?
>
> I think the answers to these two questions is "yes", so they can
> be turned into an affirmative sentence:
>
> 	We do not event try to protect ourselves from those who can
> 	write anywhere...

s/event/even/.

>
>>  - The per-repo config is not "protected" (i.e. "trusted"), because?
>>    If we are not honoring a configuration in the repository, why are
>>    we working in that repository in the first place?
>
> This requires an example:
>
> 	Some workflows use repositories stored in shared directories,
> 	which are writable by multiple unprivileged users.

Hmph, "... and we do not trust these colleagues"?  It might be true,
but sounds a bit weak rationale, at least to me.  A natural reaction
coming form a devil's advocate naïve me would be "well, then I would
not be directly interacting with such a repository; I'd work in a
clone of it of my own, and pull and push as needed".

Isn't the reason more like "users may go spelunking random places in
the filesystem, with PS1 settings and the like that causes some
"git" command invoked automatically in their current directory, and
we want to protect these users from getting harmed by a random
repository with hostile contents in their configuration and hooks
without even realizing they have wandered into such a repository"?

>>  - The per invocation config is not "protected" (i.e. "trusted"),
>>    because?  If we cannot trusting our own command line, what
>>    prevents an attacker from mucking with our command line to say
>>    "sudo whatever" using the same attack vector?
>
> With this argument, I agree that -c config can be considered
> protected. At the very least, it is visible to the user when they
> are running a command. This would unify our expectations with
> uploadPack.packObjectsHook, too.

Yup, that matches my understanding.

In any case, I'd prefer to see not just the definition but the
reasoning behind the decision that made some "protected" while
leaving others not-"protected" clearly documented to help users.

Thanks.