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

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

[Tobias Geerinckx-Rice via Guix-patches via]

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

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

[Tobias Geerinckx-Rice via Guix-patches via]

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

> (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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

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

[Maxime Devos]

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

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.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

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

[Tobias Geerinckx-Rice via Guix-patches via]

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]