[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Wojtek Kosior via Guix-patches via]

Python applications used to prioritize loading their libraries from so-called
"user site dir" (usually in ~/.local/lib/python<VERSION>/site-packages). The
libraries would only be loaded from /gnu/store when not found in the user site
dir. This used to cause hard-to-diagnose bugs like [1] when a user happened to
have a similar but incompatible version of a library installed via pip.

These patches modify the python-build-system's procedure responsible for
wrapping executables. The modified proc defines a PYTHONNOUSERSITE variable
which makes Python applications disregard the user site dir when loading
libraries.

While this solution does harden most Python applications, it can also break a
few ones like pip that operate on the user site dir itself. To work around
that, the second patch introduces a change to pip to allow installing to the
user site directory even when PYTHONNOUSERSITE is set by the Guix-created
wrapper script.

The third patch adds a boolean argument called disable-user-site? to
python-build-system. Packagers can set this argument to #f on per-package
basis to disable the hardening behavior in case it breaks some
application. Note that in the long run, it might be beneficial (although more
time-consuming) to leave disable-user-site? as #t everywhere and instead
modify the problematic applications — as done here with python-pip. It might
even be practical to only merge the first 2 patches from this series.

Please note that virtualenvs and packages that operate on them are likely
unaffected by this change. The initial bug doesn't even occur with
virtualenvs.


I tested the changes with

    ./pre-inst-env guix shell -C --network --no-cwd python-xmldiff coreutils python-pip
    pip install xmldiff==2.4
    echo > ~/.local/lib/python3.10/site-packages/xmldiff/main.py
    xmldiff --help

Without any patches, the 4th line fails. With the patches applied, the 4th
line succeeds and prints xmldiff's usage info


[1] https://issues.guix.gnu.org/63912


Wojtek Kosior (3):
  guix: build: python-build-system: Don't process user site dir
  gnu: python-pip: Enable user site even with PYTHONNOUSERSITE
  guix: build: python-build-system: Honor disable-user-site? argument

 gnu/packages/python-build.scm      | 10 +++++++++-
 guix/build-system/python.scm       |  2 ++
 guix/build/python-build-system.scm | 27 ++++++++++++++++++---------
 3 files changed, 29 insertions(+), 10 deletions(-)


base-commit: 67e22584faaa558c2a5834a5013d77660ec45e85
-- 
2.40.1

[bug#64573] [PATCH 1/3] guix: build: python-build-system: Don't process user site dir

[Wojtek Kosior via Guix-patches via]

* guix/build/python-build-system.scm (wrap): Define PYTHONNOUSERSITE for
programs so they don't incorrectly pick up local, pip-installed libraries.
---
 guix/build/python-build-system.scm | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index aa04664b25..93aafc4aa9 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -241,12 +241,16 @@ (define* (wrap #:key inputs outputs #:allow-other-keys)
   (define %sh (delay (search-input-file inputs "bin/bash")))
   (define (sh) (force %sh))
 
-  (let* ((var `("GUIX_PYTHONPATH" prefix
-                ,(search-path-as-string->list
-                  (or (getenv "GUIX_PYTHONPATH") "")))))
+  (let* ((var-pythonpath `("GUIX_PYTHONPATH" prefix
+                           ,(search-path-as-string->list
+                             (or (getenv "GUIX_PYTHONPATH") ""))))
+         ;; Harden applications by preventing Python from automatically
+         ;; picking up libraries in user site directory.
+         (var-usersite '("PYTHONNOUSERSITE" = ("GUIX_WRAPPER"))))
     (for-each (lambda (dir)
                 (let ((files (list-of-files dir)))
-                  (for-each (cut wrap-program <> #:sh (sh) var)
+                  (for-each (cut wrap-program <> #:sh (sh)
+                                 var-pythonpath var-usersite)
                             files)))
               bindirs)))
 
-- 
2.40.1

[bug#64573] [PATCH 2/3] gnu: python-pip: Enable user site even with PYTHONNOUSERSITE

[Wojtek Kosior via Guix-patches via]

* gnu/packages/python-build.scm (python-pip): Patch pip to allow installing to
user site dir when PYTHONNOUSERSITE is set by Guix wrapper script to
'GUIX_WRAPPER' string.
---
 gnu/packages/python-build.scm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gnu/packages/python-build.scm b/gnu/packages/python-build.scm
index 154c97e9e4..54d12f3fdc 100644
--- a/gnu/packages/python-build.scm
+++ b/gnu/packages/python-build.scm
@@ -269,7 +269,15 @@ (define-public python-pip
          "0jnk639v9h7ghslm4jnlic6rj3v29nygflx1hgxxndg5gs4kk1a0"))))
     (build-system python-build-system)
     (arguments
-     '(#:tests? #f))          ; there are no tests in the pypi archive.
+     `(#:tests? #f            ;there are no tests in the pypi archive
+       #:phases
+       (modify-phases %standard-phases
+         (add-after 'unpack 'allow-installing-to-user-site
+           (lambda _
+             (substitute* "src/pip/_internal/commands/install.py"
+               (("( *if not site\\.ENABLE_USER_SITE):" match if-clause)
+                (string-append if-clause
+                               " and not os.environ['PYTHONNOUSERSITE'] == 'GUIX_WRAPPER':"))))))))
     (home-page "https://pip.pypa.io/")
     (synopsis "Package manager for Python software")
     (description
-- 
2.40.1

[bug#64573] [PATCH 3/3] guix: build: python-build-system: Honor disable-user-site? argument

[Wojtek Kosior via Guix-patches via]

* guix/build/python-build-system.scm (wrap): Only define the PYTHONNOUSERSITE
wrapper variable if keyword argument disable-user-site? evaluates to true.
* guix/build-system/python.scm (python-build): Pass disable-user-site?
argument to the build side with the default of #t.
---
 guix/build-system/python.scm       |  2 ++
 guix/build/python-build-system.scm | 31 +++++++++++++++++-------------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/guix/build-system/python.scm b/guix/build-system/python.scm
index cca009fb28..dd86cbd4bf 100644
--- a/guix/build-system/python.scm
+++ b/guix/build-system/python.scm
@@ -171,6 +171,7 @@ (define* (python-build name inputs
                        (tests? #t)
                        (test-target "test")
                        (use-setuptools? #t)
+                       (disable-user-site? #t)
                        (configure-flags ''())
                        (phases '%standard-phases)
                        (outputs '("out"))
@@ -192,6 +193,7 @@ (define* (python-build name inputs
                               #:source #+source
                               #:configure-flags #$configure-flags
                               #:use-setuptools? #$use-setuptools?
+                              #:disable-user-site? #$disable-user-site?
                               #:system #$system
                               #:test-target #$test-target
                               #:tests? #$tests?
diff --git a/guix/build/python-build-system.scm b/guix/build/python-build-system.scm
index 93aafc4aa9..959d062bb2 100644
--- a/guix/build/python-build-system.scm
+++ b/guix/build/python-build-system.scm
@@ -11,6 +11,7 @@
 ;;; Copyright © 2020 Efraim Flashner <efraim@flashner.co.il>
 ;;; Copyright © 2021 Lars-Dominik Braun <lars@6xq.net>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
+;;; Copyright © 2023 Wojtek Kosior <my-contribution-is-licensed-cc0@koszko.org>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -222,7 +223,7 @@ (define* (install #:key inputs outputs (configure-flags '()) use-setuptools?
       (invoke "python" "-m" "compileall" "--invalidation-mode=unchecked-hash"
               out))))
 
-(define* (wrap #:key inputs outputs #:allow-other-keys)
+(define* (wrap #:key inputs outputs disable-user-site? #:allow-other-keys)
   (define (list-of-files dir)
     (find-files dir (lambda (file stat)
                       (and (eq? 'regular (stat:type stat))
@@ -241,18 +242,22 @@ (define* (wrap #:key inputs outputs #:allow-other-keys)
   (define %sh (delay (search-input-file inputs "bin/bash")))
   (define (sh) (force %sh))
 
-  (let* ((var-pythonpath `("GUIX_PYTHONPATH" prefix
-                           ,(search-path-as-string->list
-                             (or (getenv "GUIX_PYTHONPATH") ""))))
-         ;; Harden applications by preventing Python from automatically
-         ;; picking up libraries in user site directory.
-         (var-usersite '("PYTHONNOUSERSITE" = ("GUIX_WRAPPER"))))
-    (for-each (lambda (dir)
-                (let ((files (list-of-files dir)))
-                  (for-each (cut wrap-program <> #:sh (sh)
-                                 var-pythonpath var-usersite)
-                            files)))
-              bindirs)))
+  (let ((vars (filter identity
+                      `(("GUIX_PYTHONPATH" prefix
+                         ,(search-path-as-string->list
+                           (or (getenv "GUIX_PYTHONPATH") "")))
+                        ;; Harden applications by preventing Python from
+                        ;; automatically picking up libraries in user site
+                        ;; directory.
+                        ,(and disable-user-site?
+                              '("PYTHONNOUSERSITE" = ("GUIX_WRAPPER")))))))
+    (for-each (lambda (var)
+                (for-each (lambda (dir)
+                            (let ((files (list-of-files dir)))
+                              (for-each (cut wrap-program <> #:sh (sh) var)
+                                        files)))
+                          bindirs))
+              vars)))
 
 (define* (rename-pth-file #:key name inputs outputs #:allow-other-keys)
   "Rename easy-install.pth to NAME.pth to avoid conflicts between packages
-- 
2.40.1

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Lars-Dominik Braun]

Hi,

> These patches modify the python-build-system's procedure responsible for
> wrapping executables. The modified proc defines a PYTHONNOUSERSITE variable
> which makes Python applications disregard the user site dir when loading
> libraries.

if we’re patching applications like pip anyways, what stops us from
just setting site.ENABLE_USER_SITE to False globally in Python’s
site.py?

Note that our python package currently (unfortunately) bundles and
exposes pip (through the pip3 command), which would not be affected by
your change to the python-pip package. Also note that we have
*two* build systems for Python right now (python-build-system and
pyproject-build-system) and the new flag disable-user-site? would have
to be added to both, even though they share the wrap phase.

Cheers,
Lars

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Wojtek Kosior via Guix-patches via]

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

Hi, thanks for reviewing the series

> > These patches modify the python-build-system's procedure responsible for
> > wrapping executables. The modified proc defines a PYTHONNOUSERSITE variable
> > which makes Python applications disregard the user site dir when loading
> > libraries.  
> 
> if we’re patching applications like pip anyways, what stops us from
> just setting site.ENABLE_USER_SITE to False globally in Python’s
> site.py?

I think it would need to be set to True, not False, to have the desired
effect on Guix-installed pip application.

However, we want our change to only affect applications installed with
Guix. So that the user could theoretically still do e.g.

    python3 -m pip install --ignore-installed pip
    ~/.local/bin/pip install xmldiff

Rn I don't see a better way to achieve this than patching
python-build-system and applications like pip.

> Note that our python package currently (unfortunately) bundles and
> exposes pip (through the pip3 command), which would not be affected by
> your change to the python-pip package.

I haven't been aware of that, thanks. Fortunately, the bundled pip is
also unaffected by the change to python-build system. So although this
patch series fails to harden it, it doesn't break it either.

> Also note that we have *two* build systems for Python right now
> (python-build-system and pyproject-build-system) and the new flag
> disable-user-site? would have to be added to both, even though they
> share the wrap phase.

Fair point, thanks.

Should I send an updated patch series that also adds this flag to
pyproject-build-system? And should I include a patch that modifies the
python's bundled pip analogously to how I did with the python-pip
package?

Best,
Wojtek

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Lars-Dominik Braun]

Hi,

> I think it would need to be set to True, not False, to have the desired
> effect on Guix-installed pip application.

to clarify, the comment in site.py says

	set it to False to disable the feature or True to force the feature

and my impression was that we want to disable the user site dir by default
(i.e. disable the feature), right?

> However, we want our change to only affect applications installed with
> Guix. So that the user could theoretically still do e.g.
> 
>     python3 -m pip install --ignore-installed pip
>     ~/.local/bin/pip install xmldiff
> 
> Rn I don't see a better way to achieve this than patching
> python-build-system and applications like pip.

I can still `python3 -m pip install` with the explicit `--user`
switch, even when the user site dir is disabled globally via
ENABLE_USER_SITE=False. The only thing that changes is the default
search path. So that library will only be available if I explicitly add
.local/lib/pythonX/site-packages to PYTHONPATH.

Shouldn’t that also solve the original issue of Guix-installed
applications picking up random libraries from the user site dir.

Cheers,
Lars

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Wojtek Kosior via Guix-patches via]

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

Hi again!

> > I think it would need to be set to True, not False, to have the desired
> > effect on Guix-installed pip application.  
> 
> to clarify, the comment in site.py says
> 
> 	set it to False to disable the feature or True to force the feature
> 
> and my impression was that we want to disable the user site dir by default
> (i.e. disable the feature), right?

Oh, you were right. For some reason I previously misunderstood what you
actually wanted to change.

> > However, we want our change to only affect applications installed with
> > Guix. So that the user could theoretically still do e.g.
> > 
> >     python3 -m pip install --ignore-installed pip
> >     ~/.local/bin/pip install xmldiff
> > 
> > Rn I don't see a better way to achieve this than patching
> > python-build-system and applications like pip.  
> 
> I can still `python3 -m pip install` with the explicit `--user`
> switch, even when the user site dir is disabled globally via
> ENABLE_USER_SITE=False. The only thing that changes is the default
> search path. So that library will only be available if I explicitly add
> .local/lib/pythonX/site-packages to PYTHONPATH.

It's useful to know `--user` does the job here.

> Shouldn’t that also solve the original issue of Guix-installed
> applications picking up random libraries from the user site dir.

Yes, it should. I still see some benefits of using PYTHONNOUSERSITE env
var, though.
1. The hardening can be easily disabled for a single application if some
   not yet known need arises[1].
2. The change is limited to just applications — people running
   `python3` shall have it behave just as it used to so far.
3. As a result of 2., there's no need to explicitly add something to
   PYTHONPATH when using the user site dir.

I'm trying to imagine what I'd expect if I were just starting to use
Guix. And I believe there'd be least astonishment if both the user site
dir were working out-of-the-box and the applications were working
independently of what one puts in that dir.


During this discussion one more idea came to mind. There might exist a
different way of solving the problem. I.e. to keep user site dir
enabled, then make
- GUIX_PYTHONPATH take precedence over both user site dir and
  PYTHONPATH whenever a Guix-installed application is launched through
  its wrapper and
- PYTHONPATH with user site dir take precedence over GUIX_PYTHONPATH in
  all other cases.

This probably wouldn't require patching applications like pip. And
would also leave the control over the PYTHONNOUSERSITE variable and the
option it affects to the user. Should I try doing this?


Wojtek


[1] Perhaps with ENABLE_USER_SITE=False this can also be achieved by
    the `-S` flag to Python (although won't this approach be less
    reliable?).


-- (sig_start)
website: https://koszko.org/koszko.html
fingerprint: E972 7060 E3C5 637C 8A4F  4B42 4BC5 221C 5A79 FD1A
follow me on Fediverse: https://friendica.me/profile/koszko/profile

♥ R29kIGlzIHRoZXJlIGFuZCBsb3ZlcyBtZQ== | ÷ c2luIHNlcGFyYXRlZCBtZSBmcm9tIEhpbQ==
✝ YnV0IEplc3VzIGRpZWQgdG8gc2F2ZSBtZQ== | ? U2hhbGwgSSBiZWNvbWUgSGlzIGZyaWVuZD8=
-- (sig_end)


On Tue, 18 Jul 2023 11:41:48 +0200 Lars-Dominik Braun <lars@6xq.net> wrote:

> Hi,
> 
> > I think it would need to be set to True, not False, to have the desired
> > effect on Guix-installed pip application.  
> 
> to clarify, the comment in site.py says
> 
> 	set it to False to disable the feature or True to force the feature
> 
> and my impression was that we want to disable the user site dir by default
> (i.e. disable the feature), right?
> 
> > However, we want our change to only affect applications installed with
> > Guix. So that the user could theoretically still do e.g.
> > 
> >     python3 -m pip install --ignore-installed pip
> >     ~/.local/bin/pip install xmldiff
> > 
> > Rn I don't see a better way to achieve this than patching
> > python-build-system and applications like pip.  
> 
> I can still `python3 -m pip install` with the explicit `--user`
> switch, even when the user site dir is disabled globally via
> ENABLE_USER_SITE=False. The only thing that changes is the default
> search path. So that library will only be available if I explicitly add
> .local/lib/pythonX/site-packages to PYTHONPATH.
> 
> Shouldn’t that also solve the original issue of Guix-installed
> applications picking up random libraries from the user site dir.
> 
> Cheers,
> Lars
> 

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[bug#64573] [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[宋文武 via Guix-patches via]

Wojtek Kosior <koszko@koszko.org> writes:

> Python applications used to prioritize loading their libraries from so-called
> "user site dir" (usually in ~/.local/lib/python<VERSION>/site-packages). The
> libraries would only be loaded from /gnu/store when not found in the user site
> dir. This used to cause hard-to-diagnose bugs like [1] when a user happened to
> have a similar but incompatible version of a library installed via pip.
>
> These patches modify the python-build-system's procedure responsible for
> wrapping executables. The modified proc defines a PYTHONNOUSERSITE variable
> which makes Python applications disregard the user site dir when loading
> libraries.
>
> While this solution does harden most Python applications, it can also break a
> few ones like pip that operate on the user site dir itself. To work around
> that, the second patch introduces a change to pip to allow installing to the
> user site directory even when PYTHONNOUSERSITE is set by the Guix-created
> wrapper script.

Hello, I think we can let pip just break as other distros (eg: ArchLinux
and Debian) with PEP-668.

https://gitlab.archlinux.org/archlinux/packaging/packages/python/-/blob/main/EXTERNALLY-MANAGED
https://pythonspeed.com/articles/externally-managed-environment-pep-668/
https://peps.python.org/pep-0668/#recommendations-for-distros

With usage guide towards virtual environments, guix shell, or pipx
(not packaged yet).

Consider other distros does the same thing, this should be safer.

What do you think?  🤔

bug#64573: [PATCH 0/3] guix: build: python-build-system: Have applications by default ignore non-Guix libraries in user site dir

[Wojtek Kosior via Guix-patches via]

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

> Hello, I think we can let pip just break as other distros (eg: ArchLinux
> and Debian) with PEP-668.
> 
> https://gitlab.archlinux.org/archlinux/packaging/packages/python/-/blob/main/EXTERNALLY-MANAGED
> https://pythonspeed.com/articles/externally-managed-environment-pep-668/
> https://peps.python.org/pep-0668/#recommendations-for-distros
> 
> With usage guide towards virtual environments, guix shell, or pipx
> (not packaged yet).
> 
> Consider other distros does the same thing, this should be safer.
> 
> What do you think?  🤔

You're right, making pip break and recommend pipx seems like the right
thing to do.

I opened a new issue with patches that add python-pipx (haven't done
anything related to the 'EXTERNALLY-MANAGED' file yet, tho).

Thanks,
Wojtek

-- (sig_start)
website: https://koszko.org/koszko.html
fingerprint: E972 7060 E3C5 637C 8A4F  4B42 4BC5 221C 5A79 FD1A
follow me on Fediverse: https://friendica.me/profile/koszko/profile

♥ R29kIGlzIHRoZXJlIGFuZCBsb3ZlcyBtZQ== | ÷ c2luIHNlcGFyYXRlZCBtZSBmcm9tIEhpbQ==
✝ YnV0IEplc3VzIGRpZWQgdG8gc2F2ZSBtZQ== | ? U2hhbGwgSSBiZWNvbWUgSGlzIGZyaWVuZD8=
-- (sig_end)


On Sat, 22 Jul 2023 08:30:04 +0800 宋文武 <iyzsong@envs.net> wrote:

> Wojtek Kosior <koszko@koszko.org> writes:
> 
> > Python applications used to prioritize loading their libraries from so-called
> > "user site dir" (usually in ~/.local/lib/python<VERSION>/site-packages). The
> > libraries would only be loaded from /gnu/store when not found in the user site
> > dir. This used to cause hard-to-diagnose bugs like [1] when a user happened to
> > have a similar but incompatible version of a library installed via pip.
> >
> > These patches modify the python-build-system's procedure responsible for
> > wrapping executables. The modified proc defines a PYTHONNOUSERSITE variable
> > which makes Python applications disregard the user site dir when loading
> > libraries.
> >
> > While this solution does harden most Python applications, it can also break a
> > few ones like pip that operate on the user site dir itself. To work around
> > that, the second patch introduces a change to pip to allow installing to the
> > user site directory even when PYTHONNOUSERSITE is set by the Guix-created
> > wrapper script.  
> 
> Hello, I think we can let pip just break as other distros (eg: ArchLinux
> and Debian) with PEP-668.
> 
> https://gitlab.archlinux.org/archlinux/packaging/packages/python/-/blob/main/EXTERNALLY-MANAGED
> https://pythonspeed.com/articles/externally-managed-environment-pep-668/
> https://peps.python.org/pep-0668/#recommendations-for-distros
> 
> With usage guide towards virtual environments, guix shell, or pipx
> (not packaged yet).
> 
> Consider other distros does the same thing, this should be safer.
> 
> What do you think?  🤔

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 228 bytes --]