Gitolite home directory permissions

  • Done
  • quality assurance status badge
Details
6 participants
  • david larsson
  • Thompson, David
  • Evgeny Pisemsky
  • Ludovic Courtès
  • Maxime Devos
  • zimoun
Owner
unassigned
Submitted by
Evgeny Pisemsky
Severity
normal
E
E
Evgeny Pisemsky wrote on 7 Jul 2022 23:35
(address . bug-guix@gnu.org)
87zghkehdd.fsf@pisemsky.com
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.
E
E
Evgeny Pisemsky wrote on 8 Jul 2022 10:10
Re: bug#56444: Acknowledgement (Gitolite home directory permissions)
(address . 56444@debbugs.gnu.org)
87ilo8ja7x.fsf@pisemsky.com
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.
T
T
Thompson, David wrote on 19 Aug 2022 15:32
Patch to fix Gitolite home directory permissions
(address . 56444@debbugs.gnu.org)
CAJ=RwfbfUSX2zzOxSFPhzd1yZQREFqi_1PHh_=HX0VufNrLNAw@mail.gmail.com
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: file
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.


* 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(+)

Toggle diff (21 lines)
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
M
M
Maxime Devos wrote on 23 Aug 2022 14:41
e6b3c90c-0825-bf7a-8993-43f053af062f@telenet.be
On 19-08-2022 15:32, Thompson, David wrote:
Toggle quote (16 lines)
> 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: OpenPGP_signature
T
T
Thompson, David wrote on 23 Aug 2022 16:45
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56444@debbugs.gnu.org)
CAJ=RwfaQvY=AT+_sDwGy3tkPEkekGt-djEjPuztFb+4QuYpFZg@mail.gmail.com
Hi Maxime,

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

Toggle quote (8 lines)
>
> 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

Toggle quote (1 lines)
>
Attachment: file
T
T
Thompson, David wrote on 29 Aug 2022 14:49
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56444@debbugs.gnu.org)
CAJ=RwfYw+uoxcjUW_AyHmYnyrUOSCw-3uNs8peopF1skgFRZxg@mail.gmail.com
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:

Toggle quote (20 lines)
> 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: file
M
M
Maxime Devos wrote on 29 Aug 2022 14:52
(name . Thompson, David)(address . dthompson2@worcester.edu)(address . 56444@debbugs.gnu.org)
1967478a-3d68-a0c7-3a87-49762502232b@telenet.be
On 29-08-2022 14:49, Thompson, David wrote:
Toggle quote (6 lines)
> 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: OpenPGP_signature
T
T
Thompson, David wrote on 29 Aug 2022 14:57
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56444@debbugs.gnu.org)
CAJ=Rwfax4r3281OLMOp7xGg1-f4DSFHRsGT+qRHm_Bu82NJ2tg@mail.gmail.com
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:

Toggle quote (17 lines)
>
> 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: file
M
M
Maxime Devos wrote on 29 Aug 2022 15:09
(name . Thompson, David)(address . dthompson2@worcester.edu)(address . 56444@debbugs.gnu.org)
6f2e7dc5-ec09-b75d-24de-a7858cbd4b4b@telenet.be
On 29-08-2022 14:57, Thompson, David wrote:
Toggle quote (5 lines)
> 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: OpenPGP_signature
M
M
Maxime Devos wrote on 29 Aug 2022 15:11
(name . Thompson, David)(address . dthompson2@worcester.edu)(address . 56444@debbugs.gnu.org)
1178eeff-1c98-ebbc-3b29-95dc49debe33@telenet.be
On 29-08-2022 14:57, Thompson, David wrote:
Toggle quote (2 lines)
> [...]
> 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
Greetings,
Maxime
Attachment: OpenPGP_signature
M
M
Maxime Devos wrote on 29 Aug 2022 15:19
(name . Thompson, David)(address . dthompson2@worcester.edu)(address . 56444@debbugs.gnu.org)
31e8c6df-a9c9-7f16-69fb-e022d94c16ff@telenet.be
On 29-08-2022 14:57, Thompson, David wrote:
Toggle quote (1 lines)
> 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: OpenPGP_signature
T
T
Thompson, David wrote on 29 Aug 2022 15:30
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56444@debbugs.gnu.org)
CAJ=RwfZz7U3k9CaxkpjSQqcDcMRGXx4BQNwnVh_Mv1_AVnpPFg@mail.gmail.com
On Mon, Aug 29, 2022 at 9:19 AM Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (13 lines)
> 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: file
M
M
Maxime Devos wrote on 29 Aug 2022 15:44
(name . Thompson, David)(address . dthompson2@worcester.edu)(address . 56444@debbugs.gnu.org)
6229fb3c-5966-aa18-d691-3d31c7335315@telenet.be
On 29-08-2022 15:30, Thompson, David wrote:
Toggle quote (25 lines)
>
> 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.
Toggle quote (9 lines)
> 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.
Toggle quote (7 lines)
>   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: file
Attachment: OpenPGP_signature
T
T
Thompson, David wrote on 29 Aug 2022 15:59
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 56444@debbugs.gnu.org)
CAJ=Rwfb-DaJ1qTT6z5smQc6byWRPoSfodVjTcX-Ap8AuWgzZTg@mail.gmail.com
On Mon, Aug 29, 2022 at 9:44 AM Maxime Devos <maximedevos@telenet.be> wrote:

Toggle quote (8 lines)
> 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: file
Z
Z
zimoun wrote on 29 Aug 2022 23:05
Re: bug#56444: [EXT] Re: [EXT] Re: [EXT] Re: bug#56444: Patch to fix Gitolite home directory permissions
(address . 56444@debbugs.gnu.org)
87ilmavjyg.fsf@gmail.com
Hi David,

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

Toggle quote (3 lines)
> 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

L
L
Ludovic Courtès wrote on 30 Aug 2022 17:20
Re: bug#56444: Gitolite home directory permissions
(name . Thompson, David)(address . dthompson2@worcester.edu)
87edwxwyf9.fsf_-_@gnu.org
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’.
T
T
Thompson, David wrote on 30 Aug 2022 18:39
(name . Ludovic Courtès)(address . ludo@gnu.org)
CAJ=Rwfa4Yskk=gzV2HepozfKWRBCE9MVzhhOwVqZ3NrEqFm+Fw@mail.gmail.com
Hi Ludo,

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

Toggle quote (17 lines)
> 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: file
D
D
david larsson wrote on 30 Aug 2022 20:31
Re: bug#56444: Gitolite home directory permissions
(name . Ludovic Courtès)(address . ludo@gnu.org)
4cab77dc14a410c8a4b8252b72edb3ec@selfhosted.xyz
On 2022-08-30 17:20, Ludovic Courtès wrote:

Toggle quote (5 lines)
> 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
?