bug#56444: Gitolite home directory permissions

bug#56444: Gitolite home directory permissions

[Evgeny Pisemsky]

  Hello!

  I wanted to serve public repositories from gitolite using git-daemon.

  I tried the following configuration of services:

  ┌────
  │ (define git-daemon
  │   (git-daemon-service
  │    #:config (git-daemon-configuration
  │              (base-path "/var/lib/gitolite/repositories"))))
  │ 
  │ (define gitolite
  │   (service gitolite-service-type
  │            (gitolite-configuration
  │             (admin-pubkey user-key)
  │             (group "git-daemon")
  │             (rc-file (gitolite-rc-file
  │                       (umask #o0027))))))
  └────

  However despite setting the umask the `/var/lib/gitolite' directory
  gets the `drwx------' permissions that makes it inaccessible for the
  git-daemon.

  If I set the group permissions manually and restart the git-daemon
  everything works fine until the next system reboot, which resets the
  permissions to the above value.

bug#56444: Acknowledgement (Gitolite home directory permissions)

[Evgeny Pisemsky]

After some digging I came up to the procedure `activate-users+groups' in
the file `gnu/build/activation.scm' and found the following lines:

┌────
│ ;; Always set ownership and permissions for home directories of system
│ ;; accounts.  If a service needs looser permissions on its home
│ ;; directories, it can always chmod it in an activation snippet.
│ (chown home (passwd:uid pwd) (passwd:gid pwd))
│ (chmod home #o700)))
└────

So it looks like the case for gitolite activation procedure - it should
chmod the home directory with respect to the umask value.

bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1.1: Type: text/plain, Size: 718 bytes --]

Hi Evgeny and whoever wants to do some code review,

I have been experiencing this same issue for years now and have been
manually chmod'ing /var/lib/gitolite every time I upgraded because I didn't
understand what was happening.  All this time I thought I had gitolite
misconfigured, that maybe I didn't have its umask config set properly, but
it was Guix all along! In this case that's great, because it makes the
problem easy for me to fix.  Patch attached.  It works like a charm for my
personal git server (https://git.dthompson.us), /var/lib/gitolite was 700
before a system reconfigure, and 750 afterwards.

Big thanks to Evgeny for making a bug report and doing the research to
identify the root cause!

- Dave

[-- Attachment #1.2: Type: text/html, Size: 889 bytes --]

[-- Attachment #2: 0001-services-gitolite-Relax-permissions-on-service-user-.patch --]
[-- Type: text/x-patch, Size: 1710 bytes --]

From f35cb018df8498db45689dc0e9800b99008a9dea Mon Sep 17 00:00:00 2001
From: David Thompson <dthompson2@worcester.edu>
Date: Fri, 19 Aug 2022 09:20:06 -0400
Subject: [PATCH] services: gitolite: Relax permissions on service user home
 directory.

Fixes https://issues.guix.gnu.org/56444

* gnu/services/version-control.scm (gitolite-activation): Modify permissions
  on home directory so that git group has read access.

Reported-by: Evgeny Pisemsky <evgeny@pisemsky.com>

Experienced by David Thompson for years, wondering what was wrong. Thanks for
finding the root cause, Evgeny! :)
---
 gnu/services/version-control.scm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/gnu/services/version-control.scm b/gnu/services/version-control.scm
index defbd65c36..17a5f9c867 100644
--- a/gnu/services/version-control.scm
+++ b/gnu/services/version-control.scm
@@ -331,6 +331,14 @@ access to exported repositories under @file{/srv/git}."
                                (strip-store-file-name admin-pubkey))))
                 (rc-file #$(string-append home "/.gitolite.rc")))
 
+           ;; activate-users+groups in (gnu build activation) sets the
+           ;; permission flags of home directories to #o700 and mentions that
+           ;; services needing looser permissions should chmod it during
+           ;; service activation.  We also want the git group to be able to
+           ;; read from the gitolite home directory, so a chmod'ing we will
+           ;; go!
+           (chmod #$home #o750)
+
            (simple-format #t "guix: gitolite: installing ~A\n" #$rc-file)
            (copy-file #$rc-file rc-file)
            ;; ensure gitolite's user can read the configuration
-- 
2.25.1

bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1145 bytes --]


On 19-08-2022 15:32, Thompson, David wrote:
> Hi Evgeny and whoever wants to do some code review,
>
> I have been experiencing this same issue for years now and have been 
> manually chmod'ing /var/lib/gitolite every time I upgraded because I 
> didn't understand what was happening.  All this time I thought I had 
> gitolite misconfigured, that maybe I didn't have its umask config set 
> properly, but it was Guix all along! In this case that's great, 
> because it makes the problem easy for me to fix.  Patch attached.  It 
> works like a charm for my personal git server 
> (https://git.dthompson.us), /var/lib/gitolite was 700 before a system 
> reconfigure, and 750 afterwards.
>
> Big thanks to Evgeny for making a bug report and doing the research to 
> identify the root cause!
>
> - Dave

During "guix system reconfigure", there is now window where the 
directory temporarily has incorrect bits and hence if gitolite is 
restarted during that time it will presumably fail.  Could a 
'home-permission-bits' or such field be added instead to <user-account> 
to make things atomic?

Greetings,
Maxime.


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 702 bytes --]

Hi Maxime,

On Tue, Aug 23, 2022, 8:41 AM Maxime Devos <maximedevos@telenet.be> wrote:

>
> During "guix system reconfigure", there is now window where the
> directory temporarily has incorrect bits and hence if gitolite is
> restarted during that time it will presumably fail.  Could a
> 'home-permission-bits' or such field be added instead to <user-account>
> to make things atomic?
>

That would be a nice improvement to backlog now that such a use case has
emerged. However, I think for our immediate needs this one line patch,
while imperfect, solves a longstanding problem adequately. So how about
merging it, closing this bug, and opening a new bug for the system level
improvement?

- Dave

>

[-- Attachment #2: Type: text/html, Size: 1360 bytes --]

bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 922 bytes --]

Hi again Maxime,

What do you think of my proposal?  Do any other maintainers care to chime
in here?

- Dave

On Tue, Aug 23, 2022 at 10:45 AM Thompson, David <dthompson2@worcester.edu>
wrote:

> Hi Maxime,
>
> On Tue, Aug 23, 2022, 8:41 AM Maxime Devos <maximedevos@telenet.be> wrote:
>
>>
>> During "guix system reconfigure", there is now window where the
>> directory temporarily has incorrect bits and hence if gitolite is
>> restarted during that time it will presumably fail.  Could a
>> 'home-permission-bits' or such field be added instead to <user-account>
>> to make things atomic?
>>
>
> That would be a nice improvement to backlog now that such a use case has
> emerged. However, I think for our immediate needs this one line patch,
> while imperfect, solves a longstanding problem adequately. So how about
> merging it, closing this bug, and opening a new bug for the system level
> improvement?
>
> - Dave
>

[-- Attachment #2: Type: text/html, Size: 1778 bytes --]

bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 432 bytes --]


On 29-08-2022 14:49, Thompson, David wrote:
> Hi again Maxime,
>
> What do you think of my proposal?  Do any other maintainers care to 
> chime in here?
>
> - Dave

Backlogged thing have a tendency to be backlogged indefinitely, and my 
proposal for a home-permissions-bits seems straightforward and simple to 
me, so I would rather not trade a bug for another bug but rather do a 
proper fix.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 785 bytes --]

Hi Maxime,

I disagree.  I believe we shouldn't let perfect be the enemy of the good.
I haven't sent patches to Guix in quite some time, but I've never felt
roadblocked like this and it is concerning to me.

Can any other maintainers please chime in here?

- Dave

On Mon, Aug 29, 2022 at 8:52 AM Maxime Devos <maximedevos@telenet.be> wrote:

>
> On 29-08-2022 14:49, Thompson, David wrote:
> > Hi again Maxime,
> >
> > What do you think of my proposal?  Do any other maintainers care to
> > chime in here?
> >
> > - Dave
>
> Backlogged thing have a tendency to be backlogged indefinitely, and my
> proposal for a home-permissions-bits seems straightforward and simple to
> me, so I would rather not trade a bug for another bug but rather do a
> proper fix.
>
> Greetings,
> Maxime.
>

[-- Attachment #2: Type: text/html, Size: 1249 bytes --]

bug#56444: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1153 bytes --]


On 29-08-2022 14:57, Thompson, David wrote:
> Hi Maxime,
>
> I disagree.  I believe we shouldn't let perfect be the enemy of the 
> good.  I haven't sent patches to Guix in quite some time, but I've 
> never felt roadblocked like this and it is concerning to me.

It's almost trivial to implement 'the perfect' here, almost no more 
effort than your partial solution; there is no "perfect enemy of the 
good" situation here -- "perfect enemy of the good" only applies when 
"the perfect" is significantly harder / more effort than "the good", but 
that's not the case here.

Given that a proper fix is very easy, simple and low-effort and 
furthermore, it is even known what form the proper fix would take (see: 
extra field, + adjust procedure in (gnu build activation) slightly), 
there aren't any roadblocks except for an apparent refusal by you to 
invest a little extra effort.

If you genuinely find it actually hard to implement, please tell so and 
I can give you some pointers on what procedures appear to be need to be 
modified. Currently, your response appears to be made in bad faith t me.

Greetings,
Maxime


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 324 bytes --]


On 29-08-2022 14:57, Thompson, David wrote:
> [...]
> Can any other maintainers please chime in here?

To correct a misunderstanding, I'm not a maintainer, at least if you 
meant "maintainer" in the same sense as used at 
<https://guix.gnu.org/en/blog/2022/gnu-guix-maintainer-rotation/>.

Greetings,
Maxime



[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 659 bytes --]

On 29-08-2022 14:57, Thompson, David wrote:

> I disagree.  I believe we shouldn't let perfect be the enemy of the good.

I don't think your patch counts as "good" here -- while fixing the bug 
counts as "good", you are at the same time introducing a new bug (the 
non-atomicity), which is bad.  You would have to weigh the goodness and 
the badness to end up with an overall "good" (or maybe "bad", depending 
on the conclusion), but I'd think that the time required to do such a 
weighing is better spent by doing a tiny bit of extra effort to 
implement the new field (it should be very low effort, see other response).

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: [EXT] Re: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 1823 bytes --]

On Mon, Aug 29, 2022 at 9:19 AM Maxime Devos <maximedevos@telenet.be> wrote:

> On 29-08-2022 14:57, Thompson, David wrote:
>
> > I disagree.  I believe we shouldn't let perfect be the enemy of the good.
>
> I don't think your patch counts as "good" here -- while fixing the bug
> counts as "good", you are at the same time introducing a new bug (the
> non-atomicity), which is bad.  You would have to weigh the goodness and
> the badness to end up with an overall "good" (or maybe "bad", depending
> on the conclusion), but I'd think that the time required to do such a
> weighing is better spent by doing a tiny bit of extra effort to
> implement the new field (it should be very low effort, see other response).
>

My patch has a very limited scope of only changing the gitolite service.
Your proposal has a much greater scope of modifying a core structure used
for system configuration.  The new bug you mention is only bad in a
theoretical sense.  In practice, the permission bits are misconfigured for
a blip of time during system reconfiguration, which is a lot better than
being misconfigured all the time which is the status quo.  It's the
difference between a gitolite that works nicely with cgit/gitweb and one
that doesn't. I agree that it's a good goal to improve atomicity and I
think making <user-account> more general to allow for different permission
bits on the home directory is a good idea, but I see it as one step removed
from fixing this particular bug.  My patch follows the recommended approach
outlined in a comment in (gnu build activation) written by Ludovic in 2019:

      ;; Always set ownership and permissions for home directories of system
      ;; accounts.  If a service needs looser permissions on its home
      ;; directories, it can always chmod it in an activation snippet.

- Dave

[-- Attachment #2: Type: text/html, Size: 2321 bytes --]

bug#56444: [EXT] Re: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Maxime Devos]

[-- Attachment #1.1.1.1: Type: text/plain, Size: 3038 bytes --]


On 29-08-2022 15:30, Thompson, David wrote:
>
> On Mon, Aug 29, 2022 at 9:19 AM Maxime Devos <maximedevos@telenet.be> 
> wrote:
>
>     On 29-08-2022 14:57, Thompson, David wrote:
>
>     > I disagree.  I believe we shouldn't let perfect be the enemy of
>     the good.
>
>     I don't think your patch counts as "good" here -- while fixing the
>     bug
>     counts as "good", you are at the same time introducing a new bug (the
>     non-atomicity), which is bad.  You would have to weigh the
>     goodness and
>     the badness to end up with an overall "good" (or maybe "bad",
>     depending
>     on the conclusion), but I'd think that the time required to do such a
>     weighing is better spent by doing a tiny bit of extra effort to
>     implement the new field (it should be very low effort, see other
>     response).
>
>
> My patch has a very limited scope of only changing the gitolite 
> service.  Your proposal has a much greater scope of modifying a core 
> structure used for system configuration.
It is a greater scope, but it's not really more effort.
> The new bug you mention is only bad in a theoretical sense.  In 
> practice, the permission bits are misconfigured for a blip of time 
> during system reconfiguration, which is a lot better than being 
> misconfigured all the time which is the status quo.  It's the 
> difference between a gitolite that works nicely with cgit/gitweb and 
> one that doesn't. I agree that it's a good goal to improve atomicity 
> and I think making <user-account> more general to allow for different 
> permission bits on the home directory is a good idea, but I see it as 
> one step removed from fixing this particular bug.

The time required to analyse it as "just theoretical" could have been 
spent doing the tiny bit of extra effort.

Theoretical bugs like these are especially nasty, if you encounter them 
there is often not a clue what the cause is unless you already know what 
to look for.

>   My patch follows the recommended approach outlined in a comment in 
> (gnu build activation) written by Ludovic in 2019:
>
>       ;; Always set ownership and permissions for home directories of 
> system
>       ;; accounts.  If a service needs looser permissions on its home
>       ;; directories, it can always chmod it in an activation snippet.

I've refuted that recommendation (albeit without explicitly mentioning 
that paragraph), that paragraph is a bug, see my previous comments on 
non-atomicity. Please remove it in the v2 patch.

As there appears to be a lack of willingness to invest the tiniest bit 
of extra effort to implement a proper patch, and given the length of 
previous discussion, I think my time will be better spent continuing 
fixing things in Guix rather than any failing attempts at convincing 
you. As such, I'll stop responding until a v2 or questions on how to 
implement a v2, but that cannot be interpreted as me agreeing with you.

Greetings,
Maxime


[-- Attachment #1.1.1.2: Type: text/html, Size: 5064 bytes --]

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

bug#56444: [EXT] Re: [EXT] Re: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 1288 bytes --]

On Mon, Aug 29, 2022 at 9:44 AM Maxime Devos <maximedevos@telenet.be> wrote:

> As there appears to be a lack of willingness to invest the tiniest bit of
> extra effort to implement a proper patch, and given the length of previous
> discussion, I think my time will be better spent continuing fixing things
> in Guix rather than any failing attempts at convincing you. As such, I'll
> stop responding until a v2 or questions on how to implement a v2, but that
> cannot be interpreted as me agreeing with you.
>

From my perspective, there is a lack of willingness on your end to accept
imperfect solutions that have low impact.  In projects I maintain and in
the professional world, I try to acknowledge the work someone has already
done and accept patches/pull requests that are not ideal but solve real
world problems.  I'd be happy to do some follow-up work to make
system-level improvements when I have the time to properly test a change
that impacts literally everyone that uses the Guix distro, but it would
have been great to have improved the gitolite service in the meantime.
If/when I get around to doing this work, I'll send along a patch.

To any other maintainer or core dev: If any of you wants to sign off on
this patch as-is, I'll merge it.  I have commit access.

- Dave

[-- Attachment #2: Type: text/html, Size: 1748 bytes --]

bug#56444: [EXT] Re: [EXT] Re: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions

[zimoun]

Hi David,

On lun., 29 août 2022 at 09:59, "Thompson, David" <dthompson2@worcester.edu> wrote:

> To any other maintainer or core dev: If any of you wants to sign off on
> this patch as-is, I'll merge it.  I have commit access.

I am not maintainer, Maxime neither AFAIK, and I am not a “core dev“
neither.  For what it is worth, your patch LGTM; please push.

The patch seems a pragmatic workaround waiting an implementation more
robust as Maxime has proposed.  Feel free to send here or open another
submission for this upcoming “better” fix.

BTW, unrelated but Gitolite in Guix requires some love.  If you have
time, a look at bug#25957 [1] would be appreciated. :-)

Cheers,
simon

1: <http://issues.guix.gnu.org/issue/25957>

bug#56444: Gitolite home directory permissions

[Ludovic Courtès]

Hi there!

Please let’s avoid guessing each other’s willingness to do one thing or
another.

I agree with David that we should accept simple local fixes like this
one, while keeping the “better solution” in sight.  It’s a tradeoff, and
the goal is to make sure we can all move forward.

So I’m all for merging this Gitolite activation patch that David posted
right away; I think you can go ahead, David.

Adding ‘home-permission’ to <user-account> as Maxime suggested also
sounds like a welcome improvement to me, but I think it’s fine to do
that separately.

Thanks,
Ludo’.

bug#56444: [EXT] Re: bug#56444: Gitolite home directory permissions

[Thompson, David]

[-- Attachment #1: Type: text/plain, Size: 981 bytes --]

Hi Ludo,

On Tue, Aug 30, 2022 at 11:20 AM Ludovic Courtès <ludo@gnu.org> wrote:

> Hi there!
>
> Please let’s avoid guessing each other’s willingness to do one thing or
> another.
>
> I agree with David that we should accept simple local fixes like this
> one, while keeping the “better solution” in sight.  It’s a tradeoff, and
> the goal is to make sure we can all move forward.
>
> So I’m all for merging this Gitolite activation patch that David posted
> right away; I think you can go ahead, David.
>
> Adding ‘home-permission’ to <user-account> as Maxime suggested also
> sounds like a welcome improvement to me, but I think it’s fine to do
> that separately.
>

Patch pushed.

I will follow up with a new bug report (and a patch later when I have some
time to actually write code) to capture the improvements to <user-account>
so we can discuss any potential issues or gotchas that might come as a
result.

Thanks,

- Dave

[-- Attachment #2: Type: text/html, Size: 1409 bytes --]

bug#56444: Gitolite home directory permissions

[david larsson]

On 2022-08-30 17:20, Ludovic Courtès wrote:

> I agree with David that we should accept simple local fixes like this
> one, while keeping the “better solution” in sight.  It’s a tradeoff, 
> and
> the goal is to make sure we can all move forward.

FWIW: I think that writing comments like ;; KLUDGE: better to do X. etc. 
is a simple way to keep things in sight, and can or should be added 
before pushing patches when so is relevant. For those using emacs then 
with emacs-magit-todos you get all such TODO-things visible every time 
when checking ma(git) status from inside emacs., which is nice IMO.

Regards,
David Larsson