GNU bug report logs - #55892
[PATCH] pull: Fail if cache directory ownership is suspect.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 55892 in the body.
You can then email your comments to 55892 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: guix-patches <at> gnu.org
Subject: [PATCH] pull: Fail if cache directory ownership is suspect.
Date: Sun,  5 Jun 2022 02:04:25 +0200

New users frequently run ‘sudo guix pull’ which breaks subsequent
unprivileged ‘guix pull’s until manually fixed with chmod -R.

* guix/scripts/pull.scm (guix-pull): Fail if the cache directory (or
its innermost extant parent) is not owned by the user pulling the Guix,
with a hint about ‘sudo -i’.
---

Hi Guix,

Another one in the ‘low-level support noise paper-cut’ series.
The XXX comment would not land upstream, I think.

I didn't test this on a foreign distribution.  My understanding is
that distributions where sudo already defaults to ‘-i’ won't throw
the warning nor suffer from the problem.

Kind regards,

T G-R

 guix/scripts/pull.scm | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/guix/scripts/pull.scm b/guix/scripts/pull.scm
index f01764637b..1eaf8f087b 100644
--- a/guix/scripts/pull.scm
+++ b/guix/scripts/pull.scm
@@ -49,6 +49,7 @@ (define-module (guix scripts pull)
   #:autoload   (gnu packages bootstrap) (%bootstrap-guile)
   #:autoload   (gnu packages certs) (le-certs)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
@@ -810,6 +811,31 @@ (define (no-arguments arg _)
         ((assoc-ref opts 'generation)
          (process-generation-change opts profile))
         (else
+         ;; Bail out early when users accidentally run, e.g., ’sudo guix pull’.
+         ;; If CACHE-DIRECTORY doesn't yet exist, test where it would end up.
+         (let-values (((st dir) (let loop ((dir (cache-directory)))
+                                  (let ((st (stat dir #f)))
+                                    (if st
+                                        (values (stat dir #f) dir)
+                                        (loop (dirname dir)))))))
+           (let ((dir:uid (stat:uid st))
+                 (our:uid (getuid)))
+             (unless (= dir:uid our:uid)
+               (let ((our:user (passwd:name (getpwuid our:uid)))
+                     (dir:user (passwd:name (getpwuid dir:uid))))
+                 (raise
+                  (condition
+                   (&message
+                    (message
+                     (format #f (G_ "directory ‘~a’ is not owned by user ~a")
+                             dir dir:user)))
+                   (&fix-hint
+                    (hint
+                     ;; XXX We could check (getenv "SUDO_USER") to display this
+                     ;; only under sudo, but that would imply handling doas… &c.
+                     (format #f (G_ "You should run this command as ~a; use ‘sudo -i’ or equivalent if you really want to pull as ~a.")
+                             dir:user our:user)))))))))
+
          (with-store store
            (with-status-verbosity (assoc-ref opts 'verbosity)
              (parameterize ((%current-system (assoc-ref opts 'system))
-- 
2.36.1

Message #8 received at 55892 <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: 55892 <at> debbugs.gnu.org
Subject: Re: [PATCH] pull: Fail if cache directory ownership is suspect.
Date: Fri, 10 Jun 2022 18:10:51 +0200

[Message part 1 (text/plain, inline)]

> (let ((st (stat dir #f)))
>   (if st
>       (values (stat dir #f) dir)

Grr.  I swear the font used by Mumi has magic typo-highlighting 
properties.  Fixed locally.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Message #11 received at 55892 <at> debbugs.gnu.org (full text, mbox):

From: Maxime Devos <maximedevos <at> telenet.be>
To: Tobias Geerinckx-Rice <me <at> tobias.gr>, 55892 <at> debbugs.gnu.org
Subject: Re: [bug#55892] [PATCH] pull: Fail if cache directory ownership is
 suspect.
Date: Fri, 10 Jun 2022 23:55:28 +0200

[Message part 1 (text/plain, inline)]

Tobias Geerinckx-Rice via Guix-patches via schreef op zo 05-06-2022 om
02:04 [+0200]:
> Hi Guix,
> 
> Another one in the ‘low-level support noise paper-cut’ series.
> The XXX comment would not land upstream, I think.
> 
> I didn't test this on a foreign distribution.  My understanding is
> that distributions where sudo already defaults to ‘-i’ won't throw
> the warning nor suffer from the problem.
> 
> Kind regards,
> 
> T G-R
> 

Concept looks sounds to me!
Nitpick:

+               (let ((our:user (passwd:name (getpwuid our:uid)))
+                     (dir:user (passwd:name (getpwuid dir:uid))))

what if the current user does not have an entry in /etc/passwd or
equivalent?  (E.g. if the user accidentally removed an entry in
/etc/passwd on a foreign system and then runs "guix pull" & "guix shell
THE_EDITOR" to get their favourite editor to edit /etc/passwd back?)

Maybe in that case, it should be reported as NNNN (NNNN = user number)?
Or would that be simply considered unsupported?

Greetings,
Maxime.

[signature.asc (application/pgp-signature, inline)]

Message #16 received at 55892-done <at> debbugs.gnu.org (full text, mbox):

From: Tobias Geerinckx-Rice <me <at> tobias.gr>
To: Maxime Devos <maximedevos <at> telenet.be>
Cc: 55892-done <at> debbugs.gnu.org
Subject: Re: [bug#55892] [PATCH] pull: Fail if cache directory ownership is
 suspect.
Date: Sat, 11 Jun 2022 04:26:34 +0200

[Message part 1 (text/plain, inline)]

Maxime,

Thanks for the swift review!

Maxime Devos 写道：
> Maybe in that case, it should be reported as NNNN (NNNN = user 
> number)?
> Or would that be simply considered unsupported?

Er…  I'd say it's veering confidently into unsupported territory, 
yes.  But falling back to user IDs costs next to nothing so I made 
the change.  Thanks for the suggestion.

Odd feeling that the error message might be more robust than some 
other part of the code now :-)

Pushed as 7c52cad0464175370c44bd4695e4c01a62b8268f.

Kind regards,

T G-R

[signature.asc (application/pgp-signature, inline)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.