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

  • Done
  • quality assurance status badge
Details
2 participants
  • Maxime Devos
  • Tobias Geerinckx-Rice
Owner
unassigned
Submitted by
Tobias Geerinckx-Rice
Severity
normal

Debbugs page

Tobias Geerinckx-Rice wrote 3 years ago
(address . guix-patches@gnu.org)
20220605000425.20480-1-me@tobias.gr
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(+)

Toggle diff (46 lines)
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
Tobias Geerinckx-Rice wrote 3 years ago
(address . 55892@debbugs.gnu.org)
87leu4ijk1@nckx
Toggle quote (4 lines)
> (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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYqNtfg0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15ajcBALbCIztGKZ9E/FWQaO81zpPyKmZFxsp/Hh0CQJWt
Si4CAQD5Rl5foPMpVbMA6u/CxZnEjRjOiqy5P5S5ZdgFz612Bg==
=E0Hy
-----END PGP SIGNATURE-----

Maxime Devos wrote 3 years ago
Re: [bug#55892] [PATCH] pull: Fail if cache directory ownership is suspect.
9ed7f25f4748f52ce1d52ac14f651c366f6b5b36.camel@telenet.be
Tobias Geerinckx-Rice via Guix-patches via schreef op zo 05-06-2022 om
02:04 [+0200]:
Toggle quote (14 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqO90BccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7otHAQCsenGfmkTOhTLk83p0s3qqlzFA
nOOO5/2htUxQc1EEWAD6A5X/c68GuLki69Dh+sU/GmnSA6i1GV/uXcjyYHLoQgI=
=lRN+
-----END PGP SIGNATURE-----


Tobias Geerinckx-Rice wrote 3 years ago
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 55892-done@debbugs.gnu.org)
87r13wq62s@nckx
Maxime,

Thanks for the swift review!

Maxime Devos 写道:
Toggle quote (4 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYqP/uw0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW150XEBAItfbEgm9fOOprlxaGdWmJtYOSz6uorVX8hlOceu
gu0sAP4wH9yBETmOMDZ3Dqn+qnOUWQYFBmMOKZAPnMBDgW1gAA==
=O6Wt
-----END PGP SIGNATURE-----

Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 55892@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 55892
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help