[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
T
T
Tobias Geerinckx-Rice wrote on 5 Jun 2022 02:04
(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
T
T
Tobias Geerinckx-Rice wrote on 10 Jun 2022 18:10
(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-----

M
M
Maxime Devos wrote on 10 Jun 2022 23:55
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-----


T
T
Tobias Geerinckx-Rice wrote on 11 Jun 2022 04:26
(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
?