Re: [PATCH] t/lib-git.sh: fix ACL-related permissions failure

From: Fabian Stelzer <fs@gigacodes.de>
Date: Nov 4, 2021, 22:36 UTC
Message-ID: <20211104223633.5j556ggfga43myz5@fs>
In-Reply-To: <xmqqzgqj1yff.fsf@gitster.g>

On 04.11.2021 13:03, Junio C Hamano wrote:

Show 28 quoted lines

>Junio C Hamano <gitster@pobox.com> writes:
>
>>> This change is required in particular to avoid tests relating to SSH
>>> signing failing in Cygwin.
>>
>> ... I am not quite sure how this explains "tests relating to ssh
>> signing failing on Cygwin".  After all, this piece of code is
>> lazy_prereq, which means that ssh-keygen in this block that fails
>> (due to a less restrictive permissions) would merely mean that tests
>> that are protected with GPGSSH prerequisite will be skipped without
>> causing test failures.  After all that is the whole point of
>> computing prereq on the fly.
>
>The reason why I wondered about the above is that it can be an
>indication of another breakage, namely, that we may have tests that
>require a working ssh-keygen but are by mistake not protected with
>GPGSSH prerequisite.
>
>The test_lazy_prereq block you touched may refrain from setting the
>prerequisite on your system (due to the faulty test here that you
>touched), but if we had such unprotected tests, we still will run
>ssh signing tests and they would fail, due to the lack of the
>prerequisite.
>
>And fixing the prereq block alone will hide that other breakage, at
>least on your system.  Hence my question.
>
>Thanks.

The problem is that the ssh-keygen in the layz_prereq will succeed but might create a private key with world readable permissions. Only the remaining tests using this key will then fail with a "your private key permissions are too restrictive" like error. If we would like to make sure in the prereq that the keys actually work fine we would need to do a signing operation with them in it.

Something like the following call would be enough: echo "test" | ssh-keygen -Y sign -f $GPGSSHKEY_PRIMARY -n "git"

Not sure if we want to go that far though. The setfacl seems fine to me otherwise.