| Message ID | 20211105193106.3195-1-adam@dinwoodie.org (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f5ff9c35ad923f987f380660541946db58cc77cf |
| Headers | show |
| Series | [v2] t/lib-git.sh: fix ACL-related permissions failure | expand |
Adam Dinwoodie <adam@dinwoodie.org> writes: > As well as checking that the relevant functionality is available, the > GPGSSH prerequisite check creates the SSH keys that are used by the test > functions it gates. If these keys are created in a directory that > has a default Access Control List, the key files can inherit those > permissions. > > This can result in a scenario where the private keys are created > successfully, so the prerequisite check passes and the tests are run, > but the key files have permissions that are too permissive, meaning > OpenSSH will refuse to load them and the tests will fail. That may indicate that "private keys are created successfully" is a bit too optimistic. A key that did not exist but now exists indeed was created, but if it cannot be used in tests, calling it "successfully created" is a bit too charitable, I would say. ... where the private keys appear to have been created successfully, but at the runtime OpenSSH will refuse to load these keys due to permissions that are too loose. In other words, the keys created here are not usable. Yet the lazy_prereq is set, pretending all is well, and makes the real tests fail. And when described that way, we'd realize that "setfacl -k" solution may be closing one known way that a key, that seemingly was created successfully, can be unusable in real tests, but it is not addressing the root cause of the breakage you observed---the lazy_prereq is not set based on what really matters, i.e. is the key usable to sign and verify? > To avoid this happening, before creating the keys, clear any default ACL "happening" -> "from happening" > set on the directory that will contain them. This step allowed to fail; "allowed" -> "is allowed". > if setfacl isn't present, that's a very likely indicator that the > filesystem in question simply doesn't support default ACLs. True. Or setfacl command fails to futz with the ACL for whatever reason, in which case you may still have the "we 'successfully' created a key, but it turns out that it was unusable in real tests" problem. As long as the lazy_prereq is not set to pretend that all is well, we won't see test breakage noise that distracts those who are watching for breakage due to "git". And that is why we want to add "is the key really usable" check before the lazy_prereq declares a success. > Helped-by: Fabian Stelzer <fs@gigacodes.de> > Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org> > --- > t/lib-gpg.sh | 1 + > 1 file changed, 1 insertion(+) Other than that, the above explanation reads well. Thanks. > > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh > index f99ef3e859..1d8e5b5b7e 100644 > --- a/t/lib-gpg.sh > +++ b/t/lib-gpg.sh > @@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH ' > test $? = 0 || exit 1; > mkdir -p "${GNUPGHOME}" && > chmod 0700 "${GNUPGHOME}" && > + (setfacl -k "${GNUPGHOME}" 2>/dev/null || true) && > ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && > echo "\"principal with number 1\" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && > ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null &&
> > > To avoid this happening, before creating the keys, clear any default > > ACL > > "happening" -> "from happening" > No, original is correct. To avoid this happening. To keep this from happening. To prevent this happening. To prevent this from happening. Would I think all be correct. "to avoid from" is not right. Regards, Richard,
"Kerry, Richard" <richard.kerry@atos.net> writes: >> >> > To avoid this happening, before creating the keys, clear any default >> > ACL >> >> "happening" -> "from happening" >> > > No, original is correct. > > To avoid this happening. > To keep this from happening. > To prevent this happening. > To prevent this from happening. > > Would I think all be correct. > "to avoid from" is not right. But I meant to say "to avoid this from happening", not "to avoid from", which I gree is not right.
> -----Original Message----- > From: Junio C Hamano <gitster@pobox.com> > Sent: 08 November 2021 19:15 > To: Kerry, Richard <richard.kerry@atos.net> > Cc: Adam Dinwoodie <adam@dinwoodie.org>; git@vger.kernel.org; Fabian > Stelzer <fs@gigacodes.de> > Subject: Re: [PATCH v2] t/lib-git.sh: fix ACL-related permissions failure > > Caution! External email. Do not open attachments or click links, unless this > email comes from a known sender and you know the content is safe. > > "Kerry, Richard" <richard.kerry@atos.net> writes: > > >> > >> > To avoid this happening, before creating the keys, clear any > >> > default ACL > >> > >> "happening" -> "from happening" > >> > > > > No, original is correct. > > > > To avoid this happening. > > To keep this from happening. > > To prevent this happening. > > To prevent this from happening. > > > > Would I think all be correct. > > "to avoid from" is not right. > > But I meant to say "to avoid this from happening", not "to avoid from", which > I agree is not right. "to avoid this from happening" is wrong. "to avoid this happening" is right. Or my other examples, with more or less the same meaning. I phrased it as "to avoid from" as an example of the verb in its basic form. You were entirely clear what you meant - I was merely trying to give examples of what I think is wrong. As a native English speaker I grew up without being taught formal grammar, so I can say something is wrong without being able to explain why in a formal way..... I'd guess from his name that Adam is also a native English speaker. Regards, Richard.
"Kerry, Richard" <richard.kerry@atos.net> writes: > "to avoid this from happening" is wrong. > "to avoid this happening" is right. Ah, of course. I somehow mixed it up with "to prevent".
diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index f99ef3e859..1d8e5b5b7e 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -106,6 +106,7 @@ test_lazy_prereq GPGSSH ' test $? = 0 || exit 1; mkdir -p "${GNUPGHOME}" && chmod 0700 "${GNUPGHOME}" && + (setfacl -k "${GNUPGHOME}" 2>/dev/null || true) && ssh-keygen -t ed25519 -N "" -C "git ed25519 key" -f "${GPGSSH_KEY_PRIMARY}" >/dev/null && echo "\"principal with number 1\" $(cat "${GPGSSH_KEY_PRIMARY}.pub")" >> "${GPGSSH_ALLOWED_SIGNERS}" && ssh-keygen -t rsa -b 2048 -N "" -C "git rsa2048 key" -f "${GPGSSH_KEY_SECONDARY}" >/dev/null &&
As well as checking that the relevant functionality is available, the GPGSSH prerequisite check creates the SSH keys that are used by the test functions it gates. If these keys are created in a directory that has a default Access Control List, the key files can inherit those permissions. This can result in a scenario where the private keys are created successfully, so the prerequisite check passes and the tests are run, but the key files have permissions that are too permissive, meaning OpenSSH will refuse to load them and the tests will fail. To avoid this happening, before creating the keys, clear any default ACL set on the directory that will contain them. This step allowed to fail; if setfacl isn't present, that's a very likely indicator that the filesystem in question simply doesn't support default ACLs. Helped-by: Fabian Stelzer <fs@gigacodes.de> Signed-off-by: Adam Dinwoodie <adam@dinwoodie.org> --- t/lib-gpg.sh | 1 + 1 file changed, 1 insertion(+)