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

From: Fabian Stelzer <fs@gigacodes.de>
Date: Nov 5, 2021, 12:13 UTC
Message-ID: <20211105121323.6fbyxoibaeyeshtw@fs>
In-Reply-To: <YYUeKt0xQm/6QT+w@coredump.intra.peff.net>

On 05.11.2021 08:06, Jeff King wrote:

Show 31 quoted lines

>On Fri, Nov 05, 2021 at 11:25:25AM +0000, Adam Dinwoodie wrote:
>
>> > ... 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 issue is that the prerequisite check isn't _just_ checking a
>> prerequisite: it's also creating an SSH key that's used without further
>> modification by the tests.
>
>This is sort of a side note to your main issue, but I think that relying
>on a lazy_prereq for side effects is an anti-pattern. We make no
>promises about when or how often the prereqs might be run, and we try to
>insulate them from the main tests (by putting them in a subshell and
>switching their cwd).
>
>It does happen to work here because the prereq script writes directly to
>$GNUPGHOME, and we run the lazy prereqs about when you'd expect. So I
>don't think it's really in any danger of breaking, but it is definitely
>not using the feature as it was intended. :)
>
>I think the more usual way would be to have an actual
>test_expect_success block that creates the keys as a setup step
>(possibly triggered by a function, since it's included via lib-gpg.sh).
>If we don't want to decide whether we have the GPGSSH prereq until then,
>then that test can call test_set_prereq. See the LONG_REF case in t1401
>for an example.

I was not aware of this. I assumed prereq not just meant checking for pre requisites but also setting them up. Since i still have a follow up patch series in progress i will keep that in mind and move the actual setup code into a function in lib-gpg. There are gpg ssh tests in multiple different test files so i didn't want to create keys for each of them repeatedly. And i'm not a fan of checking in those file (like the gpg keyring for testing) since it also needs documentation on how it was generated that is not quaranteed to match how it was done.

We still could add the actual sign test into the prereq for now. A `echo "test" | ssh-keygen -Y sign -f $GPGSSHKEY_PRIMARY -n "git"` will make sure that the keys actually work.

Show 24 quoted lines

>
>Again, that's mostly a tangent to your issue, and maybe not worth
>futzing with at this point in the release cycle. I'm mostly just
>registering my surprise. ;)
>
>> There are three cases to consider:
>>
>> - On systems where this prerequisite check fails, a key may or may not
>>   be created, but the tests that rely on the key won't be run, so it
>>   doesn't matter either way.
>>
>> - On (clearly the mainline) systems where this check passes and there
>>   are no ACL problems, the key that's generated is stored with
>>   sufficiently restrictive permissions that the tests that rely on the
>>   key can pass.
>>
>> - On my system, where ACLs are a problem, the prerequisite check passes,
>>   and a key is created, but it has permissions that are too permissive.
>>   As a result, when a test calls OpenSSH to use that key, OpenSSH
>>   refuses due to the permissions, and the test fails.
>
>FWIW, that explanation makes perfect sense to me (and your patch seems
>like the right thing to do).
>

>-Peff