From 9ab09db1ab656e96c68f6d4cce0795fd992a5962 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 8 Nov 2024 12:51:49 -0500 Subject: [PATCH 01/11] Make network cache file permissions inherit from cache directory --- news/news/11012.feature.rst | 1 + src/pip/_internal/network/cache.py | 1 + tests/unit/test_network_cache.py | 18 ++++++++++++++++++ 3 files changed, 20 insertions(+) create mode 100644 news/news/11012.feature.rst diff --git a/news/news/11012.feature.rst b/news/news/11012.feature.rst new file mode 100644 index 00000000000..d51ae7ad818 --- /dev/null +++ b/news/news/11012.feature.rst @@ -0,0 +1 @@ +Network cache file permissions will inherit the permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. \ No newline at end of file diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index 4d0fb545dc2..4fc27f3b01d 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -77,6 +77,7 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) + os.chmod(f.name, os.stat(self.directory).st_mode & 0o777 | 0o600) replace(f.name, path) def set( diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index 6bba8fc670b..10a0665fc6e 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -82,3 +82,21 @@ def test_cache_hashes_are_same(self, cache_tmpdir: Path) -> None: key = "test key" fake_cache = Mock(FileCache, directory=cache.directory, encode=FileCache.encode) assert cache._get_cache_path(key) == FileCache._fn(fake_cache, key) + + @pytest.mark.skipif("sys.platform == 'win32'") + @pytest.mark.parametrize( + "perms, expected_perms", + [ + (0o300, 0o700), + (0o700, 0o700), + (0o777, 0o777) + ] + ) + def test_cache_inherits_perms( + self, cache_tmpdir: Path, perms: int, expected_perms: int + ) -> None: + key="foo" + with chmod(cache_tmpdir, perms): + cache = SafeFileCache(os.fspath(cache_tmpdir)) + cache.set(key, b"bar") + assert (os.stat(cache._get_cache_path(key)).st_mode & 0o777) == expected_perms From 36d8d621b17d8c6c4fe06f1068e4c0572ba30f78 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 8 Nov 2024 13:49:42 -0500 Subject: [PATCH 02/11] Fix news file to have correct path --- news/{news => }/11012.feature.rst | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename news/{news => }/11012.feature.rst (100%) diff --git a/news/news/11012.feature.rst b/news/11012.feature.rst similarity index 100% rename from news/news/11012.feature.rst rename to news/11012.feature.rst From 2f7d57d133dcec40b98cde966f33adb31f7b8617 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 8 Nov 2024 13:54:59 -0500 Subject: [PATCH 03/11] Fix pre-commit checks --- news/11012.feature.rst | 2 +- tests/unit/test_network_cache.py | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/news/11012.feature.rst b/news/11012.feature.rst index d51ae7ad818..9aa86bb053e 100644 --- a/news/11012.feature.rst +++ b/news/11012.feature.rst @@ -1 +1 @@ -Network cache file permissions will inherit the permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. \ No newline at end of file +Network cache file permissions will inherit the permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index 10a0665fc6e..2ad903bcb83 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -85,17 +85,12 @@ def test_cache_hashes_are_same(self, cache_tmpdir: Path) -> None: @pytest.mark.skipif("sys.platform == 'win32'") @pytest.mark.parametrize( - "perms, expected_perms", - [ - (0o300, 0o700), - (0o700, 0o700), - (0o777, 0o777) - ] + "perms, expected_perms", [(0o300, 0o700), (0o700, 0o700), (0o777, 0o777)] ) def test_cache_inherits_perms( self, cache_tmpdir: Path, perms: int, expected_perms: int ) -> None: - key="foo" + key = "foo" with chmod(cache_tmpdir, perms): cache = SafeFileCache(os.fspath(cache_tmpdir)) cache.set(key, b"bar") From 976c1ad53330422c6d587a932644a2cca3cc1ba5 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 8 Nov 2024 16:46:29 -0500 Subject: [PATCH 04/11] Only preserve the read/write permissions of the cache's directory --- news/11012.feature.rst | 2 +- src/pip/_internal/network/cache.py | 2 +- tests/unit/test_network_cache.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/news/11012.feature.rst b/news/11012.feature.rst index 9aa86bb053e..f02f7ba8c86 100644 --- a/news/11012.feature.rst +++ b/news/11012.feature.rst @@ -1 +1 @@ -Network cache file permissions will inherit the permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. +Network cache file permissions will inherit the read/write permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index 4fc27f3b01d..eaa51141834 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -77,7 +77,7 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) - os.chmod(f.name, os.stat(self.directory).st_mode & 0o777 | 0o600) + os.chmod(f.name, os.stat(self.directory).st_mode & 0o666 | 0o600) replace(f.name, path) def set( diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index 2ad903bcb83..04d9f5516a4 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -85,7 +85,7 @@ def test_cache_hashes_are_same(self, cache_tmpdir: Path) -> None: @pytest.mark.skipif("sys.platform == 'win32'") @pytest.mark.parametrize( - "perms, expected_perms", [(0o300, 0o700), (0o700, 0o700), (0o777, 0o777)] + "perms, expected_perms", [(0o300, 0o600), (0o700, 0o600), (0o777, 0o666)] ) def test_cache_inherits_perms( self, cache_tmpdir: Path, perms: int, expected_perms: int From 13f2e810e0aa64fc6573d01ae468fe160c3cf85f Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 8 Nov 2024 16:47:26 -0500 Subject: [PATCH 05/11] Add comment explaining the bitwise operations in the context of Unix permissions --- src/pip/_internal/network/cache.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index eaa51141834..40e6bb9b14f 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -77,6 +77,8 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) + # `& 0o666` selects the read/write permissions of the directory. + # `| 0o600` sets owner read/write permissions. os.chmod(f.name, os.stat(self.directory).st_mode & 0o666 | 0o600) replace(f.name, path) From d16bf1631471715b67fbb9f5d586d8ef3ca51169 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Sat, 9 Nov 2024 23:45:08 -0500 Subject: [PATCH 06/11] Clarify comment referencing cache directory --- src/pip/_internal/network/cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index 40e6bb9b14f..a4e87d3847f 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -77,7 +77,7 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) - # `& 0o666` selects the read/write permissions of the directory. + # `& 0o666` selects the read/write permissions of the cache directory. # `| 0o600` sets owner read/write permissions. os.chmod(f.name, os.stat(self.directory).st_mode & 0o666 | 0o600) replace(f.name, path) From c0f5b0a3e7715ab993ad5f886b265e503f88214f Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 15 Nov 2024 08:21:32 -0500 Subject: [PATCH 07/11] Guard against symlink related attack --- src/pip/_internal/network/cache.py | 18 +++++++++++++++--- tests/unit/test_network_cache.py | 18 +++++++++++++++++- 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index a4e87d3847f..938942cf86d 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -77,9 +77,21 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) - # `& 0o666` selects the read/write permissions of the cache directory. - # `| 0o600` sets owner read/write permissions. - os.chmod(f.name, os.stat(self.directory).st_mode & 0o666 | 0o600) + # Change permissions only if there is no risk of following a symlink + if ( + os.chmod in os.supports_fd + or os.chmod in os.supports_follow_symlinks + ): + os.chmod( + path=f.fileno() if os.chmod in os.supports_fd else f.name, + mode=( + os.stat(self.directory).st_mode + & 0o666 # select read/write permissions of cache directory + | 0o600 # set owner read/write permissions + ), + follow_symlinks=os.chmod not in os.supports_follow_symlinks, + ) + replace(f.name, path) def set( diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index 04d9f5516a4..bd4fb1618b0 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -84,10 +84,14 @@ def test_cache_hashes_are_same(self, cache_tmpdir: Path) -> None: assert cache._get_cache_path(key) == FileCache._fn(fake_cache, key) @pytest.mark.skipif("sys.platform == 'win32'") + @pytest.mark.skipif( + os.chmod not in os.supports_fd and os.chmod not in os.supports_follow_symlinks, + reason="requires os.chmod to support file descriptors or not follow symlinks", + ) @pytest.mark.parametrize( "perms, expected_perms", [(0o300, 0o600), (0o700, 0o600), (0o777, 0o666)] ) - def test_cache_inherits_perms( + def test_cache_inherit_perms( self, cache_tmpdir: Path, perms: int, expected_perms: int ) -> None: key = "foo" @@ -95,3 +99,15 @@ def test_cache_inherits_perms( cache = SafeFileCache(os.fspath(cache_tmpdir)) cache.set(key, b"bar") assert (os.stat(cache._get_cache_path(key)).st_mode & 0o777) == expected_perms + + @pytest.mark.skipif("sys.platform == 'win32'") + def test_cache_not_inherit_perms(self, cache_tmpdir: Path, monkeypatch) -> None: + monkeypatch.setattr(os, "supports_fd", os.supports_fd - {os.chmod}) + monkeypatch.setattr( + os, "supports_follow_symlinks", os.supports_follow_symlinks - {os.chmod} + ) + key = "foo" + with chmod(cache_tmpdir, 0o777): + cache = SafeFileCache(os.fspath(cache_tmpdir)) + cache.set(key, b"bar") + assert (os.stat(cache._get_cache_path(key)).st_mode & 0o777) == 0o600 From bcddae3e4f7da96654c72764892642e0d591843d Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 15 Nov 2024 08:28:24 -0500 Subject: [PATCH 08/11] Fix missing type hint --- tests/unit/test_network_cache.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index bd4fb1618b0..e8f6aeeb921 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -101,7 +101,7 @@ def test_cache_inherit_perms( assert (os.stat(cache._get_cache_path(key)).st_mode & 0o777) == expected_perms @pytest.mark.skipif("sys.platform == 'win32'") - def test_cache_not_inherit_perms(self, cache_tmpdir: Path, monkeypatch) -> None: + def test_cache_not_inherit_perms(self, cache_tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(os, "supports_fd", os.supports_fd - {os.chmod}) monkeypatch.setattr( os, "supports_follow_symlinks", os.supports_follow_symlinks - {os.chmod} From 7e9c8ef0de5fa16559be7c0ebb69d517a80b99f5 Mon Sep 17 00:00:00 2001 From: Justin van Heek Date: Fri, 15 Nov 2024 08:30:22 -0500 Subject: [PATCH 09/11] Fix formatting --- tests/unit/test_network_cache.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_network_cache.py b/tests/unit/test_network_cache.py index e8f6aeeb921..af15acae450 100644 --- a/tests/unit/test_network_cache.py +++ b/tests/unit/test_network_cache.py @@ -101,7 +101,9 @@ def test_cache_inherit_perms( assert (os.stat(cache._get_cache_path(key)).st_mode & 0o777) == expected_perms @pytest.mark.skipif("sys.platform == 'win32'") - def test_cache_not_inherit_perms(self, cache_tmpdir: Path, monkeypatch: pytest.MonkeyPatch) -> None: + def test_cache_not_inherit_perms( + self, cache_tmpdir: Path, monkeypatch: pytest.MonkeyPatch + ) -> None: monkeypatch.setattr(os, "supports_fd", os.supports_fd - {os.chmod}) monkeypatch.setattr( os, "supports_follow_symlinks", os.supports_follow_symlinks - {os.chmod} From 0a1f5469234bd6cb1a37772391fc41e9d9368e9e Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 9 Dec 2024 17:05:47 -0500 Subject: [PATCH 10/11] Clean up news fragment and code --- news/11012.feature.rst | 4 +++- src/pip/_internal/network/cache.py | 27 ++++++++++++--------------- 2 files changed, 15 insertions(+), 16 deletions(-) diff --git a/news/11012.feature.rst b/news/11012.feature.rst index f02f7ba8c86..9aa42e90b65 100644 --- a/news/11012.feature.rst +++ b/news/11012.feature.rst @@ -1 +1,3 @@ -Network cache file permissions will inherit the read/write permissions of the cache's directory with the exception of owner read/write permissions which will always be set. This feature enables sharing a cache in a multi-user environment. +Cached files will inherit the read/write permissions of pip's cache directory +(in addition to the current user retaining read/write access). This enables a +single cache to be shared among multiple users. diff --git a/src/pip/_internal/network/cache.py b/src/pip/_internal/network/cache.py index 938942cf86d..fca04e6945f 100644 --- a/src/pip/_internal/network/cache.py +++ b/src/pip/_internal/network/cache.py @@ -76,21 +76,18 @@ def _write(self, path: str, data: bytes) -> None: with adjacent_tmp_file(path) as f: f.write(data) - - # Change permissions only if there is no risk of following a symlink - if ( - os.chmod in os.supports_fd - or os.chmod in os.supports_follow_symlinks - ): - os.chmod( - path=f.fileno() if os.chmod in os.supports_fd else f.name, - mode=( - os.stat(self.directory).st_mode - & 0o666 # select read/write permissions of cache directory - | 0o600 # set owner read/write permissions - ), - follow_symlinks=os.chmod not in os.supports_follow_symlinks, - ) + # Inherit the read/write permissions of the cache directory + # to enable multi-user cache use-cases. + mode = ( + os.stat(self.directory).st_mode + & 0o666 # select read/write permissions of cache directory + | 0o600 # set owner read/write permissions + ) + # Change permissions only if there is no risk of following a symlink. + if os.chmod in os.supports_fd: + os.chmod(f.fileno(), mode) + elif os.chmod in os.supports_follow_symlinks: + os.chmod(f.name, mode, follow_symlinks=False) replace(f.name, path) From b9a41386c3476f1773e2d59f3a48c42d8ec131c5 Mon Sep 17 00:00:00 2001 From: Richard Si Date: Mon, 9 Dec 2024 17:20:35 -0500 Subject: [PATCH 11/11] It's only the network cache --- news/11012.feature.rst | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/news/11012.feature.rst b/news/11012.feature.rst index 9aa42e90b65..d913306c176 100644 --- a/news/11012.feature.rst +++ b/news/11012.feature.rst @@ -1,3 +1,3 @@ -Cached files will inherit the read/write permissions of pip's cache directory -(in addition to the current user retaining read/write access). This enables a -single cache to be shared among multiple users. +Files in the network cache will inherit the read/write permissions of pip's cache +directory (in addition to the current user retaining read/write access). This +enables a single cache to be shared among multiple users.