restrict cache storage directory permissions to owner#2048
Open
sahvx655-wq wants to merge 1 commit into
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While looking at how
filecache/blockcache/simplecachepersist downloaded data, I noticed an inconsistency in the permissions they leave behind. The cache metadata file ends up0o600because it goes throughatomic_write, which creates its temporary viamkstemp, but the cache directory and the cached data files inside it do not.CachingFileSystem.__init__,_mkcacheandSimpleCacheFileSystem.__init__each create the storage directory with a bareos.makedirs(..., exist_ok=True), so under a normal0o022umask the directory comes out0o755and the cached files0o644. Whencache_storagepoints at a persistent location (the documented way to keep a cache between runs), any other local user can traverse the directory and read another user's cached content, which for the http/s3/gcs backends is data that was fetched with that user's credentials.The change sets
mode=0o700on the threeos.makedirscalls that create the cache storage directory, so the directory fsspec owns becomes owner-only and the data files within inherit that protection whichever backend wrote them. Keeping it at the directory-creation point rather than chmod-ing each cached file individually puts the guard in one place and covers all three cache implementations and every write path through them; directories fsspec did not create are left as they are. I have added a parametrised regression test over filecache/simplecache/blockcache that fails on the current tree (the directory is0o755) and passes once the mode is applied.