Re: [PATCH] t/lib-gpg: adjust permissions for gnupg 2.1

Jeff King schrieb am 02.12.2014 um 22:07:

On Tue, Dec 02, 2014 at 02:40:27PM +0100, Michael J Gruber wrote:

quoted

Before gnupg 2.1 (aka "modern branch"), gpghome would contain only files
which allowed t/lib-gpg.sh to set permissions explicitely, and we did
that since
28a1b07 (t/lib-gpg: adjust permissions for gnupg 2.1, 2014-12-02)
in order to adjust wrong permissions from a checkout on ro file systems.

I think this 28a1b07 is wrong. Did you mean e7f224f?

Ouch, I'm afraid I got that from my copy of the commit. After all, there
is some truth in the patch/apply vs. push/pull-merge workflow discussion...

quoted

gnupg 2.1 creates a new directory in gpghome which would get its x bit removed.

Thanks for digging in this. The story is a little more tricky, though,
and I do not think this patch is strictly necessary.

We copy lib-gpg/* to the trash directory, and only run gpg on it there.
So it is there that gpg2.1 will munge the files, _after_ we have
copied and done our chmod. And that works fine with the current code.

Right. I should'nt draft patches when I don't have gpg2.1...

The problem came when I was trying to test/debug, and outside of the
tests did "cd lib-gpg && gpg2 ...". That munged my lib-gpg directory,
and the resulting breakage was copied into each subsequent trash
directory.

So while your patch is not necessary, it is a nice defense against this
sort of manual munging, or against future patches which add more
directories. But...

quoted

Adjust and use +X so that any directory would get its x bit set. This
also keeps the x bit on files which had it set for whatever wrong
reason, but we care only about having at least the necessary
permissions for the tests to run.

Taking a step back, though, I am not sure I understand the reasoning
behind the original e7f224f. The rationale in the commit message is that
we want to make sure that the files are writable. But why would they not
be? They are created by "cp -R", so unless your umask does not allow the
owner to write to the files, they should be writable, no? And if your
umask is set that way, lots of things are going to break.

IIRC: We had reports about some ro checkouts where the tests went wrong.
I.e., the git.git checkout was on a ro file system, and the cp somehow
created ro files in the tmp. Or was it the archive/pax issue?

In any case, we need writable gpghome (and contents), and the original
patch ensured that.

You messed with the wrong parts in gpghome :)

I do think the original "0700" chmod _is_ useful, though. But not
because it makes sure things are writable, but because it makes sure
that it is _not_ world-readable. GPG complains about the lax permissions
(of course it does not know that the keyrings are not really secrets in
this case). However, this does not actually prevent the tests from
running successfully.

So from my perspective, the simplest thing is to keep the original
"chmod 0700" for that reason (or make it "chmod go-rwx", if you like),
and drop the inner chmod completely (effectively reverting e7f224f). But
again, perhaps there is some case that it covers that I do not
understand.

-Peff

I would say the "modern branch" of gpg2.1 is still a bit unstable. If
the tests do run with it (as they seem to do, unless you mess with the
source of the cp) there are no permission fixes to do.

Orthogonal to that is the pinentry issue: I haven't checked whether
gpg2.1 asking for passphrases on passphrase-less secure keys is to be
fixed on the gpg side. If yes, I would just wait for that since gpg2.1
is not common yet.

If not, we should provide (in gpg config) an alternative pinentry that
just returns an empty passphrase without bugging the user.

Michael